feat: surface media usage in the task API and chat UI - #1463
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive media-usage tracking system for non-LLM modalities (such as image, video, TTS, ASR, embedding, and reranking) across both the frontend and backend. It enhances the TokenUsage model with thread-safe locking, adds aggregation helpers, and integrates usage recording into various model adapters and tools. Additionally, it introduces standalone usage scopes to capture tracking data across thread boundaries and background tasks. The review feedback suggests adding a defensive guard in TokenUsage.merge to prevent a potential self-merge from doubling the usage counts.
cc8708d to
1066ea9
Compare
184c3af to
2aff110
Compare
Completes the series: the task detail endpoint returns aggregated media rows and the chat token-usage popover renders them alongside LLM tokens. `media_usage` carries one row per (model, unit, call_type, resolution) from `aggregate_media_usage_by_model`. There is deliberately no cross-unit quantity total — summing images + seconds + characters produces a number with no meaning — and no `media_calls` companion field: the client derives its own count from these rows, so a second server-side reduction would be a duplicate that can drift. Locales cover all nine `MediaCallType` values and all five `MediaUnit` values in both en and zh. Review feedback from xorbitsai#997: - The dead `media_calls` response field is dropped rather than wired up. It had zero references in `frontend/src` while the component recomputed the same total from `media_usage`. - Media rows are normalised at the fetch boundary. `token_usage_details` is free-form legacy JSON, so a row can be null, a non-object, or carry a numeric field as a string — and both failure modes were live: reducing over a null row throws on property access, and a string quantity like `"4"` passes the `> 0` check but fails `Number.isFinite` in the formatter, silently rendering the "not yet measured" placeholder as if the call had cost nothing. One normaliser now coerces every row to finite non-negative numbers and plain strings, dropping rows that are not objects (arrays included, since they are `typeof 'object'` too), so every consumer below deals in real numbers. - The test i18n mock is synced with the real locale files: it carried a stale `unit.tokens` that no longer exists and lacked `unit.texts`, which embedding rows use. Added a test asserting the rendered `texts` label, so the mock can no longer drift from the locales undetected — the existing assertions only covered `images` and `seconds`, which is why this specific gap was invisible. Also added: null/non-object row survival and string-quantity rendering tests, both of which fail without the normaliser. Frontend tests were not runnable locally (vitest hangs against a symlinked node_modules in this worktree), so the component changes were verified by review and are relying on CI's frontend jobs.
`data` comes from `response.json()` and is `any`, so `.map(normalizeMediaUsage)` produced `any` and the type-predicate callback's parameter was implicitly `any` — `TS7006` under `noImplicitAny`, failing both the static-export build and `npm type-check`. The array is now cast to `unknown[]` and the filter callback annotated explicitly. Every frontend test file already passed, including the three new media cases; only the type checker rejected this. Verified locally this time, with a tsconfig scoped to the changed file: reverting the annotation reproduces `TS7006` at the same position, and the fix clears it.
2aff110 to
bb428d9
Compare
|
Rebased onto latest What was dropped. This branch had been cut on top of the whole media-metering stack, so it carried 15 commits that do not belong to it:
Each of those is landing through its own PR, so only the top two commits — this PR's actual increment — were replayed onto No API migration was needed. The increment's only reference to the primitives is One thing worth flagging so it does not read as an oversight: this branch does contain the string Contract re-verified against current
Drift. Checks run locally: scoped |
|
One coverage caveat, filed separately as #1577 rather than fixed here to keep this PR a pure rebase.
This is a pre-existing gap on |
rogercloud
left a comment
rogercloud
left a comment
There was a problem hiding this comment.
Summary
PR #1463 completes the read-side media-usage slice by projecting persisted media detail rows through GET /api/chat/task/{id} and rendering them in the chat usage popover. It reuses the existing aggregation, task-detail cache, polling lifecycle, and localization machinery while keeping heterogeneous units separate and preserving additive API compatibility.
Blocking: no — recommended event: APPROVE
Update summary
The update consists of commits 026a3a6f and bb428d95. 026a3a6f adds the task-detail API aggregation/serialization, media popover, English and Chinese labels, and frontend coverage; bb428d95 fixes the TypeScript annotation/noImplicitAny issue. The rebase context in conversation comment 5365048088 explains that the superseded producer stack was dropped/replayed, so this review evaluates the current five-file API/UI slice.
Round 0 design verdict
Verdict: acceptable-with-reservations. The direction is sound: the PR uses the existing persisted TokenUsage.details stream and media aggregator, adds one additive task-detail field, and uses the established popover, polling, and dynamic-i18n patterns. The mixed measured/unmeasured aggregate-state concern is the exact tracked root in #1495, and the cumulative JSON persistence/read scaling boundary is the exact tracked root in #1466; both are dropped context rather than new findings for this PR. The remaining design reservation is the missing real task-detail serialization/cache contract test, reported below as NEW-API-SEAM-TEST; the independently raised API-shape ownership concern was dropped because the backend already follows the repository's dynamic Dict[str, Any] convention and the frontend normalizer is an appropriate network/legacy-data boundary.
Prior-findings checklist (body-only and inline roots)
| Canonical root | Status | Evidence and disposition |
|---|---|---|
TokenUsage.merge self-merge doubling |
DROPPED — pre-existing/out of scope | The body-only review finding 4959106301 and its duplicate inline root 3802478157 both concern src/xagent/core/model/chat/token_context.py, which is byte-identical between the supplied base and head and is not in this PR's diff. The current in-tree caller constructs a fresh target before merging, and conversation comment 5365048088 explains the superseded stack/rebase context. This is not a FIXED claim and is not re-reported. |
| Frontend chat test CI execution gap | DROPPED — tracked pre-existing | Conversation comment 5365089129 identifies the existing explicit-launcher gap for frontend/src/components/chat/TokenUsageDisplay.test.tsx; the workflow, package scripts, and manifest are unchanged by this PR. The concern is tracked in #1577, so it is not re-reported as a PR finding. |
The mixed measured/unmeasured aggregate concern is likewise dropped as tracked context only under #1495, and the cumulative details persistence/scaling concern is dropped as tracked context only under #1466. Neither is an active finding below.
Review-criteria coverage
- Code quality and local API design: The implementation reuses existing abstractions without adding a parallel counter, endpoint, or speculative schema framework. The model-identity presentation issue is reported below; the broader contract-ownership candidate was independently dropped as non-actionable.
- Code correctness and edge cases: The review checked malformed/legacy JSON, numeric coercion, missing fields, unknown values, zero quantities, small positive quantities, unit/call-type identity, cache behavior, polling cleanup, authorization, and escaping. The confirmed display edge cases are listed below; no new resource-lifecycle, concurrency, error-propagation, security, or compatibility blocker was found.
- Test quality and coverage: Existing aggregation, locale-parity, and lower-level persistence tests were considered. The missing API seam assertion and four distinct frontend test/fixture gaps (arrays, zero/unmeasured, provider-token labels, and TTS units) are listed below.
- Design and architecture: The read-model/UI direction fits the existing
TokenUsage.detailspipeline and correctly avoids cross-unit totals. The two pre-existing architectural boundaries are tracked in #1495 and #1466 rather than duplicated here. - Documentation and localization: Both shipped locale trees contain the current media unit/type keys and preserve unknown-value fallback behavior. No separate documentation issue was found because this internal legacy endpoint has no existing field-level response documentation convention.
New findings (all minor, non-blocking)
-
frontend/src/components/chat/TokenUsageDisplay.tsx:357— minor — media rows hide meaningful model identity. The backend groups bymodel_id or model_nameand preserves both fields, but this renderer shows onlymodel_namewhen it is present. Two distinct configured IDs sharing a provider-facing name therefore produce visually indistinguishable rows. Suggestion: mirror the existing LLM-row behavior: whenmodel_idandmodel_nameare both non-empty and differ, rendermodel_idas a secondary label/title, and add a same-name/different-ID regression fixture. -
frontend/src/components/chat/TokenUsageDisplay.tsx:121— minor — small positive quantities can display as zero.Intl.NumberFormatwithmaximumFractionDigits: 1rounds a valid0 < quantity < 0.05to0, while the renderer treats the raw value as measured. Suggestion: use adaptive precision or an explicit representation such as<0.1for positive sub-threshold values, while keeping exact zero mapped to the unmeasured state. -
frontend/src/components/chat/TokenUsageDisplay.tsx:214— minor — fixed plural unit labels are grammatically wrong for quantity one. The new English labels render values such as1 images,1 chars,1 requests, and1 texts;secis an acceptable invariant abbreviation. Suggestion: add quantity-aware singular/other translations (for example,1 image,1 char,1 request, and1 text) and assert the singular row labels. -
src/xagent/web/api/chat.py:4601— minor — the new task-detail serialization seam lacks a route/cache contract test. Aggregator tests and mocked frontend tests can pass if the production GET handler omits, renames, or fails to cachemedia_usage. Suggestion: extend the existing realGET /api/chat/task/{id}test with representative media details and exactmedia_usageassertions, including zero quantity, calls, resolution, provider tokens, andtokens_estimated; preferably assert the cache-hit response too. -
frontend/src/components/chat/TokenUsageDisplay.test.tsx:387— minor — the malformed-row test does not cover arrays or prove invalid rows are absent. The production normalizer explicitly rejects arrays, but removing that guard would create an extra zero/unknown row while the current count and quantity assertions could still pass. Suggestion: add an array-shaped row such as[]and assert exactly the expected rendered row (or the absence of anUnknown/unmeasured junk row), retaining the null/string/number cases. -
frontend/src/components/chat/TokenUsageDisplay.tsx:373— minor — the zero/unmeasured rendering branch has no frontend fixture. All current media quantities are positive and the test translation mock omitschatPage.tokenUsage.unmeasured, so a regression to0 secor a missing translation can pass. Suggestion: add the mock key and a zero-quantity media row withcalls > 0, then assert the localizednot yet measuredtext rather than numeric zero. -
frontend/src/components/chat/TokenUsageDisplay.test.tsx:615— minor — provider-token tests assert numbers but not the token label. The checks for1.12kand40~would pass iftokensShortwere missing, mistyped, or rendered with the wrong translation key. Suggestion: addchatPage.tokenUsage.tokensShortto the mock and assert the complete strings, such as1.12k tokensand40~ tokens. -
frontend/src/components/chat/TokenUsageDisplay.test.tsx:325— minor — the TTS fixture uses an impossible unit pairing. The fixture setscall_type: "tts"withunit: "seconds", but the producer contract maps TTS tocharacters, so it never tests the actual TTS path. Suggestion: change the fixture tounit: "characters"and assert thecharslabel; use video or ASR for a seconds-formatting fixture.
Simplification Lens status
The dedicated Simplification Lens scan was unavailable because it returned usage_limit_reached; no simplification findings are asserted.
Validation context
CI preflight was green with no failed or pending checks. No local tests, builds, linters, formatters, or dependency installation were run; local validation was intentionally not run under the review policy.
Blocking status & recommended decision
Blocking: no. There are no confirmed major or critical findings. All eight confirmed findings above are minor, and the prior self-merge and frontend CI-gap roots were dropped as pre-existing/out-of-scope or tracked.
Recommended event: APPROVE
Renderer:
- Surface a differing model_id as a secondary label on media rows, matching
the rule the LLM rows already use. The backend groups media by
`model_id or model_name`, so two configured IDs sharing one provider-facing
name are distinct billable rows that previously rendered identically.
- Render a positive quantity below the one-decimal rounding threshold as
"<0.1" instead of "0". The row already took the measured branch, so a bare
"0" contradicted it and read as "this cost nothing".
- Choose unit labels by quantity, so a single item reads "1 image" rather
than "1 images". Locales without a plural distinction carry the same text
in both keys; `sec` stays invariant as an abbreviation.
Locales: unit labels become one/other pairs in both en and zh, keeping the
locale trees structurally identical.
Tests:
- Add a real GET /api/chat/task/{id} assertion for media_usage, covering
same-key aggregation, a zero quantity, mixed estimated tokens, and the
LLM/media split; plus a cache-hit case, since the detail response is cached
wholesale and the usage popover polls that path.
- Correct the TTS fixture to bill in characters. MediaCallType.TTS derives
MediaUnit.CHARACTERS, so the previous tts/seconds pair asserted a state the
producer cannot emit; a separate ASR row now covers seconds formatting.
- Add an array-shaped malformed row and assert no junk row is rendered. The
existing count and quantity assertions passed with the Array.isArray guard
removed, so they did not pin it.
- Register unmeasured and tokensShort in the test locale mock and assert the
full "1.12k tokens" / "40~ tokens" strings, so a missing or misnamed label
cannot pass by matching the number alone.
- Cover the zero-quantity unmeasured branch, the sub-threshold quantity, the
singular unit labels, and same-name/different-id media rows.
3 participants
Last of five changes splitting #997. Once this and the others land, #997
can be closed.
The task detail endpoint now returns aggregated media rows, and the chat
token-usage popover renders them alongside LLM tokens.
media_usagecarries one row per (model, unit, call_type, resolution)from
aggregate_media_usage_by_model. Deliberately no cross-unitquantity total — summing images + seconds + characters produces a
number with no meaning — and no
media_callscompanion field, seebelow. Locales cover all nine
MediaCallTypevalues and all fiveMediaUnitvalues in bothenandzh; I diffed the key sets againstthe enums to confirm.
Review feedback addressed from #997
The dead
media_callsresponse field is dropped rather than wiredup. It had zero references in
frontend/srcwhile the componentrecomputed the same total from
media_usage; a second server-sidereduction is a duplicate that can drift from the rows it summarises.
Media rows are normalised at the fetch boundary.
token_usage_detailsis free-form legacy JSON, and both failure modesthe review named were live: reducing over a
nullrow throws onproperty access (blanking the component), and a string
quantitylike"4"passes the> 0check but then failsNumber.isFinitein theformatter, so it silently rendered the "not yet measured" placeholder —
reading as if the call had cost nothing. One normaliser now coerces
every row to finite non-negative numbers and plain strings and drops
non-objects. Self-review added arrays to that drop list, since
typeof [] === 'object'and an array would otherwise normalise into abogus all-zero row.
The test i18n mock is synced with the real locale files. It carried
a stale
unit.tokensthat no longer exists and never hadunit.texts,which embedding rows use. One correction to the finding: the suite did
already assert rendered unit labels (
"3 images","12.5 sec"), so itwas not fully uncatchable — but neither covered
texts, which isexactly the key that was missing. Added a test asserting the rendered
textslabel so that specific drift is now caught.Also added: null/non-object row survival and string-quantity rendering
tests. Both fail without the normaliser.
Testing
CI. Frontend tests were not runnable in this worktree — vitest hangs
against a symlinked
node_modules— and the Python suite does not run onthis machine at all (importing
xagent.core.model.chathangs, onmaintoo). The component changes were verified by review;
ruff,isortandmypy(pinned versions) are clean on the changed Python. I am relying onCI's frontend jobs rather than claiming a local pass I did not get.