From f448ea5762dc061fc985e0042c733d2ceaef63d3 Mon Sep 17 00:00:00 2001 From: yuneng-jiang Date: Mon, 13 Jul 2026 14:02:27 -0700 Subject: [PATCH] fix(ui): address Virtual Keys redesign review nits (#33112) * fix(ui): address Virtual Keys redesign review nits Restore sorting by budget on the merged Spend / Budget column. The column now uses a new DataTableMultiSortHeader whose chevron opens a menu offering Spend and Budget in both directions plus Reset, so the progress-bar cell stays merged while the sort field becomes an explicit choice. Sorting is server-side, so the chosen field id (spend or max_budget, both accepted by /key/list) flows straight through as sort_by Fill the DataTable to its container width when column resizing is on. The table width was pinned to the sum of column widths, so hiding columns left an empty gutter on the right. It now keeps that width as a minimum and stretches to 100% on underflow while still scrolling on overflow, which also covers the same gap in TeamVirtualKeysTable since both share the component Drop the dark background box behind the page-header icon so the Virtual Keys header reads like the Teams header, and pull the 4-line inline filter lambda in SearchSelect out into a named matchesQuery helper Extends the DataTable and VirtualKeysTable tests to cover the new multi-field sort menu (field id maps to sort_by, active indicator, reset) and the fill-to-container width * fix(ui): emphasize the active field in the Spend / Budget sort header The merged Spend / Budget header always read "Spend / Budget" regardless of which field drove the sort, so after picking Budget descending there was no way to tell what was sorted without reopening the menu. The header now builds its label from the sort fields and emphasizes whichever one is active (bold, full-strength text) while muting the other, so the sorted column reads at a glance alongside the direction chevron. Drops the now-redundant title prop since the label is derived from the fields * fix(ui): remove w-full so the keys page content stops overflowing by 32px The virtual keys content wrapper used "w-full mx-4", which sets the width to 100% of the parent and then adds 16px of horizontal margin on each side, so its margin-box came to 100% + 32px and overflowed the scrollable main region by exactly 32px. That surfaced as a horizontal scrollbar along the bottom of the whole content area, under the pagination. A block div is already full-width, so dropping w-full lets mx-4 inset it correctly with no overflow * fix(ui): darken the clickable Key cell on hover so it reads as clickable The Key cell was the click target that opens the key detail, but hovering only faded the chevron in with no change to the cell itself, so there was no cue that the area was clickable. Give the cell a subtle muted background and a pointer cursor on hover. The button spans the full cell (a negative inline margin plus a matching width offset so the hover fill reaches both cell edges while the title stays aligned with the other columns) --- .../VirtualKeysPage/VirtualKeysTable.test.tsx | 48 ++++++++- .../VirtualKeysPage/keyTableColumns.tsx | 9 +- .../shared/DataTable/DataTable.test.tsx | 73 ++++++++++++- .../components/shared/DataTable/DataTable.tsx | 2 +- .../shared/DataTable/DataTableSortHeader.tsx | 102 +++++++++++++++++- .../src/components/shared/DataTable/index.ts | 7 +- .../src/components/shared/PageHeader.tsx | 8 +- .../src/components/shared/SearchSelect.tsx | 12 ++- .../shared/table_cells/identity_cell.test.tsx | 5 +- .../shared/table_cells/identity_cell.tsx | 5 +- .../src/components/user_dashboard.tsx | 2 +- 11 files changed, 251 insertions(+), 22 deletions(-) diff --git a/ui/litellm-dashboard/src/components/VirtualKeysPage/VirtualKeysTable.test.tsx b/ui/litellm-dashboard/src/components/VirtualKeysPage/VirtualKeysTable.test.tsx index cf4beeed40f..ee430e35f4a 100644 --- a/ui/litellm-dashboard/src/components/VirtualKeysPage/VirtualKeysTable.test.tsx +++ b/ui/litellm-dashboard/src/components/VirtualKeysPage/VirtualKeysTable.test.tsx @@ -266,7 +266,8 @@ it("should render the redesigned table headers", () => { expect(screen.getByText("Key")).toBeInTheDocument(); expect(screen.getByText("Team")).toBeInTheDocument(); expect(screen.getByText("Models")).toBeInTheDocument(); - expect(screen.getByText("Spend / Budget")).toBeInTheDocument(); + expect(screen.getByText("Spend", { selector: "[data-sort-field='spend']" })).toBeInTheDocument(); + expect(screen.getByText("Budget", { selector: "[data-sort-field='max_budget']" })).toBeInTheDocument(); }); it("sorts by the backend key_alias field (not the column label) when the Key header is clicked", async () => { @@ -280,6 +281,51 @@ it("sorts by the backend key_alias field (not the column label) when the Key hea }); }); +it("sorts by the backend max_budget field when 'Budget descending' is chosen from the Spend / Budget menu", async () => { + const user = userEvent.setup(); + renderWithProviders(); + + await user.click(screen.getByTestId("sort-trigger-spend")); + await user.click(await screen.findByText("Budget descending")); + + await waitFor(() => { + expect(mockUseKeys).toHaveBeenLastCalledWith( + 1, + 50, + expect.objectContaining({ sortBy: "max_budget", sortOrder: "desc" }), + ); + }); +}); + +it("emphasizes the active field in the Spend / Budget header so the sorted column reads without opening the menu", async () => { + const user = userEvent.setup(); + renderWithProviders(); + + await user.click(screen.getByTestId("sort-trigger-spend")); + await user.click(await screen.findByText("Budget descending")); + + await waitFor(() => { + expect(screen.getByText("Budget", { selector: "[data-sort-field='max_budget']" }).className).toContain( + "font-semibold", + ); + }); + expect(screen.getByText("Spend", { selector: "[data-sort-field='spend']" }).className).toContain( + "text-muted-foreground", + ); +}); + +it("sorts by spend ascending when 'Spend ascending' is chosen from the Spend / Budget menu", async () => { + const user = userEvent.setup(); + renderWithProviders(); + + await user.click(screen.getByTestId("sort-trigger-spend")); + await user.click(await screen.findByText("Spend ascending")); + + await waitFor(() => { + expect(mockUseKeys).toHaveBeenLastCalledWith(1, 50, expect.objectContaining({ sortBy: "spend", sortOrder: "asc" })); + }); +}); + it("should open KeyInfoView when clicking the key cell", async () => { renderWithProviders(); diff --git a/ui/litellm-dashboard/src/components/VirtualKeysPage/keyTableColumns.tsx b/ui/litellm-dashboard/src/components/VirtualKeysPage/keyTableColumns.tsx index e2dc48fed9d..96e57b1fb87 100644 --- a/ui/litellm-dashboard/src/components/VirtualKeysPage/keyTableColumns.tsx +++ b/ui/litellm-dashboard/src/components/VirtualKeysPage/keyTableColumns.tsx @@ -4,7 +4,7 @@ import { InfoCircleOutlined } from "@ant-design/icons"; import { ColumnDef } from "@tanstack/react-table"; import { Popover, Typography } from "antd"; -import { DataTableSortHeader } from "@/components/shared/DataTable"; +import { DataTableMultiSortHeader, DataTableSortHeader, type DataTableSortField } from "@/components/shared/DataTable"; import { Skeleton } from "@/components/ui/skeleton"; import { DateCell, @@ -26,6 +26,11 @@ interface KeyStatus { tooltip?: string; } +const SPEND_BUDGET_SORT_FIELDS: DataTableSortField[] = [ + { id: "spend", label: "Spend" }, + { id: "max_budget", label: "Budget" }, +]; + const getKeyStatus = (key: KeyResponse): KeyStatus => { if (key.blocked === true) { const isScimBlocked = (key.metadata as Record | null | undefined)?.scim_blocked === true; @@ -291,7 +296,7 @@ export const getKeyTableColumns = ({ id: "spend", accessorKey: "spend", meta: { title: "Spend / Budget", skeleton: "meter" }, - header: ({ column }) => , + header: ({ table }) => , size: 180, enableSorting: true, cell: ({ row }) => { diff --git a/ui/litellm-dashboard/src/components/shared/DataTable/DataTable.test.tsx b/ui/litellm-dashboard/src/components/shared/DataTable/DataTable.test.tsx index d8d4c9392dc..0d3468d4ada 100644 --- a/ui/litellm-dashboard/src/components/shared/DataTable/DataTable.test.tsx +++ b/ui/litellm-dashboard/src/components/shared/DataTable/DataTable.test.tsx @@ -5,7 +5,7 @@ import { useState } from "react"; import { describe, expect, it, vi } from "vitest"; import { DataTable } from "./DataTable"; -import { DataTableSortHeader } from "./DataTableSortHeader"; +import { DataTableMultiSortHeader, DataTableSortHeader } from "./DataTableSortHeader"; import { DataTableViewOptions } from "./DataTableViewOptions"; interface Person { @@ -55,6 +55,23 @@ const dropdownSortColumns: ColumnDef[] = [ }, ]; +const multiSortColumns: ColumnDef[] = [ + { + id: "spend", + accessorKey: "name", + header: ({ table }) => ( + + ), + cell: ({ row }) => {row.original.name}, + }, +]; + const nameEmailColumns: ColumnDef[] = [ { accessorKey: "name", @@ -165,6 +182,60 @@ describe("DataTable sorting", () => { await user.click(await screen.findByText("Reset")); expect(names()).toEqual(["Charlie", "Alice", "Bob"]); }); + + it("multi-sort header emits the chosen field id (not the column id) as the sort key", async () => { + const user = userEvent.setup(); + const onSortingChange = vi.fn(); + render( + , + ); + + await user.click(screen.getByTestId("sort-trigger-spend")); + await user.click(await screen.findByText("Budget descending")); + expect(onSortingChange).toHaveBeenLastCalledWith([{ id: "max_budget", desc: true }]); + + await user.click(screen.getByTestId("sort-trigger-spend")); + await user.click(await screen.findByText("Spend ascending")); + expect(onSortingChange).toHaveBeenLastCalledWith([{ id: "spend", desc: false }]); + }); + + it("multi-sort header reflects the active field and direction, and Reset clears it", async () => { + const user = userEvent.setup(); + const onSortingChange = vi.fn(); + render( + , + ); + + await user.click(screen.getByTestId("sort-trigger-spend")); + // The header trigger shows the active (descending) indicator while sorted by a field it owns. + expect(screen.getByTestId("sort-trigger-spend").querySelector("[data-sort-indicator='desc']")).not.toBeNull(); + + await user.click(await screen.findByText("Reset")); + expect(onSortingChange).toHaveBeenLastCalledWith([]); + }); +}); + +describe("DataTable layout", () => { + it("stretches the table to fill the container when resizing is on, so hidden columns leave no right-side gap", () => { + const { container } = render(); + + const table = container.querySelector("table"); + expect(table).not.toBeNull(); + // width pins the natural column total (horizontal scroll on overflow); minWidth:100% fills the gap on underflow. + expect(table?.style.minWidth).toBe("100%"); + }); }); describe("DataTable pagination", () => { diff --git a/ui/litellm-dashboard/src/components/shared/DataTable/DataTable.tsx b/ui/litellm-dashboard/src/components/shared/DataTable/DataTable.tsx index bc13318dd58..d55831b87ca 100644 --- a/ui/litellm-dashboard/src/components/shared/DataTable/DataTable.tsx +++ b/ui/litellm-dashboard/src/components/shared/DataTable/DataTable.tsx @@ -532,7 +532,7 @@ export function DataTable(props: DataTableProps { if (paginationSlot !== undefined) { diff --git a/ui/litellm-dashboard/src/components/shared/DataTable/DataTableSortHeader.tsx b/ui/litellm-dashboard/src/components/shared/DataTable/DataTableSortHeader.tsx index 1cf09ce4f47..8988bd6c2d0 100644 --- a/ui/litellm-dashboard/src/components/shared/DataTable/DataTableSortHeader.tsx +++ b/ui/litellm-dashboard/src/components/shared/DataTable/DataTableSortHeader.tsx @@ -1,8 +1,8 @@ "use client"; import { Menu } from "@base-ui/react/menu"; -import type { Column, SortDirection } from "@tanstack/react-table"; -import { ChevronDown, ChevronsUpDown, ChevronUp, X } from "lucide-react"; +import type { Column, SortDirection, Table } from "@tanstack/react-table"; +import { Check, ChevronDown, ChevronsUpDown, ChevronUp, X } from "lucide-react"; import type * as React from "react"; import { cn } from "@/lib/cva.config"; @@ -94,3 +94,101 @@ export function DataTableSortHeader({ ); } + +export interface DataTableSortField { + /** Backend sort column, sent verbatim as the sorting state id (e.g. "spend", "max_budget"). */ + id: string; + label: string; +} + +interface DataTableMultiSortHeaderProps { + table: Table; + fields: DataTableSortField[]; + className?: string; +} + +/** + * Sort header for a column that merges several backend-sortable fields into one cell + * (e.g. a combined Spend / Budget cell). The header label is the field labels joined by " / ", + * with the field currently driving the sort emphasized so the active column reads at a glance + * without opening the menu. The chevron opens a menu offering each field in both directions. + */ +export function DataTableMultiSortHeader({ table, fields, className }: DataTableMultiSortHeaderProps) { + const active = table.getState().sorting[0]; + const activeField = active !== undefined && fields.some((field) => field.id === active.id) ? active : undefined; + const activeDirection: SortDirection = activeField?.desc === true ? "desc" : "asc"; + const sorted: false | SortDirection = activeField === undefined ? false : activeDirection; + + const options = fields.flatMap((field) => [ + { key: `${field.id}-asc`, id: field.id, desc: false, label: `${field.label} ascending`, Icon: ChevronUp }, + { key: `${field.id}-desc`, id: field.id, desc: true, label: `${field.label} descending`, Icon: ChevronDown }, + ]); + + const segmentClass = (isActive: boolean): string => { + if (isActive) return "font-semibold text-foreground"; + if (activeField) return "text-muted-foreground"; + return ""; + }; + + const labelSegments = fields.flatMap((field, index) => { + const isActive = activeField?.id === field.id; + const segment = ( + + {field.label} + + ); + if (index === 0) return [segment]; + return [ + + {" / "} + , + segment, + ]; + }); + + return ( +
+ {labelSegments} + + field.label).join(" or ")}`} + onClick={(event) => event.stopPropagation()} + className={cn( + "inline-flex size-6 items-center justify-center rounded-md hover:bg-muted", + sorted ? "text-primary" : "text-muted-foreground", + )} + > + + + } + /> + + + + {options.map((option) => { + const isActive = activeField?.id === option.id && activeField.desc === option.desc; + return ( + table.setSorting([{ id: option.id, desc: option.desc }])} + > + {option.label} + {isActive && } + + ); + })} + table.setSorting([])}> + Reset + + + + + +
+ ); +} diff --git a/ui/litellm-dashboard/src/components/shared/DataTable/index.ts b/ui/litellm-dashboard/src/components/shared/DataTable/index.ts index c4218f6051a..1ee1eed1258 100644 --- a/ui/litellm-dashboard/src/components/shared/DataTable/index.ts +++ b/ui/litellm-dashboard/src/components/shared/DataTable/index.ts @@ -5,7 +5,12 @@ export { DataTableFilterDrawer, DataTableFilterField, type FilterDraft } from ". export { DataTablePagination, DEFAULT_PAGE_SIZE_OPTIONS } from "./DataTablePagination"; export { DataTableToolbar } from "./DataTableToolbar"; export { DataTableViewOptions } from "./DataTableViewOptions"; -export { DataTableSortHeader, type DataTableSortVariant } from "./DataTableSortHeader"; +export { + DataTableSortHeader, + DataTableMultiSortHeader, + type DataTableSortVariant, + type DataTableSortField, +} from "./DataTableSortHeader"; export type { DataTablePaginationProps } from "./DataTablePagination"; export type { ColumnPinnedSide, diff --git a/ui/litellm-dashboard/src/components/shared/PageHeader.tsx b/ui/litellm-dashboard/src/components/shared/PageHeader.tsx index 34d478e1cd9..e314e8e8bc2 100644 --- a/ui/litellm-dashboard/src/components/shared/PageHeader.tsx +++ b/ui/litellm-dashboard/src/components/shared/PageHeader.tsx @@ -12,12 +12,8 @@ interface PageHeaderProps { export function PageHeader({ title, subtitle, icon, actions }: PageHeaderProps) { return (
-
- {icon != null && ( - - {icon} - - )} +
+ {icon != null && {icon}}

{title}

{subtitle != null &&

{subtitle}

} diff --git a/ui/litellm-dashboard/src/components/shared/SearchSelect.tsx b/ui/litellm-dashboard/src/components/shared/SearchSelect.tsx index c29e099a1c6..eff5cfc804d 100644 --- a/ui/litellm-dashboard/src/components/shared/SearchSelect.tsx +++ b/ui/litellm-dashboard/src/components/shared/SearchSelect.tsx @@ -26,6 +26,12 @@ interface SearchSelectProps { className?: string; } +const matchesQuery = (option: SearchSelectOption, query: string): boolean => { + const q = query.trim().toLowerCase(); + if (!q) return true; + return option.label.toLowerCase().includes(q) || (option.sublabel?.toLowerCase().includes(q) ?? false); +}; + export function SearchSelect({ options, value, @@ -44,11 +50,7 @@ export function SearchSelect({ onValueChange={(item: SearchSelectOption | null) => onValueChange(item?.value ?? "")} isItemEqualToValue={(a: SearchSelectOption, b: SearchSelectOption) => a.value === b.value} itemToStringLabel={(item: SearchSelectOption) => item.label} - filter={(item: SearchSelectOption, query: string) => { - const q = query.trim().toLowerCase(); - if (!q) return true; - return item.label.toLowerCase().includes(q) || (item.sublabel?.toLowerCase().includes(q) ?? false); - }} + filter={matchesQuery} disabled={disabled} > { expect(screen.queryByRole("button")).not.toBeInTheDocument(); }); - it("renders a clickable button and fires onClick", async () => { + it("renders a clickable button that signals interactivity and fires onClick", async () => { const onClick = vi.fn(); const user = userEvent.setup(); render(); const button = screen.getByRole("button"); expect(button.querySelector(".lucide-chevron-right")).not.toBeNull(); + // The clickable area must read as clickable: a hover background and a pointer cursor. + expect(button.className).toContain("hover:bg-muted"); + expect(button.className).toContain("cursor-pointer"); await user.click(button); expect(onClick).toHaveBeenCalledTimes(1); }); diff --git a/ui/litellm-dashboard/src/components/shared/table_cells/identity_cell.tsx b/ui/litellm-dashboard/src/components/shared/table_cells/identity_cell.tsx index 4d3e3d8e4dd..97263436454 100644 --- a/ui/litellm-dashboard/src/components/shared/table_cells/identity_cell.tsx +++ b/ui/litellm-dashboard/src/components/shared/table_cells/identity_cell.tsx @@ -36,7 +36,10 @@ export function IdentityCell({ title, subtitle, badge, onClick, className, title