diff --git a/canvas/src/components/MissingKeysModal.tsx b/canvas/src/components/MissingKeysModal.tsx index 3adc9deeb..54eceff3e 100644 --- a/canvas/src/components/MissingKeysModal.tsx +++ b/canvas/src/components/MissingKeysModal.tsx @@ -344,7 +344,7 @@ function ProviderPickerModal({ // wrapper's bounds instead of the viewport. if (typeof document === "undefined") return null; - const allSaved = entries.length > 0 && entries.every((e) => e.saved); + const allSaved = entries.every((e) => e.saved); const anySaving = entries.some((e) => e.saving); const runtimeLabel = runtime .replace(/[-_]/g, " ") @@ -616,7 +616,7 @@ function AllKeysModal({ if (!open) return null; if (typeof document === "undefined") return null; - const allSaved = entries.length > 0 && entries.every((e) => e.saved); + const allSaved = entries.every((e) => e.saved); const anySaving = entries.some((e) => e.saving); const runtimeLabel = runtime .replace(/[-_]/g, " ") diff --git a/canvas/src/components/WorkspaceNode.tsx b/canvas/src/components/WorkspaceNode.tsx index c776dbbb7..7999e216b 100644 --- a/canvas/src/components/WorkspaceNode.tsx +++ b/canvas/src/components/WorkspaceNode.tsx @@ -13,17 +13,20 @@ import { isExternalLikeRuntime } from "@/lib/externalRuntimes"; /** Descendant count for the "N sub" badge — children are first-class nodes * rendered as full cards inside this one via React Flow's native parentId, - * so we don't need to subscribe to the actual child list here. */ + * so we don't need to subscribe to the actual child list here. + * Selecting `nodes` stably avoids a new selector reference on every store + * update (React error #185 / Zustand + React 19 Object.is strictness). */ function useDescendantCount(nodeId: string): number { - return useCanvasStore( - useCallback((s) => countDescendants(nodeId, s.nodes), [nodeId]) - ); + const nodes = useCanvasStore((s) => s.nodes); + return useMemo(() => countDescendants(nodeId, nodes), [nodeId, nodes]); } +/** Boolean flag used to drive min-size and NodeResizer dimensions. + * Selecting `nodes` stably avoids re-render loops (same issue as + * useDescendantCount). */ function useHasChildren(nodeId: string): boolean { - return useCanvasStore( - useCallback((s) => s.nodes.some((n) => n.data.parentId === nodeId), [nodeId]) - ); + const nodes = useCanvasStore((s) => s.nodes); + return useMemo(() => nodes.some((n) => n.data.parentId === nodeId), [nodes, nodeId]); } /** Eject/extract arrow icon — visually distinct from delete ✕ */ diff --git a/canvas/src/components/canvas/DropTargetBadge.tsx b/canvas/src/components/canvas/DropTargetBadge.tsx index 900b20124..1f2525525 100644 --- a/canvas/src/components/canvas/DropTargetBadge.tsx +++ b/canvas/src/components/canvas/DropTargetBadge.tsx @@ -24,16 +24,20 @@ import { */ export function DropTargetBadge() { const dragOverNodeId = useCanvasStore((s) => s.dragOverNodeId); - const targetName = useCanvasStore((s) => { - if (!s.dragOverNodeId) return null; - const n = s.nodes.find((nn) => nn.id === s.dragOverNodeId); + // Select nodes stably first — deriving targetName and childCount inside + // the same selector creates a new return value on every store mutation + // even when neither has changed (React error #185 / Zustand Object.is). + const nodes = useCanvasStore((s) => s.nodes); + const targetName = (() => { + if (!dragOverNodeId) return null; + const n = nodes.find((nn) => nn.id === dragOverNodeId); return (n?.data as WorkspaceNodeData | undefined)?.name ?? null; - }); - const childCount = useCanvasStore((s) => - !s.dragOverNodeId + })(); + const childCount = (() => + !dragOverNodeId ? 0 - : s.nodes.filter((n) => n.parentId === s.dragOverNodeId).length, - ); + : nodes.filter((n) => n.parentId === dragOverNodeId).length + )(); const { getInternalNode, flowToScreenPosition } = useReactFlow(); if (!dragOverNodeId || !targetName) return null; const internal = getInternalNode(dragOverNodeId); diff --git a/canvas/src/components/canvas/__tests__/useOrgDeployState.test.ts b/canvas/src/components/canvas/__tests__/useOrgDeployState.test.ts new file mode 100644 index 000000000..421fcd42e --- /dev/null +++ b/canvas/src/components/canvas/__tests__/useOrgDeployState.test.ts @@ -0,0 +1,311 @@ +/** + * Unit tests for buildDeployMap — the pure tree-traversal core of + * useOrgDeployState. + * + * What is tested here: + * - Root / leaf identification via parent-chain walk + * - isDeployingRoot: true when any descendant is "provisioning" + * - isActivelyProvisioning: true only for the node itself in that state + * - isLockedChild: true for non-root nodes in a deploying tree + * - isLockedChild: also true for nodes in deletingIds (even if not deploying) + * - descendantProvisioningCount: non-zero only on root nodes + * - Performance contract: O(n) single-pass walk — tested by verifying + * correctness across 50-node trees (n=50, all cases above) + * + * What is NOT tested here (hook integration — appropriate for E2E): + * - The useMemo / Zustand subscription wiring + * - React Flow integration (flowToScreenPosition, getInternalNode) + * + * Issue: #2071 (Canvas test gaps follow-up). + */ +import { describe, expect, it } from "vitest"; +import { buildDeployMap, type OrgDeployState } from "../useOrgDeployState"; + +// ── Helpers ────────────────────────────────────────────────────────────────── + +type Projection = { id: string; parentId: string | null; status: string }; + +function proj( + id: string, + parentId: string | null, + status: string, +): Projection { + return { id, parentId, status }; +} + +/** Unchecked cast — test helpers aren't production code paths. */ +function m( + ps: Projection[], + deletingIds: string[] = [], +): Map { + return buildDeployMap(ps, new Set(deletingIds)); +} + +function s( + map: Map, + id: string, +): OrgDeployState { + const got = map.get(id); + if (!got) throw new Error(`no entry for id=${id}`); + return got; +} + +// ── Empty / trivial ─────────────────────────────────────────────────────────── + +describe("buildDeployMap — empty", () => { + it("returns empty map for empty projections", () => { + expect(m([]).size).toBe(0); + }); +}); + +// ── Single node ───────────────────────────────────────────────────────────── + +describe("buildDeployMap — single node", () => { + it("isolated node is its own root and not deploying", () => { + const map = m([proj("a", null, "online")]); + expect(s(map, "a")).toEqual({ + isActivelyProvisioning: false, + isDeployingRoot: false, + isLockedChild: false, + descendantProvisioningCount: 0, + }); + }); + + it("isolated provisioning node is deploying root", () => { + const map = m([proj("a", null, "provisioning")]); + expect(s(map, "a")).toEqual({ + isActivelyProvisioning: true, + isDeployingRoot: true, + isLockedChild: false, + descendantProvisioningCount: 1, + }); + }); +}); + +// ── Parent / child chains ───────────────────────────────────────────────────── + +describe("buildDeployMap — parent / child chains", () => { + it("root with online child: root is not deploying, child is not locked", () => { + // A ──► B + const map = m([ + proj("A", null, "online"), + proj("B", "A", "online"), + ]); + expect(s(map, "A")).toMatchObject({ isDeployingRoot: false, isLockedChild: false }); + expect(s(map, "B")).toMatchObject({ isDeployingRoot: false, isLockedChild: false }); + }); + + it("root with provisioning child: root is deploying, child is locked", () => { + // A ──► B (B is provisioning) + const map = m([ + proj("A", null, "online"), + proj("B", "A", "provisioning"), + ]); + expect(s(map, "A")).toMatchObject({ isDeployingRoot: true, descendantProvisioningCount: 1 }); + expect(s(map, "B")).toMatchObject({ isLockedChild: true, isActivelyProvisioning: true }); + }); + + it("provisioning root with online child: root is deploying, child is locked", () => { + // A (provisioning) ──► B (online) + const map = m([ + proj("A", null, "provisioning"), + proj("B", "A", "online"), + ]); + expect(s(map, "A")).toMatchObject({ isDeployingRoot: true, isActivelyProvisioning: true }); + expect(s(map, "B")).toMatchObject({ isLockedChild: true, isActivelyProvisioning: false }); + }); + + it("grandchild inherits deploy lock through intermediate online node", () => { + // A ──► B ──► C (A is provisioning) + const map = m([ + proj("A", null, "provisioning"), + proj("B", "A", "online"), + proj("C", "B", "online"), + ]); + // B and C are both non-root descendants of the deploying root + expect(s(map, "B")).toMatchObject({ isLockedChild: true }); + expect(s(map, "C")).toMatchObject({ isLockedChild: true }); + expect(s(map, "A")).toMatchObject({ isDeployingRoot: true, descendantProvisioningCount: 1 }); + }); + + it("deep chain: only the topmost node with a null parent counts as root", () => { + // A ──► B ──► C ──► D (A is provisioning) + const map = m([ + proj("A", null, "provisioning"), + proj("B", "A", "online"), + proj("C", "B", "online"), + proj("D", "C", "online"), + ]); + const roots = ["A", "B", "C", "D"].filter((id) => s(map, id).isDeployingRoot); + expect(roots).toEqual(["A"]); + }); +}); + +// ── Sibling branching ───────────────────────────────────────────────────────── + +describe("buildDeployMap — sibling branching", () => { + it("parent with multiple children: deploying root propagates to all children", () => { + // A (provisioning) + // / \ + // B C + const map = m([ + proj("A", null, "provisioning"), + proj("B", "A", "online"), + proj("C", "A", "online"), + ]); + expect(s(map, "B")).toMatchObject({ isLockedChild: true }); + expect(s(map, "C")).toMatchObject({ isLockedChild: true }); + expect(s(map, "A")).toMatchObject({ descendantProvisioningCount: 1 }); + }); + + it("only one provisioning descendant marks the root as deploying", () => { + // A + // / | \ + // B C D (only C is provisioning) + const map = m([ + proj("A", null, "online"), + proj("B", "A", "online"), + proj("C", "A", "provisioning"), + proj("D", "A", "online"), + ]); + expect(s(map, "A")).toMatchObject({ isDeployingRoot: true, descendantProvisioningCount: 1 }); + expect(s(map, "B")).toMatchObject({ isLockedChild: true }); + expect(s(map, "C")).toMatchObject({ isLockedChild: true, isActivelyProvisioning: true }); + expect(s(map, "D")).toMatchObject({ isLockedChild: true }); + }); + + it("two provisioning siblings: count reflects both", () => { + const map = m([ + proj("A", null, "online"), + proj("B", "A", "provisioning"), + proj("C", "A", "provisioning"), + ]); + expect(s(map, "A")).toMatchObject({ descendantProvisioningCount: 2 }); + expect(s(map, "B")).toMatchObject({ isActivelyProvisioning: true }); + expect(s(map, "C")).toMatchObject({ isActivelyProvisioning: true }); + }); +}); + +// ── Multiple disjoint trees ─────────────────────────────────────────────────── + +describe("buildDeployMap — multiple disjoint trees", () => { + it("each tree has its own root; deploying nodes are independent", () => { + // Tree 1: X (provisioning) ──► Y + // Tree 2: P ──► Q (no provisioning) + const map = m([ + proj("X", null, "provisioning"), + proj("Y", "X", "online"), + proj("P", null, "online"), + proj("Q", "P", "online"), + ]); + expect(s(map, "X")).toMatchObject({ isDeployingRoot: true }); + expect(s(map, "Y")).toMatchObject({ isLockedChild: true }); + expect(s(map, "P")).toMatchObject({ isDeployingRoot: false, isLockedChild: false }); + expect(s(map, "Q")).toMatchObject({ isDeployingRoot: false, isLockedChild: false }); + }); +}); + +// ── Deleting nodes ──────────────────────────────────────────────────────────── + +describe("buildDeployMap — deletingIds", () => { + it("node in deletingIds is locked even if tree is not deploying", () => { + const map = m( + [ + proj("A", null, "online"), + proj("B", "A", "online"), + ], + ["B"], // B is being deleted + ); + expect(s(map, "A")).toMatchObject({ isLockedChild: false }); + expect(s(map, "B")).toMatchObject({ isLockedChild: true, isActivelyProvisioning: false }); + }); + + it("node in deletingIds: isLockedChild is true regardless of provisioning", () => { + const map = m( + [ + proj("A", null, "provisioning"), + proj("B", "A", "online"), + ], + ["B"], + ); + // B is both a deploying-child AND a deleting node — either alone locks it + expect(s(map, "B")).toMatchObject({ isLockedChild: true }); + }); + + it("empty deletingIds set has no effect", () => { + const map = m( + [ + proj("A", null, "online"), + proj("B", "A", "online"), + ], + [], + ); + expect(s(map, "B")).toMatchObject({ isLockedChild: false }); + }); +}); + +// ── descendantProvisioningCount ─────────────────────────────────────────────── + +describe("buildDeployMap — descendantProvisioningCount", () => { + it("is 0 for non-root nodes", () => { + const map = m([ + proj("A", null, "provisioning"), + proj("B", "A", "provisioning"), + ]); + expect(s(map, "B").descendantProvisioningCount).toBe(0); + }); + + it("includes the root's own status when provisioning", () => { + const map = m([ + proj("A", null, "provisioning"), + proj("B", "A", "online"), + ]); + // A is both root and provisioning → count includes itself + expect(s(map, "A").descendantProvisioningCount).toBe(1); + }); + + it("accumulates all provisioning descendants (not just immediate children)", () => { + const map = m([ + proj("A", null, "online"), + proj("B", "A", "online"), + proj("C", "B", "provisioning"), + ]); + expect(s(map, "A").descendantProvisioningCount).toBe(1); + }); +}); + +// ── O(n) performance ───────────────────────────────────────────────────────── + +describe("buildDeployMap — O(n) performance contract", () => { + it("handles a 50-node three-level tree without incorrect node assignments", () => { + // Level 0: 1 root + // Level 1: 7 children + // Level 2: 42 leaves + // Total: 50 nodes + const projections: Projection[] = []; + projections.push(proj("root", null, "provisioning")); + for (let i = 0; i < 7; i++) { + projections.push(proj(`l1-${i}`, "root", "online")); + } + for (let i = 0; i < 42; i++) { + const parent = `l1-${Math.floor(i / 6)}`; + projections.push(proj(`l2-${i}`, parent, "online")); + } + const map = m(projections); + + // Root is the only deploying node + expect(s(map, "root")).toMatchObject({ + isDeployingRoot: true, + isLockedChild: false, + descendantProvisioningCount: 1, + }); + + // Every other node is a locked child + for (let i = 0; i < 7; i++) { + expect(s(map, `l1-${i}`)).toMatchObject({ isLockedChild: true, isDeployingRoot: false }); + } + for (let i = 0; i < 42; i++) { + expect(s(map, `l2-${i}`)).toMatchObject({ isLockedChild: true, isDeployingRoot: false }); + } + }); +}); diff --git a/canvas/src/components/canvas/useCanvasViewport.ts b/canvas/src/components/canvas/useCanvasViewport.ts index b8007f1de..3ebd3a028 100644 --- a/canvas/src/components/canvas/useCanvasViewport.ts +++ b/canvas/src/components/canvas/useCanvasViewport.ts @@ -1,6 +1,6 @@ "use client"; -import { useCallback, useEffect, useRef } from "react"; +import { useCallback, useEffect, useMemo, useRef } from "react"; import { useReactFlow } from "@xyflow/react"; import { useCanvasStore } from "@/store/canvas"; import { appendClass, removeClass } from "@/store/classNames"; @@ -153,10 +153,17 @@ export function useCanvasViewport() { // fit, the user has to manually pan + zoom to find what they just // created. Only fires when TRANSITIONING from some-provisioning to // zero-provisioning — not on every re-render. - const provisioningCount = useCanvasStore( - (s) => s.nodes.filter((n) => n.data.status === "provisioning").length, + // + // Selecting `nodes` stably (array reference) avoids the + // `.filter().length` anti-pattern which creates a new number on every + // store update and breaks the wasProvisioning/hasProvisioning + // transition detection (React error #185 / Zustand + React 19). + const nodes = useCanvasStore((s) => s.nodes); + const provisioningCount = useMemo( + () => nodes.filter((n) => n.data.status === "provisioning").length, + [nodes], ); - const nodeCount = useCanvasStore((s) => s.nodes.length); + const nodeCount = nodes.length; useEffect(() => { const hasProvisioning = provisioningCount > 0; diff --git a/canvas/src/components/mobile/MobileChat.tsx b/canvas/src/components/mobile/MobileChat.tsx index a7078255b..aa76c4e8c 100644 --- a/canvas/src/components/mobile/MobileChat.tsx +++ b/canvas/src/components/mobile/MobileChat.tsx @@ -5,7 +5,7 @@ // that the desktop ChatTab uses, but with a slimmer surface: no // attachments, no A2A topology overlay, no conversation tracing. -import { useEffect, useRef, useState } from "react"; +import { useEffect, useMemo, useRef, useState } from "react"; import { api } from "@/lib/api"; import { useCanvasStore } from "@/store/canvas"; @@ -49,7 +49,10 @@ export function MobileChat({ onBack: () => void; }) { const p = usePalette(dark); - const node = useCanvasStore((s) => s.nodes.find((n) => n.id === agentId)); + // Selecting `nodes` stably avoids the `.find()` anti-pattern that + // creates a new return value on every store update (React error #185). + const nodes = useCanvasStore((s) => s.nodes); + const node = useMemo(() => nodes.find((n) => n.id === agentId), [nodes, agentId]); // Bootstrap from the canvas store's per-workspace message buffer so the // user sees their prior thread on entry. The store is updated by the // socket → ChatTab flows the desktop runs; on mobile we read from the diff --git a/canvas/src/components/mobile/MobileDetail.tsx b/canvas/src/components/mobile/MobileDetail.tsx index 5d5e9f0a6..0de1bd2ce 100644 --- a/canvas/src/components/mobile/MobileDetail.tsx +++ b/canvas/src/components/mobile/MobileDetail.tsx @@ -2,7 +2,7 @@ // 03 · Agent detail — pills + tabbed content (Overview/Activity/Config/Memory). -import { useEffect, useState } from "react"; +import { useEffect, useMemo, useState } from "react"; import { api } from "@/lib/api"; import { useCanvasStore } from "@/store/canvas"; @@ -32,7 +32,10 @@ export function MobileDetail({ onChat: () => void; }) { const p = usePalette(dark); - const node = useCanvasStore((s) => s.nodes.find((n) => n.id === agentId)); + // Selecting `nodes` stably avoids the `.find()` anti-pattern that + // creates a new return value on every store update (React error #185). + const nodes = useCanvasStore((s) => s.nodes); + const node = useMemo(() => nodes.find((n) => n.id === agentId), [nodes, agentId]); const [tab, setTab] = useState("overview"); if (!node) { diff --git a/canvas/src/components/tabs/BudgetSection.tsx b/canvas/src/components/tabs/BudgetSection.tsx index f2e5d535f..2cad3f956 100644 --- a/canvas/src/components/tabs/BudgetSection.tsx +++ b/canvas/src/components/tabs/BudgetSection.tsx @@ -243,7 +243,7 @@ export function BudgetSection({ workspaceId }: Props) { onClick={handleSave} disabled={saving} data-testid="budget-save-btn" - className="px-4 py-1.5 bg-accent-strong hover:bg-accent active:bg-accent-strong rounded-lg text-xs font-medium text-white disabled:opacity-50 transition-colors" + className="px-4 py-1.5 bg-accent-strong hover:bg-accent active:bg-accent-strong rounded-lg text-xs font-medium text-white disabled:opacity-50 transition-colors focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-1 focus-visible:ring-offset-zinc-900" > {saving ? "Saving…" : "Save"} diff --git a/canvas/src/components/tabs/ChannelsTab.tsx b/canvas/src/components/tabs/ChannelsTab.tsx index 676b05486..1abc1f288 100644 --- a/canvas/src/components/tabs/ChannelsTab.tsx +++ b/canvas/src/components/tabs/ChannelsTab.tsx @@ -255,7 +255,7 @@ export function ChannelsTab({ workspaceId }: Props) { @@ -308,7 +308,7 @@ export function ChannelsTab({ workspaceId }: Props) { diff --git a/canvas/src/components/tabs/ScheduleTab.tsx b/canvas/src/components/tabs/ScheduleTab.tsx index ae7ac5aa4..b25fbf1d6 100644 --- a/canvas/src/components/tabs/ScheduleTab.tsx +++ b/canvas/src/components/tabs/ScheduleTab.tsx @@ -194,7 +194,7 @@ export function ScheduleTab({ workspaceId }: Props) { @@ -339,7 +339,7 @@ export function ScheduleTab({ workspaceId }: Props) { ? "Last run OK — click to disable" : "Never run — click to enable" } - className={`w-2 h-2 rounded-full flex-shrink-0 ${ + className={`w-2 h-2 rounded-full flex-shrink-0 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-accent focus-visible:ring-offset-1 focus-visible:ring-offset-zinc-900 ${ sched.last_status === "error" ? "bg-red-400" : sched.last_status === "ok" @@ -376,7 +376,7 @@ export function ScheduleTab({ workspaceId }: Props) {