diff --git a/public/scripts/macros/engine/MacroEngine.js b/public/scripts/macros/engine/MacroEngine.js index 5d69d45f7..7c7536e02 100644 --- a/public/scripts/macros/engine/MacroEngine.js +++ b/public/scripts/macros/engine/MacroEngine.js @@ -108,9 +108,13 @@ class MacroEngine { * * @param {string} input - The input string to evaluate. * @param {MacroEnv} env - The environment to pass to the macro handler. + * @param {Object} [options={}] - Optional evaluation settings. + * @param {number} [options.contextOffset=0] - Base offset from the original top-level document. + * Used when evaluating nested content (via resolve() in handlers) to preserve global + * positioning for macros like {{pick}} that seed on position. * @returns {string} The resolved string. */ - evaluate(input, env) { + evaluate(input, env, { contextOffset = 0 } = {}) { if (!input) { return ''; } @@ -138,7 +142,7 @@ class MacroEngine { try { evaluated = MacroCstWalker.evaluateDocument({ text: preProcessed, - contextOffset: 0, + contextOffset, cst, env: safeEnv, resolveMacro: this.#resolveMacro.bind(this), diff --git a/public/scripts/macros/engine/MacroRegistry.js b/public/scripts/macros/engine/MacroRegistry.js index 6e64407e5..d69b4259f 100644 --- a/public/scripts/macros/engine/MacroRegistry.js +++ b/public/scripts/macros/engine/MacroRegistry.js @@ -121,7 +121,10 @@ export const MacroValueType = Object.freeze({ * seeding (e.g., in {{pick}}) to ensure identical macros at different positions produce different results. * @property {(value: any) => string} normalize - Normalize function to use on unsure macro results to make sure they return strings as expected. * @property {(content: string, options?: { trimIndent?: boolean }) => string} trimContent - Trims scoped content with optional indentation dedent. Defaults to trimming indentation. - * @property {(text: string) => string} resolve - Evaluates macros in the given text using the same environment. Use when delayArgResolution is true. + * @property {(text: string, options?: { offsetDelta?: number }) => string} resolve - Evaluates macros in the given text using the same environment. + * Use when delayArgResolution is true. By default, preserves the caller's globalOffset so nested + * macros like {{pick}} maintain deterministic position-based behavior. Pass offsetDelta to add + * an additional offset for uniqueness (e.g., to differentiate between multiple resolve calls). */ /** @@ -369,7 +372,9 @@ class MacroRegistry { globalOffset: call.globalOffset, normalize: MacroEngine.normalizeMacroResult.bind(MacroEngine), trimContent: MacroEngine.trimScopedContent.bind(MacroEngine), - resolve: (text) => MacroEngine.evaluate(text, call.env), + resolve: (text, { offsetDelta = 0 } = {}) => MacroEngine.evaluate(text, call.env, { + contextOffset: call.globalOffset + offsetDelta, + }), }; const result = def.handler(executionContext); diff --git a/tests/frontend/MacroEngine.e2e.js b/tests/frontend/MacroEngine.e2e.js index 462a8435e..e736bb574 100644 --- a/tests/frontend/MacroEngine.e2e.js +++ b/tests/frontend/MacroEngine.e2e.js @@ -818,6 +818,59 @@ test.describe('MacroEngine', () => { // Should be one of the valid options expect(['X', 'Y', 'Z'].includes(outputs[0])).toBeTruthy(); }); + + test('should use different seeds for identical picks inside different if blocks (delayArgResolution)', async ({ page }) => { + await registerTestablePick(page); + + // Key regression test: picks inside {{if}} blocks use resolve() which must preserve globalOffset + // This tests the fix for macros with delayArgResolution that call resolve() internally + const output = await page.evaluate(async () => { + /** @type {import('../../public/scripts/macros/engine/MacroEngine.js')} */ + const { MacroEngine } = await import('./scripts/macros/engine/MacroEngine.js'); + /** @type {import('../../public/scripts/macros/engine/MacroEnvBuilder.js')} */ + const { MacroEnvBuilder } = await import('./scripts/macros/engine/MacroEnvBuilder.js'); + + // Two identical pick macros inside different if blocks + // Before the fix, both would get contextOffset=0 when resolve() was called + // After the fix, resolve() passes the caller's globalOffset as contextOffset + const input = '{{if true}}{{testablePick::A::B::C}}{{/if}}###{{if true}}{{testablePick::A::B::C}}{{/if}}'; + const env = MacroEnvBuilder.buildFromRawEnv({ content: input }); + return MacroEngine.evaluate(input, env); + }); + + const parts = output.split('###'); + expect(parts.length).toBe(2); + + const seed1 = parts[0].match(/seed:([^|]+)/)?.[1]; + const seed2 = parts[1].match(/seed:([^|]+)/)?.[1]; + + expect(seed1).toBeTruthy(); + expect(seed2).toBeTruthy(); + // Seeds must be different because the {{if}} blocks are at different positions + expect(seed1).not.toBe(seed2); + }); + + test('should maintain stability for picks inside if blocks across evaluations', async ({ page }) => { + // Picks inside if blocks should still be deterministic + const outputs = await page.evaluate(async () => { + /** @type {import('../../public/scripts/macros/engine/MacroEngine.js')} */ + const { MacroEngine } = await import('./scripts/macros/engine/MacroEngine.js'); + /** @type {import('../../public/scripts/macros/engine/MacroEnvBuilder.js')} */ + const { MacroEnvBuilder } = await import('./scripts/macros/engine/MacroEnvBuilder.js'); + + const input = '{{if true}}{{pick::X::Y::Z}}{{/if}}'; + const env1 = MacroEnvBuilder.buildFromRawEnv({ content: input }); + const env2 = MacroEnvBuilder.buildFromRawEnv({ content: input }); + const result1 = MacroEngine.evaluate(input, env1); + const result2 = MacroEngine.evaluate(input, env2); + return [result1, result2]; + }); + + // Same input should produce same output (deterministic) + expect(outputs[0]).toBe(outputs[1]); + // Should be one of the valid options + expect(['X', 'Y', 'Z'].includes(outputs[0])).toBeTruthy(); + }); }); test.describe('Dynamic macros', () => {