From 270b1b5b877b1720b0b7835a501cf925b24270c6 Mon Sep 17 00:00:00 2001 From: Brenley Dueck Date: Wed, 15 Jul 2026 20:38:21 -0500 Subject: [PATCH] fix(signals): move-and-clear _optimisticNodes on transition merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mergeTransitionState copied outgoing._optimisticNodes into the target without clearing the outgoing array. Since the global queue's arrays alias the active transition's after initTransition, the adoption pass in initTransition re-pushed the same entries into the target right after the merge — duplicating every node, compounding across repeated merges. Mirror the move-and-clear pattern already used for _affectsNodes. Repro: two overlapping actions with optimistic writes; when the first resumes and writes into a signal owned by the second's transition, the merged transition held the first action's optimistic node twice. Co-Authored-By: Claude Fable 5 --- packages/solid-signals/src/core/scheduler.ts | 8 ++- .../tests/transitionMerge.test.ts | 66 +++++++++++++++++++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 packages/solid-signals/tests/transitionMerge.test.ts diff --git a/packages/solid-signals/src/core/scheduler.ts b/packages/solid-signals/src/core/scheduler.ts index 462d9c26d..028a30f8c 100644 --- a/packages/solid-signals/src/core/scheduler.ts +++ b/packages/solid-signals/src/core/scheduler.ts @@ -154,7 +154,13 @@ function mergeTransitionState(target: Transition, outgoing: Transition): void { outgoing._done = target; target._actions.push(...outgoing._actions); for (const lane of activeLanes) if (lane._transition === outgoing) lane._transition = target; - target._optimisticNodes.push(...outgoing._optimisticNodes); + if (outgoing._optimisticNodes.length) { + // Move (don't copy): the global queue may still alias the outgoing + // array, and the adoption pass in initTransition would re-push its + // contents into the target — duplicating every entry. + target._optimisticNodes.push(...outgoing._optimisticNodes); + outgoing._optimisticNodes.length = 0; + } if (outgoing._affectsNodes.length) { // Move (don't copy): the global queue may still alias the outgoing // array, and the adoption pass in initTransition would re-push its diff --git a/packages/solid-signals/tests/transitionMerge.test.ts b/packages/solid-signals/tests/transitionMerge.test.ts new file mode 100644 index 000000000..79fc343a4 --- /dev/null +++ b/packages/solid-signals/tests/transitionMerge.test.ts @@ -0,0 +1,66 @@ +// Transition-merge state transfer (mergeTransitionState + the adoption pass +// in initTransition). The global queue's arrays alias the active transition's +// after initTransition, so a merge must MOVE (not copy) the outgoing arrays — +// otherwise the adoption pass re-pushes their contents into the target, +// duplicating every entry. + +import { describe, expect, it } from "vitest"; +import { action, createOptimistic, createRoot, flush } from "../src/index.js"; +import * as scheduler from "../src/core/scheduler.js"; + +describe("transition merge", () => { + it("does not duplicate _optimisticNodes when a resumed action merges into another transition", async () => { + let setO1!: (v: number) => void; + let setO2!: (v: number) => void; + let dispose!: () => void; + createRoot(d => { + dispose = d; + [, setO1] = createOptimistic(0); + [, setO2] = createOptimistic(0); + }); + flush(); + + let resolve1!: () => void; + const p1 = new Promise(r => (resolve1 = r)); + let resolve2!: () => void; + const p2 = new Promise(r => (resolve2 = r)); + + let snapshot: unknown[] = []; + + const act1 = action(function* () { + setO1(1); // lands in transition A's _optimisticNodes + yield p1; // A gets stashed across the flush below + // Resumes with A active again (the queue re-aliases A's arrays); o2 + // holds an override owned by transition B, so this write resolves to B + // and initTransition(B) merges A into B while the queue still aliases + // A's _optimisticNodes. + setO2(2); + snapshot = [...scheduler.activeTransition!._optimisticNodes]; + }); + + const act2 = action(function* () { + setO2(99); // lands in transition B's _optimisticNodes + yield p2; // B gets stashed across the flush below + }); + + const done1 = act1(); + flush(); // stash transition A + const done2 = act2(); + flush(); // stash transition B + + resolve1(); + await Promise.resolve(); + await Promise.resolve(); + + // Pre-fix, A's node appeared twice in the merged array: once from the + // mergeTransitionState copy, once from the adoption re-push. + expect(snapshot.length).toBe(new Set(snapshot).size); + + resolve2(); + await done1; + await done2; + flush(); + dispose(); + flush(); + }); +});