From 2bb8f7f21972ac785c374672d84fd8c7912ee9ee Mon Sep 17 00:00:00 2001 From: Mateo Wang <277851410+mateo-berri@users.noreply.github.com> Date: Wed, 14 Oct 2026 10:47:05 -0700 Subject: [PATCH] fix(ui): label Decisions playground issues by question key and fix the Response header row (#46145) * fix(ui): dedupe blank-question issues and label Decisions questions by key, not position Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * fix(ui): make the Response card header a flex row so title and badges share one line Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> * test(ui): fail when duplicate blank-name issues stop being deduped * fix(ui): drop the Decisions issue dedupe, which no reachable endpoint needs * fix(ui): center the Response header and test the Decisions issue labels end to end The card's border rule set 24px of bottom padding on the Response header, which put its content 8px above center. The header now keeps 8px on both sides. New tests cover the /v1/decisions position label and the score-level warning label for an all-digit key, which mutations of validatePayload left untested * test(ui): check the Decisions Response header layout in the browser * test(ui): register the Decisions header spec's model through the API so it runs in CI The CI UI stack does not read fixtures/config.yml, so the config-only fake-decisions-model never existed there and the spec could not pick it. The spec now creates a uniquely named typesafe model against the mock server, waits until /model_group/info lists it as an evaluation model and /v1/systemone answers, and deletes it afterwards --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../e2e/ui/fixtures/mock_llm_server/server.py | 11 ++ .../decisionsResponseHeader.spec.ts | 162 ++++++++++++++++++ .../components/systemOneUI/ResponseView.tsx | 4 +- .../systemOneUI/ValidationFeedback.test.tsx | 19 +- .../systemOneUI/ValidationFeedback.tsx | 3 +- .../systemOneUI/lib/issueLabel.test.ts | 37 ++-- .../components/systemOneUI/lib/issueLabel.ts | 12 +- .../systemOneUI/lib/validatePayload.test.ts | 53 +++++- .../systemOneUI/lib/validatePayload.ts | 22 ++- 9 files changed, 275 insertions(+), 48 deletions(-) create mode 100644 tests/e2e/ui/tests/playground/decisionsResponseHeader.spec.ts diff --git a/tests/e2e/ui/fixtures/mock_llm_server/server.py b/tests/e2e/ui/fixtures/mock_llm_server/server.py index 82c90a9dd64..1e3dcecaff3 100644 --- a/tests/e2e/ui/fixtures/mock_llm_server/server.py +++ b/tests/e2e/ui/fixtures/mock_llm_server/server.py @@ -117,6 +117,17 @@ async def embeddings(request: Request): } +@app.post("/v1/systemone") +async def systemone(request: Request): + body = await request.json() + noul_keys = [key for key, question in body.get("questions", {}).items() if question.get("type") == "noul"] + return { + "model": body.get("model", "mock-decisions"), + "answers": {key: {"type": "noul", "noul": 0.9} for key in noul_keys}, + "usage": {"input_tokens": 12, "output_tokens": 3}, + } + + if __name__ == "__main__": # The port is overridable so two checkouts can run the harness at the same # time; the default keeps every existing caller (run_e2e.sh, the CircleCI diff --git a/tests/e2e/ui/tests/playground/decisionsResponseHeader.spec.ts b/tests/e2e/ui/tests/playground/decisionsResponseHeader.spec.ts new file mode 100644 index 00000000000..b4dba6765a1 --- /dev/null +++ b/tests/e2e/ui/tests/playground/decisionsResponseHeader.spec.ts @@ -0,0 +1,162 @@ +import { + test as base, + expect, + type APIRequestContext, + type Locator, + type Page as PlaywrightPage, +} from "@playwright/test"; +import { ADMIN_STORAGE_PATH } from "../../constants"; +import { Page } from "../../fixtures/pages"; +import { navigateToPage } from "../../helpers/navigation"; +import { masterKey, rootPath, uniqueSuffix } from "../../helpers/traffic"; + +const MOCK_LLM_BASE = `http://127.0.0.1:${process.env.MOCK_LLM_PORT ?? "8090"}/v1`; + +const adminHeaders = () => ({ + Authorization: `Bearer ${masterKey()}`, + "Content-Type": "application/json", +}); + +const isListedAsDecisionsModel = async ( + request: APIRequestContext, + name: string, +): Promise => { + const listed = await request.get("/model_group/info", { + headers: adminHeaders(), + }); + if (!listed.ok()) return false; + const body: { data?: { model_group?: string; mode?: string }[] } = + await listed.json(); + return (body.data ?? []).some( + (group) => group.model_group === name && group.mode === "evaluation", + ); +}; + +const isRoutable = async ( + request: APIRequestContext, + name: string, +): Promise => { + const ping = await request.post(`${rootPath()}/v1/systemone`, { + headers: adminHeaders(), + data: { + model: name, + state: "readiness ping", + questions: { + ready: { + type: "noul", + instructions: "Is this a ping?", + criteria: { true: "yes", false: "no" }, + }, + }, + }, + }); + return ping.ok(); +}; + +const test = base.extend<{ decisionsModel: string }>({ + decisionsModel: async ({ request }, use) => { + const name = `e2e-decisions-header-${uniqueSuffix()}`; + const created = await request.post("/model/new", { + headers: adminHeaders(), + data: { + model_name: name, + litellm_params: { + model: "typesafe/jev-latest", + api_base: MOCK_LLM_BASE, + api_key: "fake-key", + }, + }, + }); + expect( + created.ok(), + `POST /model/new failed (${created.status()}): ${await created.text()}`, + ).toBe(true); + const modelId: string = (await created.json()).model_id; + try { + await expect + .poll( + async () => + (await isListedAsDecisionsModel(request, name)) && + (await isRoutable(request, name)), + { + message: `${name} never became listed and routable`, + timeout: 60_000, + }, + ) + .toBe(true); + await use(name); + } finally { + await request.post("/model/delete", { + headers: adminHeaders(), + data: { id: modelId }, + }); + } + }, +}); + +const centerY = async (locator: Locator): Promise => { + const box = await locator.boundingBox(); + expect(box, "element has no layout box").not.toBeNull(); + return box!.y + box!.height / 2; +}; + +const offCenterBy = async (inner: Locator, outer: Locator): Promise => + Math.abs((await centerY(inner)) - (await centerY(outer))); + +const dismissSearchHint = async (page: PlaywrightPage): Promise => { + const dismiss = page.getByRole("button", { name: "Dismiss search hint" }); + if (await dismiss.isVisible()) { + await dismiss.click(); + } +}; + +test.describe("Decisions playground Response header", () => { + test.use({ + storageState: ADMIN_STORAGE_PATH, + viewport: { width: 1440, height: 900 }, + }); + + test("keeps the title and its badges on one row centered in the header", async ({ + page, + decisionsModel, + }) => { + await navigateToPage(page, Page.LlmPlayground); + await page.getByRole("tab", { name: /Decisions/ }).click(); + + const header = page + .getByRole("region", { name: "Decisions results" }) + .locator('[data-slot="card-header"]'); + const title = header.getByText("Response", { exact: true }); + await expect(title).toBeVisible({ timeout: 20_000 }); + expect( + await offCenterBy(title, header), + "empty-state title is off center", + ).toBeLessThanOrEqual(1); + + await page.getByRole("button", { name: "Load example" }).click(); + await page + .getByRole("menuitem", { name: "Moderate a forum comment" }) + .click(); + const modelPicker = page.getByRole("combobox", { name: "Decision model" }); + await modelPicker.click(); + await modelPicker.fill(decisionsModel); + await page + .getByRole("option", { name: decisionsModel, exact: true }) + .click(); + await dismissSearchHint(page); + await page.getByRole("button", { name: "Send", exact: true }).click(); + + const usage = header.getByText("12 in / 3 out"); + await expect(usage).toBeVisible({ timeout: 30_000 }); + expect( + await offCenterBy(title, header), + "title is off center after Send", + ).toBeLessThanOrEqual(1); + for (const badge of [header.getByText(/^\d+ ms$/), usage]) { + expect( + await offCenterBy(badge, title), + "badge wrapped off the title's row", + ).toBeLessThanOrEqual(1); + } + }); +}); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ResponseView.tsx b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ResponseView.tsx index cc73b44265c..572bf765ac1 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ResponseView.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ResponseView.tsx @@ -223,7 +223,7 @@ export default function ResponseView({ if (!response) { return ( - + Response @@ -240,7 +240,7 @@ export default function ResponseView({ return ( - + {view === "curl" ? "Response JSON" : "Response"}
{latencyMs !== undefined && ( diff --git a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ValidationFeedback.test.tsx b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ValidationFeedback.test.tsx index a2f6c24781a..5089179ef13 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ValidationFeedback.test.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ValidationFeedback.test.tsx @@ -13,9 +13,9 @@ describe("ValidationStatus", () => { render( , ); @@ -23,7 +23,9 @@ describe("ValidationStatus", () => { }); it("reports a valid payload when only warnings remain", () => { - render(); + render( + , + ); expect(screen.getByText("Valid payload")).toBeInTheDocument(); }); }); @@ -31,7 +33,10 @@ describe("ValidationStatus", () => { describe("IssueList", () => { it("lists every issue under its field label and renders nothing when there are none", () => { const { rerender } = render( - , + , ); expect(screen.getByRole("list", { name: "Payload validation issues" })).toHaveTextContent("InputRequired"); rerender(); @@ -46,8 +51,8 @@ describe("ValidationSummary", () => { rerender( , ); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ValidationFeedback.tsx b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ValidationFeedback.tsx index e8152456d15..554e7da3770 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ValidationFeedback.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/ValidationFeedback.tsx @@ -1,6 +1,5 @@ import { Badge } from "@/components/ui/badge"; import { CircleAlert, CircleCheck, TriangleAlert } from "lucide-react"; -import { issueLabel } from "./lib/issueLabel"; import { errorCount as countErrors, type PayloadValidation } from "./lib/validatePayload"; export function ValidationStatus({ validation }: { validation: PayloadValidation }) { @@ -44,7 +43,7 @@ export function IssueList({ id, validation }: { id: string; validation: PayloadV ) : ( )} - {issueLabel(issue.path)} + {issue.label} {issue.message} ))} diff --git a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/issueLabel.test.ts b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/issueLabel.test.ts index 29ae0f60ae4..85d2a576194 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/issueLabel.test.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/issueLabel.test.ts @@ -3,29 +3,34 @@ import { issueLabel } from "./issueLabel"; describe("issueLabel", () => { it("names the Input field for both endpoints instead of the JSON key", () => { - expect(issueLabel("state")).toBe("Input"); - expect(issueLabel("input")).toBe("Input"); + expect(issueLabel(["state"])).toBe("Input"); + expect(issueLabel(["input"])).toBe("Input"); }); it("names top-level fields and the request itself", () => { - expect(issueLabel("model")).toBe("Model"); - expect(issueLabel("questions")).toBe("Questions"); - expect(issueLabel("safety_identifier")).toBe("Safety identifier"); - expect(issueLabel("root")).toBe("Request"); - expect(issueLabel("syntax")).toBe("Request"); + expect(issueLabel(["model"])).toBe("Model"); + expect(issueLabel(["questions"])).toBe("Questions"); + expect(issueLabel(["safety_identifier"])).toBe("Safety identifier"); + expect(issueLabel([])).toBe("Request"); + expect(issueLabel(["syntax"])).toBe("Request"); }); - it("names a question by its key or its 1-based position and the field inside it", () => { - expect(issueLabel("questions.urgency")).toBe('Question "urgency"'); - expect(issueLabel("questions.urgency.criteria")).toBe('Question "urgency" options'); - expect(issueLabel("questions.urgency.criteria.1")).toBe('Question "urgency" options'); - expect(issueLabel("questions.category.instructions")).toBe('Question "category" instructions'); - expect(issueLabel("questions.0.name")).toBe("Question 1 name"); - expect(issueLabel("questions.2.type")).toBe("Question 3 answer type"); + it("names a keyed question by its key, even an all-digit or dotted one, and the field inside it", () => { + expect(issueLabel(["questions", "urgency"])).toBe('Question "urgency"'); + expect(issueLabel(["questions", "urgency", "criteria"])).toBe('Question "urgency" options'); + expect(issueLabel(["questions", "urgency", "criteria", 1])).toBe('Question "urgency" options'); + expect(issueLabel(["questions", "category", "instructions"])).toBe('Question "category" instructions'); + expect(issueLabel(["questions", "2", "criteria"])).toBe('Question "2" options'); + expect(issueLabel(["questions", "v1.2", "type"])).toBe('Question "v1.2" answer type'); + }); + + it("names a listed question by its 1-based position", () => { + expect(issueLabel(["questions", 0, "name"])).toBe("Question 1 name"); + expect(issueLabel(["questions", 2, "type"])).toBe("Question 3 answer type"); }); it("falls back to the humanized key for anything unknown", () => { - expect(issueLabel("max_tokens")).toBe("max tokens"); - expect(issueLabel("questions.a.max_items")).toBe('Question "a" max items'); + expect(issueLabel(["max_tokens"])).toBe("max tokens"); + expect(issueLabel(["questions", "a", "max_items"])).toBe('Question "a" max items'); }); }); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/issueLabel.ts b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/issueLabel.ts index 76acc266117..60ca47ead2a 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/issueLabel.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/issueLabel.ts @@ -15,16 +15,16 @@ const QUESTION_FIELD: Record = { criteria: "options", }; -const humanize = (segment: string): string => segment.replace(/_/g, " "); +const humanize = (segment: PropertyKey): string => String(segment).replace(/_/g, " "); -export function issueLabel(path: string): string { - const [head, question, field] = path.split("."); +export function issueLabel(path: readonly PropertyKey[]): string { + const [head = "root", question, field] = path; if (head !== "questions" || question === undefined) { - return TOP_LEVEL[head] ?? humanize(head); + return TOP_LEVEL[String(head)] ?? humanize(head); } - const title = /^\d+$/.test(question) ? `Question ${Number(question) + 1}` : `Question "${question}"`; + const title = typeof question === "number" ? `Question ${question + 1}` : `Question "${String(question)}"`; if (field === undefined) { return title; } - return `${title} ${QUESTION_FIELD[field] ?? humanize(field)}`; + return `${title} ${QUESTION_FIELD[String(field)] ?? humanize(field)}`; } diff --git a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/validatePayload.test.ts b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/validatePayload.test.ts index 6e91f09f8bd..e28d7da8e3f 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/validatePayload.test.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/validatePayload.test.ts @@ -11,11 +11,8 @@ const requestWith = (fields: Record = {}) => describe("validateSystemOnePayload", () => { it("rejects empty input", () => { - expect(validateSystemOnePayload(" ").issues).toContainEqual({ - path: "root", - message: "Payload cannot be empty.", - severity: "error", - }); + const emptyIssue = { path: "root", label: "Request", message: "Payload cannot be empty.", severity: "error" }; + expect(validateSystemOnePayload(" ").issues).toContainEqual(emptyIssue); }); it("reports JSON syntax errors", () => { @@ -249,6 +246,17 @@ describe("validateSystemOnePayload", () => { ); }); + it("labels a score-level warning by its question key even when the key is all digits", () => { + const result = validateSystemOnePayload( + requestWith({ + questions: { + "2": { type: "score", instructions: "Rate urgency", criteria: Array.from({ length: 11 }, () => "Level") }, + }, + }), + ); + expect(result.issues.map((issue) => issue.label)).toEqual(['Question "2" options']); + }); + it("accepts a score question with two levels", () => { expect( validateSystemOnePayload( @@ -346,11 +354,19 @@ describe("blank context", () => { expect(validateSystemOnePayload(raw, "/v1/systemone").issues.map((issue) => issue.path)).toEqual(["state"]); const filled = validateSystemOnePayload(raw.replace('"state":""', '"state":"A ticket"'), "/v1/systemone"); expect(filled.isValid).toBe(false); - expect(filled.issues).toContainEqual({ - path: "questions", - message: "Give every question a name.", - severity: "error", + expect(filled.issues).toEqual([ + { path: "questions", label: "Questions", message: "Give every question a name.", severity: "error" }, + ]); + }); + + it("labels a /v1/systemone question by its key even when the key is all digits", () => { + const raw = JSON.stringify({ + state: "A ticket", + questions: { "2": { type: "choice", instructions: "", criteria: {} } }, }); + const labels = validateSystemOnePayload(raw, "/v1/systemone").issues.map((issue) => issue.label); + expect(labels).toContain('Question "2" options'); + expect(labels.some((label) => label.startsWith("Question 3"))).toBe(false); }); it("accepts /v1/systemone once the state carries text", () => { @@ -365,6 +381,25 @@ describe("blank context", () => { expect(result.issues.map((issue) => issue.path)).toEqual(["input"]); }); + it("labels a /v1/decisions issue by the question's position", () => { + const duplicateChoices = [{ value: "a" }, { value: "a" }]; + const raw = JSON.stringify({ + input: "Ship it?", + questions: [ + { type: "predicate", name: "q", instructions: "Is it?" }, + { type: "choice", name: "pick", instructions: "Pick one", choices: duplicateChoices }, + ], + }); + expect(validateOpenAIDecisionsPayload(raw).issues).toEqual([ + { + path: "questions.1.choices", + label: "Question 2 choices", + message: "Choice values must be unique, including their string representations.", + severity: "error", + }, + ]); + }); + it("accepts /v1/decisions once the input carries text", () => { const raw = JSON.stringify({ input: "Ship it?", diff --git a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/validatePayload.ts b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/validatePayload.ts index 1fb32efd158..05ed0f14511 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/validatePayload.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/playground/components/systemOneUI/lib/validatePayload.ts @@ -1,4 +1,5 @@ import { z } from "zod"; +import { issueLabel } from "./issueLabel"; import { openAIDecisionsRequestSchema, type OpenAIDecisionsRequest } from "./openAIDecisions"; import { decisionsRequestSchema, @@ -11,6 +12,7 @@ const RECOMMENDED_MAX_SCORE_LEVELS = 10; export interface SystemOnePayloadIssue { path: string; + label: string; message: string; severity: "error" | "warning"; } @@ -26,21 +28,28 @@ export type SystemOnePayloadValidation = PayloadValidation; export const errorCount = (validation: PayloadValidation): number => validation.issues.filter((issue) => issue.severity === "error").length; +const error = (path: readonly PropertyKey[], message: string): SystemOnePayloadIssue => ({ + path: path.length > 0 ? path.join(".") : "root", + label: issueLabel(path), + message, + severity: "error", +}); + const toIssue = (path: PropertyKey[], message: string): SystemOnePayloadIssue => path[0] === "questions" && path[1] === "" - ? { path: "questions", message: "Give every question a name.", severity: "error" } - : { path: path.length > 0 ? path.join(".") : "root", message, severity: "error" }; + ? error(["questions"], "Give every question a name.") + : error(path, message); const invalid = (path: string, message: string) => ({ isValid: false, - issues: [{ path, message, severity: "error" as const }], + issues: [error([path], message)], }); export function parseJson(raw: string): { ok: true; value: unknown } | { ok: false; message: string } { try { return { ok: true, value: JSON.parse(raw) }; - } catch (error: unknown) { - return { ok: false, message: error instanceof Error ? error.message : String(error) }; + } catch (cause: unknown) { + return { ok: false, message: cause instanceof Error ? cause.message : String(cause) }; } } @@ -49,6 +58,7 @@ const scoreLevelWarnings = (payload: PlaygroundRequest): SystemOnePayloadIssue[] .filter(([, question]) => question.type === "score" && question.criteria.length > RECOMMENDED_MAX_SCORE_LEVELS) .map(([id]) => ({ path: `questions.${id}.criteria`, + label: issueLabel(["questions", id, "criteria"]), message: `More than ${RECOMMENDED_MAX_SCORE_LEVELS} score levels may reduce result quality.`, severity: "warning", })); @@ -90,7 +100,7 @@ const requireContext = (raw: string, path: "state" | "input", result: Payload ? { ...result, isValid: false, - issues: [{ path, message: "Add the text the model should decide about.", severity: "error" }], + issues: [error([path], "Add the text the model should decide about.")], } : result;