Fix macro variable reassignment

[chhoumann]

Fixes #1014. Fixes #1010.

Both issues report user scripts populating variables, but Templater behaves as if the values were never set (prompting for missing {{VALUE:*}} fields).

This PR makes params.variables a getter/setter backed by the shared variables Map:

• Assigning an object or Map (QuickAdd.variables = {...} / params.variables = {...}) replaces the backing store so downstream templating sees the new values.
• Self-assignment (assigning the same backing Map / params.variables = params.variables) is treated as a no-op to avoid clearing variables.
• Invalid assignments (e.g. numbers/strings) are ignored to avoid wiping the backing store.

Tests: bun run test.

Summary by CodeRabbit

• Tests

  • Added tests covering how user scripts read, replace, or modify the variables object during macro execution.

• Bug Fixes

  • Improved variable handling so replacing the variables object swaps stored entries, assigning the same map preserves existing keys, and invalid reassignments are ignored — while preserving backward compatibility.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
quickadd	Ready	Preview	Dec 13, 2025 1:32pm

[coderabbitai]

Warning

Rate limit exceeded

@chhoumann has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 12 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 8dfcd64 and 579d289.

📒 Files selected for processing (2)

• src/engine/MacroChoiceEngine.entry.test.ts (1 hunks)
• src/engine/MacroChoiceEngine.ts (1 hunks)

Walkthrough

Adds a runtime-backed params.variables property in MacroChoiceEngine that exposes a proxy over a shared Map; the setter now replaces or repopulates the backing Map when assigned a new object/Map and ignores same-backing assignments. Adds three tests covering replace, in-place mutation, and invalid reassignment behaviors.

Changes

Cohort / File(s)	Summary
Variables proxy implementation src/engine/MacroChoiceEngine.ts	Introduces variablesProxy via createVariablesProxy(sharedVariables) and defines a runtime variables property on the returned params with a getter (returns proxy) and a setter that: ignores assignments of the same backing, clears and repopulates the backing sharedVariables when assigned a new object or Map, and preserves enumerable/non-configurable descriptor behavior.
Proxy behavior tests src/engine/MacroChoiceEngine.entry.test.ts	Adds three tests in the "MacroChoiceEngine user script variable propagation" suite: (1) params.variables = {...} replaces the backing map (removes previous keys), (2) assigning into the same backing map preserves existing keys and adds new ones, and (3) invalid assignments to params.variables (non-object) are ignored without clearing existing keys while preserving any mutations the script made.

Sequence Diagram(s)

sequenceDiagram
  participant UserScript as User Script
  participant Engine as MacroChoiceEngine (params)
  participant Backing as sharedVariables (Map)
  participant ChoiceExec as choiceExecutor

  Note over Engine,Backing: Engine creates sharedVariables and variablesProxy
  Engine->>Backing: create sharedVariables Map
  Engine->>Engine: createVariablesProxy(sharedVariables)

  UserScript->>Engine: read params.variables (getter)
  Engine->>UserScript: return variablesProxy (reflects Backing)

  alt Script assigns a new object/Map (params.variables = newObj)
    UserScript->>Engine: params.variables = newObj
    Engine->>Backing: clear()
    Engine->>Backing: populate entries from newObj
    Engine->>ChoiceExec: choiceExecutor reads variables from Backing (updated)
  else Script mutates same backing (in-place)
    UserScript->>Backing: mutate via proxy (add/update keys)
    Backing-->>ChoiceExec: choiceExecutor reads variables including mutations
  else Script assigns invalid value (e.g., number)
    UserScript->>Engine: params.variables = invalid
    Engine-->>UserScript: ignore invalid assignment (do not clear Backing)
    Engine->>ChoiceExec: choiceExecutor sees Backing with any in-place mutations
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Pay attention to:
  • Correct detection of "same backing" vs new object/Map assignments.
  • Robustness of clearing and repopulating sharedVariables (iteration over objects/Maps, prototype properties).
  • Handling of invalid assignment types and preservation of descriptor flags.
  • Tests correctly simulate user scripts and assert backing Map state.

Possibly related PRs

• fix: preserve macro user script variable updates #999 — Directly related: also modifies MacroChoiceEngine params.variables handling and uses a Map-backed variables proxy with tests for replacement vs mutation semantics.

Poem

🐇 I poked the proxy, twitching whiskers bright,
Replaced a whole map in the soft morning light.
I nudged some keys gently, some stayed the same,
Numbers I sniffed — ignored their odd claim.
Hooray — a rabbit's hop for variables' game!

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix macro variable reassignment' directly describes the main change: introducing getter/setter logic for params.variables to handle reassignment correctly.
Linked Issues check	✅ Passed	The changes implement the core requirements from both #1014 and #1010: variables assigned via QuickAdd.variables are now visible to Templater by using a getter/setter backed by shared variables.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing macro variable reassignment: modifications to MacroChoiceEngine.ts and new regression tests in MacroChoiceEngine.entry.test.ts are both focused on the stated objective.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/engine/MacroChoiceEngine.ts (1)

88-121: LGTM! Clean implementation of the variables proxy with backward-compatible assignment semantics.

The dynamic property definition correctly handles both object and Map assignment, enabling scripts to replace the backing store while maintaining the proxy interface. The setter's clear-and-repopulate approach ensures downstream templates can consume updated variables.

Optional optimization: The self-assignment guard at line 104 could also check next === variablesProxy to avoid an unnecessary clear+repopulate cycle when a script does params.variables = params.variables. Currently this results in clearing and repopulating the Map with identical entries, which is functionally correct but inefficient.

 set: (next: unknown) => {
-	if (next === sharedVariables) return;
+	if (next === sharedVariables || next === variablesProxy) return;

 	const entries =

Optional clarification: The comment at line 99 mentions QuickAdd.variables but the code defines params.variables. Consider updating the comment to match the actual property name for clarity:

-// Backward compatibility: some scripts assign `QuickAdd.variables = {...}`.
+// Backward compatibility: some scripts assign `params.variables = {...}`.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1b62937 and 72dabea.

📒 Files selected for processing (2)

• src/engine/MacroChoiceEngine.entry.test.ts (1 hunks)
• src/engine/MacroChoiceEngine.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Source code lives in src/: core logic under engine/, services/, and utils/; Svelte UI in src/gui; shared types in src/types; settings entry in src/quickAddSettingsTab.ts

Files:

• src/engine/MacroChoiceEngine.entry.test.ts
• src/engine/MacroChoiceEngine.ts

src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through the logger utilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters or tests/obsidian-stub.ts

Files:

• src/engine/MacroChoiceEngine.entry.test.ts
• src/engine/MacroChoiceEngine.ts

src/**/*.{ts,tsx,svelte}

📄 CodeRabbit inference engine (AGENTS.md)

Use PascalCase for classes and Svelte components

Files:

• src/engine/MacroChoiceEngine.entry.test.ts
• src/engine/MacroChoiceEngine.ts

🧠 Learnings (1)

📚 Learning: 2025-12-09T21:20:52.398Z

Learnt from: CR
Repo: chhoumann/quickadd PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-09T21:20:52.398Z
Learning: Applies to tests/**/*.{ts,tsx} : Add regression coverage for bug fixes

Applied to files:

• src/engine/MacroChoiceEngine.entry.test.ts

🧬 Code graph analysis (2)

src/engine/MacroChoiceEngine.entry.test.ts (4)

src/types/macros/IUserScript.ts (1)

• IUserScript (3-6)

src/types/choices/IMacroChoice.ts (1)

• IMacroChoice (4-7)

src/types/macros/IMacro.ts (1)

• IMacro (3-7)

src/engine/MacroChoiceEngine.ts (1)

• MacroChoiceEngine (58-648)

src/engine/MacroChoiceEngine.ts (3)

src/utils/variablesProxy.ts (1)

• createVariablesProxy (7-54)

src/quickAddApi.ts (1)

• QuickAddApi (33-643)

src/errors/MacroAbortError.ts (1)

• MacroAbortError (1-6)

🔇 Additional comments (2)

src/engine/MacroChoiceEngine.entry.test.ts (2)

329-371: LGTM! Excellent regression coverage for replacement semantics.

This test correctly validates that assigning a new object to params.variables replaces the backing Map entirely, removing pre-existing keys. The test structure is clear and aligns with the PR objective to preserve macro variables when scripts reassign the map.

---

373-416: LGTM! Good coverage for self-assignment edge case.

This test validates that self-assigning the proxy doesn't inadvertently clear existing variables. The scenario of params.variables = params.variables followed by mutation is an important edge case to cover.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72dabea and 8a8e808.

📒 Files selected for processing (1)

• src/engine/MacroChoiceEngine.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Source code lives in src/: core logic under engine/, services/, and utils/; Svelte UI in src/gui; shared types in src/types; settings entry in src/quickAddSettingsTab.ts

Files:

• src/engine/MacroChoiceEngine.ts

src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through the logger utilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters or tests/obsidian-stub.ts

Files:

• src/engine/MacroChoiceEngine.ts

src/**/*.{ts,tsx,svelte}

📄 CodeRabbit inference engine (AGENTS.md)

Use PascalCase for classes and Svelte components

Files:

• src/engine/MacroChoiceEngine.ts

🧬 Code graph analysis (1)

src/engine/MacroChoiceEngine.ts (3)

src/utils/variablesProxy.ts (1)

• createVariablesProxy (7-54)

src/quickAddApi.ts (1)

• QuickAddApi (33-643)

src/errors/MacroAbortError.ts (1)

• MacroAbortError (1-6)

🔇 Additional comments (4)

src/engine/MacroChoiceEngine.ts (4)

50-50: LGTM!

The import is correctly added and aligns with the new variable proxy functionality.

---

88-97: LGTM!

The variables proxy is correctly initialized, and the type assertion pattern is appropriate for the dynamic property definition that follows.

---

102-103: LGTM!

The getter correctly returns the variables proxy, enabling scripts to read variables.

---

118-120: LGTM!

The property descriptor configuration is appropriate: enumerable: true ensures visibility in standard object iterations, and configurable: false protects the getter/setter mechanism from being overridden or deleted.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/engine/MacroChoiceEngine.ts (1)

114-116: Consider logging when an invalid assignment is ignored (debuggability).
Silent no-ops can be hard to diagnose in user scripts; a log.logWarning(...) here would help without changing behavior.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a8e808 and 8dfcd64.

📒 Files selected for processing (2)

• src/engine/MacroChoiceEngine.entry.test.ts (1 hunks)
• src/engine/MacroChoiceEngine.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/engine/MacroChoiceEngine.entry.test.ts

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Source code lives in src/: core logic under engine/, services/, and utils/; Svelte UI in src/gui; shared types in src/types; settings entry in src/quickAddSettingsTab.ts

Files:

• src/engine/MacroChoiceEngine.ts

src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.{ts,tsx}: Biome enforces tab indentation (width 2), LF endings, and an 80-character line guide; align editor settings
Use camelCase for variables and functions
Prefer type-only imports in TypeScript files
Route logging through the logger utilities for consistent output
Structure production code so Obsidian dependencies are injected behind interfaces; unit tests target pure logic and swap in adapters or tests/obsidian-stub.ts

Files:

• src/engine/MacroChoiceEngine.ts

src/**/*.{ts,tsx,svelte}

📄 CodeRabbit inference engine (AGENTS.md)

Use PascalCase for classes and Svelte components

Files:

• src/engine/MacroChoiceEngine.ts

🧬 Code graph analysis (1)

src/engine/MacroChoiceEngine.ts (2)

src/utils/variablesProxy.ts (1)

• createVariablesProxy (7-54)

src/errors/MacroAbortError.ts (1)

• MacroAbortError (1-6)

🔇 Additional comments (1)

src/engine/MacroChoiceEngine.ts (1)

88-125: Good backward-compatibility shim for params.variables (getter/setter + self-assignment no-op).
This should address the “script-set variables not seen by templating” regression while preventing accidental clearing on params.variables = params.variables.

[github-actions]

🎉 This PR is included in version 2.9.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀