fix(canvas): Zustand snapshot-change re-render loop in ContextMenu (React Error #185) #911
@ -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: [],
|
||||
})
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
60
canvas/src/lib/__tests__/externalRuntimes.test.ts
Normal file
60
canvas/src/lib/__tests__/externalRuntimes.test.ts
Normal 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);
|
||||
});
|
||||
});
|
||||
});
|
||||
189
canvas/src/lib/__tests__/hydrate.test.ts
Normal file
189
canvas/src/lib/__tests__/hydrate.test.ts
Normal 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);
|
||||
});
|
||||
Loading…
Reference in New Issue
Block a user