diff --git a/ui/playwright/tests/chat/panels.spec.ts b/ui/playwright/tests/chat/panels.spec.ts index 389e00430a..c6ef3cd5e0 100644 --- a/ui/playwright/tests/chat/panels.spec.ts +++ b/ui/playwright/tests/chat/panels.spec.ts @@ -108,3 +108,38 @@ test("chat: a conversation that cannot be read gives the column back", async ({ await expect(page.getByTestId("chat-context-collapse")).toBeHidden(); await expect(page.getByTestId("chat-context-expand")).toBeHidden(); }); + +/** + * The context panel's template link, and the way back from the template page. + * + * The template page is shared by every conversation with an agent, so reaching it from a + * chat must not strand the reader on a page whose only exit is the template list. + */ +test("chat: the template opened from a conversation leads back to it", async ({ page }) => { + // Wide enough for the context panel, as above. + await page.setViewportSize({ width: 1600, height: 900 }); + await page.goto(AGENT_CHAT); + const link = page.getByTestId("chat-agent-context-template"); + await expect(link).toBeVisible({ timeout: 30_000 }); + const chatUrl = page.url(); + + await test.step("1. the link says where it goes", async () => { + await link.hover(); + await expect(page.getByRole("tooltip")).toContainText("AgentTemplate"); + }); + + await test.step("2. the template page offers the way back, across its tabs", async () => { + await link.click(); + const back = page.getByTestId("template-back"); + await expect(back).toHaveText("Back to conversation", { timeout: 30_000 }); + + await page.getByRole("tab", { name: /Agents/ }).click(); + await expect(back).toHaveText("Back to conversation"); + }); + + await test.step("3. and it returns to the same conversation", async () => { + await page.getByTestId("template-back").click(); + await expect(page).toHaveURL(chatUrl); + await expect(page.getByTestId("chat-agent-context")).toBeVisible({ timeout: 30_000 }); + }); +}); diff --git a/ui/src/components/chat/AgentContextPanel.test.tsx b/ui/src/components/chat/AgentContextPanel.test.tsx index caa5512015..4216046a07 100644 --- a/ui/src/components/chat/AgentContextPanel.test.tsx +++ b/ui/src/components/chat/AgentContextPanel.test.tsx @@ -1,15 +1,26 @@ import { render, screen } from "@testing-library/react"; import { ThemeProvider } from "@emotion/react"; -import { MemoryRouter } from "react-router-dom"; +import { MemoryRouter, Route, Routes, useLocation } from "react-router-dom"; +import userEvent from "@testing-library/user-event"; import { describe, expect, it, vi } from "vitest"; import { themeFor } from "@/theme/theme"; +import type { AgentInstance } from "@/api"; import { AgentContextPanel } from "./AgentContextPanel"; const {useAgentTemplate, useAgent} = vi.hoisted(() => ({useAgentTemplate: vi.fn(), useAgent: vi.fn()})); -vi.mock("@/api", () => ({ useAgentTemplate, useAgent })); +vi.mock("@/api", () => ({ + useAgentTemplate, + useAgent, + bareName: (ref: string) => ref.split("/").pop(), +})); -function renderPanel(tools: unknown[], modelNamespace?: string) { +function LocationState() { + const location = useLocation(); + return 
{JSON.stringify(location.state)}
; +} + +function renderPanel(tools: unknown[], modelNamespace?: string, instance?: AgentInstance) { useAgent.mockReturnValue({data: {resource: {spec: {template: {ref: {name: "assistant"}}, harnessRef: {name: "runner"}}}}}); useAgentTemplate.mockReturnValue({ data: { @@ -26,16 +37,42 @@ function renderPanel(tools: unknown[], modelNamespace?: string) { render( - - + + + + } + /> + } /> + , ); } describe("AgentContextPanel", () => { + it("opens the template with a way back to this conversation", async () => { + renderPanel([], undefined, { agent: "kagent/assistant" } as AgentInstance); + await userEvent.click(screen.getByTestId("chat-agent-context-template")); + expect(JSON.parse(screen.getByTestId("location-state").textContent ?? "null")).toEqual({ + from: "/agents/abc/chat?mock=ok", + fromLabel: "Back to conversation", + }); + }); + + it("says the way back leads to the agent when no conversation is open", async () => { + renderPanel([]); + await userEvent.click(screen.getByTestId("chat-agent-context-template")); + expect(JSON.parse(screen.getByTestId("location-state").textContent ?? "null")).toMatchObject({ + fromLabel: "Back to agent", + }); + }); + it("renders an MCP binding whose optional tools list is omitted", () => { renderPanel([ { diff --git a/ui/src/components/chat/AgentContextPanel.tsx b/ui/src/components/chat/AgentContextPanel.tsx index 654c5a7219..ef9393cd51 100644 --- a/ui/src/components/chat/AgentContextPanel.tsx +++ b/ui/src/components/chat/AgentContextPanel.tsx @@ -1,9 +1,10 @@ import { referenceLabel } from "@/api/domain/agentTemplates"; -import { Alert, Skeleton, Space, Tag, Typography } from "antd"; +import { Alert, Skeleton, Space, Tag, Tooltip, Typography } from "antd"; import { useTheme } from "@emotion/react"; -import { Link } from "react-router-dom"; +import { Link, useLocation } from "react-router-dom"; import { useAgent, useAgentTemplate, bareName, type AgentInstance } from "@/api"; import { buildPath, paths } from "@/router/routes"; +import { backLinkState } from "@/router/backLink"; const { Text, Paragraph } = Typography; @@ -48,6 +49,7 @@ export function AgentContextPanel({ agentRef?: { namespace: string; name?: string }; }) { const theme = useTheme(); + const location = useLocation(); const namespace = instance?.agent?.split("/")[0] ?? agentRef?.namespace ?? ""; const name = instance?.agent ? bareName(instance.agent) : agentRef?.name; @@ -65,17 +67,33 @@ export function AgentContextPanel({
{templateRef && templateNamespace && templateName ? ( - - + {templateRef} - - + + ) : ( {definition.data?.resource.spec.template.inline ? "Inline" : "Not reported"} )} diff --git a/ui/src/components/mcp/ToolServerTools.test.tsx b/ui/src/components/mcp/ToolServerTools.test.tsx new file mode 100644 index 0000000000..f2206648bf --- /dev/null +++ b/ui/src/components/mcp/ToolServerTools.test.tsx @@ -0,0 +1,35 @@ +import { cleanup, render, screen } from "@testing-library/react"; +import { ThemeProvider } from "@emotion/react"; +import { describe, expect, it } from "vitest"; +import { themeFor } from "@/theme/theme"; +import { ToolServerTools } from "./ToolServerTools"; + +function renderedNames(names: string[]) { + render( + + ({ name, description: "" }))} /> + , + ); + return screen.getAllByTestId("tool-server-tool").map((item) => item.textContent); +} + +describe("ToolServerTools", () => { + it("lists tools alphabetically, whatever order the server reported them in", () => { + expect( + renderedNames(["k8s_get_pods", "K8s_apply", "k8s_describe"]), + ).toEqual(["K8s_apply", "k8s_describe", "k8s_get_pods"]); + }); + + it("orders numbered tools by number rather than character by character", () => { + expect(renderedNames(["tool_10", "tool_2"])).toEqual([ + "tool_2", + "tool_10", + ]); + }); + + it("gives names that differ only in case one fixed order", () => { + const first = renderedNames(["get_pods", "Get_pods"]); + cleanup(); + expect(renderedNames(["Get_pods", "get_pods"])).toEqual(first); + }); +}); diff --git a/ui/src/components/mcp/ToolServerTools.tsx b/ui/src/components/mcp/ToolServerTools.tsx index 4aaa2ec795..0099eeb909 100644 --- a/ui/src/components/mcp/ToolServerTools.tsx +++ b/ui/src/components/mcp/ToolServerTools.tsx @@ -43,7 +43,7 @@ export function ToolServerTools({ tools, highlight }: ToolServerToolsProps) { gap: theme.space(3), }} > - {tools.map((tool) => ( + {sortToolsByName(tools).map((tool) => (
  • + a.name.localeCompare(b.name, undefined, { numeric: true, sensitivity: "base" }) || + // Names that differ only in case still get one fixed order. + a.name.localeCompare(b.name, undefined, { numeric: true }), + ); +} + /** * Wraps occurrences of `term` in a ``. * diff --git a/ui/src/pages/AgentTemplateDetailsPage.tsx b/ui/src/pages/AgentTemplateDetailsPage.tsx index 4278d2b908..0906a98bf2 100644 --- a/ui/src/pages/AgentTemplateDetailsPage.tsx +++ b/ui/src/pages/AgentTemplateDetailsPage.tsx @@ -13,7 +13,7 @@ import { } from "antd"; import type { ColumnsType } from "antd/es/table"; import toast from "react-hot-toast"; -import { Link, useNavigate, useParams, useSearchParams } from "react-router-dom"; +import { Link, useLocation, useNavigate, useParams, useSearchParams } from "react-router-dom"; import { useTheme } from "@emotion/react"; import { Pencil } from "lucide-react"; import { PageFrame } from "@/components/Structure/PageFrame"; @@ -29,6 +29,7 @@ import { type AgentTemplateDraft, } from "@/components/agent-template-form/agentTemplateDraft"; import { agentTemplatesTab } from "@/router/routes"; +import { useBackLink } from "@/router/backLink"; import { apiClient, useAgentConversations, @@ -47,9 +48,13 @@ const TAB_PARAM = "tab"; export function AgentTemplateDetailsPage() { const navigate = useNavigate(); + const location = useLocation(); const theme = useTheme(); const { namespace, name } = useParams(); const [searchParams, setSearchParams] = useSearchParams(); + // Back to wherever the reader came from — a conversation's context panel, say — and + // to the list when they opened this page directly. + const back = useBackLink({ to: agentTemplatesTab, label: "Back to templates" }); const template = useAgentTemplate(namespace, name); const invalidateTemplates = useInvalidateAgentTemplates(); @@ -106,7 +111,8 @@ export function AgentTemplateDetailsPage() { const next = new URLSearchParams(searchParams); if (tab === "details") next.delete(TAB_PARAM); else next.set(TAB_PARAM, tab); - setSearchParams(next, { replace: true }); + // Carry the router state across, or switching tabs drops the way back. + setSearchParams(next, { replace: true, state: location.state }); }; const missing = template.error !== undefined && isNotFound(template.error); @@ -335,7 +341,14 @@ export function AgentTemplateDetailsPage() { description="Agents referencing this template cannot prepare new configuration until it is restored or replaced. Existing conversations and last successful revisions are retained." /> ) : null} - + } > diff --git a/ui/src/router/backLink.test.tsx b/ui/src/router/backLink.test.tsx new file mode 100644 index 0000000000..e11796ac69 --- /dev/null +++ b/ui/src/router/backLink.test.tsx @@ -0,0 +1,46 @@ +import { render, screen } from "@testing-library/react"; +import { MemoryRouter } from "react-router-dom"; +import { describe, expect, it } from "vitest"; +import { backLinkState, useBackLink } from "./backLink"; + +const FALLBACK = { to: "/agents?tab=templates", label: "Back to templates" }; + +function Probe() { + const back = useBackLink(FALLBACK); + return {back.label}; +} + +function renderAt(state: unknown) { + render( + + + , + ); + return screen.getByRole("link"); +} + +describe("useBackLink", () => { + it("leads back to the page that opened this one, search included", () => { + const link = renderAt( + backLinkState({ pathname: "/agents/abc/chat", search: "?mock=ok" }, "Back to conversation"), + ); + expect(link.textContent).toBe("Back to conversation"); + expect(link.getAttribute("href")).toBe("/agents/abc/chat?mock=ok"); + }); + + it("falls back to the list on a direct visit", () => { + const link = renderAt(undefined); + expect(link.textContent).toBe("Back to templates"); + expect(link.getAttribute("href")).toBe(FALLBACK.to); + }); + + it.each([ + { from: "https://example.com/", fromLabel: "Back" }, + { from: "//example.com/", fromLabel: "Back" }, + { from: "/agents", fromLabel: "" }, + { from: 42, fromLabel: "Back" }, + "not an object", + ])("ignores state that is not an in-app origin: %j", (state) => { + expect(renderAt(state).textContent).toBe("Back to templates"); + }); +}); diff --git a/ui/src/router/backLink.ts b/ui/src/router/backLink.ts new file mode 100644 index 0000000000..34dd3458a2 --- /dev/null +++ b/ui/src/router/backLink.ts @@ -0,0 +1,53 @@ +import { useLocation, type Location } from "react-router-dom"; + +/** + * Where a page was opened from, carried in router `state` rather than the URL. + * + * A detail page reached from somewhere other than its own list should lead back to + * that place, not to the list. State rather than a query parameter, because the origin + * is about this visit and not about the page: a copied or bookmarked URL opens the page + * cold, and a cold visit has nowhere to go back to but the list. + */ +export interface BackLinkState { + /** The path and search of the page to return to. */ + from: string; + /** What the back control says, e.g. "Back to conversation". */ + fromLabel: string; +} + +/** The `state` to pass a `Link` so the page it opens can lead back to the current one. */ +export function backLinkState( + location: Pick, + fromLabel: string, +): BackLinkState { + return { from: location.pathname + location.search, fromLabel }; +} + +function isBackLinkState(value: unknown): value is BackLinkState { + if (typeof value !== "object" || value === null) return false; + const { from, fromLabel } = value as Record; + // Only an in-app path: state is set by our own links, but a path is what makes it + // safe to navigate to whatever is found here. + return ( + typeof from === "string" && + from.startsWith("/") && + !from.startsWith("//") && + typeof fromLabel === "string" && + fromLabel.length > 0 + ); +} + +/** + * The page this one was opened from, or the fallback when it was visited directly. + */ +export function useBackLink(fallback: { to: string; label: string }): { + to: string; + label: string; + /** True when `to` is the page this one was opened from, rather than the fallback. */ + isReturn: boolean; +} { + const { state } = useLocation(); + return isBackLinkState(state) + ? { to: state.from, label: state.fromLabel, isReturn: true } + : { ...fallback, isReturn: false }; +}