Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughPDF generation now returns an in-memory buffer; chart helpers accept font/formatter and truncate labels; report utils produce localized text, compact-axis formatting, and insight callouts; Typst template made localized/conditional; locales expanded; tests added; release workflow step pinned to Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Server
participant ReportGen as Report Generator
participant Typst as Typst
participant FS as File System
Client->>Server: POST /api/report/pdf (report request)
Server->>ReportGen: buildReportData + render charts (translations & formatters)
ReportGen->>FS: write Typst template + chart SVGs to temp dir
ReportGen->>Typst: invoke Typst compile (capture stderr)
Typst-->>ReportGen: compiled PDF (on disk) / stderr on failure
ReportGen->>FS: read compiled PDF into buffer
ReportGen->>FS: remove temp dir (finally)
ReportGen-->>Server: return { buffer, reportData }
Server->>Client: sendBuffer (Content-Length + headers + PDF buffer)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/report-smoke.js (1)
10-14: Validate fixture structure before reading.daily.If the provided fixture path is valid JSON but not the expected shape, the script fails later with a less clear error. A small upfront check improves smoke-test diagnostics.
💡 Suggested hardening
const fixturePath = process.argv[2] ? path.resolve(process.cwd(), process.argv[2]) : path.join(root, 'examples', 'sample-usage.json'); -const data = JSON.parse(fs.readFileSync(fixturePath, 'utf8')).daily; +const fixture = JSON.parse(fs.readFileSync(fixturePath, 'utf8')); +if (!fixture || !Array.isArray(fixture.daily)) { + throw new Error(`Fixture must contain a top-level "daily" array: ${fixturePath}`); +} +const data = fixture.daily;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/report-smoke.js` around lines 10 - 14, The script assumes parsed JSON has a daily property and then uses it; update the code around fixturePath and data to validate the parsed fixture before accessing .daily: after reading and parsing the file (the code that sets data = JSON.parse(...).daily), parse into a variable (e.g., const fixture = JSON.parse(...)), check that fixture has a 'daily' property of the expected type (for example Array.isArray(fixture.daily) or typeof fixture.daily === 'object' depending on usage), and if the check fails log a clear error including fixturePath and exit with a non-zero code; keep references to fixturePath, data (assign data = fixture.daily after validation) and outDir unchanged so the rest of the script continues to work..github/workflows/release.yml (1)
30-30: Pin this action to a commit SHA for stronger supply-chain security.Line 30 uses a mutable tag (
@v2.1.4). Pin to the full commit SHA instead (optionally with# v2.1.4as a comment) to prevent unintended updates during workflow runs.Update line 30 to:
uses: actions/create-github-app-token@67018539274d69449ef7c02e8e71183d1719ab42 # v2.1.4🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yml at line 30, Replace the mutable tag for the GitHub Action usage so the workflow is pinned to a commit SHA: find the uses: actions/create-github-app-token@v2.1.4 entry and change it to use the full commit SHA (e.g., 67018539274d69449ef7c02e8e71183d1719ab42) and optionally append a comment with the original tag (e.g., # v2.1.4) to document the version while preventing supply-chain drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/report/index.js`:
- Line 5: The token-trend chart still uses hardcoded English legend labels;
import or access the i18n translation function and replace those hardcoded
segment labels in the token-trend chart generation (where you build the
legend/segments — referenced in this file alongside buildReportData,
formatCompactAxis, formatDateAxis) with i18n keys (e.g.
t('report.tokenTrend.segmentX')) so the legend is localized; apply the same
replacement to the other occurrences in the same token-trend block (the other
segment label locations mentioned in the review).
In `@server/report/utils.js`:
- Around line 593-601: The coverage warning currently uses the aggregated bucket
count (filtered.length < 4) which triggers false positives when the user
switches aggregation; change the condition to depend only on raw-day/calendar
coverage by removing the aggregated check and using filteredDaily alone — e.g.
replace the if condition that references filtered.length with one that reads: if
(filteredDaily.length > 0 && filteredDaily.length < 7) so the insights push is
based solely on filteredDaily.
In `@src/locales/de/common.json`:
- Around line 973-975: The "common.notAvailable" string currently uses "n/a"
which is inconsistent with the rest of the German locale that uses "n/v" (see
the "common" object); update the value of common.notAvailable from "n/a" to
"n/v" so the German translations are consistent, and ensure you only change the
value for the key "common.notAvailable" in the de locale (functionally
referenced as common.notAvailable).
---
Nitpick comments:
In @.github/workflows/release.yml:
- Line 30: Replace the mutable tag for the GitHub Action usage so the workflow
is pinned to a commit SHA: find the uses: actions/create-github-app-token@v2.1.4
entry and change it to use the full commit SHA (e.g.,
67018539274d69449ef7c02e8e71183d1719ab42) and optionally append a comment with
the original tag (e.g., # v2.1.4) to document the version while preventing
supply-chain drift.
In `@scripts/report-smoke.js`:
- Around line 10-14: The script assumes parsed JSON has a daily property and
then uses it; update the code around fixturePath and data to validate the parsed
fixture before accessing .daily: after reading and parsing the file (the code
that sets data = JSON.parse(...).daily), parse into a variable (e.g., const
fixture = JSON.parse(...)), check that fixture has a 'daily' property of the
expected type (for example Array.isArray(fixture.daily) or typeof fixture.daily
=== 'object' depending on usage), and if the check fails log a clear error
including fixturePath and exit with a non-zero code; keep references to
fixturePath, data (assign data = fixture.daily after validation) and outDir
unchanged so the rest of the script continues to work.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67232cfe-32fc-462e-93cd-d3fb1c7a7e56
📒 Files selected for processing (12)
.github/workflows/release.ymlCHANGELOG.mdscripts/report-smoke.jsserver.jsserver/report/charts.jsserver/report/index.jsserver/report/utils.jssrc/locales/de/common.jsonsrc/locales/en/common.jsontests/integration/server.test.tstests/unit/report-charts.test.tstests/unit/report-utils.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/report/utils.js`:
- Around line 615-623: The cache insight is incorrectly gated by
metrics.hasRequestData so datasets with valid token-derived cacheHitRate are
suppressed; update the condition in the block that pushes into insights (the
code referencing metrics.hasRequestData and metrics.cacheHitRate) to only check
metrics.cacheHitRate > 0 (i.e., remove the hasRequestData requirement), and
leave the existing translate(...) title/body logic intact (adjust copy only if
you later want to align wording to token-based metrics).
- Around line 702-707: The percent strings are using a fixed '.' decimal
separator; update the percent formatter and its uses so percentages are
localized: change the formatPercent function to accept a language (defaulting to
'en' or existing default), call toLocaleString(getLocale(language), {
minimumFractionDigits: 1, maximumFractionDigits: 1 }) and append '%' (e.g.,
`${...}%`), then update the report summary card callers in
server/report/utils.js (calls to formatPercent(metrics.topProvider.share),
formatPercent(metrics.cacheHitRate), formatPercent(metrics.topModelShare), etc.)
to pass the local `language` variable so percent output matches other localized
numbers.
In `@src/locales/de/common.json`:
- Around line 976-978: The German locale file contains an English eyebrow for
the PDF header; update the value for the "header.eyebrow" key in
src/locales/de/common.json (the JSON key used as report.header.eyebrow) to the
correct German string so that server/report/index.js (which renders
report.header.eyebrow) shows a localized eyebrow in German PDFs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91565b8c-00dd-4ed1-8afb-270e78bc51cf
📒 Files selected for processing (3)
server/report/index.jsserver/report/utils.jssrc/locales/de/common.json
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/report/utils.js`:
- Around line 779-781: Replace the hard-coded toFixed(1) percent formatting in
the labels object for topModel and topProvider with the locale-aware helper used
elsewhere: call formatPercent(...) instead of using .toFixed(1) and remove the
manual "%" suffix; e.g., change the template for topModel from
`${metrics.topModel.name} (${metrics.topModelShare.toFixed(1)}%)` to use
formatPercent(metrics.topModelShare, language), and similarly replace
`${metrics.topProvider.name} (${metrics.topProvider.share.toFixed(1)}%)` with
formatPercent(metrics.topProvider.share, language) so percent formatting matches
summaryCards/insights.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7eeaa485-9032-462e-ab9d-a7a3d4a03e86
📒 Files selected for processing (3)
server/report/utils.jssrc/locales/de/common.jsontests/unit/report-utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/report-utils.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/report/utils.js`:
- Around line 545-567: formatCompactAxis produces locale-aware compact labels
but other report code still uses formatCompact (which hard-codes toFixed(1) and
en suffixes), causing mixed output; fix by unifying the compact formatting:
refactor so formatCompact either delegates to formatCompactAxis (or extract a
shared helper used by both) and accept a language/locale parameter, then update
callers that currently call formatCompact to pass the language and use the
unified implementation so axis labels and summary cards/tables share the same
locale-aware suffixes and decimal formatting.
- Line 708: The label/value mismatch uses metrics.avgDailyCost but labels it per
selected period; update the value to be the period-average cost instead of the
daily average or change the label to state "daily": compute period average
(e.g., totalCost divided by the number of selected periods/period units) and
replace metrics.avgDailyCost with that new metric (or use an existing metric if
one exists), or alternatively change the label produced by
reportDataLabel/translate to indicate "daily" so metrics.avgDailyCost and the
label match; locate the usage in the object where translate(language,
'common.cost') and formatCurrency(metrics.avgDailyCost, language) are used and
update accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f63db68b-7f44-4c1a-a7eb-620e33e432ab
📒 Files selected for processing (2)
server/report/utils.jstests/unit/report-utils.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/unit/report-utils.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/report/utils.js (2)
844-853: Consider usingtranslate()for consistency.This function hardcodes German and English strings while the rest of the file uses the
translate()helper. For consistency and easier maintenance, consider using translation keys.💡 Optional: Use translate() for aggregation labels
Add keys to locale files and use translate:
function reportDataLabel(viewMode, language = 'de') { - if (language === 'en') { - if (viewMode === 'monthly') return 'monthly aggregation'; - if (viewMode === 'yearly') return 'yearly aggregation'; - return 'daily aggregation'; - } - if (viewMode === 'monthly') return 'Monats-Aggregation'; - if (viewMode === 'yearly') return 'Jahres-Aggregation'; - return 'Tages-Aggregation'; + return translate(language, `report.aggregation.${viewMode}`); }With corresponding locale entries:
{ "report": { "aggregation": { "daily": "daily aggregation", "monthly": "monthly aggregation", "yearly": "yearly aggregation" } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/report/utils.js` around lines 844 - 853, The reportDataLabel function currently returns hardcoded German/English strings; replace those returns with calls to the existing translate() helper (e.g. translate('report.aggregation.daily', language)) and add the corresponding keys (report.aggregation.daily, .monthly, .yearly) to the locale files for both 'de' and 'en' so the labels are localized consistently; keep the same function signature (reportDataLabel(viewMode, language = 'de')) and map viewMode values ('daily' default, 'monthly', 'yearly') to the appropriate translation keys.
578-594: Consider requiringemptyKeyfor safer API.If
emptyKeyis omitted from options,translate(language, undefined)returnsundefined, which could propagate as text. Current usages always provide it, but the function signature allows it to be omitted.💡 Optional: Make emptyKey required or provide a sensible default
-function summarizeSelection(values, language, { emptyKey, maxVisible = 3, normalize = (value) => value } = {}) { +function summarizeSelection(values, language, { emptyKey = 'common.noSelection', maxVisible = 3, normalize = (value) => value } = {}) {Or validate at call time:
if (normalized.length === 0) { + if (!emptyKey) throw new Error('emptyKey required when values is empty'); return translate(language, emptyKey); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/report/utils.js` around lines 578 - 594, The summarizeSelection function allows options.emptyKey to be omitted which makes translate(language, undefined) return undefined; fix this by enforcing a required emptyKey: update summarizeSelection to validate that options.emptyKey is provided (e.g., if (!emptyKey) throw or assert) or set a sensible default key (e.g., 'report.filters.none') before calling translate; reference the summarizeSelection function and the emptyKey option so callers and tests can be updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/report/utils.js`:
- Around line 844-853: The reportDataLabel function currently returns hardcoded
German/English strings; replace those returns with calls to the existing
translate() helper (e.g. translate('report.aggregation.daily', language)) and
add the corresponding keys (report.aggregation.daily, .monthly, .yearly) to the
locale files for both 'de' and 'en' so the labels are localized consistently;
keep the same function signature (reportDataLabel(viewMode, language = 'de'))
and map viewMode values ('daily' default, 'monthly', 'yearly') to the
appropriate translation keys.
- Around line 578-594: The summarizeSelection function allows options.emptyKey
to be omitted which makes translate(language, undefined) return undefined; fix
this by enforcing a required emptyKey: update summarizeSelection to validate
that options.emptyKey is provided (e.g., if (!emptyKey) throw or assert) or set
a sensible default key (e.g., 'report.filters.none') before calling translate;
reference the summarizeSelection function and the emptyKey option so callers and
tests can be updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a43777ad-3cd1-4b00-b207-05fbec94847b
📒 Files selected for processing (2)
server/report/utils.jstests/unit/report-utils.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/unit/report-utils.test.ts
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
This was referenced Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores