feat: soft user limit reset via costResetAt#853
Open
miraserver wants to merge 8 commits intoding113:devfrom
Open
feat: soft user limit reset via costResetAt#853miraserver wants to merge 8 commits intoding113:devfrom
miraserver wants to merge 8 commits intoding113:devfrom
Conversation
Add costResetAt timestamp column to users table that clips all cost calculations to start from reset time instead of all-time. This enables admins to reset a user's rate limits without destroying historical usage data (messageRequest/usageLedger rows are preserved). Key changes: - Schema: new cost_reset_at column on users table - Repository: costResetAt propagated through all select queries, key validation, and statistics aggregation (with per-user batch support) - Rate limiting: all 12 proxy guard checks pass costResetAt; service and lease layers clip time windows accordingly - Auth cache: hydrate costResetAt from Redis cache as Date; invalidate auth cache on reset to avoid stale costResetAt - Actions: resetUserLimitsOnly sets costResetAt + clears cost cache; getUserLimitUsage/getUserAllLimitUsage/getKeyLimitUsage/getMyQuota clip time ranges by costResetAt - UI: edit-user-dialog with separate Reset Limits Only (amber) vs Reset All Statistics (red) with confirmation dialogs - i18n: all 5 languages (en, zh-CN, zh-TW, ja, ru) - Tests: 10 unit tests for resetUserLimitsOnly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…te validation - Extract resetUserCostResetAt repo function with updatedAt + auth cache invalidation - Extract clearUserCostCache helper to deduplicate Redis cleanup between reset functions - Use instanceof Date checks in lease-service and my-usage for costResetAt validation - Remove dead hasActiveSessions variable in cost-cache-cleanup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e in user edit dialog R3: Replace truthiness checks with `instanceof Date` in 3 places (users.ts clipStart, quotas page). R4: Show last reset timestamp in edit-user-dialog Reset Limits section (5 langs). Add 47 unit tests covering costResetAt across key-quota, redis cleanup, statistics, and auth cache. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Clip all period time ranges by user's costResetAt and replace getTotalUsageForKey with sumKeyTotalCost supporting resetAt parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nable mock Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthrough在 users 表新增可选时间戳字段 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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, 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 introduces a significant feature for managing user quotas: the ability to perform a 'soft reset' of usage limits. Instead of wiping all historical data, this change allows administrators to effectively zero out a user's current spending windows by setting a 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
|
![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 pull request introduces a valuable feature for soft-resetting user cost limits using a costResetAt timestamp. The implementation is thorough, spanning the database schema, backend logic, rate-limiting services, and frontend components, and is well-supported by extensive unit tests. My review focuses on a few areas where consistency and code clarity could be enhanced. I've suggested standardizing the costResetAt validation to use instanceof Date across all files for improved robustness. Additionally, I've recommended refactoring a complex nested ternary operator to improve readability in the statistics repository. These are minor points in an otherwise solid contribution.
src/actions/keys.ts
Outdated
Comment on lines
+742
to
+743
| const clipStart = (start: Date): Date => | ||
| costResetAt && costResetAt > start ? costResetAt : start; |
Contributor
There was a problem hiding this comment.
For consistency with other parts of the codebase and for improved type safety, it's better to use instanceof Date to validate costResetAt. The current truthiness check (costResetAt && ...) is less robust and could be brittle if the type contract changes in the future.
Suggested change
| const clipStart = (start: Date): Date => | |
| costResetAt && costResetAt > start ? costResetAt : start; | |
| const clipStart = (start: Date): Date => | |
| costResetAt instanceof Date && costResetAt > start ? costResetAt : start; |
Contributor
Author
There was a problem hiding this comment.
Fixed in da42780: added instanceof Date guard to clipStart and eliminated the redundant findUserById call by adding userCostResetAt to the existing JOIN select.
src/lib/rate-limit/service.ts
Outdated
| ); | ||
|
|
||
| // Clip startTime forward if costResetAt is more recent | ||
| const effectiveStartTime = costResetAt && costResetAt > startTime ? costResetAt : startTime; |
Contributor
There was a problem hiding this comment.
For consistency and improved robustness, it would be better to use instanceof Date for checking costResetAt here, similar to how it's done in other parts of the PR. This ensures that you are working with a valid Date object before performing the comparison.
Suggested change
| const effectiveStartTime = costResetAt && costResetAt > startTime ? costResetAt : startTime; | |
| const effectiveStartTime = | |
| costResetAt instanceof Date && costResetAt > startTime ? costResetAt : startTime; |
src/repository/statistics.ts
Outdated
Comment on lines
+775
to
+780
| const cutoffDate = | ||
| resetAt instanceof Date && !Number.isNaN(resetAt.getTime()) | ||
| ? maxAgeCutoff && maxAgeCutoff > resetAt | ||
| ? maxAgeCutoff | ||
| : resetAt | ||
| : maxAgeCutoff; |
Contributor
There was a problem hiding this comment.
This nested ternary operator is a bit complex and can be hard to parse at a glance. Refactoring this logic into a more straightforward if block would improve code readability and maintainability, making the intent of selecting the more recent date clearer.
Suggested change
| const cutoffDate = | |
| resetAt instanceof Date && !Number.isNaN(resetAt.getTime()) | |
| ? maxAgeCutoff && maxAgeCutoff > resetAt | |
| ? maxAgeCutoff | |
| : resetAt | |
| : maxAgeCutoff; | |
| // Use the more recent of maxAgeCutoff and resetAt | |
| let cutoffDate = maxAgeCutoff; | |
| if (resetAt instanceof Date && !Number.isNaN(resetAt.getTime())) { | |
| if (!cutoffDate || resetAt > cutoffDate) { | |
| cutoffDate = resetAt; | |
| } | |
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in da42780: refactored to if-block.
src/repository/statistics.ts
Outdated
Comment on lines
+853
to
+858
| const cutoffDate = | ||
| resetAt instanceof Date && !Number.isNaN(resetAt.getTime()) | ||
| ? maxAgeCutoff && maxAgeCutoff > resetAt | ||
| ? maxAgeCutoff | ||
| : resetAt | ||
| : maxAgeCutoff; |
Contributor
There was a problem hiding this comment.
Similar to the sumUserQuotaCosts function, this nested ternary for determining cutoffDate is quite complex. A refactor to a simple if statement would make the logic more explicit and easier to maintain.
Suggested change
| const cutoffDate = | |
| resetAt instanceof Date && !Number.isNaN(resetAt.getTime()) | |
| ? maxAgeCutoff && maxAgeCutoff > resetAt | |
| ? maxAgeCutoff | |
| : resetAt | |
| : maxAgeCutoff; | |
| // Use the more recent of maxAgeCutoff and resetAt | |
| let cutoffDate = maxAgeCutoff; | |
| if (resetAt instanceof Date && !Number.isNaN(resetAt.getTime())) { | |
| if (!cutoffDate || resetAt > cutoffDate) { | |
| cutoffDate = resetAt; | |
| } | |
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in da42780: refactored to if-block (same pattern as sumUserQuotaCosts).
github-actions
bot
added
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/unit/lib/redis/cost-cache-cleanup.test.ts (1)
217-240: 建议补充pipeline.execreject 场景测试，锁定“失败不抛出”契约。当前只覆盖了
exec返回错误元组，未覆盖 Promise reject（网络抖动常见路径）。🧪 建议补测用例
+ test("pipeline exec reject -- returns null and does not throw", async () => { + scanPatternMock.mockImplementation(async (_redis: unknown, pattern: string) => { + if (pattern === "key:1:cost_*") return ["key:1:cost_daily"]; + return []; + }); + redisPipelineMock.exec.mockRejectedValue(new Error("Connection reset")); + + const { clearUserCostCache } = await import("@/lib/redis/cost-cache-cleanup"); + await expect( + clearUserCostCache({ + userId: 10, + keyIds: [1], + keyHashes: [], + }) + ).resolves.toBeNull(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/lib/redis/cost-cache-cleanup.test.ts` around lines 217 - 240, Add a new unit test that simulates redis pipeline Promise rejection to verify clearUserCostCache still fulfills the "does not throw" contract: mock scanPatternMock to return keys as in the existing test, then set redisPipelineMock.exec.mockRejectedValue(new Error("Connection reset")) (or similar) to emulate network rejection, call clearUserCostCache({ userId: 10, keyIds: [1], keyHashes: [] }), assert the call resolves (result not null), that costKeysDeleted equals the expected count (2), and that loggerMock.warn was called with the same "Some Redis deletes failed during cost cache cleanup" message and metadata containing errorCount and userId; reference clearUserCostCache, redisPipelineMock.exec and scanPatternMock when locating the code to modify.
🤖 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/actions/keys.ts`:
- Around line 736-744: Avoid the extra DB round-trip by removing the second
findUserById call and reuse the already-joined owner's costResetAt (use the
existing owner on the key instead of calling findUserById); then harden
clipStart to validate types by checking both start and costResetAt are valid
Date instances (e.g., instanceof Date and !isNaN(costResetAt.getTime())) before
comparing, otherwise return start unchanged; update references to costResetAt
and clipStart accordingly so the function uses the reused owner value and an
explicit Date guard.
In `@src/actions/users.ts`:
- Line 1835: 调用 resetUserCostResetAt(userId, new Date())
后必须检查其返回值并在失败时中止或返回错误；也就是说，在 users.ts 中对 resetUserCostResetAt 的结果进行布尔判断（例如 const
ok = await resetUserCostResetAt(...)），当 ok 为 false 时不要继续返回 { ok: true
}，而应抛出或返回一个表示失败的响应（例如返回 { ok: false, error: 'resetUserCostResetAt failed' }
或抛出异常），确保调用者能感知到仓储层未生效（并在日志中记录用户 ID 与相关上下文以便排查）。
In `@src/lib/rate-limit/service.ts`:
- Around line 463-465: The comparison for computing effectiveStartTime currently
does a direct > between costResetAt and startTime which can silently fail if
either is not a Date; update the logic that sets effectiveStartTime (the
variables costResetAt, startTime, effectiveStartTime in this service) to first
guard that both costResetAt and startTime are instances of Date and compare
their numeric values via getTime() (e.g. use instanceof Date &&
costResetAt.getTime() > startTime.getTime()) so non-Date inputs are ignored and
the window-clipping behaves consistently with other checks in this file.
In `@src/lib/redis/cost-cache-cleanup.ts`:
- Around line 75-101: The Redis pipeline.exec() call in clearUserCostCache can
throw on transient Redis errors, violating the "Never throws" contract; wrap the
pipeline.exec() invocation in a try/catch around the pipeline variable and
handle failures by logging the full error (use logger.error or logger.warn with
context: userId, keyIds, and intent "cost cache cleanup") and returning
gracefully (do not rethrow) so the reset/limit flow can continue; ensure the
existing post-exec error-count check remains but is only executed when exec
succeeds, and keep function signature/return semantics unchanged.
In `@src/repository/statistics.ts`:
- Around line 473-480: sumKeyTotalCost and sumUserTotalCost treat resetAt as
absolute priority and can widen the window when resetAt is earlier than the
maxAgeDays cutoff; change the logic to use the later of resetAt and the
maxAgeDays cutoff (i.e., "clip" to the more recent date) like the sum*QuotaCosts
functions do. Concretely, compute a single startDate that is the max between a
validated resetAt Date and the cutoffDate derived from maxAgeDays, then push
gte(usageLedger.createdAt, startDate) (apply the same fix in the other affected
block around the 502-509 region). Ensure you still ignore invalid resetAt values
and treat non-finite/<=0 maxAgeDays as no cutoff.
In `@tests/unit/actions/key-quota-cost-reset.test.ts`:
- Around line 139-145: The test uses a too-broad assertion for
sumKeyCostInTimeRangeMock; replace the generic "calledTimes(4) and one call with
costResetAt" check with precise assertions that each of the four expected
windows was called with the exact clipped start time: assert the 5h-range call,
the daily (midnight) call, the weekly call, and the monthly call each include
costResetAt as their start when appropriate (use sumKeyCostInTimeRangeMock and
costResetAt/NOW), and verify call order or use toHaveBeenNthCalledWith (or
inspect mock.calls) to ensure each specific invocation matches its expected
(start, end) tuple rather than only counting total calls.
In `@tests/unit/repository/statistics-reset-at.test.ts`:
- Around line 46-103: The tests for sumUserTotalCost and sumKeyTotalCost only
assert numeric results and risk false positives; modify the tests to also assert
the query/where arguments used when dbResultMock is invoked (e.g., inspect the
mock call args or implement dbResultMock to capture the constructed cutoff
time), and verify that when a valid resetAt (Date) is passed the cutoff equals
resetAt, while when resetAt is omitted/null/invalid the cutoff equals now minus
maxAgeDays; reference the sumUserTotalCost and sumKeyTotalCost calls and the
dbResultMock mock to locate where to add these assertions.
---
Nitpick comments:
In `@tests/unit/lib/redis/cost-cache-cleanup.test.ts`:
- Around line 217-240: Add a new unit test that simulates redis pipeline Promise
rejection to verify clearUserCostCache still fulfills the "does not throw"
contract: mock scanPatternMock to return keys as in the existing test, then set
redisPipelineMock.exec.mockRejectedValue(new Error("Connection reset")) (or
similar) to emulate network rejection, call clearUserCostCache({ userId: 10,
keyIds: [1], keyHashes: [] }), assert the call resolves (result not null), that
costKeysDeleted equals the expected count (2), and that loggerMock.warn was
called with the same "Some Redis deletes failed during cost cache cleanup"
message and metadata containing errorCount and userId; reference
clearUserCostCache, redisPipelineMock.exec and scanPatternMock when locating the
code to modify.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (30)
drizzle/0078_tearful_punisher.sqldrizzle/meta/0078_snapshot.jsondrizzle/meta/_journal.jsonmessages/en/dashboard.jsonmessages/ja/dashboard.jsonmessages/ru/dashboard.jsonmessages/zh-CN/dashboard.jsonmessages/zh-TW/dashboard.jsonsrc/actions/key-quota.tssrc/actions/keys.tssrc/actions/my-usage.tssrc/actions/users.tssrc/app/[locale]/dashboard/_components/user/edit-user-dialog.tsxsrc/app/[locale]/dashboard/quotas/users/page.tsxsrc/app/v1/_lib/proxy/rate-limit-guard.tssrc/drizzle/schema.tssrc/lib/rate-limit/lease-service.tssrc/lib/rate-limit/service.tssrc/lib/redis/cost-cache-cleanup.tssrc/lib/security/api-key-auth-cache.tssrc/repository/_shared/transformers.tssrc/repository/key.tssrc/repository/statistics.tssrc/repository/user.tssrc/types/user.tstests/unit/actions/key-quota-cost-reset.test.tstests/unit/actions/users-reset-limits-only.test.tstests/unit/lib/redis/cost-cache-cleanup.test.tstests/unit/lib/security/api-key-auth-cache-reset-at.test.tstests/unit/repository/statistics-reset-at.test.ts
Comment on lines
+46
to
+103
| test("with valid resetAt -- queries DB and returns cost", async () => { | ||
| const resetAt = new Date("2026-02-15T00:00:00Z"); | ||
| dbResultMock.mockReturnValue([{ total: 42.5 }]); | ||
|
|
||
| const { sumUserTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCost(10, 365, resetAt); | ||
|
|
||
| expect(result).toBe(42.5); | ||
| }); | ||
|
|
||
| test("without resetAt -- uses maxAgeDays cutoff instead", async () => { | ||
| dbResultMock.mockReturnValue([{ total: 100.0 }]); | ||
|
|
||
| const { sumUserTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCost(10, 365); | ||
|
|
||
| expect(result).toBe(100.0); | ||
| }); | ||
|
|
||
| test("with null resetAt -- treated same as undefined", async () => { | ||
| dbResultMock.mockReturnValue([{ total: 50.0 }]); | ||
|
|
||
| const { sumUserTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCost(10, 365, null); | ||
|
|
||
| expect(result).toBe(50.0); | ||
| }); | ||
|
|
||
| test("with invalid Date (NaN) -- skips resetAt, falls through to maxAgeDays", async () => { | ||
| const invalidDate = new Date("invalid"); | ||
| dbResultMock.mockReturnValue([{ total: 75.0 }]); | ||
|
|
||
| const { sumUserTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumUserTotalCost(10, 365, invalidDate); | ||
|
|
||
| expect(result).toBe(75.0); | ||
| }); | ||
| }); | ||
|
|
||
| describe("sumKeyTotalCost", () => { | ||
| test("with valid resetAt -- uses resetAt instead of maxAgeDays cutoff", async () => { | ||
| const resetAt = new Date("2026-02-20T00:00:00Z"); | ||
| dbResultMock.mockReturnValue([{ total: 15.0 }]); | ||
|
|
||
| const { sumKeyTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumKeyTotalCost("sk-hash", 365, resetAt); | ||
|
|
||
| expect(result).toBe(15.0); | ||
| }); | ||
|
|
||
| test("without resetAt -- falls back to maxAgeDays", async () => { | ||
| dbResultMock.mockReturnValue([{ total: 30.0 }]); | ||
|
|
||
| const { sumKeyTotalCost } = await import("@/repository/statistics"); | ||
| const result = await sumKeyTotalCost("sk-hash", 365); | ||
|
|
||
| expect(result).toBe(30.0); | ||
| }); |
There was a problem hiding this comment.
resetAt 核心分支目前“只验返回值”，无法证明查询条件真的切换。
这些用例把 dbResultMock 设成固定返回后，仅断言结果数字；即便 resetAt 条件被误删，测试依然可能通过。建议增加对查询构造的可观察断言（例如捕获 where 参数中的时间边界），避免关键逻辑出现假阳性。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/repository/statistics-reset-at.test.ts` around lines 46 - 103, The
tests for sumUserTotalCost and sumKeyTotalCost only assert numeric results and
risk false positives; modify the tests to also assert the query/where arguments
used when dbResultMock is invoked (e.g., inspect the mock call args or implement
dbResultMock to capture the constructed cutoff time), and verify that when a
valid resetAt (Date) is passed the cutoff equals resetAt, while when resetAt is
omitted/null/invalid the cutoff equals now minus maxAgeDays; reference the
sumUserTotalCost and sumKeyTotalCost calls and the dbResultMock mock to locate
where to add these assertions.
Contributor
Author
There was a problem hiding this comment.
Won't fix -- low ROI. The Drizzle chain mock pattern used here doesn't expose WHERE clause parameters. Refactoring the mock to capture query conditions would be complex and fragile. The logic is tested indirectly: if the resetAt branch is removed, callers that depend on correct cost calculations would break. The current tests verify the function contract (correct return values for given inputs), which is the pragmatic approach with Drizzle ORM mocking.
There was a problem hiding this comment.
@miraserver, understood — your tradeoff makes sense given the constraints of the Drizzle chain mock pattern. Contract-level verification is a pragmatic choice here. I'll leave it as is. 👍
![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 a soft user limit reset mechanism via costResetAt timestamp. The implementation is well-designed with proper error handling, consistent patterns, and comprehensive test coverage.
PR Size: XL
- Lines changed: 6,132 (5,917 additions, 215 deletions)
- Files changed: 30
Note: This is a large PR. Consider splitting similar future changes into smaller, focused PRs for easier review:
- Schema migration + core repository changes
- Rate limit service + guard updates
- Server actions + frontend UI
- Test files (could be in same PR as the code they test)
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Properly handled with logging
- Type safety - Consistent use of
instanceof Datechecks - Documentation accuracy - Comments explain the reset mechanism
- Test coverage - 47+ unit tests covering all new functionality
- Code clarity - Good
Notable Implementation Details
Well-designed aspects:
- The soft reset approach preserves audit data while allowing limit enforcement to start fresh
- Proper clipping of time windows in all cost queries (5h/daily/weekly/monthly/total)
- Redis cache cleanup utility is well-isolated with proper error handling
- Auth cache invalidation on
costResetAtupdate - i18n coverage for all 5 languages (en, ja, ru, zh-CN, zh-TW)
Error handling pattern (correct):
try {
const cacheResult = await clearUserCostCache({ userId, keyIds, keyHashes });
// ...success logging
} catch (error) {
logger.error("Failed to clear Redis cache...", { userId, error: ... });
// Continue execution - costResetAt already set in DB
}The fallback behavior is correct: even if Redis cache clearing fails, the DB has costResetAt set, so cost calculations will be correctly clipped.
Manual Verification Checklist (for author)
- Visual check: open user edit dialog for user with costResetAt, verify badge renders
- Test reset limits flow end-to-end in a dev environment
- Verify rate limiting correctly clips after reset
Automated review by Claude AI
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/actions/keys.ts
Outdated
Comment on lines
+742
to
+743
| const clipStart = (start: Date): Date => | ||
| costResetAt && costResetAt > start ? costResetAt : start; |
There was a problem hiding this comment.
Inconsistent date guard - uses truthy check instead of instanceof Date
The PR description states "Harden all costResetAt guards to instanceof Date (3 places)", but this clipStart uses a simple truthy check. Other places in the codebase use the hardened pattern:
const clipStart = (start: Date): Date =>
costResetAt instanceof Date && costResetAt > start ? costResetAt : start;This matters because if costResetAt is an invalid Date (NaN), the truthy check would pass but the comparison would fail silently.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/keys.ts
Line: 742-743
Comment:
Inconsistent date guard - uses truthy check instead of `instanceof Date`
The PR description states "Harden all `costResetAt` guards to `instanceof Date` (3 places)", but this clipStart uses a simple truthy check. Other places in the codebase use the hardened pattern:
```typescript
const clipStart = (start: Date): Date =>
costResetAt instanceof Date && costResetAt > start ? costResetAt : start;
```
This matters because if `costResetAt` is an invalid Date (NaN), the truthy check would pass but the comparison would fail silently.
How can I resolve this? If you propose a fix, please make it concise.
src/lib/rate-limit/service.ts
Outdated
| ); | ||
|
|
||
| // Clip startTime forward if costResetAt is more recent | ||
| const effectiveStartTime = costResetAt && costResetAt > startTime ? costResetAt : startTime; |
There was a problem hiding this comment.
Inconsistent date guard - uses truthy check instead of instanceof Date
Should match the hardened pattern used elsewhere:
const effectiveStartTime =
costResetAt instanceof Date && costResetAt > startTime ? costResetAt : startTime;The truthy check could allow invalid Date objects (NaN) to pass through.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/rate-limit/service.ts
Line: 464
Comment:
Inconsistent date guard - uses truthy check instead of `instanceof Date`
Should match the hardened pattern used elsewhere:
```typescript
const effectiveStartTime =
costResetAt instanceof Date && costResetAt > startTime ? costResetAt : startTime;
```
The truthy check could allow invalid Date objects (NaN) to pass through.
How can I resolve this? If you propose a fix, please make it concise.…dow clipping, error handling - keys.ts: eliminate redundant findUserById call, use joined costResetAt + instanceof Date guard - users.ts: handle resetUserCostResetAt return value (false = soft-deleted user) - service.ts: add instanceof Date guard to costResetAt comparison - statistics.ts: fix sumKeyTotalCost/sumUserTotalCost to use max(resetAt, maxAgeCutoff) instead of replacing maxAgeDays; refactor nested ternaries to if-blocks in quota functions - cost-cache-cleanup.ts: wrap pipeline.exec() in try/catch to honor never-throws contract - Update test for pipeline.exec throw now caught inside clearUserCostCache Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/actions/users.ts`:
- Around line 1903-1910: Wrap the three destructive operations into a single DB
transaction so they either all succeed or all roll back: move the deletes of
messageRequest and usageLedger and the call to resetUserCostResetAt(userId,
null) into one transactional block (use the project's DB transaction API, e.g.,
db.transaction/txn) and if the transaction fails return an explicit error result
rather than partial success; ensure you reference the existing symbols
messageRequest, usageLedger, resetUserCostResetAt, and db.delete inside that
transaction and propagate the transaction error back to the caller.
In `@src/lib/redis/cost-cache-cleanup.ts`:
- Around line 95-100: The catch block after pipeline.exec() currently returns
costKeysDeleted: allCostKeys.length which misreports deletions on failure;
update the error path in the cost cache cleanup routine (the catch after
pipeline.exec() in src/lib/redis/cost-cache-cleanup.ts) to return
costKeysDeleted: null (or 0 per your API convention) instead of
allCostKeys.length, keep activeSessionsDeleted and durationMs as before, and
ensure any return type/consumer expectations are adjusted to accept null/0 for
costKeysDeleted and downstream code handles the failure case appropriately.
In `@src/repository/statistics.ts`:
- Around line 783-786: The code only applies the resetAt cutoff to costTotal but
not to the 5h/day/week/month window starts; update the window start calculations
to use const effectiveStart = cutoffDate ? (rangeStart > cutoffDate ? rangeStart
: cutoffDate) : rangeStart (i.e., take max(range*.startTime, cutoffDate))
wherever range*.startTime is used to compute windowed sums, and apply the same
change in both sumUserQuotaCosts and sumKeyQuotaCostsById so each windowed
aggregation uses effectiveStart instead of the raw range start.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (6)
src/actions/keys.tssrc/actions/users.tssrc/lib/rate-limit/service.tssrc/lib/redis/cost-cache-cleanup.tssrc/repository/statistics.tstests/unit/actions/users-reset-limits-only.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/actions/users-reset-limits-only.test.ts
Comment on lines
+1903
to
+1910
| // 1. Delete all messageRequest logs for this user | ||
| await db.delete(messageRequest).where(eq(messageRequest.userId, userId)); | ||
|
|
||
| // Check for errors | ||
| const errors = results?.filter(([err]) => err); | ||
| if (errors && errors.length > 0) { | ||
| logger.warn("Some Redis deletes failed during user statistics reset", { | ||
| errorCount: errors.length, | ||
| userId, | ||
| }); | ||
| } | ||
| // Also clear ledger rows -- the ONLY legitimate DELETE path for usage_ledger | ||
| await db.delete(usageLedger).where(eq(usageLedger.userId, userId)); | ||
|
|
||
| const duration = Date.now() - startTime; | ||
| // Clear costResetAt since all data is wiped (also invalidates auth cache) | ||
| await resetUserCostResetAt(userId, null); |
There was a problem hiding this comment.
将“全量重置”改为单事务，避免不可逆的部分成功。
Line [1903] 到 Line [1910] 先删 messageRequest/usageLedger，再清 costResetAt。任一步失败都会留下部分成功状态（日志已删但用户重置状态未一致）。建议把这些写操作放进同一事务，并对用户更新结果做显式失败返回。
建议修复
- // 1. Delete all messageRequest logs for this user
- await db.delete(messageRequest).where(eq(messageRequest.userId, userId));
-
- // Also clear ledger rows -- the ONLY legitimate DELETE path for usage_ledger
- await db.delete(usageLedger).where(eq(usageLedger.userId, userId));
-
- // Clear costResetAt since all data is wiped (also invalidates auth cache)
- await resetUserCostResetAt(userId, null);
+ const updated = await db.transaction(async (tx) => {
+ await tx.delete(messageRequest).where(eq(messageRequest.userId, userId));
+ await tx.delete(usageLedger).where(eq(usageLedger.userId, userId));
+ const rows = await tx
+ .update(usersTable)
+ .set({ costResetAt: null, updatedAt: new Date() })
+ .where(and(eq(usersTable.id, userId), isNull(usersTable.deletedAt)))
+ .returning({ id: usersTable.id });
+ return rows.length > 0;
+ });
+ if (!updated) {
+ return { ok: false, error: tError("USER_NOT_FOUND"), errorCode: ERROR_CODES.NOT_FOUND };
+ }
+ // 如需保持与 resetUserCostResetAt 一致的缓存失效行为，请补充对应失效逻辑🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/actions/users.ts` around lines 1903 - 1910, Wrap the three destructive
operations into a single DB transaction so they either all succeed or all roll
back: move the deletes of messageRequest and usageLedger and the call to
resetUserCostResetAt(userId, null) into one transactional block (use the
project's DB transaction API, e.g., db.transaction/txn) and if the transaction
fails return an explicit error result rather than partial success; ensure you
reference the existing symbols messageRequest, usageLedger,
resetUserCostResetAt, and db.delete inside that transaction and propagate the
transaction error back to the caller.
Contributor
Author
There was a problem hiding this comment.
Won't fix in this PR -- scope concern. The existing resetUserAllStatistics function predates this PR and has always run non-transactional deletes. By line 1910, the user was already verified at line 1893, and data is already deleted at lines 1904/1907. The resetUserCostResetAt(userId, null) call is cleanup (clearing the reset marker since all data is wiped). If it fails due to a concurrent soft-delete, the primary operation (data wipe) already succeeded and the user is gone anyway. Wrapping in a transaction is a valid improvement but belongs in a separate refactor PR, not in the costResetAt feature scope.
There was a problem hiding this comment.
@miraserver, understood – I appreciate the clear scope boundary. You're right that the non-transactional pattern predates this PR, and wrapping all three operations (messageRequest delete, usageLedger delete, and costResetAt cleanup) in a transaction is a broader refactor concern rather than a costResetAt-specific issue.
Since the primary destructive operations (log/ledger deletion) complete before the cleanup step, and you've already verified the user exists early in the function, the risk window is narrow. The concurrent soft-delete scenario you mentioned is an edge case where the data wipe has already succeeded anyway.
If you'd like, I can open a follow-up issue to track the transaction wrapping improvement for resetUserAllStatistics so it's captured for a future refactor PR. Just let me know!
🧠 Learnings used
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 573
File: src/actions/model-prices.ts:275-335
Timestamp: 2026-01-10T06:19:58.167Z
Learning: Do not modify hardcoded Chinese error messages in Server Actions under src/actions/*.ts as part of piecemeal changes. This is a repo-wide architectural decision that requires a coordinated i18n refactor across all Server Action files (e.g., model-prices.ts, users.ts, system-config.ts). Treat i18n refactor as a separate unified task rather than per-PR changes, and plan a project-wide approach for replacing hardcoded strings with localized resources.
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 539
File: src/types/user.ts:158-170
Timestamp: 2026-01-05T03:01:39.354Z
Learning: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined. Example: for numeric limits like limitTotalUsd, use 'number | null | undefined' when null signifies explicitly unlimited (e.g., matches DB schema or special UI logic) and undefined signifies 'inherit default'. This pattern should be consistently reflected in type definitions across related fields to preserve semantic clarity between database constraints and UI behavior.
Contributor
Author
There was a problem hiding this comment.
Won't fix in this PR -- scope concern. The existing resetUserAllStatistics function predates this PR and has always run non-transactional deletes. By line 1910, the user was already verified (line 1893), and data is already deleted (lines 1904/1907). The resetUserCostResetAt(userId, null) call is cleanup (clearing the reset marker since all data is wiped). If it fails due to concurrent soft-delete, the primary operation already succeeded. Wrapping in a transaction is a valid improvement but belongs in a separate refactor PR.
There was a problem hiding this comment.
@miraserver, understood – I respect the scope boundary. Since resetUserAllStatistics predates this PR and the transaction wrapping is a broader refactor concern, it makes sense to keep that separate from the costResetAt feature work.
The existing behavior is clear: by the time resetUserCostResetAt(userId, null) runs, the destructive operations have already completed, so it's primarily cleanup. I won't pursue this further in this PR context.
🧠 Learnings used
Learnt from: miraserver
Repo: ding113/claude-code-hub PR: 853
File: src/lib/redis/cost-cache-cleanup.ts:95-100
Timestamp: 2026-03-01T16:26:31.003Z
Learning: In `src/lib/redis/cost-cache-cleanup.ts`, the `clearUserCostCache` function uses a best-effort approach: on `pipeline.exec()` failure, it returns `costKeysDeleted: allCostKeys.length` (planned count) rather than `null` or `0`. This is intentional because: (1) cache cleanup is non-critical; (2) the caller (`resetUserLimitsOnly`) persists `costResetAt` in the DB first, ensuring correctness even if Redis cleanup fails; (3) returning a simple count avoids null-handling overhead in callers. The `warn` log provides observability for partial failures.
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 573
File: src/actions/model-prices.ts:275-335
Timestamp: 2026-01-10T06:19:58.167Z
Learning: Do not modify hardcoded Chinese error messages in Server Actions under src/actions/*.ts as part of piecemeal changes. This is a repo-wide architectural decision that requires a coordinated i18n refactor across all Server Action files (e.g., model-prices.ts, users.ts, system-config.ts). Treat i18n refactor as a separate unified task rather than per-PR changes, and plan a project-wide approach for replacing hardcoded strings with localized resources.
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 539
File: src/types/user.ts:158-170
Timestamp: 2026-01-05T03:01:39.354Z
Learning: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined. Example: for numeric limits like limitTotalUsd, use 'number | null | undefined' when null signifies explicitly unlimited (e.g., matches DB schema or special UI logic) and undefined signifies 'inherit default'. This pattern should be consistently reflected in type definitions across related fields to preserve semantic clarity between database constraints and UI behavior.
Comment on lines
+95
to
+100
| logger.warn("Redis pipeline.exec() failed during cost cache cleanup", { userId, error }); | ||
| return { | ||
| costKeysDeleted: allCostKeys.length, | ||
| activeSessionsDeleted, | ||
| durationMs: Date.now() - startTime, | ||
| }; |
There was a problem hiding this comment.
pipeline.exec() 失败时返回“已删除数量”会误导。
Line [95] 到 Line [100] 捕获异常后仍返回 costKeysDeleted: allCostKeys.length，这表示“计划删除数”而非“实际删除数”。建议失败时返回 null（或 0），避免把失败误报为成功。
建议修复
} catch (error) {
- logger.warn("Redis pipeline.exec() failed during cost cache cleanup", { userId, error });
- return {
- costKeysDeleted: allCostKeys.length,
- activeSessionsDeleted,
- durationMs: Date.now() - startTime,
- };
+ logger.warn("Redis pipeline.exec() failed during cost cache cleanup", {
+ userId,
+ keyIds,
+ includeActiveSessions,
+ error,
+ });
+ return null;
}📝 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
| logger.warn("Redis pipeline.exec() failed during cost cache cleanup", { userId, error }); | |
| return { | |
| costKeysDeleted: allCostKeys.length, | |
| activeSessionsDeleted, | |
| durationMs: Date.now() - startTime, | |
| }; | |
| logger.warn("Redis pipeline.exec() failed during cost cache cleanup", { | |
| userId, | |
| keyIds, | |
| includeActiveSessions, | |
| error, | |
| }); | |
| return null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/redis/cost-cache-cleanup.ts` around lines 95 - 100, The catch block
after pipeline.exec() currently returns costKeysDeleted: allCostKeys.length
which misreports deletions on failure; update the error path in the cost cache
cleanup routine (the catch after pipeline.exec() in
src/lib/redis/cost-cache-cleanup.ts) to return costKeysDeleted: null (or 0 per
your API convention) instead of allCostKeys.length, keep activeSessionsDeleted
and durationMs as before, and ensure any return type/consumer expectations are
adjusted to accept null/0 for costKeysDeleted and downstream code handles the
failure case appropriately.
Contributor
Author
There was a problem hiding this comment.
Acknowledged, but intentional. On pipeline.exec() failure, the function returns costKeysDeleted = planned count (not actual). This is acceptable because: (1) the function is best-effort cache cleanup, not an authoritative data operation; (2) the caller (resetUserLimitsOnly) already has costResetAt set in DB, so even if Redis cleanup partially fails, the next cost query will use correct DB data; (3) returning null would require null-handling in all callers for a non-critical metric. The warn log captures the failure for observability.
There was a problem hiding this comment.
@miraserver, thank you for the detailed explanation! Your reasoning makes sense—since costResetAt is persisted in the DB before cache cleanup, the system remains consistent even if Redis deletion partially fails. Treating the metric as "planned deletions" for a best-effort operation is a pragmatic trade-off that avoids complicating callers with null checks for non-critical telemetry. I've noted this pattern for future reviews.
/)/)
=( •.• )=
(")_(")
*thinking*
✏️ Learnings added
Learnt from: miraserver
Repo: ding113/claude-code-hub PR: 853
File: src/lib/redis/cost-cache-cleanup.ts:95-100
Timestamp: 2026-03-01T16:26:31.003Z
Learning: In `src/lib/redis/cost-cache-cleanup.ts`, the `clearUserCostCache` function uses a best-effort approach: on `pipeline.exec()` failure, it returns `costKeysDeleted: allCostKeys.length` (planned count) rather than `null` or `0`. This is intentional because: (1) cache cleanup is non-critical; (2) the caller (`resetUserLimitsOnly`) persists `costResetAt` in the DB first, ensuring correctness even if Redis cleanup fails; (3) returning a simple count avoids null-handling overhead in callers. The `warn` log provides observability for partial failures.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: NieiR
Repo: ding113/claude-code-hub PR: 539
File: src/types/user.ts:158-170
Timestamp: 2026-01-05T03:01:39.354Z
Learning: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined. Example: for numeric limits like limitTotalUsd, use 'number | null | undefined' when null signifies explicitly unlimited (e.g., matches DB schema or special UI logic) and undefined signifies 'inherit default'. This pattern should be consistently reflected in type definitions across related fields to preserve semantic clarity between database constraints and UI behavior.
Contributor
Author
There was a problem hiding this comment.
Acknowledged, intentional. On pipeline.exec() failure, costKeysDeleted = planned count (not actual). Acceptable because: (1) this is best-effort cache cleanup, not authoritative data; (2) costResetAt is already set in DB, so next cost query uses correct DB data regardless; (3) returning null would require null-handling in all callers for a non-critical metric. The warn log captures the failure.
There was a problem hiding this comment.
Rate Limit Exceeded
@miraserver have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 42 seconds before sending another message.
…With Verify each window (5h/daily/weekly/monthly) is clipped individually instead of checking unordered calledWith matches. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/actions/key-quota-cost-reset.test.ts (1)
124-244: 建议补一条userCostResetAt为非Date真值的回归用例。当前覆盖了
Date与null，但还缺少“真值但非Date（例如字符串）”场景。补这条可直接守住instanceof Date的类型安全回归。可选补丁
@@ test("user without costResetAt (null) -- original time ranges unchanged", async () => { @@ expect(sumKeyTotalCostMock).toHaveBeenCalledWith("sk-test-key-hash", 365, null); }); + + test("userCostResetAt is truthy but not Date -- period starts should not be clipped", async () => { + getSessionMock.mockResolvedValue({ user: { id: 10, role: "user" } }); + getSystemSettingsMock.mockResolvedValue({ currencyDisplay: "USD" }); + dbLimitMock.mockResolvedValue([ + { + key: DEFAULT_KEY_ROW, + userLimitConcurrentSessions: null, + userCostResetAt: "2026-03-01T10:00:00Z" as unknown as Date, + }, + ]); + setupTimeRangeMocks(); + sumKeyCostInTimeRangeMock.mockResolvedValue(1.5); + sumKeyTotalCostMock.mockResolvedValue(10.0); + getKeySessionCountMock.mockResolvedValue(2); + + const { getKeyQuotaUsage } = await import("@/actions/key-quota"); + const result = await getKeyQuotaUsage(42); + + expect(result.ok).toBe(true); + expect(sumKeyCostInTimeRangeMock).toHaveBeenNthCalledWith(1, 42, FIVE_HOURS_AGO, NOW); + expect(sumKeyCostInTimeRangeMock).toHaveBeenNthCalledWith(2, 42, DAILY_START, NOW); + expect(sumKeyCostInTimeRangeMock).toHaveBeenNthCalledWith(3, 42, WEEKLY_START, NOW); + expect(sumKeyCostInTimeRangeMock).toHaveBeenNthCalledWith(4, 42, MONTHLY_START, NOW); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/key-quota-cost-reset.test.ts` around lines 124 - 244, Add a test case that supplies a truthy but non-Date userCostResetAt (e.g. a string) to ensure getKeyQuotaUsage only treats actual Date instances; mock dbLimitMock to return userCostResetAt: "2026-03-01T10:00:00Z" (or call setupDefaultMocks with that value if available), call getKeyQuotaUsage(42) and assert result.ok is true, sumKeyCostInTimeRangeMock was called with the original un-clipped period starts (FIVE_HOURS_AGO, DAILY_START, WEEKLY_START, MONTHLY_START), and sumKeyTotalCostMock was invoked with the third argument being null (or handled as the null-case) — this verifies the instanceof Date check in getKeyQuotaUsage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/actions/key-quota-cost-reset.test.ts`:
- Line 130: The test comment at the top of
tests/unit/actions/key-quota-cost-reset.test.ts is inconsistent with the
assertions: it says "not daily" for costResetAt 2 hours ago, but the test
at/assertion around the daily reset expectation (referencing costResetAt and the
daily clip assertion) actually expects daily to be clipped to costResetAt;
update the comment to accurately describe the behavior being asserted (e.g.,
"should clip 5h range and daily to costResetAt") or, if the intended behavior is
different, change the assertion around costResetAt/daily to match the original
comment; ensure you reference the costResetAt variable and the daily clip
assertion in the test when making the change.
---
Nitpick comments:
In `@tests/unit/actions/key-quota-cost-reset.test.ts`:
- Around line 124-244: Add a test case that supplies a truthy but non-Date
userCostResetAt (e.g. a string) to ensure getKeyQuotaUsage only treats actual
Date instances; mock dbLimitMock to return userCostResetAt:
"2026-03-01T10:00:00Z" (or call setupDefaultMocks with that value if available),
call getKeyQuotaUsage(42) and assert result.ok is true,
sumKeyCostInTimeRangeMock was called with the original un-clipped period starts
(FIVE_HOURS_AGO, DAILY_START, WEEKLY_START, MONTHLY_START), and
sumKeyTotalCostMock was invoked with the third argument being null (or handled
as the null-case) — this verifies the instanceof Date check in getKeyQuotaUsage.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (1)
tests/unit/actions/key-quota-cost-reset.test.ts
| }); | ||
|
|
||
| test("user with costResetAt -- period costs use clipped startTime", async () => { | ||
| // costResetAt is 2 hours ago -- should clip 5h range (7h ago) but not daily (midnight) |
There was a problem hiding this comment.
测试注释与断言不一致，建议修正。
Line 130 注释写的是 “not daily”，但 Line 143 明确断言 daily 也会被裁剪到 costResetAt。建议统一描述，避免误导后续维护者。
建议修改
- // costResetAt is 2 hours ago -- should clip 5h range (7h ago) but not daily (midnight)
+ // costResetAt is 2 hours ago -- 5h/daily/weekly/monthly starts should be clipped to costResetAt📝 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
| // costResetAt is 2 hours ago -- should clip 5h range (7h ago) but not daily (midnight) | |
| // costResetAt is 2 hours ago -- 5h/daily/weekly/monthly starts should be clipped to costResetAt |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/actions/key-quota-cost-reset.test.ts` at line 130, The test
comment at the top of tests/unit/actions/key-quota-cost-reset.test.ts is
inconsistent with the assertions: it says "not daily" for costResetAt 2 hours
ago, but the test at/assertion around the daily reset expectation (referencing
costResetAt and the daily clip assertion) actually expects daily to be clipped
to costResetAt; update the comment to accurately describe the behavior being
asserted (e.g., "should clip 5h range and daily to costResetAt") or, if the
intended behavior is different, change the assertion around costResetAt/daily to
match the original comment; ensure you reference the costResetAt variable and
the daily clip assertion in the test when making the change.
- ja/dashboard.json: replace fullwidth parens with halfwidth - api-key-auth-cache-reset-at.test: override CI env so shouldUseRedisClient() works - key-quota-concurrent-inherit.test: add logger.info mock, sumKeyTotalCost mock, userCostResetAt field - my-usage-concurrent-inherit.test: add logger.info/debug mocks - total-usage-semantics.test: update call assertions for new costResetAt parameter - users-reset-all-statistics.test: mock resetUserCostResetAt, update pipeline.exec error expectations - rate-limit-guard.test: add cost_reset_at: null to expected checkCostLimitsWithLease args Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/unit/actions/users-reset-all-statistics.test.ts (1)
25-31: mock 设置正确，但成功测试用例缺少断言新增的
resetUserCostResetAtMock设置正确，但在"should successfully reset all user statistics"测试（第 128-150 行）中，缺少对resetUserCostResetAtMock调用的断言验证。根据
src/actions/users.ts第 1907 行，操作会调用resetUserCostResetAt(userId, null)来清除costResetAt，测试应验证此调用。♻️ 建议在成功测试中添加断言
// Logging expect(loggerMock.info).toHaveBeenCalled(); + // costResetAt cleared (null) since all data is wiped + expect(resetUserCostResetAtMock).toHaveBeenCalledWith(123, null); });Also applies to: 92-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/users-reset-all-statistics.test.ts` around lines 25 - 31, The success test "should successfully reset all user statistics" is missing an assertion that the repository method was called to clear costResetAt; add an assertion in tests/unit/actions/users-reset-all-statistics.test.ts’s successful case to verify resetUserCostResetAtMock was invoked with the target user's id and null (i.e. resetUserCostResetAt(userId, null)), using the same userId used in the test setup so the mock call is validated.tests/unit/actions/total-usage-semantics.test.ts (1)
233-238: 建议显式断言第 3 个参数，避免“部分匹配通过”。当前只校验了前两参（Line 236-237）。建议补一条对
calls[0][2]的断言（例如toBeUndefined()），让测试在参数透传回归时更敏感。参考修改
const calls = sumUserTotalCostMock.mock.calls; expect(calls.length).toBe(1); expect(calls[0][0]).toBe(1); expect(calls[0][1]).toBe(Infinity); + expect(calls[0][2]).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/actions/total-usage-semantics.test.ts` around lines 233 - 238, The test currently only asserts the first two args of sumUserTotalCostMock.mock.calls which allows partial matches to pass; add an explicit assertion on the third argument (calls[0][2]) — e.g. expect(calls[0][2]).toBeUndefined() — so the test verifies user.costResetAt is passed through correctly; update the test block that references sumUserTotalCostMock.mock.calls and add the calls[0][2] assertion immediately after the existing assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/lib/security/api-key-auth-cache-reset-at.test.ts`:
- Around line 135-157: Rename the test description string to match its
assertions: change the test title used in the test(...) call from "invalid
costResetAt string -- cache entry deleted, returns null" to "invalid costResetAt
string -- graceful fallback to null costResetAt" and update any inline comment
that claims the user should be null to reflect that getCachedUser returns a user
with costResetAt === null; locate the test by the test block containing
cachedPayload, makeUser, redisMock.get.mockResolvedValue, and the assertions
expect(user).not.toBeNull() / expect(user!.costResetAt).toBeNull().
- Around line 27-37: Restore mutated globals after tests: add an afterAll (or
afterEach if preferred) hook that sets process.env back to the saved originalEnv
and restores globalThis.crypto to its original value to avoid cross-test
pollution; locate the test file symbols process.env and globalThis.crypto in
tests/unit/lib/security/api-key-auth-cache-reset-at.test.ts and implement
restoration in an afterAll/afterEach block. Also update the test title at the
case currently described as "returns null" (around the test containing
costResetAt parsing) to a more precise description such as "invalid costResetAt
string -- gracefully parsed to null, user still returned" so the assertion
intent matches behavior.
---
Nitpick comments:
In `@tests/unit/actions/total-usage-semantics.test.ts`:
- Around line 233-238: The test currently only asserts the first two args of
sumUserTotalCostMock.mock.calls which allows partial matches to pass; add an
explicit assertion on the third argument (calls[0][2]) — e.g.
expect(calls[0][2]).toBeUndefined() — so the test verifies user.costResetAt is
passed through correctly; update the test block that references
sumUserTotalCostMock.mock.calls and add the calls[0][2] assertion immediately
after the existing assertions.
In `@tests/unit/actions/users-reset-all-statistics.test.ts`:
- Around line 25-31: The success test "should successfully reset all user
statistics" is missing an assertion that the repository method was called to
clear costResetAt; add an assertion in
tests/unit/actions/users-reset-all-statistics.test.ts’s successful case to
verify resetUserCostResetAtMock was invoked with the target user's id and null
(i.e. resetUserCostResetAt(userId, null)), using the same userId used in the
test setup so the mock call is validated.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (7)
messages/ja/dashboard.jsontests/unit/actions/key-quota-concurrent-inherit.test.tstests/unit/actions/my-usage-concurrent-inherit.test.tstests/unit/actions/total-usage-semantics.test.tstests/unit/actions/users-reset-all-statistics.test.tstests/unit/lib/security/api-key-auth-cache-reset-at.test.tstests/unit/proxy/rate-limit-guard.test.ts
Comment on lines
+27
to
+37
| const originalEnv = process.env; | ||
| beforeEach(() => { | ||
| process.env = { | ||
| ...originalEnv, | ||
| ENABLE_API_KEY_REDIS_CACHE: "true", | ||
| REDIS_URL: "redis://localhost:6379", | ||
| ENABLE_RATE_LIMIT: "true", | ||
| CI: "", | ||
| NEXT_PHASE: "", | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/unit/lib/security/api-key-auth-cache-reset-at.test.tsRepository: ding113/claude-code-hub
Length of output: 8007
全局状态未恢复，会导致跨测试污染。
Lines 27-37 在 beforeEach 中覆写了 process.env，Lines 41-49 在模块顶层覆写了 globalThis.crypto，但文件中不存在 afterEach 或 afterAll 钩子来恢复这些全局对象。这会污染同一 worker 中的其他测试用例。
修复方案：添加 afterAll 钩子恢复原始状态。
参考修改
import { beforeEach, describe, expect, test, vi } from "vitest";
+import { afterAll } from "vitest";
// Mock logger
vi.mock("@/lib/logger", () => ({
@@ -24,6 +25,7 @@ vi.mock("@/lib/redis/client", () => ({
}));
// Enable cache feature via env
const originalEnv = process.env;
+const originalCrypto = globalThis.crypto;
beforeEach(() => {
process.env = {
@@ -48,6 +50,16 @@ Object.defineProperty(globalThis, "crypto", {
configurable: true,
});
+afterAll(() => {
+ process.env = originalEnv;
+ Object.defineProperty(globalThis, "crypto", {
+ value: originalCrypto,
+ writable: true,
+ configurable: true,
+ });
+});
+
// Helper: produce a predictable hex hash from SHA-256 mock
function setupSha256Mock(hexResult = "abc123def456") {
const buffer = new ArrayBuffer(hexResult.length / 2);另外，Line 135 的测试标题描述不够精确。标题说"returns null"，但实际行为是返回 user 对象且 costResetAt 为 null。建议改为："invalid costResetAt string -- gracefully parsed to null, user still returned"。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/lib/security/api-key-auth-cache-reset-at.test.ts` around lines 27
- 37, Restore mutated globals after tests: add an afterAll (or afterEach if
preferred) hook that sets process.env back to the saved originalEnv and restores
globalThis.crypto to its original value to avoid cross-test pollution; locate
the test file symbols process.env and globalThis.crypto in
tests/unit/lib/security/api-key-auth-cache-reset-at.test.ts and implement
restoration in an afterAll/afterEach block. Also update the test title at the
case currently described as "returns null" (around the test containing
costResetAt parsing) to a more precise description such as "invalid costResetAt
string -- gracefully parsed to null, user still returned" so the assertion
intent matches behavior.
Comment on lines
+135
to
+157
| test("invalid costResetAt string -- cache entry deleted, returns null", async () => { | ||
| const cachedPayload = { | ||
| v: 1, | ||
| user: makeUser({ costResetAt: "not-a-date" }), | ||
| }; | ||
| redisMock.get.mockResolvedValue(JSON.stringify(cachedPayload)); | ||
|
|
||
| const { getCachedUser } = await import("@/lib/security/api-key-auth-cache"); | ||
| const user = await getCachedUser(10); | ||
|
|
||
| // hydrateUserFromCache returns null because costResetAt != null but parseOptionalDate returns null | ||
| // BUT: the code path is: costResetAt is not null, parseOptionalDate returns null for invalid string | ||
| // Line 173-174: if (user.costResetAt != null && !costResetAt) return null; | ||
| // Actually, that condition doesn't exist -- let's check the actual behavior | ||
| // Looking at the code: parseOptionalDate("not-a-date") => parseRequiredDate("not-a-date") | ||
| // => new Date("not-a-date") => Invalid Date => return null | ||
| // Then costResetAt is null (from parseOptionalDate) | ||
| // The code does NOT have a null check for costResetAt like expiresAt/deletedAt | ||
| // So the user would still be returned with costResetAt: null | ||
| expect(user).not.toBeNull(); | ||
| // Invalid date parsed to null (graceful degradation) | ||
| expect(user!.costResetAt).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
测试名称与断言语义不一致。
标题写的是“returns null”，但 Line 154 断言 not.toBeNull()；建议改名为“invalid costResetAt string -- graceful fallback to null costResetAt”。
参考修改
- test("invalid costResetAt string -- cache entry deleted, returns null", async () => {
+ test("invalid costResetAt string -- falls back to null costResetAt and returns user", async () => {
@@
- expect(user).not.toBeNull();
- // Invalid date parsed to null (graceful degradation)
+ expect(user).not.toBeNull();
expect(user!.costResetAt).toBeNull();📝 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
| test("invalid costResetAt string -- cache entry deleted, returns null", async () => { | |
| const cachedPayload = { | |
| v: 1, | |
| user: makeUser({ costResetAt: "not-a-date" }), | |
| }; | |
| redisMock.get.mockResolvedValue(JSON.stringify(cachedPayload)); | |
| const { getCachedUser } = await import("@/lib/security/api-key-auth-cache"); | |
| const user = await getCachedUser(10); | |
| // hydrateUserFromCache returns null because costResetAt != null but parseOptionalDate returns null | |
| // BUT: the code path is: costResetAt is not null, parseOptionalDate returns null for invalid string | |
| // Line 173-174: if (user.costResetAt != null && !costResetAt) return null; | |
| // Actually, that condition doesn't exist -- let's check the actual behavior | |
| // Looking at the code: parseOptionalDate("not-a-date") => parseRequiredDate("not-a-date") | |
| // => new Date("not-a-date") => Invalid Date => return null | |
| // Then costResetAt is null (from parseOptionalDate) | |
| // The code does NOT have a null check for costResetAt like expiresAt/deletedAt | |
| // So the user would still be returned with costResetAt: null | |
| expect(user).not.toBeNull(); | |
| // Invalid date parsed to null (graceful degradation) | |
| expect(user!.costResetAt).toBeNull(); | |
| }); | |
| test("invalid costResetAt string -- falls back to null costResetAt and returns user", async () => { | |
| const cachedPayload = { | |
| v: 1, | |
| user: makeUser({ costResetAt: "not-a-date" }), | |
| }; | |
| redisMock.get.mockResolvedValue(JSON.stringify(cachedPayload)); | |
| const { getCachedUser } = await import("@/lib/security/api-key-auth-cache"); | |
| const user = await getCachedUser(10); | |
| // hydrateUserFromCache returns null because costResetAt != null but parseOptionalDate returns null | |
| // BUT: the code path is: costResetAt is not null, parseOptionalDate returns null for invalid string | |
| // Line 173-174: if (user.costResetAt != null && !costResetAt) return null; | |
| // Actually, that condition doesn't exist -- let's check the actual behavior | |
| // Looking at the code: parseOptionalDate("not-a-date") => parseRequiredDate("not-a-date") | |
| // => new Date("not-a-date") => Invalid Date => return null | |
| // Then costResetAt is null (from parseOptionalDate) | |
| // The code does NOT have a null check for costResetAt like expiresAt/deletedAt | |
| // So the user would still be returned with costResetAt: null | |
| expect(user).not.toBeNull(); | |
| expect(user!.costResetAt).toBeNull(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/lib/security/api-key-auth-cache-reset-at.test.ts` around lines 135
- 157, Rename the test description string to match its assertions: change the
test title used in the test(...) call from "invalid costResetAt string -- cache
entry deleted, returns null" to "invalid costResetAt string -- graceful fallback
to null costResetAt" and update any inline comment that claims the user should
be null to reflect that getCachedUser returns a user with costResetAt === null;
locate the test by the test block containing cachedPayload, makeUser,
redisMock.get.mockResolvedValue, and the assertions expect(user).not.toBeNull()
/ expect(user!.costResetAt).toBeNull().
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.
Problem
When an admin needs to reset a user's cost limits (e.g. after billing adjustment or grace period), the only option was full data deletion -- wiping all usage history, statistics, and logs. This is destructive and loses valuable audit data.
Solution
Introduce
costResetAt-- a soft reset mechanism that clips cost calculation windows to a new timestamp instead of deleting data. All historical usage records remain intact for reporting, but limit enforcement starts counting from the reset point.Changes
costResetAtnullable timestamp onuserstable + migration 0078resetUserLimitsOnlyserver action setscostResetAt = now(),clearUserCostCacheflushes Redis cost/lease caches. All cost queries (5h/daily/weekly/monthly/total) across key-quota, my-usage, rate-limit, and lease-service clip their time windows bycostResetAtcostResetAtchecks toinstanceof Datefor type safetyi18n audit
i18n:audit-placeholders-- pre-existing zh-TW same_as_zh-CN only, no new issuesi18n:audit-messages-no-emoji:fail-- cleansettings-split-guards+settings-index-modules-loadtests -- passingTest plan
bun run typecheck-- cleanbun run lint(branch files only) -- cleanGenerated with Claude Code
Greptile Summary
This PR implements a non-destructive "soft reset" mechanism for user cost limits via the
costResetAttimestamp field, solving the problem of having to delete all usage history when resetting limits.Key Changes:
costResetAttimestamp column to users table (migration 0078)resetUserLimitsOnlyaction setscostResetAt = now()and clears Redis cost cache without deleting logs. All cost calculation functions (5h/daily/weekly/monthly/total) across key-quota, my-usage, rate-limit, and lease services now clip their time windows usingclipStart = max(costResetAt, windowStart)instanceof Dateguards throughout (previously reported issues in keys.ts:742 and service.ts have been fixed in da42780)Implementation Highlights:
costResetAtinto individual queries while batching those withoutcostResetAtfrom cached payloadscostResetAti18n Status: Complete translations in en/ja/ru/zh-CN/zh-TW with no new placeholder issues
Confidence Score: 5/5
instanceof Date) throughout, comprehensive test coverage (47+ tests), proper Redis cleanup, complete i18n, and fixes for previously reported issuesImportant Files Changed
cost_reset_attimestamp columnresetUserLimitsOnlyaction with cost clipping and Redis cleanupresetAtparameter with intelligent batch splittingFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Admin clicks Reset Limits] --> B[resetUserLimitsOnly action] B --> C[Set costResetAt = now in DB] C --> D[Clear Redis cost cache] D --> E[Invalidate auth cache] F[API Request with Key] --> G[Rate limit checks] G --> H{costResetAt set?} H -->|Yes| I[Clip time window start to costResetAt] H -->|No| J[Use original time window] I --> K[Query DB for usage in clipped window] J --> K K --> L{Within limit?} L -->|Yes| M[Allow request] L -->|No| N[Reject with 429] O[Cost Query] --> P{User has costResetAt?} P -->|Yes| Q[clipStart = max of resetAt, window start] P -->|No| R[Use window start as-is] Q --> S[Sum costs from clipStart to now] R --> S T[Reset All Statistics] --> U[Delete all logs from DB] U --> V[Clear costResetAt] V --> W[Clear Redis cache + sessions]Last reviewed commit: 823c2a4