From c6559765b75db6133d2b3924da17c44fdbf66a50 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Tue, 15 Sep 2026 11:07:10 +0300 Subject: [PATCH] fix: pipeline_state.js -- deep-merge queued saves, close a load race Review found the save() queue's pendingBody merge was shallow: two queued edits that both touch the top-level "inputs" key (two different slots, or even two fields on the same slot from models.js's per-call-fresh objects) had the second's inputs object silently REPLACE the first's rather than combine -- the earlier edit vanished with no error, the field just reverted on the next render. engine_settings.js's own calls happened to be immune by accident (one long-lived mutated `values` object, so the last queued snapshot already contained every earlier edit) -- that immunity was never a property of save() itself, and models.js's fresh-object-per- call shape hit it immediately. Fixed with a one-level-deeper merge for "inputs"/"values" specifically. Verified live via two real race reproductions run through the browser console: two edits on different slots queued with no await between them, and two edits on the SAME slot -- both survive now, confirmed against the actual saved graph state. Also closed a load race: ensureLoaded()'s only guard was `slots !== null`, which doesn't cover the window while a first load is still in flight -- two panels mounting close together could both start a fetch. Now every caller awaits the same in-flight promise, so exactly one fetch ever goes out per session. Documented (not fixed, deliberately) that Models & Pipeline's node-level pin target is currently unreachable: pinTarget() only ever returns a group-level pin today, since there's no "currently open node" concept in this flat slot list yet -- the openNode/ctx.sub("node:...") plumbing stays for whatever old pin data might still reference it, but nothing produces a fresh one. Real gap against v4's per-node pinning, not a regression. Co-Authored-By: Claude Sonnet 5 --- app/legacy/models.js | 8 +++++ app/legacy/pipeline_state.js | 67 +++++++++++++++++++++++++++++------- 2 files changed, 62 insertions(+), 13 deletions(-) diff --git a/app/legacy/models.js b/app/legacy/models.js index 71a959cf..f6505176 100644 --- a/app/legacy/models.js +++ b/app/legacy/models.js @@ -242,6 +242,14 @@ iconBg: "linear-gradient(180deg,#b18cff,#7a4fd0)", icon: '', mount, + // Group-level pinning only, deliberately -- v4 could pin ONE node (settings_window.js's + // own comment: "the node page is the thing that takes several clicks to reach"), which + // needed a concept of "the currently open node" this flat slot list doesn't have yet + // (nothing here drills into a single slot the way the old per-role node page did). + // `openNodeId`/`ModelsModal.openNode` below still work if something hands them a real + // slot id -- pinned_buttons.js still calls them -- but nothing on this screen currently + // PRODUCES a node-level pin to begin with, so a per-node pin from before this rewrite + // is the only way this path is reached today. Group-level is the honest current target. pinTarget: () => (group ? { kind: "section", id: "models", sub: group, label: `Models ▸ ${group}` } : null), diff --git a/app/legacy/pipeline_state.js b/app/legacy/pipeline_state.js index 28625a38..6193c972 100644 --- a/app/legacy/pipeline_state.js +++ b/app/legacy/pipeline_state.js @@ -48,21 +48,62 @@ return merged; } + // Two panels can mount close enough together that both call this before + // the first fetch settles (`slots` only becomes non-null at the end of a + // load, so a bare `if (slots !== null) return` doesn't cover the window + // while one is already in flight). Every caller awaits the SAME in-flight + // promise instead, so exactly one fetch ever goes out per session, however + // many panels ask. + let loadPromise = null; + async function ensureLoaded() { if (slots !== null) return; // session-wide: every consumer shares this one load + if (loadPromise) return loadPromise; loading = true; loadError = null; - try { - const [manifest, pipe] = await Promise.all([API.modules(), API.pipeline()]); - modulesById = {}; - (manifest.modules || []).forEach((m) => { modulesById[m.id] = m; }); - slots = pipe.slots || []; - incomplete = pipe.incomplete || []; - refused = pipe.refused || []; - queueable = !!pipe.queueable; - } catch (e) { - loadError = e && e.message ? e.message : String(e); - } - loading = false; + loadPromise = (async () => { + try { + const [manifest, pipe] = await Promise.all([API.modules(), API.pipeline()]); + modulesById = {}; + (manifest.modules || []).forEach((m) => { modulesById[m.id] = m; }); + slots = pipe.slots || []; + incomplete = pipe.incomplete || []; + refused = pipe.refused || []; + queueable = !!pipe.queueable; + } catch (e) { + loadError = e && e.message ? e.message : String(e); + } + loading = false; + loadPromise = null; + })(); + return loadPromise; + } + + // `save()`'s body is one level deeper than a shallow spread can merge + // correctly: `inputs`/`values` are both {outerId: {innerKey: value}}, and + // two queued saves that both touch `inputs` (two different slots, or even + // two fields on the SAME slot from models.js's per-call-fresh objects) had + // the second's `inputs` object silently REPLACE the first's rather than + // combine with it -- the earlier edit vanished with no error. Merges one + // level deeper than Object.assign/spread does, for exactly the two keys + // that are ever shaped this way. + function mergeBodies(a, b) { + const out = { ...(a || {}) }; + Object.entries(b || {}).forEach(([key, val]) => { + const prior = out[key]; + const bothPlainObjects = val && typeof val === "object" && !Array.isArray(val) + && prior && typeof prior === "object" && !Array.isArray(prior); + if (!bothPlainObjects) { out[key] = val; return; } + const merged = { ...prior }; + Object.entries(val).forEach(([innerId, inner]) => { + const priorInner = merged[innerId]; + merged[innerId] = (inner && typeof inner === "object" && !Array.isArray(inner) + && priorInner && typeof priorInner === "object" && !Array.isArray(priorInner)) + ? { ...priorInner, ...inner } + : inner; + }); + out[key] = merged; + }); + return out; } // One shared save queue for every caller. `body` is layered onto the @@ -74,7 +115,7 @@ // retry-after-in-flight shape exists (a same-panel double-edit dropped // one edit before this). async function save(body) { - pendingBody = { ...(pendingBody || {}), ...body }; + pendingBody = mergeBodies(pendingBody, body); if (saving) { pending = true; return; } saving = true; do {