diff --git a/ui/playwright/tests/chat/agent-rail.spec.ts b/ui/playwright/tests/chat/agent-rail.spec.ts index 7eb076211d..d30cb6e0da 100644 --- a/ui/playwright/tests/chat/agent-rail.spec.ts +++ b/ui/playwright/tests/chat/agent-rail.spec.ts @@ -146,6 +146,57 @@ test("chat agent rail: the identity card switches agent", async ({ page }) => { await expect(page).toHaveURL(/\/agents\/[^/]+\/[^/]+\/new$/); }); +/** + * One loading state, and a visible one. + * + * The switcher read agents with an empty namespace list while namespaces were still on + * their way, so it claimed the cluster had no other agents, then went blank, then listed + * them. Holding the read fixed the false empty message, but the placeholder that held + * the panel's height was painted in the panel's own colour — so the panel still opened + * blank and filled a moment later. + * + * `slow` holds every read long enough to see the frame the bug lived in, which is what + * makes the middle state assertable rather than a matter of timing. + */ +test("chat agent rail: the switcher shows a visible loading skeleton, not an empty cluster", async ({ + page, +}) => { + await page.goto(withScenario(AGENT_CHAT, "slow")); + await expect(page.getByTestId("agent-rail-identity")).toBeVisible({ timeout: 30_000 }); + await page.getByTestId("agent-rail-identity").click(); + + const switcher = page.getByTestId("agent-switcher"); + await expect(switcher).toBeVisible(); + + await test.step("1. the wait is a skeleton, not a claim that the cluster is empty", async () => { + const placeholder = switcher.getByTestId("agent-switcher-loading").first(); + await expect(placeholder).toBeVisible(); + await expect(switcher.getByTestId("agent-switcher-empty")).toHaveCount(0); + + /* + * Drawn, not merely present. A box in the panel's own colour has a size and no + * paint, which is exactly the state this replaced, so what is asserted is the + * fill and the shimmer rather than the element. + */ + const paint = await placeholder.locator(".ant-skeleton").first().evaluate((el) => { + const box = el.firstElementChild ?? el; + const style = getComputedStyle(box); + return { fill: style.backgroundImage, animation: style.animationName }; + }); + expect(paint.fill, "a placeholder with no fill is a blank panel").not.toBe("none"); + expect(paint.animation, "a still placeholder reads as content that failed").not.toBe( + "none", + ); + }); + + await test.step("2. the list replaces it, and the skeleton goes", async () => { + await expect( + switcher.locator('[data-testid^="agent-switcher-option-"]').first(), + ).toBeVisible({ timeout: 30_000 }); + await expect(switcher.getByTestId("agent-switcher-loading")).toHaveCount(0); + }); +}); + /** * The rail is sticky and so is the header, and the header draws on top. * diff --git a/ui/src/components/agent/AgentSwitcher.test.tsx b/ui/src/components/agent/AgentSwitcher.test.tsx new file mode 100644 index 0000000000..cd6a9b37fa --- /dev/null +++ b/ui/src/components/agent/AgentSwitcher.test.tsx @@ -0,0 +1,178 @@ +import { render, screen } from "@testing-library/react"; +import { ThemeProvider } from "@emotion/react"; +import { MemoryRouter } from "react-router-dom"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { Agent, ApiResource, NamespaceResponse } from "@/api"; +import { ApiError } from "@/api/ApiError"; +import { themeFor } from "@/theme/theme"; +import { AgentSwitcher } from "./AgentSwitcher"; + +const { useNamespaces, useAgentsAcrossNamespaces } = vi.hoisted(() => ({ + useNamespaces: vi.fn(), + useAgentsAcrossNamespaces: vi.fn(), +})); + +vi.mock("@/api", () => ({ + useNamespaces, + useAgentsAcrossNamespaces, + harnessRefName: (agent: Agent) => agent.resource.spec.harnessRef.name, +})); + +type Definitions = { agents: Agent[]; refused: { namespace: string; reason: string }[] }; + +const namespaces: NamespaceResponse[] = [{ name: "kagent", status: "Active" }]; + +function agent(namespace: string, name: string, harness: string): Agent { + return { + ref: `${namespace}/${name}`, + namespace, + name, + resource: { + metadata: { name }, + spec: { template: { ref: { name: "assistant" } }, harnessRef: { name: harness } }, + }, + }; +} + +const CURRENT = agent("kagent", "k8s", "default"); +const OTHER = agent("kagent", "reporting", "default"); + +function loaded(data: T): ApiResource { + return { + data, + isLoading: false, + isValidating: false, + error: undefined, + isEmpty: Array.isArray(data) ? data.length === 0 : false, + refresh: async () => {}, + }; +} + +function reading(): ApiResource { + return { + data: undefined, + isLoading: true, + isValidating: true, + error: undefined, + isEmpty: false, + refresh: async () => {}, + }; +} + +/** A read held back because its key is not ready: SWR reports this with isLoading false. */ +function held(): ApiResource { + return { + data: undefined, + isLoading: false, + isValidating: false, + error: undefined, + isEmpty: false, + refresh: async () => {}, + }; +} + +function failing(message: string): ApiResource { + return { + ...held(), + error: new ApiError(message, { kind: "http", url: "kagent.test/Read", status: 500 }), + }; +} + +function renderSwitcher() { + return render( + + + {}} /> + + , + ); +} + +afterEach(() => { + vi.clearAllMocks(); +}); + +describe("AgentSwitcher", () => { + it("holds the agents read back until the namespaces are known", () => { + useNamespaces.mockReturnValue(reading()); + useAgentsAcrossNamespaces.mockReturnValue(held()); + + renderSwitcher(); + + expect(useAgentsAcrossNamespaces).toHaveBeenCalledWith(undefined); + }); + + it("shows a placeholder while the namespaces are read, not an empty cluster", () => { + useNamespaces.mockReturnValue(reading()); + useAgentsAcrossNamespaces.mockReturnValue(held()); + + renderSwitcher(); + + const placeholders = screen.getAllByTestId("agent-switcher-loading"); + expect(placeholders.length).toBeGreaterThan(0); + // Visible, not just height: the placeholder used to be painted in the panel's own + // colour, so it held the shape and still read as an empty panel. + expect(placeholders[0].querySelector(".ant-skeleton-active")).not.toBeNull(); + expect(screen.queryByTestId("agent-switcher-empty")).toBeNull(); + }); + + it("keeps the placeholder while the agents are read", () => { + useNamespaces.mockReturnValue(loaded(namespaces)); + useAgentsAcrossNamespaces.mockReturnValue(reading()); + + renderSwitcher(); + + expect(screen.getAllByTestId("agent-switcher-loading").length).toBeGreaterThan(0); + expect(screen.queryByTestId("agent-switcher-empty")).toBeNull(); + }); + + it("lists the other agents once both reads land, without the one you are on", () => { + useNamespaces.mockReturnValue(loaded(namespaces)); + useAgentsAcrossNamespaces.mockReturnValue( + loaded({ agents: [CURRENT, OTHER], refused: [] }), + ); + + renderSwitcher(); + + expect(screen.getByTestId(`agent-switcher-option-${OTHER.name}`)).toBeInTheDocument(); + expect(screen.queryByTestId(`agent-switcher-option-${CURRENT.name}`)).toBeNull(); + expect(screen.queryByTestId("agent-switcher-loading")).toBeNull(); + expect(screen.queryByTestId("agent-switcher-empty")).toBeNull(); + }); + + it("says the cluster is empty only after both reads have said so", () => { + useNamespaces.mockReturnValue(loaded(namespaces)); + useAgentsAcrossNamespaces.mockReturnValue(loaded({ agents: [], refused: [] })); + + renderSwitcher(); + + expect(screen.getByTestId("agent-switcher-empty")).toHaveTextContent( + "No other agents on this cluster.", + ); + expect(screen.queryByTestId("agent-switcher-loading")).toBeNull(); + }); + + it("reports a failed agents read as a failure, not as an empty cluster", () => { + useNamespaces.mockReturnValue(loaded(namespaces)); + useAgentsAcrossNamespaces.mockReturnValue(failing("the read failed")); + + renderSwitcher(); + + expect(screen.getByTestId("agent-switcher-error")).toHaveTextContent( + "Could not list agents. the read failed", + ); + expect(screen.queryByTestId("agent-switcher-empty")).toBeNull(); + }); + + it("reports a failed namespaces read rather than an empty cluster", () => { + useNamespaces.mockReturnValue(failing("no namespaces")); + useAgentsAcrossNamespaces.mockReturnValue(held()); + + renderSwitcher(); + + expect(screen.getByTestId("agent-switcher-error")).toHaveTextContent( + "Could not list agents. no namespaces", + ); + expect(screen.queryByTestId("agent-switcher-empty")).toBeNull(); + }); +}); diff --git a/ui/src/components/agent/AgentSwitcher.tsx b/ui/src/components/agent/AgentSwitcher.tsx index 915fd15670..c6b3d4c74b 100644 --- a/ui/src/components/agent/AgentSwitcher.tsx +++ b/ui/src/components/agent/AgentSwitcher.tsx @@ -1,6 +1,6 @@ import { useEffect, useMemo, useRef, useState } from "react"; import { useNavigate } from "react-router-dom"; -import { Input, Typography } from "antd"; +import { Input, Skeleton, Typography } from "antd"; import { useTheme } from "@emotion/react"; import { Search } from "lucide-react"; import { @@ -45,11 +45,15 @@ export function AgentSwitcher({ const navigate = useNavigate(); const namespaces = useNamespaces(); const namespaceNames = useMemo( - () => (namespaces.data ?? []).map((entry) => entry.name), + () => namespaces.data?.map((entry) => entry.name), [namespaces.data], ); const definitions = useAgentsAcrossNamespaces(namespaceNames); + /** Either read failing means the list is unknown, not empty. */ + const readError = namespaces.error ?? definitions.error; + /** One loading state while the namespaces or the agents are on their way. */ + const isReading = namespaces.isLoading || definitions.isLoading; const agents = useMemo(() => definitions.data?.agents ?? [], [definitions.data]); const [query, setQuery] = useState(""); @@ -149,9 +153,9 @@ export function AgentSwitcher({ css={searchInputStyles(theme)} /> - {definitions.error ? ( + {readError ? ( - Could not list agents. {definitions.error.message} + Could not list agents. {readError.message} ) : null} @@ -265,31 +269,22 @@ export function AgentSwitcher({ })} {/* - Rows of the same size as the real ones while they are on their way. + Rows of the same size and shape as the real ones while they are on their way. The list rendered nothing at all until the agents arrived, so the panel opened at the height of its search field and then jumped to the height of a list — under a pointer that was already moving toward where the first row was about to be. Standing in for three rows is enough to hold the shape: the panel scrolls beyond that anyway, so being wrong about the count costs nothing. + + Drawn as skeleton rows rather than as boxes holding height. A box painted in + the panel's own colour holds the shape but cannot be seen, so the panel still + opened blank and filled a moment later — the same blank beat this is meant to + remove, in a quieter form. */} - {definitions.isLoading - ? [0, 1, 2].map((row) => ( -
- )) - : null} + {isReading ? [0, 1, 2].map((row) => ) : null} - {!definitions.isLoading && matches.length === 0 ? ( + {!isReading && !readError && matches.length === 0 ? ( ); } + +/** + * One agent row, drawn as shimmering boxes while the list is on its way. + * + * Deliberately built from the same pieces as a real row — the same padding, radius, + * badge size and two lines of text — so the list that replaces it lands where the + * placeholder was instead of growing under the pointer. antd's skeleton carries the + * fill and the animation, so it is visible on either theme's panel and needs no + * colour of its own. + */ +function LoadingRow() { + const theme = useTheme(); + + return ( +
+ + {/* Widths differ so the pair reads as a name and its subtitle rather than as a + block; both are narrower than the row so no bar reaches the scrollbar. + + `display: flex` on the wrapper is not cosmetic: left as an inline block, each + bar sits in a line box whose strut is taller than the bar, and the row comes + out six pixels taller than the one it stands in for. */} + + +
+ ); +}