diff --git a/.changeset/history-undo-multi-source-fix.md b/.changeset/history-undo-multi-source-fix.md new file mode 100644 index 000000000..f1039cbb4 --- /dev/null +++ b/.changeset/history-undo-multi-source-fix.md @@ -0,0 +1,9 @@ +--- +"@solid-primitives/history": patch +--- + +Fix `createUndoHistory` misfiring restores when tracking multiple sources and one of them pauses (returns a falsy value) while another stays active. + +Previously each history entry stored a compacted array of setters, dropping any paused source — so entries on either side of a pause/resume boundary could end up with different lengths, causing `undo`/`redo` to compare setters by the wrong array index and either restore the wrong source or spuriously re-fire one that hadn't actually changed. Entries now keep a fixed-length slot per source (`undefined` when paused), so index alignment is stable across every recorded entry. + +Credit to @mesram, whose `createStore`/`createOptimistic`-based rewrite (for the Solid 2.0 line) is the basis for this fix. diff --git a/packages/history/src/index.ts b/packages/history/src/index.ts index 0037eaf96..2eb6f2de3 100644 --- a/packages/history/src/index.ts +++ b/packages/history/src/index.ts @@ -85,19 +85,21 @@ export function createUndoHistory( let ignoreNext = false; + // One slot per source, always the same length as `sources` — even when a + // source is paused (returns falsy) it keeps its `undefined` slot instead of + // being dropped, so entries never end up with mismatched lengths and index + // i always refers to the same source across every recorded entry. + type Setters = (VoidFunction | undefined)[]; + const limit = options?.limit ?? 100, sources = Array.isArray(source) ? source.map(s => createMemo(s)) : [source], clearIgnore = createMicrotask(() => (ignoreNext = false)), - history = createMemo<{ count: Signal; list: VoidFunction[][] }>( + history = createMemo<{ count: Signal; list: Setters[] }>( p => { // always track the sources - const setters: VoidFunction[] = []; - for (const s of sources) { - const setter = s(); - if (setter) setters.push(setter); - } + const setters: Setters = sources.map(s => s() || undefined); - if (ignoreNext || !setters.length) { + if (ignoreNext || setters.every(s => s === undefined)) { ignoreNext = false; return p; } @@ -125,8 +127,13 @@ export function createUndoHistory( prevSetters = h.list[newIndex + n]!, setters = h.list[newIndex]!; for (let i = 0; i < setters.length; i++) { - // only call the setter if the current value is different - if (setters[i] !== prevSetters[i]) setters[i]!(); + // only call the setter if it was active on both sides of the move + // and the value actually differs — if a source was paused on either + // side we have no tracked value to compare against, so skip it + // rather than firing a spurious restore + const setter = setters[i], + prevSetter = prevSetters[i]; + if (setter !== undefined && prevSetter !== undefined && setter !== prevSetter) setter(); } }; diff --git a/packages/history/test/index.test.ts b/packages/history/test/index.test.ts index 8bf9e0529..6fc420d14 100644 --- a/packages/history/test/index.test.ts +++ b/packages/history/test/index.test.ts @@ -287,4 +287,55 @@ describe("createUndoHistory", () => { dispose(); }); }); + + test("multiple sources, one intermittently paused - no spurious/misaligned restores", () => { + createRoot(dispose => { + const [a, setA] = createSignal(0); + const [b, setB] = createSignal(0); + const [trackB, setTrackB] = createSignal(true); + let bCalls = 0; + + const history = createUndoHistory([ + () => { + const v = a(); + return () => setA(v); + }, + () => { + if (!trackB()) return undefined; + const v = b(); + return () => { + bCalls++; + setB(v); + }; + }, + ]); + + // recorded entries: E0(a0,b0) E1(a1,b0) E2(a1,-b paused-) E3(a2,-b paused-) + setA(1); + setTrackB(false); + setA(2); + + history.undo(); // E3 -> E2 + expect(a()).toBe(1); + expect(bCalls).toBe(0); // b never changed, must not fire + + history.undo(); // E2 -> E1 (crosses the pause boundary) + expect(a()).toBe(1); // a is legitimately unchanged between E1/E2 — not a bug + expect(bCalls).toBe(0); // must not spuriously re-fire b just because array shapes differ across the boundary + + history.undo(); // E1 -> E0 + expect(a()).toBe(0); + expect(bCalls).toBe(0); + expect(history.canUndo()).toBe(false); + + history.redo(); // E0 -> E1 + history.redo(); // E1 -> E2 (crosses the pause boundary going forward) + expect(bCalls).toBe(0); + history.redo(); // E2 -> E3 + expect(a()).toBe(2); + expect(bCalls).toBe(0); + + dispose(); + }); + }); });