From a2b88c981e8115ec975b13eaf6f63bb51c3bfa94 Mon Sep 17 00:00:00 2001 From: Brenley Dueck Date: Mon, 5 Oct 2026 18:26:01 -0500 Subject: [PATCH 1/2] fix(signals): skip the run for a re-staging effect pass --- .changeset/restaged-effect-no-early-run.md | 5 ++ packages/signals/src/core/core.ts | 4 +- .../held-effect-restage-run-3802.test.ts | 66 ++++++++++++++++++ .../test/held-action-memo-mount-3802.spec.tsx | 68 +++++++++++++++++++ 4 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 .changeset/restaged-effect-no-early-run.md create mode 100644 packages/signals/tests/held-effect-restage-run-3802.test.ts create mode 100644 packages/web/test/held-action-memo-mount-3802.spec.tsx diff --git a/.changeset/restaged-effect-no-early-run.md b/.changeset/restaged-effect-no-early-run.md new file mode 100644 index 000000000..058c7fd7a --- /dev/null +++ b/.changeset/restaged-effect-no-early-run.md @@ -0,0 +1,5 @@ +--- +"@solidjs/signals": patch +--- + +A render effect created while an action holds a value it reads no longer runs its callback when a later pass re-stages it before the action lands (#3802). That run passed `undefined` as the value, because the effect had nothing committed yet, so compiled JSX bindings threw and halted reactivity. The effect now runs once, at the commit that lands the action, with the latest staged value. diff --git a/packages/signals/src/core/core.ts b/packages/signals/src/core/core.ts index 80f163985..6bdf4b034 100644 --- a/packages/signals/src/core/core.ts +++ b/packages/signals/src/core/core.ts @@ -570,7 +570,9 @@ export function recompute(el: Computed, create: boolean = false): void { el._flags & REACTIVE_SCREEN_READ && !(el as any)._modified && Object.is((el as any)._prevValue, value) - ) + ) && + // A re-staging pass owes no run: the commit queues it (#3802). + el._pendingValue === NOT_PENDING ) { (el as any)._modified = !el._x?._error; // Reuse one bound runner per effect — runEffect no-ops on a stale diff --git a/packages/signals/tests/held-effect-restage-run-3802.test.ts b/packages/signals/tests/held-effect-restage-run-3802.test.ts new file mode 100644 index 000000000..b30066130 --- /dev/null +++ b/packages/signals/tests/held-effect-restage-run-3802.test.ts @@ -0,0 +1,66 @@ +// #3802: a render effect born held under an action, re-run by a write to a +// committed source, ran its callback with no committed value. + +import { afterEach, expect, it, vi } from "vitest"; +import { + action, + createMemo, + createRenderEffect, + createRoot, + createSignal, + flush +} from "../src/index.js"; + +afterEach(() => vi.restoreAllMocks()); + +async function settle() { + for (let i = 0; i < 8; i++) await Promise.resolve(); + flush(); +} + +it("a re-staging pass of a born-held effect does not run it before the commit", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const [value, setValue] = createSignal(false); + const [mounted, setMounted] = createSignal(false); + const [title, setTitle] = createSignal(""); + let resolve!: () => void; + const gate = new Promise(r => (resolve = r)); + const runs: [boolean, string][] = []; + + const save = action(function* () { + setValue(true); + yield gate; + }); + + createRoot(() => { + const child = createMemo(() => { + if (!mounted()) return; + createMemo(() => value()); + createRenderEffect( + () => ({ v: value(), t: title() }), + state => { + runs.push([state.v, state.t]); + } + ); + }); + createRenderEffect(child, () => {}); + }); + flush(); + + const pending = save(); + await settle(); + setMounted(true); + await settle(); + setTitle("Updated"); + await settle(); + + expect(error).not.toHaveBeenCalled(); + expect(runs).toEqual([]); + + resolve(); + await pending; + await settle(); + + expect(error).not.toHaveBeenCalled(); + expect(runs).toEqual([[true, "Updated"]]); +}); diff --git a/packages/web/test/held-action-memo-mount-3802.spec.tsx b/packages/web/test/held-action-memo-mount-3802.spec.tsx new file mode 100644 index 000000000..378fe3cb6 --- /dev/null +++ b/packages/web/test/held-action-memo-mount-3802.spec.tsx @@ -0,0 +1,68 @@ +/** + * @jsxImportSource @solidjs/web + * @vitest-environment jsdom + */ +import { afterEach, expect, test, vi } from "vitest"; +import { Show, action, createMemo, createSignal, flush } from "solid-js"; +import { render } from "../src/index.js"; + +function deferred() { + let resolve!: () => void; + const promise = new Promise(r => (resolve = r)); + return { promise, resolve }; +} + +async function settle() { + for (let i = 0; i < 8; i++) await Promise.resolve(); + flush(); +} + +afterEach(() => vi.restoreAllMocks()); + +test("updating an attribute of a subtree mounted under a held action does not crash (#3802)", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const div = document.createElement("div"); + const [value, setValue] = createSignal(false); + const [mounted, setMounted] = createSignal(false); + const [title, setTitle] = createSignal(""); + const gate = deferred(); + + const save = action(function* () { + setValue(true); + yield gate.promise; + }); + + const dispose = render( + () => ( +
+ + {_ => { + createMemo(() => value()); + return
; + }} + +
+ ), + div + ); + flush(); + + const pending = save(); + await settle(); + setMounted(true); + await settle(); + setTitle("Updated"); + await settle(); + + expect(error).not.toHaveBeenCalled(); + + gate.resolve(); + await pending; + await settle(); + + expect(error).not.toHaveBeenCalled(); + const el = div.querySelector("div")!; + expect(el.className).toBe("true"); + expect(el.title).toBe("Updated"); + dispose(); +}); From 976fe1e07242d7cfc3ae01a424c3c1c50c4f5e42 Mon Sep 17 00:00:00 2001 From: Ryan Carniato Date: Mon, 5 Oct 2026 21:57:02 -0700 Subject: [PATCH 2/2] test(signals): pin the lane pass of a born-held effect (#3802) The broader condition also skips the run an optimistic lane queues for a born-held effect; that run applied an uncommitted value too. Also align the inline comment with effect.ts ('the commit replays the effect'). Co-authored-by: Claude via Cursor Co-authored-by: Cursor --- packages/signals/src/core/core.ts | 2 +- .../held-effect-restage-run-3802.test.ts | 56 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/packages/signals/src/core/core.ts b/packages/signals/src/core/core.ts index 6bdf4b034..2ee50e388 100644 --- a/packages/signals/src/core/core.ts +++ b/packages/signals/src/core/core.ts @@ -571,7 +571,7 @@ export function recompute(el: Computed, create: boolean = false): void { !(el as any)._modified && Object.is((el as any)._prevValue, value) ) && - // A re-staging pass owes no run: the commit queues it (#3802). + // A re-staging pass owes no run: the commit replays the effect (#3802). el._pendingValue === NOT_PENDING ) { (el as any)._modified = !el._x?._error; diff --git a/packages/signals/tests/held-effect-restage-run-3802.test.ts b/packages/signals/tests/held-effect-restage-run-3802.test.ts index b30066130..98ed7ad0d 100644 --- a/packages/signals/tests/held-effect-restage-run-3802.test.ts +++ b/packages/signals/tests/held-effect-restage-run-3802.test.ts @@ -5,6 +5,7 @@ import { afterEach, expect, it, vi } from "vitest"; import { action, createMemo, + createOptimistic, createRenderEffect, createRoot, createSignal, @@ -64,3 +65,58 @@ it("a re-staging pass of a born-held effect does not run it before the commit", expect(error).not.toHaveBeenCalled(); expect(runs).toEqual([[true, "Updated"]]); }); + +// The same effect re-derived by an optimistic write's lane rather than a +// mainline write: the lane's run had nothing committed to apply either. +it("a lane pass of a born-held effect does not run it with no committed value", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + const [value, setValue] = createSignal(false); + const [mounted, setMounted] = createSignal(false); + const [guess, setGuess] = createOptimistic(0); + let resolveSave!: () => void; + let resolveVote!: () => void; + const runs: [boolean, number][] = []; + + const save = action(function* () { + setValue(true); + yield new Promise(r => (resolveSave = r)); + }); + const vote = action(function* () { + setGuess(5); + yield new Promise(r => (resolveVote = r)); + }); + + createRoot(() => { + const child = createMemo(() => { + if (!mounted()) return; + createMemo(() => value()); + createRenderEffect( + () => ({ v: value(), g: guess() }), + state => { + runs.push([state.v, state.g]); + } + ); + }); + createRenderEffect(child, () => {}); + }); + flush(); + + const saving = save(); + await settle(); + setMounted(true); + await settle(); + const voting = vote(); + await settle(); + + expect(error).not.toHaveBeenCalled(); + + resolveVote(); + await voting; + await settle(); + resolveSave(); + await saving; + await settle(); + + expect(error).not.toHaveBeenCalled(); + expect(runs.at(-1)).toEqual([true, 0]); +});