Conversation
…drilldown Provider ranking adds avgCostPerRequest and avgCostPerMillionTokens with null-safe division guards. Cache-hit ranking gains per-provider expandable model-level breakdown (modelStats) via a second grouped query keyed by billingModelSource. LeaderboardTable now supports generic expandable rows. API route formats new fields immutably via spread pattern. i18n keys added for all 5 locales. 16 new tests cover repository semantics, API formatting, and multi-provider model grouping.
📝 WalkthroughWalkthrough此 PR 为提供商排行榜添加成本效率指标（每请求平均成本和每百万 Token 平均成本），为表格引入可展开行功能以显示按模型的缓存命中统计，跨五种语言扩展本地化支持，并包含相应测试覆盖。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 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 |
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the leaderboard functionality by introducing granular cost-efficiency metrics for providers and a detailed, expandable view of cache hit rates at the model level. These additions provide deeper insights into provider performance and resource utilization, while UI enhancements ensure a more interactive and informative user experience. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
size/L
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return ( | ||
| <div className="px-8 py-3"> | ||
| <div className="text-xs text-muted-foreground mb-2 font-medium"> |
There was a problem hiding this comment.
Expand label is inverted
The expanded section currently renders {t("expandModelStats")} even when the row is already expanded. This makes the UI text incorrect in the expanded state and the new collapseModelStats i18n key is never used. Use collapseModelStats when the row is expanded (and expandModelStats when collapsed), or remove the unused key.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx
Line: 526:528
Comment:
**Expand label is inverted**
The expanded section currently renders `{t("expandModelStats")}` even when the row is already expanded. This makes the UI text incorrect in the expanded state and the new `collapseModelStats` i18n key is never used. Use `collapseModelStats` when the row is expanded (and `expandModelStats` when collapsed), or remove the unused key.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/repository/leaderboard.ts
Line: 500:502
Comment:
**Cache-hit model query unbounded**
`findProviderCacheHitRateLeaderboardWithTimezone` adds a second grouped query (`modelRows`) that reuses the same `whereConditions`, but it does not restrict results to the *top-ranked providers* returned by the first `rankings` query. In periods with many providers/models this will scan and group across all providers matching the date range, even when the UI only displays a small page of providers, which is a real performance regression for this endpoint. Filter the model sub-query by `providerId IN (rankings providerIds)` (or apply the same limit) before grouping.
How can I resolve this? If you propose a fix, please make it concise. |
![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 significant enhancements to the leaderboard feature. It adds average cost metrics (avgCostPerRequest, avgCostPerMillionTokens) to the provider leaderboard and implements an expandable row feature to show a detailed model-level breakdown for cache-hit statistics. The changes are well-structured, spanning the repository, API, and UI layers, and are accompanied by comprehensive i18n updates and new unit tests. The implementation of the expandable table component is robust, and the backend logic correctly handles calculations and data aggregation. I have one suggestion to optimize the database query for fetching model-level statistics, which could improve performance.
| // Model-level cache hit breakdown per provider | ||
| const systemSettings = await getSystemSettings(); | ||
| const billingModelSource = systemSettings.billingModelSource; | ||
| const modelField = | ||
| billingModelSource === "original" | ||
| ? sql<string>`COALESCE(${messageRequest.originalModel}, ${messageRequest.model})` | ||
| : sql<string>`COALESCE(${messageRequest.model}, ${messageRequest.originalModel})`; | ||
|
|
||
| const modelTotalInput = sql<number>`COALESCE(sum(${totalInputTokensExpr})::double precision, 0::double precision)`; | ||
| const modelCacheRead = sql<number>`COALESCE(sum(COALESCE(${messageRequest.cacheReadInputTokens}, 0))::double precision, 0::double precision)`; | ||
| const modelCacheHitRate = sql<number>`COALESCE( | ||
| ${modelCacheRead} / NULLIF(${modelTotalInput}, 0::double precision), | ||
| 0::double precision | ||
| )`; | ||
|
|
||
| const modelRows = await db | ||
| .select({ | ||
| providerId: messageRequest.providerId, | ||
| model: modelField, | ||
| totalRequests: sql<number>`count(*)::double precision`, | ||
| cacheReadTokens: modelCacheRead, | ||
| totalInputTokens: modelTotalInput, | ||
| cacheHitRate: modelCacheHitRate, | ||
| }) | ||
| .from(messageRequest) | ||
| .innerJoin( | ||
| providers, | ||
| and(sql`${messageRequest.providerId} = ${providers.id}`, isNull(providers.deletedAt)) | ||
| ) | ||
| .where( | ||
| and(...whereConditions.filter((c): c is NonNullable<(typeof whereConditions)[number]> => !!c)) | ||
| ) | ||
| .groupBy(messageRequest.providerId, modelField) | ||
| .orderBy(desc(modelCacheHitRate), desc(sql`count(*)`)); | ||
|
|
||
| // Group model stats by providerId | ||
| const modelStatsByProvider = new Map<number, ModelCacheHitStat[]>(); | ||
| for (const row of modelRows) { | ||
| if (!row.model || row.model.trim() === "") continue; | ||
| const stats = modelStatsByProvider.get(row.providerId) ?? []; | ||
| stats.push({ | ||
| model: row.model, | ||
| totalRequests: row.totalRequests, | ||
| cacheReadTokens: row.cacheReadTokens, | ||
| totalInputTokens: row.totalInputTokens, | ||
| cacheHitRate: Math.min(Math.max(row.cacheHitRate ?? 0, 0), 1), | ||
| }); | ||
| modelStatsByProvider.set(row.providerId, stats); | ||
| } |
There was a problem hiding this comment.
The current implementation uses two separate database queries to fetch provider-level and model-level statistics, which are then joined in the application code. This is functionally correct, but could be optimized for better performance and atomicity by using a single query.
Consider using a subquery with a PostgreSQL aggregation function like jsonb_agg to construct the modelStats array directly in the database. This would reduce database round-trips and delegate the joining logic to the database, which is generally more efficient. This would involve creating a subquery for the model stats and then LEFT JOIN-ing it in the main provider query.
![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
🤖 Fix all issues with AI agents
In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:
- Around line 527-530: Replace the action-style i18n key t("expandModelStats")
used as the expanded-section title with a descriptive content key (e.g. add
"modelStatsTitle" = "模型明细" to your locale files) or reuse an existing noun key
like t("columns.model"); update the component where t("expandModelStats") is
called (the t(...) inside the expanded content header in leaderboard-view.tsx)
to use the new/reused key, and remove or stop referencing the unused
"collapseModelStats" key if it’s not needed.
🧹 Nitpick comments (4)
src/app/api/leaderboard/route.ts (1)
200-211: 日志中记录了userName和userTags/userGroups，属于用户可识别信息。
logger.info中包含userName、userTags、userGroups，这些在 GDPR/CCPA 语境下可被视为 PII 或用户标识。虽然这是管理员审计日志且为已有模式，但建议确认日志存储策略是否满足合规要求，或考虑在非 admin 场景下脱敏用户名。tests/unit/repository/leaderboard-provider-metrics.test.ts (1)
110-114: 类型检查断言可读性偏低，建议简化。Lines 110-114 中的
_typeCheck变量赋值实际上是一个编译期类型断言，运行时仅检查toBeDefined()，对象始终为{}，这个expect永远通过。如果目的是类型约束，可以考虑用 TypeScript 的satisfies关键字或expectTypeOf（Vitest 内置），无需运行时断言。建议使用 Vitest 内置的 expectTypeOf
- type HasAvgCostPerRequest = { avgCostPerRequest: number | null }; - const _typeCheck: HasAvgCostPerRequest = {} as Awaited< - ReturnType<typeof findDailyProviderLeaderboard> - >[number]; - expect(_typeCheck).toBeDefined(); + expectTypeOf<Awaited<ReturnType<typeof findDailyProviderLeaderboard>>[number]>() + .toHaveProperty("avgCostPerRequest") + .toEqualTypeOf<number | null>();src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx (1)
232-234:renderExpandedContent在每次渲染时对所有行都会被调用，存在性能隐患。Line 233 对每一行都调用了
renderExpandedContent(row, index)，即使该行处于折叠状态。这是因为返回值同时用于判断是否可展开（chevron 图标 + cursor 样式）和渲染展开内容。当数据量较大或展开内容渲染较复杂时，会产生不必要的开销。建议将「是否可展开」的判断和「展开内容的渲染」解耦，或对
expandedContent进行延迟求值：建议的优化方案
const hasExpandable = renderExpandedContent != null; -const expandedContent = hasExpandable ? renderExpandedContent(row, index) : null; +const canExpand = hasExpandable; // or a separate prop like `isExpandable` const isExpanded = expandedRows.has(rowKey); +const expandedContent = isExpanded && hasExpandable ? renderExpandedContent(row, index) : null;如果需要在折叠时也显示 chevron 图标来指示可展开（当前逻辑是只有 expandedContent 非 null 才显示），可以额外提供一个
isRowExpandable?: (row: T) => boolean回调来做轻量判断。src/repository/leaderboard.ts (1)
512-560: 模型级别缓存命中率子查询：逻辑正确，有两个可改进点。
额外的
getSystemSettings()调用（Line 513）：此函数中已执行了 provider 级查询，再调用一次getSystemSettings()增加了一次 DB roundtrip。考虑将billingModelSource作为参数从调用方传入，或在函数开头一次性获取。
modelField构建逻辑重复：Lines 515-518 与findModelLeaderboardWithTimezone（Lines 653-656）中的逻辑完全相同。可以抽取为一个共享的辅助函数。建议抽取共享辅助函数
+function buildModelField(billingModelSource: string) { + return billingModelSource === "original" + ? sql<string>`COALESCE(${messageRequest.originalModel}, ${messageRequest.model})` + : sql<string>`COALESCE(${messageRequest.model}, ${messageRequest.originalModel})`; +}然后在两处替换使用。
| <div className="px-8 py-3"> | ||
| <div className="text-xs text-muted-foreground mb-2 font-medium"> | ||
| {t("expandModelStats")} | ||
| </div> |
There was a problem hiding this comment.
t("expandModelStats") 用作展开后的节标题语义不准确。
此处已经是展开后的内容区域，但使用了 expandModelStats（"展开模型" / "Expand models"）作为标题，这是一个动作描述而非内容标题。建议新增一个描述性的 i18n key（如 modelStatsTitle = "模型明细"），或者直接复用已有的 columns.model 等键来作为小节标题。collapseModelStats 键在当前实现中也未被使用。
🤖 Prompt for AI Agents
In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx
around lines 527 - 530, Replace the action-style i18n key t("expandModelStats")
used as the expanded-section title with a descriptive content key (e.g. add
"modelStatsTitle" = "模型明细" to your locale files) or reuse an existing noun key
like t("columns.model"); update the component where t("expandModelStats") is
called (the t(...) inside the expanded content header in leaderboard-view.tsx)
to use the new/reused key, and remove or stop referencing the unused
"collapseModelStats" key if it’s not needed.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return r.avgCostPerRequestFormatted ?? r.avgCostPerRequest.toFixed(4); | ||
| }, | ||
| sortKey: "avgCostPerRequest", | ||
| getValue: (row) => (row as ProviderEntry).avgCostPerRequest ?? 0, |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] Sorting null avg-cost values as 0 makes "no data" providers look cheapest
Evidence (new code)
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:282getValue: (row) => (row as ProviderEntry).avgCostPerRequest ?? 0,
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:293getValue: (row) => (row as ProviderEntry).avgCostPerMillionTokens ?? 0,
Backend defines these as null when the denominator is 0 (e.g. avgCostPerRequest: number | null; // ... null when totalRequests === 0 in src/repository/leaderboard.ts). The UI also renders "-" when the value is null, so sorting them as 0 is misleading (first click is ascending).
Suggested fix: allow getValue to return null and make LeaderboardTable sort nulls last for both directions.
// src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx
export interface ColumnDef<T> {
// ...
getValue?: (row: T) => number | string | null | undefined;
}
// In the sort comparator (before number/string comparison):
if (valueA == null && valueB == null) return 0;
if (valueA == null) return 1;
if (valueB == null) return -1;
// src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx
getValue: (row) => (row as ProviderEntry).avgCostPerRequest,
getValue: (row) => (row as ProviderEntry).avgCostPerMillionTokens,
There was a problem hiding this comment.
Code Review Summary
This PR introduces provider avg-cost metrics and adds model-level cache-hit drilldowns, with corresponding UI expansion support and API formatting. One high-confidence issue remains in the new provider avg-cost columns: sorting treats null as 0, which can mis-rank "no data" providers as the cheapest when sorting ascending.
PR Size: L
- Lines changed: 936
- Files changed: 11
- Split suggestion (recommended for L PRs): (1) repository + API contract + unit tests, (2) UI table expansion + view changes, (3) i18n message updates.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 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 |
Critical Issues (Must Fix)
- None
High Priority Issues (Should Fix)
- [HIGH] [LOGIC-BUG]
nullavg-cost values sorted as0misleads ranking on ascending sort (src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:282,src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:293). Confidence: 85
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.
- Reviewed PR
#753and labeled itsize/L(936 lines changed across 11 files) viagh pr edit --add-label "size/L". - Filed 1 inline comment ([HIGH] [LOGIC-BUG]) on new diff lines: sorting
nullavg-cost values as0mis-ranks “no data” providers when sorting ascending (src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:282,src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:293). Suggested fix: allowColumnDef.getValueto returnnulland makeLeaderboardTablesort nulls last (update comparator insrc/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx). - Submitted the required review summary to the PR with
gh pr review 753 --comment --body ...(includes the L-size split suggestion).
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
There was a problem hiding this comment.
Code Review Summary
No significant issues identified in this PR. The implementation is clean, follows project conventions, and includes thorough test coverage. The null-safe division pattern for cost metrics, the generic expandable row support, and the model-level cache hit drilldown are all well-implemented.
PR Size: L
- Lines changed: 936 (893 additions, 43 deletions)
- Files changed: 11
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate (10 repository tests + 3 API tests)
- Code clarity - Good
Automated review by Claude AI
1 participant
Summary
Add two average cost metrics to the provider leaderboard and a per-model expandable drilldown to the cache-hit rate ranking, with full i18n and test coverage.
Problem
The provider leaderboard showed total cost but lacked normalized cost-efficiency metrics (cost per request, cost per million tokens), making it hard to compare providers of different scale. The cache-hit rate ranking showed only provider-level aggregates with no way to see which models contributed to the hit rate.
Related PRs:
Solution
avgCostPerRequest(totalCost / totalRequests) andavgCostPerMillionTokens(totalCost * 1_000_000 / totalTokens) with null-safe division (returnsnullwhen denominator is 0). Add a second grouped query for cache-hit rankings keyed bybillingModelSourceto produce per-modelModelCacheHitStatentries.LeaderboardTablesupport generic expandable rows with chevron toggle. Add 2 new provider columns and a cache-hit model drilldown sub-table.Changes
Core Changes
src/repository/leaderboard.ts-avgCostPerRequest,avgCostPerMillionTokensfields onProviderLeaderboardEntry; newModelCacheHitStatinterface and model sub-query onProviderCacheHitRateLeaderboardEntrysrc/app/api/leaderboard/route.ts- Format new cost fields with currency display; refactored to immutable spread patternsrc/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx- Generic expandable row support (renderExpandedContentprop, chevron toggle,Fragmentkeying)src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx- 2 new provider columns (avg cost/req, avg cost/1M tokens); cache-hit model drilldown with color-coded hit ratesSupporting Changes
messages/{en,ja,ru,zh-CN,zh-TW}/dashboard.json- 4 new i18n keys per locale (avgCostPerRequest,avgCostPerMillionTokens,expandModelStats,collapseModelStats)Breaking Changes
None. All changes are additive - new fields are appended to existing interfaces and API responses. Existing fields and behavior are unchanged.
Testing
Automated Tests
tests/unit/repository/leaderboard-provider-metrics.test.ts(10 tests covering avg-cost computation, null-safe division, model breakdown grouping, empty model exclusion, field preservation)tests/unit/api/leaderboard-route.test.ts(3 new tests covering provider formatted fields, null cost handling, modelStats passthrough)Manual Testing
Changed Files
src/repository/leaderboard.tsavgCostPerRequest,avgCostPerMillionTokens,ModelCacheHitStat, model sub-querysrc/app/api/leaderboard/route.tsleaderboard-table.tsxleaderboard-view.tsxmessages/*/dashboard.jsonleaderboard-provider-metrics.test.tsleaderboard-route.test.tsChecklist
Description enhanced by Claude AI