feat(usage-logs): XLSX export with summary sheet + Excel-safe numeric & timezone-aware timestamps#1245
Conversation
Stream every matching usage log into an XLSX workbook containing a detail sheet (mirrors the CSV) and a daily/hourly summary sheet aggregated per system timezone. Numeric columns are normalized to stay within Excel's 15-significant-digit ceiling so SUM() works, and timestamps are rendered as real Excel dates in the resolved system timezone. The existing CSV export is refactored to share column definitions and normalization logic with the XLSX path. The UI exposes both CSV and XLSX options via a dropdown. Also add i18n labels, API schema additions (format parameter), fflate dependency for ZIP compression, and comprehensive tests.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough将使用日志导出从 CSV-only 扩展为 CSV 与 XLSX：新增导出库（列/CSV/XLSX/数值/时区/汇总）、统一后端构建与异步任务结果存储、更新 API 下载元信息、前端导出下拉菜单与大量测试。 Changes使用日志多格式导出
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
github-actions
Bot
added
enhancement
![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 the ability to export usage logs in XLSX format (complete with hourly or daily summaries) alongside the existing CSV format, utilizing a lightweight, hand-rolled XLSX writer to avoid heavy spreadsheet dependencies. The export process has been refactored to stream logs in batches, normalize numeric precision for Excel compatibility, and render timestamps in the system's timezone. Feedback on these changes focuses on enhancing robustness and standards compliance: specifically, adding a second default fill in styles.xml to comply with the ECMA-376 specification, implementing defensive checks against invalid Date objects to prevent runtime crashes during timezone formatting, validating string types before trimming, and strictly filtering illegal XML 1.0 characters to avoid workbook corruption.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const STYLES_XML = `<?xml version="1.0" encoding="UTF-8" standalone="yes"?> | ||
| <styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><numFmts count="2"><numFmt numFmtId="164" formatCode="yyyy-mm-dd hh:mm:ss"/><numFmt numFmtId="165" formatCode="0.00######"/></numFmts><fonts count="2"><font><sz val="11"/><name val="Calibri"/></font><font><b/><sz val="11"/><name val="Calibri"/></font></fonts><fills count="1"><fill><patternFill patternType="none"/></fill></fills><borders count="1"><border/></borders><cellStyleXfs count="1"><xf numFmtId="0" fontId="0" fillId="0" borderId="0"/></cellStyleXfs><cellXfs count="5"><xf numFmtId="0" fontId="0" fillId="0" borderId="0" xfId="0"/><xf numFmtId="0" fontId="1" fillId="0" borderId="0" xfId="0" applyFont="1"/><xf numFmtId="164" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/><xf numFmtId="1" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/><xf numFmtId="165" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/></cellXfs><cellStyles count="1"><cellStyle name="Normal" xfId="0" builtinId="0"/></cellStyles></styleSheet>`; |
There was a problem hiding this comment.
According to the ECMA-376 (OpenXML) specification, the first two fills in the fills collection of styles.xml are reserved and must be exactly none and gray125 respectively. Providing only one fill can cause strict spreadsheet readers (like Microsoft Excel) to trigger a "We found a problem with some content..." warning and attempt to repair/strip styles from the workbook. Adding the second default fill ensures robust compatibility.
| const STYLES_XML = `<?xml version="1.0" encoding="UTF-8" standalone="yes"?> | |
| <styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><numFmts count="2"><numFmt numFmtId="164" formatCode="yyyy-mm-dd hh:mm:ss"/><numFmt numFmtId="165" formatCode="0.00######"/></numFmts><fonts count="2"><font><sz val="11"/><name val="Calibri"/></font><font><b/><sz val="11"/><name val="Calibri"/></font></fonts><fills count="1"><fill><patternFill patternType="none"/></fill></fills><borders count="1"><border/></borders><cellStyleXfs count="1"><xf numFmtId="0" fontId="0" fillId="0" borderId="0"/></cellStyleXfs><cellXfs count="5"><xf numFmtId="0" fontId="0" fillId="0" borderId="0" xfId="0"/><xf numFmtId="0" fontId="1" fillId="0" borderId="0" xfId="0" applyFont="1"/><xf numFmtId="164" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/><xf numFmtId="1" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/><xf numFmtId="165" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/></cellXfs><cellStyles count="1"><cellStyle name="Normal" xfId="0" builtinId="0"/></cellStyles></styleSheet>`; | |
| const STYLES_XML = `<?xml version="1.0" encoding="UTF-8" standalone="yes"?> | |
| <styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><numFmts count="2"><numFmt numFmtId="164" formatCode="yyyy-mm-dd hh:mm:ss"/><numFmt numFmtId="165" formatCode="0.00######"/></numFmts><fonts count="2"><font><sz val="11"/><name val="Calibri"/></font><font><b/><sz val="11"/><name val="Calibri"/></font></fonts><fills count="2"><fill><patternFill patternType="none"/></fill><fill><patternFill patternType="gray125"/></fill></fills><borders count="1"><border/></borders><cellStyleXfs count="1"><xf numFmtId="0" fontId="0" fillId="0" borderId="0"/></cellStyleXfs><cellXfs count="5"><xf numFmtId="0" fontId="0" fillId="0" borderId="0" xfId="0"/><xf numFmtId="0" fontId="1" fillId="0" borderId="0" xfId="0" applyFont="1"/><xf numFmtId="164" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/><xf numFmtId="1" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/><xf numFmtId="165" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/></cellXfs><cellStyles count="1"><cellStyle name="Normal" xfId="0" builtinId="0"/></cellStyles></styleSheet>`; |
| export function toFiniteNumber(value: string | number | null | undefined): number | null { | ||
| if (value === null || value === undefined) { | ||
| return null; | ||
| } | ||
| if (typeof value === "number") { | ||
| return Number.isFinite(value) ? value : null; | ||
| } | ||
| const trimmed = value.trim(); | ||
| if (trimmed === "") { | ||
| return null; | ||
| } | ||
| const parsed = Number(trimmed); | ||
| return Number.isFinite(parsed) ? parsed : null; | ||
| } |
There was a problem hiding this comment.
In toFiniteNumber, if value is not a string at runtime (e.g., if it is a boolean or an object), calling value.trim() will throw a TypeError. To prevent potential runtime crashes, we should defensively check that value is a string before calling .trim().
export function toFiniteNumber(value: string | number | null | undefined): number | null {
if (value === null || value === undefined) {
return null;
}
if (typeof value === "number") {
return Number.isFinite(value) ? value : null;
}
if (typeof value !== "string") {
return null;
}
const trimmed = value.trim();
if (trimmed === "") {
return null;
}
const parsed = Number(trimmed);
return Number.isFinite(parsed) ? parsed : null;
}| function stripIllegalXmlChars(value: string): string { | ||
| let out = ""; | ||
| for (const char of value) { | ||
| const code = char.codePointAt(0) ?? 0; | ||
| if (code === 0x09 || code === 0x0a || code === 0x0d || code >= 0x20) { | ||
| out += char; | ||
| } | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
The current implementation of stripIllegalXmlChars allows surrogate code points in the range [0xD800, 0xDFFF] if they are unpaired, as well as other illegal XML 1.0 characters like \uFFFE or \uFFFF. To strictly adhere to the XML 1.0 specification and prevent spreadsheet corruption, we should filter characters precisely according to the allowed XML character ranges.
function stripIllegalXmlChars(value: string): string {
let out = "";
for (const char of value) {
const code = char.codePointAt(0) ?? 0;
if (
code === 0x09 ||
code === 0x0a ||
code === 0x0d ||
(code >= 0x20 && code <= 0xd7ff) ||
(code >= 0xe000 && code <= 0xfffd) ||
(code >= 0x10000 && code <= 0x10ffff)
) {
out += char;
}
}
return out;
}| if (column.kind === "datetime") { | ||
| return raw instanceof Date ? dateCell(ref, toExcelZonedDate(raw, timezone)) : `<c r="${ref}"/>`; | ||
| } |
There was a problem hiding this comment.
If raw is an invalid Date object (e.g., new Date(NaN)), raw instanceof Date will still evaluate to true. Passing an invalid Date to toExcelZonedDate will cause formatInTimeZone to throw a RangeError: Invalid time value and crash the export process. We should defensively guard against invalid Date objects by checking !isNaN(raw.getTime()).
| if (column.kind === "datetime") { | |
| return raw instanceof Date ? dateCell(ref, toExcelZonedDate(raw, timezone)) : `<c r="${ref}"/>`; | |
| } | |
| if (column.kind === "datetime") { | |
| return raw instanceof Date && !isNaN(raw.getTime()) ? dateCell(ref, toExcelZonedDate(raw, timezone)) : `<c r="${ref}"/>`; | |
| } |
| if (!log.createdAt) { | ||
| unknown ??= emptyRow(UNKNOWN_PERIOD); | ||
| accumulate(unknown, log); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If log.createdAt is an invalid Date object, !log.createdAt will evaluate to false (since the object is truthy). This will bypass the null check and call formatInTimeZone with an invalid Date, throwing a RangeError and crashing the export. We should defensively guard against invalid Date objects by checking !isNaN(log.createdAt.getTime()).
| if (!log.createdAt) { | |
| unknown ??= emptyRow(UNKNOWN_PERIOD); | |
| accumulate(unknown, log); | |
| return; | |
| } | |
| if (!log.createdAt || isNaN(log.createdAt.getTime())) { | |
| unknown ??= emptyRow(UNKNOWN_PERIOD); | |
| accumulate(unknown, log); | |
| return; | |
| } |
| case "datetime": | ||
| return value instanceof Date ? formatExportTimestamp(value, timezone) : ""; |
There was a problem hiding this comment.
If value is an invalid Date object, value instanceof Date will still evaluate to true. Passing it to formatExportTimestamp will cause formatInTimeZone to throw a RangeError and crash the export. We should defensively guard against invalid Date objects by checking !isNaN(value.getTime()).
| case "datetime": | |
| return value instanceof Date ? formatExportTimestamp(value, timezone) : ""; | |
| case "datetime": | |
| return value instanceof Date && !isNaN(value.getTime()) ? formatExportTimestamp(value, timezone) : ""; |
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function stripIllegalXmlChars(value: string): string { | ||
| let out = ""; | ||
| for (const char of value) { | ||
| const code = char.codePointAt(0) ?? 0; | ||
| if (code === 0x09 || code === 0x0a || code === 0x0d || code >= 0x20) { | ||
| out += char; | ||
| } | ||
| } | ||
| return out; | ||
| } |
There was a problem hiding this comment.
stripIllegalXmlChars guard only excludes C0 control characters (code < 0x20 except TAB/LF/CR), but the XML 1.0 character production also forbids U+FFFE and U+FFFF. These non-characters are technically valid Unicode scalars ≥ 0x20, so they slip through the current filter and could appear in a model or endpoint string that originates from a third-party API. An XML parser that follows the spec would reject the resulting worksheet file.
| function stripIllegalXmlChars(value: string): string { | |
| let out = ""; | |
| for (const char of value) { | |
| const code = char.codePointAt(0) ?? 0; | |
| if (code === 0x09 || code === 0x0a || code === 0x0d || code >= 0x20) { | |
| out += char; | |
| } | |
| } | |
| return out; | |
| } | |
| function stripIllegalXmlChars(value: string): string { | |
| let out = ""; | |
| for (const char of value) { | |
| const code = char.codePointAt(0) ?? 0; | |
| if (code === 0x09 || code === 0x0a || code === 0x0d || (code >= 0x20 && code !== 0xfffe && code !== 0xffff)) { | |
| out += char; | |
| } | |
| } | |
| return out; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/usage-logs/export/xlsx.ts
Line: 66-75
Comment:
The `stripIllegalXmlChars` guard only excludes C0 control characters (code < 0x20 except TAB/LF/CR), but the XML 1.0 character production also forbids U+FFFE and U+FFFF. These non-characters are technically valid Unicode scalars ≥ 0x20, so they slip through the current filter and could appear in a model or endpoint string that originates from a third-party API. An XML parser that follows the spec would reject the resulting worksheet file.
```suggestion
function stripIllegalXmlChars(value: string): string {
let out = "";
for (const char of value) {
const code = char.codePointAt(0) ?? 0;
if (code === 0x09 || code === 0x0a || code === 0x0d || (code >= 0x20 && code !== 0xfffe && code !== 0xffff)) {
out += char;
}
}
return out;
}
```
How can I resolve this? If you propose a fix, please make it concise.| const STYLES_XML = `<?xml version="1.0" encoding="UTF-8" standalone="yes"?> | ||
| <styleSheet xmlns="http://schemas.openxmlformats.org/spreadsheetml/2006/main"><numFmts count="2"><numFmt numFmtId="164" formatCode="yyyy-mm-dd hh:mm:ss"/><numFmt numFmtId="165" formatCode="0.00######"/></numFmts><fonts count="2"><font><sz val="11"/><name val="Calibri"/></font><font><b/><sz val="11"/><name val="Calibri"/></font></fonts><fills count="1"><fill><patternFill patternType="none"/></fill></fills><borders count="1"><border/></borders><cellStyleXfs count="1"><xf numFmtId="0" fontId="0" fillId="0" borderId="0"/></cellStyleXfs><cellXfs count="5"><xf numFmtId="0" fontId="0" fillId="0" borderId="0" xfId="0"/><xf numFmtId="0" fontId="1" fillId="0" borderId="0" xfId="0" applyFont="1"/><xf numFmtId="164" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/><xf numFmtId="1" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/><xf numFmtId="165" fontId="0" fillId="0" borderId="0" xfId="0" applyNumberFormat="1"/></cellXfs><cellStyles count="1"><cellStyle name="Normal" xfId="0" builtinId="0"/></cellStyles></styleSheet>`; |
There was a problem hiding this comment.
fills missing required gray125 entry
The OOXML spec (ECMA-376 Part 1 §18.8.21) reserves the first two fill slots: index 0 = patternType="none" (present) and index 1 = patternType="gray125" (missing). With count="1" and only one fill defined, the stylesheet is technically non-conformant. In practice Excel auto-repairs it, but some Excel versions show a "We found a problem with some content" dialog that can alarm users. All major spreadsheet libraries (ExcelJS, SheetJS, openpyxl) emit both reserved fills. The fix is to change <fills count="1"> to <fills count="2"> and add <fill><patternFill patternType="gray125"/></fill> as the second entry in STYLES_XML.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/usage-logs/export/xlsx.ts
Line: 219-220
Comment:
**OOXML `fills` missing required `gray125` entry**
The OOXML spec (ECMA-376 Part 1 §18.8.21) reserves the first two fill slots: index 0 = `patternType="none"` (present) and index 1 = `patternType="gray125"` (missing). With `count="1"` and only one fill defined, the stylesheet is technically non-conformant. In practice Excel auto-repairs it, but some Excel versions show a "We found a problem with some content" dialog that can alarm users. All major spreadsheet libraries (ExcelJS, SheetJS, openpyxl) emit both reserved fills. The fix is to change `<fills count="1">` to `<fills count="2">` and add `<fill><patternFill patternType="gray125"/></fill>` as the second entry in `STYLES_XML`.
How can I resolve this? If you propose a fix, please make it concise.| c.get("auth") | ||
| ); | ||
| if (!result.ok) return actionError(c, result); | ||
| return new Response(String(result.data), { | ||
| const download = result.data as { | ||
| content: string; | ||
| encoding: "utf8" | "base64"; | ||
| format: "csv" | "xlsx"; | ||
| filename: string; | ||
| }; | ||
| const isXlsx = download.format === "xlsx"; | ||
| const body = | ||
| download.encoding === "base64" ? Buffer.from(download.content, "base64") : download.content; | ||
| const contentType = isXlsx | ||
| ? "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet" | ||
| : "text/csv; charset=utf-8"; | ||
| return new Response(body, { | ||
| headers: { | ||
| "Content-Type": "text/csv; charset=utf-8", | ||
| "Content-Disposition": `attachment; filename="usage-logs-${params.jobId}.csv"`, | ||
| "Content-Type": contentType, | ||
| "Content-Disposition": `attachment; filename="${download.filename}"`, |
There was a problem hiding this comment.
result.data is typed as unknown after callAction, so the cast result.data as { content: string; encoding: ...; ... } silently succeeds even if the runtime shape changes. If downloadUsageLogsExport ever returns a different structure, this cast won't catch the mismatch at compile time and will produce a runtime error at one of the property accesses below. Importing and using UsageLogsExportDownload from @/actions/usage-logs here would give the TypeScript compiler a chance to catch such drift.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/v1/resources/usage-logs/handlers.ts
Line: 135-153
Comment:
**Untyped cast for download payload**
`result.data` is typed as `unknown` after `callAction`, so the cast `result.data as { content: string; encoding: ...; ... }` silently succeeds even if the runtime shape changes. If `downloadUsageLogsExport` ever returns a different structure, this cast won't catch the mismatch at compile time and will produce a runtime error at one of the property accesses below. Importing and using `UsageLogsExportDownload` from `@/actions/usage-logs` here would give the TypeScript compiler a chance to catch such drift.
How can I resolve this? If you propose a fix, please make it concise.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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/dashboard-logs-export-progress-ui.test.tsx (1)
31-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick win这里 mock 错模块了，导出流程不会走到这些桩。
UsageLogsFilters实际依赖的是@/lib/api-client/v1/actions/usage-logs，但这里 mock 的是@/actions/usage-logs。结果就是后面的调用断言绑不到真实依赖，测试很容易变成误报，甚至直接落到真实fetch路径。🤖 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 `@tests/unit/dashboard-logs-export-progress-ui.test.tsx` around lines 31 - 35, The test is mocking the wrong module: change the vi.mock target from "`@/actions/usage-logs`" to the actual dependency "`@/lib/api-client/v1/actions/usage-logs`" so the UsageLogsFilters component uses the mocked functions; keep the same exported mock names (startUsageLogsExportMock, getUsageLogsExportStatusMock, downloadUsageLogsExportMock) in the vi.mock call so the component's calls/expectations hook up to these mocks.
🤖 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 `@src/actions/usage-logs.ts`:
- Around line 340-345: The code silently drops input.format (const { format:
_format, ...filters } = input) and always builds CSV via
buildUsageLogsExport(..., "csv"), causing a mismatch for callers that pass
format: "xlsx"; update the export action in src/actions/usage-logs.ts to either
(A) explicitly validate input.format and throw a clear error when input.format
=== "xlsx" (or any unsupported value) before calling
resolveUsageLogFiltersForSession/resolveSystemTimezone, or (B) narrow the
action's public parameter type to only allow CSV and remove the unused format
destructure; reference the input destructuring,
resolveUsageLogFiltersForSession, resolveSystemTimezone, and
buildUsageLogsExport symbols when making the change so the behavior is explicit
and callers cannot receive an unexpected CSV when they requested XLSX.
In `@src/app/`[locale]/dashboard/logs/_components/usage-logs-filters.tsx:
- Around line 201-202: The export currently uses the draft localFilters
(localFilters -> sanitizeFilters(localFilters)) which causes exported data to
differ from the applied list; change the export to sanitize and use the applied
filters variable (filters) when calling startUsageLogsExport so exportFilters =
sanitizeFilters(filters) and then call startUsageLogsExport({ ...exportFilters,
format: exportFormat }) to ensure exports match the applied list view.
In `@src/lib/usage-logs/export/columns.ts`:
- Around line 34-110: DETAIL_COLUMNS currently contains hardcoded English
headers and buildDetailHeaders appends timezone to them; change this to use i18n
by replacing the header string field with a translation key (e.g., headerKey) in
DETAIL_COLUMNS and update buildDetailHeaders to accept a translator function or
messages object (from next-intl) and return localized strings (formatting
datetime headers with the timezone as before). Specifically, update
DETAIL_COLUMNS entries to use headerKey instead of header, update any code
referencing DETAIL_COLUMNS.header to use the new key, and modify
buildDetailHeaders(timezone) to become buildDetailHeaders(timezone, t) or
buildDetailHeaders(timezone, messages) and call t(headerKey) (or
messages[headerKey]) when building the array so CSV/XLSX exports use next-intl
localized titles for zh-CN, zh-TW, en, ja, ru.
In `@src/lib/usage-logs/export/summary.ts`:
- Around line 35-47: SUMMARY_HEADERS and UNKNOWN_PERIOD are hardcoded English
strings; change the summary builder to accept translated labels from the caller
(or a next-intl formatter) instead of using these constants directly. Replace
SUMMARY_HEADERS and UNKNOWN_PERIOD usage by injecting an object like {
periodLabel, requestsLabel, inputTokensLabel, outputTokensLabel,
cacheWrite5mLabel, cacheWrite1hLabel, cacheReadLabel, totalTokensLabel,
costLabel, unknownPeriodLabel, totalLabel } from the caller so all user-facing
text is localized (supports zh-CN, zh-TW, en, ja, ru) and update any references
in the summary export function(s) to use those passed-in labels. Ensure no
display strings remain hardcoded in SUMMARY_HEADERS, UNKNOWN_PERIOD or the
"Total" label.
In `@tests/unit/api/v1/api-client-actions.test.ts`:
- Around line 440-464: The tests for usageLogs.downloadUsageLogsExport stub the
global fetch with vi.stubGlobal("fetch") but call vi.unstubAllGlobals() only
after assertions, risking mock leakage if an assertion throws; change cleanup to
always run by adding a global afterEach(() => vi.unstubAllGlobals()) in the test
suite (or wrap each test's stub in a try/finally calling vi.unstubAllGlobals())
so fetchMock is always restored; update the tests referencing
downloadUsageLogsExport, fetchMock, and vi.stubGlobal accordingly.
---
Outside diff comments:
In `@tests/unit/dashboard-logs-export-progress-ui.test.tsx`:
- Around line 31-35: The test is mocking the wrong module: change the vi.mock
target from "`@/actions/usage-logs`" to the actual dependency
"`@/lib/api-client/v1/actions/usage-logs`" so the UsageLogsFilters component uses
the mocked functions; keep the same exported mock names
(startUsageLogsExportMock, getUsageLogsExportStatusMock,
downloadUsageLogsExportMock) in the vi.mock call so the component's
calls/expectations hook up to these mocks.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 806c3497-7e8f-426d-ae87-fd8b3d455acf
📒 Files selected for processing (27)
messages/en/dashboard.jsonmessages/ja/dashboard.jsonmessages/ru/dashboard.jsonmessages/zh-CN/dashboard.jsonmessages/zh-TW/dashboard.jsonpackage.jsonsrc/actions/usage-logs.tssrc/app/[locale]/dashboard/logs/_components/usage-logs-filters.tsxsrc/app/api/v1/resources/usage-logs/handlers.tssrc/lib/api-client/v1/actions/usage-logs.tssrc/lib/api-client/v1/openapi-types.gen.tssrc/lib/api/v1/schemas/usage-logs.tssrc/lib/usage-logs/export/columns.tssrc/lib/usage-logs/export/csv.tssrc/lib/usage-logs/export/format.tssrc/lib/usage-logs/export/numeric.tssrc/lib/usage-logs/export/summary.tssrc/lib/usage-logs/export/xlsx.tstests/api/v1/usage-logs/usage-logs.test.tstests/unit/actions/usage-logs-export-retry-count.test.tstests/unit/actions/usage-logs-export-xlsx.test.tstests/unit/api/v1/api-client-actions.test.tstests/unit/dashboard-logs-export-progress-ui.test.tsxtests/unit/usage-logs/export-csv.test.tstests/unit/usage-logs/export-numeric.test.tstests/unit/usage-logs/export-summary.test.tstests/unit/usage-logs/export-xlsx.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR adds XLSX export with a two-sheet workbook (detail + daily/hourly summary), fixes Excel numeric precision for cost values with >15 significant digits, and replaces UTC ISO timestamps with timezone-aware local-time strings in both CSV and XLSX outputs. The implementation is well-tested and architecturally sound, with clean separation of concerns across six new export modules.
PR Size: XL
- Lines changed: 1828 (1699 additions + 129 deletions)
- Files changed: 27
Split suggestion: Consider extracting the XLSX writer (xlsx.ts) into a separate PR as it's a substantial self-contained module (275 lines) with its own test coverage.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 1 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| XML Spec Conformance | 0 | 1 | 1 | 0 |
High Priority Issues (Should Fix)
1. XML Character Filtering Incomplete (xlsx.ts:70)
- Issue:
stripIllegalXmlCharsonly excludes control characters < 0x20 but misses U+FFFE and U+FFFF, which are also illegal in XML 1.0 per the spec. - Impact: If a model name or endpoint contains these non-characters (e.g., from a third-party API), the generated XLSX would be invalid XML and could be rejected by strict parsers.
- Fix: Add
&& code \!== 0xfffe && code \!== 0xffffto the condition on line 69.
2. OOXML Stylesheet Non-Conformance (xlsx.ts:219)
- Issue: STYLES_XML specifies
<fills count="1">with only one fill, but ECMA-376 Part 1 §18.8.21 requires two default fills: index 0 =patternType="none"(present) and index 1 =patternType="gray125"(missing). - Impact: Some Excel versions display a "We found a problem with some content" repair dialog, which can alarm users even though Excel auto-repairs the file.
- Fix: Change to
<fills count="2">and add<fill><patternFill patternType="gray125"/></fill>after the first fill entry.
Medium Priority Issues (Consider Fixing)
3. Untyped Cast in Download Handler (handlers.ts:138)
- Issue:
result.datais cast to an inline type literal{ content: string; encoding: ...; ... }instead of importing and usingUsageLogsExportDownloadfrom the action module. - Impact: If
UsageLogsExportDownloadchanges shape, this inline cast will silently pass compilation but crash at runtime when accessing properties. - Fix: Import
UsageLogsExportDownloadfrom@/actions/usage-logsand cast to that named type instead.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - One unsafe cast (see above)
- Documentation accuracy - Clean
- Test coverage - Comprehensive (6 test files, 80%+ coverage)
- Code clarity - Excellent
- OOXML/XML spec conformance - Two spec issues (see above)
Strengths
- Excellent test coverage across all export modules
- Clean architecture with single responsibility modules
- Streaming batch processing avoids memory issues
- Proper timezone handling throughout
- i18n coverage for all 5 languages
- No emoji in code (CLAUDE.md compliance)
Automated review by Claude AI
…mpliance - Add gray125 fill to styles.xml for OOXML compliance - Tighten XML 1.0 character filter to exclude surrogates and non-characters - Guard against invalid Date values in CSV, summary, and XLSX - Reject synchronous XLSX export with a clear error - Use typed download cast in v1 handler
🧪 测试结果
总体结果: ✅ 所有测试通过 |
| status: 400, | ||
| instance: new URL(c.req.url).pathname, | ||
| errorCode: "usage_logs.xlsx_requires_async", | ||
| detail: "xlsx export requires asynchronous processing (set 'Prefer: respond-async').", |
There was a problem hiding this comment.
[HIGH] [STANDARD-VIOLATION] The new 400 response embeds hardcoded English text instead of using the repository's required i18n flow.
Why this is a problem: CLAUDE.md says, "i18n Required - All user-facing strings must use i18n (5 languages supported). Never hardcode display text". The detail payload here is returned to users when they request format: "xlsx" without Prefer: respond-async, so this is a new user-facing string that bypasses translation entirely.
Suggested fix:
import { getTranslations } from "next-intl/server";
const tErrors = await getTranslations("errors");
return createProblemResponse({
status: 400,
instance: new URL(c.req.url).pathname,
errorCode: "usage_logs.xlsx_requires_async",
detail: tErrors("usage_logs.xlsx_requires_async"),
});Add usage_logs.xlsx_requires_async to each locale's messages/*/errors.json so the error stays localized.
|
|
||
| const usageLogsExportCsvStore = new RedisKVStore<string>({ | ||
| prefix: "cch:usage-logs:export:csv:", | ||
| const usageLogsExportResultStore = new RedisKVStore<string>({ |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] The export result store key changed, so any async CSV job started before this deploy becomes undownloadable after the new code lands.
Why this is a problem: old workers write the finished file under cch:usage-logs:export:csv:${jobId}:csv, but the new download path only reads cch:usage-logs:export:result:${jobId}:result. If a user queues an export and the deployment rolls while it is still running, status can still reach completed while the download endpoint returns "Export file not found or expired".
Suggested fix:
const legacyCsvStore = new RedisKVStore<string>({
prefix: "cch:usage-logs:export:csv:",
defaultTtlSeconds: USAGE_LOGS_EXPORT_JOB_TTL_SECONDS,
});
const content =
(await usageLogsExportResultStore.get(usageLogsExportResultKey(jobId))) ??
(job.format === "csv" ? await legacyCsvStore.get(`${jobId}:csv`) : null);Keep the legacy read path until the old 15-minute TTL window has fully drained.
| row.cacheWrite1h += log.cacheCreation1hInputTokens ?? 0; | ||
| row.cacheRead += log.cacheReadInputTokens ?? 0; | ||
| row.totalTokens += log.totalTokens ?? 0; | ||
| row.cost += toFiniteNumber(log.costUsd) ?? 0; |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] XLSX summary costs are accumulated from raw DB floats instead of the normalized spreadsheet values used in the detail rows.
Why this is a problem: buildDetailRowXml() and buildCsvRows() normalize costUsd to stay within Excel's 15-significant-digit ceiling, but accumulate() does row.cost += toFiniteNumber(log.costUsd) ?? 0;. With values like 1.234567890123456, the visible cells become 1.23456789012346 while the summary total formats the raw sum as 2.46913578024691, so the Total row no longer reconciles with SUM() over the sheet.
Suggested fix:
import Decimal from "decimal.js-light";
function accumulate(row: SummaryRow, log: UsageLogRow): void {
row.cost = new Decimal(row.cost)
.plus(normalizeDecimalForSpreadsheet(log.costUsd))
.toNumber();
}Apply the same decimal-based addition in merge() so day rollups and the final Total row stay consistent.
| const isXlsx = download.format === "xlsx"; | ||
| const body = | ||
| download.encoding === "base64" ? Buffer.from(download.content, "base64") : download.content; | ||
| const contentType = isXlsx |
There was a problem hiding this comment.
[HIGH] [STANDARD-VIOLATION] This handler now serves binary XLSX bytes, but the published /usage-logs/exports/{jobId}/download contract still documents CSV text.
Why this is a problem: The runtime path at src/app/api/v1/resources/usage-logs/handlers.ts:147 switches Content-Type to application/vnd.openxmlformats-officedocument.spreadsheetml.sheet for XLSX jobs, but src/app/api/v1/resources/usage-logs/router.ts still advertises text/csv, and src/lib/api-client/v1/openapi-types.gen.ts was not updated for a binary response. Generated clients will keep treating the payload as text and corrupt XLSX downloads.
Suggested fix:
responses: {
200: {
description: "CSV or XLSX file.",
content: {
"text/csv": { schema: z.string() },
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet": {
schema: z.string().openapi({ format: "binary" }),
},
},
},
}Then regenerate the OpenAPI types so the client contract matches the new binary response.
There was a problem hiding this comment.
Code Review Summary
This PR ships a substantial export pipeline upgrade, but I found a few high-confidence regressions around contract drift and data correctness. The main risks are XLSX summary totals no longer matching the detail sheet normalization, a documented CSV-only download contract that now serves binary XLSX, and a rollout hazard for in-flight async exports.
PR Size: XL
- Lines changed: 1828
- Files changed: 27
- Split suggestion: Separate the export-format plumbing/API-contract work from the XLSX writer and the dashboard UI changes; that would make the binary download contract and the summary math easier to review independently.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 2 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
High Priority Issues (Should Fix)
src/lib/usage-logs/export/summary.ts:71— summarycostis accumulated from raw DB numeric strings instead of the normalized spreadsheet values used in the exported detail rows.src/app/api/v1/resources/usage-logs/handlers.ts:147/src/app/api/v1/resources/usage-logs/router.ts:274/src/lib/api-client/v1/openapi-types.gen.ts:2839— the download endpoint now serves XLSX bytes, but the published API contract still documents CSV text.src/app/api/v1/resources/usage-logs/handlers.ts:98— the new async-only XLSX rejection message is hardcoded English, which violates the repo i18n rule.src/actions/usage-logs.ts:80— the export result Redis key changed, so in-flight CSV jobs from a rolling deploy can become undownloadable.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
Done — I posted the PR review on #1245 and applied size/XL.
I left 4 high-priority inline comments covering:
src/lib/usage-logs/export/summary.ts:71— summary totals use raw numeric values instead of the Excel-normalized values used in the detail sheet.src/app/api/v1/resources/usage-logs/handlers.ts:98— the new async-only XLSX rejection message is hardcoded English, which violates the repo i18n rule.src/app/api/v1/resources/usage-logs/handlers.ts:147/src/app/api/v1/resources/usage-logs/router.ts:274— the download endpoint now serves XLSX bytes, but the published contract still advertises CSV text.src/actions/usage-logs.ts:80— changing the Redis export result key can break downloads for in-flight jobs during a rolling deploy.
I couldn’t run the Vitest suite here because bun isn’t installed in this environment.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
1 participant
背景
使用记录导出存在三个问题，本 PR 一并解决：
cost_usd是numeric(21,15)，值 ≥ 1 时会产生 16+ 位有效数字（如1.234567890123456），超过 Excel 的 15 位有效数字上限，被当作文本，无法直接SUM()。值 < 1（首位为 0）有效数字更少，所以只有「首位不为 0」的数字出问题。toISOString()（UTC），不跟随用户设置的系统时区，难以辨认。改动
1. 数值规整（Excel 友好）
新增
src/lib/usage-logs/export/numeric.ts：normalizeDecimalForSpreadsheet将数值规整为 ≤15 位有效数字、纯小数（无科学计数法）、去除尾随零。CSV 与 XLSX 共用，保证 Excel 把花费识别为数字、SUM()正确。2. 时区
时间戳用
resolveSystemTimezone()（DB → env TZ → UTC）渲染为该时区的本地时间yyyy-MM-dd HH:mm:ss，时区写在列标题里（如Time (Asia/Shanghai)）。这样 Excel 仍能把单元格识别为可排序/可计算的日期，时区信息也一目了然。3. XLSX 导出（含汇总表）
新增手写的轻量 XLSX writer（基于已有的
fflate，无重型依赖）：XLSX 走异步任务流（与现有 CSV 异步导出一致）；下载端点返回二进制并带正确的 Content-Type。
架构
CSV 与 XLSX 共用列定义（
columns.ts）、时区格式化（format.ts）、汇总聚合（summary.ts）。导出逐批流式写入字符串缓冲（CSV 行 / XLSX 行 XML + 增量汇总累加器），不再整体保留UsageLogRow[]，内存占用与 CSV 路径对齐。XLSX 压缩用 fflate 异步zip，避免阻塞事件循环。给 reviewer 的说明
Z）改为系统时区本地时间（时区在表头标注），这是按需求刻意调整的行为；若有下游脚本按 UTC ISO 解析第一列需注意。openpyxl验证：花费为float、SUM正确、时间为真正datetime且时区正确、文本单元格不会被当公式执行。验证
本地全绿（不计成本并行验证）：
bun run typecheck、bun run lint、bun run buildbun run test：6283 passed / 13 skipped / 0 failedbun run test:v1：Critical v1 coverage check passedopenapi:check/openapi:lint本 PR 已经过多智能体代码评审（9 个查找角度 → 验证 → 复扫），并据评审修复了内存/事件循环/契约/测试覆盖等问题。
Related Issues & PRs
src/lib/usage-logs/export/.resolveSystemTimezone()for timezone-aware output; fix(dashboard): unify timezone handling #1231 unifies timezone handling across dashboard statistics/chart views, while this PR applies the same foundation to export timestamps.🤖 Generated with Claude Code
Greptile Summary
This PR adds XLSX export to the usage-logs feature, alongside three targeted fixes: Excel-safe numeric normalization (≤15 significant digits), timezone-aware timestamps in the column header, and a two-sheet workbook (detail + daily/hourly summary). A new
src/lib/usage-logs/export/module tree centralises shared logic for both CSV and XLSX renderers.columns.ts,csv.ts,format.ts,numeric.ts,summary.ts,xlsx.ts): hand-rolled XLSX writer on top of the already-presentfflatecodec; CSV and XLSX share column definitions, timezone formatting, and summary aggregation so they cannot drift.UsageLogRowbatches incrementally and never retain the full row array; the XLSX path further avoids blocking the event loop by running fflate's asynczip.Blobvia the REST endpoint instead of a raw string.Confidence Score: 5/5
Safe to merge; all changed paths are well-covered by new unit and integration tests.
The hand-rolled XLSX writer correctly handles XML safety, Excel epoch conversion, timezone-shifted date serials, and numeric precision. The previously flagged gray125 fill and U+FFFE/FFFF stripping issues are addressed and confirmed by new tests. No logic errors, contract mismatches, or auth boundary issues were found.
No files require special attention. The hardcoded cost-column index in xlsx.ts summaryRowCells is a minor maintenance concern, not a current defect.
Important Files Changed
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "test(api): restore stubbed fetch globals..." | Re-trigger Greptile