fix(dashboard): use primitive deps in onMetrics effect to break render loop

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.
This commit is contained in:
Offending Commit
2026-06-03 17:41:57 -05:00
parent e59bfd908a
commit 9cc8637dc7
2 changed files with 102 additions and 23 deletions

View File

@@ -1,7 +1,7 @@
import { useQueries } from "@tanstack/react-query"; import { useQueries } from "@tanstack/react-query";
import { motion } from "framer-motion"; import { motion } from "framer-motion";
import { ChevronRight, CircleDot } from "lucide-react"; import { ChevronRight, CircleDot } from "lucide-react";
import { useEffect, useMemo, useRef } from "react"; import { useEffect, useMemo } from "react";
import { import {
scopedConclusionsCountOptions, scopedConclusionsCountOptions,
scopedQueueStatusOptions, scopedQueueStatusOptions,
@@ -28,16 +28,6 @@ interface Props {
const WORKSPACE_PAGE_SIZE = 100; 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 `<tr>` per workspace on a single server (instance), labelled * Renders one `<tr>` per workspace on a single server (instance), labelled
* `<workspace> (<server>)`, and reports the server's summed metrics up so the * `<workspace> (<server>)`, and reports the server's summed metrics up so the
@@ -80,24 +70,19 @@ export function ServerWorkspaceRows({ instance, onOpenWorkspace, onMetrics }: Pr
: workspacesQ.isSuccess : workspacesQ.isSuccess
? "ok" ? "ok"
: "loading"; : "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, workspaceCount: totalWorkspaces,
conclusionCount, conclusionCount,
queueActive, queueActive,
queuePending, queuePending,
lastSeen: null, lastSeen: null,
health, health,
}), });
[totalWorkspaces, conclusionCount, queueActive, queuePending, health], }, [instance.id, totalWorkspaces, conclusionCount, queueActive, queuePending, health, onMetrics]);
);
const lastReported = useRef<FleetRowMetrics | null>(null);
useEffect(() => {
if (lastReported.current && metricsEqual(lastReported.current, metrics)) return;
lastReported.current = metrics;
onMetrics(instance.id, metrics);
}, [instance.id, metrics, onMetrics]);
if (workspacesQ.isError) { if (workspacesQ.isError) {
return ( return (

View File

@@ -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<typeof vi.fn>) {
const qc = makeQc();
return render(
<QueryClientProvider client={qc}>
<DemoProvider>
<table>
<tbody>
<ServerWorkspaceRows
instance={instance}
onOpenWorkspace={vi.fn()}
onMetrics={onMetrics}
/>
</tbody>
</table>
</DemoProvider>
</QueryClientProvider>,
);
}
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" })),
);
});
});