Skip to content

refactor(ui): reskin shared DataTable from tremor onto shadcn table primitives - #32209

Merged
ryan-crabbe-berri merged 11 commits into
litellm_internal_stagingfrom
litellm_shadcn_table_reskin
Jul 8, 2026
Merged

refactor(ui): reskin shared DataTable from tremor onto shadcn table primitives#32209
ryan-crabbe-berri merged 11 commits into
litellm_internal_stagingfrom
litellm_shadcn_table_reskin

Conversation

@ryan-crabbe-berri

@ryan-crabbe-berri ryan-crabbe-berri commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Relevant issues

Linear ticket

Pre-Submission checklist

Please complete all items before asking a LiteLLM maintainer to review your PR

  • I have added meaningful tests
  • My PR passes all CI/CD checks (e.g., lint, format, unit tests) (the only red check is CodSpeed perf analysis, a Python benchmark unrelated to this UI-only diff)
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • I have requested a Greptile review by commenting @greptileai and received a Confidence Score of at least 4/5 before requesting a maintainer review

Delays in PR merge?

If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).

Screenshots / Proof of Fix

Stacked on #32208 (characterization tests), so the diff here is the reskin itself; it retargets to staging when that merges. To verify in the app (screenshots to follow):

  1. cd ui/litellm-dashboard && npx vitest run src/components/view_logs/table.test.tsx (14 passing: the surviving characterization tests plus getRowId reorder-stability, numeric-alignment, and placeholder-hover coverage)
  2. Run the proxy + dashboard, then check each DataTable consumer renders and behaves: http://localhost:4000/ui/?page=logs (row click opens details; this page now passes an explicit request_id getRowId), the Usage page top-keys and top-models tables, MCP toolsets tab (sortable headers), and pass-through endpoint settings (empty state message)

Type

🧹 Refactoring

Changes

First tremor removal of the ShadCN migration's tables track. The shared view_logs DataTable keeps TanStack for behavior but its presentational layer moves from @tremor/react to the in-repo components/ui/table primitives (already on staging via the chat UI migration; plain styled table elements, no Radix or Base UI involvement, so this is independent of the primitive-library decision)

The seam is hardened per the migration plan since every later table migration copies this API. getRowId is now injected instead of hardcoded to request_id through an any cast; it defaults to the row index and the logs page passes request_id explicitly, which keeps expansion state attached to the right row when a refetch reorders data (covered by a new test). The two expansion render paths collapse to one: renderChildRows had zero consumers and is deleted, leaving renderSubComponent (single colspan cell, the TanStack-documented pattern). Four consumers were passing dead renderSubComponent={() => <div/>} + getRowCanExpand={() => false} boilerplate that expansion never used; it's dropped. Loading and empty defaults become generic ("Loading..." / "No results") instead of log-flavored, since the component long outgrew the logs page

Per review feedback, data rows keep the shadcn primitive's hover highlight (it is the design system default and clickability is still signaled by the cursor, gated on onRowClick), but the loading, empty, and expansion detail placeholder rows suppress it with hover:bg-transparent since a highlight there implies interactivity that does not exist. A test pins the suppression class on all three placeholders and its absence on data rows

A polish pass rounds out the reskin: the empty state gets proper vertical breathing room, the header band is visually distinct (muted background and smaller muted text), and columns can declare meta: { numeric: true } to right-align with tabular figures so decimals line up; the logs Cost/Duration/TTFT/Tokens columns and the Usage top-key/top-model numeric columns opt in. Tests cover the numeric alignment classes and hover suppression on all placeholder rows

An adversarial review pass caught two cosmetic regressions that are fixed here: the outer wrapper regained its overflow clip (the shadcn primitive moved horizontal scroll to an inner unrounded container, so rounded-lg had stopped clipping and the header band bled square corners), and the Duration/TTFT value spans switched from block to inline-block so text-right actually moves them (block boxes ignore text-align). A test pins the wrapper clip

The characterization tests from #32208 pass unchanged apart from the deleted dead path's test. eslint-suppressions.json shrinks by the tremor-table entries this removes

Pins the shared view_logs DataTable contract with library-agnostic
queries ahead of the tremor-to-shadcn table migration: loading and
empty states, TanStack column defs with custom cell renderers,
onRowClick payload, both expansion render paths (colspan sub-component
and sibling child rows), the getRowCanExpand gate, and client-side
sorting on and off. These must pass unchanged after the reskin.
…rimitives

Swaps the view_logs DataTable's presentational layer from @tremor/react
to the in-repo components/ui/table primitives and hardens the seam that
every later table migration copies:

- getRowId is injected instead of hardcoded to request_id through an
  any cast; identity defaults to the row index and the logs page now
  passes request_id explicitly, keeping expansion state attached to the
  right row across refetch reorders
- one expansion render path: renderChildRows had zero consumers and is
  removed; renderSubComponent (colspan cell) is the single path
- the four consumers passing dead no-op renderSubComponent and
  getRowCanExpand boilerplate drop it
- loading and empty defaults become generic (Loading... / No results)
  instead of log-specific

The characterization tests from the previous commit pass unchanged
except the dead child-rows path test, replaced by a reorder-stability
test for injected getRowId plus coverage of the new generic defaults.

First tremor removal of the tables track; view_logs/table.tsx no longer
imports @tremor/react.
@greptile-apps

greptile-apps Bot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR migrates the shared DataTable component's presentational layer from @tremor/react to the in-repo shadcn components/ui/table primitives, keeping TanStack Table for behavior. The seam is tightened: getRowId is now an injectable prop (defaulting to index, with the logs page passing request_id explicitly), the dead renderChildRows path is removed, four consumer sites drop their no-op expansion boilerplate, and numeric columns gain right-alignment via a meta: { numeric: true } convention.

  • table.tsx: drops the tremor import, switches to shadcn primitives, cleans up getRowId to be prop-injectable, removes renderChildRows, tightens hover suppression on placeholder rows, and adds the numeric meta path.
  • columns.tsx: opts five log columns into meta: { numeric: true } and fixes Duration/TTFT spans to inline-block so text-right applies.
  • table.test.tsx: replaces the deleted renderChildRows test with a getRowId reorder-stability test and adds new cases for generic defaults, hover suppression, wrapper clip, and numeric alignment.

Confidence Score: 5/5

Safe to merge — this is a pure presentational reskin with no logic changes to data fetching, auth, or API calls.

The change touches only UI rendering: swapping tremor primitives for shadcn equivalents, removing a dead expansion path, and tightening the DataTable API. All behavioral guarantees (expansion stability, hover suppression, numeric alignment, wrapper clip) are covered by the new and updated tests. Consumer callsites are simplified without altering their runtime behavior. No backend, auth, or data-path code is affected.

No files require special attention.

Important Files Changed

Filename Overview
ui/litellm-dashboard/src/components/view_logs/table.tsx Core reskin from tremor to shadcn primitives; getRowId made injectable, renderChildRows removed, hover suppression and numeric meta added — all correct and well-tested.
ui/litellm-dashboard/src/components/view_logs/table.test.tsx Old renderChildRows test legitimately replaced with getRowId reorder-stability test; new tests cover defaults, hover suppression, wrapper clip, and numeric alignment — no coverage regressions.
ui/litellm-dashboard/src/components/view_logs/columns.tsx Adds meta: { numeric: true } to Cost/Duration/TTFT/Tokens columns and fixes Duration/TTFT spans to inline-block so text-right alignment takes effect.
ui/litellm-dashboard/src/components/view_logs/index.tsx Adds explicit getRowId={(row) => row.request_id} to preserve expansion stability across refetches; straightforward and correct.
ui/litellm-dashboard/src/components/UsagePage/components/EntityUsage/TopKeyView.tsx Drops dead no-op expansion props and adds meta: { numeric: true } to the Spend column; clean callsite simplification.
ui/litellm-dashboard/src/components/UsagePage/components/EntityUsage/TopModelView.tsx Drops dead no-op expansion props and opts all four numeric columns into meta: { numeric: true }; clean callsite simplification.
ui/litellm-dashboard/src/components/mcp_tools/MCPToolsetsTab.tsx Removes dead renderSubComponent and getRowCanExpand no-op props; no behavioral change.
ui/litellm-dashboard/src/components/pass_through_settings.tsx Removes dead renderSubComponent and getRowCanExpand no-op props; no behavioral change.

Reviews (4): Last reviewed commit: "fix(ui): clip DataTable to its rounded w..." | Re-trigger Greptile

Comment thread ui/litellm-dashboard/src/components/view_logs/table.tsx
@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Base automatically changed from litellm_shadcn_datatable_tests to litellm_internal_staging July 7, 2026 19:06
@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor Author

@greptileai re review

…le_reskin

# Conflicts:
#	ui/litellm-dashboard/eslint-metrics.json
@codspeed-hq

codspeed-hq Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 30 untouched benchmarks


Comparing litellm_shadcn_table_reskin (4a36b23) with litellm_internal_staging (46d9742)

Open in CodSpeed

@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor Author

@greptileai re review

@ryan-crabbe-berri

Copy link
Copy Markdown
Contributor Author

@greptileai re review

@ryan-crabbe-berri
ryan-crabbe-berri merged commit e9e30df into litellm_internal_staging Jul 8, 2026
126 checks passed
@ryan-crabbe-berri
ryan-crabbe-berri deleted the litellm_shadcn_table_reskin branch July 8, 2026 20:47
edelauna pushed a commit to edelauna/litellm that referenced this pull request Jul 22, 2026
…rimitives (BerriAI#32209)

* test(ui): characterize DataTable behavior before shadcn reskin

Pins the shared view_logs DataTable contract with library-agnostic
queries ahead of the tremor-to-shadcn table migration: loading and
empty states, TanStack column defs with custom cell renderers,
onRowClick payload, both expansion render paths (colspan sub-component
and sibling child rows), the getRowCanExpand gate, and client-side
sorting on and off. These must pass unchanged after the reskin.

* refactor(ui): reskin shared DataTable from tremor onto shadcn table primitives

Swaps the view_logs DataTable's presentational layer from @tremor/react
to the in-repo components/ui/table primitives and hardens the seam that
every later table migration copies:

- getRowId is injected instead of hardcoded to request_id through an
  any cast; identity defaults to the row index and the logs page now
  passes request_id explicitly, keeping expansion state attached to the
  right row across refetch reorders
- one expansion render path: renderChildRows had zero consumers and is
  removed; renderSubComponent (colspan cell) is the single path
- the four consumers passing dead no-op renderSubComponent and
  getRowCanExpand boilerplate drop it
- loading and empty defaults become generic (Loading... / No results)
  instead of log-specific

The characterization tests from the previous commit pass unchanged
except the dead child-rows path test, replaced by a reorder-stability
test for injected getRowId plus coverage of the new generic defaults.

First tremor removal of the tables track; view_logs/table.tsx no longer
imports @tremor/react.

* test(ui): assert child rows hidden before expansion in DataTable test

* fix(ui): suppress row hover on DataTable placeholder rows

* feat(ui): polish DataTable with skeleton loading, header band, and numeric column alignment

* feat(ui): shape DataTable skeletons per column and keep stale rows during refetch

* revert(ui): drop DataTable skeleton loading, restore text loading row

* fix(ui): clip DataTable to its rounded wrapper and right-align Duration/TTFT values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants