feat(ui): add per-key Savings tab to key detail page - #37693
Conversation
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.
Greptile SummaryAdds a per-key Savings tab with date-scoped savings metrics and charts, extends daily-activity networking calls with an API-key filter, and extracts shared metric and summary-card helpers.
Confidence Score: 4/5The org-admin scoping mismatch should be fixed before merging because it silently presents partial savings as the full key total. The networking tuple and key identifier are correctly wired, but org_admin is classified as a whole-key viewer in the new component while the backend narrows that role to caller-owned activity, producing misleading totals without the scope note. Files Needing Attention: ui/litellm-dashboard/src/components/templates/KeySavingsTab.tsx; ui/litellm-dashboard/src/components/templates/KeySavingsTab.test.tsx
|
| Filename | Overview |
|---|---|
| ui/litellm-dashboard/src/components/templates/KeySavingsTab.tsx | Adds the per-key savings UI and request scope selection, but misclassifies org_admin as receiving whole-key scope. |
| ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/useDailyActivityRange.ts | Extracts a scoped activity hook and correctly aligns arguments for aggregated and paginated fetch functions. |
| ui/litellm-dashboard/src/components/networking.tsx | Correctly forwards the optional api_key filter through both daily-activity wrappers. |
| ui/litellm-dashboard/src/components/templates/key_info_view.tsx | Lazily mounts the new Savings tab and supplies the hashed key identifier used by daily activity. |
| ui/litellm-dashboard/src/components/templates/KeySavingsTab.test.tsx | Covers key rendering and scope states, but the full-component suite uses the unit-test filename instead of the integration-test tier. |
| ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/costOptimizationUtils.ts | Extracts shared savings and cache-hit helpers without changing their formulas. |
| ui/litellm-dashboard/src/components/shared/SummaryCard.tsx | Extracts the existing summary-card presentation into a reusable component. |
Reviews (1): Last reviewed commit: "feat(ui): add per-key Savings tab to key..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| apiKey: keyToken, | ||
| }); | ||
|
|
||
| const { dateValue, onDateChange, results, loading, isFetchingMore } = activity; | ||
| const startTime = dateValue.from ?? null; | ||
| const endTime = dateValue.to ?? null; | ||
|
|
||
| const compressionTotal = useMemo(() => results.reduce((sum, d) => sum + compressionOf(d.metrics), 0), [results]); |
There was a problem hiding this comment.
When an org_admin opens this tab, all_admin_roles causes the UI to request whole-key activity and hide the scope note, but the backend narrows the request to that caller's activity. The displayed savings therefore omit other users' requests while being presented as the key's full total.
Knowledge Base Used: Admin dashboard (ui/litellm-dashboard)
There was a problem hiding this comment.
Not live: this prop is a session-role label, so org_admin arrives as Org Admin and was already scoped. Hardened anyway with spendScopeUserId mirroring user_api_key_has_admin_view
There was a problem hiding this comment.
You're right — the current diff already uses hasProxyWideSpendView(userRole) and spendScopeUserId(userRole, userId) instead of the raw all_admin_roles.includes(userRole) check I flagged. The role string mismatch issue and the scoping logic are both handled correctly in the updated code. My comment no longer applies. Thanks for the clarification.
| userId="user-123" | ||
| userRole="user" | ||
| /> | ||
| ); | ||
|
|
||
| expect(screen.getByText("Total saved")).toBeInTheDocument(); | ||
| expect(screen.getByText("Compression savings")).toBeInTheDocument(); | ||
| expect(screen.getByText("Prompt caching savings")).toBeInTheDocument(); | ||
| expect(screen.getByText("Cache hit rate")).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("shows empty state when no results in range", () => { | ||
| vi.spyOn(useScopedDailyActivityRangeModule, "useScopedDailyActivityRange").mockReturnValue(mockActivity()); | ||
|
|
||
| render( | ||
| <KeySavingsTab | ||
| accessToken="test-token" |
There was a problem hiding this comment.
This suite renders KeySavingsTab with its real child component tree, so it belongs in the dashboard's integration-test tier with a .integration.test.tsx filename. Keeping it as a unit test bypasses the repository's intended test organization and selection boundary.
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.
Agreed. Renamed to KeySavingsTab.integration.test.tsx and moved scope-resolution coverage to unit tests in roles.test.ts
…ll_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
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4.2 / 5 What works wellRole scoping is correct and well-tested.
Shared utils prevent formula drift. Extracting No Test coverage is solid. The 7 tests cover the important paths: totals math, edge-case zero denominator, empty vs loading state, and scoping for each role tier. Issues1. Null vs undefined inconsistency for In api_key: apiKey, // passes null when no key is givenIn api_key: apiKey || undefined, // converts null/empty-string to undefined
2. Positional-args coupling comment signals a fragile interface (pre-existing, but now extended) The comment in
This is a pre-existing design issue, not introduced by this PR, but the PR extends the positional tuple one step further. Adding a future param (e.g., 3. Looking at Overall this is a well-designed, carefully reasoned feature addition. The role scoping correctness is the most important property for a spend-visibility feature and it's handled correctly. The null-vs-undefined inconsistency is the only concrete code issue worth addressing before merge. |
![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 3b592c0. Configure here.
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.
Keep the four tiles this page is meant to show: total, compression, prompt caching, and auto-router savings.
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.
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4 / 5 Here's the breakdown: What earns the high mark:
What keeps it from a 5:
None of these are blockers, but items 1 and 3 in particular are the kind of thing that causes a subtle regression when the next engineer touches either file. |
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 2776356. Configure here.
…the null boundary
|
Thanks, this was a useful round. Two fixed as one change, two I want to push back on with evidence. Pushed as 1 + 2, fixed together: the
|
|
@greptileai can you reply here with a score and why that score? |
|
bugbot run |
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 51855fc. Configure here.
|
Score: 4 / 5 Why 4 and not 5Strengths that push it high:
Two things that keep it from 5: 1. The positional // Positional, and read by two functions whose signatures diverge at index 3
args: [accessToken, startTime, endTime, userId, true, apiKey],That comment is the only thing preventing a future contributor from inserting a parameter in the wrong position. An object-shaped 2. The eager JS import vs "lazy" framing. |
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.
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.
…ignatures 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.
|
On the positional args: agreed the comment was the only guard, so there is now a test that drives the hook through the real networking callers instead of mocking them. Swapping On the eager import: correct, the module ships with the key page either way, so the comment now says the request is what gets deferred rather than the mount. Separately, both surfaces share @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 8/10 The responses to both concerns land well. Here's what tips it to an 8 rather than higher, and what holds it back. What earns the score: The The The integration test that drives through the real networking callers rather than mocking them is the right answer to the positional args concern given What keeps it from a 9:
The module-level import is fine for bundle purposes; the comment update makes the intent clear, so that's resolved. Minor: |
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 697a2ae. Configure here.
|
On the Admin Viewer entry: it is pinned, not untested. roles.test.ts feeds the literal string "Admin Viewer" into spendScopeUserId, which is one line, hasProxyWideSpendView(role) ? null : userId, so the toBeNull assertion only passes while the entry exists. Mutation check: deleting "Admin Viewer" from rolesWithProxyWideSpendView fails two existing tests, "accepts Admin Viewer in either the session-role or raw spelling" and "differs from all_admin_roles by exactly org admin". The grant itself mirrors the backend, user_api_key_has_admin_view returns true for PROXY_ADMIN and PROXY_ADMIN_VIEW_ONLY, and the helper only decides what the UI asks for: the endpoint force-scopes a non-admin's null user_id to their own rows and 403s any other id, so a wrong entry here cannot widen a read On the hook home: the tiles and the by-driver donut are its only consumers and both render beside it. costOptimizationUtils is pure functions, so a React hook is an odd fit there too. It can move when a third consumer appears @greptileai can you reply here with a score and why that score? |
|
Both points hold up under scrutiny. Admin Viewer: The mutation argument is correct. The test "accepts Admin Viewer in either the session-role or raw spelling" pins it directly, and "differs from all_admin_roles by exactly org admin" would fail if the entry disappeared. The endpoint-force-scopes argument is also right — the backend won't honour a null Hook home: Agreed. Score: 4/5 The The one remaining fragility is the positional args array itself. The comment acknowledges it and the integration test catches drift, but future callers who add a parameter to one signature and not the other won't get a compile-time error — they'll get a silent wrong-field read until the integration test fires. A named-options object on Nothing here would block merge. |
…ument 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.
…itellm_key_savings_tab # Conflicts: # ui/litellm-dashboard/src/app/(dashboard)/cost-optimization/_components/useDailyActivityRange.ts
|
Merged staging. #37659 added progress/cancelled/cancel to DailyActivityRange; those now flow through the scoped hook, and the role branch stays spendScopeUserId rather than the all_admin_roles check that PR carried, which is this PR's org-admin narrowing. Its hook test passes unchanged against the split. @greptileai can you reply here with a score and why that score? |
|
bugbot run |
|
Score: 4.5 / 5 What makes this strong: The
The shared The The integration test for positional args is smart. The unit test mocks networking and can't catch a transposition between The one design smell worth noting: The positional args array Summary: The filter-widening fix is correct, the role narrowing for org admin aligns with backend behavior, the abstractions prevent drift, and the test strategy (mutation testing + integration URL pinning) is well-matched to the failure modes. The positional-args coupling is the only thing I'd want to see typed away eventually. |
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 e631629. Configure here.
2 participants
What
Adds a "Savings" tab to the key detail view, showing the same savings drivers and time-series chart as the proxy-wide Cost Optimization view, filtered to one API key. Four tiles: total saved, compression, prompt caching, auto-router. A driver the key does not use reads $0.00 rather than being hidden, so the tab always answers "what is this key saving, and from where"
For proxy admins the tab shows the key's full savings across all requests. Everyone else, org admins included, sees only their own requests on that key, with a scope note saying so
Root cause
userDailyActivityCallanduserDailyActivityAggregatedCallnever forwarded anapi_keyquery parameter to the backend, even though both handlers already accept and filter by itImplementation
Unbundled scoping: extracted
useScopedDailyActivityRange(accessToken, scope: {userId, apiKey?})so role resolution lives only in the org-wide entry point (useDailyActivityRange), not in every per-key callerShared helpers: extracted
compressionOf,cachingOf,autorouterOf,savedTokensOfinto utils so UsageTab and KeySavingsTab read the same formulas and cannot drift apartShared component: extracted
SummaryCard, and above itSavingsTilesplususeSavingsTotals, so the proxy-wide tab and the key tab render one tile block and total through one path. Both previously carried a byte-identical four-tile block, the three metric-definition strings included, and five identicaluseMemototals, which is how the by-driver donut could have come to disagree with the tile above itEmpty
api_keyno longer widens the read: the aggregated wrapper built its query with||, so an empty-string key collapsed toundefinedand the parameter was dropped, silently turning a key-scoped request into an unscoped one. Both wrappers now use??, which only drops null and undefined. The paginated wrapper already behaved this way, so the two transports now agreeFiles
components/networking.tsx, add an optionalapiKeyparam to both daily-activity wrappers, and fix the||to??costOptimizationUtils.ts, extract the shared metric accessors andshortDateuseDailyActivityRange.ts, split intouseScopedDailyActivityRangeplus the role-resolving wrapperUsageTab.tsx, renderSavingsTilesand total throughuseSavingsTotals, which the by-driver donut now sliceskey_info_view.tsx, add the "Savings" tab trigger and wiring, withoutkeepMountedso the rollup request only fires when the tab is opened. The module itself ships with the key page either wayutils/roles.ts,spendScopeUserIdandhasProxyWideSpendViewso org admin is not treated as proxy-wide for this endpointcomponents/shared/SummaryCard.tsxcomponents/shared/SavingsTiles.tsxcomponents/templates/KeySavingsTab.tsxcomponents/templates/KeySavingsTab.integration.test.tsxuseDailyActivityRange.integration.test.tsxTests
129 tests across the six files this branch touches, all passing with no type errors:
roles.test.ts(61),networking.test.ts(37),UsageTab.test.tsx(18),useDailyActivityRange.test.tsx(7),KeySavingsTab.integration.test.tsx(5),useDailyActivityRange.integration.test.tsx(1)The
api_keyregression tests are mutation-checked. Reverting??back to||fails the aggregated case while the paginated case still passes, which is exactly the divergence the fix closes. A test written only againstnullwould have passed on the broken code, since both transports already stripped null identicallyuseDailyActivityRange.integration.test.tsxpins the positional args array against the real caller signatures, which the sibling unit test cannot do because it mocks networking and so compares the array only against itself. Swappinguser_idandapi_keyin the aggregated signature alone leaves that unit test green and fails this one onuser_id=hash-abcKeySavingsTab.integration.test.tsxcovers tile totals across a multi-day range, empty state distinguished from loading, and the scope note appearing for a non-admin and an org admin but not a proxy adminPrior art / collision notes
PR #37570 (budgets tab) lands in the same TabsList hunks as the "Savings" tab, under a different name, so a merge conflict there would be trivial
PR #37659 has since merged, adding
progress,cancelledandcanceltoDailyActivityRange. Staging is merged in and the one conflict is resolved: those three fields now flow throughuseScopedDailyActivityRange, and the role branch staysspendScopeUserIdrather than theall_admin_rolescheck #37659 carried, which is the org-admin narrowing this PR is for. That PR also added a hook test, which passes unchanged against the splitNote
Medium Risk
Touches spend-data scoping (who can see proxy-wide vs own-user rows) and query serialization for user_id/api_key; a filter drop would over-report savings. Backend still enforces admin view, but the UI now decides what it asks for.
Overview
Adds a Savings tab on the key detail page with the same compression / prompt-caching / auto-router tiles and time-series as Cost Optimization, filtered to one API key. Proxy admins see the whole key; everyone else (including org admins) sees only their own requests, with an explicit scope note. The tab is not
keepMounted, so the daily rollup only loads when opened.Daily-activity networking now forwards
api_key. Emptyuser_id/api_keystay as filters instead of being dropped (the aggregated caller used||, which could widen a scoped read to proxy-wide). Role checks use newspendScopeUserId/hasProxyWideSpendViewso org admin is not treated as proxy-wide.Shared
SavingsTiles,useSavingsTotals, and metric helpers so the proxy-wide usage tab and the key tab total through one path. Tests cover filter serialization, org-admin scoping, and tile totals.Reviewed by Cursor Bugbot for commit e631629. Bugbot is set up for automated code reviews on this repo. Configure here.