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.
This commit is contained in:
ryan-crabbe-berri 2026-09-16 11:33:25 -07:00
parent 06d9802ed6
commit f2bb8703f8
3 changed files with 166 additions and 11 deletions

View file

@ -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<void>();
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(<WorkflowRuns accessToken="tok" />, { 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", () => {

View file

@ -419,8 +419,6 @@ const DetailSection: React.FC<{
</Collapsible>
);
// ── run detail drawer body ────────────────────────────────────────────────────
const DrawerSpinner: React.FC = () => (
<div className="flex justify-center py-20">
<UiLoadingSpinner className="size-8 text-muted-foreground" />
@ -521,8 +519,9 @@ const WorkflowRuns: React.FC<WorkflowRunsProps> = ({ 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<WorkflowRunsProps> = ({ 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<WorkflowRunsProps> = ({ accessToken }) => {
{/* detail drawer */}
<Sheet
open={runId !== null}
open={drawerOpen}
onOpenChange={(open) => {
if (!open) closeRun();
}}

View file

@ -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<React.ComponentProps<typeof ToolPoliciesTable>> = {},
urlOptions: Parameters<typeof renderWithProviders>[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);
});
});