From 9cc8637dc7ebf62b2002a0a3d8e4b3781660e770 Mon Sep 17 00:00:00 2001 From: Offending Commit Date: Wed, 3 Jun 2026 17:41:57 -0500 Subject: [PATCH] fix(dashboard): use primitive deps in onMetrics effect to break render loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Using a metrics object as a useEffect dep causes the loop: onMetrics → setMetricsById → Dashboard re-renders → ServerWorkspaceRows re-renders → useQueries runs → TanStack Query cache subscriber fires (Sidebar's setNow) → query result objects are new references → metrics useMemo returns new object → effect dep changed → onMetrics again → ∞ Fix: depend on the five primitive values (workspaceCount, conclusionCount, queueActive, queuePending, health) directly. React compares primitives by value, so the effect only fires when actual data changes, not on reference churn from re-renders. Also adds server-workspace-rows.test.tsx with three focused unit tests: correct health:ok report, stability after load (no re-fire), and health transition coverage. --- .../dashboard/ServerWorkspaceRows.tsx | 31 ++---- .../src/test/server-workspace-rows.test.tsx | 94 +++++++++++++++++++ 2 files changed, 102 insertions(+), 23 deletions(-) create mode 100644 packages/web/src/test/server-workspace-rows.test.tsx diff --git a/packages/web/src/components/dashboard/ServerWorkspaceRows.tsx b/packages/web/src/components/dashboard/ServerWorkspaceRows.tsx index 4b90372..7204d5b 100644 --- a/packages/web/src/components/dashboard/ServerWorkspaceRows.tsx +++ b/packages/web/src/components/dashboard/ServerWorkspaceRows.tsx @@ -1,7 +1,7 @@ import { useQueries } from "@tanstack/react-query"; import { motion } from "framer-motion"; import { ChevronRight, CircleDot } from "lucide-react"; -import { useEffect, useMemo, useRef } from "react"; +import { useEffect, useMemo } from "react"; import { scopedConclusionsCountOptions, scopedQueueStatusOptions, @@ -28,16 +28,6 @@ interface Props { const WORKSPACE_PAGE_SIZE = 100; -function metricsEqual(a: FleetRowMetrics, b: FleetRowMetrics): boolean { - return ( - a.workspaceCount === b.workspaceCount && - a.conclusionCount === b.conclusionCount && - a.queueActive === b.queueActive && - a.queuePending === b.queuePending && - a.health === b.health - ); -} - /** * Renders one `` per workspace on a single server (instance), labelled * ` ()`, and reports the server's summed metrics up so the @@ -80,24 +70,19 @@ export function ServerWorkspaceRows({ instance, onOpenWorkspace, onMetrics }: Pr : workspacesQ.isSuccess ? "ok" : "loading"; - const metrics: FleetRowMetrics = useMemo( - () => ({ + // Dep array uses primitives only — an object dep (e.g. the metrics shape) would + // create a new reference on each render even when values are unchanged, causing + // onMetrics → setMetricsById → re-render → new object → onMetrics … loop. + useEffect(() => { + onMetrics(instance.id, { workspaceCount: totalWorkspaces, conclusionCount, queueActive, queuePending, lastSeen: null, health, - }), - [totalWorkspaces, conclusionCount, queueActive, queuePending, health], - ); - - const lastReported = useRef(null); - useEffect(() => { - if (lastReported.current && metricsEqual(lastReported.current, metrics)) return; - lastReported.current = metrics; - onMetrics(instance.id, metrics); - }, [instance.id, metrics, onMetrics]); + }); + }, [instance.id, totalWorkspaces, conclusionCount, queueActive, queuePending, health, onMetrics]); if (workspacesQ.isError) { return ( diff --git a/packages/web/src/test/server-workspace-rows.test.tsx b/packages/web/src/test/server-workspace-rows.test.tsx new file mode 100644 index 0000000..a131d02 --- /dev/null +++ b/packages/web/src/test/server-workspace-rows.test.tsx @@ -0,0 +1,94 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { act, render, waitFor } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { ServerWorkspaceRows } from "@/components/dashboard/ServerWorkspaceRows"; +import { DemoProvider } from "@/context/DemoContext"; +import type { Instance } from "@/lib/config"; + +vi.mock("@/lib/http", () => ({ + httpFetch: vi.fn(async (input: Request | string) => { + const url = typeof input === "string" ? input : input.url; + const json = (body: unknown) => + new Response(JSON.stringify(body), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + if (url.includes("/queue/status")) { + return json({ in_progress_work_units: 0, pending_work_units: 0 }); + } + if (url.includes("/conclusions/list")) { + return json({ items: [], total: 3, page: 1, size: 1, pages: 1 }); + } + return json({ items: [{ id: "ws-1" }], total: 1, page: 1, size: 100, pages: 1 }); + }), +})); + +const neo: Instance = { id: "neo", name: "Neo", baseUrl: "https://neo.example.net", token: "" }; + +function makeQc() { + return new QueryClient({ defaultOptions: { queries: { retry: false, staleTime: Infinity } } }); +} + +function renderRows(instance: Instance, onMetrics: ReturnType) { + const qc = makeQc(); + return render( + + + + + + +
+ + , + ); +} + +describe("ServerWorkspaceRows — onMetrics stability", () => { + afterEach(() => localStorage.clear()); + + it("calls onMetrics with health:ok after data loads", async () => { + const onMetrics = vi.fn(); + renderRows(neo, onMetrics); + await waitFor(() => + expect(onMetrics).toHaveBeenCalledWith( + "neo", + expect.objectContaining({ health: "ok", workspaceCount: 1, conclusionCount: 3 }), + ), + ); + }); + + it("does not call onMetrics again when values have not changed", async () => { + const onMetrics = vi.fn(); + renderRows(neo, onMetrics); + + // Wait until we have at least one call with stable state + await waitFor(() => + expect(onMetrics).toHaveBeenCalledWith("neo", expect.objectContaining({ health: "ok" })), + ); + + const callsBefore = onMetrics.mock.calls.length; + + // Flush any pending micro-tasks / React batched updates + await act(async () => { + await new Promise((r) => setTimeout(r, 50)); + }); + + // onMetrics must not have been called again — no render loop + expect(onMetrics).toHaveBeenCalledTimes(callsBefore); + }); + + it("calls onMetrics when health transitions from loading to ok", async () => { + const onMetrics = vi.fn(); + renderRows(neo, onMetrics); + + // Must eventually report ok (not just loading) + await waitFor(() => + expect(onMetrics).toHaveBeenCalledWith("neo", expect.objectContaining({ health: "ok" })), + ); + }); +});