Skip to content

Commit 6a291b0

Browse files
chhoumannchhoumann
committed
docs: add macro variables proxy guide
1 parent bf19a7d commit 6a291b0

1 file changed

Lines changed: 136 additions & 0 deletions

File tree

‎docs/dev/macro-variables-proxy.md‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
# QuickAdd Macro Variables: Single-Source-of-Truth Refactor Guide
2+
3+
**Audience:** QuickAdd contributors touching macro execution, user scripts, AI helpers, or choice executor plumbing.
4+
**Goal:** Explain the regression (BUG #995), the Map-backed proxy solution, and how to work safely with shared macro variables going forward.
5+
6+
---
7+
8+
## 1) Context: How macro variables are shared
9+
10+
- **ChoiceExecutor** owns a `Map<string, unknown> variables` that survives across nested choice executions.
11+
- **MacroChoiceEngine** exposes `params` to user scripts. Historically, `params.variables` was a plain object copied from the Map after each command and pushed back afterward.
12+
- **SingleMacroEngine** and **SingleInlineScriptEngine** reuse the same `MacroChoiceEngine` core.
13+
14+
### The old flow (v2.8.0, broken)
15+
1. Execute command (including user script) using `this.params.variables` (plain object).
16+
2. After command, pull values from executor Map into `params.variables`.
17+
3. Then push `params.variables` back into the Map.
18+
19+
This ordering overwrote mutations a script just made with stale Map data. Multiple user scripts in the same macro would all see the first non-null value—BUG #995.
20+
21+
### Requirements we must satisfy
22+
- **Correctness:** A script’s changes must be immediately visible to the next command.
23+
- **Safety:** Avoid stale overwrites; handle commands that write directly to the Map (e.g., AI helpers).
24+
- **Compatibility:** User scripts often destructure `const { variables } = params;` and call helpers like `hasOwnProperty`.
25+
- **Maintainability:** Minimize sync points and special cases.
26+
27+
---
28+
29+
## 2) The fix: Map-backed proxy as single source of truth
30+
31+
**Key idea:** Keep one canonical store (the Map). Present an object-like facade (`params.variables`) via a `Proxy` so reads/writes/delete/has/iteration map directly to the Map. No push/pull needed.
32+
33+
File: `src/utils/variablesProxy.ts`
34+
- Target object includes only a `hasOwnProperty` shim for backward compatibility; no full Object prototype.
35+
- Proxy traps:
36+
- `get/set/deleteProperty/has` delegate to the Map.
37+
- `ownKeys` and `getOwnPropertyDescriptor` make `Object.keys`, `for...in`, `JSON.stringify`, and `Object.assign` work as expected.
38+
- Symbols are ignored to keep behavior predictable.
39+
40+
MacroChoiceEngine now sets:
41+
```ts
42+
this.params.variables = createVariablesProxy(this.choiceExecutor.variables);
43+
```
44+
and removes all pull/push sync logic.
45+
46+
---
47+
48+
## 3) Variable Map selection & merge rules
49+
50+
Constructor logic (`MacroChoiceEngine`):
51+
- If a `variables` Map is supplied, use it as the shared map.
52+
- If the executor already had a Map and a new Map is provided, merge existing keys into the provided Map (don’t drop prior state).
53+
- Otherwise, reuse executor Map or create a fresh one.
54+
- Always reassign `choiceExecutor.variables = sharedVariables` so downstream sees the canonical Map.
55+
56+
Rationale: prevent accidental data loss when callers pass a new Map but expect existing executor state to persist.
57+
58+
---
59+
60+
## 4) AI helper consistency
61+
62+
In `src/quickAddApi.ts`, AI prompt helpers previously did `Object.assign(choiceExecutor.variables, assistantRes)`—a no-op on Map. Now they iterate `Object.entries(assistantRes)` and `set` into the Map. This aligns with the single-source-of-truth model.
63+
64+
---
65+
66+
## 5) Tests added/updated
67+
68+
- `src/engine/MacroChoiceEngine.entry.test.ts`
69+
- Regression: sequential user scripts log `undefined, 1, 2, 3` and final `target === 3`.
70+
- Merge safeguard: existing executor vars are preserved when a custom Map is provided.
71+
- `src/utils/variablesProxy.test.ts`
72+
- Read/write/delete, enumeration, external Map mutation visibility.
73+
- `hasOwnProperty` shim works; `toString` remains undefined (no Object prototype).
74+
- `for...in`, `Object.assign`, `JSON.stringify` compatibility.
75+
76+
Run: `bun run test` (full suite), `bun run lint`, `bun run build-with-lint`.
77+
78+
---
79+
80+
## 6) How to extend safely
81+
82+
When adding new commands or helpers that manipulate variables:
83+
- **Always write through the Map** (`choiceExecutor.variables.set(...)`) or through `params.variables` (proxy will hit the Map).
84+
- **Do not reintroduce push/pull sync**—the proxy eliminates the need.
85+
- **Avoid spreading/cloning `params.variables` into plain objects** unless you truly need a snapshot. Remember snapshots will decouple from live Map updates.
86+
- If you must add helpers that expect Object prototype methods, consider explicit shims instead of restoring the prototype chain.
87+
88+
---
89+
90+
## 7) Potential pitfalls & how to avoid them
91+
92+
- **Object.assign on Map**: No-op. Always iterate and `set`.
93+
- **Prototype expectations**: Only `hasOwnProperty` is provided. Other prototype methods (e.g., `toString`) are intentionally absent to avoid prototype pollution. Accessing them should yield `undefined`.
94+
- **Multiple Maps**: Passing a new Map into engines will merge in prior executor keys, but not vice versa. Be explicit about which Map you want shared.
95+
- **Serialization**: `JSON.stringify(params.variables)` works because `ownKeys`/descriptors are implemented; Symbol keys are ignored.
96+
97+
---
98+
99+
## 8) Quick implementation checklist (if you touch this area)
100+
101+
1. Need shared macro vars? Use the existing `choiceExecutor.variables` Map; wrap with `createVariablesProxy` for object-style access.
102+
2. Writing new helpers? Use `Map.set`/`get`/`delete` or the proxy—never `Object.assign` on a Map.
103+
3. Adding tests? Cover:
104+
- Sequential command visibility.
105+
- External Map mutations seen through proxy.
106+
- Enumeration and reflection behaviors.
107+
4. Run `bun run build-with-lint` and `bun run test`.
108+
109+
---
110+
111+
## 9) Rationale recap
112+
113+
- **Single source of truth** prevents sync races.
114+
- **Proxy facade** preserves the ergonomic API (`params.variables.foo`) scripts rely on.
115+
- **Shims, not prototypes** balance backward compatibility with safety.
116+
- **Map merge** avoids silent data loss when callers swap Maps.
117+
- **Consistent writes** ensure AI/other helpers don’t bypass the canonical store.
118+
119+
---
120+
121+
## 10) If you need to change behavior later
122+
123+
- Update `variablesProxy` first; add targeted tests.
124+
- Keep constructor merge rules documented; adjust tests accordingly.
125+
- Re-run full suite—performance tests are occasionally spiky; rerun once if a perf check flakes.
126+
127+
---
128+
129+
## File map for quick navigation
130+
131+
- `src/utils/variablesProxy.ts` — proxy implementation.
132+
- `src/engine/MacroChoiceEngine.ts` — variable Map selection and params wiring.
133+
- `src/engine/MacroChoiceEngine.entry.test.ts` — macro regression tests.
134+
- `src/utils/variablesProxy.test.ts` — proxy behavior tests.
135+
- `src/quickAddApi.ts` — AI helper variable assignment fixes.
136+

0 commit comments

Comments
 (0)