fix(usage): disclose the window a truncated read actually covers - #1532
Conversation
The usage API reads the newest managementUsageMaxReadBytes and applies the 7d/30d/all filter afterwards, so on a busy installation both 30d and Available history summarize the same moving byte tail. A measured case had 30d omitting 73.6% of in-range requests while still labelled 30d, and cumulative totals can decrease as older rows fall out of the window. The response now reports snapshotWindowStart/End: the timestamp bounds of the rows the reader loaded, before range and surface filtering. It describes the READ rather than the query, because truncation is a property of the read. Deliberately not a completeness claim. usage.jsonl is appended when a request completes while each row carries the request start time, so a long-running request can be appended after shorter ones that started later. The oldest loaded timestamp therefore does not bound what the dropped prefix contains, and no derived rangeFullyCovered flag would be sound. The dashboard names the loaded window and downgrades the notice from ok to warn, since a total that omits in-range rows is a caveat. This is a partial mitigation, not the fix #1497 asks for: complete aggregation still needs the rollup work tracked in #1008. Refs #1497, #1008
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughThe ChangesUsage history window
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant UsageClient
participant UsageAPI
participant UsagePage
participant LocaleCatalog
UsageClient->>UsageAPI: request /api/usage
UsageAPI-->>UsageClient: snapshot bounds and usage data
UsagePage->>LocaleCatalog: resolve truncation message
UsagePage-->>UsageClient: render localized warning and date range
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7c7814e18
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| start: new Date(data.snapshotWindowStart).toLocaleString(), | ||
| end: new Date(data.snapshotWindowEnd).toLocaleString(), |
There was a problem hiding this comment.
When the dashboard language differs from the browser or operating-system locale, Usage.tsx renders these dates using the browser default, producing a mixed-language warning with the wrong date ordering and punctuation. Pass the existing locale value to both toLocaleString(locale) calls, as this page already does for its other localized values.
Useful? React with 👍 / 👎.
| snapshotWindowStart: window.start, | ||
| snapshotWindowEnd: window.end, |
There was a problem hiding this comment.
These fields add a user-visible management API contract whose pre-filter semantics and nullable bounds are not documented in docs-site/src/content/docs/reference/management-api.md or its translated versions, leaving API consumers unable to discover or correctly interpret them. Update the management API reference alongside this response change.
AGENTS.md reference: AGENTS.md:L234-L235
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/pages/Usage.tsx`:
- Around line 833-838: Update both date formatters in the history truncation
message within Usage to pass the active locale from useI18n() toLocaleString,
preserving the existing snapshotWindowStart and snapshotWindowEnd handling while
ensuring both dates follow the selected GUI locale.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 742f13fc-05cf-4b11-8726-1014c5bca1dd
📒 Files selected for processing (12)
gui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/tr.tsgui/src/i18n/zh-TW.tsgui/src/i18n/zh.tsgui/src/pages/Usage.tsxgui/tests/usage-layout.test.tssrc/server/management/logs-usage-routes.tstests/api-usage.test.ts
| {data.snapshotWindowStart != null && data.snapshotWindowEnd != null | ||
| ? t("usage.historyTruncatedWindow", { | ||
| start: new Date(data.snapshotWindowStart).toLocaleString(), | ||
| end: new Date(data.snapshotWindowEnd).toLocaleString(), | ||
| }) | ||
| : t("usage.historyTruncated")} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pass the active locale into the date formatter.
new Date(data.snapshotWindowStart).toLocaleString() and the matching call for snapshotWindowEnd do not pass a locale argument. Intl-based toLocaleString() falls back to the browser/OS default locale when no argument is given. Every other formatter in this file (formatTokens(summary.totalTokens, locale) at Line 289, formatUsdEstimate(summary.estimatedCostUsd, locale) at Line 306) explicitly threads the locale value from useI18n(). If a user selects a GUI locale that differs from their OS locale (for example, tr GUI on an en-US OS), the truncation window dates render in the wrong locale while every other number on the page respects the selected locale.
Pass locale explicitly to keep the date format consistent with the rest of the page.
🛡️ Proposed fix
{data.snapshotWindowStart != null && data.snapshotWindowEnd != null
? t("usage.historyTruncatedWindow", {
- start: new Date(data.snapshotWindowStart).toLocaleString(),
- end: new Date(data.snapshotWindowEnd).toLocaleString(),
+ start: new Date(data.snapshotWindowStart).toLocaleString(locale),
+ end: new Date(data.snapshotWindowEnd).toLocaleString(locale),
})
: t("usage.historyTruncated")}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {data.snapshotWindowStart != null && data.snapshotWindowEnd != null | |
| ? t("usage.historyTruncatedWindow", { | |
| start: new Date(data.snapshotWindowStart).toLocaleString(), | |
| end: new Date(data.snapshotWindowEnd).toLocaleString(), | |
| }) | |
| : t("usage.historyTruncated")} | |
| {data.snapshotWindowStart != null && data.snapshotWindowEnd != null | |
| ? t("usage.historyTruncatedWindow", { | |
| start: new Date(data.snapshotWindowStart).toLocaleString(locale), | |
| end: new Date(data.snapshotWindowEnd).toLocaleString(locale), | |
| }) | |
| : t("usage.historyTruncated")} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/pages/Usage.tsx` around lines 833 - 838, Update both date formatters
in the history truncation message within Usage to pass the active locale from
useI18n() toLocaleString, preserving the existing snapshotWindowStart and
snapshotWindowEnd handling while ensuring both dates follow the selected GUI
locale.
Three review findings, all correct. The notice said totals cover usage 'from start to end' and that older entries exceed the read limit. Both claims overstate what the data supports, and they overstate it in exactly the way this PR exists to stop: usage.jsonl is appended on request completion while rows carry the request start time, so retained rows can have gaps and the dropped prefix can contain requests whose start times fall inside the displayed interval. The wording now describes the bounds as what they are — the range of request start times among loaded rows — in all eight locales. The filter-invariance test was vacuous. A 256-byte tail retained a single row, so the surface and range filters discarded nothing and the comparisons held trivially; on unmodified dev every compared field was undefined, which also compared equal. It now builds its own fixture where the retained set is 3 rows that the filters cut to 2 and then 1, asserts exact independently computed bounds, and asserts the fields are numeric. The cache test now asserts the second request is a cache hit. Presence-checking the bounds prevented an omitted field from rendering 'Invalid Date' but not an out-of-range one, and the reader preserves whatever a hand-edited row carries. Both bounds now have to round-trip through Date before the detailed wording is used, with a rendered test covering the unrenderable case.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/tests/usage-layout.test.ts`:
- Around line 164-173: Update the global-state setup and teardown around
globalKeys to save each property’s original descriptor with
Object.getOwnPropertyDescriptor(globalThis, key), rather than only its value. In
the cleanup logic, restore existing descriptors with Object.defineProperty and
delete keys whose descriptors were undefined, preserving the original
own-property shape for document, window, navigator, localStorage, and
IS_REACT_ACT_ENVIRONMENT.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d9e7e781-db43-4759-a6d6-244d32e2f27f
📒 Files selected for processing (11)
gui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/tr.tsgui/src/i18n/zh-TW.tsgui/src/i18n/zh.tsgui/src/pages/Usage.tsxgui/tests/usage-layout.test.tstests/api-usage.test.ts
| const globalKeys = ["document", "window", "navigator", "localStorage", "IS_REACT_ACT_ENVIRONMENT"] as const; | ||
| const previous = Object.fromEntries(globalKeys.map(key => [key, Reflect.get(globalThis, key)])); | ||
| const originalFetch = globalThis.fetch; | ||
| const testWindow = new Window({ url: "http://localhost/" }); | ||
| Object.defineProperties(globalThis, { | ||
| document: { configurable: true, value: testWindow.document }, | ||
| window: { configurable: true, value: testWindow }, | ||
| navigator: { configurable: true, value: testWindow.navigator }, | ||
| localStorage: { configurable: true, value: testWindow.localStorage }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the original global-property shape.
Lines 164-165 save only values. Lines 231-233 then define every global key, even when that key did not exist before this test. This leaks own window, document, navigator, localStorage, or IS_REACT_ACT_ENVIRONMENT properties into later tests.
Save each original property descriptor. Restore the descriptor when it exists. Delete the property when it did not exist.
Proposed fix
- const previous = Object.fromEntries(globalKeys.map(key => [key, Reflect.get(globalThis, key)]));
+ const previous = Object.fromEntries(
+ globalKeys.map(key => [key, Object.getOwnPropertyDescriptor(globalThis, key)]),
+ );
...
for (const key of globalKeys) {
- Object.defineProperty(globalThis, key, { configurable: true, value: previous[key] });
+ const descriptor = previous[key];
+ if (descriptor) Object.defineProperty(globalThis, key, descriptor);
+ else Reflect.deleteProperty(globalThis, key);
}Also applies to: 231-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/tests/usage-layout.test.ts` around lines 164 - 173, Update the
global-state setup and teardown around globalKeys to save each property’s
original descriptor with Object.getOwnPropertyDescriptor(globalThis, key),
rather than only its value. In the cleanup logic, restore existing descriptors
with Object.defineProperty and delete keys whose descriptors were undefined,
preserving the original own-property shape for document, window, navigator,
localStorage, and IS_REACT_ACT_ENVIRONMENT.
1 participant
Summary
GET /api/usagereads the newestmanagementUsageMaxReadBytes(64 MiB by default) and applies the 7d/30d/all filter after that read. On a high-volume installation the newest 64 MiB can cover far less than the selected range, so30dandAvailable historysummarize the same moving byte tail while the UI keeps both labels. In the reported case30domitted 73.6% of in-range requests, and cumulative totals can decrease as older rows fall out of the window.This PR does not make aggregation complete. It stops the product from presenting a truncated read as the range the user asked for.
The response now carries
snapshotWindowStart/snapshotWindowEnd: the timestamp bounds of the rows the reader actually loaded, computed oversnapshot.entriesbefore range and surface filtering. That population is deliberate — truncation is a property of the read, not of the query, so the window that matters is the one the reader could see.Why there is no
rangeFullyCoveredflag. An earlier draft proposed one, derived from the oldest retained entry. It is unsound:usage.jsonlis appended when a request completes while each row carries the request start time, so a long-running request can be appended after shorter ones that started later. The oldest loaded timestamp therefore does not bound what the dropped prefix contains, and a flag claiming "this range is complete" could be wrong. Shipping a field whose entire value is trustworthiness, in a state where it can lie, would repeat the defect this issue reports.The dashboard now names the loaded window and the notice moves from
oktowarn— a total that silently omits in-range rows is a caveat, not a status update. The rendering guard uses!= nullso a dashboard talking to an older proxy that omits the fields falls back to the existing generic wording instead of renderingInvalid Date.This is a partial mitigation and does not close #1497. The issue's bar is complete 7d/30d aggregation and monotonic all-time totals, which needs the daily-rollup work tracked in #1008.
Verification
Live reproduction against a real proxy on an isolated home, with a 4,000-row / 40-day ledger and
managementUsageMaxReadBytes: 65536:30dselected — the range claims 30 days; the banner names the ~2 days actually loaded.Selecting
Available historyon the same data returns the identical 280 requests — exactly the confusion #1497 reports — now explained rather than hidden.The wording states observed extrema rather than a covered interval. Review caught an earlier draft that said totals "cover the usage loaded from X to Y" and that "older entries exceed the read limit": both overstate what the data supports, in precisely the way this PR exists to stop. Retained rows can have gaps, and the dropped prefix can contain requests whose start times fall inside the displayed interval.
Automated, on a Linux runner (Bun 1.3.14):
bun x tsc --noEmit— exit 0bun test tests/api-usage.test.ts tests/usage-summary.test.ts tests/usage-log.test.ts— 75 pass, 0 failcd gui && bun run lint— exit 0cd gui && bun run lint:i18n— exit 0cd gui && bun test tests— 772 pass, 0 failcd gui && bun run build— exit 0Red-before evidence: with only the test diff applied to unmodified
dev, 3 of the new API tests fail. Weakening the GUI date guard back to a presence check makes 2 of the GUI tests fail.One pre-existing flake is worth flagging so it is not mistaken for this change:
usage route does not cache a summary whose overlay version changed mid-readfails intermittently. It reproduces on pristineorigin/devfiles at the same rate, so it is not introduced here.New coverage asserts the contract rather than the implementation: the window matches independently computed bounds on a truncated read; range and surface filters do not move it; an untruncated read spans the whole fixture; an empty ledger yields
nullrather thanNaN/Infinity; a cached response carries both fields through unchanged; and an out-of-range bound falls back to the generic wording instead of renderingInvalid Date.The filter-invariance test builds its own fixture rather than reusing the shared one. Review found the first version vacuous: a 256-byte tail retained a single row, so the filters discarded nothing and the comparison held trivially. It now retains three rows that the range filter cuts to two and the surface filter to one, and asserts exact bounds plus numeric types. The cache test now asserts the second request is a real cache hit.
All eight locale modules carry the new string.
Checklist
Refs #1497, #1008