From df503e26b0ac6b64b127e00a693994464e849f60 Mon Sep 17 00:00:00 2001 From: root Date: Sat, 7 Feb 2026 18:57:37 +0800 Subject: [PATCH] fix: prevent infinite re-renders in Renderer component Root causes fixed: 1. DataProvider.get callback depended on data state, causing cascading memo invalidation through all consumers. Now uses a ref to read current data, keeping the callback identity stable. 2. ActionProvider.execute depended on data/handlers/set/navigate directly, so every data change recreated the callback and invalidated the context value. Now uses refs for frequently-changing values. 3. createRenderer created a new actionHandlers object on every render, causing ActionProvider to receive new props each cycle. Now memoized with useMemo. 4. DataProvider setData in the sync useEffect could produce a new object reference even when values hadn't changed. Added bail-out check. 5. Added useEffect to sync ActionProvider handlers when props change. Closes #53 --- packages/react/src/contexts/actions.tsx | 101 +++++---- packages/react/src/contexts/data.tsx | 19 +- packages/react/src/contexts/rerender.test.tsx | 196 ++++++++++++++++++ packages/react/src/renderer.tsx | 45 ++-- 4 files changed, 302 insertions(+), 59 deletions(-) create mode 100644 packages/react/src/contexts/rerender.test.tsx diff --git a/packages/react/src/contexts/actions.tsx b/packages/react/src/contexts/actions.tsx index d0ac13e5..92bc59db 100644 --- a/packages/react/src/contexts/actions.tsx +++ b/packages/react/src/contexts/actions.tsx @@ -5,7 +5,9 @@ import React, { useContext, useState, useCallback, + useEffect, useMemo, + useRef, type ReactNode, } from "react"; import { @@ -80,6 +82,18 @@ export function ActionProvider({ const [pendingConfirmation, setPendingConfirmation] = useState(null); + // Sync handlers when initialHandlers prop changes (e.g. from createRenderer) + const initialHandlersRef = useRef(initialHandlers); + useEffect(() => { + // Only update if the handlers object reference actually changed and has entries + if (initialHandlers !== initialHandlersRef.current) { + initialHandlersRef.current = initialHandlers; + if (Object.keys(initialHandlers).length > 0) { + setHandlers(initialHandlers); + } + } + }, [initialHandlers]); + const registerHandler = useCallback( (name: string, handler: ActionHandler) => { setHandlers((prev) => ({ ...prev, [name]: handler })); @@ -87,16 +101,55 @@ export function ActionProvider({ [], ); + // Use refs for values that change frequently to avoid re-creating `execute` + // on every data change. This breaks the cycle: + // data changes → execute changes → context value changes → consumers re-render → ... + const dataRef = useRef(data); + dataRef.current = data; + const handlersRef = useRef(handlers); + handlersRef.current = handlers; + const setRef = useRef(set); + setRef.current = set; + const navigateRef = useRef(navigate); + navigateRef.current = navigate; + const execute = useCallback( async (action: Action) => { - const resolved = resolveAction(action, data); - const handler = handlers[resolved.name]; + const currentData = dataRef.current; + const currentHandlers = handlersRef.current; + const currentSet = setRef.current; + const currentNavigate = navigateRef.current; + + const resolved = resolveAction(action, currentData); + const handler = currentHandlers[resolved.name]; if (!handler) { console.warn(`No handler registered for action: ${resolved.name}`); return; } + const doExecute = async () => { + setLoadingActions((prev) => new Set(prev).add(resolved.name)); + try { + await executeAction({ + action: resolved, + handler, + setData: currentSet, + navigate: currentNavigate, + executeAction: async (name) => { + const subAction: Action = { name }; + await execute(subAction); + }, + }); + } finally { + setLoadingActions((prev) => { + const next = new Set(prev); + next.delete(resolved.name); + return next; + }); + } + }; + // If confirmation is required, show dialog if (resolved.confirm) { return new Promise((resolve, reject) => { @@ -112,51 +165,13 @@ export function ActionProvider({ reject(new Error("Action cancelled")); }, }); - }).then(async () => { - setLoadingActions((prev) => new Set(prev).add(resolved.name)); - try { - await executeAction({ - action: resolved, - handler, - setData: set, - navigate, - executeAction: async (name) => { - const subAction: Action = { name }; - await execute(subAction); - }, - }); - } finally { - setLoadingActions((prev) => { - const next = new Set(prev); - next.delete(resolved.name); - return next; - }); - } - }); + }).then(doExecute); } // Execute immediately - setLoadingActions((prev) => new Set(prev).add(resolved.name)); - try { - await executeAction({ - action: resolved, - handler, - setData: set, - navigate, - executeAction: async (name) => { - const subAction: Action = { name }; - await execute(subAction); - }, - }); - } finally { - setLoadingActions((prev) => { - const next = new Set(prev); - next.delete(resolved.name); - return next; - }); - } + await doExecute(); }, - [data, handlers, set, navigate], + [], // stable — reads current values from refs ); const confirm = useCallback(() => { diff --git a/packages/react/src/contexts/data.tsx b/packages/react/src/contexts/data.tsx index 217e11df..8897887e 100644 --- a/packages/react/src/contexts/data.tsx +++ b/packages/react/src/contexts/data.tsx @@ -68,12 +68,27 @@ export function DataProvider({ if (newJson !== initialDataJsonRef.current) { initialDataJsonRef.current = newJson; if (initialData && Object.keys(initialData).length > 0) { - setData((prev) => ({ ...prev, ...initialData })); + setData((prev) => { + const next = { ...prev, ...initialData }; + // Bail out if the merged result is identical to avoid unnecessary re-renders + if (JSON.stringify(next) === JSON.stringify(prev)) { + return prev; + } + return next; + }); } } }, [initialData]); - const get = useCallback((path: string) => getByPath(data, path), [data]); + // Use a ref so `get` doesn't need `data` in its dependency array, + // preventing cascading re-renders through callbacks that depend on `get`. + const dataRef = useRef(data); + dataRef.current = data; + + const get = useCallback( + (path: string) => getByPath(dataRef.current, path), + [], + ); const set = useCallback( (path: string, value: unknown) => { diff --git a/packages/react/src/contexts/rerender.test.tsx b/packages/react/src/contexts/rerender.test.tsx new file mode 100644 index 00000000..30c9e2bc --- /dev/null +++ b/packages/react/src/contexts/rerender.test.tsx @@ -0,0 +1,196 @@ +import { describe, it, expect, vi } from "vitest"; +import React, { useRef, useEffect } from "react"; +import { render, act } from "@testing-library/react"; +import { DataProvider, useData } from "./data"; +import { VisibilityProvider } from "./visibility"; +import { ActionProvider } from "./actions"; + +/** + * Regression test for infinite re-render bug (Issue #53). + * + * The Renderer component (and its context providers) caused infinite + * re-renders when given a static spec in a non-streaming scenario. + * + * Root causes: + * 1. DataProvider.get depended on `data`, causing cascading memo invalidation + * 2. ActionProvider.execute depended on `data` and `handlers` directly, + * so every data change recreated the execute callback → new context value + * 3. createRenderer created a new `actionHandlers` object on every render + */ + +/** Helper component that counts how many times it renders */ +function RenderCounter({ onRender }: { onRender: (count: number) => void }) { + const countRef = useRef(0); + countRef.current += 1; + + useEffect(() => { + onRender(countRef.current); + }); + + return React.createElement( + "div", + { "data-testid": "counter" }, + `renders: ${countRef.current}`, + ); +} + +/** Helper that reads from DataProvider to subscribe to context changes */ +function DataConsumer({ onRender }: { onRender: (count: number) => void }) { + const { data } = useData(); + const countRef = useRef(0); + countRef.current += 1; + + useEffect(() => { + onRender(countRef.current); + }); + + return React.createElement("div", null, JSON.stringify(data)); +} + +describe("Infinite re-render prevention", () => { + it("DataProvider does not cause re-renders when initialData reference changes but value is the same", async () => { + const renderCounts: number[] = []; + const onRender = (count: number) => { + renderCounts.push(count); + }; + + const staticData = { user: { name: "John" } }; + + const { rerender } = render( + React.createElement( + DataProvider, + { initialData: staticData }, + React.createElement(DataConsumer, { onRender }), + ), + ); + + // Re-render with a new object reference but same values + await act(async () => { + rerender( + React.createElement( + DataProvider, + { initialData: { user: { name: "John" } } }, + React.createElement(DataConsumer, { onRender }), + ), + ); + }); + + // Should render at most twice (initial + rerender from parent), + // NOT keep growing indefinitely + const lastCount = renderCounts[renderCounts.length - 1]; + expect(lastCount).toBeLessThanOrEqual(3); + }); + + it("DataProvider with empty initialData does not trigger infinite updates", async () => { + const renderCounts: number[] = []; + const onRender = (count: number) => { + renderCounts.push(count); + }; + + const { rerender } = render( + React.createElement( + DataProvider, + { initialData: {} }, + React.createElement(DataConsumer, { onRender }), + ), + ); + + // Re-render with a new empty object reference + await act(async () => { + rerender( + React.createElement( + DataProvider, + { initialData: {} }, + React.createElement(DataConsumer, { onRender }), + ), + ); + }); + + const lastCount = renderCounts[renderCounts.length - 1]; + expect(lastCount).toBeLessThanOrEqual(3); + }); + + it("full provider stack does not cause infinite re-renders with static data", async () => { + const renderCounts: number[] = []; + const onRender = (count: number) => { + renderCounts.push(count); + }; + + const staticData = { items: [1, 2, 3] }; + + const tree = React.createElement( + DataProvider, + { initialData: staticData }, + React.createElement( + VisibilityProvider, + null, + React.createElement( + ActionProvider, + { handlers: {} }, + React.createElement(RenderCounter, { onRender }), + ), + ), + ); + + const { rerender } = render(tree); + + // Re-render the entire tree with same-value data but new references + await act(async () => { + rerender( + React.createElement( + DataProvider, + { initialData: { items: [1, 2, 3] } }, + React.createElement( + VisibilityProvider, + null, + React.createElement( + ActionProvider, + { handlers: {} }, + React.createElement(RenderCounter, { onRender }), + ), + ), + ), + ); + }); + + // With the fix, render count should stabilize (not grow unbounded) + const lastCount = renderCounts[renderCounts.length - 1]; + expect(lastCount).toBeLessThanOrEqual(4); + }); + + it("DataProvider.get callback identity is stable across data changes", async () => { + const getCallbacks: Array<(path: string) => unknown> = []; + + function GetCollector() { + const { get, set } = useData(); + const isFirst = useRef(true); + + getCallbacks.push(get); + + useEffect(() => { + if (isFirst.current) { + isFirst.current = false; + // Trigger a data change — `get` should NOT change identity + set("/foo", "bar"); + } + }, [set]); + + return null; + } + + render( + React.createElement( + DataProvider, + { initialData: {} }, + React.createElement(GetCollector), + ), + ); + + // Wait for effects + await act(async () => {}); + + // `get` should have the same reference across renders + expect(getCallbacks.length).toBeGreaterThanOrEqual(2); + expect(getCallbacks[0]).toBe(getCallbacks[1]); + }); +}); diff --git a/packages/react/src/renderer.tsx b/packages/react/src/renderer.tsx index 114bcd8c..ae985480 100644 --- a/packages/react/src/renderer.tsx +++ b/packages/react/src/renderer.tsx @@ -1,6 +1,11 @@ "use client"; -import React, { type ComponentType, type ReactNode, useMemo } from "react"; +import React, { + type ComponentType, + type ReactNode, + useMemo, + useRef, +} from "react"; import type { UIElement, Spec, @@ -458,19 +463,31 @@ export function createRenderer< authState, fallback, }: CreateRendererProps) { - // Wrap onAction to match internal API - const actionHandlers = onAction - ? { - __default__: (params: Record) => { - const actionName = params.__actionName__ as string; - const actionParams = params.__actionParams__ as Record< - string, - unknown - >; - return onAction(actionName, actionParams); - }, - } - : undefined; + // Stabilize onAction ref so the actionHandlers object doesn't change every render + const onActionRef = useRef(onAction); + onActionRef.current = onAction; + + // Memoize actionHandlers to prevent re-creating the object on every render, + // which would cause ActionProvider to receive new props and potentially + // trigger infinite re-renders. + const actionHandlers = useMemo( + () => + onAction + ? { + __default__: (params: Record) => { + const actionName = params.__actionName__ as string; + const actionParams = params.__actionParams__ as Record< + string, + unknown + >; + return onActionRef.current?.(actionName, actionParams); + }, + } + : undefined, + // Only re-create when onAction goes from defined to undefined or vice versa + // eslint-disable-next-line react-hooks/exhaustive-deps + [!!onAction], + ); return (