feat(keys): add per-key soft quota reset (costResetAt)#929
Conversation
- Add costResetAt column to keys table with migration - Implement resolveKeyCostResetAt() using MAX(key, user) logic - Update all 7 repository SELECT queries + create/update/auth cache - Update rate-limit-guard: all 5 key-level checks use resolved resetAt - Fix service.ts: 365 -> Infinity for total cost queries (6 places) - Update 3 quota display paths (key-quota, keys, my-usage) with resolved resetAt - Split my-usage clipStart into separate key/user ranges - Fix quotas/users/page.tsx batch queries: per-key resolved + Infinity - Add resetKeyLimitsOnly action with Redis cache cleanup - Register POST /api/actions/keys/resetKeyLimitsOnly endpoint - Add reset button with confirmation dialog in edit-key-form - Add i18n translations for 5 languages (zh-CN, zh-TW, en, ja, ru)
📝 WalkthroughWalkthrough本PR为API密钥添加了成本重置功能：数据库 schema 扩展 cost_reset_at、在仓库与类型中暴露 costResetAt、新增 resolveKeyCostResetAt 合并规则、清理单密钥 Redis 缓存、重置密钥限额的后端路由与前端“重置配额”对话框，并在配额/速率限制路径中使用合并后的重置时间。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 |
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 significantly enhances the flexibility of quota management by introducing the ability to perform soft resets on a per-API key basis. Previously, quota resets were only possible at the user level, impacting all associated keys. This change provides more granular control for administrators, allowing them to manage individual key usage more effectively without disrupting other services or users. The implementation includes database schema updates, core logic adjustments, API endpoints, and a user interface for seamless integration. 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
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for resetting soft quotas on a per-key basis, which is a significant improvement over the previous user-level only reset. The implementation is comprehensive, touching the database schema, core rate-limiting logic, repository layer, API, and frontend UI. The addition of the cost_reset_at field on keys and the MAX(key.costResetAt, user.costResetAt) resolution logic are well-implemented. The new admin endpoint and the corresponding UI in the key edit form are also solid additions. I've included one suggestion to improve the user experience on the frontend by avoiding a full page reload after a quota reset.
| } | ||
| toast.success(t("resetLimits.success")); | ||
| setResetLimitsDialogOpen(false); | ||
| window.location.reload(); |
There was a problem hiding this comment.
Using window.location.reload() forces a full page reload, which can be a jarring user experience. It's better to use Next.js's router.refresh() to update the data from the server without losing client-side state. This will update the 'Last reset at' time in the dialog while keeping it open, which is a better user experience.
| window.location.reload(); | |
| router.refresh(); |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| {/* Reset Quota Section - Less destructive (amber) */} | ||
| <section className="rounded-lg border border-muted p-4 space-y-3 mt-6"> | ||
| <h3 className="text-sm font-medium">{t("resetLimits.title")}</h3> | ||
| <div className="rounded-md border border-amber-500/30 bg-amber-500/5 p-3"> | ||
| <div className="flex flex-col gap-2 sm:flex-row sm:items-center sm:justify-between"> | ||
| <div className="space-y-1"> | ||
| <h4 className="text-sm font-medium text-amber-700 dark:text-amber-400"> | ||
| {t("resetLimits.title")} | ||
| </h4> | ||
| <p className="text-xs text-muted-foreground">{t("resetLimits.description")}</p> | ||
| {keyData?.costResetAt && ( | ||
| <p className="text-xs text-amber-600/80 dark:text-amber-400/80"> | ||
| {t("resetLimits.lastResetAt", { | ||
| date: new Intl.DateTimeFormat(locale as string, { | ||
| dateStyle: "medium", | ||
| timeStyle: "short", | ||
| }).format(new Date(keyData.costResetAt)), | ||
| })} | ||
| </p> | ||
| )} | ||
| </div> | ||
|
|
||
| <AlertDialog open={resetLimitsDialogOpen} onOpenChange={setResetLimitsDialogOpen}> | ||
| <AlertDialogTrigger asChild> | ||
| <Button | ||
| type="button" | ||
| variant="outline" | ||
| className="border-amber-500/50 text-amber-700 hover:bg-amber-500/10 dark:text-amber-400 dark:hover:bg-amber-500/10" | ||
| > | ||
| <RotateCcw className="h-4 w-4 mr-2" /> | ||
| {t("resetLimits.button")} | ||
| </Button> | ||
| </AlertDialogTrigger> | ||
| <AlertDialogContent> | ||
| <AlertDialogHeader> | ||
| <AlertDialogTitle>{t("resetLimits.confirmTitle")}</AlertDialogTitle> | ||
| <AlertDialogDescription> | ||
| {t("resetLimits.confirmDescription")} | ||
| </AlertDialogDescription> | ||
| </AlertDialogHeader> | ||
| <AlertDialogFooter> | ||
| <AlertDialogCancel disabled={isResettingLimits}> | ||
| {tCommon("cancel")} | ||
| </AlertDialogCancel> | ||
| <AlertDialogAction | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| handleResetLimitsOnly(); | ||
| }} | ||
| disabled={isResettingLimits} | ||
| className="bg-amber-600 text-white hover:bg-amber-700" | ||
| > | ||
| {isResettingLimits ? ( | ||
| <> | ||
| <Loader2 className="h-4 w-4 mr-2 animate-spin" /> | ||
| {t("resetLimits.loading")} | ||
| </> | ||
| ) : ( | ||
| t("resetLimits.confirm") | ||
| )} | ||
| </AlertDialogAction> | ||
| </AlertDialogFooter> | ||
| </AlertDialogContent> | ||
| </AlertDialog> | ||
| </div> | ||
| </div> | ||
| </section> |
There was a problem hiding this comment.
Reset button visible to non-admin users
The entire "Reset Quota" section (lines 427-493) is rendered unconditionally for all users, but the server action resetKeyLimitsOnly enforces session.user.role !== "admin". Non-admin users will see the reset button, click it, and receive a confusing "Permission Denied" error. This section should be guarded by the isAdmin prop that is already available in this component.
| {/* Reset Quota Section - Less destructive (amber) */} | |
| <section className="rounded-lg border border-muted p-4 space-y-3 mt-6"> | |
| <h3 className="text-sm font-medium">{t("resetLimits.title")}</h3> | |
| <div className="rounded-md border border-amber-500/30 bg-amber-500/5 p-3"> | |
| <div className="flex flex-col gap-2 sm:flex-row sm:items-center sm:justify-between"> | |
| <div className="space-y-1"> | |
| <h4 className="text-sm font-medium text-amber-700 dark:text-amber-400"> | |
| {t("resetLimits.title")} | |
| </h4> | |
| <p className="text-xs text-muted-foreground">{t("resetLimits.description")}</p> | |
| {keyData?.costResetAt && ( | |
| <p className="text-xs text-amber-600/80 dark:text-amber-400/80"> | |
| {t("resetLimits.lastResetAt", { | |
| date: new Intl.DateTimeFormat(locale as string, { | |
| dateStyle: "medium", | |
| timeStyle: "short", | |
| }).format(new Date(keyData.costResetAt)), | |
| })} | |
| </p> | |
| )} | |
| </div> | |
| <AlertDialog open={resetLimitsDialogOpen} onOpenChange={setResetLimitsDialogOpen}> | |
| <AlertDialogTrigger asChild> | |
| <Button | |
| type="button" | |
| variant="outline" | |
| className="border-amber-500/50 text-amber-700 hover:bg-amber-500/10 dark:text-amber-400 dark:hover:bg-amber-500/10" | |
| > | |
| <RotateCcw className="h-4 w-4 mr-2" /> | |
| {t("resetLimits.button")} | |
| </Button> | |
| </AlertDialogTrigger> | |
| <AlertDialogContent> | |
| <AlertDialogHeader> | |
| <AlertDialogTitle>{t("resetLimits.confirmTitle")}</AlertDialogTitle> | |
| <AlertDialogDescription> | |
| {t("resetLimits.confirmDescription")} | |
| </AlertDialogDescription> | |
| </AlertDialogHeader> | |
| <AlertDialogFooter> | |
| <AlertDialogCancel disabled={isResettingLimits}> | |
| {tCommon("cancel")} | |
| </AlertDialogCancel> | |
| <AlertDialogAction | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| handleResetLimitsOnly(); | |
| }} | |
| disabled={isResettingLimits} | |
| className="bg-amber-600 text-white hover:bg-amber-700" | |
| > | |
| {isResettingLimits ? ( | |
| <> | |
| <Loader2 className="h-4 w-4 mr-2 animate-spin" /> | |
| {t("resetLimits.loading")} | |
| </> | |
| ) : ( | |
| t("resetLimits.confirm") | |
| )} | |
| </AlertDialogAction> | |
| </AlertDialogFooter> | |
| </AlertDialogContent> | |
| </AlertDialog> | |
| </div> | |
| </div> | |
| </section> | |
| {/* Reset Quota Section - Admin only, less destructive (amber) */} | |
| {isAdmin && ( | |
| <section className="rounded-lg border border-muted p-4 space-y-3 mt-6"> | |
| <h3 className="text-sm font-medium">{t("resetLimits.title")}</h3> | |
| <div className="rounded-md border border-amber-500/30 bg-amber-500/5 p-3"> | |
| <div className="flex flex-col gap-2 sm:flex-row sm:items-center sm:justify-between"> | |
| <div className="space-y-1"> | |
| <h4 className="text-sm font-medium text-amber-700 dark:text-amber-400"> | |
| {t("resetLimits.title")} | |
| </h4> | |
| <p className="text-xs text-muted-foreground">{t("resetLimits.description")}</p> | |
| {keyData?.costResetAt && ( | |
| <p className="text-xs text-amber-600/80 dark:text-amber-400/80"> | |
| {t("resetLimits.lastResetAt", { | |
| date: new Intl.DateTimeFormat(locale as string, { | |
| dateStyle: "medium", | |
| timeStyle: "short", | |
| }).format(new Date(keyData.costResetAt)), | |
| })} | |
| </p> | |
| )} | |
| </div> | |
| <AlertDialog open={resetLimitsDialogOpen} onOpenChange={setResetLimitsDialogOpen}> | |
| <AlertDialogTrigger asChild> | |
| <Button | |
| type="button" | |
| variant="outline" | |
| className="border-amber-500/50 text-amber-700 hover:bg-amber-500/10 dark:text-amber-400 dark:hover:bg-amber-500/10" | |
| > | |
| <RotateCcw className="h-4 w-4 mr-2" /> | |
| {t("resetLimits.button")} | |
| </Button> | |
| </AlertDialogTrigger> | |
| <AlertDialogContent> | |
| <AlertDialogHeader> | |
| <AlertDialogTitle>{t("resetLimits.confirmTitle")}</AlertDialogTitle> | |
| <AlertDialogDescription> | |
| {t("resetLimits.confirmDescription")} | |
| </AlertDialogDescription> | |
| </AlertDialogHeader> | |
| <AlertDialogFooter> | |
| <AlertDialogCancel disabled={isResettingLimits}> | |
| {tCommon("cancel")} | |
| </AlertDialogCancel> | |
| <AlertDialogAction | |
| onClick={(e) => { | |
| e.preventDefault(); | |
| handleResetLimitsOnly(); | |
| }} | |
| disabled={isResettingLimits} | |
| className="bg-amber-600 text-white hover:bg-amber-700" | |
| > | |
| {isResettingLimits ? ( | |
| <> | |
| <Loader2 className="h-4 w-4 mr-2 animate-spin" /> | |
| {t("resetLimits.loading")} | |
| </> | |
| ) : ( | |
| t("resetLimits.confirm") | |
| )} | |
| </AlertDialogAction> | |
| </AlertDialogFooter> | |
| </AlertDialogContent> | |
| </AlertDialog> | |
| </div> | |
| </div> | |
| </section> | |
| )} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/dashboard/_components/user/forms/edit-key-form.tsx
Line: 427-493
Comment:
**Reset button visible to non-admin users**
The entire "Reset Quota" section (lines 427-493) is rendered unconditionally for all users, but the server action `resetKeyLimitsOnly` enforces `session.user.role !== "admin"`. Non-admin users will see the reset button, click it, and receive a confusing "Permission Denied" error. This section should be guarded by the `isAdmin` prop that is already available in this component.
```suggestion
{/* Reset Quota Section - Admin only, less destructive (amber) */}
{isAdmin && (
<section className="rounded-lg border border-muted p-4 space-y-3 mt-6">
<h3 className="text-sm font-medium">{t("resetLimits.title")}</h3>
<div className="rounded-md border border-amber-500/30 bg-amber-500/5 p-3">
<div className="flex flex-col gap-2 sm:flex-row sm:items-center sm:justify-between">
<div className="space-y-1">
<h4 className="text-sm font-medium text-amber-700 dark:text-amber-400">
{t("resetLimits.title")}
</h4>
<p className="text-xs text-muted-foreground">{t("resetLimits.description")}</p>
{keyData?.costResetAt && (
<p className="text-xs text-amber-600/80 dark:text-amber-400/80">
{t("resetLimits.lastResetAt", {
date: new Intl.DateTimeFormat(locale as string, {
dateStyle: "medium",
timeStyle: "short",
}).format(new Date(keyData.costResetAt)),
})}
</p>
)}
</div>
<AlertDialog open={resetLimitsDialogOpen} onOpenChange={setResetLimitsDialogOpen}>
<AlertDialogTrigger asChild>
<Button
type="button"
variant="outline"
className="border-amber-500/50 text-amber-700 hover:bg-amber-500/10 dark:text-amber-400 dark:hover:bg-amber-500/10"
>
<RotateCcw className="h-4 w-4 mr-2" />
{t("resetLimits.button")}
</Button>
</AlertDialogTrigger>
<AlertDialogContent>
<AlertDialogHeader>
<AlertDialogTitle>{t("resetLimits.confirmTitle")}</AlertDialogTitle>
<AlertDialogDescription>
{t("resetLimits.confirmDescription")}
</AlertDialogDescription>
</AlertDialogHeader>
<AlertDialogFooter>
<AlertDialogCancel disabled={isResettingLimits}>
{tCommon("cancel")}
</AlertDialogCancel>
<AlertDialogAction
onClick={(e) => {
e.preventDefault();
handleResetLimitsOnly();
}}
disabled={isResettingLimits}
className="bg-amber-600 text-white hover:bg-amber-700"
>
{isResettingLimits ? (
<>
<Loader2 className="h-4 w-4 mr-2 animate-spin" />
{t("resetLimits.loading")}
</>
) : (
t("resetLimits.confirm")
)}
</AlertDialogAction>
</AlertDialogFooter>
</AlertDialogContent>
</AlertDialog>
</div>
</div>
</section>
)}
```
How can I resolve this? If you propose a fix, please make it concise.![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR adds per-key soft quota reset functionality (costResetAt) with a well-structured implementation across database, repository, service layer, and UI.
PR Size: XL
- Lines changed: 4,567 (4,514 additions + 53 deletions)
- Files changed: 27
Suggestion: Given the scope of changes, consider splitting into smaller PRs for future features of this size:
- Core logic (schema, repository, utility functions)
- Rate limit integration and bug fix
- Frontend UI changes
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 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Medium Priority Issues (Consider Fixing)
[TEST-MISSING-COVERAGE] No tests for new utility and action functions
The following new functions lack test coverage:
- `resolveKeyCostResetAt()` in `src/lib/rate-limit/cost-reset-utils.ts`
- `resetKeyLimitsOnly()` action in `src/actions/keys.ts`
- `clearSingleKeyCostCache()` in `src/lib/redis/cost-cache-cleanup.ts`
While these functions are relatively simple and follow established patterns, having test coverage would improve maintainability and catch regressions early.
Suggested test cases:
```typescript
// Test resolveKeyCostResetAt
describe("resolveKeyCostResetAt", () => {
it("returns null when both are null", () => {...});
it("returns key reset when user reset is null", () => {...});
it("returns user reset when key reset is null", () => {...});
it("returns max of key and user reset", () => {...});
});
```
Review Coverage
- Logic and correctness - Clean implementation of MAX(key.costResetAt, user.costResetAt) logic
- Security (OWASP Top 10) - Admin-only endpoint with proper session validation
- Error handling - Proper try/catch with logging, graceful degradation on Redis failures
- Type safety - Proper type definitions and transformers
- Documentation accuracy - PR description matches implementation
- Test coverage - Minor gaps noted above
- Code clarity - Clean separation of concerns with new utility file
Notable Improvements
- Bug fix: Changed `365` to `Infinity` for total cost queries to prevent unintended 365-day expiry on total limits
- Clean architecture with new `cost-reset-utils.ts` utility file
- Proper i18n coverage for all 5 languages
Automated review by Claude AI
![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/key.ts (1)
221-250:⚠️ Potential issue | 🟠 Major将
cost_reset_at从通用UpdateKeyData类型中移除，防止非 admin 路径可能的绕过。虽然当前调用方都没有通过
updateKey传递cost_reset_at，但该字段已被加入通用UpdateKeyData类型（src/types/key.ts:84）并在updateKey中被无条件处理（src/repository/key.ts:245）。这种结构上的开放性会增加未来被误用的风险——如果有新的非管理员路径使用updateKey并传入该字段，就会绕过专用的resetKeyLimitsOnly流程。建议将
cost_reset_at从UpdateKeyData中删除，保留在CreateKeyData中；所有 cost 重置操作专门通过resetKeyCostResetAt进行，该函数已正确实现了缓存失效（src/repository/key.ts:468的invalidateCachedKey调用）。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/key.ts` around lines 221 - 250, Remove cost_reset_at from the generic UpdateKeyData type and stop handling it in the updateKey code path: update the types so CreateKeyData keeps cost_reset_at but UpdateKeyData does not, and remove the db mapping for costResetAt in the updateKey logic (the block that sets dbData.costResetAt). Ensure all cost-reset changes go through resetKeyCostResetAt (which calls invalidateCachedKey) and that resetKeyLimitsOnly remains the admin-only path for limit resets.
🧹 Nitpick comments (2)
src/app/[locale]/dashboard/_components/user/forms/edit-key-form.tsx (2)
100-102: 可选优化：考虑使用router.refresh()替代window.location.reload()。当前使用完整页面重载来刷新数据。可以考虑使用更轻量的方式：
router.refresh(); queryClient.invalidateQueries();这与表单提交成功后的刷新方式（第173行）保持一致。不过，由于
costResetAt可能影响多个组件，完整重载也是可接受的保守选择。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/user/forms/edit-key-form.tsx around lines 100 - 102, Replace the full-page reload after successful reset (where toast.success and setResetLimitsDialogOpen are called) with a lighter client-side refresh: call router.refresh() and optionally queryClient.invalidateQueries() to refresh affected data instead of window.location.reload(); update the code path that currently calls window.location.reload() to invoke router.refresh() (and invalidate queries for keys related to costResetAt or user limits) so the behavior matches the other successful-submit path.
427-493: 建议：考虑为重置限额按钮添加管理员权限检查。重置限额区块目前对所有用户可见，但
resetKeyLimitsOnly服务端操作仅限管理员使用。虽然服务端会正确拒绝非管理员请求，但在 UI 层隐藏该区块可以提供更好的用户体验。♻️ 建议的修改
- {/* Reset Quota Section - Less destructive (amber) */} - <section className="rounded-lg border border-muted p-4 space-y-3 mt-6"> + {/* Reset Quota Section - Less destructive (amber) - Admin only */} + {isAdmin && ( + <section className="rounded-lg border border-muted p-4 space-y-3 mt-6"> ... - </section> + </section> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/_components/user/forms/edit-key-form.tsx around lines 427 - 493, The Reset Quota section is shown to all users but the server action (resetKeyLimitsOnly) is admin-only; update the UI to hide/disable this section for non-admins by checking the admin flag (e.g., keyData.isAdmin, currentUser.isAdmin, or the relevant auth/context value) before rendering the section (or the AlertDialogTrigger/Button). Specifically, wrap or gate the entire section that uses resetLimitsDialogOpen, AlertDialog, Button and the handler handleResetLimitsOnly so it only renders for admins (or render a disabled state with a tooltip) to prevent exposing the action to non-admin users.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@drizzle/0083_aromatic_wolverine.sql`:
- Line 1: 删除手写迁移文件 0083_aromatic_wolverine.sql（包含 ALTER TABLE "keys" ADD COLUMN
"cost_reset_at" ...）并在已更新 src/drizzle/schema.ts（在 keys 表上添加 cost_reset_at
字段）后运行生成命令：bun run db:generate，以由工具生成规范的迁移文件；确保不要将手写 SQL 提交，验证新生成的迁移包含对
"cost_reset_at" 的变更并通过现有迁移测试/校验流程。
In `@messages/zh-TW/quota.json`:
- Line 332: Update the Traditional Chinese copy for the "confirmDescription"
entry in messages/zh-TW/quota.json by replacing the Simplified character "每周"
with the Traditional variant "每週" so the string reads with consistent zh-TW
wording; locate the "confirmDescription" key and change that substring only.
In `@src/app/api/actions/`[...route]/route.ts:
- Around line 353-354: The route metadata fields description and summary in
route.ts are hardcoded; replace them with i18n lookups: create corresponding
i18n keys (e.g. "route.resetKeyQuota.summary" and
"route.resetKeyQuota.description") in the locale resource files for zh-CN,
zh-TW, en, ja, ru, then use the project's i18n accessor (e.g. i18n.t or similar
used elsewhere) to populate summary and description instead of literal strings
so the route uses localized text at runtime.
In `@src/repository/key.ts`:
- Around line 460-472: The function resetKeyCostResetAt currently treats DB
update success as overall success while silently swallowing invalidateCachedKey
failures; change it so cache invalidation errors are observable: after the DB
update in resetKeyCostResetAt, await invalidateCachedKey(result[0].key) and if
it throws, log the error with context (include keyId and key) and rethrow or
return a distinct failure (do not .catch(() => {})). This makes callers see a
partial-failure (DB updated but cache stale) so they can handle or retry;
reference the functions resetKeyCostResetAt and invalidateCachedKey when making
the change.
---
Outside diff comments:
In `@src/repository/key.ts`:
- Around line 221-250: Remove cost_reset_at from the generic UpdateKeyData type
and stop handling it in the updateKey code path: update the types so
CreateKeyData keeps cost_reset_at but UpdateKeyData does not, and remove the db
mapping for costResetAt in the updateKey logic (the block that sets
dbData.costResetAt). Ensure all cost-reset changes go through
resetKeyCostResetAt (which calls invalidateCachedKey) and that
resetKeyLimitsOnly remains the admin-only path for limit resets.
---
Nitpick comments:
In `@src/app/`[locale]/dashboard/_components/user/forms/edit-key-form.tsx:
- Around line 100-102: Replace the full-page reload after successful reset
(where toast.success and setResetLimitsDialogOpen are called) with a lighter
client-side refresh: call router.refresh() and optionally
queryClient.invalidateQueries() to refresh affected data instead of
window.location.reload(); update the code path that currently calls
window.location.reload() to invoke router.refresh() (and invalidate queries for
keys related to costResetAt or user limits) so the behavior matches the other
successful-submit path.
- Around line 427-493: The Reset Quota section is shown to all users but the
server action (resetKeyLimitsOnly) is admin-only; update the UI to hide/disable
this section for non-admins by checking the admin flag (e.g., keyData.isAdmin,
currentUser.isAdmin, or the relevant auth/context value) before rendering the
section (or the AlertDialogTrigger/Button). Specifically, wrap or gate the
entire section that uses resetLimitsDialogOpen, AlertDialog, Button and the
handler handleResetLimitsOnly so it only renders for admins (or render a
disabled state with a tooltip) to prevent exposing the action to non-admin
users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dabe3f34-2bf8-45f4-8255-46077478e00e
📒 Files selected for processing (27)
drizzle/0083_aromatic_wolverine.sqldrizzle/meta/0083_snapshot.jsonmessages/en/quota.jsonmessages/ja/quota.jsonmessages/ru/quota.jsonmessages/zh-CN/quota.jsonmessages/zh-TW/quota.jsonsrc/actions/key-quota.tssrc/actions/keys.tssrc/actions/my-usage.tssrc/actions/users.tssrc/app/[locale]/dashboard/_components/user/edit-key-dialog.tsxsrc/app/[locale]/dashboard/_components/user/forms/edit-key-form.tsxsrc/app/[locale]/dashboard/_components/user/user-key-table-row.tsxsrc/app/[locale]/dashboard/quotas/users/page.tsxsrc/app/api/actions/[...route]/route.tssrc/app/v1/_lib/proxy/rate-limit-guard.tssrc/drizzle/schema.tssrc/lib/rate-limit/cost-reset-utils.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/types/key.tssrc/types/user.tstests/integration/usage-ledger.test.ts
| @@ -0,0 +1 @@ | |||
| ALTER TABLE "keys" ADD COLUMN "cost_reset_at" timestamp with time zone; No newline at end of file | |||
There was a problem hiding this comment.
请移除手写迁移并改为自动生成。
当前是手写 SQL 迁移，违反仓库迁移流程约束。建议删除该文件后，在已更新 src/drizzle/schema.ts 的前提下执行 bun run db:generate 重新产出迁移。
As per coding guidelines: "Modify database schema in src/drizzle/schema.ts, then run bun run db:generate to generate migrations. Never create SQL migration files manually".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@drizzle/0083_aromatic_wolverine.sql` at line 1, 删除手写迁移文件
0083_aromatic_wolverine.sql（包含 ALTER TABLE "keys" ADD COLUMN "cost_reset_at"
...）并在已更新 src/drizzle/schema.ts（在 keys 表上添加 cost_reset_at 字段）后运行生成命令：bun run
db:generate，以由工具生成规范的迁移文件；确保不要将手写 SQL 提交，验证新生成的迁移包含对 "cost_reset_at"
的变更并通过现有迁移测试/校验流程。
| description: "仅重置单个密钥的消费限额累计（不删除日志）", | ||
| summary: "重置密钥限额累计", |
There was a problem hiding this comment.
新增路由文案仍为硬编码，未接入 i18n。
Line 353-354 的 description/summary 是直接字符串；按仓库规则，这类用户可见文案应走多语言资源。建议改为统一的 i18n key 映射（至少覆盖 zh-CN、zh-TW、en、ja、ru）。
As per coding guidelines "All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/api/actions/`[...route]/route.ts around lines 353 - 354, The route
metadata fields description and summary in route.ts are hardcoded; replace them
with i18n lookups: create corresponding i18n keys (e.g.
"route.resetKeyQuota.summary" and "route.resetKeyQuota.description") in the
locale resource files for zh-CN, zh-TW, en, ja, ru, then use the project's i18n
accessor (e.g. i18n.t or similar used elsewhere) to populate summary and
description instead of literal strings so the route uses localized text at
runtime.
| export async function resetKeyCostResetAt(keyId: number, resetAt: Date | null): Promise<boolean> { | ||
| const result = await db | ||
| .update(keys) | ||
| .set({ costResetAt: resetAt, updatedAt: new Date() }) | ||
| .where(and(eq(keys.id, keyId), isNull(keys.deletedAt))) | ||
| .returning({ id: keys.id, key: keys.key }); | ||
|
|
||
| if (result.length > 0) { | ||
| await invalidateCachedKey(result[0].key).catch(() => {}); | ||
| } | ||
|
|
||
| return result.length > 0; | ||
| } |
There was a problem hiding this comment.
把 reset 做成完整且可观测的操作。
这里返回成功的前提只是 DB 更新成功，但 reset 语义还依赖缓存侧同步。现在鉴权缓存失效被静默吞掉；一旦 Redis 删除失败，后续请求仍可能命中旧 key 缓存并继续使用旧 costResetAt，最终表现成“重置成功但限额还没重置”。至少要把这类失败显式记录/上抛为 partial failure，而不是无声忽略。
Based on learnings: In src/lib/redis/cost-cache-cleanup.ts, best-effort cleanup is acceptable only because the DB reset already preserves correctness; that rationale does not hold for stale auth-cache entries carrying an old costResetAt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/repository/key.ts` around lines 460 - 472, The function
resetKeyCostResetAt currently treats DB update success as overall success while
silently swallowing invalidateCachedKey failures; change it so cache
invalidation errors are observable: after the DB update in resetKeyCostResetAt,
await invalidateCachedKey(result[0].key) and if it throws, log the error with
context (include keyId and key) and rethrow or return a distinct failure (do not
.catch(() => {})). This makes callers see a partial-failure (DB updated but
cache stale) so they can handle or retry; reference the functions
resetKeyCostResetAt and invalidateCachedKey when making the change.
|
感谢这个 PR！整体方向我觉得很好：引入 这边 review 下来有几个点想提醒一下（语气偏建议，供参考）：
整体上改动逻辑我觉得是对的，上面主要是把 CI 阻断点和一些可选优化点提出来，方便你快速收敛合并。 |
- Register migration 0083 in _journal.json (fixes CI AUTO_MIGRATE) - Add trailing newline to migration SQL file - Guard reset quota section with isAdmin check - Replace window.location.reload() with router.refresh() - Fix zh-TW: use traditional '每週' instead of simplified '每周'
5ce72eb to
a25cd69
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/actions/keys.ts (1)
823-892: 建议补一条重置限额回归测试。建议加 API/E2E 用例：先制造 key usage，再调用
resetKeyLimitsOnly，断言 key 维度的 5h/日/周/月/total 被正确 clip，而 user 维度累计不被错误重置。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/keys.ts` around lines 823 - 892, 为 resetKeyLimitsOnly 增加回归测试：在测试中用 makeKey / createKeyUsage 或相应 helper 先制造对指定 key 的累计使用（覆盖 5h/日/周/月/total 边界），然后调用 resetKeyLimitsOnly(keyId) 并断言该 key 的限额相关字段（costResetAt 触发后对 key 维度的 5 小时、日、周、月、total 等累计值应被正确 clip/重置或回退到期望范围），同时断言同一用户下的 user 维度累计（userId 相关的累计/配额记录）没有被错误重置；测试应覆盖成功清理 Redis cache 的路径（mock/spy clearSingleKeyCostCache）及 cache 清理失败的错误分支以验证日志/错误处理。
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/actions/keys.ts`:
- Around line 823-892: 为 resetKeyLimitsOnly 增加回归测试：在测试中用 makeKey /
createKeyUsage 或相应 helper 先制造对指定 key 的累计使用（覆盖 5h/日/周/月/total 边界），然后调用
resetKeyLimitsOnly(keyId) 并断言该 key 的限额相关字段（costResetAt 触发后对 key 维度的 5
小时、日、周、月、total 等累计值应被正确 clip/重置或回退到期望范围），同时断言同一用户下的 user 维度累计（userId
相关的累计/配额记录）没有被错误重置；测试应覆盖成功清理 Redis cache 的路径（mock/spy clearSingleKeyCostCache）及
cache 清理失败的错误分支以验证日志/错误处理。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e60ade05-a4ee-4873-839e-b76c3ce3b0bc
📒 Files selected for processing (7)
drizzle/0083_aromatic_wolverine.sqldrizzle/meta/_journal.jsonmessages/zh-TW/quota.jsonsrc/actions/keys.tssrc/app/[locale]/dashboard/_components/user/forms/edit-key-form.tsxsrc/repository/_shared/transformers.tstests/integration/usage-ledger.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- drizzle/0083_aromatic_wolverine.sql
- tests/integration/usage-ledger.test.ts
| current = await sumKeyTotalCost(options.keyHash, Infinity, options?.resetAt); | ||
| } else if (entityType === "user") { | ||
| current = await sumUserTotalCost(entityId, 365, options?.resetAt); | ||
| current = await sumUserTotalCost(entityId, Infinity, options?.resetAt); |
There was a problem hiding this comment.
Behavior change: 365 → Infinity for total cost queries
All six sumKeyTotalCost / sumUserTotalCost calls previously passed 365 (days) as the look-back window, effectively capping total-cost accumulation at one year. Switching to Infinity is the correct fix for a "lifetime total" limit, but it is a breaking behavior change for existing deployments: any user or key with a limitTotalUsd who has more than 365 days of history will now see their total shoot up and may unexpectedly hit—or appear to already be over—their limit on the first request after upgrading.
Consider communicating this change in release notes or adding a migration note so operators can adjust their limits proactively.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/rate-limit/service.ts
Line: 379-381
Comment:
**Behavior change: `365` → `Infinity` for total cost queries**
All six `sumKeyTotalCost` / `sumUserTotalCost` calls previously passed `365` (days) as the look-back window, effectively capping total-cost accumulation at one year. Switching to `Infinity` is the correct fix for a "lifetime total" limit, but it is a **breaking behavior change for existing deployments**: any user or key with a `limitTotalUsd` who has more than 365 days of history will now see their total shoot up and may unexpectedly hit—or appear to already be over—their limit on the first request after upgrading.
Consider communicating this change in release notes or adding a migration note so operators can adjust their limits proactively.
How can I resolve this? If you propose a fix, please make it concise.| const updated = await resetKeyCostResetAt(keyId, new Date()); | ||
| if (!updated) { | ||
| return { | ||
| ok: false, | ||
| error: tError("KEY_NOT_FOUND"), | ||
| errorCode: ERROR_CODES.NOT_FOUND, | ||
| }; | ||
| } | ||
|
|
||
| try { | ||
| const { clearSingleKeyCostCache } = await import("@/lib/redis/cost-cache-cleanup"); | ||
| const cacheResult = await clearSingleKeyCostCache({ | ||
| keyId, | ||
| keyHash: key.key, | ||
| }); | ||
| if (cacheResult) { | ||
| logger.info("Reset key limits only - Redis cost cache cleared", { | ||
| keyId, | ||
| userId: key.userId, | ||
| ...cacheResult, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.error("Failed to clear Redis cache during key limits reset", { | ||
| keyId, | ||
| userId: key.userId, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Redis cost-cache cleared after DB commit, not atomically
The reset flow is:
resetKeyCostResetAt(keyId, new Date())— commitscostResetAt = NOWto Postgres and invalidates the auth Redis cache.- (Separately)
clearSingleKeyCostCache(...)— scans and deletes rate-limit Redis keys.
Between steps 1 and 2 there is a window (typically a few milliseconds, but visible under load) during which a fresh request can read the updated costResetAt from the just-invalidated auth cache miss (DB read) and attempt rate-limit checks against the un-cleared Redis period counters. This means the rate-limiter may briefly see the new costResetAt clipping the time window while the underlying Redis counter still reflects the pre-reset accumulated value.
The PR description acknowledges "In-flight requests started before reset may still write costs after reset", but this is a slightly different scenario: it can also allow a request arriving after the reset to be incorrectly rate-limited by the old counter for a short window.
This is a minor eventual-consistency gap that is probably acceptable for a soft quota reset, but it should be documented with a comment near the clearSingleKeyCostCache call so future maintainers understand the design choice.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/keys.ts
Line: 845-873
Comment:
**Redis cost-cache cleared after DB commit, not atomically**
The reset flow is:
1. `resetKeyCostResetAt(keyId, new Date())` — commits `costResetAt = NOW` to Postgres and invalidates the auth Redis cache.
2. (Separately) `clearSingleKeyCostCache(...)` — scans and deletes rate-limit Redis keys.
Between steps 1 and 2 there is a window (typically a few milliseconds, but visible under load) during which a fresh request can read the updated `costResetAt` from the just-invalidated auth cache miss (DB read) and attempt rate-limit checks against the **un-cleared** Redis period counters. This means the rate-limiter may briefly see the new `costResetAt` clipping the time window while the underlying Redis counter still reflects the pre-reset accumulated value.
The PR description acknowledges "In-flight requests started before reset may still write costs after reset", but this is a slightly different scenario: it can also allow a request arriving *after* the reset to be incorrectly rate-limited by the old counter for a short window.
This is a minor eventual-consistency gap that is probably acceptable for a soft quota reset, but it should be documented with a comment near the `clearSingleKeyCostCache` call so future maintainers understand the design choice.
How can I resolve this? If you propose a fix, please make it concise.
github-project-automation
Bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
3 participants
Summary
costResetAtcolumn for soft quota reset without affecting other keys under the same userMAX(key.costResetAt, user.costResetAt)logic so whichever reset is more recent takes effectPOST /api/actions/keys/resetKeyLimitsOnlyadmin-only endpointMotivation
Previously, resetting quota limits was only possible at the user level (
resetUserLimitsOnly), which reset ALL keys at once. Admins needed the ability to reset a single API key's accumulated usage independently.Related
costResetAtfeature to individual API keyscostResetAt(affects both user-level and key-level resets)Design
Soft reset only - sets
key.costResetAt = NOW(), never deletes logs or data.MAX logic -
resolveKeyCostResetAt(keyCRA, userCRA)returns the more recent of key-level and user-level reset timestamps. Whenkey.costResetAtis null (never reset), behavior is identical to the old code path.Changes
Database
cost_reset_atcolumn tokeystable (migration0083_aromatic_wolverine.sql)Core Logic
resolveKeyCostResetAt()utility insrc/lib/rate-limit/cost-reset-utils.tsrate-limit-guard.ts: all 5 key-level checks (total/5h/daily/weekly/monthly) use resolved resetAtservice.ts: fix365->Infinityfor total cost queries (6 places) - prevents 365-day expiry on total limitsRepository
key.ts+createKey/updateKeyincludecostResetAtresetKeyCostResetAt()function with auth cache invalidationtransformers.ts:toKey()hydratescostResetAtapi-key-auth-cache.ts: cache hydration includescostResetAtQuota Display (3 paths)
key-quota.ts: uses resolved resetAtkeys.ts(getKeyLimitUsage): uses resolved resetAtmy-usage.ts: splits into separatekeyClipStart/userClipStartwith independent clipped rangesquotas/users/page.tsx: per-keyresolveKeyCostResetAt+Infinityfor batch queriesReset Action + API
resetKeyLimitsOnly(keyId)inkeys.ts- admin-only, sets costResetAt, clears Redis cacheclearSingleKeyCostCache()incost-cache-cleanup.ts(4 Redis patterns)POST /api/actions/keys/resetKeyLimitsOnlyendpointFrontend
AlertDialoginedit-key-form.tsxedit-key-dialog.tsx,user-key-table-row.tsxKnown Limitations
cost-alert.tsdoes not considercostResetAt- this is a pre-existing issue affecting user-level reset too (see cost-alert.ts: missing costResetAt clipping and limitTotalUsd/limitDailyUsd checks #927)Description enhanced by Claude AI
Greptile Summary
This PR adds per-key soft quota reset (
costResetAt) so admins can reset a single API key's accumulated cost window without affecting other keys under the same user. The coreresolveKeyCostResetAt(key.costResetAt, user.costResetAt)utility correctly returns the more-recent of the two timestamps, ensuring the user-level reset still acts as a floor.Key changes:
cost_reset_atcolumn added tokeystable (migration0083).rate-limit-guard.tsuse the resolvedcostResetAt; user-level checks are unaffected, preserving isolation.365 → Infinity: SixsumKeyTotalCost/sumUserTotalCostcalls inservice.tspreviously used a 365-day look-back for "total" limits. Changing toInfinityis correct but is a breaking behavior change for instances older than one year — existing users withlimitTotalUsdset will see their measured usage increase on upgrade.resetKeyLimitsOnlyis admin-only, commitscostResetAt = NOW, invalidates the auth Redis cache, then clears key-scoped cost/lease Redis counters viaclearSingleKeyCostCache. There is a brief eventual-consistency window between the DB commit and the Redis counter sweep.isAdminprop after addressing the previous review thread. Minor cosmetic issue: both<h3>and<h4>in the reset section use the sameresetLimits.titletranslation key, rendering the title twice.my-usage.ts,key-quota.ts,quotas/users/page.tsx): All three display paths correctly apply the resolvedcostResetAtas the window clip start.Confidence Score: 4/5
365 → Infinitychange being a silent breaking change for long-running deployments that should be communicated in release notes, (2) the duplicate title in the reset UI, and (3) the missing fail-open comment in the key auth cache hydration function.src/lib/rate-limit/service.ts(365→Infinity behavior change),src/app/[locale]/dashboard/_components/user/forms/edit-key-form.tsx(duplicate title)Important Files Changed
365days toInfinity. Correct fix for lifetime limits but is a breaking behavior change for deployments older than one year; needs release note.resolveKeyCostResetAtforcost_reset_atparameter; user-level checks correctly continue to use onlyuser.costResetAt.resetKeyLimitsOnlyaction is admin-gated and correctly chains DB update → auth-cache invalidation → Redis cost-cache cleanup, with a minor eventual-consistency window between steps 2 and 3.clearSingleKeyCostCachefunction correctly scans and deletes 4 key-scoped Redis patterns (period counters, total-cost cache, and lease keys) without touching user-level caches.costResetAtcorrectly hydrated inhydrateKeyFromCachewith fail-open behavior, but lacks the explanatory comment present inhydrateUserFromCache.{isAdmin && ...}. Minor issue: both<h3>and<h4>in the reset section render the sameresetLimits.titletranslation key, duplicating the title text.cost_reset_at timestamptzcolumn addition tokeystable — safe additive migration with no data loss risk.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Admin clicks Reset Quota] --> B[resetKeyLimitsOnly action] B --> C{Is admin?} C -- No --> D[Return PERMISSION_DENIED] C -- Yes --> E[UPDATE keys SET cost_reset_at = NOW in DB] E --> F[Invalidate Redis auth cache for key] F --> G[clearSingleKeyCostCache] G --> H[Scan and delete key period counters] G --> I[Scan and delete total cost cache] G --> J[Scan and delete lease cache] H & I & J --> K[Reset complete] L[Incoming API request] --> M[ProxyRateLimitGuard] M --> N[resolveKeyCostResetAt] N --> O{key.costResetAt vs user.costResetAt} O --> P[Return MAX of both timestamps] P --> Q[Use resolved timestamp to clip period window] Q --> R{Cost within limit?} R -- Yes --> S[Allow request] R -- No --> T[Reject with rate limit error]Prompt To Fix All With AI
Last reviewed commit: b688b9e