Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughCentralizes model color resolution into a theme-aware shared module, updates frontend components to use alpha-aware color helpers, enhances report/chart generation (locale-aware cost axes, chart alt/summary, PDF metadata/layout), and adds unit/integration tests and truncation utilities for consistent labeling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ReportModule
participant SharedColors
participant PDF
Client->>Server: Request report (locale, filters)
Server->>ReportModule: buildReportData(filters, locale)
ReportModule->>SharedColors: getModelColorRgb(name, {theme: light})
SharedColors-->>ReportModule: rgb(...) / hsl spec
ReportModule->>ReportModule: compute chartDescriptions, truncated labels
ReportModule->>ReportModule: createChartAssets(chart data, formatCostAxisValue)
ReportModule->>SharedColors: getModelColorRgb(...) for chart assets
ReportModule->>PDF: build Typst template (title, alt, summaries, charts)
PDF-->>Server: bytes with metadata (/Title, /Alt, /Figure, /StructTreeRoot)
Server-->>Client: PDF response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/report/index.js (1)
330-342:⚠️ Potential issue | 🟠 MajorUse the locale-aware cost formatter for
top-models.svg.
cost-trend.svgnow usesformatCostAxisValue, buttop-models.svgstill hardcodestoFixed(). That leaves German reports with a different decimal separator and a different rounding policy between the two cost charts.💡 Proposed fix
return { 'cost-trend.svg': lineChart(costTrend, { title: reportData.text.charts.costTrend, valueKey: 'cost', secondaryKey: reportData.meta.filterSummary.viewModeKey === 'daily' ? 'ma7' : null, formatter: (value) => formatCostAxisValue(value, reportData.meta.language), }), 'top-models.svg': horizontalBarChart(topModels, { title: reportData.text.charts.topModels, getValue: (entry) => entry.cost, getLabel: (entry) => entry.name, getColor: (entry) => entry.color, - formatter: (value) => `$${value.toFixed(value >= 100 ? 0 : 2)}`, + formatter: (value) => formatCostAxisValue(value, reportData.meta.language), }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/report/index.js` around lines 330 - 342, Replace the hardcoded formatter for 'top-models.svg' with the locale-aware cost formatter: update the horizontalBarChart call's formatter (the inline function currently using value.toFixed) to call formatCostAxisValue(value, reportData.meta.language) so the bar chart uses the same locale-aware formatting and rounding as the cost-trend chart; ensure you reference the same reportData.meta.language and formatCostAxisValue function used by the lineChart block.
🧹 Nitpick comments (3)
shared/model-colors.js (1)
167-170: Return a fresh spec fromgetModelColorSpec.For known models this returns the exact object stored in
MODEL_COLOR_RULES, so one caller mutatingh/s/lwill change the shared palette globally. Clone the spec before returning.🛡️ Proposed fix
function getModelColorSpec(name, options = {}) { const theme = normalizeTheme(options.theme) const known = findKnownColor(name) - return known ? known[theme] : fallbackColor(name, theme) + return known ? { ...known[theme] } : fallbackColor(name, theme) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/model-colors.js` around lines 167 - 170, getModelColorSpec currently returns the exact object from MODEL_COLOR_RULES for known models, allowing external callers to mutate shared h/s/l values; modify getModelColorSpec so that when findKnownColor(name) returns a spec you return a shallow clone (e.g., new object with copied properties) instead of the original object, preserving normalized theme logic and leaving fallbackColor(name, theme) behavior unchanged; reference getModelColorSpec, findKnownColor, and MODEL_COLOR_RULES when locating where to clone the spec before returning.server/report/utils.js (1)
470-473: KeepfullNamesNotetied to the same truncation rule.This note is meant to expand shortened labels, but it hardcodes
> 30instead of deriving that fromtruncateLabel(). That makes the note easy to desync from whatever the chart actually shortens.♻️ Suggested refactor
const topChartModels = modelRows.slice(0, 8); const truncatedTopModelNames = topChartModels - .filter((entry) => entry.name.length > 30) + .filter((entry) => truncateLabel(entry.name) !== entry.name) .map((entry) => entry.name);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/report/utils.js` around lines 470 - 473, fullNamesNote is hardcoded to check name.length > 30 while the chart uses truncateLabel(), which can desync; update the logic that builds truncatedTopModelNames/fullNamesNote to derive truncation from truncateLabel() instead of a magic number — e.g., call truncateLabel(entry.name) and include the original name when truncateLabel(entry.name) !== entry.name; adjust references around topChartModels, truncatedTopModelNames and wherever fullNamesNote is composed so the note reflects the actual truncation rule.tests/unit/model-colors.test.ts (1)
19-74: Cover the omitted-theme path too.This suite only checks explicit
'light'/'dark', butsrc/lib/model-utils.tsnow has DOM-dependent fallback behavior whenthemeis omitted. A small case arounddocument.documentElement.classListwould protect that new default path from regressing.As per coding guidelines: "Prefer focused
*.test.tsor*.test.tsxcoverage for data transforms, hooks, or complex UI behavior".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/model-colors.test.ts` around lines 19 - 74, The tests miss the codepath where theme is omitted and model-utils.ts falls back to reading document.documentElement.classList; add focused tests that call getModelColor (and optionally getModelColorAlpha/getModelColorRgb) without passing the theme to cover both fallback outcomes: mock or stub document.documentElement.classList.contains('dark') to true and assert getModelColor('GPT-5.4') equals getModelColor('GPT-5.4','dark'), then mock contains('dark') false (or contains('light') true) and assert it equals the explicit 'light' result, and also include a deterministic unknown-model case to ensure the DOM-dependent default is stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@server/report/index.js`:
- Around line 330-342: Replace the hardcoded formatter for 'top-models.svg' with
the locale-aware cost formatter: update the horizontalBarChart call's formatter
(the inline function currently using value.toFixed) to call
formatCostAxisValue(value, reportData.meta.language) so the bar chart uses the
same locale-aware formatting and rounding as the cost-trend chart; ensure you
reference the same reportData.meta.language and formatCostAxisValue function
used by the lineChart block.
---
Nitpick comments:
In `@server/report/utils.js`:
- Around line 470-473: fullNamesNote is hardcoded to check name.length > 30
while the chart uses truncateLabel(), which can desync; update the logic that
builds truncatedTopModelNames/fullNamesNote to derive truncation from
truncateLabel() instead of a magic number — e.g., call truncateLabel(entry.name)
and include the original name when truncateLabel(entry.name) !== entry.name;
adjust references around topChartModels, truncatedTopModelNames and wherever
fullNamesNote is composed so the note reflects the actual truncation rule.
In `@shared/model-colors.js`:
- Around line 167-170: getModelColorSpec currently returns the exact object from
MODEL_COLOR_RULES for known models, allowing external callers to mutate shared
h/s/l values; modify getModelColorSpec so that when findKnownColor(name) returns
a spec you return a shallow clone (e.g., new object with copied properties)
instead of the original object, preserving normalized theme logic and leaving
fallbackColor(name, theme) behavior unchanged; reference getModelColorSpec,
findKnownColor, and MODEL_COLOR_RULES when locating where to clone the spec
before returning.
In `@tests/unit/model-colors.test.ts`:
- Around line 19-74: The tests miss the codepath where theme is omitted and
model-utils.ts falls back to reading document.documentElement.classList; add
focused tests that call getModelColor (and optionally
getModelColorAlpha/getModelColorRgb) without passing the theme to cover both
fallback outcomes: mock or stub
document.documentElement.classList.contains('dark') to true and assert
getModelColor('GPT-5.4') equals getModelColor('GPT-5.4','dark'), then mock
contains('dark') false (or contains('light') true) and assert it equals the
explicit 'light' result, and also include a deterministic unknown-model case to
ensure the DOM-dependent default is stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dfd44ea3-6fd4-45ae-8ad0-8b6c0f95ecb1
📒 Files selected for processing (17)
CHANGELOG.mdserver/report/charts.jsserver/report/index.jsserver/report/utils.jsshared/model-colors.d.tsshared/model-colors.jssrc/components/layout/FilterBar.tsxsrc/components/tables/ModelEfficiency.tsxsrc/components/tables/RecentDays.tsxsrc/lib/constants.tssrc/lib/model-utils.tssrc/locales/de/common.jsonsrc/locales/en/common.jsontests/integration/server.test.tstests/unit/model-colors.test.tstests/unit/report-charts.test.tstests/unit/report-utils.test.ts
💤 Files with no reviewable changes (1)
- src/lib/constants.ts
This was referenced Apr 15, 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
Localization
Tests