From f2bb8703f8cdbea611e78aad0ef0577061387e7f Mon Sep 17 00:00:00 2001 From: ryan-crabbe-berri Date: Wed, 16 Sep 2026 11:33:25 -0700 Subject: [PATCH] fix(ui): address review on workflows-tool-policies url state Treat an empty ?run= as closed so a bare link lands on the list, drop a section banner comment, and harden the tests: tool policy sort tests use data whose order differs from the default sort, the ?run= deep link is checked while the list is still loading, and page resets, page_size, Escape close and run id encoding are covered. --- .../workflows/WorkflowRuns.test.tsx | 90 +++++++++++++++++++ .../(dashboard)/workflows/WorkflowRuns.tsx | 9 +- .../ToolPolicies/ToolPoliciesTable.test.tsx | 78 ++++++++++++++-- 3 files changed, 166 insertions(+), 11 deletions(-) diff --git a/ui/litellm-dashboard/src/app/(dashboard)/workflows/WorkflowRuns.test.tsx b/ui/litellm-dashboard/src/app/(dashboard)/workflows/WorkflowRuns.test.tsx index d1236bc44e8..3281bea07d8 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/workflows/WorkflowRuns.test.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/workflows/WorkflowRuns.test.tsx @@ -386,6 +386,42 @@ describe("WorkflowRuns URL table state", () => { await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("page")).toBe("2")); expect(rowIds()[0]).toBe("run-50"); }); + + it("writes the page size chosen in the pager to the URL", async () => { + const user = userEvent.setup(); + const { onUrlUpdate } = renderRuns(MANY_RUNS); + await screen.findByText("Run number 0"); + + await chooseSelectOption(user, screen.getByTestId("pagination-page-size"), "100"); + + await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("page_size")).toBe("100")); + expect(rowIds()).toHaveLength(60); + }); + + it("drops the page from the URL when the search changes", async () => { + const { onUrlUpdate } = renderRuns(MANY_RUNS, "?page=2"); + await waitFor(() => expect(screen.getByTestId("pagination-page")).toHaveTextContent("Page 2 of 2")); + + fireEvent.change(screen.getByTestId("datatable-search"), { target: { value: "Run number" } }); + + await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("search")).toBe("Run number")); + expect(lastUrlUpdate(onUrlUpdate)?.searchParams.has("page")).toBe(false); + expect(screen.getByTestId("pagination-page")).toHaveTextContent("Page 1 of 2"); + }); + + it("drops the page from the URL when a drawer filter is applied", async () => { + const user = userEvent.setup(); + const { onUrlUpdate } = renderRuns(MANY_RUNS, "?page=2"); + await waitFor(() => expect(screen.getByTestId("pagination-page")).toHaveTextContent("Page 2 of 2")); + + await user.click(screen.getByTestId("datatable-filters-trigger")); + await user.type(await screen.findByPlaceholderText("Filter by type…"), "grill"); + await user.click(screen.getByTestId("filter-drawer-apply")); + + await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("filter_type")).toBe("grill")); + expect(lastUrlUpdate(onUrlUpdate)?.searchParams.has("page")).toBe(false); + expect(screen.getByTestId("pagination-page")).toHaveTextContent("Page 1 of 2"); + }); }); describe("WorkflowRuns ?run= drawer", () => { @@ -429,6 +465,60 @@ describe("WorkflowRuns ?run= drawer", () => { expect(await within(drawer).findByText("Workflow run not found.")).toBeInTheDocument(); expect(detailFetchUrls(fetchSpy)).toEqual([]); }); + + it("keeps the drawer closed when the run param is empty", async () => { + const { fetchSpy } = renderRuns(RUNS, "?run="); + + await screen.findByText("First run"); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + expect(detailFetchUrls(fetchSpy)).toEqual([]); + }); + + it("shows a spinner rather than not-found while the runs list is still loading", async () => { + const listGate = Promise.withResolvers(); + const fallback = mockFetch(RUNS); + vi.stubGlobal( + "fetch", + vi.fn(async (url: string, init: RequestInit) => { + if (url.includes("/runs?limit")) await listGate.promise; + return fallback(url, init); + }), + ); + renderWithProviders(, { searchParams: "?run=run-bbbbbbbb-2222" }); + + const drawer = await screen.findByRole("dialog"); + expect(within(drawer).queryByText("Workflow run not found.")).not.toBeInTheDocument(); + expect(within(drawer).queryByText("Timeline")).not.toBeInTheDocument(); + + listGate.resolve(); + + await waitFor(() => expect(within(drawer).getByText("Timeline")).toBeInTheDocument()); + expect(within(drawer).queryByText("Workflow run not found.")).not.toBeInTheDocument(); + }); + + it("removes the run from the URL when the drawer is dismissed with Escape", async () => { + const user = userEvent.setup(); + const { onUrlUpdate } = renderRuns(RUNS, "?run=run-aaaaaaaa-1111"); + const drawer = await screen.findByRole("dialog"); + await waitFor(() => expect(within(drawer).getByText("Timeline")).toBeInTheDocument()); + + await user.keyboard("{Escape}"); + + await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.has("run")).toBe(false)); + await waitFor(() => expect(screen.queryAllByRole("dialog")).toHaveLength(0)); + }); + + it("encodes the run id in the detail request paths", async () => { + const encodedRun: FakeRun = { ...RUNS[0], run_id: "run/a b", metadata: { title: "Slashed run" } }; + const { fetchSpy } = renderRuns([encodedRun], "?run=run%2Fa%20b"); + + const drawer = await screen.findByRole("dialog"); + await waitFor(() => expect(within(drawer).getByText("Timeline")).toBeInTheDocument()); + expect(detailFetchUrls(fetchSpy)).toEqual([ + "/v1/workflows/runs/run%2Fa%20b/events", + "/v1/workflows/runs/run%2Fa%20b/messages", + ]); + }); }); describe("WorkflowRuns column visibility", () => { diff --git a/ui/litellm-dashboard/src/app/(dashboard)/workflows/WorkflowRuns.tsx b/ui/litellm-dashboard/src/app/(dashboard)/workflows/WorkflowRuns.tsx index 7a1e60de446..bef2483f15a 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/workflows/WorkflowRuns.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/workflows/WorkflowRuns.tsx @@ -419,8 +419,6 @@ const DetailSection: React.FC<{ ); -// ── run detail drawer body ──────────────────────────────────────────────────── - const DrawerSpinner: React.FC = () => (
@@ -521,8 +519,9 @@ const WorkflowRuns: React.FC = ({ accessToken }) => { useUrlTableState(TABLE_STATE_OPTIONS); const { columnVisibility, onColumnVisibilityChange } = usePersistedColumnVisibility("workflow-runs"); const [runId, setRunId] = useQueryState("run", RUN_PARAM); + const drawerOpen = Boolean(runId); const [shownRunId, setShownRunId] = useState(runId); - if (runId !== null && runId !== shownRunId) { + if (runId && runId !== shownRunId) { setShownRunId(runId); } const shownRun = runs.find((run) => run.run_id === shownRunId); @@ -533,7 +532,7 @@ const WorkflowRuns: React.FC = ({ accessToken }) => { accessToken && shownRun !== undefined ? ({ signal }) => fetchRunDetail(accessToken, shownRun.run_id, signal) : skipToken, - enabled: runId !== null, + enabled: drawerOpen, refetchOnWindowFocus: false, retry: false, }; @@ -707,7 +706,7 @@ const WorkflowRuns: React.FC = ({ accessToken }) => { {/* detail drawer */} { if (!open) closeRun(); }} diff --git a/ui/litellm-dashboard/src/components/ToolPolicies/ToolPoliciesTable.test.tsx b/ui/litellm-dashboard/src/components/ToolPolicies/ToolPoliciesTable.test.tsx index f65b5aa18da..d4300b79d36 100644 --- a/ui/litellm-dashboard/src/components/ToolPolicies/ToolPoliciesTable.test.tsx +++ b/ui/litellm-dashboard/src/components/ToolPolicies/ToolPoliciesTable.test.tsx @@ -4,7 +4,7 @@ import { fireEvent, screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import type { OnUrlUpdateFunction } from "nuqs/adapters/testing"; -import { renderWithProviders } from "../../../tests/test-utils"; +import { chooseSelectOption, renderWithProviders } from "../../../tests/test-utils"; import type { ToolRow } from "@/components/networking"; import { ToolPoliciesTable } from "./ToolPoliciesTable"; @@ -43,6 +43,10 @@ const TOOLS: ToolRow[] = [ }, ]; +const SHUFFLED_TOOLS: ToolRow[] = [TOOLS[1], TOOLS[2], TOOLS[0]]; + +const SAME_TIME_TOOLS: ToolRow[] = SHUFFLED_TOOLS.map((tool) => ({ ...tool, created_at: "2026-07-20T10:00:00Z" })); + const renderTable = ( overrides: Partial> = {}, urlOptions: Parameters[1] = {}, @@ -77,7 +81,7 @@ const pickFilter = async ( describe("ToolPoliciesTable sorting", () => { it("should default to newest discovered first", () => { - renderTable(); + renderTable({ data: SHUFFLED_TOOLS }); expect(rowIds()).toEqual(["tool-1", "tool-2", "tool-3"]); }); @@ -278,14 +282,25 @@ describe("ToolPoliciesTable URL state", () => { expect(rowIds()).toEqual(["tool-1", "tool-2", "tool-3"]); }); - it("orders rows by the sort in the URL", () => { - renderWithUrl("?sort_by=call_count&sort_order=desc"); + it("orders rows by the sort column and direction in the URL", () => { + renderWithUrl("?sort_by=call_count&sort_order=asc"); - expect(rowIds()).toEqual(["tool-3", "tool-1", "tool-2"]); + expect(rowIds()).toEqual(["tool-2", "tool-1", "tool-3"]); + }); + + it.each([ + ["input_policy", "asc", ["tool-3", "tool-2", "tool-1"]], + ["output_policy", "desc", ["tool-3", "tool-1", "tool-2"]], + ["team_id", "asc", ["tool-3", "tool-1", "tool-2"]], + ["key_alias", "asc", ["tool-3", "tool-2", "tool-1"]], + ])("orders rows by ?sort_by=%s&sort_order=%s", (column, order, expected) => { + renderWithUrl(`?sort_by=${column}&sort_order=${order}`, SAME_TIME_TOOLS); + + expect(rowIds()).toEqual(expected); }); it("falls back to newest first for an unknown sort column", () => { - renderWithUrl("?sort_by=user_agent&sort_order=desc"); + renderWithUrl("?sort_by=user_agent&sort_order=desc", SHUFFLED_TOOLS); expect(rowIds()).toEqual(["tool-1", "tool-2", "tool-3"]); }); @@ -300,6 +315,40 @@ describe("ToolPoliciesTable URL state", () => { expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("sort_order")).toBe("asc"); }); + it("drops the page from the URL when the sort changes", async () => { + const user = userEvent.setup(); + const onUrlUpdate = renderWithUrl("?page=2", MANY_TOOLS); + + await user.click(screen.getByTestId("sort-header-tool_name")); + + await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("sort_by")).toBe("tool_name")); + expect(lastUrlUpdate(onUrlUpdate)?.searchParams.has("page")).toBe(false); + expect(screen.getByTestId("pagination-page")).toHaveTextContent("Page 1 of 2"); + }); + + it("drops the page from the URL when the search changes", async () => { + const onUrlUpdate = renderWithUrl("?page=2", MANY_TOOLS); + + fireEvent.change(screen.getByTestId("datatable-search"), { target: { value: "bulk_tool" } }); + + await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("search")).toBe("bulk_tool")); + expect(lastUrlUpdate(onUrlUpdate)?.searchParams.has("page")).toBe(false); + expect(screen.getByTestId("pagination-page")).toHaveTextContent("Page 1 of 2"); + }); + + it("drops the page from the URL when a drawer filter is applied", async () => { + const user = userEvent.setup(); + const onUrlUpdate = renderWithUrl("?page=2", MANY_TOOLS); + + await user.click(screen.getByTestId("datatable-filters-trigger")); + await pickFilter(user, "filter-input-policy", "untrusted"); + await user.click(screen.getByTestId("filter-drawer-apply")); + + await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("filter_input_policy")).toBe("untrusted")); + expect(lastUrlUpdate(onUrlUpdate)?.searchParams.has("page")).toBe(false); + expect(screen.getByTestId("pagination-page")).toHaveTextContent("Page 1 of 2"); + }); + it("opens the page named in the URL", () => { renderWithUrl("?page=2", MANY_TOOLS); @@ -316,4 +365,21 @@ describe("ToolPoliciesTable URL state", () => { await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("page")).toBe("2")); expect(rowIds()[0]).toBe("bulk-50"); }); + + it("uses the page size from the URL", () => { + renderWithUrl("?page_size=100", MANY_TOOLS); + + expect(rowIds()).toHaveLength(55); + expect(screen.getByTestId("pagination-page")).toHaveTextContent("Page 1 of 1"); + }); + + it("writes the page size chosen in the pager to the URL", async () => { + const user = userEvent.setup(); + const onUrlUpdate = renderWithUrl("", MANY_TOOLS); + + await chooseSelectOption(user, screen.getByTestId("pagination-page-size"), "100"); + + await waitFor(() => expect(lastUrlUpdate(onUrlUpdate)?.searchParams.get("page_size")).toBe("100")); + expect(rowIds()).toHaveLength(55); + }); });