Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 58 additions & 43 deletions packages/react/src/contexts/actions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ import React, {
useContext,
useState,
useCallback,
useEffect,
useMemo,
useRef,
type ReactNode,
} from "react";
import {
Expand Down Expand Up @@ -80,23 +82,74 @@ export function ActionProvider({
const [pendingConfirmation, setPendingConfirmation] =
useState<PendingConfirmation | null>(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) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Sync handler state when handlers prop is cleared

The new synchronization effect only updates state when initialHandlers has keys, so if the parent later passes {}/undefined (for example, createRenderer toggling onAction off), the previous handler map is retained indefinitely. In that state, execute still sees actions as registered and can continue running downstream onSuccess/onError behavior, so disabling handlers no longer actually disables action execution. The sync path should also clear handlers when the prop becomes empty.

Useful? React with 👍 / 👎.

setHandlers(initialHandlers);
}
}
}, [initialHandlers]);

const registerHandler = useCallback(
(name: string, handler: ActionHandler) => {
setHandlers((prev) => ({ ...prev, [name]: handler }));
},
[],
);

// 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<void>((resolve, reject) => {
Expand All @@ -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(() => {
Expand Down
19 changes: 17 additions & 2 deletions packages/react/src/contexts/data.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<DataModel>(data);
dataRef.current = data;

const get = useCallback(
(path: string) => getByPath(dataRef.current, path),
[],
);

const set = useCallback(
(path: string, value: unknown) => {
Expand Down
196 changes: 196 additions & 0 deletions packages/react/src/contexts/rerender.test.tsx
Original file line number Diff line number Diff line change
@@ -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]);
});
});
Loading