fix(ui): surface the paginated fallback on Cost Optimization - #37659
Conversation
The page streamed its fallback silently: useDailyActivityRange dropped the hook's progress and cancel fields and CacheLeakageCard only showed a loading state while empty. Extract the Usage page's fetch banner into a shared PaginationStatusAlerts component, render it above the tabs, and note on the cache leakage tables when pages are still arriving.
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 5/5 This is a clean, focused refactor with genuine UX improvement. Here's the breakdown: What's done well:
No issues. |
Greptile SummaryThis PR exposes paginated fallback progress and cancellation on Cost Optimization, annotates partially populated cache-leakage tables, and consolidates three Usage-page banner implementations into a shared component
Confidence Score: 4/5The PR needs the cache-leakage loading condition corrected so a new whole-range request does not misrepresent stale rows as streamed partial data The shared cancellation banner is wired correctly, but the cache-leakage note now renders during initial loading while the hook retains results from the previous date range Files Needing Attention: ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/CacheLeakageCard.tsx, ui/litellm-dashboard/src/components/shared/PaginationStatusAlerts.tsx
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/CacheLeakageCard.tsx | Adds a loading annotation, but its loading branch mislabels retained prior-range rows as a continuing page stream |
| ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/CostOptimizationView.tsx | Correctly renders the shared pagination status using the activity hook's forwarded state and cancellation callback |
| ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/useDailyActivityRange.ts | Extends the activity contract to forward pagination progress and cancellation controls without changing query scope |
| ui/litellm-dashboard/src/components/shared/PaginationStatusAlerts.tsx | Faithfully consolidates the existing banners, but adds a source comment prohibited by the repository's comment policy |
| ui/litellm-dashboard/src/app/(dashboard)/usage/_components/components/EntityUsage/EntityUsage.tsx | Replaces duplicated spend and agent pagination banners while preserving agent-visibility gating and cancellation wiring |
| ui/litellm-dashboard/src/app/(dashboard)/usage/_components/components/UsagePageView.tsx | Replaces the inline fallback status UI with the shared component while preserving pagination state and actions |
Reviews (1): Last reviewed commit: "fix(ui): surface the paginated fallback ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| </Tabs> | ||
| </CardHeader> | ||
| <CardContent> | ||
| {rows.length > 0 && (loading || isFetchingMore) && ( |
There was a problem hiding this comment.
When the date range changes, loading retains prior rows, so this note mislabels stale data as pages still arriving
| {rows.length > 0 && (loading || isFetchingMore) && ( | |
| {rows.length > 0 && isFetchingMore && ( |
Knowledge Base Used: Admin dashboard (ui/litellm-dashboard)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed: the note now gates on isFetchingMore alone, with a regression test pinning that loading over prior rows shows nothing
| /** | ||
| * The fetching-progress and partial-data banners shown while a daily activity | ||
| * range streams in page by page. Rendered wherever usePaginatedDailyActivity | ||
| * falls back to pagination; both banners stay hidden on the single-shot | ||
| * aggregated path because it never sets isFetchingMore or cancelled. | ||
| */ |
There was a problem hiding this comment.
This comment restates straightforward rendering conditions, adding documentation that must be maintained without explaining complex business logic
| /** | |
| * The fetching-progress and partial-data banners shown while a daily activity | |
| * range streams in page by page. Rendered wherever usePaginatedDailyActivity | |
| * falls back to pagination; both banners stay hidden on the single-shot | |
| * aggregated path because it never sets isFetchingMore or cancelled. | |
| */ |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Removed the comment, the props and render conditions speak for themselves
loading also covers a fresh aggregated request over the previous range's rows, where pagination copy mislabels stale data. Drop the redundant component comment flagged against the repo comment policy.
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8425b57. Configure here.
|
5/5 — clean, well-scoped, and correctly executed. What's done well:
Minor note (not blocking): |
* feat(ui): add per-key Savings tab to key detail page
Adds a "Savings" tab to the key detail view, showing the same four metrics
and time-series chart as the proxy-wide Cost Optimization view, but scoped
to a single API key.
For org admins, the tab shows the key's full savings across all requests.
Non-admins see only their own requests on the key, with a scope note
explaining the limitation.
Root cause: userDailyActivityCall and userDailyActivityAggregatedCall
never forwarded an api_key query parameter to the backend, even though
both handlers already accept and filter by it.
Changes:
- networking.tsx: Add optional apiKey param to both daily activity call
wrappers (appended to variadic options tuple for backward compatibility).
- costOptimizationUtils.ts: Extract shared metrics helpers (compressionOf,
cachingOf, autorouterOf, savedTokensOf, cacheHitRatio) and shortDate
so both UsageTab and KeySavingsTab use the same formulas and prevent
divergence.
- useDailyActivityRange.ts: Refactor into useScopedDailyActivityRange(
accessToken, scope: {userId, apiKey?}) for reuse-by-parameter unbundling.
Role resolution stays at the entry point (useDailyActivityRange), not in
a scoped caller. Update test expectations for new 6-arg tuple.
- UsageTab.tsx: Simplify by importing extracted helpers and SummaryCard
component instead of defining them inline. No behavioral change.
- key_info_view.tsx: Insert "Savings" tab trigger between "Overview" and
"Settings"; wire TabsContent to new KeySavingsTab component with lazy
mounting (no keepMounted) to defer daily-activity fetch until tab opened.
- NEW: components/shared/SummaryCard.tsx — Shared presenter for four-tile
summary row (label + value + hint + optional info popover). Extracted
from UsageTab so both surfaces show identical tile layout without CSS
divergence.
- NEW: components/templates/KeySavingsTab.tsx — Per-key view with admin/
non-admin scope branching, empty-state messaging, same chart toggles
and info popovers as UsageTab.
- NEW: components/templates/KeySavingsTab.test.tsx — 7 tests covering mount,
loading state, empty state, scoping, and scope-note visibility.
Authorization: No new permission check. Both backends gate api_key filter
by the same user role check that governs the request itself. Non-admins
must send their own user_id and can only see their own keys.
Tests: 6121 pass (1 pre-existing failure unrelated to this change).
Prior art / collision note:
- PR #37570 (budgets tab) lands in same TabsList hunks as "Savings" tab,
but different tab names so conflict trivial if both merge.
- PR #37659 (my own) adds progress/cancelled/cancel to DailyActivityRange,
but this PR uses stable three-field interface from staging.
* fix(ui): scope spend view by the backend's admin-view contract, not all_admin_roles
Greptile flagged org admin handling on the key savings tab. The live bug it
described does not fire today: useAuthorized supplies session-role labels and
all_admin_roles only carries the raw org_admin spelling, so an org admin was
already scoped. That safety was accidental, so replace the predicate with
spendScopeUserId / hasProxyWideSpendView in utils/roles.ts, mirroring the
backend's user_api_key_has_admin_view (proxy admin and admin viewer only, org
admin excluded in both spellings), and use it in both useDailyActivityRange
and KeySavingsTab
Reclassify the KeySavingsTab render test as an integration test per the
repo's unit/integration split, move scope-resolution coverage to roles.test.ts
as a full role matrix, use real session-role values instead of raw ones, and
assert tile totals against non-empty metrics. Replace the nested ternary in
the chart body (frontend-lint error) with flat conditional rendering
* fix(ui): show auto-router savings as the fourth key-savings tile
Cache hit rate had displaced auto-router savings from the fourth slot,
diverging from the org-wide Cost Optimization page's tile order. Match
it: Total / Compression / Prompt caching / Auto-router, with cache hit
rate as a fifth tile.
* fix(ui): drop cache hit rate from the key savings tiles
Keep the four tiles this page is meant to show: total, compression,
prompt caching, and auto-router savings.
* fix(ui): stop an empty api_key from widening a key-scoped activity read
The paginated and aggregated daily-activity wrappers disagreed on an
empty filter value: the paginated one appended it, the aggregated one
coerced it to undefined with || and dropped it. Since the aggregated
call is the one tried first, an empty key hash would have silently
turned a key-scoped read into a proxy-wide one and reported every
key's savings as this key's. Use ?? so both send the filter through
and it matches nothing instead.
* style(ui): satisfy prettier and the inline-object lint rule in key savings tests
* refactor(ui): drop the cacheHitRatio extraction left over from the removed tile
* fix(ui): pass daily-activity filters raw so both transports agree at the null boundary
* refactor(ui): share the savings tiles and totals between both surfaces
The per-key Savings tab and the proxy-wide Cost Optimization tab carried a byte-identical
four-tile block, three long metric-definition strings included, and five identical useMemo
totals. Both now render SavingsTiles and total through useSavingsTotals, so the donut cannot
slice numbers the tile above it disagrees with.
* docs(ui): say request, not mount, in the savings tab comment
The comment claimed mounting eagerly would fire the rollup sweep, which reads as a claim about
the bundle. Only the request is deferred; the module ships with the key page either way.
* test(ui): pin the daily-activity args array against the real caller signatures
The sibling unit test mocks networking, so it checks the positional array against itself and
stays green when the array and a networking signature drift apart. Swapping user_id and api_key
in the aggregated signature alone passes there and fails here on user_id=hash-abc.
* style(ui): hoist the daily-activity query options out of the call argument
The four-property object literal tripped local/no-large-inline-object-arg. The violation predates
this branch, which only moved the line into the annotated range, and the rule count drops 550 to 549.
2 participants
TLDR
Problem this solves:
How it solves it:
User Flow
Before: during a slow load the user watches numbers change with no explanation and no way out
After: the same degraded load announces itself, reports progress, and can be stopped
Relevant issues
aiohttp_openai/route - get to 1K RPS on single instance #7539)Linear ticket
Resolves LIT-5700
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Setup: live proxy on localhost:4000 backed by real Postgres, LiteLLM_DailyUserSpend seeded with 29,002 rows across four dates so the range spans 30 pages at the UI's page size. The fallback legs force the aggregated route to return 500 (the route was temporarily patched in the running rig to raise, identically for Before and After); the happy-path legs run the unmodified route
Before (c164944)
Fallback streams silently with no progress or cancel
curl -s -o /dev/null -w "%{http_code}" "http://localhost:4000/user/daily/activity/aggregated?start_date=2026-08-10&end_date=2026-08-17&timezone=420" -H "Authorization: Bearer sk-1234"returns 500Cache leakage table presents partial rows as final
After (8425b57)
Fallback streams silently with no progress or cancel
Cache leakage table presents partial rows as final
Happy path unchanged
document.body.textContent.includes("Currently fetching spend data")is false)Type
🐛 Bug Fix
Caveats (if any)
Final Attestation
Note
Low Risk
UI-only loading/status presentation; no auth, data, or fetch-logic changes beyond forwarding existing hook fields.
Overview
When Cost Optimization falls back to page-by-page daily activity, the page now shows the same fetch-progress banner and Stop control as Usage, instead of silently reshuffling numbers.
useDailyActivityRangeforwardsprogress,cancelled, andcancelfrom the paginated hook. Cache leakage tables add a still-loading note while extra pages stream in (not during a full range reload).The duplicated Usage/EntityUsage banners are extracted into shared
PaginationStatusAlerts(optionalsubjectfor agent vs spend data). Happy-path single-shot loads still show no banner.Reviewed by Cursor Bugbot for commit 8425b57. Bugbot is set up for automated code reviews on this repo. Configure here.