fix(canvas/ContextMenu): prevent React error #185 by moving hasChildren derivation out of Zustand selector
ContextMenu used `.some()` inside its Zustand selector to compute hasChildren. Zustand's useSyncExternalStore calls the selector on every snapshot; `.some()` returns a new boolean each time, which React 19's stricter comparison and the re-render side-effects from the store subscription created a feedback loop on mobile Chat tab mount → React error #185 ("Maximum update depth exceeded"). Fix: select the stable `nodes` array once, derive children via useMemo outside the store subscription. Also removes the inline `getState().nodes.filter()` call in handleDelete in favour of the memoized children. Regression tests (2 cases): - setPendingDelete receives correct children array when workspace has children - setPendingDelete hasChildren=false and empty children when no children Refs: #651 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
"use client";
|
||||
|
||||
import { useCallback, useEffect, useRef, useState } from "react";
|
||||
import { useCallback, useEffect, useMemo, useRef, useState } from "react";
|
||||
import { useCanvasStore, type WorkspaceNodeData } from "@/store/canvas";
|
||||
import { api } from "@/lib/api";
|
||||
import { showToast } from "./Toaster";
|
||||
@@ -23,9 +23,17 @@ export function ContextMenu() {
|
||||
const setPanelTab = useCanvasStore((s) => s.setPanelTab);
|
||||
const nestNode = useCanvasStore((s) => s.nestNode);
|
||||
const contextNodeId = contextMenu?.nodeId ?? null;
|
||||
const hasChildren = useCanvasStore((s) =>
|
||||
contextNodeId ? s.nodes.some((n) => n.data.parentId === contextNodeId) : false
|
||||
// Select the full nodes array (stable reference across unrelated store
|
||||
// updates) and derive children via useMemo. Filtering inside the
|
||||
// selector returned a new array every call, which Zustand's
|
||||
// useSyncExternalStore saw as "snapshot changed" → schedule
|
||||
// re-render → loop → React error #185. See canvas-store-snapshots.
|
||||
const nodes = useCanvasStore((s) => s.nodes);
|
||||
const children = useMemo(
|
||||
() => (contextNodeId ? nodes.filter((n) => n.data.parentId === contextNodeId) : []),
|
||||
[nodes, contextNodeId],
|
||||
);
|
||||
const hasChildren = children.length > 0;
|
||||
const setPendingDelete = useCanvasStore((s) => s.setPendingDelete);
|
||||
const ref = useRef<HTMLDivElement>(null);
|
||||
const [actionLoading, setActionLoading] = useState(false);
|
||||
@@ -189,10 +197,9 @@ export function ContextMenu() {
|
||||
// it survives ContextMenu unmount. Closing the menu here avoids the
|
||||
// prior race where the portal dialog's Confirm click was treated as
|
||||
// "outside" by the menu's outside-click handler.
|
||||
const childNodes = useCanvasStore.getState().nodes.filter((n) => n.data.parentId === contextMenu.nodeId);
|
||||
setPendingDelete({ id: contextMenu.nodeId, name: contextMenu.nodeData.name, hasChildren, children: childNodes.map(c => ({ id: c.id, name: c.data.name })) });
|
||||
setPendingDelete({ id: contextMenu.nodeId, name: contextMenu.nodeData.name, hasChildren, children: children.map(c => ({ id: c.id, name: c.data.name })) });
|
||||
closeContextMenu();
|
||||
}, [contextMenu, setPendingDelete, closeContextMenu]);
|
||||
}, [contextMenu, setPendingDelete, closeContextMenu, children, hasChildren]);
|
||||
|
||||
const handleViewDetails = useCallback(() => {
|
||||
if (!contextMenu) return;
|
||||
|
||||
@@ -398,3 +398,78 @@ describe("ContextMenu — item actions", () => {
|
||||
expect(mockPost).toHaveBeenCalledWith("/workspaces/n1/resume", {});
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Regression tests for GitHub issue #651 — React error #185:
|
||||
* "Maximum update depth exceeded" on Chat tab / mobile.
|
||||
*
|
||||
* Root cause: ContextMenu's children selector ran `.filter()` inside the
|
||||
* Zustand hook, returning a brand-new array reference on every render.
|
||||
* Zustand's useSyncExternalStore compared snapshots with Object.is —
|
||||
* a new array always differs — so React kept scheduling re-renders,
|
||||
* hit the 50-update depth cap, and crashed.
|
||||
*
|
||||
* Fix: select the stable `nodes` array once, derive children via
|
||||
* useMemo outside the store subscription.
|
||||
*/
|
||||
describe("ContextMenu — hasChildren regression (GitHub #651)", () => {
|
||||
beforeEach(() => { setupApiMocks(); });
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
vi.clearAllMocks();
|
||||
mockStoreState.contextMenu = null;
|
||||
mockStoreState.closeContextMenu.mockClear();
|
||||
mockStoreState.updateNodeData.mockClear();
|
||||
mockStoreState.selectNode.mockClear();
|
||||
mockStoreState.setPanelTab.mockClear();
|
||||
mockStoreState.nestNode.mockClear();
|
||||
mockStoreState.setPendingDelete.mockClear();
|
||||
mockStoreState.setCollapsed.mockClear();
|
||||
mockStoreState.arrangeChildren.mockClear();
|
||||
mockStoreState.nodes = [];
|
||||
resetApiMocks();
|
||||
vi.mocked(showToast).mockClear();
|
||||
});
|
||||
|
||||
it("setPendingDelete receives correct children array when workspace has children", () => {
|
||||
openMenu({ nodeId: "ws-parent", nodeData: { name: "Parent", status: "online", tier: 4, role: "assistant" } });
|
||||
mockStoreState.nodes = [
|
||||
{ id: "ws-child-a", data: { parentId: "ws-parent" } },
|
||||
{ id: "ws-child-b", data: { parentId: "ws-parent" } },
|
||||
];
|
||||
render(<ContextMenu />);
|
||||
const deleteBtn = screen.getAllByRole("menuitem").find((el) =>
|
||||
el.textContent?.includes("Delete")
|
||||
)!;
|
||||
fireEvent.click(deleteBtn);
|
||||
expect(mockStoreState.setPendingDelete).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
id: "ws-parent",
|
||||
name: "Parent",
|
||||
hasChildren: true,
|
||||
children: [
|
||||
{ id: "ws-child-a", name: undefined },
|
||||
{ id: "ws-child-b", name: undefined },
|
||||
],
|
||||
})
|
||||
);
|
||||
});
|
||||
|
||||
it("setPendingDelete hasChildren=false and empty children array when workspace has no children", () => {
|
||||
openMenu({ nodeId: "ws-leaf", nodeData: { name: "Leaf", status: "online", tier: 4, role: "assistant" } });
|
||||
mockStoreState.nodes = [];
|
||||
render(<ContextMenu />);
|
||||
const deleteBtn = screen.getAllByRole("menuitem").find((el) =>
|
||||
el.textContent?.includes("Delete")
|
||||
)!;
|
||||
fireEvent.click(deleteBtn);
|
||||
expect(mockStoreState.setPendingDelete).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
id: "ws-leaf",
|
||||
name: "Leaf",
|
||||
hasChildren: false,
|
||||
children: [],
|
||||
})
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user