app, core: fix Undo silently no-op'ing after a variable edit

Round 2 of adversarial review found a real bug in the fix the previous
round's own review prompted: mutating the live project.variables array (and
its row objects) in place before calling setVariables() meant projects.js's
remember() -- which deep-clones the project for the undo stack BEFORE
applying the new value -- captured the already-edited state as the "before"
snapshot. Undo after any variable edit or removal was a silent no-op.

Fixed by never mutating what project.variables already points at: every
commit now reads project.variables fresh and hands setVariables a brand new
array, which is also what closes the SAME stale-closure race the in-place
version was built to fix in the first place (redraw-mid-edit still reads
whatever the previous commit already saved, since it reads live rather than
from a snapshot taken at render time). Also fixed core/prompt_build.py's
docstring, which claimed the opposite of what resolve-variables-last
actually does and what its own test already pinned.

app/shell/inspector.js

@@ -134,33 +134,46 @@ export function createInspector({ project, onRename } = {}) {
       }),
       composer.label.section({ text: "Variables" }),
       composer.hint.default({ text: "$name in the anchor, a scene, or the postfix is replaced with its value." }),
-      // `vars` (project.variables) is mutated IN PLACE and handed back to the
-      // same setVariables call, rather than rebuilt via .map() from this
-      // closure's own index -- setVariables's changed() triggers boot.js's
-      // `inspector.draw()`, which tears down and rebuilds every field here
-      // (including the one NOT being edited) before a second, still-pending
-      // commit on this same row can fire. A rebuilt-`next`-array version reads
-      // its OWN stale `vars` snapshot at that point and would silently
-      // overwrite whatever the first commit just wrote; mutating the shared
-      // object means the second commit's own edit survives no matter which
-      // node the redraw already detached.
+      // Every write below reads `project.variables` FRESH at commit time
+      // (never the `vars` snapshot this render closed over) and hands
+      // setVariables a brand new array -- two things that both have to hold:
+      //
+      // 1. setVariables's own remember() clones `project` for undo BEFORE
+      //    applying the new list, so that clone must still show the OLD
+      //    values. An earlier version of this mutated the shared row object
+      //    in place and passed the SAME array back -- by the time remember()
+      //    cloned it, the "before" snapshot already held the new value, and
+      //    Undo after any variable edit silently did nothing.
+      // 2. setVariables's changed() triggers boot.js's `inspector.draw()`,
+      //    rebuilding every field here (including ones not being edited)
+      //    before a second, still-pending commit on the same row can fire.
+      //    Reading `project.variables` fresh (rather than the `vars` this
+      //    closure captured at render time) means that second commit builds
+      //    its own edit on top of whatever the first commit already saved,
+      //    instead of overwriting it with a stale copy.
+      //
+      // The index `i` stays valid across such a redraw because only Add/✕
+      // change the array's shape, and neither can fire between two commits
+      // on the same still-open row.
       ...vars.map((v, i) => composer.settingsRow.default({
         label: `$${v.name || "…"}`,
         control: composer.toolbar.default({ label: `$${v.name}`, items: [
           composer.input.md({
             value: v.name, placeholder: "name",
-            onCommit: (name) => { v.name = name.replace(/^\$+/, ""); project.setVariables(vars); },
+            onCommit: (name) => project.setVariables(project.variables.map(
+              (x, j) => (j === i ? { ...x, name: name.replace(/^\$+/, "") } : x))),
           }),
           composer.input.md({
             value: v.value, placeholder: "value",
-            onCommit: (value) => { v.value = value; project.setVariables(vars); },
+            onCommit: (value) => project.setVariables(project.variables.map(
+              (x, j) => (j === i ? { ...x, value } : x))),
           }),
           composer.button.sm({ label: "✕", tone: "danger",
-            onClick: () => { vars.splice(i, 1); project.setVariables(vars); } }),
+            onClick: () => project.setVariables(project.variables.filter((_, j) => j !== i)) }),
         ] }),
       })),
       composer.button.md({ label: "+ Add variable",
-        onClick: () => project.setVariables([...vars, { name: "", value: "" }]) }),
+        onClick: () => project.setVariables([...project.variables, { name: "", value: "" }]) }),
     ];
   }
 

app/shell/tests/ui.test.js

@@ -207,6 +207,35 @@ test("whatever mounted at project.negative shows only on the Project tab", () =>
   assert.ok(inspector.node.contains(marker), "the field did not survive switching away and back");
 });
 
+test("editing a variable never mutates the array a caller already holds a reference to", () => {
+  // The real projects.js store's setVariables() calls remember() -- which
+  // deep-clones `project` for the undo stack -- BEFORE applying the new
+  // list. If a caller mutated the array project.variables already points at
+  // and handed that SAME array back, remember()'s clone would capture the
+  // ALREADY-EDITED values as the "before" state, and Undo would restore
+  // exactly what is already on screen: a silent no-op. This fixture stands
+  // in for that hazard by keeping its own reference to the array from
+  // before the edit and checking it is untouched afterward.
+  const before = [{ name: "a", value: "1" }];
+  let current = before;
+  const project = fakeProject({
+    variables: before,
+    setVariables: (list) => { current = list; },
+  });
+  Object.defineProperty(project, "variables", { get: () => current });
+  const inspector = createInspector({ project });
+  document.body.replaceChildren(inspector.node);
+  inspector.show("project");
+
+  const nameInput = inspector.node.querySelector('input[placeholder="name"]');
+  nameInput.value = "b";
+  fire(nameInput, "change");
+
+  assert.notEqual(current, before, "the same array was handed back to setVariables");
+  assert.deepEqual(before, [{ name: "a", value: "1" }],
+    "a reference taken before the edit changed value out from under it");
+});
+
 test("with no scene selected the inspector says so instead of drawing nothing", () => {
   const inspector = createInspector({ project: fakeProject({ selected: null, scenes: [] }) });
   document.body.replaceChildren(inspector.node);

core/prompt_build.py

@@ -9,8 +9,11 @@ expanded copy nothing can trace back.
 Order matches v4's: anchor and postfix are joined to the scene text FIRST,
 then shortcuts expand across the whole combined string (a shortcut trigger
 sitting right at the anchor/scene boundary still matches), then `$variables`
-resolve last (so a variable's value can itself contain a shortcut trigger or
-another variable and still expand).
+resolve last. Resolving last means a variable's OWN text is never re-scanned
+for shortcut triggers -- only the anchor/scene/postfix text is -- but a
+variable may still reference another variable, since resolve_variables()
+recurses through the variable map itself (see its own docstring for the
+cycle-safety that recursion needs).
 """
 
 from __future__ import annotations