diff --git a/app/shell/inspector.js b/app/shell/inspector.js index 711e4698..dc2fc5cc 100644 --- a/app/shell/inspector.js +++ b/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: "" }]) }), ]; } diff --git a/app/shell/tests/ui.test.js b/app/shell/tests/ui.test.js index 948b3c97..3a2ca5e4 100644 --- a/app/shell/tests/ui.test.js +++ b/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); diff --git a/core/prompt_build.py b/core/prompt_build.py index 2d576a16..295f4958 100644 --- a/core/prompt_build.py +++ b/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