Conversation
Summary of ChangesHello @tesgth032, 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! 此 PR 旨在解决首页统计图表在视口高度较小时内容被裁切的 UI 问题，尤其是在启用多用户图例时。通过为图表卡片引入纵向滚动和动态高度调整机制，确保了所有图表内容（包括图例和控制按钮）在不同屏幕尺寸下都能完整显示。此外，为了便于开发和验证这些 UI 改进，新增了一个专门的内部预览页面，并相应地调整了开发环境下的访问权限。 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
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ? "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)-248px),240px)] w-full" | ||
| : "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)-138px),280px)] w-full"; |
There was a problem hiding this comment.
calc() operator requires spaces in Tailwind arbitrary values - use underscores
| ? "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)-248px),240px)] w-full" | |
| : "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)-138px),280px)] w-full"; | |
| ? "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)_-_248px),240px)] w-full" | |
| : "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)_-_138px),280px)] w-full"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 171-172
Comment:
calc() operator requires spaces in Tailwind arbitrary values - use underscores
```suggestion
? "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)_-_248px),240px)] w-full"
: "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)_-_138px),280px)] w-full";
```
How can I resolve this? If you propose a fix, please make it concise.| "p-0 md:p-0 overflow-x-hidden overflow-y-auto max-h-[var(--cch-viewport-height-50)]", | ||
| "scrollbar-thin scrollbar-thumb-muted scrollbar-track-transparent", | ||
| className |
There was a problem hiding this comment.
removed flex flex-col - verify header/tabs/chart/legend still stack correctly
Also, md:p-0 is redundant when p-0 is already set
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 177-179
Comment:
removed `flex flex-col` - verify header/tabs/chart/legend still stack correctly
Also, `md:p-0` is redundant when `p-0` is already set
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}
| const chartContainerClassName = enableUserFilter | ||
| ? "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)-248px),240px)] w-full" | ||
| : "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)-138px),280px)] w-full"; |
There was a problem hiding this comment.
这里的 248px 和 138px 是“魔术数字”（Magic Numbers）。它们的含义不明确，导致代码难以理解和维护。如果卡片内其他元素的高度发生变化，就需要手动重新计算和更新这些值，这很容易出错。
建议添加注释来解释这些值的来源，以提高代码的可读性和可维护性。
const chartContainerClassName = enableUserFilter
// 248px is the approximate height of non-chart elements when the legend is visible.
? "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)-248px),240px)] w-full"
// 138px is the approximate height of non-chart elements when the legend is hidden.
: "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)-138px),280px)] w-full";
|
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新增统计图表预览组件与预览页，修改仪表盘统计图表卡片的高度与滚动布局；在开发模式下将 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 |
|
已根据 review 修复 对应提交：f639b139 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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]/internal/ui-preview/statistics-chart/page.tsx:
- Line 2: 当前在 page.tsx 中通过相对路径导入 StatisticsChartPreview（import {
StatisticsChartPreview } from
"./_components/statistics-chart-preview";），请把相对路径改为仓库约定的别名导入，使用 '@/...'
别名来引用该组件（即将开头的 "./" 换为 "@/..."），确保导入符号仍为 StatisticsChartPreview 并保存原有导出路径末段
"_components/statistics-chart-preview"。
In `@src/proxy.ts`:
- Around line 55-59: The current isDevPublicPath computation uses
DEV_PUBLIC_PATH_PATTERNS.some(... pattern => pathWithoutLocale === pattern ||
pathWithoutLocale.startsWith(pattern)) which is too permissive; change the
second check to require a true subpath boundary (e.g., pathWithoutLocale ===
pattern || pathWithoutLocale.startsWith(pattern + '/') or ensure the next
character after the match is '/'), so only exact matches or proper child paths
match; update the logic used by isDevPublicPath (referencing
DEV_PUBLIC_PATH_PATTERNS and pathWithoutLocale) accordingly.
ℹ️ 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/_components/bento/statistics-chart-card.tsxsrc/app/[locale]/internal/ui-preview/statistics-chart/_components/statistics-chart-preview.tsxsrc/app/[locale]/internal/ui-preview/statistics-chart/page.tsxsrc/proxy.ts
| const chartContainerClassName = enableUserFilter | ||
| ? "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)_-_248px),240px)] w-full" | ||
| : "aspect-auto h-[clamp(140px,calc(var(--cch-viewport-height-50)_-_138px),280px)] w-full"; |
There was a problem hiding this comment.
chart height uses calc(50vh - offset) but card has max-h-50vh not fixed h-50vh. when content is smaller than 50vh, chart might be unnecessarily small. consider flex layout with min-h on chart instead for more predictable sizing
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 170-172
Comment:
chart height uses `calc(50vh - offset)` but card has `max-h-50vh` not fixed `h-50vh`. when content is smaller than 50vh, chart might be unnecessarily small. consider flex layout with `min-h` on chart instead for more predictable sizing
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
|
补充：已按 CodeRabbit 建议收紧开发环境放行规则，仅匹配 对应提交：c586906c |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/proxy/proxy-dev-public-path.test.ts (1)
37-62: 建议添加beforeEach清理 mock 状态以确保测试隔离。
mockIntlMiddleware在测试间共享状态，可能导致测试顺序依赖或状态泄漏。同时建议使用vi.resetModules()确保每个测试获得干净的模块实例。♻️ 建议添加测试钩子
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { describe, expect, it, vi } from "vitest"; // ... mocks ... describe("proxy dev public paths", () => { + beforeEach(() => { + vi.resetModules(); + mockIntlMiddleware.mockReset(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + it("allows /internal/ui-preview/* without any cookie in development", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/proxy/proxy-dev-public-path.test.ts` around lines 37 - 62, Add a beforeEach that isolates tests by calling vi.resetModules() and clearing/resetting the shared mockIntlMiddleware (e.g., mockIntlMiddleware.mockReset() or mockIntlMiddleware.mockClear()) before each it; this ensures that when importing proxyHandler via await import("@/proxy") and invoking makeRequest(...) each test gets a fresh module instance and no leaked mock state.src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx (1)
170-178: 魔法数字（248px / 138px）的可维护性注释解释了这些值的来源，但它们与 Header、Tabs、Padding、Legend 等的实际高度耦合较紧。如果后续调整了这些区域的间距或字体大小，这里的估算值可能会失效。
建议：考虑将这些偏移量提取为命名常量或 CSS 变量，并在注释中列出各部分的具体高度构成，便于后续维护时快速定位和调整。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/bento/statistics-chart-card.tsx around lines 170 - 178, The hard-coded offsets 248px and 138px in chartContainerClassName are magic numbers; extract them into named constants or CSS variables (e.g., CHART_OFFSET_WITH_LEGEND, CHART_OFFSET_NO_LEGEND or --chart-offset-with-legend, --chart-offset-no-legend) and replace the inline literals in the conditional class strings; also update the surrounding comment to enumerate the constituent heights (Header, Tabs, Padding, Legend) and reference the new constants/variables so future UI changes can be adjusted in one place (use enableUserFilter and chartContainerClassName to locate the code).
🤖 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/_components/bento/statistics-chart-card.tsx:
- Around line 183-185: The scrollbar utility classes used in
statistics-chart-card.tsx ("scrollbar-thin", "scrollbar-thumb-muted",
"scrollbar-track-transparent") are not defined; fix by either installing and
enabling the tailwind-scrollbar plugin (npm/yarn add tailwind-scrollbar and add
it to plugins in tailwind.config.js) so these classes are generated, or add
custom utility definitions in your global stylesheet (src/app/globals.css) using
an `@layer` utilities block that defines .scrollbar-thin, .scrollbar-thumb-muted,
and .scrollbar-track-transparent so the component’s className in
statistics-chart-card.tsx works as intended.
In `@tests/unit/proxy/proxy-dev-public-path.test.ts`:
- Around line 55-56: The test calls proxyHandler(...) without awaiting its
(likely) async result; update the test to await the proxy handler invocation so
the call to proxyHandler returned promise is resolved before assertions.
Specifically, in tests/unit/proxy/proxy-dev-public-path.test.ts change the line
that calls proxyHandler(makeRequest("/zh-CN/internal/ui-preview-xxx")) to await
the async call to proxyHandler (the symbol proxyHandler) so the response is
properly resolved before using it in assertions.
---
Nitpick comments:
In `@src/app/`[locale]/dashboard/_components/bento/statistics-chart-card.tsx:
- Around line 170-178: The hard-coded offsets 248px and 138px in
chartContainerClassName are magic numbers; extract them into named constants or
CSS variables (e.g., CHART_OFFSET_WITH_LEGEND, CHART_OFFSET_NO_LEGEND or
--chart-offset-with-legend, --chart-offset-no-legend) and replace the inline
literals in the conditional class strings; also update the surrounding comment
to enumerate the constituent heights (Header, Tabs, Padding, Legend) and
reference the new constants/variables so future UI changes can be adjusted in
one place (use enableUserFilter and chartContainerClassName to locate the code).
In `@tests/unit/proxy/proxy-dev-public-path.test.ts`:
- Around line 37-62: Add a beforeEach that isolates tests by calling
vi.resetModules() and clearing/resetting the shared mockIntlMiddleware (e.g.,
mockIntlMiddleware.mockReset() or mockIntlMiddleware.mockClear()) before each
it; this ensures that when importing proxyHandler via await import("@/proxy")
and invoking makeRequest(...) each test gets a fresh module instance and no
leaked mock state.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (2)
src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsxtests/unit/proxy/proxy-dev-public-path.test.ts
src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/_components/bento/statistics-chart-card.tsx:
- Around line 176-178: 将第 170-175 行过长的注释拆成多行以满足 100 字符行宽限制：在同一文件的
chartContainerClassName 定义附近（变量名 chartContainerClassName）将原来几行超过 100
字符的注释分割为多行注释，每行不超过 100 字符，保留原注释的全部信息（关于 max-h 50vh、启用多用户 Legend
导致非图表部分高度增大、使用“可用高度估算 + clamp”方式自适应高度，以及 overflow-y-auto
对裁切影响的说明），确保注释语义不变且对齐/缩进与周围注释风格一致。
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (2)
src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsxtests/unit/proxy/proxy-dev-public-path.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/proxy/proxy-dev-public-path.test.ts
src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Outdated
Show resolved
Hide resolved
| describe("proxy dev public paths", () => { | ||
| beforeEach(() => { | ||
| vi.resetModules(); | ||
| mockIntlMiddleware.mockReset(); | ||
| }); | ||
|
|
||
| it("allows /internal/ui-preview/* without any cookie in development", async () => { | ||
| const localeResponse = new Response(null, { | ||
| status: 200, | ||
| headers: { "x-test": "dev-public-ok" }, | ||
| }); | ||
| mockIntlMiddleware.mockReturnValue(localeResponse); | ||
|
|
||
| const { default: proxyHandler } = await import("@/proxy"); | ||
| const response = await proxyHandler(makeRequest("/zh-CN/internal/ui-preview/statistics-chart")); | ||
|
|
||
| expect(response.headers.get("x-test")).toBe("dev-public-ok"); | ||
| }); | ||
|
|
||
| it("does not overmatch /internal/ui-preview-xxx", async () => { | ||
| const localeResponse = new Response(null, { status: 200 }); | ||
| mockIntlMiddleware.mockReturnValue(localeResponse); | ||
|
|
||
| const { default: proxyHandler } = await import("@/proxy"); | ||
| const response = await proxyHandler(makeRequest("/zh-CN/internal/ui-preview-xxx")); | ||
|
|
||
| expect(response.status).toBeGreaterThanOrEqual(300); | ||
| expect(response.status).toBeLessThan(400); | ||
| const location = response.headers.get("location"); | ||
| expect(location).toContain("/login"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing production-mode security test
The entire test file mocks isDevelopment: () => true, so it only verifies the development happy-path. There is no test case that confirms the security guarantee stated in the PR description — that /internal/ui-preview is blocked (redirects to login) when isDevelopment() returns false.
The existing proxy-auth-cookie-passthrough.test.ts covers isDevelopment: () => false for /dashboard and /login, but never exercises the new /internal/ui-preview path. If the isDevelopment() guard were accidentally removed from the proxy, no test in either file would catch the regression.
Consider adding a third case to this describe block:
it("blocks /internal/ui-preview/* in production (isDevelopment = false)", async () => {
// Re-mock isDevelopment to return false for this test
vi.doMock("@/lib/config/env.schema", () => ({
isDevelopment: () => false,
}));
vi.resetModules();
const localeResponse = new Response(null, { status: 200 });
mockIntlMiddleware.mockReturnValue(localeResponse);
const { default: proxyHandler } = await import("@/proxy");
const response = await proxyHandler(
makeRequest("/zh-CN/internal/ui-preview/statistics-chart")
);
expect(response.status).toBeGreaterThanOrEqual(300);
expect(response.status).toBeLessThan(400);
expect(response.headers.get("location")).toContain("/login");
});Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/proxy/proxy-dev-public-path.test.ts
Line: 37-68
Comment:
**Missing production-mode security test**
The entire test file mocks `isDevelopment: () => true`, so it only verifies the development happy-path. There is no test case that confirms the security guarantee stated in the PR description — that `/internal/ui-preview` is **blocked** (redirects to login) when `isDevelopment()` returns `false`.
The existing `proxy-auth-cookie-passthrough.test.ts` covers `isDevelopment: () => false` for `/dashboard` and `/login`, but never exercises the new `/internal/ui-preview` path. If the `isDevelopment()` guard were accidentally removed from the proxy, no test in either file would catch the regression.
Consider adding a third case to this `describe` block:
```ts
it("blocks /internal/ui-preview/* in production (isDevelopment = false)", async () => {
// Re-mock isDevelopment to return false for this test
vi.doMock("@/lib/config/env.schema", () => ({
isDevelopment: () => false,
}));
vi.resetModules();
const localeResponse = new Response(null, { status: 200 });
mockIntlMiddleware.mockReturnValue(localeResponse);
const { default: proxyHandler } = await import("@/proxy");
const response = await proxyHandler(
makeRequest("/zh-CN/internal/ui-preview/statistics-chart")
);
expect(response.status).toBeGreaterThanOrEqual(300);
expect(response.status).toBeLessThan(400);
expect(response.headers.get("location")).toContain("/login");
});
```
How can I resolve this? If you propose a fix, please make it concise.| if (process.env.NODE_ENV === "production") { | ||
| notFound(); | ||
| } |
There was a problem hiding this comment.
Inconsistent env guard vs. proxy
The proxy uses isDevelopment() (i.e. NODE_ENV === "development") to decide whether to bypass authentication. This page guard uses process.env.NODE_ENV === "production" to call notFound(). The two conditions are not logical inverses:
NODE_ENV |
Proxy allows access? | Page calls notFound()? |
|---|---|---|
"development" |
✅ yes | ❌ no → renders ✓ |
"production" |
❌ no | ✅ yes → 404 ✓ |
"test" |
❌ no | ❌ no → renders |
In a NODE_ENV=test environment the page itself would serve the preview content rather than returning 404, while the proxy simultaneously blocks all access. Defense-in-depth works best when both layers agree. Using !== "development" (mirroring the proxy) closes the gap:
| if (process.env.NODE_ENV === "production") { | |
| notFound(); | |
| } | |
| if (process.env.NODE_ENV !== "development") { | |
| notFound(); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/internal/ui-preview/statistics-chart/page.tsx
Line: 7-9
Comment:
**Inconsistent env guard vs. proxy**
The proxy uses `isDevelopment()` (i.e. `NODE_ENV === "development"`) to decide whether to bypass authentication. This page guard uses `process.env.NODE_ENV === "production"` to call `notFound()`. The two conditions are **not logical inverses**:
| `NODE_ENV` | Proxy allows access? | Page calls `notFound()`? |
|---|---|---|
| `"development"` | ✅ yes | ❌ no → renders ✓ |
| `"production"` | ❌ no | ✅ yes → 404 ✓ |
| `"test"` | ❌ no | ❌ no → **renders** ⚠️ |
In a `NODE_ENV=test` environment the page itself would serve the preview content rather than returning 404, while the proxy simultaneously blocks all access. Defense-in-depth works best when both layers agree. Using `!== "development"` (mirroring the proxy) closes the gap:
```suggestion
if (process.env.NODE_ENV !== "development") {
notFound();
}
```
How can I resolve this? If you propose a fix, please make it concise.| </div> | ||
| {/* User list with max 3 rows and scroll - only show users with non-zero usage */} | ||
| <div className="max-h-[72px] overflow-y-auto scrollbar-thin scrollbar-thumb-muted scrollbar-track-transparent"> | ||
| <div className="max-h-[72px] overflow-y-auto"> |
There was a problem hiding this comment.
Scrollbar styling removed from legend list
scrollbar-thin scrollbar-thumb-muted scrollbar-track-transparent were removed from this inner scrollable container. scrollbar-thin is still used elsewhere in the codebase (e.g. request-list-sidebar.tsx), so removing it here means the legend list will now show the default browser-native scrollbar, which can appear visually heavier than the rest of the UI on Windows/Linux.
Since the outer BentoCard now also scrolls without any scrollbar styling, both scroll surfaces render as native scrollbars. If thin scrollbars are desired on the legend list (consistent with the rest of the app), the original classes should be retained:
| <div className="max-h-[72px] overflow-y-auto"> | |
| <div className="max-h-[72px] overflow-y-auto scrollbar-thin scrollbar-thumb-muted scrollbar-track-transparent"> |
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 453
Comment:
**Scrollbar styling removed from legend list**
`scrollbar-thin scrollbar-thumb-muted scrollbar-track-transparent` were removed from this inner scrollable container. `scrollbar-thin` is still used elsewhere in the codebase (e.g. `request-list-sidebar.tsx`), so removing it here means the legend list will now show the default browser-native scrollbar, which can appear visually heavier than the rest of the UI on Windows/Linux.
Since the outer `BentoCard` now also scrolls without any scrollbar styling, both scroll surfaces render as native scrollbars. If thin scrollbars are desired on the legend list (consistent with the rest of the app), the original classes should be retained:
```suggestion
<div className="max-h-[72px] overflow-y-auto scrollbar-thin scrollbar-thumb-muted scrollbar-track-transparent">
```
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
|
已按 AI 审阅意见做收尾：
当前 CI 已全绿。 @coderabbitai 如方便请重新 review/更新状态（此前标记的 changes requested 项已处理）。 |
|
✅ 将高度计算参数提取为常量并通过 我会立即重新审查本 PR 并更新状态。 ✅ Actions performedReview triggered.
|
- 统计卡片在 50vh 外框下改为可滚动，图表高度按可用空间自适应，避免 Legend 被裁切\n- 开发环境新增 /internal/ui-preview/statistics-chart 预览页（无需 DB）\n- 开发环境放行 /internal/ui-preview 免登录访问，方便本地 UI 检查
- 将图表高度与非图表区域偏移提取为常量，改用 style 计算 clamp() 高度 - 单测增加 beforeEach 重置模块与 mock，避免状态泄漏
tesgth032
force-pushed
the
fix/statistics-chart-clipping
branch
from
85bc440 to
51d0b83
Compare
| // - DOM 计算高度可能带小数（浏览器缩放 / 子像素） | ||
| // - 卡片自身 border 也会占用 max-h 的可用空间 | ||
| const reservedHeightRounded = Math.ceil(reservedHeight); | ||
| const safetyGapPx = 4; |
There was a problem hiding this comment.
safetyGapPx = 4 seems excessive. Comment says avoiding "1px 级的裁切" (1-2px clipping), but adds 4px buffer. Consider 2px or document why 4px is necessary
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 125
Comment:
`safetyGapPx = 4` seems excessive. Comment says avoiding "1px 级的裁切" (1-2px clipping), but adds 4px buffer. Consider 2px or document why 4px is necessary
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| </div> | ||
| {/* User list with max 3 rows and scroll - only show users with non-zero usage */} | ||
| <div className="max-h-[72px] overflow-y-auto scrollbar-thin scrollbar-thumb-muted scrollbar-track-transparent"> | ||
| <div className="max-h-[72px] overflow-y-auto"> |
There was a problem hiding this comment.
max-h-[72px] magic number. Comment says "max 3 rows" - document calculation (e.g., 3 rows × ~24px per row)
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 539
Comment:
`max-h-[72px]` magic number. Comment says "max 3 rows" - document calculation (e.g., 3 rows × ~24px per row)
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| {t("totalCost")} | ||
| </span> | ||
| <span className="text-lg font-bold tabular-nums"> | ||
| <span className="text-[17px] leading-tight font-bold tabular-nums"> |
There was a problem hiding this comment.
text-[17px] breaks design system. Use theme values (text-base: 16px, text-lg: 18px) for consistency
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 345
Comment:
`text-[17px]` breaks design system. Use theme values (`text-base`: 16px, `text-lg`: 18px) for consistency
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| {t("totalCalls")} | ||
| </span> | ||
| <span className="text-lg font-bold tabular-nums"> | ||
| <span className="text-[17px] leading-tight font-bold tabular-nums"> |
There was a problem hiding this comment.
text-[17px] breaks design system. Use theme values for consistency
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 361
Comment:
`text-[17px]` breaks design system. Use theme values for consistency
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.| const parsed = Number.parseFloat(trimmed.slice(0, -2)); | ||
| return Number.isFinite(parsed) ? parsed : null; |
There was a problem hiding this comment.
parsePx doesn't validate non-negative. Add check: return parsed >= 0 ? parsed : null
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 41-42
Comment:
`parsePx` doesn't validate non-negative. Add check: `return parsed >= 0 ? parsed : null`
How can I resolve this? If you propose a fix, please make it concise.
| <div className="flex items-center justify-center gap-2 mb-2"> | ||
| <div ref={legendRef} className="relative px-4 pb-2"> | ||
| {/* Control buttons (floating, does not take extra vertical space) */} | ||
| <div className="absolute right-4 top-0.5 z-10 w-20 flex flex-wrap justify-end gap-x-2 gap-y-0.5"> |
There was a problem hiding this comment.
fixed w-20 (80px) width may be insufficient for i18n button text
The container is w-20 with flex-wrap, and the user list has pr-20 to compensate. If translated button text is longer (e.g., German "Alle auswählen" ≈ 13 chars vs English "Select All" ≈ 10 chars), buttons could wrap to a second line and overlap with user badges below since there's no top padding on the user list
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 507
Comment:
fixed `w-20` (80px) width may be insufficient for i18n button text
The container is `w-20` with `flex-wrap`, and the user list has `pr-20` to compensate. If translated button text is longer (e.g., German "Alle auswählen" ≈ 13 chars vs English "Select All" ≈ 10 chars), buttons could wrap to a second line and overlap with user badges below since there's no top padding on the user list
How can I resolve this? If you propose a fix, please make it concise.| const cardStyle = getComputedStyle(card); | ||
| const maxHeightPx = | ||
| parsePx(cardStyle.maxHeight) ?? | ||
| Math.floor((window.visualViewport?.height ?? window.innerHeight) * 0.5); |
There was a problem hiding this comment.
window.visualViewport fallback behavior differs from CSS vh units
visualViewport.height accounts for virtual keyboards on mobile but on zoomed pages, innerHeight returns zoomed pixels while vh units use layout viewport, potentially causing 1-2px discrepancy. Consider using document.documentElement.clientHeight as fallback for consistency with CSS viewport units
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 100
Comment:
`window.visualViewport` fallback behavior differs from CSS `vh` units
`visualViewport.height` accounts for virtual keyboards on mobile but on zoomed pages, `innerHeight` returns zoomed pixels while `vh` units use layout viewport, potentially causing 1-2px discrepancy. Consider using `document.documentElement.clientHeight` as fallback for consistency with CSS viewport units
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Add Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 565
Comment:
`max-w-[80px]` truncates usernames without tooltip
Add `title={user.name}` to the button or span so users can see full names on hover
How can I resolve this? If you propose a fix, please make it concise. |
| wrapperStyle={{ | ||
| transform: "translateY(-100%)", | ||
| marginTop: "-20px", | ||
| zIndex: 1000, | ||
| }} |
There was a problem hiding this comment.
hardcoded tooltip positioning can cause overflow on small chart heights
transform: "translateY(-100%)" moves tooltip up by its full height, plus 20px margin. when chart is ~50-100px tall (small viewport scenario this PR addresses), tooltip near top of chart would render outside card/viewport boundaries, getting clipped or hidden
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 420-424
Comment:
hardcoded tooltip positioning can cause overflow on small chart heights
`transform: "translateY(-100%)"` moves tooltip up by its full height, plus 20px margin. when chart is ~50-100px tall (small viewport scenario this PR addresses), tooltip near top of chart would render outside card/viewport boundaries, getting clipped or hidden
How can I resolve this? If you propose a fix, please make it concise.
| onClick={() => { | ||
| if (data.users.length > 0) { | ||
| setSelectedUserIds(new Set([data.users[0].id])); | ||
| const firstUserWithUsageId = | ||
| data.users.find((u) => { | ||
| const total = userTotals[u.dataKey]; | ||
| return total.cost.greaterThan(0) || total.calls > 0; | ||
| })?.id ?? data.users[0]?.id; | ||
|
|
||
| if (firstUserWithUsageId != null) { | ||
| setSelectedUserIds(new Set([firstUserWithUsageId])); | ||
| } | ||
| }} |
There was a problem hiding this comment.
"Deselect All" button doesn't actually deselect all users—it selects the first user with usage data. Users expecting typical "deselect all" behavior (clearing the selection entirely) may be confused. Consider a more descriptive label like "Select One" or "Reset Selection".
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 564-574
Comment:
"Deselect All" button doesn't actually deselect all users—it selects the first user with usage data. Users expecting typical "deselect all" behavior (clearing the selection entirely) may be confused. Consider a more descriptive label like "Select One" or "Reset Selection".
How can I resolve this? If you propose a fix, please make it concise.
| <div className="text-xs font-medium text-center mb-2 pb-2 border-b border-border/50"> | ||
| <div | ||
| className="rounded-lg border bg-background shadow-sm min-w-[180px] overflow-y-auto" | ||
| style={{ maxHeight: "min(var(--cch-viewport-height-80), 720px)" }} |
There was a problem hiding this comment.
PR description says tooltip max-height is min(80dvh,560px) but code has 720px
The "更新" section states: "并将 tooltip 列表尽量展开（max-height: min(80dvh,560px)）" but implementation uses 720px. Update PR description or code to match.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 472
Comment:
PR description says tooltip max-height is `min(80dvh,560px)` but code has `720px`
The "更新" section states: "并将 tooltip 列表尽量展开（`max-height: min(80dvh,560px)`）" but implementation uses 720px. Update PR description or code to match.
How can I resolve this? If you propose a fix, please make it concise.
| ref={cardRef} | ||
| className={cn( | ||
| "flex flex-col p-0 overflow-hidden max-h-[var(--cch-viewport-height-50)]", | ||
| "p-0 md:p-1 min-h-[300px] max-h-[min(var(--cch-viewport-height-60),720px)] overflow-visible hover:z-20", |
There was a problem hiding this comment.
overflow-visible allows card content to overflow beyond max-h boundaries, contradicting PR goal "不让卡片本身滚动" (prevent card scrolling). If calculated height constraints fail, card internals (header/legend) will render outside the card.
Use overflow-hidden to enforce containment. Tooltip can still overflow using higher z-index + wrapperStyle positioning (already set on line 459).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 314
Comment:
`overflow-visible` allows card content to overflow beyond `max-h` boundaries, contradicting PR goal "不让卡片本身滚动" (prevent card scrolling). If calculated height constraints fail, card internals (header/legend) will render outside the card.
Use `overflow-hidden` to enforce containment. Tooltip can still overflow using higher `z-index` + `wrapperStyle` positioning (already set on line 459).
How can I resolve this? If you propose a fix, please make it concise.| const nextHeight = Math.max(0, Math.min(maxChartHeight, availableHeight)); | ||
| setChartHeightPx((prev) => (prev === nextHeight ? prev : nextHeight)); |
There was a problem hiding this comment.
chart can become 0px height at extreme viewport sizes (e.g., 300px viewport with min-h, leaving ~50px for chart after subtracting header/tabs/legend/padding). Math.max(0, ...) allows this. user sees empty space with no feedback.
add minimum threshold:
| const nextHeight = Math.max(0, Math.min(maxChartHeight, availableHeight)); | |
| setChartHeightPx((prev) => (prev === nextHeight ? prev : nextHeight)); | |
| const nextHeight = Math.max(80, Math.min(maxChartHeight, availableHeight)); | |
| setChartHeightPx((prev) => (prev === nextHeight ? prev : nextHeight)); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 138-139
Comment:
chart can become 0px height at extreme viewport sizes (e.g., 300px viewport with min-h, leaving ~50px for chart after subtracting header/tabs/legend/padding). `Math.max(0, ...)` allows this. user sees empty space with no feedback.
add minimum threshold:
```suggestion
const nextHeight = Math.max(80, Math.min(maxChartHeight, availableHeight));
setChartHeightPx((prev) => (prev === nextHeight ? prev : nextHeight));
```
How can I resolve this? If you propose a fix, please make it concise.| <div className="flex items-center justify-center gap-2 mb-2"> | ||
| <div ref={legendRef} className="relative px-4 pb-2"> | ||
| {/* Control buttons (floating, does not take extra vertical space) */} | ||
| <div className="absolute right-4 top-0.5 z-10 w-24 flex flex-wrap justify-end gap-x-2 gap-y-0.5"> |
There was a problem hiding this comment.
buttons container is w-24 (96px) with flex-wrap. if i18n button text wraps to second line (e.g., long translations), buttons will expand vertically and overlap user badges below (no gap between button container and line 590).
user list has pr-24 to reserve space, but only accounts for single-line buttons. when buttons wrap, the reserved space becomes insufficient.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 553
Comment:
buttons container is `w-24` (96px) with `flex-wrap`. if i18n button text wraps to second line (e.g., long translations), buttons will expand vertically and overlap user badges below (no gap between button container and line 590).
user list has `pr-24` to reserve space, but only accounts for single-line buttons. when buttons wrap, the reserved space becomes insufficient.
How can I resolve this? If you propose a fix, please make it concise.| const compute = () => { | ||
| const cardStyle = getComputedStyle(card); | ||
| const viewportHeightPx = window.visualViewport?.height ?? window.innerHeight; | ||
| const fallbackMaxHeightPx = Math.floor(Math.min(viewportHeightPx * 0.6, 720)); |
There was a problem hiding this comment.
fallback multiplies viewport height by 0.6, but CSS uses 60dvh (dynamic viewport height) which excludes browser UI. on mobile with virtual keyboard visible, window.innerHeight includes keyboard area while dvh doesn't, causing 50-100px+ mismatch between calculated fallback and actual CSS max-height.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 100
Comment:
fallback multiplies viewport height by 0.6, but CSS uses `60dvh` (dynamic viewport height) which excludes browser UI. on mobile with virtual keyboard visible, `window.innerHeight` includes keyboard area while `dvh` doesn't, causing 50-100px+ mismatch between calculated fallback and actual CSS max-height.
How can I resolve this? If you propose a fix, please make it concise.| React.useEffect(() => { | ||
| const chartWrapper = chartWrapperRef.current; | ||
| if (!chartWrapper) return; | ||
|
|
||
| const handleWheel = (event: WheelEvent) => { | ||
| const scrollContainer = tooltipScrollRef.current; | ||
| if (!scrollContainer) return; | ||
|
|
||
| const maxScrollTop = scrollContainer.scrollHeight - scrollContainer.clientHeight; | ||
| if (maxScrollTop <= 0) return; | ||
|
|
||
| const deltaY = | ||
| event.deltaMode === 1 | ||
| ? event.deltaY * 16 | ||
| : event.deltaMode === 2 | ||
| ? event.deltaY * 240 | ||
| : event.deltaY; | ||
| if (!Number.isFinite(deltaY) || deltaY === 0) return; | ||
|
|
||
| const current = scrollContainer.scrollTop; | ||
| const next = Math.min(maxScrollTop, Math.max(0, current + deltaY)); | ||
| if (next === current) return; | ||
|
|
||
| scrollContainer.scrollTop = next; | ||
| event.preventDefault(); | ||
| }; | ||
|
|
||
| chartWrapper.addEventListener("wheel", handleWheel, { passive: false }); | ||
| return () => chartWrapper.removeEventListener("wheel", handleWheel); | ||
| }, []); |
There was a problem hiding this comment.
wheel handler registers with passive: false on mount and persists for component lifetime. if tooltip never appears (single user mode or no hover), handler still evaluates on every wheel event.
minor performance impact: ~6 checks per scroll gesture. consider adding enableUserFilter guard in effect dependency to skip handler registration entirely when legend is disabled.
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 165-194
Comment:
wheel handler registers with `passive: false` on mount and persists for component lifetime. if tooltip never appears (single user mode or no hover), handler still evaluates on every wheel event.
minor performance impact: ~6 checks per scroll gesture. consider adding `enableUserFilter` guard in effect dependency to skip handler registration entirely when legend is disabled.
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
| <div className="flex items-center justify-center gap-2 mb-2"> | ||
| <div ref={legendRef} className="relative px-4 pb-2"> | ||
| {/* Control buttons (floating, does not take extra vertical space) */} | ||
| <div className="absolute right-4 top-0.5 z-10 w-24 flex flex-wrap justify-end gap-x-2 gap-y-0.5"> |
There was a problem hiding this comment.
right-4 won't flip for RTL languages - buttons stay on wrong side
Use logical property end-4 instead:
| <div className="absolute right-4 top-0.5 z-10 w-24 flex flex-wrap justify-end gap-x-2 gap-y-0.5"> | |
| <div className="absolute end-4 top-0.5 z-10 w-24 flex flex-wrap justify-end gap-x-2 gap-y-0.5"> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 554
Comment:
`right-4` won't flip for RTL languages - buttons stay on wrong side
Use logical property `end-4` instead:
```suggestion
<div className="absolute end-4 top-0.5 z-10 w-24 flex flex-wrap justify-end gap-x-2 gap-y-0.5">
```
How can I resolve this? If you propose a fix, please make it concise.| {/* User list with max 3 rows and scroll - only show users with non-zero usage */} | ||
| <div className="max-h-[72px] overflow-y-auto scrollbar-thin scrollbar-thumb-muted scrollbar-track-transparent"> | ||
| {/* User list with max 3 rows (3 * 24px = 72px) and scroll - only show users with non-zero usage */} | ||
| <div className="max-h-[72px] overflow-y-auto pr-24"> |
There was a problem hiding this comment.
pr-24 reserves space on right side only, breaks in RTL
Use logical property pe-24:
| <div className="max-h-[72px] overflow-y-auto pr-24"> | |
| <div className="max-h-[72px] overflow-y-auto pe-24"> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 591
Comment:
`pr-24` reserves space on right side only, breaks in RTL
Use logical property `pe-24`:
```suggestion
<div className="max-h-[72px] overflow-y-auto pe-24">
```
How can I resolve this? If you propose a fix, please make it concise.
| const observer = new ResizeObserver(compute); | ||
| observer.observe(card); | ||
| observer.observe(header); | ||
| observer.observe(metricTabs); | ||
| observer.observe(chartWrapper); | ||
| if (enableUserFilter && legendRef.current) { | ||
| observer.observe(legendRef.current); | ||
| } | ||
| window.addEventListener("resize", compute); |
There was a problem hiding this comment.
redundant window resize listener when ResizeObserver is available
line 158 adds window resize listener even when ResizeObserver is defined (line 145 check). since ResizeObserver already tracks size changes of observed elements, the window listener is redundant in modern browsers. consider removing window listener when ResizeObserver is available:
| const observer = new ResizeObserver(compute); | |
| observer.observe(card); | |
| observer.observe(header); | |
| observer.observe(metricTabs); | |
| observer.observe(chartWrapper); | |
| if (enableUserFilter && legendRef.current) { | |
| observer.observe(legendRef.current); | |
| } | |
| window.addEventListener("resize", compute); | |
| const observer = new ResizeObserver(compute); | |
| observer.observe(card); | |
| observer.observe(header); | |
| observer.observe(metricTabs); | |
| observer.observe(chartWrapper); | |
| if (enableUserFilter && legendRef.current) { | |
| observer.observe(legendRef.current); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 150-158
Comment:
redundant window resize listener when ResizeObserver is available
line 158 adds window resize listener even when ResizeObserver is defined (line 145 check). since ResizeObserver already tracks size changes of observed elements, the window listener is redundant in modern browsers. consider removing window listener when ResizeObserver is available:
```suggestion
const observer = new ResizeObserver(compute);
observer.observe(card);
observer.observe(header);
observer.observe(metricTabs);
observer.observe(chartWrapper);
if (enableUserFilter && legendRef.current) {
observer.observe(legendRef.current);
}
```
How can I resolve this? If you propose a fix, please make it concise.| const compute = () => { | ||
| const cardStyle = getComputedStyle(card); | ||
| const viewportHeightPx = window.visualViewport?.height ?? window.innerHeight; | ||
| const fallbackMaxHeightPx = Math.floor(Math.min(viewportHeightPx * 0.6, 720)); |
There was a problem hiding this comment.
Math.floor before Math.min reduces readability
flooring viewportHeightPx * 0.6 before comparing with 720 is less clear than Math.min(Math.floor(viewportHeightPx * 0.6), 720). current code: viewport 1199px → floor(719.4) = 719 → min(719, 720) = 719. suggested rewrite produces identical result but more clearly shows "60% of viewport, capped at 720px, rounded down"
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 100
Comment:
Math.floor before Math.min reduces readability
flooring `viewportHeightPx * 0.6` before comparing with 720 is less clear than `Math.min(Math.floor(viewportHeightPx * 0.6), 720)`. current code: viewport 1199px → floor(719.4) = 719 → min(719, 720) = 719. suggested rewrite produces identical result but more clearly shows "60% of viewport, capped at 720px, rounded down"
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
while not immediately problematic outside forms, explicit Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 329-343
Comment:
missing `type="button"` attributes on all interactive buttons
while not immediately problematic outside forms, explicit `type="button"` prevents unintended form submissions if this component is later used within a form context. affects buttons on lines 329, 337, 351, 372, 389, 555, 567, 604
How can I resolve this? If you propose a fix, please make it concise. |
| // - 但 vh/dvh 上限变大后，需要让图表高度回弹 | ||
| // 同时监听 visualViewport，覆盖移动端地址栏/键盘导致的可视区域变化。 | ||
| visualViewport?.addEventListener("resize", compute); | ||
| visualViewport?.addEventListener("scroll", compute); |
There was a problem hiding this comment.
visualViewport scroll listener is unnecessary
visualViewport scroll events fire in specific mobile scenarios (e.g., keyboard appearance), but scrolling shouldn't require chart height recalculation. The resize event on line 155 is sufficient.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 156
Comment:
`visualViewport` scroll listener is unnecessary
`visualViewport` scroll events fire in specific mobile scenarios (e.g., keyboard appearance), but scrolling shouldn't require chart height recalculation. The resize event on line 155 is sufficient.
How can I resolve this? If you propose a fix, please make it concise.| ref={cardRef} | ||
| className={cn( | ||
| "flex flex-col p-0 overflow-hidden max-h-[var(--cch-viewport-height-50)]", | ||
| "p-0 md:p-1 min-h-[300px] max-h-[min(var(--cch-viewport-height-60),720px)] overflow-visible hover:z-20", |
There was a problem hiding this comment.
hover:z-20 elevates entire card above adjacent elements on hover
while intended to help tooltip overflow, this creates z-index stacking issues - the card will appear above nearby cards/content when hovered, potentially causing visual disruption. tooltip already has zIndex: 1000 (line 470), which should be sufficient.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/bento/statistics-chart-card.tsx
Line: 325
Comment:
`hover:z-20` elevates entire card above adjacent elements on hover
while intended to help tooltip overflow, this creates z-index stacking issues - the card will appear above nearby cards/content when hovered, potentially causing visual disruption. tooltip already has `zIndex: 1000` (line 470), which should be sufficient.
How can I resolve this? If you propose a fix, please make it concise.
|
本轮根据 AI review 的性能建议做了小幅收敛：
已在本地重新跑完： |
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
目的
修复首页「使用统计」卡片（StatisticsChartCard）在浏览器视口高度较小时，底部 Legend / 操作按钮被裁切不可见的问题。
根因
首页 Bento 布局为保持紧凑，对该卡片设置了
max-h；但 Admin 多用户模式会额外渲染 Legend 与操作按钮。如果图表仍使用固定高度，总高度可能超过卡片高度上限，从而发生裁切。方案
在不引入卡片整体滚动条的前提下：
min-h: 300px+max-h: min(60dvh, 720px)（补充--cch-viewport-height-60变量，支持dvh时优先使用）。720pxcap：在常见 1080p 屏下60vh≈648px，不会触发 cap；在更大屏幕上避免卡片过高影响首页布局。ResizeObserver（仅观察 header/tabs/legend）+window.resize+visualViewport.resize；并用 rounding + safety gap 避免缩放/子像素导致的 1–2px 级裁切。min(80dvh, 720px)；鼠标悬停图表区域时滚轮优先滚动 tooltip 列表（无需把光标移入 tooltip 才能滚动）。UI 细节
p-0 md:p-1；Legend 操作按钮改为右上角悬浮，减少额外占行。whitespace-nowrap+ 预留宽度，并补rtl:适配；Legend 用户名截断补title。验证
bun run lint/bun run lint:fix/bun run typecheck/bun run test/bun run build手动验证建议
bun run dev，访问/dashboardGreptile Summary
Implements dynamic chart height calculation to prevent clipping of legend and controls on small viewports. The approach uses ResizeObserver to measure actual DOM element heights (header, tabs, legend, padding, borders) and calculates available space within card constraints (
min-h-[300px],max-h-[min(60vh, 720px)]).Key Changes:
getBoundingClientRect()measurements andparsePxutility to calculate remaining space after accounting for all UI elements--cch-viewport-height-60with dvh fallback for better mobile supportmin(80vh, 720px)p-0 md:p-1, metric tabs compressed frompy-3topy-2Major Concerns from Previous Review (36 existing comments):
overflow-visiblecontradicts stated goal of preventing card scrolling - allows content to overflow boundariesdata.usersdependency risk if users array changesright-4/pr-24workarounds instead of logical properties (end-4,pe-24)w-24,max-h-[72px]) lack documentationNote: PR title indicates this is [未完成] (incomplete/work-in-progress). Many of the flagged issues should be addressed before marking as ready for merge.
Confidence Score: 2/5
statistics-chart-card.tsx- address the 0px chart height edge case, reconsider overflow-visible vs stated goals, review all 36 flagged comments for validityImportant Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Component Mount] --> B[Initialize chartHeightPx State] B --> C[useLayoutEffect: Setup Height Calculation] C --> D{Check Required Refs} D -->|Missing| E[Return Early] D -->|All Present| F[Create compute Function] F --> G[Get viewport height<br/>visualViewport or innerHeight] G --> H[Calculate fallback: min 60% vh, 720px] H --> I[Parse CSS maxHeight from card] I --> J[Get effectiveMaxHeight<br/>max minHeight, maxHeight] J --> K[Measure Elements<br/>header, tabs, legend, padding, border] K --> L[Calculate Reserved Height<br/>sum + round + safetyGap 4px] L --> M[Calculate Available Height<br/>effectiveMax - reserved - border] M --> N[Clamp to Chart Max<br/>240px or 280px based on legend] N --> O{Height Changed?} O -->|No| P[Skip Update] O -->|Yes| Q[Set chartHeightPx State] Q --> R[Chart Re-renders with New Height] C --> S[Setup ResizeObserver<br/>or fallback to window resize] S --> T[Observe: header, tabs, legend<br/>Listen: window, visualViewport] T --> U[On Resize: Trigger compute] U --> F V[useEffect: Wheel Handler] --> W[Listen to Chart Wheel Events] W --> X{Tooltip Visible<br/>& Scrollable?} X -->|Yes| Y[Scroll Tooltip<br/>preventDefault] X -->|No| Z[Allow Default Scroll]Last reviewed commit: 9aad41f