perf(dashboard): comprehensive homepage performance optimization#808
Merged
Conversation
Replace direct DB calls (getUserStatisticsFromDB, getKeyStatisticsFromDB, getMixedStatisticsFromDB) with getStatisticsWithCache() in getUserStatistics(). The cache module handles Redis read-through with 30s TTL, distributed locking, and fail-open fallback to direct DB queries.
… charts - Remove cache: no-store from fetchLeaderboard to respect s-maxage=60 - Reduce overview polling from 5s to 15s with staleTime: 10_000 - Add staleTime: 30_000 and keepPreviousData to statistics query - Add staleTime: 60_000 to all 3 leaderboard queries - Lazy load StatisticsChartCard via next/dynamic with ssr: false
📝 WalkthroughWalkthrough本 PR 引入了基于 Redis 的概览和统计指标缓存层，通过新增数据库索引优化消息请求查询，重构统计数据仓库以提升性能，更新仪表板以利用缓存数据和服务端初始数据，并扩展缓存功能的测试覆盖。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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 |
Contributor
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a comprehensive performance optimization for the dashboard homepage, focusing on improving first-screen load speed, time range switching responsiveness, and overall user experience. It achieves this through significant PostgreSQL query rewrites, the introduction of robust Redis caching strategies, and various client-side rendering enhancements. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
Bot
added
size/XL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is an excellent and comprehensive performance optimization pull request. The changes, including the SQL query rewrite to eliminate CROSS JOIN, the introduction of Redis caching with thundering herd protection, and various client-side optimizations like SSR prefetching and lazy loading, are all well-implemented and will significantly improve the dashboard's performance and user experience. The code is well-structured and includes thorough testing for the new caching logic. I have a few suggestions to further improve maintainability and performance, but overall this is a fantastic contribution.
| logger.info("[StatisticsCache] Cache invalidated", { timeRange, scope, keysToDelete }); | ||
| } else { | ||
| const pattern = `statistics:*:*:${scope}`; | ||
| const matchedKeys = await redis.keys(pattern); |
Contributor
There was a problem hiding this comment.
The use of KEYS in production can be dangerous as it's a blocking command that can degrade Redis performance by scanning the entire keyspace. It's recommended to use SCAN for this purpose, which is an iterative and non-blocking alternative. Please use the existing scanPattern helper here and import it from ./scan-helper.
Suggested change
| const matchedKeys = await redis.keys(pattern); | |
| const matchedKeys = await scanPattern(pattern); |
Comment on lines
+20
to
+27
| function buildCacheKey( | ||
| timeRange: TimeRange, | ||
| mode: "users" | "keys" | "mixed", | ||
| userId?: number | ||
| ): string { | ||
| const scope = userId !== undefined ? `${userId}` : "global"; | ||
| return `statistics:${timeRange}:${mode}:${scope}`; | ||
| } |
Contributor
There was a problem hiding this comment.
This buildCacheKey function duplicates logic from buildStatisticsCacheKey in the new src/types/dashboard-cache.ts file. Similarly, src/lib/redis/overview-cache.ts duplicates its key building logic. To improve maintainability and avoid logic drift, these local implementations should be removed, and the centralized functions from src/types/dashboard-cache.ts should be imported and used in both cache modules.
Comment on lines
+10
to
+48
| const redisStore = new Map<string, { value: string; expiresAt: number }>(); | ||
|
|
||
| function readRedisValue(key: string): string | null { | ||
| const entry = redisStore.get(key); | ||
| if (!entry) { | ||
| return null; | ||
| } | ||
|
|
||
| if (entry.expiresAt <= Date.now()) { | ||
| redisStore.delete(key); | ||
| return null; | ||
| } | ||
|
|
||
| return entry.value; | ||
| } | ||
|
|
||
| const redisSetexMock = vi.fn(async (key: string, ttlSeconds: number, value: string) => { | ||
| redisStore.set(key, { | ||
| value, | ||
| expiresAt: Date.now() + ttlSeconds * 1000, | ||
| }); | ||
| return "OK"; | ||
| }); | ||
|
|
||
| const redisGetMock = vi.fn(async (key: string) => readRedisValue(key)); | ||
|
|
||
| const redisDelMock = vi.fn(async (key: string) => { | ||
| const existed = redisStore.delete(key); | ||
| return existed ? 1 : 0; | ||
| }); | ||
|
|
||
| const redisEvalMock = vi.fn(async (_script: string, _numKeys: number, key: string) => { | ||
| const value = readRedisValue(key); | ||
| if (value === null) { | ||
| return null; | ||
| } | ||
| redisStore.delete(key); | ||
| return value; | ||
| }); |
Contributor
There was a problem hiding this comment.
This block of code for mocking Redis (redisStore, readRedisValue, mock functions) is duplicated across several test files in this PR (e.g., provider-undo-edit.test.ts, providers-apply-engine.test.ts). To improve maintainability and reduce boilerplate, this logic should be extracted into a shared test utility (e.g., in a tests/test-utils directory) and imported where needed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/repository/statistics.ts (1)
486-488: 🛠️ Refactor suggestion | 🟠 Major删除旧索引
idx_message_request_user_date_cost，保留新索引idx_message_request_user_created_at_cost_stats两个索引列完全相同
(user_id, created_at, cost_usd)，但过滤条件不同。经过全库扫描，所有访问这些列的查询都包含EXCLUDE_WARMUP_CONDITION过滤（约 30+ 个查询点）。这意味着旧索引在生产环境中从未被用到，新索引已完全取代它。维持两个索引只会增加写入开销，无任何收益。建议在下一个迁移中删除旧索引。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/statistics.ts` around lines 486 - 488, Drop the redundant old index named idx_message_request_user_date_cost from the database migration and keep only idx_message_request_user_created_at_cost_stats, since both index columns are identical (user_id, created_at, cost_usd) and all queries use EXCLUDE_WARMUP_CONDITION so the old index is never used; update the next migration to remove idx_message_request_user_date_cost and ensure any creation or rollback code does not recreate it while keeping idx_message_request_user_created_at_cost_stats intact.
🧹 Nitpick comments (16)
tests/unit/actions/provider-undo-edit.test.ts (1)
170-174:vi.clearAllMocks()已覆盖各 mock 的mockClear()，171-174 行冗余第 168 行的
vi.clearAllMocks()内部对所有vi.fn()实例调用.mockClear()（清除calls/instances/results），因此第 171-174 行对各 redis mock 单独调用mockClear()是多余的。只有第 170 行的redisStore.clear()是真正必要的（原生Map不由 vitest 追踪）。♻️ 建议修改
beforeEach(() => { vi.clearAllMocks(); vi.resetModules(); redisStore.clear(); - redisSetexMock.mockClear(); - redisGetMock.mockClear(); - redisDelMock.mockClear(); - redisEvalMock.mockClear(); getSessionMock.mockResolvedValue({ user: { id: 1, role: "admin" } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/provider-undo-edit.test.ts` around lines 170 - 174, 删除对各 redis mock 的冗余 .mockClear() 调用：保留 redisStore.clear()（原生 Map 需手动清理）和 vi.clearAllMocks()，移除对 redisSetexMock.mockClear()、redisGetMock.mockClear()、redisDelMock.mockClear() 和 redisEvalMock.mockClear() 的单独调用，以避免重复清理由 vi.clearAllMocks() 已完成的 mocks。src/repository/statistics.ts (1)
149-153:normalizeTotalCost中value === undefined检查超出类型签名范围。参数类型为
string | number | null，不包含undefined。虽然是防御性编程，但会误导读者认为undefined是预期输入。建议移除或在类型签名中显式添加undefined。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/statistics.ts` around lines 149 - 153, The parameter check for undefined in normalizeTotalCost is inconsistent with its signature (string | number | null); remove the defensive "value === undefined" branch or alternatively widen the parameter type to include undefined—preferably remove the undefined check and leave: handle null explicitly, return finite numbers or 0 for non-finite, and return string values as-is in normalizeTotalCost so the function behavior matches its declared signature.tests/unit/actions/providers-undo-engine.test.ts (2)
9-71: Redis 模拟实现在多个测试文件间重复。
redisStore、readRedisValue、redisSetexMock/redisGetMock/redisDelMock/redisEvalMock以及vi.mock("@/lib/redis/client", ...)在providers-undo-engine.test.ts和providers-patch-actions-contract.test.ts中完全一致。可以考虑抽取到tests/helpers/redis-mock.ts共享模块中，减少后续维护成本。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/providers-undo-engine.test.ts` around lines 9 - 71, Extract the duplicate Redis mock into a shared helper module (e.g. tests/helpers/redis-mock.ts) that exports redisStore, readRedisValue, redisSetexMock, redisGetMock, redisDelMock, redisEvalMock and a function to register the vi.mock for "@/lib/redis/client"; then replace the inline definitions and the vi.mock("@/lib/redis/client", ...) block in providers-undo-engine.test.ts and providers-patch-actions-contract.test.ts with imports from that helper and call the helper's register function so both tests reuse the same mock symbols.
152-159:vi.clearAllMocks()之后的mockClear()调用是冗余的。
vi.clearAllMocks()已经会重置所有 mock 的调用历史。后续对redisSetexMock.mockClear()等的逐个调用在大多数情况下是多余的。不过考虑到vi.resetModules()的存在可能导致模块级 mock 状态不确定，保留也无害。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/providers-undo-engine.test.ts` around lines 152 - 159, 在 beforeEach 中调用 vi.clearAllMocks() 后，逐个调用 redisSetexMock.mockClear(), redisGetMock.mockClear(), redisDelMock.mockClear(), redisEvalMock.mockClear() 是冗余的；请删除这些单独的 mockClear() 调用，只保留 vi.clearAllMocks()（保留 vi.resetModules() 和 redisStore.clear() 不变），以简化测试初始化（定位参考：beforeEach, vi.clearAllMocks, vi.resetModules, redisSetexMock.mockClear, redisGetMock.mockClear, redisDelMock.mockClear, redisEvalMock.mockClear）。src/types/dashboard-cache.ts (1)
3-6:OverviewCacheKey类型的userId字段应根据scope值进行条件约束。当前类型允许
{ scope: "user" }不带userId，与上方函数签名问题同根同源。可以使用判别联合类型 (discriminated union) 提升类型安全：建议修改
-export type OverviewCacheKey = { - scope: "global" | "user"; - userId?: number; -}; +export type OverviewCacheKey = + | { scope: "global" } + | { scope: "user"; userId: number };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/dashboard-cache.ts` around lines 3 - 6, The OverviewCacheKey type currently allows a { scope: "user" } without a userId; change it to a discriminated union so scope "global" has no userId and scope "user" requires userId. Update the type definition for OverviewCacheKey to be a union of two shapes (one for scope: "global" and one for scope: "user" with userId: number) so the compiler enforces presence of userId when scope === "user".drizzle/0071_purple_captain_midlands.sql (1)
1-2: 生产环境大表上的CREATE INDEX（非 CONCURRENTLY）可能导致锁表。
message_request是写入频繁的大表，两条CREATE INDEX语句在不加CONCURRENTLY的情况下会对表加ShareLock，阻塞所有写入操作直到索引构建完成。建议在生产环境部署时考虑：
- 在低峰期执行迁移
- 或手动先执行
CREATE INDEX CONCURRENTLY，再运行迁移（迁移会发现索引已存在而跳过）此文件由
db:generate自动生成，不建议直接修改。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@drizzle/0071_purple_captain_midlands.sql` around lines 1 - 2, The two non-CONCURRENTLY CREATE INDEX statements on table "message_request" (indexes idx_message_request_user_created_at_cost_stats and idx_message_request_provider_created_at_active) will take ShareLock and block writes on a hot table; fix by either (A) updating the migration to create these indexes using CONCURRENTLY (CREATE INDEX CONCURRENTLY ...) so index build doesn't block writes, or (B) do not modify the auto-generated file and instead run CREATE INDEX CONCURRENTLY for those two indexes manually during a low-traffic window before applying the migration so the migration will skip them if they already exist; ensure you target the exact index names above when applying either change.src/lib/redis/overview-cache.ts (2)
51-60: DB 查询异常时锁未释放（依赖 TTL 自动过期）如果
getOverviewMetricsWithComparison(userId)抛出异常，执行会跳入catch块，锁不会被主动释放，需等待LOCK_TTL（5 秒）过期。当前行为可接受，但可考虑在finally块中释放锁以缩短阻塞窗口。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/redis/overview-cache.ts` around lines 51 - 60, When calling getOverviewMetricsWithComparison(userId) inside the cache-miss path ensure the lock (lockKey) is always released in a finally block: move the await redis.del(lockKey) into a finally so it runs whether the DB call succeeds or throws, and optionally only delete the lock if this process still owns it (if you use lock values); adjust the flow around redis.setex and redis.del accordingly so CACHE_TTL/LOCK_TTL remain unchanged.
39-58: 锁释放不安全：可能误删其他实例持有的锁当 DB 查询耗时超过
LOCK_TTL（5 秒）时，锁会自动过期，另一个实例可能已获取新锁。此时redis.del(lockKey)会删除其他实例的锁，破坏互斥保护。经典做法是用 Lua 脚本进行 compare-and-delete，确保只删除自己持有的锁。考虑到
LOCK_TTL=5s且概览查询通常较快，实际风险较低，但建议改进。建议修复
+import { randomUUID } from "crypto"; + +const UNLOCK_SCRIPT = ` +if redis.call("get", KEYS[1]) == ARGV[1] then + return redis.call("del", KEYS[1]) +else + return 0 +end +`; + export async function getOverviewWithCache( userId?: number ): Promise<OverviewMetricsWithComparison> { const redis = getRedisClient(); const cacheKey = buildCacheKey(userId); const lockKey = `${cacheKey}:lock`; + const lockValue = randomUUID(); // ... // 2. Acquire lock (prevent thundering herd) - const lockAcquired = await redis.set(lockKey, "1", "EX", LOCK_TTL, "NX"); + const lockAcquired = await redis.set(lockKey, lockValue, "EX", LOCK_TTL, "NX"); // ... // 5. Release lock - await redis.del(lockKey); + await redis.eval(UNLOCK_SCRIPT, 1, lockKey, lockValue);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/redis/overview-cache.ts` around lines 39 - 58, The lock release is unsafe because redis.del(lockKey) can remove a lock acquired by another instance after LOCK_TTL expires; change the lock to store a unique token (e.g., UUID) when calling redis.set(lockKey, token, "EX", LOCK_TTL, "NX") and then release the lock with a compare-and-delete Lua script (via redis.eval) that checks the stored value equals your token before deleting; update the acquire/release flow around lockKey, LOCK_TTL, LOCK_WAIT_MS and getOverviewMetricsWithComparison so the token is kept per-attempt and the release is executed in a finally path only if the script confirms the token matched.tests/unit/actions/provider-undo-delete.test.ts (2)
96-103:vi.clearAllMocks()后的mockClear()调用冗余
vi.clearAllMocks()已清理所有 mock 的调用记录和返回值，后续的redisSetexMock.mockClear()等调用是多余的。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/provider-undo-delete.test.ts` around lines 96 - 103, The beforeEach contains redundant per-mock mockClear() calls after vi.clearAllMocks(); remove the explicit redisSetexMock.mockClear(), redisGetMock.mockClear(), redisDelMock.mockClear(), and redisEvalMock.mockClear() calls and keep vi.clearAllMocks(), vi.resetModules(), and redisStore.clear() intact so mocks are globally cleared but the test-specific store is still reset; update the beforeEach block around the beforeEach function, vi.clearAllMocks(), vi.resetModules(), and redisStore.clear() accordingly.
10-48: Redis mock 基础设施在多个测试文件中重复
redisStore、readRedisValue、redisSetexMock/redisGetMock/redisDelMock/redisEvalMock及vi.mock("@/lib/redis/client", ...)模式在provider-undo-delete.test.ts和providers-apply-engine.test.ts中完全相同。建议提取到共享的测试辅助模块（如tests/helpers/redis-mock.ts），减少维护成本。Also applies to: 62-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/provider-undo-delete.test.ts` around lines 10 - 48, Extract the duplicated Redis test fixtures into a shared test helper module (e.g., redis-mock) and import it into both tests: move redisStore, readRedisValue, redisSetexMock, redisGetMock, redisDelMock, redisEvalMock and the vi.mock("@/lib/redis/client", ...) wrapper into that module; update provider-undo-delete.test.ts and providers-apply-engine.test.ts to import the mocks and ensure they are reset between tests (clear redisStore and mock histories) so behavior remains identical. Use the same exported symbol names (redisStore, readRedisValue, redisSetexMock, redisGetMock, redisDelMock, redisEvalMock) so existing test code can reference them with minimal edits. Ensure the vi.mock factory in the helper returns the mocks used by tests. Finally, run the unit tests and adjust any import/namespace references that changed.src/actions/statistics.ts (1)
65-94: 缓存返回值的类型断言（as）缺乏运行时保障三个分支都对
getStatisticsWithCache的返回值使用as强制类型转换。如果缓存中存在旧格式数据（如 schema 变更后），运行时不会报错但会产生不可预期行为。建议在缓存层添加轻量运行时校验（如 zod schema 或简单字段检查），或在反序列化时做基本结构验证，以提高容错性。当前实现依赖 TTL 较短（30 秒），实际风险较低。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/statistics.ts` around lines 65 - 94, The code unsafely casts getStatisticsWithCache results to specific shapes (used in statsData, mixedData) without runtime checks; add lightweight runtime validation after each getStatisticsWithCache call to verify expected fields/types (e.g., check arrayness and required properties like id, name, usage, or use a small zod/io-ts schema) and handle mismatches by falling back to an empty array or re-fetching/invalidating cache before assigning to statsData or using mixedData. Specifically, validate the return in the "users", "keys", and "mixed" branches (including mixedData.ownKeys and mixedData.othersAggregate) and only assign to statsData/entities when the shape matches; on validation failure log the error and treat as empty/refresh to avoid silent runtime issues.tests/unit/dashboard/dashboard-cache-keys.test.ts (1)
5-12: 建议补充边界用例：scope="user"但userId未传当调用
buildOverviewCacheKey("user")不传userId时，会生成"overview:user:undefined"这种无效 key。建议添加测试用例以记录此行为或在实现侧增加防御。建议补充的测试
+ it("produces a key with 'undefined' when scope is 'user' but userId is omitted", () => { + expect(buildOverviewCacheKey("user")).toBe("overview:user:undefined"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/dashboard/dashboard-cache-keys.test.ts` around lines 5 - 12, Add a boundary test and/or guard for buildOverviewCacheKey when called with scope="user" but no userId: update or add a unit test that calls buildOverviewCacheKey("user") and asserts the expected behavior (preferably that it throws an error or returns a defined safe value instead of "overview:user:undefined"), and if you choose to enforce the contract, modify buildOverviewCacheKey to validate the userId (throw a descriptive Error like "userId required for user scope" or return a safe fallback) and update tests to assert that validated behavior.src/app/[locale]/dashboard/_components/bento/dashboard-bento.tsx (2)
33-33:REFRESH_INTERVAL常量名存在语义偏差。overview 查询已改为
15_000字面量（Line 129），REFRESH_INTERVAL = 5000现在仅在 active-sessions 处使用（Line 138）。常量名暗示"通用刷新间隔"，但实际只适用于会话轮询，容易误导后续维护者。建议重命名以明确其用途，或将
15_000也提取为命名常量以保持一致性。建议修改
-const REFRESH_INTERVAL = 5000; +const SESSION_REFRESH_INTERVAL = 5_000; +const OVERVIEW_REFRESH_INTERVAL = 15_000;并相应更新引用处。
Also applies to: 129-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/bento/dashboard-bento.tsx at line 33, REFRESH_INTERVAL is misleading because it's only used for active-sessions polling while the overview query uses a separate literal 15_000; rename REFRESH_INTERVAL to something specific like ACTIVE_SESSIONS_REFRESH_MS (or extract OVERVIEW_REFRESH_MS = 15_000) and update all references in dashboard-bento.tsx (e.g., where REFRESH_INTERVAL is used for active sessions and the overview query at the line using 15_000) so both intervals are named constants that clearly express their purpose.
28-31: 动态导入StatisticsChartCard时缺少 loading 占位。
next/dynamic的ssr: false配置合理，可以避免在服务端渲染大型图表库。但当statistics有值而 JS chunk 尚未加载完成时，用户会看到空白区域。建议添加loading选项提供占位骨架，改善首屏体验。建议修改
const StatisticsChartCard = dynamic( () => import("./statistics-chart-card").then((mod) => ({ default: mod.StatisticsChartCard })), - { ssr: false } + { + ssr: false, + loading: () => <div className="h-[400px] animate-pulse rounded-xl bg-muted" />, + } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/bento/dashboard-bento.tsx around lines 28 - 31, The dynamic import for StatisticsChartCard uses ssr: false but no loading placeholder, causing an empty gap while the JS chunk loads; update the dynamic(...) call that creates StatisticsChartCard to include a loading option that renders a lightweight skeleton or placeholder component (e.g., a small Skeleton/Spinner or a stub that mimics the chart card layout) so when statistics data exists but the chunk hasn't loaded the UI shows a proper placeholder until the real StatisticsChartCard mounts.tests/unit/redis/overview-cache.test.ts (1)
176-209:invalidateOverviewCache测试覆盖了核心场景。包括正常删除、Redis 不可用时静默返回、以及 Redis 错误吞没。不过注意：没有测试全局 key（无 userId 参数）的正常删除路径——Line 189 和 Line 208 分别测试了有 userId 和无 userId 的场景，但前者验证的是
"overview:user:42"的删除，后者只验证了错误吞没。建议补充一个测试验证
invalidateOverviewCache()在无 userId 时删除"overview:global"key 的行为。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/redis/overview-cache.test.ts` around lines 176 - 209, The tests miss asserting the normal deletion path for the global key when invalidateOverviewCache() is called without a userId; add a new unit test in tests/unit/redis/overview-cache.test.ts that mocks getRedisClient to return a createRedisMock(), has redis.del.mockResolvedValueOnce(1), calls await invalidateOverviewCache(), and asserts redis.del was called with "overview:global" (similar structure to the existing "deletes the correct cache key" test but without passing a userId).src/lib/redis/statistics-cache.ts (1)
159-164:redis.keys()在大规模 Redis 实例中可能阻塞主线程。
KEYS命令的时间复杂度为 O(N)（N 为 Redis 中所有 key 数量），在生产环境中可能造成延迟尖峰。当前场景下统计缓存 key 数量有限，风险可控，但如果 Redis 实例共用于其他高频数据，建议后续迁移到SCAN迭代方式。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/redis/statistics-cache.ts` around lines 159 - 164, The code uses redis.keys(pattern) which can block large Redis instances; replace this with a SCAN-based iterator to collect matching keys without blocking: use the existing redis client (redis) and the pattern variable (pattern) to repeatedly call redis.scan with a cursor, accumulate matches into batches, and call redis.del on each batch (instead of awaiting redis.del(...matchedKeys) for all keys at once); update the logic in the same block in statistics-cache.ts where matchedKeys and pattern are used to perform non-blocking, batched deletions.
🤖 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/lib/redis/statistics-cache.ts`:
- Around line 87-104: The branch that handles locked === "OK" (inside the
function in statistics-cache.ts that uses lockKey, cacheKey, CACHE_TTL,
queryDatabase, redis.setex and redis.del) must ensure the distributed lock is
always released; wrap the work after acquiring the lock in a try/finally: call
queryDatabase and set the cache (redis.setex) inside the try and always call
redis.del(lockKey) in the finally so the lock is released even if queryDatabase
throws, while preserving the existing logging (logger.debug/info) and returning
data when successful.
- Around line 33-46: queryDatabase currently uses non-null assertion userId!
when calling getKeyStatisticsFromDB and getMixedStatisticsFromDB, which can pass
undefined at runtime; add a runtime validation at the top of queryDatabase that
throws a clear Error if mode is "keys" or "mixed" and userId is undefined (e.g.,
"queryDatabase: userId required for mode 'keys'/'mixed'"), then call
getKeyStatisticsFromDB(userId, timeRange) / getMixedStatisticsFromDB(userId,
timeRange) without the !; alternatively you can enforce correct call patterns by
replacing the signature with overloads that require userId for "keys" and
"mixed" and keep the runtime check for safety.
In `@src/repository/statistics.ts`:
- Around line 251-261: The returned user_name currently uses the hardcoded
Chinese string "其他用户" inside the buckets.map return (see buckets.map and rowMap
usage); replace that literal with a semantic identifier like "__others__" (keep
user_id: -1) so the server action returns a locale-agnostic token; ensure the
frontend (or the StatisticsUser construction path) performs i18n mapping for
user_name === "__others__" into the five supported locales (zh-CN, zh-TW, en,
ja, ru).
In `@src/types/dashboard-cache.ts`:
- Around line 14-16: The buildOverviewCacheKey function allows calling
buildOverviewCacheKey("user") without a userId, producing an invalid key; fix by
adding TypeScript overloads to enforce types at compile time—declare overloads
like (scope: "global") => string and (scope: "user", userId: number) =>
string—then keep a single implementation signature (scope: "global" | "user",
userId?: number): string and add a runtime guard in the implementation of
buildOverviewCacheKey to throw (or assert) if scope === "user" and userId is
missing before returning the key; reference function name buildOverviewCacheKey
in the change.
---
Outside diff comments:
In `@src/repository/statistics.ts`:
- Around line 486-488: Drop the redundant old index named
idx_message_request_user_date_cost from the database migration and keep only
idx_message_request_user_created_at_cost_stats, since both index columns are
identical (user_id, created_at, cost_usd) and all queries use
EXCLUDE_WARMUP_CONDITION so the old index is never used; update the next
migration to remove idx_message_request_user_date_cost and ensure any creation
or rollback code does not recreate it while keeping
idx_message_request_user_created_at_cost_stats intact.
---
Duplicate comments:
In `@tests/unit/actions/providers-apply-engine.test.ts`:
- Around line 8-46: This test file repeats Redis mock setup (redisStore,
readRedisValue, redisSetexMock, redisGetMock, redisDelMock, redisEvalMock);
extract that shared mock infrastructure into a single helper module (e.g.,
tests/helpers/redis-mock) exporting the store, readRedisValue, and the mock
functions, then replace the in-file definitions with imports of those symbols
and call a reset/clear function between tests to avoid cross-test leakage;
update both providers-apply-engine.test.ts and provider-undo-delete.test.ts to
import and use the shared mocks
(redisStore/readRedisValue/redisSetexMock/redisGetMock/redisDelMock/redisEvalMock)
instead of duplicating code.
- Around line 150-158: The individual mockClear() calls are redundant after
vi.clearAllMocks(); remove the redundant calls to redisSetexMock.mockClear(),
redisGetMock.mockClear(), redisDelMock.mockClear(), and
redisEvalMock.mockClear() in the beforeEach block, keeping vi.clearAllMocks(),
vi.resetModules(), and redisStore.clear() as needed to reset state.
In `@tests/unit/actions/providers-patch-actions-contract.test.ts`:
- Around line 7-69: The Redis mock implementation (redisStore, readRedisValue,
redisSetexMock, redisGetMock, redisDelMock, redisEvalMock) is duplicated with
providers-undo-engine.test.ts; extract this shared mock into a test utility
module (e.g., tests/utils/mockRedis.ts) and import it from both tests, replacing
the inline definitions and vi.mock("@/lib/redis/client", ...) usage with the
shared mock factory or exported mocks so both files reuse the same
implementation and avoid duplication.
---
Nitpick comments:
In `@drizzle/0071_purple_captain_midlands.sql`:
- Around line 1-2: The two non-CONCURRENTLY CREATE INDEX statements on table
"message_request" (indexes idx_message_request_user_created_at_cost_stats and
idx_message_request_provider_created_at_active) will take ShareLock and block
writes on a hot table; fix by either (A) updating the migration to create these
indexes using CONCURRENTLY (CREATE INDEX CONCURRENTLY ...) so index build
doesn't block writes, or (B) do not modify the auto-generated file and instead
run CREATE INDEX CONCURRENTLY for those two indexes manually during a
low-traffic window before applying the migration so the migration will skip them
if they already exist; ensure you target the exact index names above when
applying either change.
In `@src/actions/statistics.ts`:
- Around line 65-94: The code unsafely casts getStatisticsWithCache results to
specific shapes (used in statsData, mixedData) without runtime checks; add
lightweight runtime validation after each getStatisticsWithCache call to verify
expected fields/types (e.g., check arrayness and required properties like id,
name, usage, or use a small zod/io-ts schema) and handle mismatches by falling
back to an empty array or re-fetching/invalidating cache before assigning to
statsData or using mixedData. Specifically, validate the return in the "users",
"keys", and "mixed" branches (including mixedData.ownKeys and
mixedData.othersAggregate) and only assign to statsData/entities when the shape
matches; on validation failure log the error and treat as empty/refresh to avoid
silent runtime issues.
In `@src/app/`[locale]/dashboard/_components/bento/dashboard-bento.tsx:
- Line 33: REFRESH_INTERVAL is misleading because it's only used for
active-sessions polling while the overview query uses a separate literal 15_000;
rename REFRESH_INTERVAL to something specific like ACTIVE_SESSIONS_REFRESH_MS
(or extract OVERVIEW_REFRESH_MS = 15_000) and update all references in
dashboard-bento.tsx (e.g., where REFRESH_INTERVAL is used for active sessions
and the overview query at the line using 15_000) so both intervals are named
constants that clearly express their purpose.
- Around line 28-31: The dynamic import for StatisticsChartCard uses ssr: false
but no loading placeholder, causing an empty gap while the JS chunk loads;
update the dynamic(...) call that creates StatisticsChartCard to include a
loading option that renders a lightweight skeleton or placeholder component
(e.g., a small Skeleton/Spinner or a stub that mimics the chart card layout) so
when statistics data exists but the chunk hasn't loaded the UI shows a proper
placeholder until the real StatisticsChartCard mounts.
In `@src/lib/redis/overview-cache.ts`:
- Around line 51-60: When calling getOverviewMetricsWithComparison(userId)
inside the cache-miss path ensure the lock (lockKey) is always released in a
finally block: move the await redis.del(lockKey) into a finally so it runs
whether the DB call succeeds or throws, and optionally only delete the lock if
this process still owns it (if you use lock values); adjust the flow around
redis.setex and redis.del accordingly so CACHE_TTL/LOCK_TTL remain unchanged.
- Around line 39-58: The lock release is unsafe because redis.del(lockKey) can
remove a lock acquired by another instance after LOCK_TTL expires; change the
lock to store a unique token (e.g., UUID) when calling redis.set(lockKey, token,
"EX", LOCK_TTL, "NX") and then release the lock with a compare-and-delete Lua
script (via redis.eval) that checks the stored value equals your token before
deleting; update the acquire/release flow around lockKey, LOCK_TTL, LOCK_WAIT_MS
and getOverviewMetricsWithComparison so the token is kept per-attempt and the
release is executed in a finally path only if the script confirms the token
matched.
In `@src/lib/redis/statistics-cache.ts`:
- Around line 159-164: The code uses redis.keys(pattern) which can block large
Redis instances; replace this with a SCAN-based iterator to collect matching
keys without blocking: use the existing redis client (redis) and the pattern
variable (pattern) to repeatedly call redis.scan with a cursor, accumulate
matches into batches, and call redis.del on each batch (instead of awaiting
redis.del(...matchedKeys) for all keys at once); update the logic in the same
block in statistics-cache.ts where matchedKeys and pattern are used to perform
non-blocking, batched deletions.
In `@src/repository/statistics.ts`:
- Around line 149-153: The parameter check for undefined in normalizeTotalCost
is inconsistent with its signature (string | number | null); remove the
defensive "value === undefined" branch or alternatively widen the parameter type
to include undefined—preferably remove the undefined check and leave: handle
null explicitly, return finite numbers or 0 for non-finite, and return string
values as-is in normalizeTotalCost so the function behavior matches its declared
signature.
In `@src/types/dashboard-cache.ts`:
- Around line 3-6: The OverviewCacheKey type currently allows a { scope: "user"
} without a userId; change it to a discriminated union so scope "global" has no
userId and scope "user" requires userId. Update the type definition for
OverviewCacheKey to be a union of two shapes (one for scope: "global" and one
for scope: "user" with userId: number) so the compiler enforces presence of
userId when scope === "user".
In `@tests/unit/actions/provider-undo-delete.test.ts`:
- Around line 96-103: The beforeEach contains redundant per-mock mockClear()
calls after vi.clearAllMocks(); remove the explicit redisSetexMock.mockClear(),
redisGetMock.mockClear(), redisDelMock.mockClear(), and
redisEvalMock.mockClear() calls and keep vi.clearAllMocks(), vi.resetModules(),
and redisStore.clear() intact so mocks are globally cleared but the
test-specific store is still reset; update the beforeEach block around the
beforeEach function, vi.clearAllMocks(), vi.resetModules(), and
redisStore.clear() accordingly.
- Around line 10-48: Extract the duplicated Redis test fixtures into a shared
test helper module (e.g., redis-mock) and import it into both tests: move
redisStore, readRedisValue, redisSetexMock, redisGetMock, redisDelMock,
redisEvalMock and the vi.mock("@/lib/redis/client", ...) wrapper into that
module; update provider-undo-delete.test.ts and providers-apply-engine.test.ts
to import the mocks and ensure they are reset between tests (clear redisStore
and mock histories) so behavior remains identical. Use the same exported symbol
names (redisStore, readRedisValue, redisSetexMock, redisGetMock, redisDelMock,
redisEvalMock) so existing test code can reference them with minimal edits.
Ensure the vi.mock factory in the helper returns the mocks used by tests.
Finally, run the unit tests and adjust any import/namespace references that
changed.
In `@tests/unit/actions/provider-undo-edit.test.ts`:
- Around line 170-174: 删除对各 redis mock 的冗余 .mockClear() 调用：保留
redisStore.clear()（原生 Map 需手动清理）和 vi.clearAllMocks()，移除对
redisSetexMock.mockClear()、redisGetMock.mockClear()、redisDelMock.mockClear() 和
redisEvalMock.mockClear() 的单独调用，以避免重复清理由 vi.clearAllMocks() 已完成的 mocks。
In `@tests/unit/actions/providers-undo-engine.test.ts`:
- Around line 9-71: Extract the duplicate Redis mock into a shared helper module
(e.g. tests/helpers/redis-mock.ts) that exports redisStore, readRedisValue,
redisSetexMock, redisGetMock, redisDelMock, redisEvalMock and a function to
register the vi.mock for "@/lib/redis/client"; then replace the inline
definitions and the vi.mock("@/lib/redis/client", ...) block in
providers-undo-engine.test.ts and providers-patch-actions-contract.test.ts with
imports from that helper and call the helper's register function so both tests
reuse the same mock symbols.
- Around line 152-159: 在 beforeEach 中调用 vi.clearAllMocks() 后，逐个调用
redisSetexMock.mockClear(), redisGetMock.mockClear(), redisDelMock.mockClear(),
redisEvalMock.mockClear() 是冗余的；请删除这些单独的 mockClear() 调用，只保留 vi.clearAllMocks()（保留
vi.resetModules() 和 redisStore.clear() 不变），以简化测试初始化（定位参考：beforeEach,
vi.clearAllMocks, vi.resetModules, redisSetexMock.mockClear,
redisGetMock.mockClear, redisDelMock.mockClear, redisEvalMock.mockClear）。
In `@tests/unit/dashboard/dashboard-cache-keys.test.ts`:
- Around line 5-12: Add a boundary test and/or guard for buildOverviewCacheKey
when called with scope="user" but no userId: update or add a unit test that
calls buildOverviewCacheKey("user") and asserts the expected behavior
(preferably that it throws an error or returns a defined safe value instead of
"overview:user:undefined"), and if you choose to enforce the contract, modify
buildOverviewCacheKey to validate the userId (throw a descriptive Error like
"userId required for user scope" or return a safe fallback) and update tests to
assert that validated behavior.
In `@tests/unit/redis/overview-cache.test.ts`:
- Around line 176-209: The tests miss asserting the normal deletion path for the
global key when invalidateOverviewCache() is called without a userId; add a new
unit test in tests/unit/redis/overview-cache.test.ts that mocks getRedisClient
to return a createRedisMock(), has redis.del.mockResolvedValueOnce(1), calls
await invalidateOverviewCache(), and asserts redis.del was called with
"overview:global" (similar structure to the existing "deletes the correct cache
key" test but without passing a userId).
Comment on lines
+33
to
+46
| async function queryDatabase( | ||
| timeRange: TimeRange, | ||
| mode: "users" | "keys" | "mixed", | ||
| userId?: number | ||
| ): Promise<StatisticsCacheData> { | ||
| switch (mode) { | ||
| case "users": | ||
| return await getUserStatisticsFromDB(timeRange); | ||
| case "keys": | ||
| return await getKeyStatisticsFromDB(userId!, timeRange); | ||
| case "mixed": | ||
| return await getMixedStatisticsFromDB(userId!, timeRange); | ||
| } | ||
| } |
There was a problem hiding this comment.
queryDatabase 中 userId! 非空断言存在运行时风险。
当 mode 为 "keys" 或 "mixed" 时，userId 是必需的，但函数签名允许省略（userId?: number）。如果调用方传入 mode="keys" 但不传 userId，userId! 断言不会抛错，而是将 undefined 传入 DB 函数，导致 SQL 查询异常。
建议增加运行时校验或使用函数重载约束合法调用组合：
建议修改（运行时校验）
async function queryDatabase(
timeRange: TimeRange,
mode: "users" | "keys" | "mixed",
userId?: number
): Promise<StatisticsCacheData> {
+ if ((mode === "keys" || mode === "mixed") && userId === undefined) {
+ throw new Error(`userId is required for mode="${mode}"`);
+ }
switch (mode) {
case "users":
return await getUserStatisticsFromDB(timeRange);
case "keys":
return await getKeyStatisticsFromDB(userId!, timeRange);
case "mixed":
return await getMixedStatisticsFromDB(userId!, timeRange);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function queryDatabase( | |
| timeRange: TimeRange, | |
| mode: "users" | "keys" | "mixed", | |
| userId?: number | |
| ): Promise<StatisticsCacheData> { | |
| switch (mode) { | |
| case "users": | |
| return await getUserStatisticsFromDB(timeRange); | |
| case "keys": | |
| return await getKeyStatisticsFromDB(userId!, timeRange); | |
| case "mixed": | |
| return await getMixedStatisticsFromDB(userId!, timeRange); | |
| } | |
| } | |
| async function queryDatabase( | |
| timeRange: TimeRange, | |
| mode: "users" | "keys" | "mixed", | |
| userId?: number | |
| ): Promise<StatisticsCacheData> { | |
| if ((mode === "keys" || mode === "mixed") && userId === undefined) { | |
| throw new Error(`userId is required for mode="${mode}"`); | |
| } | |
| switch (mode) { | |
| case "users": | |
| return await getUserStatisticsFromDB(timeRange); | |
| case "keys": | |
| return await getKeyStatisticsFromDB(userId!, timeRange); | |
| case "mixed": | |
| return await getMixedStatisticsFromDB(userId!, timeRange); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/redis/statistics-cache.ts` around lines 33 - 46, queryDatabase
currently uses non-null assertion userId! when calling getKeyStatisticsFromDB
and getMixedStatisticsFromDB, which can pass undefined at runtime; add a runtime
validation at the top of queryDatabase that throws a clear Error if mode is
"keys" or "mixed" and userId is undefined (e.g., "queryDatabase: userId required
for mode 'keys'/'mixed'"), then call getKeyStatisticsFromDB(userId, timeRange) /
getMixedStatisticsFromDB(userId, timeRange) without the !; alternatively you can
enforce correct call patterns by replacing the signature with overloads that
require userId for "keys" and "mixed" and keep the runtime check for safety.
Comment on lines
+14
to
+16
| export function buildOverviewCacheKey(scope: "global" | "user", userId?: number): string { | ||
| return scope === "global" ? "overview:global" : `overview:user:${userId}`; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/types/dashboard-cache.tsRepository: ding113/claude-code-hub
Length of output: 860
🏁 Script executed:
rg -n "buildOverviewCacheKey" --type=ts -C 3Repository: ding113/claude-code-hub
Length of output: 1744
🏁 Script executed:
rg -n "buildOverviewCacheKey|buildStatisticsCacheKey" --type=ts --type=tsx -g "!tests" -g "!*.test.ts" -C 2Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
rg -n "overview:user|overview:global|statistics:" --type=ts --type=tsx -C 2Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
rg -n "buildOverviewCacheKey|buildStatisticsCacheKey" --type=ts -g "!tests" -C 2Repository: ding113/claude-code-hub
Length of output: 630
🏁 Script executed:
rg -n "buildOverviewCacheKey|buildStatisticsCacheKey" -g "!*.test.*" -C 2Repository: ding113/claude-code-hub
Length of output: 630
🏁 Script executed:
rg "import.*buildOverviewCacheKey" -C 5Repository: ding113/claude-code-hub
Length of output: 777
🏁 Script executed:
rg "buildOverviewCacheKey\(" -C 3Repository: ding113/claude-code-hub
Length of output: 1350
当 scope === "user" 时缺少必需的 userId 参数保证。
函数签名允许 buildOverviewCacheKey("user") 不传 userId，会生成 "overview:user:undefined" 的无效缓存键。虽然当前测试覆盖了正确用法，但建议使用 TypeScript 重载强制类型约束，防止错误调用：
建议的类型安全方案
-export function buildOverviewCacheKey(scope: "global" | "user", userId?: number): string {
- return scope === "global" ? "overview:global" : `overview:user:${userId}`;
-}
+export function buildOverviewCacheKey(scope: "global"): string;
+export function buildOverviewCacheKey(scope: "user", userId: number): string;
+export function buildOverviewCacheKey(scope: "global" | "user", userId?: number): string {
+ return scope === "global" ? "overview:global" : `overview:user:${userId}`;
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/types/dashboard-cache.ts` around lines 14 - 16, The buildOverviewCacheKey
function allows calling buildOverviewCacheKey("user") without a userId,
producing an invalid key; fix by adding TypeScript overloads to enforce types at
compile time—declare overloads like (scope: "global") => string and (scope:
"user", userId: number) => string—then keep a single implementation signature
(scope: "global" | "user", userId?: number): string and add a runtime guard in
the implementation of buildOverviewCacheKey to throw (or assert) if scope ===
"user" and userId is missing before returning the key; reference function name
buildOverviewCacheKey in the change.
3 tasks
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR implements comprehensive performance optimizations for the dashboard homepage, including PostgreSQL index additions, Redis caching layers, SSR prefetching, and client-side optimizations. The changes are well-structured and include thorough test coverage.
PR Size: XL
- Lines changed: 5,481
- Files changed: 22
Split Recommendation: Consider splitting future PRs of this size into:
- SQL optimizations (indexes + query changes)
- Redis caching layer
- Client-side optimizations
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 1 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issue
[ERROR-SILENT] Silent Failure in Cache Invalidation
File: src/lib/redis/statistics-cache.ts:171
The invalidateStatisticsCache function silently swallows Redis errors. While this follows the fail-open pattern used elsewhere, it provides no visibility when Redis operations fail.
Current code:
} catch (error) {
// Silent failure - no logging
}Suggested fix:
} catch (error) {
logger.error('[StatisticsCache] Failed to invalidate cache', {
timeRange,
scope,
error
});
}Positive Notes
-
Excellent Test Coverage: 22 new unit tests with comprehensive coverage of cache hit/miss scenarios, lock contention, and fallback behaviors.
-
Well-Implemented Thundering Herd Protection: Both
overview-cache.tsandstatistics-cache.tsproperly implement distributed locking with SET NX EX patterns. -
Proper Fail-Open Design: Redis failures consistently fall back to direct database queries without breaking functionality.
-
SSR Prefetch Implementation: The
dashboard-bento-sections.tsxproperly prefetches data server-side to eliminate client waterfalls. -
Clean Cache Key Design: The cache key builders properly separate admin (global) vs user-scoped queries.
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - 1 minor issue identified
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Excellent (22 new tests)
- Code clarity - Good
Automated review by Claude AI
| return data; | ||
| } catch (error) { | ||
| logger.warn("[OverviewCache] Redis error, fallback to direct query", { userId, error }); | ||
| return await getOverviewMetricsWithComparison(userId); |
Contributor
There was a problem hiding this comment.
[HIGH] [ERROR-BROAD-CATCH] getOverviewWithCache can re-run the DB query and mislabel non-Redis failures
Evidence: src/lib/redis/overview-cache.ts:63
Problematic code:
return await getOverviewMetricsWithComparison(userId);Why this is a problem: The outer try { ... } catch wraps both Redis operations and getOverviewMetricsWithComparison(). If any step throws after the DB query already succeeded (e.g. redis.setex() / redis.del()), the catch logs a "Redis error" and executes the DB query again, doubling DB load and producing misleading logs.
Suggested fix (avoid double-query + always release lock):
let lockAcquired = false;
let data: OverviewMetricsWithComparison | undefined;
try {
// ... cache read ...
lockAcquired = (await redis.set(lockKey, "1", "EX", LOCK_TTL, "NX")) === "OK";
// ... retry + fallback when !lockAcquired ...
data = await getOverviewMetricsWithComparison(userId);
try {
await redis.setex(cacheKey, CACHE_TTL, JSON.stringify(data));
} catch (error) {
logger.warn("[OverviewCache] Redis write failed, serving uncached result", { userId, error });
}
return data;
} catch (error) {
logger.warn("[OverviewCache] Cache path error, fallback to direct query", { userId, error });
return data ?? (await getOverviewMetricsWithComparison(userId));
} finally {
if (lockAcquired) {
try {
await redis.del(lockKey);
} catch (error) {
logger.warn("[OverviewCache] Failed to release lock", { userId, error });
}
}
}
| mode, | ||
| error, | ||
| }); | ||
| return await queryDatabase(timeRange, mode, userId); |
Contributor
There was a problem hiding this comment.
[HIGH] [ERROR-BROAD-CATCH] getStatisticsWithCache can re-run the DB query and mislabel non-Redis failures
Evidence: src/lib/redis/statistics-cache.ts:132
Problematic code:
return await queryDatabase(timeRange, mode, userId);Why this is a problem: The outer try { ... } catch wraps both Redis operations and queryDatabase(). If any step throws after data was already computed (e.g. redis.setex() / redis.del()), the catch logs a "Redis error" and executes the DB query again, doubling DB load and hiding the real failure source.
Suggested fix (compute once; best-effort cache write; always release lock):
if (locked === "OK") {
let data: StatisticsCacheData;
try {
data = await queryDatabase(timeRange, mode, userId);
try {
await redis.setex(cacheKey, CACHE_TTL, JSON.stringify(data));
} catch (error) {
logger.warn("[StatisticsCache] Redis write failed, serving uncached result", {
timeRange,
mode,
userId,
error,
});
}
return data;
} finally {
try {
await redis.del(lockKey);
} catch (error) {
logger.warn("[StatisticsCache] Failed to release lock", { timeRange, mode, userId, error });
}
}
}
Contributor
There was a problem hiding this comment.
Code Review Summary
This is an XL PR that changes dashboard stats aggregation, introduces new Redis caches, and adjusts client/SSR fetching behavior. The main risk is failure-path correctness: the new cache implementations can re-run expensive DB queries and mislabel non-Redis errors as Redis failures, which undermines both performance and debuggability.
PR Size: XL
- Lines changed: 5481
- Files changed: 22
- Split suggestion (XL): Consider splitting into (1) SQL/statistics rewrite + indexes/migration, (2) Redis cache modules + wiring, (3) dashboard SSR/client query tuning, (4) tests and drizzle meta snapshots.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 2 | 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/lib/redis/overview-cache.ts:63[HIGH] [ERROR-BROAD-CATCH] Catch-all fallback can double-query DB and mislabel non-Redis failures (Confidence: 90)src/lib/redis/statistics-cache.ts:132[HIGH] [ERROR-BROAD-CATCH] Catch-all fallback can double-query DB and mislabel non-Redis failures (Confidence: 90)
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
PR #808
- Applied label
size/XL(5481 lines changed across 22 files). - Submitted PR review summary and posted 2 inline comments.
High-Priority Issues
src/lib/redis/overview-cache.ts:63[HIGH] [ERROR-BROAD-CATCH] Catch-all fallback can re-run the DB query (e.g., aftersetex/delfailures) and mislabel non-Redis failures as Redis errors; suggested restructuring to compute once +finallylock release.src/lib/redis/statistics-cache.ts:132[HIGH] [ERROR-BROAD-CATCH] Same pattern: catch-all fallback can double-hit DB and obscure the real failure source; suggested compute-once + best-effort cache write +finallylock release.
- Replace O(N) redis.keys() with cursor-based scanPattern() in invalidateStatisticsCache (issue 1) - Fix lock not released when queryDatabase throws: move del(lockKey) to finally block in both statistics-cache and overview-cache (issues 2+4) - Wrap setex in inner try/catch so Redis write failure doesn't trigger double DB query via outer catch (issues 3+4) - Guard queryDatabase against undefined userId for keys/mixed modes (issue 5) - Remove duplicate buildCacheKey; use buildStatisticsCacheKey from dashboard-cache.ts throughout (issue 6) - Add TypeScript overloads to buildOverviewCacheKey preventing overview:user:undefined keys at compile time (issue 7) - Replace hardcoded Chinese sentinel "其他用户" with "__others__" and map it to i18n key othersAggregate in 5 locales (issue 8) - Extract duplicated Redis in-memory mock into shared tests/unit/actions/redis-mock-utils.ts (issue 9)
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
This was referenced Feb 23, 2026
This was referenced Mar 1, 2026
18 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Comprehensive performance optimization for the dashboard homepage — first-screen load speed, time range switching speed, PostgreSQL query optimization, Redis caching, and smooth UX during data transitions.
Related Issues:
message_requestto eliminate full table scans during statistics queriesChanges
SQL Optimization
CROSS JOIN users x generate_seriesanti-pattern that defeated PostgreSQL indexes. Replaced with directWHERErange conditions +GROUP BY+ application-level zero-fill. (~634 lines removed, ~301 added)message_requestfor statistics and provider-scoped queries (migration0071)Redis Caching
overview-cache.ts) — 10s TTL, fail-open fallback, thundering herd protection via distributed lockstatistics-cache.ts) — 30s TTL, same pattern, mode-aware cache keys (users/keys/mixed)SSR & Client Optimization
keepPreviousDataon statistics query — No blank flash when switching time rangesstaleTimealigned with Redis TTLs — Overview 10s, statistics 30s, leaderboard 60scache: "no-store"from leaderboard fetch — Free win, lets browser respect existings-maxage=60headernext/dynamicwithssr: falseTesting
Files Changed (15)
src/repository/statistics.tssrc/lib/redis/overview-cache.tssrc/lib/redis/statistics-cache.tssrc/types/dashboard-cache.tssrc/lib/redis/index.tssrc/actions/overview.tssrc/actions/statistics.tssrc/app/.../dashboard-bento-sections.tsxsrc/app/.../dashboard-bento.tsxsrc/drizzle/schema.tsdrizzle/0071_*.sqltests/unit/redis/overview-cache.test.tstests/unit/redis/statistics-cache.test.tstests/unit/dashboard/dashboard-cache-keys.test.tsVerification
bun run build— PASSbun run typecheck— PASSbun run lint— PASSbun run test(new files) — 22/22 PASSDescription enhanced by Claude AI
Greptile Summary
Comprehensive dashboard performance optimization addressing SQL anti-patterns, Redis caching, and frontend UX.
Key Improvements:
CROSS JOIN users x generate_seriesanti-pattern that defeated PostgreSQL indexes. Replaced with directWHERErange conditions +GROUP BY+ application-level zero-fill, reducing ~634 lines to ~301 lines while maintaining correctnessmessage_requesttable matching theblockedBy IS NULL OR blockedBy <> 'warmup'filter used throughout statistics queries - eliminates full table scanskeepPreviousDataprevents blank flash when switching time rangesstaleTimealigned with Redis TTLs (10s/30s/60s)next/dynamic+ssr: falsecache: "no-store"from leaderboard (respects existings-maxage=60)Architecture:
The change correctly separates concerns - SQL queries return sparse data (only rows with activity), and zero-filling happens in application code. This allows PostgreSQL to use the new partial indexes effectively. Redis cache sits between actions and repository layers with proper thundering herd protection.
Confidence Score: 5/5
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Client: Dashboard Page Request] --> B[SSR: dashboard-bento-sections] B --> C{Prefetch Data in Parallel} C --> D[getUserStatistics] C --> E[getOverviewData] D --> F[getStatisticsWithCache] E --> G[getOverviewWithCache] F --> H{Redis Available?} G --> I{Redis Available?} H -->|Yes| J{Cache Hit?} H -->|No| K[Direct DB Query] I -->|Yes| L{Cache Hit?} I -->|No| M[Direct DB Query] J -->|Yes| N[Return Cached Data - 30s TTL] J -->|No| O{Acquire Lock?} L -->|Yes| P[Return Cached Data - 10s TTL] L -->|No| Q{Acquire Lock?} O -->|Yes| R[Query DB via getUserStatisticsFromDB] O -->|No| S[Wait 100ms & Retry 50x] Q -->|Yes| T[Query DB via getOverviewMetricsWithComparison] Q -->|No| U[Wait 100ms & Retry Once] R --> V[LEFT JOIN message_request<br/>WHERE created_at >= startTs<br/>AND created_at < endTs<br/>Uses idx_message_request_user_created_at_cost_stats] V --> W[App-Level Zero-Fill] W --> X[Cache Result & Release Lock] X --> N T --> Y[Aggregate Queries<br/>Uses partial indexes] Y --> Z[Cache Result & Release Lock] Z --> P S -->|Timeout| K U -->|Timeout| M N --> AA[Render with initialData] P --> AA K --> AA M --> AA AA --> AB[Client: React Query] AB --> AC{keepPreviousData<br/>staleTime Checks} AC --> AD[Smooth Transitions<br/>No Flash on Time Range Switch]Last reviewed commit: b429b34