fix: 修复首页模型排行榜排序问题#1127
Conversation
The model rankings query was sorting by request count, which contradicted the "Cost Leaderboard" framing and diverged from the user and provider scopes that both order by sum(cost_usd). Switch the ORDER BY to sum(cost_usd) DESC with request count as tiebreaker, matching the provider model sub-rows pattern.
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthrough此PR通过将多个页面和组件更新为接收动态 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
bug
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 explicit locale handling for server-side translations, a refined language switcher with session-based refresh persistence, and support for per-request billing in scenarios where token usage is unavailable. Additionally, it refactors Langfuse tracing into a centralized utility and updates the leaderboard sorting logic to prioritize total cost. Review feedback highlighted a logic issue in the session observability sync, where certain conditions prevent the recording of error statuses in Redis when usage or cost data is missing.
I am having trouble creating individual review comments. Click here to see my feedback.
src/app/v1/_lib/proxy/response-handler.ts (1343-1347)
The condition (usageMetrics || costUsdStr !== undefined) prevents the session status from being updated to error in Redis when a request fails without usage or cost. It should be updated to always sync the status if session.sessionId is present. Additionally, ensure this Redis I/O is executed as a fire-and-forget task to avoid blocking the main logic in this performance-sensitive path.
if (session.sessionId && session.shouldTrackSessionObservability()) {References
- In performance-sensitive code paths like provider failover, non-critical I/O operations (e.g., releasing a session in Redis) should be executed as fire-and-forget tasks to avoid blocking the main logic.
src/app/v1/_lib/proxy/response-handler.ts (3678-3682)
Similar to the issue in handleNonStream, the condition perRequestCostUsd !== undefined prevents updating the session status in Redis for requests that fail. The status should be updated regardless of whether a cost was calculated. Ensure this Redis update is performed as a fire-and-forget task to avoid blocking the main logic, as per repository performance guidelines.
if (session.sessionId && session.shouldTrackSessionObservability()) {References
- In performance-sensitive code paths like provider failover, non-critical I/O operations (e.g., releasing a session in Redis) should be executed as fire-and-forget tasks to avoid blocking the main logic.
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94c8f930a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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 changes the model leaderboard ordering to sort by total cost (with request count as a secondary sort) and adds a unit test for the expected order. The SQL ORDER BY currently uses raw sum(costUsd) while totalCost is computed with COALESCE(sum(costUsd), 0), which can produce inconsistent ordering for nullable cost data.
PR Size: S
- Lines changed: 51
- Files changed: 2
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)
src/repository/leaderboard.ts:1156(Confidence: 85)ORDER BYuses rawsum(costUsd)instead of the sameCOALESCE(sum(costUsd), 0)expression used fortotalCost.
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.
- Identified PR
#1127and applied labelsize/S(51 lines changed across 2 files). - Posted 1 inline review comment on
src/repository/leaderboard.ts:1156flagging inconsistent ordering whencostUsdis nullable (ORDER BY sum(costUsd)vs selectedCOALESCE(sum(costUsd), 0)), with a concrete code fix. - Submitted the required summary review comment to the PR via
gh pr review --comment.
There was a problem hiding this comment.
Code Review Summary
This PR correctly fixes the model leaderboard sort order to rank by total cost (descending) with request count as tiebreaker, matching the user leaderboard (L367), provider leaderboard (L699), and provider model sub-rows (L750). The change is minimal (1 line), well-aligned with existing patterns, and includes an appropriate unit test.
PR Size: S
- Lines changed: 51
- Files changed: 2
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Validation Notes
- The SELECT clause uses
COALESCE(sum(costUsd), 0)while the ORDER BY uses baresum(costUsd)-- this is a pre-existing pattern consistent across all leaderboard queries (L367, L407, L699, L750). Not introduced by this PR; not flagged. - Test coverage is adequate: verifies both result ordering (expensive-low-volume ranks above cheap-high-volume) and ORDER BY argument structure (sum + count).
- No error handling, security, or type safety concerns in the diff scope.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate
- Code clarity - Good
Automated review by Claude AI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Issue was fixed in #1128 |
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
Summary
Fix the model leaderboard sort order to rank by total cost (descending) instead of request count, aligning it with the user and provider leaderboards.
Problem
The model leaderboard query (
findModelLeaderboardWithTimezone) was sorting bycount(*)(request count) while all other leaderboard scopes (user at L367/L407, provider at L699) sort bysum(cost_usd). This caused the "Cost Leaderboard" display to show models ordered by volume rather than cost, making expensive low-volume models appear below cheap high-volume ones.Solution
Switch the
ORDER BYclause from:to:
This matches the provider model sub-rows pattern (L750) and is consistent with all other leaderboard scopes.
Changes
src/repository/leaderboard.ts: Change model leaderboardorderBytosum(cost_usd) DESCwithcount(*)as tiebreakertests/unit/repository/leaderboard-provider-metrics.test.ts: Add test verifying cost-based ordering with an expensive-low-volume model ranking above a cheap-high-volume modelTesting
Description enhanced by Claude AI
Greptile Summary
This PR fixes the homepage model leaderboard to sort by total cost (descending) with request count as a tiebreaker, replacing the previous sort by request count alone. It bundles several related improvements: explicit
localepropagation togetTranslations()across all RSC pages (fixing potential locale-mismatch in server components), aLanguageSwitcherrefresh fix that triggersrouter.refresh()after locale navigation, per-request billing support for image APIs without token usage, and extraction of the Langfuse trace helper into a shared module with error-path coverage.Confidence Score: 4/5
Safe to merge; all changes are well-tested and the single finding is a minor ORDER BY NULL-handling inconsistency.
Only P2 findings present. The core leaderboard sort fix is correct and covered by a new unit test. The i18n, language-switcher, billing, and Langfuse changes all have matching tests. One minor inconsistency exists between COALESCE in SELECT vs bare sum() in ORDER BY, but practical impact is negligible.
src/repository/leaderboard.ts — minor COALESCE inconsistency between SELECT and ORDER BY for null costUsd rows.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Model Leaderboard Query] --> B[GROUP BY modelField] B --> C{ORDER BY} C -->|Before PR| D["count(*) DESC\n(request count)"] C -->|After PR| E["sum(costUsd) DESC\nthen count(*) DESC"] E --> F[Return sorted rankings] D --> F F --> G[Filter null/empty models] G --> H[Map to ModelLeaderboardEntry]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(dashboard): order model leaderboard ..." | Re-trigger Greptile