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:
@@ -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 `<tr>` per workspace on a single server (instance), labelled
|
||||
* `<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
|
||||
? "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<FleetRowMetrics | null>(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 (
|
||||
|
||||
94
packages/web/src/test/server-workspace-rows.test.tsx
Normal file
94
packages/web/src/test/server-workspace-rows.test.tsx
Normal 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" })),
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user