From 6385d56155181d94cce858e2354ca66b30584a77 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 25 Jun 2026 07:05:28 -0400 Subject: [PATCH] fix(state): don't drop effects under render-phase convergence (v3.5.4) Regression in v3.5.3: the render-phase convergence loop cleared the fiber's effect list before each re-run. Combined with the deps-equality check (a stable-deps effect does not re-register when deps are unchanged), the effect ended up registered in no iteration and never ran. The loop no longer clears parseFiber.effects. Effects accumulate across convergence iterations (a stable effect appears once; a deps-changing effect once per iteration) and are de-duplicated afterwards by cleanup-index via dedupRenderPhaseEffects, keeping the last (final render's) effect per slot in declaration order. Each effect now runs exactly once with the converged state for both stable and changing deps; multiple effects run once each in order. Covered by TestRenderPhaseEffectRunsOnceStableDeps / ...ChangingDeps / TestRenderPhaseMultipleEffectsEachOnce. Full suite + native/wasm pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 16 +++ .../assets/docs/latest-release-notes.md | 25 ++--- internal/runtime/hot_reload.go | 46 +++++++- ui/render_phase_effects_test.go | 101 ++++++++++++++++++ 4 files changed, 172 insertions(+), 16 deletions(-) create mode 100644 ui/render_phase_effects_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 441d38b2..6e72f4da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,21 @@ # Changelog +## v3.5.4 - 2026-06-25 + +### Fixed + +- **Effects were dropped when a component used render-phase updates (regression in + v3.5.3).** The render-phase convergence loop cleared the fiber's per-iteration + effect list before each re-run; combined with the deps-equality check (a + stable-deps effect does not re-register when its deps are unchanged), the effect + ended up registered in no iteration and never ran. The loop no longer clears the + effect list — effects accumulate across convergence iterations and are then + de-duplicated by cleanup-index (keeping the last, i.e. the final render's effect + per slot). Each effect now runs exactly once with the converged state, for both + stable and changing deps, and multiple effects run once each in declaration + order. Covered by `TestRenderPhaseEffectRunsOnceStableDeps` / + `...ChangingDeps` / `TestRenderPhaseMultipleEffectsEachOnce`. + ## v3.5.3 - 2026-06-25 ### Fixed diff --git a/examples/public-examples-site/assets/docs/latest-release-notes.md b/examples/public-examples-site/assets/docs/latest-release-notes.md index 0ef4a482..0754d512 100644 --- a/examples/public-examples-site/assets/docs/latest-release-notes.md +++ b/examples/public-examples-site/assets/docs/latest-release-notes.md @@ -3,18 +3,19 @@ This docs-site page mirrors the first section returned by `tools/changelogcheck.LatestEntry(CHANGELOG.md)`. -## v3.5.3 - 2026-06-25 +## v3.5.4 - 2026-06-25 -The current latest changelog section is `v3.5.3 - 2026-06-25`. **Fixed:** a -render-phase state update (a component calling its own `setState` during render — -the derived-state pattern) left the DOM showing a value the component never -actually rendered and never converged. `renderFunctionComponent` now detects a -render-phase update (via a new `activeRenderFiber` flag, distinct from the ambient -current fiber so it never trips on the SSR path or hook unit tests) and re-runs -the component to convergence — bounded at 25 iterations with a diagnostic to guard -against an unconditional-setState infinite loop — matching React's "keeps -restarting until there are no more new updates." Found while bug-hunting GWC's -state management against React. It is checked by the blocking `tools/changelogcheck` -release gate before a versioned release can ship. +The current latest changelog section is `v3.5.4 - 2026-06-25`. **Fixed:** a +regression in v3.5.3 where effects were dropped when a component used render-phase +updates. The convergence loop cleared the fiber's per-iteration effect list before +each re-run; combined with the deps-equality check (a stable-deps effect does not +re-register when its deps are unchanged), the effect ended up registered in no +iteration and never ran. The loop no longer clears the effect list — effects +accumulate across iterations and are then de-duplicated by cleanup-index (keeping +the final render's effect per slot), so each effect runs exactly once with the +converged state (for both stable and changing deps), and multiple effects run once +each in declaration order. Found by stress-testing the v3.5.3 fix. It is checked by +the blocking `tools/changelogcheck` release gate before a versioned release can +ship. See the repository root `CHANGELOG.md` for the full entry body. diff --git a/internal/runtime/hot_reload.go b/internal/runtime/hot_reload.go index b7acc8d4..0fd95a9b 100644 --- a/internal/runtime/hot_reload.go +++ b/internal/runtime/hot_reload.go @@ -911,8 +911,16 @@ func (parseRt *Runtime) renderFunctionComponent(parseFiber *Fiber) (*Element, bo // this render (parseFiber.renderPhaseUpdate, set by the hook setter), re-run // it with the new state until it stops updating — bounded like React's // re-render limit — so the committed output reflects the final state instead - // of a half-rendered intermediate. + // of a half-rendered intermediate. The hook indices are reset per iteration + // but the deps/cleanups/effects arrays persist; effects accumulate across + // iterations (the deps-equality check makes a stable effect register only + // once, while a deps-changing effect registers per iteration), so they are + // de-duplicated by cleanup index afterwards to commit exactly the final + // render's effect per slot. NOTE: do not clear parseFiber.effects here — + // clearing it loses an effect whose deps did not change across iterations. + parseConverged := false for parseRPIters := 0; parseFiber.renderPhaseUpdate; parseRPIters++ { + parseConverged = true parseFiber.renderPhaseUpdate = false if parseRPIters >= 25 { ReportDiagnosticWithContext("runtime", DiagnosticError, @@ -934,9 +942,6 @@ func (parseRt *Runtime) renderFunctionComponent(parseFiber *Fiber) (*Element, bo parseFiber.hooks.cleanupIndex = 0 parseFiber.hooks.signature = parseFiber.hooks.signature[:0] } - if parseFiber.effects != nil { - parseFiber.effects = parseFiber.effects[:0] - } SetCurrentFiber(parseFiber) parseRt.activeRenderFiber = parseFiber func() { @@ -954,6 +959,9 @@ func (parseRt *Runtime) renderFunctionComponent(parseFiber *Fiber) (*Element, bo } }() } + if parseConverged { + parseFiber.effects = dedupRenderPhaseEffects(parseFiber.effects) + } if parseAttempt == 0 && parseRestore != nil { if !componentSnapshotSerializableCompatible(parseRestore, parseFiber) { @@ -975,3 +983,33 @@ func (parseRt *Runtime) renderFunctionComponent(parseFiber *Fiber) (*Element, bo return nil, false, nil } + +// dedupRenderPhaseEffects collapses the effects accumulated across render-phase +// convergence iterations to exactly one per cleanup-index slot — the last one, +// i.e. the effect produced by the final converged render — preserving +// declaration (cleanup-index) order. Without this, an effect whose deps change as +// state converges would be registered (and run) once per iteration. +func dedupRenderPhaseEffects(parseEffects []Effect) []Effect { + if len(parseEffects) <= 1 { + return parseEffects + } + parseLastBySlot := make(map[int]Effect, len(parseEffects)) + parseMaxSlot := -1 + for _, parseEffect := range parseEffects { + parseLastBySlot[parseEffect.CleanupIndex] = parseEffect + if parseEffect.CleanupIndex > parseMaxSlot { + parseMaxSlot = parseEffect.CleanupIndex + } + } + if len(parseLastBySlot) == len(parseEffects) { + // No duplicates (every slot appeared once); keep the original order. + return parseEffects + } + parseOut := parseEffects[:0] + for parseSlot := 0; parseSlot <= parseMaxSlot; parseSlot++ { + if parseEffect, parseOk := parseLastBySlot[parseSlot]; parseOk { + parseOut = append(parseOut, parseEffect) + } + } + return parseOut +} diff --git a/ui/render_phase_effects_test.go b/ui/render_phase_effects_test.go new file mode 100644 index 00000000..e220f8f8 --- /dev/null +++ b/ui/render_phase_effects_test.go @@ -0,0 +1,101 @@ +//go:build !js || !wasm + +package ui_test + +import ( + "fmt" + "testing" + + "github.com/monstercameron/GoWebComponents/internal/platform/mockdom" + "github.com/monstercameron/GoWebComponents/internal/runtime" +) + +// Regression test for v3.5.4: render-phase convergence (v3.5.3) must commit +// exactly the final render's effects — running each effect once, with the +// converged state — for both stable and changing deps. The v3.5.3 convergence +// loop cleared the per-iteration effect list, which (combined with the +// deps-equality check) dropped a stable-deps effect entirely. +func TestRenderPhaseEffectRunsOnceStableDeps(t *testing.T) { + adapter := mockdom.NewMockDOMAdapter() + rt := runtime.NewRuntime(runtime.Config{DOMAdapter: adapter, Reset: true}) + root := adapter.CreateElement("div") + + parseEffectRuns := 0 + var parseSaw []int + rt.RenderInto(root, runtime.CreateElement(func() *runtime.Element { + parseGet, parseSet := runtime.GoUseState(rt, 0) + if parseGet() < 3 { + parseSet(parseGet() + 1) + } + runtime.GoUseEffect(func() func() { + parseEffectRuns++ + parseSaw = append(parseSaw, parseGet()) + return nil + }, 0) // stable deps + return runtime.CreateElement("span", map[string]any{}, fmt.Sprint(parseGet())) + }, map[string]any{})) + + if parseEffectRuns != 1 { + t.Errorf("effect runs = %d, want 1 (only the final converged render), saw=%v", parseEffectRuns, parseSaw) + } + if len(parseSaw) != 1 || parseSaw[0] != 3 { + t.Errorf("effect observed %v, want [3] (the converged state)", parseSaw) + } +} + +func TestRenderPhaseEffectRunsOnceChangingDeps(t *testing.T) { + adapter := mockdom.NewMockDOMAdapter() + rt := runtime.NewRuntime(runtime.Config{DOMAdapter: adapter, Reset: true}) + root := adapter.CreateElement("div") + + parseEffectRuns := 0 + var parseSaw []int + rt.RenderInto(root, runtime.CreateElement(func() *runtime.Element { + parseGet, parseSet := runtime.GoUseState(rt, 0) + if parseGet() < 3 { + parseSet(parseGet() + 1) + } + runtime.GoUseEffect(func() func() { + parseEffectRuns++ + parseSaw = append(parseSaw, parseGet()) + return nil + }, parseGet()) // deps change as state converges + return runtime.CreateElement("span", map[string]any{}, fmt.Sprint(parseGet())) + }, map[string]any{})) + + if parseEffectRuns != 1 { + t.Errorf("changing-deps effect runs = %d, want 1 (deduped to the final render), saw=%v", parseEffectRuns, parseSaw) + } + if len(parseSaw) != 1 || parseSaw[0] != 3 { + t.Errorf("changing-deps effect observed %v, want [3]", parseSaw) + } +} + +// Two effects in a render-phase-converging component: each runs once, in order, +// with the converged state. +func TestRenderPhaseMultipleEffectsEachOnce(t *testing.T) { + adapter := mockdom.NewMockDOMAdapter() + rt := runtime.NewRuntime(runtime.Config{DOMAdapter: adapter, Reset: true}) + root := adapter.CreateElement("div") + + var parseLog []string + rt.RenderInto(root, runtime.CreateElement(func() *runtime.Element { + parseGet, parseSet := runtime.GoUseState(rt, 0) + if parseGet() < 2 { + parseSet(parseGet() + 1) + } + runtime.GoUseEffect(func() func() { + parseLog = append(parseLog, fmt.Sprintf("a:%d", parseGet())) + return nil + }, 0) + runtime.GoUseEffect(func() func() { + parseLog = append(parseLog, fmt.Sprintf("b:%d", parseGet())) + return nil + }, 0) + return runtime.CreateElement("span", map[string]any{}, fmt.Sprint(parseGet())) + }, map[string]any{})) + + if len(parseLog) != 2 || parseLog[0] != "a:2" || parseLog[1] != "b:2" { + t.Errorf("two effects log = %v, want [a:2 b:2] (each once, converged, in order)", parseLog) + } +}