Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough将表格展开 API 从单一渲染回调替换为基于子行的 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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, 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! 此拉取请求旨在增强排行榜功能，允许用户查看供应商级别数据的模型明细。通过引入新的API参数和前端组件的重构，现在可以以可展开的子行形式在排行榜中直接展示每个供应商的模型统计数据，从而提供更细致的性能洞察，并优化了用户界面体验。 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
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
本次 PR 实现了供应商榜单对模型明细的展开支持，这是一个很棒的功能增强。主要改动包括：
- 重构了
LeaderboardTable组件，用更通用的getSubRows替代了renderExpandedContent，以支持内联子行展示，这使得组件更加灵活和类型安全。 - 更新了后端 API (
/api/leaderboard) 和数据仓库层 (repository)，增加了includeModelStats参数来按需查询和返回供应商下各模型的细分数据。 - 相应地更新了前端视图
LeaderboardView来使用新的表格功能，并调整了列定义以兼容父行和子行（模型行）的数据结构。 - 添加了相应的单元测试来覆盖新的后端逻辑和前端组件功能。
代码整体结构清晰，实现方式合理。我提出了一些关于类型安全和代码复用性的改进建议，希望能帮助您在完成功能时进一步提升代码质量。
| cell: (row) => { | ||
| const r = row as ProviderEntry; | ||
| if (r.avgCostPerRequest == null) return "-"; | ||
| return r.avgCostPerRequestFormatted ?? r.avgCostPerRequest.toFixed(4); | ||
| if (row.avgCostPerRequest == null) return "-"; | ||
| const formatted = (row as { avgCostPerRequestFormatted?: string | null }) | ||
| .avgCostPerRequestFormatted; | ||
| return formatted ?? row.avgCostPerRequest.toFixed(4); | ||
| }, |
There was a problem hiding this comment.
此处以及其他几处 cell 定义中使用了类型断言（如 row as { avgCostPerRequestFormatted?: string | null }）来访问格式化后的字段。这通常表明前端的类型定义与 API 的实际返回数据结构存在差异。
为了提高类型安全性和代码可读性，建议在组件内部为模型统计数据定义一个客户端专用的类型，该类型继承自 ModelProviderStat 并包含由 API 添加的 ...Formatted 字段。这样可以避免在多处进行类型断言，使代码更清晰且更易于维护。
例如，您可以这样定义：
type ModelProviderStatClient = ModelProviderStat & {
totalCostFormatted?: string;
avgCostPerRequestFormatted?: string | null;
avgCostPerMillionTokensFormatted?: string | null;
};
type ProviderRow = ProviderEntry | ModelProviderStatClient;然后在 LeaderboardTable 的 getSubRows prop 中进行一次类型转换，之后所有 cell 函数都可以直接、安全地访问这些属性，无需再次断言。
| const modelRows = await db | ||
| .select({ | ||
| providerId: usageLedger.finalProviderId, | ||
| model: modelField, | ||
| totalRequests: sql<number>`count(*)::double precision`, | ||
| totalCost: sql<string>`COALESCE(sum(${usageLedger.costUsd}), 0)`, | ||
| totalTokens: sql<number>`COALESCE( | ||
| sum( | ||
| ${usageLedger.inputTokens} + | ||
| ${usageLedger.outputTokens} + | ||
| COALESCE(${usageLedger.cacheCreationInputTokens}, 0) + | ||
| COALESCE(${usageLedger.cacheReadInputTokens}, 0) | ||
| )::double precision, | ||
| 0::double precision | ||
| )`, | ||
| successRate: sql<number>`COALESCE( | ||
| count(CASE WHEN ${usageLedger.isSuccess} THEN 1 END)::double precision | ||
| / NULLIF(count(*)::double precision, 0), | ||
| 0::double precision | ||
| )`, | ||
| avgTtfbMs: sql<number>`COALESCE(avg(${usageLedger.ttfbMs})::double precision, 0::double precision)`, | ||
| avgTokensPerSecond: sql<number>`COALESCE( | ||
| avg( | ||
| CASE | ||
| WHEN ${usageLedger.outputTokens} > 0 | ||
| AND ${usageLedger.durationMs} IS NOT NULL | ||
| AND ${usageLedger.ttfbMs} IS NOT NULL | ||
| AND ${usageLedger.ttfbMs} < ${usageLedger.durationMs} | ||
| AND (${usageLedger.durationMs} - ${usageLedger.ttfbMs}) >= 100 | ||
| THEN (${usageLedger.outputTokens}::double precision) | ||
| / ((${usageLedger.durationMs} - ${usageLedger.ttfbMs}) / 1000.0) | ||
| END | ||
| )::double precision, | ||
| 0::double precision | ||
| )`, | ||
| }) |
There was a problem hiding this comment.
此处的模型分解查询（modelRows）与前面的供应商级别查询（rankings）中，用于计算 totalTokens、successRate、avgTtfbMs 和 avgTokensPerSecond 的 SQL 表达式存在大量重复。
这种代码重复会增加维护成本，当指标计算逻辑需要更新时，必须在两个地方同步修改，容易出错。为了遵循 DRY (Don't Repeat Yourself) 原则，建议将这些共享的 SQL 片段提取为常量。
例如：
const totalTokensExpr = sql<number>`COALESCE(sum(...)::double precision, 0::double precision)`;
const successRateExpr = sql<number>`COALESCE(count(...) / NULLIF(count(*), 0), 0)`;
// ... 其他指标
// 在两个查询中复用这些常量
const rankings = await db.select({
// ...
totalTokens: totalTokensExpr,
successRate: successRateExpr,
// ...
});
const modelRows = await db.select({
// ...
totalTokens: totalTokensExpr,
successRate: successRateExpr,
// ...
});这样做可以确保指标计算逻辑的一致性，并使代码更易于维护。
![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)
src/app/api/leaderboard/route.ts (1)
1-12:⚠️ Potential issue | 🟠 Major迁移至 Hono 框架以符合编码指南要求。
根据验证，项目中 19 个 API 路由都使用标准 Next.js App Router 模式，当前文件与其他路由保持一致。然而，编码指南明确要求所有 API 路由应使用 Hono 框架。这是一个系统性问题，影响整个 API 层。建议将此文件及其他相关 API 路由迁移至 Hono 框架以符合项目编码指南。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/leaderboard/route.ts` around lines 1 - 12, The route currently uses Next.js App Router imports (NextRequest/NextResponse) and must be migrated to Hono: replace the exported Next.js handler with a Hono app instance (e.g., create new Hono(), register a GET handler) and use the Hono Context/request/response APIs instead of NextRequest/NextResponse; wire in existing helpers (getSession, logger, getLeaderboardWithCache, getSystemSettings, formatCurrency) inside the Hono handler, adapt getSession to accept the Hono context or extract auth from ctx.req headers/cookies, and return results with ctx.json/ctx.text response methods; ensure you export the Hono app as the default export so the route is served by Hono and update any typing to use Hono-compatible types while keeping references to symbols like getLeaderboardWithCache, getSystemSettings, formatCurrency, and getSession.
🧹 Nitpick comments (3)
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx (1)
420-428: 子行渲染逻辑正确，但类型断言可优化。
data as ProviderEntry[] as ProviderRow[]双重类型断言可以工作，但稍显冗余。♻️ 可选优化：简化类型断言
- data={data as ProviderEntry[] as ProviderRow[]} + data={data as ProviderRow[]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx around lines 420 - 428, The data prop is using a redundant double type assertion ("data as ProviderEntry[] as ProviderRow[]"); simplify by ensuring data is cast once to the correct type or transformed to match ProviderRow before passing to LeaderboardTable — update the call to LeaderboardTable<ProviderRow> to pass data casted directly (e.g., data as ProviderRow[]) or map ProviderEntry->ProviderRow if structural differences exist; locate the LeaderboardTable usage and the types ProviderEntry and ProviderRow to make the single, accurate assertion or conversion.src/repository/leaderboard.ts (1)
514-593: 模型级别统计查询实现正确，但存在 SQL 聚合逻辑重复。模型分组查询（lines 522-567）与基础查询（lines 447-492）存在大量重复的聚合表达式。当前实现可以正常工作，建议后续考虑抽取公共聚合表达式为常量或辅助函数，以减少维护成本。
此外，第 571 行对空模型名的过滤
if (!row.model || row.model.trim() === "") continue;是正确的防御性编程。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/leaderboard.ts` around lines 514 - 593, The model-level stats query duplicates many aggregation expressions already used in the base query; extract the repeated SQL aggregation expressions into shared constants or helper functions (e.g., reusable sql snippets for totalRequests, totalCost, totalTokens, successRate, avgTtfbMs, avgTokensPerSecond) and use those identifiers in both the modelRows select block and the base query to avoid duplication while keeping the existing modelField, modelRows, usageLedger, and baseEntries logic intact; retain the defensive filter for empty model names (the if (!row.model || row.model.trim() === "") continue;) when assembling modelStatsByProvider.src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx (1)
251-257: 展开图标的条件渲染可能导致轻微的视觉不对齐。当同一表格中部分行有子行（显示 chevron）、部分行没有子行（不显示 chevron）时，排名徽章的位置会有细微差异。不过考虑到当前使用场景：
userscope：所有行都没有子行providerscope：所有行都有modelStats（可能为空数组）modelscope：所有行都没有子行在实际使用中，同一 scope 内的行为应该是一致的，所以这个问题影响有限。
如果后续出现混合场景，可以考虑在
hasExpandable为 false 时也渲染一个同尺寸的占位元素：♻️ 可选优化：统一占位元素
- {hasExpandable ? ( + {hasExpandable ? ( isExpanded ? ( <ChevronDown className="h-4 w-4 text-muted-foreground shrink-0" /> ) : ( <ChevronRight className="h-4 w-4 text-muted-foreground shrink-0" /> ) - ) : null} + ) : ( + <div className="h-4 w-4 shrink-0" /> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx around lines 251 - 257, Rows without expanders cause badge alignment shifts; when rendering the chevron block (using hasExpandable and isExpanded with ChevronDown/ChevronRight), render a same-sized invisible placeholder element when hasExpandable is false so the ranking badge stays aligned across rows. Locate the chevron render (checks for hasExpandable and isExpanded and the ChevronDown/ChevronRight usage) and add an element with identical dimensions and spacing but visually hidden (e.g., transparent or aria-hidden) as the fallback when hasExpandable is false.
🤖 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 `@src/app/api/leaderboard/route.ts`:
- Around line 1-12: The route currently uses Next.js App Router imports
(NextRequest/NextResponse) and must be migrated to Hono: replace the exported
Next.js handler with a Hono app instance (e.g., create new Hono(), register a
GET handler) and use the Hono Context/request/response APIs instead of
NextRequest/NextResponse; wire in existing helpers (getSession, logger,
getLeaderboardWithCache, getSystemSettings, formatCurrency) inside the Hono
handler, adapt getSession to accept the Hono context or extract auth from
ctx.req headers/cookies, and return results with ctx.json/ctx.text response
methods; ensure you export the Hono app as the default export so the route is
served by Hono and update any typing to use Hono-compatible types while keeping
references to symbols like getLeaderboardWithCache, getSystemSettings,
formatCurrency, and getSession.
---
Nitpick comments:
In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx:
- Around line 251-257: Rows without expanders cause badge alignment shifts; when
rendering the chevron block (using hasExpandable and isExpanded with
ChevronDown/ChevronRight), render a same-sized invisible placeholder element
when hasExpandable is false so the ranking badge stays aligned across rows.
Locate the chevron render (checks for hasExpandable and isExpanded and the
ChevronDown/ChevronRight usage) and add an element with identical dimensions and
spacing but visually hidden (e.g., transparent or aria-hidden) as the fallback
when hasExpandable is false.
In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:
- Around line 420-428: The data prop is using a redundant double type assertion
("data as ProviderEntry[] as ProviderRow[]"); simplify by ensuring data is cast
once to the correct type or transformed to match ProviderRow before passing to
LeaderboardTable — update the call to LeaderboardTable<ProviderRow> to pass data
casted directly (e.g., data as ProviderRow[]) or map ProviderEntry->ProviderRow
if structural differences exist; locate the LeaderboardTable usage and the types
ProviderEntry and ProviderRow to make the single, accurate assertion or
conversion.
In `@src/repository/leaderboard.ts`:
- Around line 514-593: The model-level stats query duplicates many aggregation
expressions already used in the base query; extract the repeated SQL aggregation
expressions into shared constants or helper functions (e.g., reusable sql
snippets for totalRequests, totalCost, totalTokens, successRate, avgTtfbMs,
avgTokensPerSecond) and use those identifiers in both the modelRows select block
and the base query to avoid duplication while keeping the existing modelField,
modelRows, usageLedger, and baseEntries logic intact; retain the defensive
filter for empty model names (the if (!row.model || row.model.trim() === "")
continue;) when assembling modelStatsByProvider.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (8)
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsxsrc/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsxsrc/app/api/leaderboard/route.tssrc/lib/redis/leaderboard-cache.tssrc/repository/leaderboard.tstests/unit/api/leaderboard-route.test.tstests/unit/dashboard/leaderboard-table-expandable-rows.test.tsxtests/unit/repository/leaderboard-provider-metrics.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 implements provider model breakdown functionality in the leaderboard, allowing users to expand providers to see model-level statistics. The implementation is generally sound with good test coverage, but there are several issues that should be addressed.
PR Size: M
- Lines changed: 790 (625 additions, 165 deletions)
- Files changed: 8
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 2 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 2 | 0 |
High Priority Issues (Should Fix)
[High] [PERFORMANCE-ISSUE] Model stats query lacks pagination
Location: src/repository/leaderboard.ts:796-841
When includeModelStats=true, the model breakdown query fetches all models without any limit. For providers with many models or high volume, this could cause performance issues.
Suggested fix:
const modelRows = await db
.select({...})
.where(...)
.groupBy(usageLedger.finalProviderId, modelField)
.orderBy(desc(sql\`sum(\${usageLedger.costUsd})\`), desc(sql\`count(*)\`))
.limit(100); // Add reasonable limitMedium Priority Issues (Consider Fixing)
[Medium] [STANDARD-VIOLATION] PR marked as incomplete
The PR title contains "[未完成]" (Not Complete), indicating it may not be ready for review.
Suggested fix: Remove "[未完成]" from the title when ready.
[Medium] [TYPE-WEAK-INVARIANT] Type predicate using 'in' operator is fragile
Location: src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:144-153
Using "providerName" in row as a type guard is fragile because it relies on runtime property existence rather than proper type discrimination.
Suggested fix: Use a discriminated union with a literal type field:
interface ProviderEntry { kind: 'provider'; providerId: number; ... }
interface ModelProviderStat { kind: 'model'; model: string; ... }
// Check: row.kind === 'provider'[Medium] [SIMPLIFY-READABILITY] Unnecessary type assertions
Location: src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:158-249
Multiple unnecessary type assertions (row as ProviderEntry) when row is already typed as ProviderRow.
Suggested fix: Remove redundant type assertions throughout the file.
[Medium] [LOGIC-BUG] Empty model filtering logic
Location: src/repository/leaderboard.ts:845
The check if (!row.model || row.model.trim() === "") could be simplified.
Suggested fix: Use optional chaining: if (!row.model?.trim()) continue;
[Medium] [SIMPLIFY-COMPLEXITY] Complex formatting logic can be extracted
Location: src/app/api/leaderboard/route.ts:517-546
The modelStatsFormatted mapping logic is complex and repeats similar patterns.
Suggested fix: Extract a helper function for formatting model stats.
Review Coverage
- Logic and correctness - Minor issues identified
- Security (OWASP Top 10) - Clean
- Error handling - Adequate
- Type safety - Minor improvements needed
- Documentation accuracy - Acceptable
- Test coverage - Good (new tests added)
- Code clarity - Can be improved
Positive Notes
- Good test coverage: New test file added for expandable rows, existing tests extended for includeModelStats
- Proper cache key handling: The
includeModelStatsflag is correctly included in Redis cache keys - Clean separation: Model breakdown logic is well-isolated in the repository layer
- i18n compliance: All user-facing strings use translation keys
Automated review by Claude AI
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/redis/leaderboard-cache.ts (1)
325-337:⚠️ Potential issue | 🟡 Minor
invalidateLeaderboardCache无法清除带includeModelStats的缓存。当前函数不接受
filters参数，无法构造包含:includeModelStats后缀的缓存键。当数据变更需要主动清除缓存时，includeModelStats=true的缓存条目可能变为陈旧数据。考虑扩展签名以支持可选的
filters参数：建议的修改
export async function invalidateLeaderboardCache( period: LeaderboardPeriod, currencyDisplay: string, - scope: LeaderboardScope = "user" + scope: LeaderboardScope = "user", + filters?: LeaderboardFilters ): Promise<void> { const redis = getRedisClient(); if (!redis) { return; } // Resolve timezone once per request const timezone = await resolveSystemTimezone(); - const cacheKey = buildCacheKey(period, currencyDisplay, timezone, scope); + const cacheKey = buildCacheKey(period, currencyDisplay, timezone, scope, undefined, filters); try { await redis.del(cacheKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/redis/leaderboard-cache.ts` around lines 325 - 337, The invalidateLeaderboardCache function currently cannot evict cache entries that include the includeModelStats suffix because it doesn't accept filters; update invalidateLeaderboardCache to accept an optional filters parameter (e.g., filters?: LeaderboardFilters) and pass those filters into buildCacheKey when constructing the cacheKey so keys with :includeModelStats (or other filter-derived suffixes) are generated and deleted; update any internal calls to invalidateLeaderboardCache to supply filters where relevant and ensure buildCacheKey and any key-deletion logic handle a undefined/null filters value gracefully.src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx (1)
248-259:⚠️ Potential issue | 🟠 Major展开交互仅绑定在行点击上，键盘不可达。
Line 248 和 Line 249 把交互挂在
<TableRow>，键盘用户无法稳定触发展开/收起，且缺少aria-expanded语义，属于可访问性阻断问题。建议把交互放到可聚焦按钮上。建议修复（按钮化展开控件）
-<TableRow - className={`${isTopThree ? "bg-muted/50" : ""} ${hasExpandable ? "cursor-pointer" : ""}`} - onClick={hasExpandable ? () => toggleRow(rowKey) : undefined} -> +<TableRow className={`${isTopThree ? "bg-muted/50" : ""}`}> ... - {hasExpandable ? ( - isExpanded ? ( - <ChevronDown className="h-4 w-4 text-muted-foreground shrink-0" /> - ) : ( - <ChevronRight className="h-4 w-4 text-muted-foreground shrink-0" /> - ) - ) : null} + {hasExpandable ? ( + <button + type="button" + className="inline-flex items-center" + onClick={() => toggleRow(rowKey)} + aria-expanded={isExpanded} + aria-label={isExpanded ? t("actions.collapseRow") : t("actions.expandRow")} + > + {isExpanded ? ( + <ChevronDown className="h-4 w-4 text-muted-foreground shrink-0" /> + ) : ( + <ChevronRight className="h-4 w-4 text-muted-foreground shrink-0" /> + )} + </button> + ) : null}As per coding guidelines
**/*.{ts,tsx,js,jsx}: All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx around lines 248 - 259, The row expand/collapse interaction is currently only on <TableRow> (onClick using toggleRow(rowKey)) which is not keyboard-accessible and lacks aria-expanded; move the interactive handler into a focusable button rendered where the chevron icons are (inside <TableCell>), keep toggleRow(rowKey) as the handler, set aria-expanded={isExpanded} on that button, and preserve the row-level hasExpandable cursor styling but remove the row onClick; also add an i18n label (e.g., t('leaderboard.expandRow') / t('leaderboard.collapseRow')) for the button's aria-label or visually hidden text using the existing translation mechanism to avoid hardcoded strings while keeping ChevronDown/ChevronRight icons.
🧹 Nitpick comments (1)
src/repository/leaderboard.ts (1)
522-522:successRate处理不一致：基础条目未做区间限制。模型级统计（Line 574）对
successRate进行了Math.min(Math.max(..., 0), 1)限制，但基础 provider 条目（Line 522）仅使用entry.successRate ?? 0。建议统一处理，避免理论上出现超出[0, 1]的异常值。统一 successRate 处理
successRate: entry.successRate ?? 0, + // 如需与 modelStats 一致，可改为: + // successRate: Math.min(Math.max(entry.successRate ?? 0, 0), 1),Also applies to: 574-574
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/leaderboard.ts` at line 522, The provider entry sets successRate as "entry.successRate ?? 0" which can produce values outside [0,1], inconsistent with the model-level clamp; update the mapping that assigns successRate (the place using entry.successRate) to apply the same clamp used later (e.g., successRate = Math.min(Math.max(entry.successRate ?? 0, 0), 1)) so both provider-level and model-level statistics consistently restrict successRate to the [0,1] range.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx:
- Around line 241-243: The expanded state is computed using rowKey which falls
back to the current index when getRowKey is not provided, so sorting/reordering
causes expansion to attach to the wrong row; update the logic in
leaderboard-table.tsx (referencing getRowKey, getSubRows, expandedRows, and
rowKey) to derive a stable key that does not change on sort—either require or
prefer a stable identifier from the row (e.g., id field) or, if absent, generate
and persist a stable key map when the data first mounts (mapping original item
identity to a stable key) and use that map instead of the mutable index to check
expandedRows.
---
Outside diff comments:
In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx:
- Around line 248-259: The row expand/collapse interaction is currently only on
<TableRow> (onClick using toggleRow(rowKey)) which is not keyboard-accessible
and lacks aria-expanded; move the interactive handler into a focusable button
rendered where the chevron icons are (inside <TableCell>), keep
toggleRow(rowKey) as the handler, set aria-expanded={isExpanded} on that button,
and preserve the row-level hasExpandable cursor styling but remove the row
onClick; also add an i18n label (e.g., t('leaderboard.expandRow') /
t('leaderboard.collapseRow')) for the button's aria-label or visually hidden
text using the existing translation mechanism to avoid hardcoded strings while
keeping ChevronDown/ChevronRight icons.
In `@src/lib/redis/leaderboard-cache.ts`:
- Around line 325-337: The invalidateLeaderboardCache function currently cannot
evict cache entries that include the includeModelStats suffix because it doesn't
accept filters; update invalidateLeaderboardCache to accept an optional filters
parameter (e.g., filters?: LeaderboardFilters) and pass those filters into
buildCacheKey when constructing the cacheKey so keys with :includeModelStats (or
other filter-derived suffixes) are generated and deleted; update any internal
calls to invalidateLeaderboardCache to supply filters where relevant and ensure
buildCacheKey and any key-deletion logic handle a undefined/null filters value
gracefully.
---
Nitpick comments:
In `@src/repository/leaderboard.ts`:
- Line 522: The provider entry sets successRate as "entry.successRate ?? 0"
which can produce values outside [0,1], inconsistent with the model-level clamp;
update the mapping that assigns successRate (the place using entry.successRate)
to apply the same clamp used later (e.g., successRate =
Math.min(Math.max(entry.successRate ?? 0, 0), 1)) so both provider-level and
model-level statistics consistently restrict successRate to the [0,1] range.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (3)
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsxsrc/lib/redis/leaderboard-cache.tssrc/repository/leaderboard.ts
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx
Show resolved
Hide resolved
|
已根据 AI review 做了小幅整理：
说明：本次未对 modelStats 做硬性 limit，以保证“供应商 -> 模型”展开数据的完整性；如后续观察到极端大 model 数场景，可再考虑按 provider 做 top-N 或分页策略。 本地已再次验证： |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (scope === "provider") { | ||
| return ( | ||
| <LeaderboardTable<ProviderRow> | ||
| data={data as ProviderEntry[] as ProviderRow[]} |
There was a problem hiding this comment.
avoid double type assertions - consider refactoring data to be properly typed upstream
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: 421
Comment:
avoid double type assertions - consider refactoring data to be properly typed upstream
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!
| columns={providerColumns} | ||
| getRowKey={(row) => ("providerId" in row ? row.providerId : row.model)} | ||
| getSubRows={(row) => ("modelStats" in row ? row.modelStats : null)} | ||
| getSubRowKey={(row) => ("model" in row ? row.model : row.providerId)} |
There was a problem hiding this comment.
the fallback case in this ternary should never execute since subRow (first parameter) always has model field for provider sub-rows - simplify by using just subRow.model
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: 426
Comment:
the fallback case in this ternary should never execute since `subRow` (first parameter) always has `model` field for provider sub-rows - simplify by using just `subRow.model`
How can I resolve this? If you propose a fix, please make it concise.- 展开按钮化（aria-expanded + i18n label），避免仅依赖行点击 - getRowKey 缺失时在排序/数据变化清空展开态，避免错位 - invalidateLeaderboardCache 支持 dateRange/filters 以清理 includeModelStats 缓存 - successRate 统一 clamp 到 [0,1]，并收敛前端类型断言
|
已按 CodeRabbit / Greptile / Gemini 的 review 修复并补充：
本地已验证： |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/repository/leaderboard.ts (2)
48-50: 建议补全modelStats的语义注释（undefined与[]）。当前注释说明了启用条件，但建议再明确两种返回状态的含义，减少前后端契约歧义。
建议补充注释
- /** 可选：按模型拆分（仅在 includeModelStats=true 时填充） */ + /** + * 可选：按模型拆分 + * - undefined: 未请求 includeModelStats + * - []: 已请求 includeModelStats，但该 provider 下无可用模型统计 + */ modelStats?: ModelProviderStat[];Based on learnings: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/leaderboard.ts` around lines 48 - 50, 为避免前后端契约歧义，补充并明确 leaderboard 接口中 modelStats 字段的语义：说明当 includeModelStats=false 时字段为 undefined（未请求该统计），当 includeModelStats=true 且无数据时返回空数组 []（已请求但无条目）；在注释中引用 modelStats 字段、ModelProviderStat 类型和 includeModelStats 参数以提示使用者这两种不同语义，并在必要时在接口文档或注释中建议前端按 undefined/[] 分支处理。
541-562: 建议为模型级聚合分支补充运行时观测。建议记录该查询的耗时与返回行数，后续可基于真实负载再决定是否引入 top‑N 或分页策略。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/leaderboard.ts` around lines 541 - 562, Wrap the model-level aggregation query call (the await db.select(...).from(...).innerJoin(...).where(...).groupBy(...).orderBy(...)) with simple runtime metrics: capture a start timestamp (e.g., Date.now() or performance.now()), await the query into modelRows, compute duration = now - start, then emit a log/metric including the duration and modelRows.length. Use the existing logging/metrics facility in this module (e.g., processLogger or logger) and include identifiers like usageLedger.finalProviderId and modelField in the log to make the observation actionable.src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx (1)
135-137: 可考虑将includeModelStats改为按需加载以降低默认开销。当前 provider 榜单每次都会请求模型拆分数据；当数据量增大时，查询与响应体都会放大。后续可评估“先加载父行、展开时再请求子行”的策略。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx around lines 135 - 137, The code currently appends "&includeModelStats=1" whenever scope === "provider" which causes the provider leaderboard to always request per-model breakdowns; change this to lazy-load child rows instead: stop adding includeModelStats to the default URL in leaderboard-view.tsx, and implement an onExpand (or loadChildRows) handler that, when a provider row is expanded, issues a targeted request to the same API with includeModelStats=1 and the provider identifier, then merge the returned model stats into the provider row state; reference the variables/functions scope and url in leaderboard-view.tsx and add a fetchModelStats or loadChildRows function to make the on-demand request and update the component state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:
- Around line 135-137: The code currently appends "&includeModelStats=1"
whenever scope === "provider" which causes the provider leaderboard to always
request per-model breakdowns; change this to lazy-load child rows instead: stop
adding includeModelStats to the default URL in leaderboard-view.tsx, and
implement an onExpand (or loadChildRows) handler that, when a provider row is
expanded, issues a targeted request to the same API with includeModelStats=1 and
the provider identifier, then merge the returned model stats into the provider
row state; reference the variables/functions scope and url in
leaderboard-view.tsx and add a fetchModelStats or loadChildRows function to make
the on-demand request and update the component state.
In `@src/repository/leaderboard.ts`:
- Around line 48-50: 为避免前后端契约歧义，补充并明确 leaderboard 接口中 modelStats 字段的语义：说明当
includeModelStats=false 时字段为 undefined（未请求该统计），当 includeModelStats=true
且无数据时返回空数组 []（已请求但无条目）；在注释中引用 modelStats 字段、ModelProviderStat 类型和
includeModelStats 参数以提示使用者这两种不同语义，并在必要时在接口文档或注释中建议前端按 undefined/[] 分支处理。
- Around line 541-562: Wrap the model-level aggregation query call (the await
db.select(...).from(...).innerJoin(...).where(...).groupBy(...).orderBy(...))
with simple runtime metrics: capture a start timestamp (e.g., Date.now() or
performance.now()), await the query into modelRows, compute duration = now -
start, then emit a log/metric including the duration and modelRows.length. Use
the existing logging/metrics facility in this module (e.g., processLogger or
logger) and include identifiers like usageLedger.finalProviderId and modelField
in the log to make the observation actionable.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (5)
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsxsrc/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsxsrc/lib/redis/leaderboard-cache.tssrc/repository/leaderboard.tstests/unit/dashboard/leaderboard-table-expandable-rows.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/dashboard/leaderboard-table-expandable-rows.test.tsx
| url += `&providerType=${encodeURIComponent(providerTypeFilter)}`; | ||
| } | ||
| if (scope === "provider") { | ||
| url += "&includeModelStats=1"; |
There was a problem hiding this comment.
includeModelStats always set to 1 for provider scope with no option to disable. Consider making this configurable if large datasets cause performance issues.
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: 136
Comment:
`includeModelStats` always set to `1` for provider scope with no option to disable. Consider making this configurable if large datasets cause performance issues.
How can I resolve this? If you propose a fix, please make it concise.| type ProviderRow = ProviderEntry | ModelProviderStatClient; | ||
| type ProviderCacheHitRateEntry = ProviderCacheHitRateLeaderboardEntry; | ||
| type ModelEntry = ModelLeaderboardEntry & { totalCostFormatted?: string }; | ||
| type ProviderCacheHitRateRow = ProviderCacheHitRateEntry | ModelCacheHitStat; |
There was a problem hiding this comment.
Union type suggests data array contains both parents and children, but only parents are in array (children nested in modelStats). Rename to ProviderEntryRow for clarity.
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: 46-48
Comment:
Union type suggests data array contains both parents and children, but only parents are in array (children nested in `modelStats`). Rename to `ProviderEntryRow` for clarity.
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!
| const modelRows = await db | ||
| .select({ | ||
| providerId: usageLedger.finalProviderId, | ||
| model: modelField, | ||
| totalRequests: totalRequestsExpr, | ||
| totalCost: totalCostExpr, | ||
| totalTokens: totalTokensExpr, | ||
| successRate: successRateExpr, | ||
| avgTtfbMs: avgTtfbMsExpr, | ||
| avgTokensPerSecond: avgTokensPerSecondExpr, | ||
| }) | ||
| .from(usageLedger) | ||
| .innerJoin( | ||
| providers, | ||
| and(sql`${usageLedger.finalProviderId} = ${providers.id}`, isNull(providers.deletedAt)) | ||
| ) | ||
| .where( | ||
| and(...whereConditions.filter((c): c is NonNullable<(typeof whereConditions)[number]> => !!c)) | ||
| ) | ||
| .groupBy(usageLedger.finalProviderId, modelField) | ||
| .orderBy(desc(sql`sum(${usageLedger.costUsd})`), desc(sql`count(*)`)); |
There was a problem hiding this comment.
Consider optimizing model breakdown query with WHERE finalProviderId IN (...) filter using IDs from first query, instead of re-joining and re-applying all conditions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/leaderboard.ts
Line: 541-561
Comment:
Consider optimizing model breakdown query with `WHERE finalProviderId IN (...)` filter using IDs from first query, instead of re-joining and re-applying all conditions.
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!
- LeaderboardTable 支持 Parent/Sub 泛型，避免 data 误导为父子混合 - provider/cacheHitRate 调用改用 providerId 作为父行 key，子行 key 直接用 model - 补充 ProviderLeaderboardEntry.modelStats 的 undefined/[] 语义注释 - 更新可展开行单测
|
跟进最新的 Greptile / CodeRabbit nitpick（commit: f062847）：
关于“includeModelStats 默认总是加载 / 以及用 WHERE IN 优化 modelRows 查询 / lazy-load”的建议：目前为保证展开体验与数据完整性，暂维持一次性返回；若后续在真实数据集上观测到响应体/耗时压力，再评估按展开懒加载或分页/top-N（需要 API 与 UI 配套改动）。 本地已再次验证： |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx (1)
227-234: 模型子行名称渲染存在重复逻辑，建议抽一个小 helper。当前两处实现一致，后续样式或缩进策略调整时容易漏改。
可选重构示例
+ const renderSubModelLabel = (model: string) => ( + <div className="pl-6"> + <span className="font-mono text-sm">{model}</span> + </div> + ); + const providerColumns: ColumnDef<ProviderTableRow>[] = [ { header: t("columns.provider"), cell: (row) => { if ("providerName" in row) return row.providerName; - return ( - <div className="pl-6"> - <span className="font-mono text-sm">{row.model}</span> - </div> - ); + return renderSubModelLabel(row.model); }, @@ const providerCacheHitRateColumns: ColumnDef<ProviderCacheHitRateTableRow>[] = [ { header: t("columns.provider"), cell: (row) => { if ("providerName" in row) return row.providerName; - return ( - <div className="pl-6"> - <span className="font-mono text-sm">{row.model}</span> - </div> - ); + return renderSubModelLabel(row.model); },Also applies to: 312-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx around lines 227 - 234, The model/sub-row name rendering logic in the cell renderers is duplicated; extract it into a small helper (e.g., formatModelCell or renderModelCell) that accepts the row object and returns the JSX for either row.providerName or the wrapped <div><span className="font-mono text-sm">{row.model}</span></div>; replace the inline logic in the cell renderers (current occurrences in the anonymous cell function around the shown block and the similar block at the later occurrence) to call this helper so future style/indent changes are updated in one place.tests/unit/dashboard/leaderboard-table-expandable-rows.test.tsx (1)
54-113: 建议补充“再次点击收起”的断言，完善展开状态回归保护。当前只验证了展开后的可见性，建议同时校验
aria-expanded从true回到false且子行再次隐藏。可选补丁
@@ act(() => { expandButton!.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); + expect(expandButton!.getAttribute("aria-expanded")).toBe("true"); @@ const modelRow = modelCell!.closest("tr"); expect(modelRow).toBeTruthy(); expect(modelRow!.className).toContain("bg-muted/30"); + + act(() => { + expandButton!.dispatchEvent(new MouseEvent("click", { bubbles: true })); + }); + expect(expandButton!.getAttribute("aria-expanded")).toBe("false"); + expect(findCellByText("model-x")).toBeNull(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/dashboard/leaderboard-table-expandable-rows.test.tsx` around lines 54 - 113, Test lacks assertion that clicking the expand button a second time collapses the sub-rows; after the existing act() that clicks expand, add a second act() that clicks the same expandButton (or re-query it) and then assert that expandButton.getAttribute("aria-expanded") is "false" and that findCellByText("model-x") returns null (and optionally that the model row is not present). Update the test in leaderboard-table-expandable-rows.test.tsx around the expandButton / modelCell assertions to perform this collapse click and the two checks so the toggle behavior is fully verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/repository/leaderboard.ts`:
- Around line 569-577: The model name is only whitespace-checked (if
(!row.model?.trim()) continue) but the original row.model is still used when
pushing into stats, causing visually identical models with extra spaces to be
treated as distinct; before computing avgCosts and pushing into
modelStatsByProvider, normalize the model string by calling row.model =
row.model.trim() (or assign a trimmed const like const model = row.model.trim())
and use that trimmed identifier in computeAvgCosts input and in the object
pushed to stats so all downstream code (e.g., computeAvgCosts and
modelStatsByProvider entries) consistently use the trimmed model name.
---
Nitpick comments:
In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:
- Around line 227-234: The model/sub-row name rendering logic in the cell
renderers is duplicated; extract it into a small helper (e.g., formatModelCell
or renderModelCell) that accepts the row object and returns the JSX for either
row.providerName or the wrapped <div><span className="font-mono
text-sm">{row.model}</span></div>; replace the inline logic in the cell
renderers (current occurrences in the anonymous cell function around the shown
block and the similar block at the later occurrence) to call this helper so
future style/indent changes are updated in one place.
In `@tests/unit/dashboard/leaderboard-table-expandable-rows.test.tsx`:
- Around line 54-113: Test lacks assertion that clicking the expand button a
second time collapses the sub-rows; after the existing act() that clicks expand,
add a second act() that clicks the same expandButton (or re-query it) and then
assert that expandButton.getAttribute("aria-expanded") is "false" and that
findCellByText("model-x") returns null (and optionally that the model row is not
present). Update the test in leaderboard-table-expandable-rows.test.tsx around
the expandButton / modelCell assertions to perform this collapse click and the
two checks so the toggle behavior is fully verified.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (4)
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsxsrc/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsxsrc/repository/leaderboard.tstests/unit/dashboard/leaderboard-table-expandable-rows.test.tsx
- provider/modelStats 聚合使用 TRIM+NULLIF 规范化模型名，避免空白差异导致重复行 - 提取 renderSubModelLabel，消除模型子行缩进渲染重复 - 单测补充二次点击收起断言（aria-expanded 回落 + 子行隐藏）
|
跟进 CodeRabbit 最新的 CHANGES_REQUESTED / nitpick（commit: f49386f）：
本地已再次验证： |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/repository/leaderboard.ts (1)
696-717: 可选：复用clampRatio01，减少重复夹紧逻辑。当前这里仍有两处手写
Math.min(Math.max(...))，可直接复用已有 helper，后续维护更一致。♻️ 可选补丁
- cacheHitRate: Math.min(Math.max(row.cacheHitRate ?? 0, 0), 1), + cacheHitRate: clampRatio01(row.cacheHitRate), ... - cacheHitRate: Math.min(Math.max(entry.cacheHitRate ?? 0, 0), 1), + cacheHitRate: clampRatio01(entry.cacheHitRate),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/leaderboard.ts` around lines 696 - 717, Replace the manual clamping logic Math.min(Math.max(..., 0), 1) with the existing helper clampRatio01 to avoid duplication: in the loop where you push stats (currently clamping row.cacheHitRate) use clampRatio01(row.cacheHitRate) and in the return mapping (currently clamping entry.cacheHitRate) use clampRatio01(entry.cacheHitRate); update imports/usages if necessary so clampRatio01 is referenced in leaderboard.ts.src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx (1)
398-443: 建议减少分支中的as断言，降低后续维护风险。
renderTable中多处分支强转可读性尚可，但后续字段演进时更容易产生静默类型漂移；可考虑按 scope 拆分成小型渲染函数并在函数边界收敛类型。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx around lines 398 - 443, Split renderTable into small, typed renderer functions to avoid repeated "as" assertions: create renderUserTable(): JSX.Element typed to UserEntry[] that returns LeaderboardTable<UserEntry> with data typed as UserEntry[], renderProviderTable(): JSX.Element typed to ProviderEntry[] returning LeaderboardTable<ProviderEntry, ModelProviderStatClient> (no casts), renderProviderCacheHitRateTable(): JSX.Element typed to ProviderCacheHitRateEntry[] returning LeaderboardTable<ProviderCacheHitRateEntry, ModelCacheHitStat>, and renderModelTable(): JSX.Element typed to ModelEntry[] returning LeaderboardTable<ModelEntry>; then have renderTable switch on scope and call the appropriate renderer. Ensure each renderer uses the matching columns (userColumns, providerColumns, providerCacheHitRateColumns, modelColumns) and fully-typed getRowKey/getSubRows/getSubRowKey callbacks so there are no inline "as" casts in renderTable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/app/`[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx:
- Around line 398-443: Split renderTable into small, typed renderer functions to
avoid repeated "as" assertions: create renderUserTable(): JSX.Element typed to
UserEntry[] that returns LeaderboardTable<UserEntry> with data typed as
UserEntry[], renderProviderTable(): JSX.Element typed to ProviderEntry[]
returning LeaderboardTable<ProviderEntry, ModelProviderStatClient> (no casts),
renderProviderCacheHitRateTable(): JSX.Element typed to
ProviderCacheHitRateEntry[] returning
LeaderboardTable<ProviderCacheHitRateEntry, ModelCacheHitStat>, and
renderModelTable(): JSX.Element typed to ModelEntry[] returning
LeaderboardTable<ModelEntry>; then have renderTable switch on scope and call the
appropriate renderer. Ensure each renderer uses the matching columns
(userColumns, providerColumns, providerCacheHitRateColumns, modelColumns) and
fully-typed getRowKey/getSubRows/getSubRowKey callbacks so there are no inline
"as" casts in renderTable.
In `@src/repository/leaderboard.ts`:
- Around line 696-717: Replace the manual clamping logic Math.min(Math.max(...,
0), 1) with the existing helper clampRatio01 to avoid duplication: in the loop
where you push stats (currently clamping row.cacheHitRate) use
clampRatio01(row.cacheHitRate) and in the return mapping (currently clamping
entry.cacheHitRate) use clampRatio01(entry.cacheHitRate); update imports/usages
if necessary so clampRatio01 is referenced in leaderboard.ts.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (3)
src/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsxsrc/repository/leaderboard.tstests/unit/dashboard/leaderboard-table-expandable-rows.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/dashboard/leaderboard-table-expandable-rows.test.tsx
| const modelStatsByProvider = new Map<number, ModelProviderStat[]>(); | ||
| for (const row of modelRows) { | ||
| if (!row.model) continue; | ||
| const totalCost = parseFloat(row.totalCost); | ||
| const totalRequests = row.totalRequests; | ||
| const totalTokens = row.totalTokens; | ||
| const avgCosts = computeAvgCosts(totalCost, totalRequests, totalTokens); | ||
| const stats = modelStatsByProvider.get(row.providerId) ?? []; | ||
| stats.push({ | ||
| model: row.model, | ||
| totalRequests, | ||
| totalCost, | ||
| totalTokens, | ||
| successRate: clampRatio01(row.successRate), | ||
| avgTtfbMs: row.avgTtfbMs ?? 0, | ||
| avgTokensPerSecond: row.avgTokensPerSecond ?? 0, | ||
| ...avgCosts, | ||
| }); | ||
| modelStatsByProvider.set(row.providerId, stats); | ||
| } |
There was a problem hiding this comment.
no limit on number of models per provider - if a provider has 100+ models, modelStats array could become very large and slow down response
consider adding a LIMIT clause to the model breakdown query or implementing pagination for model stats
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/leaderboard.ts
Line: 568-587
Comment:
no limit on number of models per provider - if a provider has 100+ models, `modelStats` array could become very large and slow down response
consider adding a `LIMIT` clause to the model breakdown query or implementing pagination for model stats
How can I resolve this? If you propose a fix, please make it concise.| const modelStatsFormatted = | ||
| scope === "provider" && Array.isArray(typedEntry.modelStats) | ||
| ? typedEntry.modelStats.map((ms) => { | ||
| const stat = ms as { | ||
| totalCost: number; | ||
| avgCostPerRequest: number | null; | ||
| avgCostPerMillionTokens: number | null; | ||
| } & Record<string, unknown>; | ||
|
|
||
| return { | ||
| ...stat, | ||
| totalCostFormatted: formatCurrency(stat.totalCost, systemSettings.currencyDisplay), | ||
| avgCostPerRequestFormatted: | ||
| stat.avgCostPerRequest != null | ||
| ? formatCurrency(stat.avgCostPerRequest, systemSettings.currencyDisplay) | ||
| : null, | ||
| avgCostPerMillionTokensFormatted: | ||
| stat.avgCostPerMillionTokens != null | ||
| ? formatCurrency(stat.avgCostPerMillionTokens, systemSettings.currencyDisplay) | ||
| : null, | ||
| }; | ||
| }) | ||
| : undefined; |
There was a problem hiding this comment.
type assertion at line 209 assumes modelStats structure matches without runtime validation
if repository schema changes, this could cause silent errors. consider adding runtime type guards or validation
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/api/leaderboard/route.ts
Line: 206-228
Comment:
type assertion at line 209 assumes modelStats structure matches without runtime validation
if repository schema changes, this could cause silent errors. consider adding runtime type guards or validation
How can I resolve this? If you propose a fix, please make it concise.| if (!includeModelStats) return baseEntries; | ||
|
|
||
| // Model breakdown per provider |
There was a problem hiding this comment.
if model stats query fails (lines 546-566), entire request fails with no fallback
consider wrapping the model stats query in try-catch to return base provider stats even if model breakdown fails
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/leaderboard.ts
Line: 535-537
Comment:
if model stats query fails (lines 546-566), entire request fails with no fallback
consider wrapping the model stats query in try-catch to return base provider stats even if model breakdown fails
How can I resolve this? If you propose a fix, please make it concise.| // 当调用方未提供稳定 rowKey 时（回退到 index），排序会导致展开态错位；此时在排序/数据变化时清空展开态，至少避免错位展开。 | ||
| // biome-ignore lint/correctness/useExhaustiveDependencies: 依赖用于在排序/数据变化时触发清空，避免 index key 造成错位展开 | ||
| useEffect(() => { | ||
| if (!getRowKey) { | ||
| setExpandedRows(new Set()); | ||
| } | ||
| }, [data, sortKey, sortDirection, getRowKey]); |
There was a problem hiding this comment.
clearing expanded rows on data/sort changes prevents users from keeping rows expanded when changing sort order
consider preserving expand state across sorts by always requiring stable getRowKey instead of clearing on change
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx
Line: 84-90
Comment:
clearing expanded rows on data/sort changes prevents users from keeping rows expanded when changing sort order
consider preserving expand state across sorts by always requiring stable `getRowKey` instead of clearing on change
How can I resolve this? If you propose a fix, please make it concise.- providerCacheHitRate 的 cacheHitRate 复用 clampRatio01，避免重复实现 - 拆分 render*Table helpers，集中处理 scope 分支与类型断言
|
继续跟进 CodeRabbit nitpick（commit: e9e4a71）：
本地已再次验证： |
| // Model-level cache hit breakdown per provider | ||
| const systemSettings = await getSystemSettings(); |
There was a problem hiding this comment.
Unconditional model-stats secondary query on every cache hit rate request
Unlike findProviderLeaderboardWithTimezone which gates the second DB query behind if (!includeModelStats) return baseEntries; (line 535), findProviderCacheHitRateLeaderboardWithTimezone unconditionally executes the model breakdown query (lines 673–691) and calls getSystemSettings() (which has no in-request caching and hits the DB every call) on every single cache hit rate leaderboard request.
This means:
- Every
GET /api/leaderboard?scope=providerCacheHitRaterequest incurs two full SQL aggregation queries + one extrasystem_settingsDB hit, even when model stats are not needed. route.tsalso callsgetSystemSettings()independently at line 53, so this path now has two redundantsystem_settingsqueries per request.
Consider either:
- Adding an opt-in
includeModelStatsparameter to this function (matching the provider leaderboard design), or - Passing the already-fetched
systemSettingsandbillingModelSourceinto the function to avoid the redundant DB query.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/leaderboard.ts
Line: 657-658
Comment:
**Unconditional model-stats secondary query on every cache hit rate request**
Unlike `findProviderLeaderboardWithTimezone` which gates the second DB query behind `if (!includeModelStats) return baseEntries;` (line 535), `findProviderCacheHitRateLeaderboardWithTimezone` unconditionally executes the model breakdown query (lines 673–691) **and** calls `getSystemSettings()` (which has no in-request caching and hits the DB every call) on **every** single cache hit rate leaderboard request.
This means:
- Every `GET /api/leaderboard?scope=providerCacheHitRate` request incurs two full SQL aggregation queries + one extra `system_settings` DB hit, even when model stats are not needed.
- `route.ts` also calls `getSystemSettings()` independently at line 53, so this path now has two redundant `system_settings` queries per request.
Consider either:
1. Adding an opt-in `includeModelStats` parameter to this function (matching the provider leaderboard design), or
2. Passing the already-fetched `systemSettings` and `billingModelSource` into the function to avoid the redundant DB query.
How can I resolve this? If you propose a fix, please make it concise.| key={idx} | ||
| className={`${col.className || ""} ${shouldBold ? "font-bold" : ""}`} | ||
| > | ||
| {col.cell(subRow, subIndex)} |
There was a problem hiding this comment.
Sub-row index parameter receives within-parent position, not global row index
col.cell(subRow, subIndex) passes subIndex (the sub-row's position within its parent's subRows array: 0, 1, 2, …) as the index argument. For parent rows (line 286), index is the global sorted position (0 = top of table). The ColumnDef.cell signature doesn't distinguish between these two contexts.
If any current or future column renderer uses the index parameter for logic (e.g., alternate row styling, rank-dependent text), sub-rows would receive incorrect values. Consider documenting this semantics difference in the ColumnDef interface, or introduce a separate isSubRow flag so renderers can handle both cases safely:
export interface ColumnDef<T> {
// ...
/** index is the global sorted position for parent rows, and the within-parent
* position for sub-rows. */
cell: (row: T, index: number, isSubRow?: boolean) => React.ReactNode;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/leaderboard/_components/leaderboard-table.tsx
Line: 311
Comment:
**Sub-row `index` parameter receives within-parent position, not global row index**
`col.cell(subRow, subIndex)` passes `subIndex` (the sub-row's position within its parent's `subRows` array: 0, 1, 2, …) as the `index` argument. For parent rows (line 286), `index` is the global sorted position (0 = top of table). The `ColumnDef.cell` signature doesn't distinguish between these two contexts.
If any current or future column renderer uses the `index` parameter for logic (e.g., alternate row styling, rank-dependent text), sub-rows would receive incorrect values. Consider documenting this semantics difference in the `ColumnDef` interface, or introduce a separate `isSubRow` flag so renderers can handle both cases safely:
```ts
export interface ColumnDef<T> {
// ...
/** index is the global sorted position for parent rows, and the within-parent
* position for sub-rows. */
cell: (row: T, index: number, isSubRow?: boolean) => React.ReactNode;
}
```
How can I resolve this? If you propose a fix, please make it concise.
|
已修复： @coderabbitai 如方便请重新给出 approve 以清除当前的 CHANGES_REQUESTED（CI/Checks 均已通过）。 |
|
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Actions performedComments resolved and changes approved. |
|
补充修正：当 本地已回归： |
|
补充优化：对无可展开的行也渲染一个固定宽度的占位（与展开按钮同宽），避免同一榜单中“可展开/不可展开”行的排名徽标左右错位。 本地已回归： |
|
跟进 AI review 的一个潜在语义坑： 当前 PR Checks 已全绿（mergeStateStatus=CLEAN）。 |
1 participant
Summary
供应商榜单（Provider Leaderboard）新增模型级明细展开功能。用户现在可以在供应商排行和缓存命中率排行中展开查看每个供应商下的模型拆分数据。
This PR adds model-level breakdown expansion to the Provider Leaderboard. Users can now expand each provider to view per-model statistics in both the provider consumption ranking and cache hit rate ranking.
Changes
UI Enhancements
getSubRows和getSubRowKey属性，取代原来的renderExpandedContent方式API Changes
includeModelStats参数 (New query parameter):GET /api/leaderboard?scope=provider支持includeModelStats=1参数，返回供应商下各模型的拆分数据includeModelStats开关纳入 Redis 缓存键，确保不同参数请求返回正确缓存Repository Layer
ModelProviderStat类型 (New type): 定义供应商消耗排行的模型级统计结构includeModelStats时，通过 SQL 聚合查询 provider+model 级别的指标数据Testing
新增单元测试
tests/unit/api/leaderboard-route.test.ts- 验证includeModelStats参数传递及响应格式化tests/unit/dashboard/leaderboard-table-expandable-rows.test.tsx- 验证 LeaderboardTable 子行渲染逻辑tests/unit/repository/leaderboard-provider-metrics.test.ts- 验证供应商指标计算及 modelStats 聚合手动验证清单
includeModelStats标识API 文档
GET /api/leaderboard?scope=provider新增参数：
includeModelStats1|true|yes响应示例：
[ { "providerId": 1, "providerName": "test-provider", "totalRequests": 10, "totalCost": 1.5, "totalCostFormatted": "$1.50", "totalTokens": 1000, "successRate": 1, "avgTtfbMs": 100, "avgTokensPerSecond": 20, "avgCostPerRequest": 0.15, "avgCostPerRequestFormatted": "$0.15", "avgCostPerMillionTokens": 1500, "avgCostPerMillionTokensFormatted": "$1,500.00", "modelStats": [ { "model": "model-a", "totalRequests": 6, "totalCost": 1.0, "totalCostFormatted": "$1.00", "totalTokens": 600, "successRate": 1, "avgTtfbMs": 110, "avgTokensPerSecond": 25, "avgCostPerRequest": 0.1667, "avgCostPerRequestFormatted": "$0.17", "avgCostPerMillionTokens": 1666.7, "avgCostPerMillionTokensFormatted": "$1,666.70" } ] } ]Checklist
Description enhanced by Claude AI
Greptile Summary
This PR adds model-level breakdown expansion to provider leaderboards. Users can expand provider rows to view per-model statistics for consumption and cache hit rate rankings.
Key Changes
LeaderboardTableto support sub-row rendering viagetSubRowsandgetSubRowKeypropsincludeModelStatsquery parameter forGET /api/leaderboard?scope=providerincludeModelStatsflag for correct cache differentiationIssues Addressed in Previous Threads
Many potential issues were already identified in previous review threads and should be considered before merge. Notable concerns include performance implications of the unconditional cache hit rate model query, lack of configurability for
includeModelStatsin the UI, and missing error handling for model stats query failures.Confidence Score: 3/5
src/repository/leaderboard.ts(unconditional model query performance) andsrc/app/[locale]/dashboard/leaderboard/_components/leaderboard-view.tsx(hardcoded includeModelStats with no UI control)Important Files Changed
getSubRowsandgetSubRowKeyprops. Sub-rows render inline with gray background. AddedisSubRowparameter to cell renderers for semantic clarity.includeModelStats=1for provider scope with no UI toggle. Column definitions handle both parent and sub-row rendering.includeModelStatsparameter parsing for provider scope. Formats nestedmodelStatsarrays with currency fields. Type assertions assume repository schema without runtime validation.Last reviewed commit: 8bf905a