fix(canvas): Zustand snapshot-change re-render loop in ContextMenu (React Error #185) #911

Merged
devops-engineer merged 3 commits from fix/wcag-hover-contrast-remaining into main 2026-05-14 00:44:58 +00:00
4 changed files with 337 additions and 6 deletions

View File

@ -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;

View File

@ -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: [],
})
);
});
});

View File

@ -0,0 +1,60 @@
/**
* Tests for `isExternalLikeRuntime` mirrors the backend's
* isExternalLikeRuntime() in workspace-server/internal/handlers/runtime_registry.go.
*
* These runtimes have no platform-owned container (no Files, Terminal, Docker config).
* Both frontend and backend must agree on which runtimes are "external-like" so
* the canvas can show/hide those tabs correctly and the backend can enforce
* the same semantics server-side.
*/
import { describe, it, expect } from "vitest";
import { isExternalLikeRuntime } from "../externalRuntimes";
describe("isExternalLikeRuntime", () => {
describe("known external-like runtimes", () => {
it.each([
["external"],
["kimi"],
["kimi-cli"],
])("%q returns true", (runtime) => {
expect(isExternalLikeRuntime(runtime)).toBe(true);
});
});
describe("non-external runtimes", () => {
it.each([
"claude-code",
"hermes",
"docker",
"local",
"agent",
"crewai",
"langgraph",
"openclaw",
"custom-runtime",
])("%q returns false", (runtime) => {
expect(isExternalLikeRuntime(runtime)).toBe(false);
});
});
describe("edge cases", () => {
it("returns false for undefined", () => {
expect(isExternalLikeRuntime(undefined)).toBe(false);
});
it("returns false for null", () => {
// @ts-expect-error — intentional runtime test, null is not a valid type
expect(isExternalLikeRuntime(null)).toBe(false);
});
it("returns false for empty string", () => {
expect(isExternalLikeRuntime("")).toBe(false);
});
it("is case-sensitive — kimi vs KIMI vs Kimi", () => {
expect(isExternalLikeRuntime("KIMI")).toBe(false);
expect(isExternalLikeRuntime("Kimi")).toBe(false);
expect(isExternalLikeRuntime("kimi")).toBe(true);
});
});
});

View File

@ -0,0 +1,189 @@
// @vitest-environment jsdom
/**
* Tests for hydrate.ts canvas store hydration with exponential backoff.
*
* Covers:
* - Successful hydration on first attempt (no retries)
* - Retry with exponential backoff on failure
* - onRetrying callback called at correct intervals
* - Error propagation after MAX_RETRIES exhausted
* - Viewport persisted on success
* - Viewport failure is non-fatal
*/
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
import type { WorkspaceData } from "@/store/socket";
// ---------------------------------------------------------------------------
// Mock modules — must precede imports that use them
// ---------------------------------------------------------------------------
const mockHydrate = vi.fn();
const mockSetViewport = vi.fn();
vi.mock("@/lib/api", () => ({
api: {
get: vi.fn(),
},
PLATFORM_URL: "https://platform.test",
}));
vi.mock("@/store/canvas", () => ({
useCanvasStore: Object.assign(
() => ({}),
{
getState: () => ({
hydrate: mockHydrate,
setViewport: mockSetViewport,
}),
},
),
}));
// ---------------------------------------------------------------------------
// Import after mocks
// ---------------------------------------------------------------------------
import { api } from "@/lib/api";
import { hydrateCanvas, MAX_RETRIES } from "../hydrate";
// ---------------------------------------------------------------------------
// Mock data
// ---------------------------------------------------------------------------
const WORKSPACES: WorkspaceData[] = [
{ id: "ws-1", name: "Test Workspace" } as WorkspaceData,
];
const VIEWPORT = { x: 10, y: 20, zoom: 1.5 };
// ---------------------------------------------------------------------------
// Helpers
// ---------------------------------------------------------------------------
const mockApiGet = vi.mocked(api.get);
/** Resolves successfully for `count` parallel workspace fetches; viewport always succeeds. */
function succeedTimes(count: number) {
let workspaceRemaining = count;
mockApiGet.mockImplementation(async (url: string) => {
if (url === "/canvas/viewport") return VIEWPORT;
if (workspaceRemaining > 0) {
workspaceRemaining--;
return WORKSPACES;
}
throw new Error("API error");
});
}
/** Always fails with the given message. */
function alwaysFail(msg = "Network error") {
mockApiGet.mockRejectedValue(new Error(msg));
}
// ---------------------------------------------------------------------------
// Tests
// ---------------------------------------------------------------------------
describe("hydrateCanvas", () => {
beforeEach(() => {
vi.clearAllMocks();
mockApiGet.mockReset();
mockHydrate.mockReset();
mockSetViewport.mockReset();
});
// ── Success on first attempt ─────────────────────────────────────────────
it("hydrates the store and returns null error on first attempt success", async () => {
succeedTimes(1);
const result = await hydrateCanvas();
expect(result).toEqual({ error: null });
expect(mockHydrate).toHaveBeenCalledOnce();
});
it("persists viewport when returned by the API", async () => {
succeedTimes(1);
const result = await hydrateCanvas();
expect(result).toEqual({ error: null });
expect(mockSetViewport).toHaveBeenCalledWith(VIEWPORT);
});
// ── Viewport failure is non-fatal ─────────────────────────────────────────
it("returns null error when viewport fetch fails but workspaces succeed", async () => {
mockApiGet.mockImplementation(async (url: string) => {
if (url === "/canvas/viewport") throw new Error("Viewport error");
return WORKSPACES;
});
const result = await hydrateCanvas();
expect(result).toEqual({ error: null });
expect(mockHydrate).toHaveBeenCalledOnce();
expect(mockSetViewport).not.toHaveBeenCalled();
});
// ── Retry logic ──────────────────────────────────────────────────────────
it("retries MAX_RETRIES times before returning an error", async () => {
alwaysFail();
const onRetrying = vi.fn();
const result = await Promise.race([
hydrateCanvas(onRetrying),
new Promise<"timeout">((resolve) => setTimeout(() => resolve("timeout"), 5000)),
]);
if (result === "timeout") throw new Error("Test timed out — retries not awaited correctly");
expect(result.error).not.toBeNull();
expect(onRetrying).toHaveBeenCalledTimes(MAX_RETRIES - 1);
}, 10000);
it("onRetrying is called with attempt number before each retry", async () => {
alwaysFail();
const onRetrying = vi.fn();
await Promise.race([
hydrateCanvas(onRetrying),
new Promise<"timeout">((resolve) => setTimeout(() => resolve("timeout"), 5000)),
]);
expect(onRetrying).toHaveBeenNthCalledWith(1, 1);
expect(onRetrying).toHaveBeenNthCalledWith(2, 2);
}, 10000);
it("succeeds on second attempt — hydrates after transient failure", async () => {
let callCount = 0;
mockApiGet.mockImplementation(async (url: string) => {
if (url === "/canvas/viewport") return null;
callCount++;
if (callCount === 1) throw new Error("Transient error");
return WORKSPACES;
});
const result = await Promise.race([
hydrateCanvas(),
new Promise<"timeout">((resolve) => setTimeout(() => resolve("timeout"), 5000)),
]);
if (result === "timeout") throw new Error("Test timed out");
expect(result).toEqual({ error: null });
expect(mockHydrate).toHaveBeenCalledOnce();
}, 10000);
// ── Error messages ────────────────────────────────────────────────────────
it("error message includes the platform URL after all retries exhausted", async () => {
alwaysFail("Connection refused");
const result = await Promise.race([
hydrateCanvas(),
new Promise<"timeout">((resolve) => setTimeout(() => resolve("timeout"), 5000)),
]);
if (result === "timeout") throw new Error("Test timed out");
expect(result.error).toContain("platform.test");
expect(result.error).toContain("Unable to connect");
}, 10000);
it("error message includes the underlying error message", async () => {
alwaysFail("TLS certificate expired");
const result = await Promise.race([
hydrateCanvas(),
new Promise<"timeout">((resolve) => setTimeout(() => resolve("timeout"), 5000)),
]);
if (result === "timeout") throw new Error("Test timed out");
expect(result.error).not.toBeNull();
expect(typeof result.error).toBe("string");
}, 10000);
});