Feat/chart overlay stacked#224
Conversation
small update
fix(ui): 修复供应商可用性监控页面排序顺序
fix: 故障转移后无条件更新 Session 绑定
Visual presentation modes:
1. Overlay (对比模式)
- 关注点: 个体差异 ("谁更强")
- 逻辑: 独立渲染，基准线均为 0
- 用途: 数值直接对比 (e.g., User A vs User B)
2. Stacked (堆叠模式)
- 关注点: 整体构成 ("整体怎样")
- 逻辑: 累加渲染，后续层基于前一层
- 用途: 总量及占比分析 (e.g., Total Spend composition)
Summary of ChangesHello @sususu98, 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 enhances data visualization capabilities by introducing 'Overlay' and 'Stacked' display modes for charts, allowing users to choose how multi-user statistics are presented. Alongside this new feature, the PR includes comprehensive updates to model pricing data and a significant update to the project's changelog and version number, reflecting a broad set of recent improvements and bug fixes. Highlights
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
|
🔒 Security Scan Results✅ No security vulnerabilities detected This PR has been scanned against OWASP Top 10, CWE Top 25, and common security anti-patterns. No security issues were identified in the code changes. 📋 Changes Analyzed
🔍 Scanned Categories
📝 NotesThe code changes are primarily:
All React component changes use proper JSX escaping and do not introduce any attack surface. The component receives data via props and renders it using standard React patterns. 🤖 Automated security scan by Claude AI - OWASP Top 10 & CWE coverage |
![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 实现了图表的“对比”和“堆叠”模式切换功能，整体实现得非常出色。代码逻辑清晰，并且考虑到了“对比”模式下图形遮挡的视觉优化问题。
主要亮点：
- 动态切换: 通过
chartModestate 和对Area组件stackId的动态赋值，实现了两种模式的无缝切换。 - 视觉优化: 在“对比”模式下，通过对数据系列进行降序排序渲染，确保了数值较大的区域不会遮挡数值较小的区域，提升了图表的可读性。
- 坐标轴优化: 为 Y 轴添加了
domain={[0, "dataMax"]}，保证了图表基线从 0 开始，使得数据对比更加直观和准确。
代码质量很高，我只在 chart.tsx 中发现一个可以优化的点，以提高性能和代码结构。详细信息请见具体评论。
此外，这个 PR 还包含了大量的 CHANGELOG.md 和 litellm-prices.json 的更新，以及版本号的提升，看起来像一个集合了多个功能的发布版本。
| {(chartMode === "overlay" | ||
| ? [...visibleUsers].sort((a, b) => { | ||
| const totalA = userTotals[a.dataKey]; | ||
| const totalB = userTotals[b.dataKey]; | ||
| if (!totalA || !totalB) return 0; | ||
| if (activeChart === "cost") { | ||
| return totalB.cost.comparedTo(totalA.cost); | ||
| } | ||
| return totalB.calls - totalA.calls; | ||
| }) | ||
| : visibleUsers | ||
| ).map((user) => { |
There was a problem hiding this comment.
为了提升性能和代码可读性，建议将此排序逻辑提取到 React.useMemo 中。当前实现在每次组件渲染时都会重新创建数组并进行排序，当用户数量较多时可能会产生不必要的性能开销。
通过 useMemo 缓存排序结果，可以确保只有在相关依赖（如 chartMode, visibleUsers, userTotals, activeChart）变化时才重新计算。
示例:
const sortedVisibleUsers = React.useMemo(() => {
if (chartMode !== "overlay") {
return visibleUsers;
}
// Overlay 模式下按数值降序渲染，大的在底层，小的在顶层避免遮挡
return [...visibleUsers].sort((a, b) => {
const totalA = userTotals[a.dataKey];
const totalB = userTotals[b.dataKey];
if (!totalA || !totalB) return 0;
if (activeChart === "cost") {
return totalB.cost.comparedTo(totalA.cost);
}
return totalB.calls - totalA.calls;
});
}, [chartMode, visibleUsers, userTotals, activeChart]);
// ... 在 JSX 中使用 ...
{sortedVisibleUsers.map((user) => {
// ...
})}
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📋 Code Review Summary
This PR adds a chart display mode toggle (Overlay vs Stacked) to the statistics dashboard, allowing users to switch between comparing individual values and viewing cumulative totals. The implementation is clean, well-structured, and includes complete i18n support for all 5 locales.
🔍 Issues Found
- Critical (🔴): 0 issues
- High (🟠): 0 issues
- Medium (🟡): 0 issues
- Low (🟢): 0 issues
🎯 Priority Actions
No issues requiring action were identified.
💡 General Observations
- The chart mode state management using React
useStateis appropriate for this use case - The sorting logic for overlay mode (rendering larger values first/behind) is a good UX decision to prevent visual occlusion
- The
stackIdconditional (chartMode === "stacked" ? "a" : undefined) correctly toggles between stacked and overlay rendering - The Y-axis domain constraint
[0, "dataMax"]ensures consistent axis scaling across both modes - All locale files (en, ja, ru, zh-CN, zh-TW) have been updated with the new
chartModetranslations
The additional CHANGELOG, VERSION, and litellm-prices.json changes appear to be bundled release updates and are outside the core feature scope.
🤖 Automated review by Claude AI - focused on identifying issues for improvement
There was a problem hiding this comment.
🟠 Missing Feature Documentation: PR feature not documented in CHANGELOG
Problem: This PR adds a new chart display mode feature (Overlay vs Stacked modes) for the statistics dashboard, but this feature is NOT documented in the CHANGELOG. The CHANGELOG updates only include changes from other merged PRs (#199, #207, #210, #216, #219, #220) but omit the actual feature being added in PR #224.
Impact: Users will not know about this new feature when reviewing release notes. The CHANGELOG should accurately reflect what this specific PR adds.
Suggested Fix:
Add an entry under ### Added for this PR's feature:
### Added
- Add chart display mode toggle (Overlay/Stacked) for statistics dashboard with optimized rendering order (#224) @ding113Or under ### Changed if you prefer:
### Changed
- Add Overlay and Stacked display modes to statistics charts for better multi-user data visualization (#224) @ding113
ding113
left a comment
ding113
left a comment
There was a problem hiding this comment.
📝 Documentation Review
📊 Issues Summary
- Critical (🔴): 0 - No critical issues
- High (🟠): 1 - Should fix before merge
- Medium (🟡): 0 - No medium issues
- Low (🟢): 0 - No low issues
⚡ Priority Fixes
- Missing Feature Documentation: The main feature of this PR (chart Overlay/Stacked display modes) is not documented in CHANGELOG.md. The CHANGELOG updates only consolidate entries from previously merged PRs (#199, #207, #210, #216, #219, #220) but miss the actual feature being added in PR #224.
📋 Review Coverage
- Technical accuracy - 0 issues
- Completeness - 1 issue (missing feature documentation)
- Code examples - N/A (no code examples in docs)
- Links and references - 0 issues (all PR references verified as valid)
- Clarity and organization - 0 issues
💡 General Observations
- All referenced PR numbers (#173, #171, #178, #179, #181, #183, #184, #193, #199, #207, #210, #216, #219, #220) are valid and point to merged PRs
- Translation files (messages/*.json) are complete across all 5 locales (en, ja, ru, zh-CN, zh-TW)
- The pricing data file (litellm-prices.json) additions appear to be valid model configurations
- The VERSION bump from 0.3.9 to 0.3.15 is a significant jump - consider if all intermediate version changes are documented
📝 Recommendation
Add a CHANGELOG entry for the chart display mode feature before merging:
### Added
- Add chart display mode toggle (Overlay/Stacked) for statistics dashboard with optimized multi-user rendering (#224) @ding113🤖 Automated docs review by Claude AI
|
咦，你的 base 分支是从 main 拉出来的吗，这样可能会导致后续合并冲突 可否从 dev 拉一个分支，把你的提交 cherry-pick 过来 |
2 participants
Summary
Add chart display mode toggle (Overlay/Stacked) for multi-user statistics visualization in the dashboard.
Problem
When viewing statistics for multiple users, the chart only supported stacked mode which made it difficult to directly compare individual user values since all data was cumulative.
Solution
Introduce a display mode toggle that allows switching between:
Overlay (对比模式)
Stacked (堆叠模式)
Changes
UserStatisticsChartcomponentstackIdassignment based on selected modeTesting