feat: provider group cost multiplier & request detail price breakdown#1025
Merged
feat: provider group cost multiplier & request detail price breakdown#1025
Conversation
…akdown Add cost multiplier support for provider groups (compound with provider multiplier), a group management sub-tab in the providers page, and a detailed per-component price breakdown in the request detail billing section. - New `provider_groups` table with name, cost_multiplier, description - New `group_cost_multiplier` and `cost_breakdown` columns on message_request - New `group_cost_multiplier` column on usage_ledger (via trigger update) - Compound multiplier: finalCost = baseCost * providerMultiplier * groupMultiplier - Provider group management UI (Groups tab in provider settings) - Request detail shows per-component costs, base total, and both multipliers - Server actions for group CRUD with admin auth - i18n strings for all 5 locales (en, zh-CN, zh-TW, ja, ru) - Unit tests for cost calculation, repository, and existing test mocks updated Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthrough新增提供商分组功能与群组级成本倍率：添加多语言本地化、DB 模式与触发器、存储的成本分解类型、仓库与动作 CRUD、代理/会话/成本计算/持久化管道变更，以及设置面板与日志详情 UI 的集成与展示调整。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 docstrings
🧪 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 |
github-actions
bot
added
size/XL
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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 provider groups, enabling group-level cost multipliers and detailed billing breakdowns. Key updates include database schema modifications, proxy logic to apply group multipliers, and new UI components for group management. Review feedback identifies a critical missing function reference in the provider selector, potential performance issues when counting providers, and consistency risks with in-memory caching in multi-instance environments. Additionally, suggestions were made to improve error logging and user input validation.
| } | ||
|
|
||
| // === Resolve group cost multiplier === | ||
| const effectiveGroup = getEffectiveProviderGroup(session); |
Contributor
There was a problem hiding this comment.
The function 'getEffectiveProviderGroup' is used here but it is neither imported nor defined within this file. This will lead to a runtime error when resolving the provider group multiplier.
Comment on lines
+41
to
+44
| const [groups, providers] = await Promise.all([ | ||
| findAllProviderGroups(), | ||
| findAllProvidersFresh(), | ||
| ]); |
Contributor
There was a problem hiding this comment.
Fetching all providers using 'findAllProvidersFresh()' just to count them per group is inefficient, especially as the number of providers grows. Consider using a specialized repository method that performs a SQL 'COUNT' with 'GROUP BY' on the 'group_tag' column to retrieve these counts more efficiently.
Comment on lines
+3250
to
+3252
| } catch { | ||
| /* non-critical */ | ||
| } |
Contributor
There was a problem hiding this comment.
Swallowing errors silently in the cost breakdown calculation makes it difficult to diagnose issues when the breakdown fails to generate. It is recommended to log the error for observability.
Suggested change
| } catch { | |
| /* non-critical */ | |
| } | |
| } catch (error) { | |
| logger.error("[CostCalculation] Failed to calculate cost breakdown", { messageId, error }); | |
| } |
Comment on lines
+118
to
+120
| if (Number.isNaN(costMultiplier) || costMultiplier < 0) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
The function returns silently when the cost multiplier is invalid. This provides a poor user experience as the user won't know why their changes weren't saved. Consider showing a toast notification or setting an error state in the UI.
Suggested change
| if (Number.isNaN(costMultiplier) || costMultiplier < 0) { | |
| return; | |
| } | |
| if (Number.isNaN(costMultiplier) || costMultiplier < 0) { | |
| toast.error(t("invalidMultiplier")); | |
| return; | |
| } |
Comment on lines
+33
to
+48
| const CACHE_TTL_MS = 60_000; // 60 seconds | ||
|
|
||
| interface CacheEntry { | ||
| value: number; | ||
| expiresAt: number; | ||
| } | ||
|
|
||
| const multiplierCache = new Map<string, CacheEntry>(); | ||
|
|
||
| /** | ||
| * Invalidate the in-memory cost multiplier cache. | ||
| * Call this after any mutation (create / update / delete) to provider groups. | ||
| */ | ||
| export function invalidateGroupMultiplierCache(): void { | ||
| multiplierCache.clear(); | ||
| } |
Contributor
There was a problem hiding this comment.
The in-memory cache 'multiplierCache' is local to the server instance. In a multi-instance or serverless environment (like Vercel), 'invalidateGroupMultiplierCache' will only clear the cache on the instance where the mutation occurred, leading to potential billing inconsistencies on other instances for up to 60 seconds. Consider using a distributed cache like Redis or reducing the TTL if strict consistency is required for billing multipliers.
| .select() | ||
| .from(providerGroups) | ||
| .orderBy( | ||
| sql`CASE WHEN ${providerGroups.name} = 'default' THEN 0 ELSE 1 END`, |
Contributor
There was a problem hiding this comment.
The string 'default' is hardcoded here. It is better to use the 'PROVIDER_GROUP.DEFAULT' constant for consistency and to avoid potential typos, as seen in other parts of the codebase.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+116
to
+120
| const handleSave = useCallback(() => { | ||
| const costMultiplier = Number.parseFloat(form.costMultiplier); | ||
| if (Number.isNaN(costMultiplier) || costMultiplier < 0) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
When costMultiplier parses to NaN (e.g. the field is cleared or contains non-numeric text), the function returns silently — no toast, no error state. The user clicks "Save" and nothing happens, with no indication of why. A validation toast should be shown:
Suggested change
| const handleSave = useCallback(() => { | |
| const costMultiplier = Number.parseFloat(form.costMultiplier); | |
| if (Number.isNaN(costMultiplier) || costMultiplier < 0) { | |
| return; | |
| } | |
| const costMultiplier = Number.parseFloat(form.costMultiplier); | |
| if (Number.isNaN(costMultiplier) || costMultiplier < 0) { | |
| toast.error(t("invalidMultiplier")); | |
| return; | |
| } |
(Add the corresponding "invalidMultiplier" key to the i18n files for all 5 locales.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/provider-group-tab.tsx
Line: 116-120
Comment:
**Silent validation failure gives no user feedback**
When `costMultiplier` parses to `NaN` (e.g. the field is cleared or contains non-numeric text), the function returns silently — no toast, no error state. The user clicks "Save" and nothing happens, with no indication of why. A validation toast should be shown:
```suggestion
const costMultiplier = Number.parseFloat(form.costMultiplier);
if (Number.isNaN(costMultiplier) || costMultiplier < 0) {
toast.error(t("invalidMultiplier"));
return;
}
```
(Add the corresponding `"invalidMultiplier"` key to the i18n files for all 5 locales.)
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+111
to
+130
| const setData: Record<string, unknown> = { | ||
| updatedAt: new Date(), | ||
| }; | ||
|
|
||
| if (input.costMultiplier !== undefined) { | ||
| setData.costMultiplier = input.costMultiplier.toString(); | ||
| } | ||
| if (input.description !== undefined) { | ||
| setData.description = input.description; | ||
| } | ||
|
|
||
| const [row] = await db | ||
| .update(providerGroups) | ||
| .set(setData) | ||
| .where(eq(providerGroups.id, id)) | ||
| .returning(); | ||
|
|
||
| if (!row) return null; | ||
|
|
||
| invalidateGroupMultiplierCache(); |
There was a problem hiding this comment.
setData object bypasses TypeScript column safety
setData is typed as Record<string, unknown>, so a typo like setData.costMUltiplier would compile and silently not save. Since only two optional fields are ever set here, a typed partial is both safer and more readable:
Suggested change
| const setData: Record<string, unknown> = { | |
| updatedAt: new Date(), | |
| }; | |
| if (input.costMultiplier !== undefined) { | |
| setData.costMultiplier = input.costMultiplier.toString(); | |
| } | |
| if (input.description !== undefined) { | |
| setData.description = input.description; | |
| } | |
| const [row] = await db | |
| .update(providerGroups) | |
| .set(setData) | |
| .where(eq(providerGroups.id, id)) | |
| .returning(); | |
| if (!row) return null; | |
| invalidateGroupMultiplierCache(); | |
| const setCols: { | |
| updatedAt: Date; | |
| costMultiplier?: string; | |
| description?: string | null; | |
| } = { updatedAt: new Date() }; | |
| if (input.costMultiplier !== undefined) { | |
| setCols.costMultiplier = input.costMultiplier.toString(); | |
| } | |
| if (input.description !== undefined) { | |
| setCols.description = input.description; | |
| } | |
| const [row] = await db | |
| .update(providerGroups) | |
| .set(setCols) | |
| .where(eq(providerGroups.id, id)) | |
| .returning(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/repository/provider-groups.ts
Line: 111-130
Comment:
**Untyped `setData` object bypasses TypeScript column safety**
`setData` is typed as `Record<string, unknown>`, so a typo like `setData.costMUltiplier` would compile and silently not save. Since only two optional fields are ever set here, a typed partial is both safer and more readable:
```suggestion
const setCols: {
updatedAt: Date;
costMultiplier?: string;
description?: string | null;
} = { updatedAt: new Date() };
if (input.costMultiplier !== undefined) {
setCols.costMultiplier = input.costMultiplier.toString();
}
if (input.description !== undefined) {
setCols.description = input.description;
}
const [row] = await db
.update(providerGroups)
.set(setCols)
.where(eq(providerGroups.id, id))
.returning();
```
How can I resolve this? If you propose a fix, please make it concise.![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx (1)
333-351:⚠️ Potential issue | 🟠 Major混合 TTL 时会把同一笔 cache write 成本展示两次。
这里展示的
costBreakdown.cache_creation是聚合后的 cache write 基础成本，但 5m 和 1h 两行都会复用它。只要两种 TTL 同时存在，界面就会把同一笔金额显示两遍，用户看到的 breakdown 会比base_total更大。要么把 5m/1h 成本分别持久化，要么在 mixed 场景下只展示一次聚合后的 cache write 成本。Also applies to: 355-372
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx around lines 333 - 351, The UI duplicates the same aggregated cache write cost (costBreakdown.cache_creation) in both the 5m and 1h rows when mixed TTLs are present (cacheCreation5mInputTokens and cacheCreationInputTokens both >0), inflating the breakdown; update the rendering logic in SummaryTab.tsx (the blocks that reference cacheCreation5mInputTokens, cacheCreationInputTokens, cacheTtlApplied and costBreakdown.cache_creation — also the similar block for the 1h row) so that when both TTLs exist you only show the aggregated cost once (e.g., render costBreakdown.cache_creation in one row only or omit it in both and persist separate costs), by gating the costBreakdown display on a check that ensures not mixed TTLs (or that only one of the rows is responsible for showing costBreakdown.cache_creation).src/app/v1/_lib/proxy/response-handler.ts (1)
3361-3382:⚠️ Potential issue | 🟠 MajorRedis/限流侧的成本仍然少乘了 group multiplier。
这里已经把
groupCostMultiplier传进了updateRequestCostFromUsage()，但紧接着调用的trackCostToRedis()下面仍按旧逻辑计算成本，没有把 group multiplier 带进去。结果会变成：DB 中的请求成本是复合倍率，Redis 里的额度/租约/日消费却还是 provider-only 成本，两边会长期不一致。建议补齐 `trackCostToRedis()` 的成本计算
const cost = calculateRequestCost( usage, resolvedPricing.priceData, buildCostCalculationOptions( provider.costMultiplier, session.getContext1mApplied(), priorityServiceTierApplied, - longContextPricing + longContextPricing, + session.getGroupCostMultiplier() ) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 3361 - 3382, The Redis tracking call is missing the session-level group multiplier so Redis-stored costs differ from DB; modify the call to trackCostToRedis to include the group multiplier (e.g., pass session.getGroupCostMultiplier() or the already-computed composite cost) and update trackCostToRedis (and any other call sites) to apply that multiplier when deriving the cost from normalizedUsage/resolvedPricing (references: updateRequestCostFromUsage, trackCostToRedis, session.getGroupCostMultiplier, costUpdateResult.resolvedPricing, normalizedUsage, priorityServiceTierApplied).
🧹 Nitpick comments (3)
src/drizzle/schema.ts (1)
162-170:providerGroups表缺少索引和软删除支持。与其他业务表（如
providers、users）相比，providerGroups表缺少：
deletedAt列：其他表使用软删除模式以支持数据恢复和审计追踪。createdAt索引：用于按时间排序的列表查询。如果预期分组数量较少且不需要软删除，当前设计可以接受；否则建议补充。
♻️ 建议的改进
export const providerGroups = pgTable('provider_groups', { id: serial('id').primaryKey(), name: varchar('name', { length: 200 }).notNull().unique(), costMultiplier: numeric('cost_multiplier', { precision: 10, scale: 4 }).notNull().default('1.0'), description: varchar('description', { length: 500 }), createdAt: timestamp('created_at', { withTimezone: true }).defaultNow(), updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow(), + deletedAt: timestamp('deleted_at', { withTimezone: true }), +}, (table) => ({ + providerGroupsCreatedAtIdx: index('idx_provider_groups_created_at').on(table.createdAt), + providerGroupsDeletedAtIdx: index('idx_provider_groups_deleted_at').on(table.deletedAt), +})); -});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/drizzle/schema.ts` around lines 162 - 170, Add soft-delete support and a createdAt index to the providerGroups table: in the pgTable definition for providerGroups (provider_groups) add a nullable timestamp column named deletedAt (withTimezone true) to support soft deletes, and create an index on the createdAt column to speed time-ordered queries; update any related CRUD/soft-delete helpers to set/clear deletedAt instead of hard-deleting if applicable.src/app/v1/_lib/proxy/session.ts (1)
309-315: 建议给分组倍率 setter 增加有效值保护。Line 309-311 直接赋值；若上游传入
NaN、Infinity或<= 0，会污染后续费用计算与落库。建议在此处兜底回退为1。参考修改
setGroupCostMultiplier(value: number): void { - this.groupCostMultiplier = value; + if (!Number.isFinite(value) || value <= 0) { + logger.warn("[ProxySession] Invalid groupCostMultiplier, fallback to 1", { value }); + this.groupCostMultiplier = 1; + return; + } + this.groupCostMultiplier = value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/session.ts` around lines 309 - 315, The setter setGroupCostMultiplier should validate incoming value before assigning to the groupCostMultiplier field: check for NaN, non-finite (Infinity/-Infinity) or values <= 0 and, in those cases, fallback to 1; otherwise assign the provided number. Keep getGroupCostMultiplier unchanged; reference setGroupCostMultiplier and groupCostMultiplier when applying this defensive check.src/repository/message-write-buffer.ts (1)
32-33: 统一costBreakdown的null/undefined语义，避免类型与 SQL 分支脱节。Line 110 已支持
null清空列，但 Line 32 类型不允许null。建议把costBreakdown改为StoredCostBreakdown | null，并明确：undefined=不更新，null=清空。建议修改
export type MessageRequestUpdatePatch = { @@ - costBreakdown?: StoredCostBreakdown; + /** + * undefined: 不更新该字段 + * null: 显式清空该字段 + */ + costBreakdown?: StoredCostBreakdown | null; };Based on learnings: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined.
Also applies to: 109-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/message-write-buffer.ts` around lines 32 - 33, The property costBreakdown on the message write buffer type should allow explicit null to represent "clear the DB column" while undefined means "do not update"—change its type from StoredCostBreakdown to StoredCostBreakdown | null (reference: costBreakdown and StoredCostBreakdown in src/repository/message-write-buffer.ts) and add a short comment documenting the semantics (undefined = do not update, null = clear column); also update any related usage/assignment sites around the write/update logic that branch on null/undefined (the code paths around the update/insert handling currently mentioned in the review) to follow this convention.
🤖 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/provider-groups.ts`:
- Around line 171-172: 在删除 provider group 时不要静默删除仍被 provider 引用的 group：在
provider-groups.ts 中的删除逻辑（当前直接调用 repoDeleteProviderGroup(id)
并返回）之前，调用查询引用关系的函数（例如查找 providers 中 groupTag/ groupId 的 repo 方法或新增一个
repoListProvidersByGroup(id)）来判断是否存在关联的 provider；如果有关联，拒绝删除并返回错误/异常响应（例如 {
ok:false, error: "... has linked providers" }），只有在不存在引用时才调用
repoDeleteProviderGroup(id) 并返回成功。
- Around line 91-97: The costMultiplier validation currently only checks
input.costMultiplier < 0 which lets NaN and Infinity slip through; update the
validation in the provider-groups handler to ensure the value is a finite
non-negative number by using Number.isFinite(input.costMultiplier) and
input.costMultiplier >= 0 (i.e., reject when
!Number.isFinite(input.costMultiplier) || input.costMultiplier < 0), keeping the
same error shape ("INVALID_MULTIPLIER") and referencing the existing
input.costMultiplier check to locate the correction.
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:
- Around line 308-317: The SummaryTab TSX renders hardcoded unit labels like
"tokens" (and similar unit text around formatTokenAmount/formatCurrency) which
must be i18n'd; replace these hardcoded strings with translation keys (e.g., use
t("billingDetails.unit.tokens") for the tokens unit and
t("billingDetails.unit.usd") or include unit formatting inside the translation
for currency) in the JSX where you render the spans that contain
"{formatTokenAmount(inputTokens)} tokens" and the sibling currency spans (also
update the similar occurrences around formatTokenAmount/formatCurrency at the
other affected blocks noted); keep numeric formatting functions
(formatTokenAmount, formatCurrency) as-is but feed/concatenate their output with
t(...) instead of literal text so all user-visible labels use the i18n keys
(update SummaryTab component imports to include the t hook/translator if not
already present).
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 116-120: handleSave currently returns silently on invalid
costMultiplier (from form.costMultiplier) causing no user feedback; update
handleSave to show a validation error to the user instead of returning quietly —
for example, set a local validation error state (e.g., setCostMultiplierError)
or call the existing app toast/error UI (e.g., toast.error) with a clear message
like "请输入有效的非负成本倍数", prevent save, and ensure the error is cleared when the user
corrects the input or on successful save; modify the handleSave function and the
component state (or use the component's existing error/toast utilities) to
implement this behavior.
In `@src/app/`[locale]/settings/providers/_components/provider-manager.tsx:
- Around line 519-528: The "Groups" view toggle is unreachable on small screens;
ensure a mobile-accessible control toggles viewMode to "groups". Make the
existing Button (using setViewMode("groups"), viewMode, tStrings, Layers)
visible or add an alternative icon-only control for small screens (e.g., render
a compact Button or MenuItem with the Layers icon and onClick={() =>
setViewMode("groups")} that is shown when sm/md classes hide the desktop label).
Keep the same accessibility (title from tStrings("viewModeGroups")) and visual
active state (variant using viewMode === "groups") so mobile users can switch to
the groups view.
In `@src/app/v1/_lib/proxy/provider-selector.ts`:
- Around line 217-222: The call to getGroupCostMultiplier(effectiveGroup) can
throw and currently will bubble out of ensure(); wrap that call in a try/catch:
if getGroupCostMultiplier throws, set multiplier = 1.0 and call
session.setGroupCostMultiplier(1.0), and emit a warning log (via the existing
logger used in this module) including the effectiveGroup id and the error;
otherwise use the returned multiplier as before. Ensure this handling is applied
where getEffectiveProviderGroup(session) and session.setGroupCostMultiplier are
used so optional cost info cannot convert into a request failure.
In `@src/lib/utils/cost-calculation.ts`:
- Around line 305-306: 在倍率归一化阶段为所有来自 multiplierOrOptions 的倍率字段（例如
groupMultiplier、（在 314-315 处的）其他 multiplier 字段）增加数值兜底：在使用前验证为
Number.isFinite(value) 且 value >= 0，否则将其回退为 1.0；并在构造 Decimal（在 985-989
处直接叠乘的逻辑）之前应用该归一化结果以避免 NaN/Infinity/负值污染计费结果。定位并修改处理倍数的函数/变量（如
multiplierOrOptions、groupMultiplier，以及所有后续直接用作 Decimal 运算的变量），统一调用该校验/回退逻辑后再进行
Decimal 运算。
---
Outside diff comments:
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:
- Around line 333-351: The UI duplicates the same aggregated cache write cost
(costBreakdown.cache_creation) in both the 5m and 1h rows when mixed TTLs are
present (cacheCreation5mInputTokens and cacheCreationInputTokens both >0),
inflating the breakdown; update the rendering logic in SummaryTab.tsx (the
blocks that reference cacheCreation5mInputTokens, cacheCreationInputTokens,
cacheTtlApplied and costBreakdown.cache_creation — also the similar block for
the 1h row) so that when both TTLs exist you only show the aggregated cost once
(e.g., render costBreakdown.cache_creation in one row only or omit it in both
and persist separate costs), by gating the costBreakdown display on a check that
ensures not mixed TTLs (or that only one of the rows is responsible for showing
costBreakdown.cache_creation).
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 3361-3382: The Redis tracking call is missing the session-level
group multiplier so Redis-stored costs differ from DB; modify the call to
trackCostToRedis to include the group multiplier (e.g., pass
session.getGroupCostMultiplier() or the already-computed composite cost) and
update trackCostToRedis (and any other call sites) to apply that multiplier when
deriving the cost from normalizedUsage/resolvedPricing (references:
updateRequestCostFromUsage, trackCostToRedis, session.getGroupCostMultiplier,
costUpdateResult.resolvedPricing, normalizedUsage, priorityServiceTierApplied).
---
Nitpick comments:
In `@src/app/v1/_lib/proxy/session.ts`:
- Around line 309-315: The setter setGroupCostMultiplier should validate
incoming value before assigning to the groupCostMultiplier field: check for NaN,
non-finite (Infinity/-Infinity) or values <= 0 and, in those cases, fallback to
1; otherwise assign the provided number. Keep getGroupCostMultiplier unchanged;
reference setGroupCostMultiplier and groupCostMultiplier when applying this
defensive check.
In `@src/drizzle/schema.ts`:
- Around line 162-170: Add soft-delete support and a createdAt index to the
providerGroups table: in the pgTable definition for providerGroups
(provider_groups) add a nullable timestamp column named deletedAt (withTimezone
true) to support soft deletes, and create an index on the createdAt column to
speed time-ordered queries; update any related CRUD/soft-delete helpers to
set/clear deletedAt instead of hard-deleting if applicable.
In `@src/repository/message-write-buffer.ts`:
- Around line 32-33: The property costBreakdown on the message write buffer type
should allow explicit null to represent "clear the DB column" while undefined
means "do not update"—change its type from StoredCostBreakdown to
StoredCostBreakdown | null (reference: costBreakdown and StoredCostBreakdown in
src/repository/message-write-buffer.ts) and add a short comment documenting the
semantics (undefined = do not update, null = clear column); also update any
related usage/assignment sites around the write/update logic that branch on
null/undefined (the code paths around the update/insert handling currently
mentioned in the review) to follow this convention.
🪄 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: 30ad057a-6b96-4203-873f-ccb19a7909b6
📒 Files selected for processing (60)
messages/en/dashboard.jsonmessages/en/providers.jsonmessages/en/settings/index.tsmessages/en/settings/providers/providerGroups.jsonmessages/en/settings/providers/strings.jsonmessages/ja/dashboard.jsonmessages/ja/providers.jsonmessages/ja/settings/index.tsmessages/ja/settings/providers/providerGroups.jsonmessages/ja/settings/providers/strings.jsonmessages/ru/dashboard.jsonmessages/ru/providers.jsonmessages/ru/settings/index.tsmessages/ru/settings/providers/providerGroups.jsonmessages/ru/settings/providers/strings.jsonmessages/zh-CN/dashboard.jsonmessages/zh-CN/providers.jsonmessages/zh-CN/settings/index.tsmessages/zh-CN/settings/providers/providerGroups.jsonmessages/zh-CN/settings/providers/strings.jsonmessages/zh-TW/dashboard.jsonmessages/zh-TW/providers.jsonmessages/zh-TW/settings/index.tsmessages/zh-TW/settings/providers/providerGroups.jsonmessages/zh-TW/settings/providers/strings.jsonsrc/actions/provider-groups.tssrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/index.tsxsrc/app/[locale]/dashboard/logs/_components/error-details-dialog/types.tssrc/app/[locale]/dashboard/logs/_components/usage-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/usage-logs-table.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.test.tsxsrc/app/[locale]/dashboard/logs/_components/virtualized-logs-table.tsxsrc/app/[locale]/settings/providers/_components/provider-group-tab.tsxsrc/app/[locale]/settings/providers/_components/provider-manager.tsxsrc/app/v1/_lib/proxy/message-service.tssrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/session.tssrc/drizzle/schema.tssrc/lib/ledger-backfill/trigger.sqlsrc/lib/utils/cost-calculation.tssrc/repository/message-write-buffer.tssrc/repository/message.tssrc/repository/provider-groups.tssrc/repository/usage-logs.tssrc/types/cost-breakdown.tssrc/types/message.tssrc/types/provider-group.tstests/unit/langfuse/langfuse-trace.test.tstests/unit/lib/cost-calculation-group-multiplier.test.tstests/unit/proxy/error-handler-session-id-error.test.tstests/unit/proxy/error-handler-verbose-provider-error-details.test.tstests/unit/proxy/openai-embeddings-forwarder.test.tstests/unit/proxy/openai-official-standard-forwarder.test.tstests/unit/proxy/proxy-forwarder-raw-passthrough-regression.test.tstests/unit/proxy/response-handler-endpoint-circuit-isolation.test.tstests/unit/proxy/response-handler-lease-decrement.test.tstests/unit/proxy/response-handler-non200.test.tstests/unit/repository/provider-groups.test.ts
Comment on lines
+519
to
+528
| <Button | ||
| variant={viewMode === "groups" ? "secondary" : "ghost"} | ||
| size="sm" | ||
| className="h-7 px-2 gap-1.5 text-xs" | ||
| onClick={() => setViewMode("groups")} | ||
| title={tStrings("viewModeGroups")} | ||
| > | ||
| <Layers className="h-3.5 w-3.5" /> | ||
| <span className="hidden sm:inline">{tStrings("viewModeGroups")}</span> | ||
| </Button> |
There was a problem hiding this comment.
“Groups” 视图在移动端不可达。
该切换按钮只渲染在 md 以上区域，移动端没有入口把 viewMode 切到 "groups"，会导致小屏设备无法使用分组管理功能。
建议修复（为移动端补充视图切换入口）
- <div className="flex items-center gap-2 md:hidden">
+ <div className="flex flex-col gap-2 md:hidden">
+ <Select
+ value={viewMode}
+ onValueChange={(value) => setViewMode(value as "list" | "vendor" | "groups")}
+ disabled={loading}
+ >
+ <SelectTrigger>
+ <SelectValue />
+ </SelectTrigger>
+ <SelectContent>
+ <SelectItem value="list">{tStrings("viewModeList")}</SelectItem>
+ <SelectItem value="vendor">{tStrings("viewModeVendor")}</SelectItem>
+ <SelectItem value="groups">{tStrings("viewModeGroups")}</SelectItem>
+ </SelectContent>
+ </Select>
+ <div className="flex items-center gap-2">
<div className="relative flex-1">
<Search className="absolute left-3 top-1/2 -translate-y-1/2 h-4 w-4 text-muted-foreground" />
<Input
@@
: tFilter("mobileFilter")}
</Button>
+ </div>
</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/settings/providers/_components/provider-manager.tsx around
lines 519 - 528, The "Groups" view toggle is unreachable on small screens;
ensure a mobile-accessible control toggles viewMode to "groups". Make the
existing Button (using setViewMode("groups"), viewMode, tStrings, Layers)
visible or add an alternative icon-only control for small screens (e.g., render
a compact Button or MenuItem with the Layers icon and onClick={() =>
setViewMode("groups")} that is shown when sm/md classes hide the desktop label).
Keep the same accessibility (title from tStrings("viewModeGroups")) and visual
active state (variant using viewMode === "groups") so mobile users can switch to
the groups view.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Provider Groups table | ||
| export const providerGroups = pgTable('provider_groups', { | ||
| id: serial('id').primaryKey(), |
Contributor
There was a problem hiding this comment.
[CRITICAL] [LOGIC-BUG] New schema without committed Drizzle migration
Why this is a problem: src/drizzle/schema.ts:163 adds provider_groups, and src/drizzle/schema.ts:457 / src/drizzle/schema.ts:460 / src/drizzle/schema.ts:932 add new columns used by runtime writes. This PR does not include a generated drizzle/*.sql migration, so existing DBs will be missing these objects and inserts/updates will fail at runtime (e.g., src/lib/ledger-backfill/trigger.sql:30 references group_cost_multiplier).
Suggested fix:
bun run db:generate
# review the generated drizzle/00xx_*.sql (create provider_groups + add group_cost_multiplier/cost_breakdown columns)
git add drizzle/00xx_*.sql
| const effectiveGroup = getEffectiveProviderGroup(session); | ||
| if (effectiveGroup) { | ||
| const multiplier = await getGroupCostMultiplier(effectiveGroup); | ||
| session.setGroupCostMultiplier(multiplier); |
Contributor
There was a problem hiding this comment.
[CRITICAL] [LOGIC-BUG] Group cost multiplier lookup breaks for multi-tag provider groups
Why this is a problem: src/app/v1/_lib/proxy/provider-selector.ts:218 passes effectiveGroup directly into getGroupCostMultiplier(). effectiveGroup is a comma-separated tag list (see src/app/[locale]/dashboard/_components/user/forms/provider-group-select.tsx:12), but getGroupCostMultiplier(groupName) only supports a single group name. For values like "vip,default" or "*", the DB lookup will miss and the code silently falls back to 1.0, under-applying group multipliers and skewing billing + cost-based limits.
Suggested fix: resolve a single matched group name from the selected provider’s tags, then look up that group’s multiplier.
const effectiveGroups = parseProviderGroups(getEffectiveProviderGroup(session));
const providerTags = parseProviderGroups(session.provider?.groupTag) ?? [PROVIDER_GROUP.DEFAULT];
const matchedGroup = effectiveGroups.includes(PROVIDER_GROUP.ALL)
? providerTags[0] ?? PROVIDER_GROUP.DEFAULT
: providerTags.find((tag) => effectiveGroups.includes(tag)) ?? PROVIDER_GROUP.DEFAULT;
session.setGroupCostMultiplier(await getGroupCostMultiplier(matchedGroup));
| setGroups(result.data); | ||
| } else { | ||
| toast.error(result.error); | ||
| } |
Contributor
There was a problem hiding this comment.
[HIGH] [STANDARD-VIOLATION] User-facing errors bypass i18n and show raw server strings
Why this is a problem: src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:81 renders toast.error(result.error). The backing server actions return hardcoded English strings (e.g., src/actions/provider-groups.ts:38 / src/actions/provider-groups.ts:67 / src/actions/provider-groups.ts:83). This violates CLAUDE.md: "i18n Required" - "All user-facing strings must use i18n (5 languages supported). Never hardcode display text".
Suggested fix: use errorCode to map to localized messages on the client (and ensure actions always set an errorCode).
const errorText = (() => {
switch (result.errorCode) {
case "NAME_REQUIRED":
return t("nameRequired");
case "DUPLICATE_NAME":
return t("duplicateName");
case "INVALID_MULTIPLIER":
return t("invalidMultiplier");
case "CANNOT_DELETE_DEFAULT":
return t("cannotDeleteDefault");
default:
return t("updateFailed");
}
})();
toast.error(errorText);
| {costBreakdown ? ( | ||
| <span className="ml-3 text-muted-foreground"> | ||
| {formatCurrency(costBreakdown.cache_creation, "USD", 6)} | ||
| </span> |
Contributor
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] Cache write cost can be double-counted in the UI
Why this is a problem: src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:347 and src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:368 both render formatCurrency(costBreakdown.cache_creation, ...) under Cache Write 5m and Cache Write 1h. But the stored breakdown only has a single cache_creation total (see src/types/cost-breakdown.ts:12). When both cache buckets are present (mixed TTL), both rows can render and the same total cost is shown twice.
Suggested fix (minimal, no schema change): only show the currency value when exactly one cache-write row is displayed.
const showCacheCreationCost =
!!costBreakdown &&
!((cacheCreation5mInputTokens ?? 0) > 0 && (cacheCreation1hInputTokens ?? 0) > 0);
// ...
{showCacheCreationCost ? (
<span className="ml-3 text-muted-foreground">
{formatCurrency(costBreakdown.cache_creation, "USD", 6)}
</span>
) : null}
| const handleSave = useCallback(() => { | ||
| const costMultiplier = Number.parseFloat(form.costMultiplier); | ||
| if (Number.isNaN(costMultiplier) || costMultiplier < 0) { | ||
| return; |
Contributor
There was a problem hiding this comment.
[High] Silent validation failure - no user feedback when cost multiplier is invalid
Why this is a problem: When costMultiplier parses to NaN (e.g., the field is cleared by the user), the function returns silently with no toast, no error state, nothing. The user clicks "Save" and the dialog stays open with no indication of why nothing happened.
This is inconsistent with the name validation at line 135, which correctly shows toast.error(t("nameRequired")).
Suggested fix:
const costMultiplier = Number.parseFloat(form.costMultiplier);
if (Number.isNaN(costMultiplier) || costMultiplier < 0) {
toast.error(t("invalidMultiplier"));
return;
}Add a dedicated i18n key invalidMultiplier to all 5 messages/{locale}/settings/providers/providerGroups.json locale files. The en/providers.json already has this key ("Cost multiplier must be a non-negative number") but the providerGroups.json locale file does not.
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR is XL and mixes database, proxy billing logic, and UI changes in one diff. Two changes are production-impacting: the schema updates are not shipped with a Drizzle migration, and group cost multipliers are looked up using a comma-separated group list (commonly causing groupMultiplier to fall back to 1.0 and skew billing/limits).
PR Size: XL
- Lines changed: 1753
- Files changed: 60
- Split suggestions (XL):
- PR1:
src/drizzle/schema.ts+ generateddrizzle/*.sqlmigration (+ backfill trigger changes) - PR2: proxy billing pipeline (
src/app/v1/_lib/proxy/*,src/lib/utils/cost-calculation.ts) - PR3: UI billing breakdown (
src/app/[locale]/dashboard/logs/_components/*) - PR4: provider group management UI/actions/i18n (
src/actions/provider-groups.ts,src/app/[locale]/settings/providers/_components/*,messages/*)
- PR1:
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 2 | 1 | 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 |
Critical Issues (Must Fix)
- (Confidence 100) Missing DB migration for new table/columns:
src/drizzle/schema.ts:163(also impactssrc/drizzle/schema.ts:457,src/drizzle/schema.ts:460,src/drizzle/schema.ts:932,src/lib/ledger-backfill/trigger.sql:30). - (Confidence 95) Group multiplier lookup uses a multi-tag string and often falls back to 1.0:
src/app/v1/_lib/proxy/provider-selector.ts:218.
High Priority Issues (Should Fix)
- (Confidence 90) Billing breakdown can show
cache_creationcost twice when both cache buckets render:src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:347. - (Confidence 80) User-facing errors bypass i18n and show raw server strings:
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:81(server strings originate fromsrc/actions/provider-groups.ts:38).
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR adds provider group cost multiplier support with compound billing (baseCost * providerMultiplier * groupMultiplier), a management UI tab, and per-component price breakdown display. The core billing logic is correctly implemented with backward-compatible defaults and consistent multiplier application across all three response handler paths (non-streaming, SSE, Codex).
PR Size: XL
- Lines changed: 1,753
- Files changed: 60
Split suggestion: Consider separating into (1) core billing + repository layer (schema, cost calculation, provider-groups repo, proxy pipeline) and (2) UI/UX (provider group tab, request detail breakdown). This would make review and revert easier if issues arise in either area.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 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 |
High Priority Issues (Should Fix)
- Silent validation failure in group save (
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:118-119) - When the cost multiplier input is cleared or invalid,parseFloatreturns NaN and the function returns with no user feedback. Inconsistent with the name validation at line 135 which correctly showstoast.error(t("nameRequired")). Fix: addtoast.error(t("invalidMultiplier"))before the return, and add the i18n key to all 5 locale files.
Medium Priority Issues (Consider Fixing)
- No unit tests for provider group server actions (
src/actions/provider-groups.ts) - The 4 server action functions (getProviderGroups, createProviderGroup, updateProviderGroup, deleteProviderGroup) have no test coverage. The repository layer is well-tested (6 tests), but the action-level logic (auth checks, duplicate name validation, negative multiplier validation, default group deletion guard) is untested. Per CLAUDE.md: "All new features must have unit test coverage of at least 80%."
Deployment Note
The trigger SQL in src/lib/ledger-backfill/trigger.sql must be manually re-applied to the database after running bun run db:migrate. The standard migration workflow generates and applies the schema migration but does not re-execute this trigger file. Without this step, usage_ledger.group_cost_multiplier will remain NULL for all new records. Consider adding this to the PR checklist and/or the deployment runbook.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Claude AI
Contributor
There was a problem hiding this comment.
- Identified open PR
#1025and applied labelsize/XL. - Posted 4 inline review comments and submitted a PR review summary.
Flagged issues (diff lines only):
- [CRITICAL] Missing Drizzle migration for new schema:
src/drizzle/schema.ts:163(alsosrc/drizzle/schema.ts:457,src/drizzle/schema.ts:460,src/drizzle/schema.ts:932,src/lib/ledger-backfill/trigger.sql:30). - [CRITICAL] Group multiplier lookup breaks for multi-tag provider groups:
src/app/v1/_lib/proxy/provider-selector.ts:218. - [HIGH] Cache write cost can be double-counted in billing breakdown UI:
src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:347. - [HIGH] User-facing errors bypass i18n via raw server strings:
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:81(server strings originate fromsrc/actions/provider-groups.ts:38).
Critical: - trackCostToRedis now includes group multiplier so Redis-tracked costs match DB costs - Split cache_creation into cache_creation_5m / cache_creation_1h in breakdown to avoid duplicated display under mixed-TTL requests Major: - Provider-selector wraps getGroupCostMultiplier in try/catch, falls back to 1.0 on errors - Provider-group actions now reject NaN/Infinity multipliers (Number.isFinite check) - Delete group action blocks when providers still reference the group (new GROUP_IN_USE error code) - Group form handleSave shows toast.error instead of silent return on invalid multiplier Minor / hardening: - Cost-calculation normalizeRequestCostOptions sanitizes multipliers (NaN/Infinity/negative -> 1.0) - Session setGroupCostMultiplier validates value before assignment - SummaryTab replaces hardcoded "tokens" label with i18n key (unit.tokens) - costBreakdown type widened to allow null for explicit column clearing Tests: added 5 new cases covering multiplier sanitization and cache breakdown split. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
Comment on lines
+220
to
+233
| const effectiveGroup = getEffectiveProviderGroup(session); | ||
| if (effectiveGroup) { | ||
| try { | ||
| const multiplier = await getGroupCostMultiplier(effectiveGroup); | ||
| session.setGroupCostMultiplier(multiplier); | ||
| } catch (error) { | ||
| logger.warn( | ||
| "[ProviderResolver] Failed to resolve group cost multiplier, falling back to 1.0", | ||
| { | ||
| effectiveGroup, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| } | ||
| ); | ||
| session.setGroupCostMultiplier(1.0); |
There was a problem hiding this comment.
getEffectiveProviderGroup returns the raw providerGroup string unmodified (e.g. "premium,enterprise"). getGroupCostMultiplier then does an exact name lookup (WHERE name = 'premium,enterprise') which never matches any single row, so it falls back to 1.0 and silently applies no multiplier for users/keys with multiple group assignments.
parseProviderGroups (used for filtering elsewhere) handles comma-separated tags, but the multiplier path never splits the string. A user in group "premium" with multiplier 2.0x would be billed correctly only if their providerGroup is exactly "premium" — adding a second group breaks billing for that key.
To fix, parse the effective group string and resolve multipliers for each individual group (e.g. take the first group, the maximum, or compound them per product requirement).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/provider-selector.ts
Line: 220-233
Comment:
**Multi-group key/user silently skips group multiplier**
`getEffectiveProviderGroup` returns the raw `providerGroup` string unmodified (e.g. `"premium,enterprise"`). `getGroupCostMultiplier` then does an exact name lookup (`WHERE name = 'premium,enterprise'`) which never matches any single row, so it falls back to 1.0 and silently applies no multiplier for users/keys with multiple group assignments.
`parseProviderGroups` (used for filtering elsewhere) handles comma-separated tags, but the multiplier path never splits the string. A user in group "premium" with multiplier 2.0x would be billed correctly only if their `providerGroup` is exactly `"premium"` — adding a second group breaks billing for that key.
To fix, parse the effective group string and resolve multipliers for each individual group (e.g. take the first group, the maximum, or compound them per product requirement).
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx (1)
123-156:⚠️ Potential issue | 🟠 MajorCreate/Update 错误码未映射为本地化消息，与 Delete 处理不一致。
Line 135 和 Line 153 在创建/更新失败时直接使用
result.error（英文服务端字符串）作为 toast 文案，而 Line 182-188 的删除处理正确地根据errorCode（GROUP_IN_USE、CANNOT_DELETE_DEFAULT）映射到本地化消息。服务端createProviderGroup/updateProviderGroup返回的errorCode（NAME_REQUIRED、DUPLICATE_NAME、INVALID_MULTIPLIER）应同样做映射，否则非英文用户会看到原始英文错误文案。♻️ 建议修改
+const mapSaveError = (errorCode: string | undefined, fallback: string) => { + switch (errorCode) { + case "NAME_REQUIRED": return t("nameRequired"); + case "DUPLICATE_NAME": return t("duplicateName"); + case "INVALID_MULTIPLIER": return t("invalidMultiplier"); + default: return fallback; + } +}; ... - toast.error(result.error ?? t("updateFailed")); + toast.error(mapSaveError(result.errorCode, t("updateFailed"))); ... - toast.error(result.error ?? t("createFailed")); + toast.error(mapSaveError(result.errorCode, t("createFailed")));需确保 5 个语言文件均补齐
duplicateName等键。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx around lines 123 - 156, The create/update branch uses result.error directly instead of mapping result.errorCode to localized strings like the delete flow does; update the startSaveTransition block handling for createProviderGroup and updateProviderGroup to switch on result.errorCode (e.g., NAME_REQUIRED, DUPLICATE_NAME, INVALID_MULTIPLIER) and call toast.error(t("<appropriateKey>")) for each case (falling back to result.error or t("createFailed"/"updateFailed") if unknown), keeping existing flow that on result.ok shows success, calls closeDialog() and fetchGroups(); ensure you reference result.errorCode in the toast logic for both createProviderGroup and updateProviderGroup and add the corresponding translation keys (duplicateName etc.) to all language files.
🧹 Nitpick comments (8)
src/actions/provider-groups.ts (2)
48-59:providerGroups局部变量与导入的表对象同名易混淆。Line 51 的
providerGroups是从单个 provider 解析出的分组数组，而providerGroups在src/drizzle/schema.ts中也是 Drizzle 表对象名。虽然本文件未直接导入表对象，但在此模块上下文下仍建议重命名为parsedGroups或groupTags以提升可读性。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/provider-groups.ts` around lines 48 - 59, The local variable providerGroups in the loop conflicts by name with the Drizzle table object providerGroups and is confusing; rename the loop variable returned from parseProviderGroups (e.g., to parsedGroups or groupTags) wherever it's declared and referenced in this function, update the for-loop to iterate over the new name, and ensure usages of PROVIDER_GROUP.DEFAULT and groupCounts (Map<string, number>) remain unchanged so counting logic in the providerGroups parsing/counting block continues to work.
204-211: 基于错误消息字符串匹配的回退逻辑较脆弱。Line 205 通过
error.message.includes("default")来识别默认分组删除错误，但：
- 前置检查 Line 180-191 已经覆盖了此场景（通过
existing.name === PROVIDER_GROUP.DEFAULT），此 catch 分支实际上不可达。- 字符串匹配对 i18n 错误消息或消息文案变更不够健壮，任何包含 "default" 子串的错误（例如 DB 约束错误消息）都会被错误归类。
建议移除此回退分支，或让 repository 抛出可区分的错误类型（如自定义 Error 子类）供此处识别。
♻️ 建议修改
- if (error instanceof Error && error.message.includes("default")) { - return { - ok: false, - error: "Cannot delete the default group", - errorCode: "CANNOT_DELETE_DEFAULT", - }; - } logger.error("Failed to delete provider group:", error); return { ok: false, error: "Failed to delete provider group" };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/provider-groups.ts` around lines 204 - 211, The catch branch that classifies errors by checking error instanceof Error && error.message.includes("default") is brittle and effectively unreachable because existing pre-checks (existing.name === PROVIDER_GROUP.DEFAULT) already prevent default-group deletion; remove this string-match fallback from the catch block and either (a) let the error propagate/return a generic error response, or (b) update the repository to throw a specific error type (e.g., CannotDeleteDefaultError) and here check for that type instead—replace the message.includes("default") check with an instanceof check for the custom error class (or remove the branch entirely).src/repository/provider-groups.ts (2)
169-183: 建议使用PROVIDER_GROUP.DEFAULT常量替代硬编码字符串。Line 177 中的
"default"字面量与代码库中其他位置（如src/actions/provider-groups.ts的 Line 185、src/app/v1/_lib/proxy/provider-selector.ts）使用PROVIDER_GROUP.DEFAULT常量的约定不一致，未来若常量值变更可能导致遗漏。♻️ 建议修改
+import { PROVIDER_GROUP } from "@/lib/constants/provider.constants"; ... - if (existing?.name === "default") { + if (existing?.name === PROVIDER_GROUP.DEFAULT) { throw new Error("Cannot delete the default provider group"); }同时 Line 63 的 SQL 排序也可改用
sqlCASE WHEN ${providerGroups.name} = ${PROVIDER_GROUP.DEFAULT} THEN 0 ELSE 1 END``。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/provider-groups.ts` around lines 169 - 183, Replace the hardcoded "default" literal in deleteProviderGroup with the shared constant PROVIDER_GROUP.DEFAULT: when checking existing?.name in deleteProviderGroup, compare against PROVIDER_GROUP.DEFAULT instead of "default"; also update the SQL ordering elsewhere (the earlier sort that uses providerGroups.name) to use sql`CASE WHEN ${providerGroups.name} = ${PROVIDER_GROUP.DEFAULT} THEN 0 ELSE 1 END` so all places consistently reference the PROVIDER_GROUP.DEFAULT constant (ensure you import PROVIDER_GROUP where used).
196-213: 缓存未命中项永不失效可能导致的延迟问题。当
findProviderGroupByName返回null时，1.0会被缓存 60 秒。虽然invalidateGroupMultiplierCache()在 create/update/delete 时会清空缓存，但在多实例/多进程部署（如多个 Next.js 节点）下，其他进程的缓存不会被本进程的变更触发失效，新创建的分组最多需要等待 60 秒才能在所有节点生效。建议在部署文档中注明此 TTL 行为，或在多实例场景下考虑使用 Redis 等共享缓存。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/provider-groups.ts` around lines 196 - 213, The current getGroupCostMultiplier caches a fallback value (1.0) when findProviderGroupByName returns null, causing new groups to take up to CACHE_TTL_MS to appear across processes; update getGroupCostMultiplier to avoid caching misses (do not set multiplierCache when group is null) or cache misses with a much shorter TTL, and/or replace multiplierCache with a shared cache (e.g., Redis) and wire invalidateGroupMultiplierCache to publish invalidations across instances so create/update/delete propagate immediately; reference functions/vars getGroupCostMultiplier, multiplierCache, CACHE_TTL_MS, findProviderGroupByName, and invalidateGroupMultiplierCache in your changes and also add a note in deployment docs about TTL behavior if you keep a local in-process cache.src/app/[locale]/settings/providers/_components/provider-group-tab.tsx (1)
235-235: 建议使用PROVIDER_GROUP.DEFAULT常量替代硬编码"default"。与
src/lib/constants/provider.constants.ts中定义的PROVIDER_GROUP.DEFAULT保持一致可避免字符串散落，便于未来变更。♻️ 建议修改
+import { PROVIDER_GROUP } from "@/lib/constants/provider.constants"; ... - const isDefault = group.name === "default"; + const isDefault = group.name === PROVIDER_GROUP.DEFAULT;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx at line 235, 将硬编码的字符串 "default" 替换为常量 PROVIDER_GROUP.DEFAULT：在 provider-group-tab 组件中找到 isDefault 的判断（const isDefault = group.name === "default";）并改用导入的 PROVIDER_GROUP.DEFAULT 常量（来自 provider.constants 中），确保在文件顶部导入该常量并替换比较表达式以避免散落的字面量。src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx (1)
488-539: Provider / Group 倍率块可抽成复用函数以减少 JSX 重复。两个 IIFE 几乎完全对称（仅数据源与文案 key 不同），建议抽出一个
renderMultiplierRow(value, labelKey)之类的本地辅助，降低后续维护成本。♻️ 参考实现
const renderMultiplierRow = (rawValue: number | string | null | undefined, labelKey: string) => { const v = typeof rawValue === "number" ? rawValue : rawValue ? Number(rawValue) : NaN; if (!Number.isFinite(v) || v === 1) return null; return ( <div className="flex justify-between"> <span className="text-muted-foreground">{t(labelKey)}:</span> <span className="font-mono">{v.toFixed(2)}x</span> </div> ); }; {renderMultiplierRow( costBreakdown ? costBreakdown.provider_multiplier : costMultiplier, "billingDetails.providerMultiplier", )} {renderMultiplierRow( costBreakdown ? costBreakdown.group_multiplier : groupCostMultiplier, "billingDetails.groupMultiplier", )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx around lines 488 - 539, The two IIFE blocks rendering provider and group multipliers are duplicated; extract a local helper (e.g., renderMultiplierRow(rawValue, labelKey)) that normalizes rawValue (number|string|null|undefined) into a Number, returns null if not finite or equals 1, and otherwise renders the same JSX with t(labelKey) and value.toFixed(2); then replace the two IIFEs with calls like renderMultiplierRow(costBreakdown ? costBreakdown.provider_multiplier : costMultiplier, "billingDetails.providerMultiplier") and similarly for group using costBreakdown?.group_multiplier or groupCostMultiplier to remove duplication and centralize logic.src/app/v1/_lib/proxy/response-handler.ts (2)
1126-1142: 非流式和流式两处"raw cost"分支逻辑完全重复，可抽成本文件内的辅助函数。
provider.costMultiplier !== 1 || session.getGroupCostMultiplier() !== 1判断 + 重新调用calculateRequestCost的 raw 分支在 L1126-1142 和 L2145-2161 几乎逐行相同。随着后续倍率维度增加（比如再引入 key 级倍率），两处容易出现不同步。建议抽个局部 helper，例如computeRawCostForLangfuse(usage, priceData, session, ctxApplied, priority, longContextPricing, finalCostStr)，在两处复用。Also applies to: 2145-2161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 1126 - 1142, Extract the duplicated "raw cost" branch into a single helper (e.g., computeRawCostForLangfuse) and replace both occurrences with calls to it; specifically, centralize the check of provider.costMultiplier !== 1 || session.getGroupCostMultiplier() !== 1 and the calculateRequestCost(...) call with buildCostCalculationOptions(1.0, session.getContext1mApplied(), priorityServiceTierApplied, longContextPricing), returning the appropriate rawCostUsdStr (falling back to costUsdStr when the condition is false), then update the two spots that currently set rawCostUsdStr to call this new helper instead of duplicating the logic (references: provider.costMultiplier, session.getGroupCostMultiplier, calculateRequestCost, buildCostCalculationOptions, rawCostUsdStr, costUsdStr).
353-367: 新参数默认值带来双源真相的小隐患。
buildCostCalculationOptions把groupCostMultiplier定为可选（默认 1），但本文件内除了 L1134 / L2153（raw cost，刻意传 1.0，未带 group multiplier）外，所有实际计费路径都会显式传session.getGroupCostMultiplier()。这个默认值对"raw cost 不含倍率"的场景是对的，但也意味着将来若新增 call site 忘记透传 session 倍率，会静默按 1 计费而不会报错。可选：把参数改成必填（
number而非number = 1），在 raw cost 路径显式传1.0，由类型系统帮你守住不变量。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 353 - 367, The function buildCostCalculationOptions currently has an optional groupCostMultiplier with a default of 1, which can silently hide missing callers; make groupCostMultiplier a required number (remove = 1), then update every call site (including the raw-cost callers that currently pass 1.0) to explicitly pass a group multiplier (use session.getGroupCostMultiplier() where appropriate and pass 1.0 explicitly for raw-cost paths), so the type system enforces callers to consider the group multiplier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@messages/zh-CN/dashboard.json`:
- Around line 417-421: The two billingDetails translation structures differ:
logs.details.billingDetails includes a unit.tokens subobject while
logs.billingDetails does not; to avoid future confusion and align with usages in
usage-logs-table.tsx and virtualized-logs-table.tsx, add the missing unit.tokens
key(s) under the logs.billingDetails block in messages/zh-CN/dashboard.json
(mirror the same keys/labels used under logs.details.billingDetails) or, if
intentional, add a clear comment/note in the JSON and surrounding code
indicating the structural difference and where each variant is used (reference:
logs.details.billingDetails, logs.billingDetails, usage-logs-table.tsx,
virtualized-logs-table.tsx).
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:
- Around line 333-388: The 5m/1h rows in SummaryTab incorrectly attribute the
entire legacy costBreakdown.cache_creation to the 5m row when cache_creation_5m
and cache_creation_1h are undefined and cacheTtlApplied === "mixed"; update the
rendering logic that checks cacheCreation5mInputTokens,
cacheCreation1hInputTokens, cacheCreationInputTokens, cacheTtlApplied and
costBreakdown to handle the "mixed" case by either (A) when cache_creation_5m
and cache_creation_1h are both undefined but costBreakdown.cache_creation exists
and cacheTtlApplied === "mixed", split costBreakdown.cache_creation into
cache_creation_5m/cache_creation_1h proportionally using the token counts
(cacheCreation5mInputTokens vs cacheCreation1hInputTokens derived from
cacheCreationInputTokens), or (B) suppress per-ttl currency values and only
render the aggregate total in a single combined line; implement the chosen
approach in SummaryTab where formatCurrency is currently called for
cache_creation_5m / cache_creation_1h fallback paths.
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 137-156: The name validation (currently using form.name.trim())
should run before calling startSaveTransition so the UI (isSaving/loading)
doesn't briefly flash; move the check for form.name.trim() alongside the
existing costMultiplier validation outside the startSaveTransition callback,
return early on failure (toast.error(t("nameRequired"))) and only invoke
startSaveTransition to run the async createProviderGroup / updateProviderGroup
logic when validations pass; update references to form.name, costMultiplier,
startSaveTransition, isSaving, createProviderGroup and the create branch so the
transition contains only the network call and result handling.
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 3248-3250: The breakdown currently persists the raw costMultiplier
and groupCostMultiplier params which may be unsanitized and thus inconsistent
with total computed by calculateRequestCost; update the code that builds the
breakdown (fields provider_multiplier and group_multiplier) to use the sanitized
multipliers used by calculateRequestCost—reuse the existing sanitizeMultiplier
(or the exported normalizeRequestCostOptions helper) to compute
sanitizedProviderMultiplier and sanitizedGroupMultiplier and write those
sanitized values into provider_multiplier and group_multiplier so total =
base_total × provider_multiplier × group_multiplier remains consistent.
---
Duplicate comments:
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 123-156: The create/update branch uses result.error directly
instead of mapping result.errorCode to localized strings like the delete flow
does; update the startSaveTransition block handling for createProviderGroup and
updateProviderGroup to switch on result.errorCode (e.g., NAME_REQUIRED,
DUPLICATE_NAME, INVALID_MULTIPLIER) and call toast.error(t("<appropriateKey>"))
for each case (falling back to result.error or t("createFailed"/"updateFailed")
if unknown), keeping existing flow that on result.ok shows success, calls
closeDialog() and fetchGroups(); ensure you reference result.errorCode in the
toast logic for both createProviderGroup and updateProviderGroup and add the
corresponding translation keys (duplicateName etc.) to all language files.
---
Nitpick comments:
In `@src/actions/provider-groups.ts`:
- Around line 48-59: The local variable providerGroups in the loop conflicts by
name with the Drizzle table object providerGroups and is confusing; rename the
loop variable returned from parseProviderGroups (e.g., to parsedGroups or
groupTags) wherever it's declared and referenced in this function, update the
for-loop to iterate over the new name, and ensure usages of
PROVIDER_GROUP.DEFAULT and groupCounts (Map<string, number>) remain unchanged so
counting logic in the providerGroups parsing/counting block continues to work.
- Around line 204-211: The catch branch that classifies errors by checking error
instanceof Error && error.message.includes("default") is brittle and effectively
unreachable because existing pre-checks (existing.name ===
PROVIDER_GROUP.DEFAULT) already prevent default-group deletion; remove this
string-match fallback from the catch block and either (a) let the error
propagate/return a generic error response, or (b) update the repository to throw
a specific error type (e.g., CannotDeleteDefaultError) and here check for that
type instead—replace the message.includes("default") check with an instanceof
check for the custom error class (or remove the branch entirely).
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:
- Around line 488-539: The two IIFE blocks rendering provider and group
multipliers are duplicated; extract a local helper (e.g.,
renderMultiplierRow(rawValue, labelKey)) that normalizes rawValue
(number|string|null|undefined) into a Number, returns null if not finite or
equals 1, and otherwise renders the same JSX with t(labelKey) and
value.toFixed(2); then replace the two IIFEs with calls like
renderMultiplierRow(costBreakdown ? costBreakdown.provider_multiplier :
costMultiplier, "billingDetails.providerMultiplier") and similarly for group
using costBreakdown?.group_multiplier or groupCostMultiplier to remove
duplication and centralize logic.
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Line 235: 将硬编码的字符串 "default" 替换为常量 PROVIDER_GROUP.DEFAULT：在 provider-group-tab
组件中找到 isDefault 的判断（const isDefault = group.name === "default";）并改用导入的
PROVIDER_GROUP.DEFAULT 常量（来自 provider.constants
中），确保在文件顶部导入该常量并替换比较表达式以避免散落的字面量。
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 1126-1142: Extract the duplicated "raw cost" branch into a single
helper (e.g., computeRawCostForLangfuse) and replace both occurrences with calls
to it; specifically, centralize the check of provider.costMultiplier !== 1 ||
session.getGroupCostMultiplier() !== 1 and the calculateRequestCost(...) call
with buildCostCalculationOptions(1.0, session.getContext1mApplied(),
priorityServiceTierApplied, longContextPricing), returning the appropriate
rawCostUsdStr (falling back to costUsdStr when the condition is false), then
update the two spots that currently set rawCostUsdStr to call this new helper
instead of duplicating the logic (references: provider.costMultiplier,
session.getGroupCostMultiplier, calculateRequestCost,
buildCostCalculationOptions, rawCostUsdStr, costUsdStr).
- Around line 353-367: The function buildCostCalculationOptions currently has an
optional groupCostMultiplier with a default of 1, which can silently hide
missing callers; make groupCostMultiplier a required number (remove = 1), then
update every call site (including the raw-cost callers that currently pass 1.0)
to explicitly pass a group multiplier (use session.getGroupCostMultiplier()
where appropriate and pass 1.0 explicitly for raw-cost paths), so the type
system enforces callers to consider the group multiplier.
In `@src/repository/provider-groups.ts`:
- Around line 169-183: Replace the hardcoded "default" literal in
deleteProviderGroup with the shared constant PROVIDER_GROUP.DEFAULT: when
checking existing?.name in deleteProviderGroup, compare against
PROVIDER_GROUP.DEFAULT instead of "default"; also update the SQL ordering
elsewhere (the earlier sort that uses providerGroups.name) to use sql`CASE WHEN
${providerGroups.name} = ${PROVIDER_GROUP.DEFAULT} THEN 0 ELSE 1 END` so all
places consistently reference the PROVIDER_GROUP.DEFAULT constant (ensure you
import PROVIDER_GROUP where used).
- Around line 196-213: The current getGroupCostMultiplier caches a fallback
value (1.0) when findProviderGroupByName returns null, causing new groups to
take up to CACHE_TTL_MS to appear across processes; update
getGroupCostMultiplier to avoid caching misses (do not set multiplierCache when
group is null) or cache misses with a much shorter TTL, and/or replace
multiplierCache with a shared cache (e.g., Redis) and wire
invalidateGroupMultiplierCache to publish invalidations across instances so
create/update/delete propagate immediately; reference functions/vars
getGroupCostMultiplier, multiplierCache, CACHE_TTL_MS, findProviderGroupByName,
and invalidateGroupMultiplierCache in your changes and also add a note in
deployment docs about TTL behavior if you keep a local in-process cache.
🪄 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: e3422ef9-c05c-4e58-b993-f6008aa99efd
📒 Files selected for processing (22)
messages/en/dashboard.jsonmessages/en/settings/providers/providerGroups.jsonmessages/ja/dashboard.jsonmessages/ja/settings/providers/providerGroups.jsonmessages/ru/dashboard.jsonmessages/ru/settings/providers/providerGroups.jsonmessages/zh-CN/dashboard.jsonmessages/zh-CN/settings/providers/providerGroups.jsonmessages/zh-TW/dashboard.jsonmessages/zh-TW/settings/providers/providerGroups.jsonsrc/actions/provider-groups.tssrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsxsrc/app/[locale]/settings/providers/_components/provider-group-tab.tsxsrc/app/v1/_lib/proxy/provider-selector.tssrc/app/v1/_lib/proxy/response-handler.tssrc/app/v1/_lib/proxy/session.tssrc/lib/utils/cost-calculation.tssrc/repository/message-write-buffer.tssrc/repository/provider-groups.tssrc/types/cost-breakdown.tstests/unit/lib/cost-calculation-breakdown.test.tstests/unit/lib/cost-calculation-group-multiplier.test.ts
✅ Files skipped from review due to trivial changes (7)
- messages/zh-CN/settings/providers/providerGroups.json
- messages/ja/settings/providers/providerGroups.json
- messages/ru/settings/providers/providerGroups.json
- messages/ru/dashboard.json
- messages/en/settings/providers/providerGroups.json
- src/types/cost-breakdown.ts
- messages/zh-TW/settings/providers/providerGroups.json
🚧 Files skipped from review as they are similar to previous changes (7)
- src/repository/message-write-buffer.ts
- src/lib/utils/cost-calculation.ts
- messages/zh-TW/dashboard.json
- src/app/v1/_lib/proxy/provider-selector.ts
- tests/unit/lib/cost-calculation-group-multiplier.test.ts
- messages/en/dashboard.json
- messages/ja/dashboard.json
Critical (P1):
- Multi-group providerGroup ("premium,enterprise") no longer silently falls back to
1.0x; getGroupCostMultiplier now parses the string and uses the first matching
group in the provider_groups table. 4 new unit tests cover single/multi/no-match.
Major:
- Create / Update errors now map errorCode to localized toast messages
(NAME_REQUIRED / DUPLICATE_NAME / INVALID_MULTIPLIER), matching the Delete flow.
- Name validation moved out of startSaveTransition to avoid a brief loading flash
on validation failures.
Minor / hardening:
- storedBreakdown now persists sanitizeMultiplier()-clamped values so
total === base_total * provider_multiplier * group_multiplier holds for
unsanitized inputs. sanitizeMultiplier is now exported from cost-calculation.
- Legacy mixed-TTL records (no cache_creation_5m/_1h, cacheTtlApplied="mixed")
now split the aggregate proportionally by token counts instead of dumping it
into the 5m row.
Nitpicks:
- Replace hardcoded "default" with PROVIDER_GROUP.DEFAULT constant in repo,
provider-group-tab, and SQL ordering.
- Don't cache getGroupCostMultiplier misses (1.0 fallback) so newly-created
groups propagate on the next request rather than up to 60s later.
- Rename local providerGroups variable to parsedGroups to avoid shadowing the
Drizzle table identifier.
- Remove unreachable default-group catch fallback in deleteProviderGroup action
(pre-check already handles this case).
- Mirror unit.tokens key into dashboard.logs.billingDetails in all 5 locales so
the two billingDetails blocks stay structurally consistent.
Tests: +4 new cases for multi-group resolution, first-wins, no-match, and
no-cache-on-miss. Total provider-groups repo tests: 10 pass.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx (1)
76-85:⚠️ Potential issue | 🟠 Major
fetchGroups回退错误仍然直显服务端原始英文字符串。
getProviderGroups()在非管理员或查询失败时分别返回硬编码英文"Unauthorized"/"Failed to fetch provider groups"，这里直接以toast.error(result.error)抛给用户，绕过了 i18n。该问题在既往评审中已被标记，保存流程已通过mapSaveError解决，但列表加载路径尚未对齐。建议与
handleDelete的回退分支一并收敛到一个通用的 i18n 提示（如t("loadFailed")/t("deleteFailed")），原始result.error仅保留到logger/开发者控制台。♻️ 建议修改
startLoadTransition(async () => { const result = await getProviderGroups(); if (result.ok) { setGroups(result.data); } else { - toast.error(result.error); + toast.error(t("loadFailed")); } });并在
messages/*/settings/providers/providerGroups.json中补齐loadFailed键。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/`[locale]/settings/providers/_components/provider-group-tab.tsx around lines 76 - 85, fetchGroups currently displays raw server error strings via toast.error(result.error) which bypasses i18n; change the failure branch in fetchGroups (the callback passed to startLoadTransition that calls getProviderGroups) to use the i18n key (e.g. t("loadFailed")) for the user-facing toast, log or console.error the original result.error for developers, and align this behavior with handleDelete/mapSaveError; also add the loadFailed key to messages/*/settings/providers/providerGroups.json for all supported languages.src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx (1)
101-136:⚠️ Potential issue | 🟡 MinorLegacy mixed TTL 且缺少 per-TTL token 数时的回退归因
resolveCacheCreationSplit在cacheTtlApplied === "mixed"但tokens5m + tokens1h === 0时（legacy 数据既未写入cache_creation_5m/_1h，又未回填 5m/1h token 明细）会穿过 mixed 分支，落入末尾的 "Default / 5m" 分支，将整段cache_creation计入 5m 行、1h 行显示 0。这种组合在线上应非常罕见（mixed 通常伴随至少一边的 token 明细），当前实现不会重复计数、仅是归因近似，可以接受。如果后续在真实数据中观察到 mixed 归因偏差，建议在这种零 token 明细的兜底里改为"不展示分项金额，仅保留 token 数与合计"，避免误导性地把聚合额度全部显示在 5m 一行。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx around lines 101 - 136, resolveCacheCreationSplit currently falls back to attributing the entire legacy cost_creation to 5m when cacheTtlApplied === "mixed" but cacheCreation5mInputTokens + cacheCreation1hInputTokens === 0; change this so mixed+zero-tokens does not fabricate a per-TTL split: inside resolveCacheCreationSplit detect cacheTtlApplied === "mixed" && (cacheCreation5mInputTokens ?? 0) + (cacheCreation1hInputTokens ?? 0) === 0 and return { fiveM: null, oneH: null } (or otherwise signal "no per-TTL amount" using the same null semantics as the top-of-function early return), and update any consumers that read the fiveM/oneH values to handle null as "do not show split, only show aggregate".
🧹 Nitpick comments (7)
src/repository/provider-groups.ts (2)
35-50:multiplierCache未设容量上限,长期运行可能持续膨胀
Map只在显式调用invalidateGroupMultiplierCache或相关 mutation 时清空,过期条目不会被主动回收。若输入键基数较高(例如不同用户的组字符串组合多样),进程内条目会随时间单调增长直至下一次 mutation。建议加一个简单的上限/LRU 策略,或在查找时惰性清理过期条目,避免边角场景下的内存累积。一个极简改法:示例:简单容量上限
+const CACHE_MAX_ENTRIES = 1024; @@ if (resolved !== null) { + if (multiplierCache.size >= CACHE_MAX_ENTRIES) { + // Drop the oldest inserted entry (Map preserves insertion order). + const oldestKey = multiplierCache.keys().next().value; + if (oldestKey !== undefined) multiplierCache.delete(oldestKey); + } multiplierCache.set(rawGroupString, { value: resolved, expiresAt: now + CACHE_TTL_MS, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/provider-groups.ts` around lines 35 - 50, multiplierCache currently has no capacity limit and only cleared by invalidateGroupMultiplierCache, so expired keys can accumulate; update functions interacting with multiplierCache (the Map named multiplierCache and the lookup logic that uses CACHE_TTL_MS and CacheEntry) to prevent unbounded growth by implementing either a simple max-size eviction (e.g., drop oldest or random entry when size>MAX_CACHE_SIZE) or lazy cleanup on lookup (scan and delete expired CacheEntry items before inserting/reading), and ensure invalidateGroupMultiplierCache still clears the Map; pick one strategy and add a MAX_CACHE_SIZE constant and eviction logic near where multiplierCache is read/updated to keep memory bounded.
211-247: 热路径可合并为单次查询,并规范化缓存键当前实现对每个解析出的组名都会触发一次
findProviderGroupByName(即 N 次独立 SELECT)。虽然典型输入只有 1–2 个组,但用inArray一次性取回候选行、再按声明顺序挑选命中项,可使最坏情况也只需 1 次往返,且逻辑更简洁。此外,缓存键直接使用rawGroupString,会让"a,b"、"b,a"、" a , b "这类语义相同的输入各自占用独立条目;用parseProviderGroups归一化后再作为键,可提高命中率并减少条目数。建议的重构
-import { asc, eq, sql } from "drizzle-orm"; +import { asc, eq, inArray, sql } from "drizzle-orm"; @@ export async function getGroupCostMultiplier(rawGroupString: string): Promise<number> { const now = Date.now(); - // Cache hit fast-path: we key the cache on the raw input string so that - // repeated lookups for the same user bypass parsing + DB entirely. - const cached = multiplierCache.get(rawGroupString); + const parsedGroups = parseProviderGroups(rawGroupString); + if (parsedGroups.length === 0) { + return 1.0; + } + + // Normalize the cache key so equivalent inputs share one entry. + const cacheKey = parsedGroups.join(","); + const cached = multiplierCache.get(cacheKey); if (cached && cached.expiresAt > now) { return cached.value; } - const parsedGroups = parseProviderGroups(rawGroupString); - if (parsedGroups.length === 0) { - return 1.0; - } - - // Walk the user's declared groups in order; the first matching DB row wins. - let resolved: number | null = null; - for (const name of parsedGroups) { - const group = await findProviderGroupByName(name); - if (group) { - resolved = group.costMultiplier; - break; - } - } + // Fetch all candidate rows in one query, then pick the first match in + // declared order so precedence semantics are preserved. + const rows = await db + .select() + .from(providerGroups) + .where(inArray(providerGroups.name, parsedGroups)); + const byName = new Map(rows.map((r) => [r.name, toProviderGroup(r)])); + let resolved: number | null = null; + for (const name of parsedGroups) { + const group = byName.get(name); + if (group) { + resolved = group.costMultiplier; + break; + } + } if (resolved !== null) { - multiplierCache.set(rawGroupString, { + multiplierCache.set(cacheKey, { value: resolved, expiresAt: now + CACHE_TTL_MS, }); return resolved; } return 1.0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/provider-groups.ts` around lines 211 - 247, The hot path issues are that getGroupCostMultiplier calls findProviderGroupByName once per parsed group (N queries) and uses rawGroupString as the cache key; fix by parsing and normalizing the groups first (use parseProviderGroups(rawGroupString)), form a normalized cache key (e.g., parsedGroups.join(',')), check multiplierCache with that key, and on cache miss perform a single batch DB query (implement or use a new findProviderGroupsByNames(names: string[]) that does a single SELECT ... WHERE name IN (...)) to fetch candidate rows; then iterate parsedGroups in declaration order to pick the first hit (group.costMultiplier), cache only real hits under the normalized key with expiresAt, and return 1.0 if none matched. Ensure you update getGroupCostMultiplier to reference parseProviderGroups, multiplierCache, and the new batch finder instead of calling findProviderGroupByName in a loop.src/app/[locale]/settings/providers/_components/provider-group-tab.tsx (3)
150-179:description在创建 / 更新时的落库语义不一致。
- 创建分支：
description: form.description || undefined—— 空串时走undefined，server action 内再?? null入库为null。- 更新分支：
description: form.description || null—— 空串时直接显式为null。两条路径最终效果相同，但读起来容易让人以为有语义差异。建议统一为同一种写法（二选一），降低后续维护误解。
♻️ 建议修改
- description: form.description || null, + description: form.description.trim() || null, ... - description: form.description || undefined, + description: form.description.trim() || null,同时顺手
trim()能避免把纯空白写入数据库。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx around lines 150 - 179, The two branches differ in how they pass description when saving (create uses `form.description || undefined`, update uses `form.description || null`), which is confusing; change both the create and update calls in the `startSaveTransition` block to use the same canonical value (either `null` or `undefined`) and ensure you pass a trimmed string (e.g. `form.description?.trim()` or its absence) so pure-whitespace becomes empty and stored consistently; update the calls to `updateProviderGroup(...)` and `createProviderGroup(...)` to use the unified `description` handling.
307-370: 保存过程中对话框仍可被 ESC / 点击外部关闭。
onOpenChange={(open) => !open && closeDialog()}不受isSaving限制，用户在 transition 执行期间按 ESC 或点击遮罩会提前关闭对话框并清空form，之后toast.success/error仍会触发，但交互观感是“还没保存就关了”。删除对话框同理（第 373 行）。♻️ 建议修改
- <Dialog open={dialogOpen} onOpenChange={(open) => !open && closeDialog()}> + <Dialog + open={dialogOpen} + onOpenChange={(open) => { + if (!open && !isSaving) closeDialog(); + }} + >- <Dialog open={!!deleteTarget} onOpenChange={(open) => !open && closeDeleteConfirm()}> + <Dialog + open={!!deleteTarget} + onOpenChange={(open) => { + if (!open && !isDeleting) closeDeleteConfirm(); + }} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx around lines 307 - 370, The dialog is still closable via ESC/overlay while saving because onOpenChange={(open) => !open && closeDialog()} doesn't check isSaving; update the Dialog and the delete-confirm Dialog to guard against closing during saves by changing the handler to something like onOpenChange={(open) => { if (!open && isSaving) return; if (!open) closeDialog(); }} (or add the same check inside closeDialog/closeDeleteDialog), and ensure any close helpers referenced by handleSave (closeDialog, closeDeleteDialog) respect isSaving so the form isn't cleared while a save is in progress.
101-109: 确保costMultiplier在编辑对话框和表格中的显示格式一致。
costMultiplier在数据库中存储为numeric(10, 4)（如"1.0000"），但在 JavaScript 中被转换为 number 类型。当用户输入1时，表单显示"1"，而表格中也显示1x；但当输入1.5时，两处都显示1.5x。这导致视觉上的不一致。建议：
- 在 105 行将
costMultiplier转换为字符串时，使用group.costMultiplier.toFixed(2)确保统一格式（如"1.00"）- 在 269 行的表格显示处，也使用
group.costMultiplier.toFixed(2)保持一致，显示为1.00x、1.50x等示例修复
// 第 105 行 costMultiplier: group.costMultiplier.toFixed(2), // 第 269 行 <TableCell className="font-mono">{group.costMultiplier.toFixed(2)}x</TableCell>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx around lines 101 - 109, The costMultiplier formatting is inconsistent between the edit dialog and the table; update the openEditDialog handler (where setForm is called for ProviderGroupWithCount) to set costMultiplier as a fixed 2-decimal string (use group.costMultiplier.toFixed(2)) and likewise change the table cell that renders the multiplier (the <TableCell> that shows group.costMultiplier) to render group.costMultiplier.toFixed(2) + "x" so both UI places display e.g. "1.00" / "1.00x" consistently.src/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx (1)
521-572: 可选重构：两个倍率 IIFE 结构高度重复，可抽取为一个小渲染辅助
Provider Multiplier与Group Multiplier两个 IIFE 的分支逻辑（优先costBreakdown.*_multiplier，缺失时回退到 props 字符串并Number()解析，统一在!Number.isFinite(x) || x === 1时不渲染）几乎一一对应。可以抽成一个本地renderMultiplierRow(label, breakdownValue, fallbackRaw)辅助来消除重复，也便于将来同步调整四舍五入、显示精度或新增更多倍率维度。♻️ 建议的抽取示意
const renderMultiplierRow = ( label: string, breakdownValue: number | undefined, fallbackRaw: string | number | null | undefined ) => { const resolved = breakdownValue !== undefined ? breakdownValue : fallbackRaw === "" || fallbackRaw == null ? null : Number(fallbackRaw); if (resolved === null || !Number.isFinite(resolved) || resolved === 1) return null; return ( <div className="flex justify-between"> <span className="text-muted-foreground">{label}:</span> <span className="font-mono">{resolved.toFixed(2)}x</span> </div> ); }; // Usage: {renderMultiplierRow( t("billingDetails.providerMultiplier"), costBreakdown?.provider_multiplier, costMultiplier, )} {renderMultiplierRow( t("billingDetails.groupMultiplier"), costBreakdown?.group_multiplier, groupCostMultiplier, )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx around lines 521 - 572, Two almost-identical IIFEs render "Provider Multiplier" and "Group Multiplier"; extract a small helper like renderMultiplierRow(label, breakdownValue, fallbackRaw) and use it for both rows. Implement renderMultiplierRow in SummaryTab (or next to the component) to: resolve value = breakdownValue if !== undefined else parse Number(fallbackRaw) unless fallbackRaw is ""/null, return null when value is null/!Number.isFinite(value)/value===1, otherwise render the shared <div className="flex justify-between"> with label and value.toFixed(2)+"x"; replace the two duplicated IIFE blocks with two calls to renderMultiplierRow using costBreakdown?.provider_multiplier / costMultiplier and costBreakdown?.group_multiplier / groupCostMultiplier respectively.src/app/v1/_lib/proxy/response-handler.ts (1)
3229-3258: 建议直接复用breakdown.total作为base_total,避免二次求和引入舍入漂移。当前实现将四个已经过
toDecimalPlaces(COST_SCALE).toNumber()的字段重新包成Decimal再相加,理论上可能与breakdown.total出现最后一位的舍入差异(四舍五入后求和 vs 求和后四舍五入)。参见src/lib/utils/cost-calculation.tsLine 722 —total = inputBucket.add(outputBucket).add(cacheCreationBucket).add(cacheReadBucket),breakdown.total已经是权威的base_total。直接复用可以消除"base_total × provider × group ≠ total"被前端校验/审计判为不一致的边界风险。♻️ 建议改法
- const baseTotal = new Decimal(breakdown.input) - .plus(breakdown.output) - .plus(breakdown.cache_creation) - .plus(breakdown.cache_read); // Use the same sanitization rules as calculateRequestCost so that // total === base_total * provider_multiplier * group_multiplier // holds even when the caller passes NaN / Infinity / negative values. storedBreakdown = { input: String(breakdown.input), output: String(breakdown.output), cache_creation: String(breakdown.cache_creation), cache_creation_5m: String(breakdown.cache_creation_5m), cache_creation_1h: String(breakdown.cache_creation_1h), cache_read: String(breakdown.cache_read), - base_total: baseTotal.toDecimalPlaces(COST_SCALE).toString(), + base_total: String(breakdown.total), provider_multiplier: sanitizeMultiplier(costMultiplier), group_multiplier: sanitizeMultiplier(groupCostMultiplier), total: cost.toString(), };若同时移除
Decimal/COST_SCALE的直接使用,记得清理对应 import。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/v1/_lib/proxy/response-handler.ts` around lines 3229 - 3258, The code recomputes base_total by re-summing Decimal-wrapped fields which can introduce rounding drift; instead assign base_total directly from the authoritative breakdown.total returned by calculateRequestCostBreakdown (e.g. storedBreakdown.base_total = String(breakdown.total) or use breakdown.total as-is if already a string/normalized), remove the Decimal re-sum and any now-unused Decimal/COST_SCALE imports, and keep the rest of storedBreakdown fields and sanitizer usage (sanitizeMultiplier, costMultiplier, groupCostMultiplier) unchanged so total == base_total * provider_multiplier * group_multiplier remains stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 203-211: The default error branch in the provider group deletion
flow exposes raw English from result.error; update the else branch in
provider-group-tab.tsx (the deleteProviderGroup result handling) to never
display result.error directly—use toast.error(t("deleteFailed")) as the fallback
instead of toast.error(result.error ?? t("deleteFailed")). Also apply the same
i18n-only fallback approach to the save error mapping (mapSaveError) so no raw
result.error is shown to users; reference result.errorCode checks for
GROUP_IN_USE and CANNOT_DELETE_DEFAULT and keep those localized mappings as-is.
---
Duplicate comments:
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:
- Around line 101-136: resolveCacheCreationSplit currently falls back to
attributing the entire legacy cost_creation to 5m when cacheTtlApplied ===
"mixed" but cacheCreation5mInputTokens + cacheCreation1hInputTokens === 0;
change this so mixed+zero-tokens does not fabricate a per-TTL split: inside
resolveCacheCreationSplit detect cacheTtlApplied === "mixed" &&
(cacheCreation5mInputTokens ?? 0) + (cacheCreation1hInputTokens ?? 0) === 0 and
return { fiveM: null, oneH: null } (or otherwise signal "no per-TTL amount"
using the same null semantics as the top-of-function early return), and update
any consumers that read the fiveM/oneH values to handle null as "do not show
split, only show aggregate".
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 76-85: fetchGroups currently displays raw server error strings via
toast.error(result.error) which bypasses i18n; change the failure branch in
fetchGroups (the callback passed to startLoadTransition that calls
getProviderGroups) to use the i18n key (e.g. t("loadFailed")) for the
user-facing toast, log or console.error the original result.error for
developers, and align this behavior with handleDelete/mapSaveError; also add the
loadFailed key to messages/*/settings/providers/providerGroups.json for all
supported languages.
---
Nitpick comments:
In
`@src/app/`[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsx:
- Around line 521-572: Two almost-identical IIFEs render "Provider Multiplier"
and "Group Multiplier"; extract a small helper like renderMultiplierRow(label,
breakdownValue, fallbackRaw) and use it for both rows. Implement
renderMultiplierRow in SummaryTab (or next to the component) to: resolve value =
breakdownValue if !== undefined else parse Number(fallbackRaw) unless
fallbackRaw is ""/null, return null when value is
null/!Number.isFinite(value)/value===1, otherwise render the shared <div
className="flex justify-between"> with label and value.toFixed(2)+"x"; replace
the two duplicated IIFE blocks with two calls to renderMultiplierRow using
costBreakdown?.provider_multiplier / costMultiplier and
costBreakdown?.group_multiplier / groupCostMultiplier respectively.
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 150-179: The two branches differ in how they pass description when
saving (create uses `form.description || undefined`, update uses
`form.description || null`), which is confusing; change both the create and
update calls in the `startSaveTransition` block to use the same canonical value
(either `null` or `undefined`) and ensure you pass a trimmed string (e.g.
`form.description?.trim()` or its absence) so pure-whitespace becomes empty and
stored consistently; update the calls to `updateProviderGroup(...)` and
`createProviderGroup(...)` to use the unified `description` handling.
- Around line 307-370: The dialog is still closable via ESC/overlay while saving
because onOpenChange={(open) => !open && closeDialog()} doesn't check isSaving;
update the Dialog and the delete-confirm Dialog to guard against closing during
saves by changing the handler to something like onOpenChange={(open) => { if
(!open && isSaving) return; if (!open) closeDialog(); }} (or add the same check
inside closeDialog/closeDeleteDialog), and ensure any close helpers referenced
by handleSave (closeDialog, closeDeleteDialog) respect isSaving so the form
isn't cleared while a save is in progress.
- Around line 101-109: The costMultiplier formatting is inconsistent between the
edit dialog and the table; update the openEditDialog handler (where setForm is
called for ProviderGroupWithCount) to set costMultiplier as a fixed 2-decimal
string (use group.costMultiplier.toFixed(2)) and likewise change the table cell
that renders the multiplier (the <TableCell> that shows group.costMultiplier) to
render group.costMultiplier.toFixed(2) + "x" so both UI places display e.g.
"1.00" / "1.00x" consistently.
In `@src/app/v1/_lib/proxy/response-handler.ts`:
- Around line 3229-3258: The code recomputes base_total by re-summing
Decimal-wrapped fields which can introduce rounding drift; instead assign
base_total directly from the authoritative breakdown.total returned by
calculateRequestCostBreakdown (e.g. storedBreakdown.base_total =
String(breakdown.total) or use breakdown.total as-is if already a
string/normalized), remove the Decimal re-sum and any now-unused
Decimal/COST_SCALE imports, and keep the rest of storedBreakdown fields and
sanitizer usage (sanitizeMultiplier, costMultiplier, groupCostMultiplier)
unchanged so total == base_total * provider_multiplier * group_multiplier
remains stable.
In `@src/repository/provider-groups.ts`:
- Around line 35-50: multiplierCache currently has no capacity limit and only
cleared by invalidateGroupMultiplierCache, so expired keys can accumulate;
update functions interacting with multiplierCache (the Map named multiplierCache
and the lookup logic that uses CACHE_TTL_MS and CacheEntry) to prevent unbounded
growth by implementing either a simple max-size eviction (e.g., drop oldest or
random entry when size>MAX_CACHE_SIZE) or lazy cleanup on lookup (scan and
delete expired CacheEntry items before inserting/reading), and ensure
invalidateGroupMultiplierCache still clears the Map; pick one strategy and add a
MAX_CACHE_SIZE constant and eviction logic near where multiplierCache is
read/updated to keep memory bounded.
- Around line 211-247: The hot path issues are that getGroupCostMultiplier calls
findProviderGroupByName once per parsed group (N queries) and uses
rawGroupString as the cache key; fix by parsing and normalizing the groups first
(use parseProviderGroups(rawGroupString)), form a normalized cache key (e.g.,
parsedGroups.join(',')), check multiplierCache with that key, and on cache miss
perform a single batch DB query (implement or use a new
findProviderGroupsByNames(names: string[]) that does a single SELECT ... WHERE
name IN (...)) to fetch candidate rows; then iterate parsedGroups in declaration
order to pick the first hit (group.costMultiplier), cache only real hits under
the normalized key with expiresAt, and return 1.0 if none matched. Ensure you
update getGroupCostMultiplier to reference parseProviderGroups, multiplierCache,
and the new batch finder instead of calling findProviderGroupByName in a loop.
🪄 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: 9cc43933-d830-421a-9f00-a90380e57c6f
📒 Files selected for processing (12)
messages/en/dashboard.jsonmessages/ja/dashboard.jsonmessages/ru/dashboard.jsonmessages/zh-CN/dashboard.jsonmessages/zh-TW/dashboard.jsonsrc/actions/provider-groups.tssrc/app/[locale]/dashboard/logs/_components/error-details-dialog/components/SummaryTab.tsxsrc/app/[locale]/settings/providers/_components/provider-group-tab.tsxsrc/app/v1/_lib/proxy/response-handler.tssrc/lib/utils/cost-calculation.tssrc/repository/provider-groups.tstests/unit/repository/provider-groups.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- messages/ru/dashboard.json
- messages/zh-TW/dashboard.json
- messages/ja/dashboard.json
- tests/unit/repository/provider-groups.test.ts
- src/actions/provider-groups.ts
Comment on lines
+203
to
+211
| } else { | ||
| // Map known error codes to localized messages. | ||
| if (result.errorCode === "GROUP_IN_USE") { | ||
| toast.error(t("groupInUse")); | ||
| } else if (result.errorCode === "CANNOT_DELETE_DEFAULT") { | ||
| toast.error(t("cannotDeleteDefault")); | ||
| } else { | ||
| toast.error(result.error ?? t("deleteFailed")); | ||
| } |
There was a problem hiding this comment.
删除失败的兜底分支同样会直显英文。
deleteProviderGroup 未携带 errorCode 的失败路径（Unauthorized / Provider group not found / Failed to delete provider group）在第 210 行通过 result.error ?? t("deleteFailed") 泄漏到 UI。既然已对 GROUP_IN_USE 和 CANNOT_DELETE_DEFAULT 做了本地化映射，建议把默认分支也直接使用 t("deleteFailed")，不要把 result.error 当作用户可见文案。
♻️ 建议修改
if (result.errorCode === "GROUP_IN_USE") {
toast.error(t("groupInUse"));
} else if (result.errorCode === "CANNOT_DELETE_DEFAULT") {
toast.error(t("cannotDeleteDefault"));
} else {
- toast.error(result.error ?? t("deleteFailed"));
+ toast.error(t("deleteFailed"));
}保存流程 mapSaveError 的 fallback 同理可进一步收敛为纯 i18n 文案（当前 result.error 也可能是英文原串）。
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/`[locale]/settings/providers/_components/provider-group-tab.tsx
around lines 203 - 211, The default error branch in the provider group deletion
flow exposes raw English from result.error; update the else branch in
provider-group-tab.tsx (the deleteProviderGroup result handling) to never
display result.error directly—use toast.error(t("deleteFailed")) as the fallback
instead of toast.error(result.error ?? t("deleteFailed")). Also apply the same
i18n-only fallback approach to the save error mapping (mapSaveError) so no raw
result.error is shown to users; reference result.errorCode checks for
GROUP_IN_USE and CANNOT_DELETE_DEFAULT and keep those localized mappings as-is.
Comment on lines
+193
to
+200
| const referenceCount = await countProvidersUsingGroup(existing.name); | ||
| if (referenceCount > 0) { | ||
| return { | ||
| ok: false, | ||
| error: "Cannot delete a group that is still referenced by providers", | ||
| errorCode: "GROUP_IN_USE", | ||
| }; | ||
| } |
There was a problem hiding this comment.
countProvidersUsingGroup only checks the providers.groupTag column. If a group exists solely as a billing construct — multiplier ≠ 1 but zero providers tagged with it, while users or keys still have providerGroup = "<name>" — referenceCount returns 0, the deletion succeeds, and every affected user silently reverts to a 1.0× multiplier for all future requests. The Groups tab displays providerCount = 0, so the admin has no signal that users are assigned.
At minimum, count users and keys that reference the group before allowing deletion:
// In repository/provider-groups.ts — add alongside countProvidersUsingGroup
export async function countUsersAndKeysUsingGroup(name: string): Promise<number> {
const [usersResult, keysResult] = await Promise.all([
db.select({ count: sql<number>`count(*)` }).from(users).where(eq(users.providerGroup, name)),
db.select({ count: sql<number>`count(*)` }).from(keysTable).where(eq(keysTable.providerGroup, name)),
]);
return Number(usersResult[0]?.count ?? 0) + Number(keysResult[0]?.count ?? 0);
}Then guard deletion:
const userCount = await countUsersAndKeysUsingGroup(existing.name);
if (userCount > 0) {
return { ok: false, error: "Cannot delete a group that is assigned to users or keys", errorCode: "GROUP_IN_USE" };
}Alternatively, show a user count in the Groups tab alongside providerCount so admins know the impact before clicking delete.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/provider-groups.ts
Line: 193-200
Comment:
**Silent billing change when deleting a users-only group**
`countProvidersUsingGroup` only checks the `providers.groupTag` column. If a group exists solely as a billing construct — multiplier ≠ 1 but zero providers tagged with it, while users or keys still have `providerGroup = "<name>"` — `referenceCount` returns 0, the deletion succeeds, and every affected user silently reverts to a 1.0× multiplier for all future requests. The Groups tab displays `providerCount = 0`, so the admin has no signal that users are assigned.
At minimum, count users and keys that reference the group before allowing deletion:
```typescript
// In repository/provider-groups.ts — add alongside countProvidersUsingGroup
export async function countUsersAndKeysUsingGroup(name: string): Promise<number> {
const [usersResult, keysResult] = await Promise.all([
db.select({ count: sql<number>`count(*)` }).from(users).where(eq(users.providerGroup, name)),
db.select({ count: sql<number>`count(*)` }).from(keysTable).where(eq(keysTable.providerGroup, name)),
]);
return Number(usersResult[0]?.count ?? 0) + Number(keysResult[0]?.count ?? 0);
}
```
Then guard deletion:
```typescript
const userCount = await countUsersAndKeysUsingGroup(existing.name);
if (userCount > 0) {
return { ok: false, error: "Cannot delete a group that is assigned to users or keys", errorCode: "GROUP_IN_USE" };
}
```
Alternatively, show a user count in the Groups tab alongside `providerCount` so admins know the impact before clicking delete.
How can I resolve this? If you propose a fix, please make it concise.
ding113
added a commit
that referenced
this pull request
Apr 17, 2026
Phase 1–4 of IP recording & audit-log feature: Schema: - Add `client_ip` column to `message_request` and `usage_ledger` (plus a filtered index on message_request). The `fn_upsert_usage_ledger` trigger is updated (in-migration) to propagate `client_ip` and the previously-unwired `group_cost_multiplier` from message_request. - Add `ip_extraction_config` (jsonb) and `ip_geo_lookup_enabled` (bool) to `system_settings`. - New `audit_log` table with indexes for category/operator/target/time keyset pagination. - Also bundles the orphaned schema delta from #1025 (provider_groups table, group_cost_multiplier, cost_breakdown) which was committed to schema.ts without a matching migration; migration 0089 brings the on-disk state back in sync. Unified IP extractor (src/lib/ip/): - `extractClientIp(headers, config)` — configurable rule chain supporting leftmost / rightmost / 0-based-index picks for XFF-style headers, with IPv4/IPv6 parsing, port/bracket stripping, and graceful fallback. - `isPrivateIp(ip)` — RFC 1918, loopback, ULA, link-local, CGN classifier. - `getClientIp(headers)` convenience wrapper that reads system settings' `ipExtractionConfig` from the in-memory cache. - 43 unit tests covering header variants, XFF picks, invalid input, IPv6, case-insensitivity, and settings integration. Proxy pipeline: - `ProxySession.clientIp` is populated by `ProxyAuthenticator` using the new unified extractor (replaces the ad-hoc prefer-x-real-ip-then-XFF logic in auth-guard.ts). - `ProxyMessageService` forwards the IP into `createMessageRequest`; the repository persists it and includes it in SELECT paths. - `/api/auth/login` drops its copy of the extractor and uses the unified util too (still respects platform-provided IP first). IP geolocation client (src/lib/ip-geo/): - `lookupIp(ip, { lang })` — Redis-cached (TTL configurable, 3600s default), skips upstream for private IPs, caches negative results for 60s, aborts via AbortController on timeout, never throws. - Env: `IP_GEO_API_URL` (default https://ip.api.claude-code-hub.app), `IP_GEO_API_TOKEN` (optional Bearer), `IP_GEO_CACHE_TTL_SECONDS`, `IP_GEO_TIMEOUT_MS`. - `/api/ip-geo/:ip` route exposes the client to the dashboard for authenticated users; gated by the new `ipGeoLookupEnabled` system setting. - 8 tests covering cache hits, Bearer auth, private short-circuit, upstream 5xx / network / malformed / timeout fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ding113
added a commit
that referenced
this pull request
Apr 17, 2026
* feat(ip): schema + unified IP extraction + IP geolocation client Phase 1–4 of IP recording & audit-log feature: Schema: - Add `client_ip` column to `message_request` and `usage_ledger` (plus a filtered index on message_request). The `fn_upsert_usage_ledger` trigger is updated (in-migration) to propagate `client_ip` and the previously-unwired `group_cost_multiplier` from message_request. - Add `ip_extraction_config` (jsonb) and `ip_geo_lookup_enabled` (bool) to `system_settings`. - New `audit_log` table with indexes for category/operator/target/time keyset pagination. - Also bundles the orphaned schema delta from #1025 (provider_groups table, group_cost_multiplier, cost_breakdown) which was committed to schema.ts without a matching migration; migration 0089 brings the on-disk state back in sync. Unified IP extractor (src/lib/ip/): - `extractClientIp(headers, config)` — configurable rule chain supporting leftmost / rightmost / 0-based-index picks for XFF-style headers, with IPv4/IPv6 parsing, port/bracket stripping, and graceful fallback. - `isPrivateIp(ip)` — RFC 1918, loopback, ULA, link-local, CGN classifier. - `getClientIp(headers)` convenience wrapper that reads system settings' `ipExtractionConfig` from the in-memory cache. - 43 unit tests covering header variants, XFF picks, invalid input, IPv6, case-insensitivity, and settings integration. Proxy pipeline: - `ProxySession.clientIp` is populated by `ProxyAuthenticator` using the new unified extractor (replaces the ad-hoc prefer-x-real-ip-then-XFF logic in auth-guard.ts). - `ProxyMessageService` forwards the IP into `createMessageRequest`; the repository persists it and includes it in SELECT paths. - `/api/auth/login` drops its copy of the extractor and uses the unified util too (still respects platform-provided IP first). IP geolocation client (src/lib/ip-geo/): - `lookupIp(ip, { lang })` — Redis-cached (TTL configurable, 3600s default), skips upstream for private IPs, caches negative results for 60s, aborts via AbortController on timeout, never throws. - Env: `IP_GEO_API_URL` (default https://ip.api.claude-code-hub.app), `IP_GEO_API_TOKEN` (optional Bearer), `IP_GEO_CACHE_TTL_SECONDS`, `IP_GEO_TIMEOUT_MS`. - `/api/ip-geo/:ip` route exposes the client to the dashboard for authenticated users; gated by the new `ipGeoLookupEnabled` system setting. - 8 tests covering cache hits, Bearer auth, private short-circuit, upstream 5xx / network / malformed / timeout fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(audit): audit-log infra + login & admin-mutation audit hooks Phase 5–7: Audit infrastructure (src/lib/audit/): - `request-context.ts` — AsyncLocalStorage holding operator IP + UA. Populated by the action adapter once per request so audit hooks don't re-parse headers. - `redact.ts` — deep-copy with sensitive keys (key/apiKey/token/secret/ password/authorization/webhook_secret) replaced by `[REDACTED]`; extraKeys passed through by callers (e.g. provider.key, key.key). - `with-audit.ts` — generic wrapper that snapshots before/after, emits a success row on completion and a failure row + rethrow on exception. - `emit.ts` — one-shot helper used by action authors who prefer inline success/failure emits over function wrapping. Tolerant of partial `@/lib/auth` mocks in tests (falls back to null session). Audit-log repository (src/repository/audit-log.ts): - `createAuditLogAsync` — fire-and-forget insert; never blocks or fails the hot path. - `listAuditLogs` — keyset-paginated read with category/operator/target/time filters. - `getAuditLog`, `countAuditLogs` — single-row + count helpers. Action adapter (src/lib/api/action-adapter-openapi.ts): - Populate `runWithRequestContext` with operator IP + UA around every action execution. Defensive against missing Hono context fields (tests may mock `c.req` with only a subset of methods). Login audit (src/app/api/auth/login/route.ts): - `auth.login.success` on every successful admin-token or user-key login (operator identity captured from the resolved session). - `auth.login.failure` on missing key, invalid key. - `auth.login.rate_limited` on `LoginAbusePolicy` deny. Mutation audit (src/actions/*.ts): - users.ts: addUser / editUser / removeUser - providers.ts: addProvider / editProvider / removeProvider - provider-groups.ts: createProviderGroup / updateProviderGroup / deleteProviderGroup - keys.ts: addKey / editKey / removeKey (raw key never logged; redactor strips `key` from before/after) - system-config.ts: saveSystemSettings (full before/after snapshot) - notifications.ts: updateNotificationSettingsAction - sensitive-words.ts: create / update / delete - model-prices.ts: upsertSingleModelPrice / deleteSingleModelPrice / uploadPriceTable / syncLiteLLMPrices Tests: +11 unit tests for redact + withAudit (`src/lib/audit/*.test.ts`). Full suite: 4468 pass, 1 pre-existing failure unrelated to this change (`tests/api/api-endpoints.test.ts` 健康检查 — fails on dev before this PR because the test environment lacks DSN). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(ui): IP details dialog, request-detail IP display, settings form + i18n Phase 8 (partial): IP details dialog (src/app/[locale]/dashboard/_components/ip-details-dialog.tsx): - Reusable Radix Dialog that renders the full `IpGeoLookupResult` from the `/api/ip-geo/:ip` endpoint: country + flag, region / city / postal / coordinates, timezone, ASN + organization + route + RIR, privacy badges (VPN / Proxy / Tor / Relay / Threat / Clean), threat score + risk level, and abuse contact. - useQuery is gated to `open &&` — tests that never open the dialog don't need a QueryClientProvider in their harness. Request-detail drawer (SummaryTab / ErrorDetailsDialog): - Show client IP alongside User-Agent and Endpoint in the client-info card. - IP renders as a clickable underlined button that opens the IP-details dialog. - `clientIp` plumbed through ErrorDetailsDialog → SummaryTab via the TabSharedProps type, and populated from the usage-log rows. System settings form (src/app/[locale]/settings/config/_components/system-settings-form.tsx): - New "IP logging & extraction" section with: - Toggle for `ipGeoLookupEnabled` - JSON textarea for `ipExtractionConfig` + "Reset to default" button - Helper text describing the fallback chain semantics - Wired through `saveSystemSettings`, `UpdateSystemSettingsSchema`, and the system-config repo update/returning columns. i18n: - New `ipDetails` and `auditLogs` namespaces for all 5 locales (en / zh-CN / zh-TW / ja / ru), registered in each locale's index. - New `ipLogging` section under `settings/config.json` for all 5 locales. Repo/types: - `UsageLogRow.clientIp` added; select list on both messageRequest and ledger-fallback query paths updated. - `messageRequest.clientIp` + `usageLedger.clientIp` already added in phase 1. Tests: test fixtures for usage-logs tables updated with `clientIp: null`. Full suite: 4468 pass, 1 pre-existing unrelated failure (tests/api/api-endpoints.test.ts 健康检查 — fails on dev because the test environment lacks DSN, unchanged by this PR). Not yet included in this commit: audit-log dashboard page, IP column in usage-logs table. Those land in phase 9 if time permits; the column in the request-detail drawer already unblocks IP visibility. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(ui): audit-logs dashboard page + IP column on usage logs Phase 8 (complete): Audit logs dashboard: - `src/actions/audit-logs.ts` — admin-only server action pair (`getAuditLogsBatch` + `getAuditLogDetail`) wrapping the existing audit-log repository. ISO date strings on the filter converted to `Date` internally. - `src/app/[locale]/dashboard/audit-logs/page.tsx` — admin-gated server page (redirects non-admins to /dashboard). - `_components/audit-logs-view.tsx` — virtualized TanStack-Query infinite list with category + status filters; IP clicks open `IpDetailsDialog`; row clicks open the detail sheet. - `_components/audit-log-detail-sheet.tsx` — Sheet showing full entry: category, action, target, operator (name + key name + user id with "Admin Token" fallback), IP (clickable), user-agent, error message, and before/after JSON in `<pre>` blocks. - Registered both actions in `src/app/api/actions/[...route]/route.ts` (admin-only). - Added admin-only "Audit logs" nav entry in `dashboard-header.tsx`. IP column on usage logs table: - Non-virtualized (`usage-logs-table.tsx`) and virtualized (`virtualized-logs-table.tsx`) both gain a new IP column between sessionId and provider. Cell is a clickable underlined monospace IP that opens `IpDetailsDialog`; null → `—`. Empty-row colSpan bumped. - `src/lib/column-visibility.ts` — added `"ip"` to the column union + default visible set. - `column-visibility-dropdown.tsx` — added the `ip` label key. Audit-log repository — `createAuditLogAsync` is now an `async` function (returns a Promise callers should `void`) so the file satisfies Next's "use server" constraint. Behaviour unchanged: still fire-and-forget, still swallows errors with a warn-level log. Fixes a production-build failure. i18n: - `dashboard.json` gains `nav.auditLogs` + `logs.columns.ip` across all 5 locales. Pre-commit (per CLAUDE.md): - `bun run build` ✓ (was failing before the async fix; now clean). - `bun run typecheck` ✓. - `bun run lint` ✓ (20 pre-existing warnings, no errors). - `bun run test` — 4468 pass, 1 pre-existing unrelated failure (`tests/api/api-endpoints.test.ts 健康检查`, fails on dev too: needs DSN in test env). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: address PR #1027 review comments (security + i18n + robustness) Security / correctness: - P1: remove `cf-connecting-ip` from DEFAULT_IP_EXTRACTION_CONFIG. Trusted by default it was spoofable in non-CF deployments (login / pre-auth rate-limit bypass). Operators fronted by Cloudflare add it explicitly. - Critical: `fn_upsert_usage_ledger` no longer overwrites `created_at` on conflict. Ledger rows' insert time is immutable by design. - Major: `redactSensitive()` now only walks POJOs — Date / Map / Buffer / class instances pass through intact instead of being rewritten to `{}`. - Major: fix `webhookSecret` redaction. DEFAULT_SENSITIVE_KEYS was mixed- case vs the lowercased walker; camelCase webhook secrets slipped through unscrubbed. All keys normalized to lowercase; also added `api-key` and `webhook-secret` variants. +3 tests. - Restrict `/api/ip-geo/:ip` to admin (IP lookup is an operator tool; non-admins going through it would scan IPs against our upstream quota). - Never write raw `error.message` into `audit_log.error_message`. Pg errors can carry SQL fragments, constraint names, or user input (duplicate key values). All mutation actions now persist a stable `UPDATE_FAILED` / `CREATE_FAILED` / `DELETE_FAILED` / `SYNC_FAILED` code instead. - Stricter IP-geo upstream shape validation. Cached-for-1h payloads must now carry `ip`, `location.country.{code,name}`, and `connection.asn`; partial drifts fall into the 60s negative cache instead of blowing up the dashboard dialog. Audit robustness: - Remove dead `withAudit` wrapper. Production only uses `emitActionAudit`, and the wrapper's own `extractAfter` / `redactSensitive` lived inside the success-path try block and could turn a completed mutation into a thrown exception. Flagged by two reviewers. - `emitActionAudit` uses a new `resolveRequestContext()` that falls back to `next/headers` when the OpenAPI adapter's AsyncLocalStorage is empty. This captures operator IP / User-Agent for direct Next.js Server Actions (e.g. the settings form's `startTransition`) that bypass the adapter. - `editUser`: before-snapshot + after-snapshot via `safeFindUser()` (never throws — audit is best-effort, must not break the mutation). - Explicit `void` on `createAuditLogAsync(...)` to mark fire-and-forget intent rather than leaving a floating promise. UX / i18n: - System settings: invalid IP-extraction JSON (or wrong shape) now blocks save with a localized toast error rather than silently reverting to the default chain. `ipLoggingInvalidJson` / `ipLoggingInvalidShape` keys in all 5 locales. - `risk_level` rendered from upstream now goes through `ipDetails.riskLevels.*` (none/low/medium/high/critical) in all 5 locales; unknown future enums fall back to the raw string. - Localize all hardcoded strings in audit dashboard: "Admin Token" fallback, "User-Agent" row label, "id/key/user id:" label prefixes, "Error" loader fallback, `auditLogs.adminTokenOperator` / `loadError` / `errors.*`. - `auditLogs` actions use `getTranslations("errors")` + `ERROR_CODES` so non-admin / failure paths speak the operator's language and expose a stable `errorCode` for the UI. Smaller things: - `/api/ip-geo/:ip` accepts `?lang=`; dashboard's `useIpGeo` forwards the current locale so country/city names come back localized. - `useIpGeo` route params no longer double-decoded (Next already decodes path params; the old `decodeURIComponent` would throw on a lone `%`). - `countAuditLogs` now accepts the same filter set as `listAuditLogs` so paginated counts stay consistent with page contents. - `audit-logs-view` virtualizer `itemCount` includes a trailing loader row when `hasNextPage` is true, so the in-list loader actually renders. - Replace `../../_components/ip-details-dialog` relative imports with the `@/` alias across logs + audit-log views. - Default `IP_GEO_API_URL` switched to `https://ip-api.claude-code-hub.app`. Pre-commit: typecheck ✓ / lint ✓ (20 pre-existing warnings, 0 errors) / build ✓ / test 4466 pass, 1 pre-existing unrelated failure (`tests/api/api-endpoints.test.ts 健康检查`, fails on dev too — missing DSN in test env). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(ip-geo): accept partial payloads + hide null fields in IP dialog Upstream returns null for many fields when the IP is CGN / bogon / Tailscale (100.64/10, 198.18/15, etc.) — concrete example from prod: { "ip": "100.85.244.112", "location": { "country": { "code": "ZZ", "name": "Unknown", ... }, "region": null, "city": null, "postal_code": null, "coordinates": { "latitude": 0, "longitude": 0, "accuracy_radius_km": null } }, "connection": { "asn": null, "handle": null, "route": null, "domain": null, "organization": "Carrier-Grade NAT RFC6598", "rir": "UNKNOWN", ... }, ... } Two problems before this commit: 1. `isValidLookupResult` required `connection.asn: number`, so the entire CGN payload was rejected as "unexpected shape" and negative-cached for 60s. Operators never saw the carrier/NAT org info we DO have. 2. The dialog rendered every field unconditionally, producing strings like `ASnull`, a bare "Route:" row with no value, and `0, 0` coordinates. Fixes: - Types: `IpGeoConnection.asn` and `route` are `string | number | null`; `IpGeoFlag.svg` and `png` are `string | null`. Mirrors reality. - Validator: require only the truly UI-critical subtree (`ip`, `location.country.{code,name}`, `timezone.id`, `connection` as an object). Nullable asn / route pass through. Payloads still missing country / timezone are still rejected. - Dialog hides rows when their value is null or the "unknown" sentinel: - `asn === null` → row hidden (instead of `ASnull`) - `route === null` → row hidden - `organization === null` → row hidden - `rir === "UNKNOWN"` → row hidden - `is_anycast === false` → row hidden (it's the common case) - Coordinates hidden when `accuracy_radius_km === null` OR lat/lng are both 0 — the API's "we don't actually know" signal. Pure helper `hasMeaningfulCoordinates()` is exported and unit-tested. Tests (+7 cases): - `client.test.ts`: the full CGN payload is accepted; partial payload missing top-level required subtree is still rejected. - `ip-details-dialog.test.tsx` (new, happy-dom): - `hasMeaningfulCoordinates()` across null accuracy / 0,0 null-island / real lat+lng / accuracy=0 cases (4 cases). - Full-render check with the CGN payload: asserts `ASnull` never appears, coordinates row hidden, `UNKNOWN` RIR hidden, `Anycast` row hidden when false, organization + hostname + timezone still present. Typecheck / lint / build / test pass (1 pre-existing unrelated failure). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: format code (feat-ip-logging-and-audit-8f35b00) * fix: follow-up review comments (round 3) After re-reading every inline review comment against current code, 7 were still valid and worth fixing: Schema: - `provider_groups.{created_at, updated_at}` gain NOT NULL in both schema.ts and the migration CREATE TABLE. Matches the `audit_log.created_at` pattern and prevents an explicit NULL slipping in via raw SQL. Drizzle emitted migration 0090 as the catch-up ALTER for installs that already ran 0089 with nullable columns; it's idempotent on fresh installs. i18n: - All 5 locale hint strings for `ipLogging.extractionConfigHint` now describe the actual safe default chain (`x-real-ip → x-forwarded-for rightmost`), not the old CF-first chain. `cf-connecting-ip` appears only inside the "custom example for Cloudflare deployments" JSON so users aren't misled about what the default trusts. users.ts: - `user.create` audit extracted to `emitUserCreateAudit(user)` and called from BOTH `addUser` and `createUserOnly` (the unified-edit-dialog create path). Previously `createUserOnly` emitted no audit at all. - Success-path `after` snapshot is now the full user record returned by `createUser()` (id, name, role, isEnabled, providerGroup, quota / limit fields, expiresAt, tags, allowed/blocked clients & models, …) instead of the partial shape. - `createUserOnly`'s catch block now emits a `CREATE_FAILED` failure row too. - `editUser` / `removeUser`: hoisted `safeFindUser(userId)` above the try/catch boundary so failure audit events also carry `before` snapshot + `targetName`. The helper never throws, so the hoist is safe. emit.ts: - Wrapped `emitAsync` body in try/catch. `createAuditLogAsync` and `resolveRequestContext` already swallow their own errors, but `redactSensitive()` walks caller-supplied values — a circular ref or a throwing getter would otherwise surface as an unhandled rejection at the `void emitAsync(...)` site. Caught errors are logged at `warn`. ip-geo/client.ts: - `isValidLookupResult` also requires `country.flag` to be an object with a string `emoji`. The dialog dereferences `.flag.emoji` unconditionally, so a drifted-upstream payload without the flag would render-crash. With the extra gate such payloads fall into the 60s negative cache instead. (Note: the review pointed at a phantom "second identical block around line 99-105" — that's the single `isValidLookupResult` caller in `fetchFromUpstream`, not a duplicate. Only one validator needed updating.) - +1 test: payload with flag but no `emoji` is rejected. audit-logs.ts: - `AUDIT_CATEGORY_VALUES` now uses `as const satisfies readonly AuditCategory[]` + an `Exclude<>`-based exhaustiveness type-check. Adding a variant to `AuditCategory` without listing it here becomes a TS error instead of a runtime-only drift. Pre-commit: typecheck ✓ / lint ✓ / build ✓ / test 4476 pass, 1 pre-existing unrelated failure (tests/api/api-endpoints.test.ts 健康检查). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
baseCost * providerMultiplier * groupMultiplier)Changes
Database
provider_groupstable (name, cost_multiplier, description)group_cost_multipliercolumn onmessage_requestandusage_ledgercost_breakdownjsonb column onmessage_requeststoring per-component costs at calculation timeCore Logic
calculateRequestCost()now applies compound multiplier:baseCost * providerMultiplier * groupMultiplierRequestCostCalculationOptionsgainsgroupMultiplierfield (default 1.0, backward compatible)ProxySessioncarriesgroupCostMultiplierthrough the proxy pipelineUI
i18n
Test plan
bun run db:generateto generate migration SQL, review and apply🤖 Generated with Claude Code
Greptile Summary
This PR adds compound cost multiplier support (provider × group) with a new
provider_groupstable, propagatesgroupCostMultiplierthrough the full proxy pipeline, stores a per-componentcost_breakdownjsonb for historical billing display, and adds a Groups management tab with i18n coverage across all 5 locales. The core calculation logic, sanitization, and multi-group parsing are well-implemented and thoroughly tested.deleteProviderGroupguards against providers referencing the group viacountProvidersUsingGroup, but does not checkusers.provider_grouporkeys.provider_group. A billing-only group (multiplier ≠ 1, providerCount = 0, users still assigned) can be silently deleted — the Groups tab showsproviderCount = 0, giving the admin no indication that users will lose their configured multiplier on the next request.Confidence Score: 4/5
Safe to merge after addressing the group-deletion guard — billing could silently revert to 1.0× for users in provider-less groups
All core cost calculation, multiplier sanitization, multi-group parsing, and i18n changes are correct and well-tested. The one P1 finding is confined to the admin delete flow in actions/provider-groups.ts and does not affect the hot request path.
src/actions/provider-groups.ts — deleteProviderGroup pre-check needs to include users and keys, not only providers
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Incoming Request] --> B[GuardPipeline] B --> C[ProxyProviderResolver.resolve] C --> D{session.provider set?} D -->|No| E[pickRandomProvider] E --> F[session.setProvider] F --> G D -->|Yes - retry| G[getEffectiveProviderGroup] G --> H{effectiveGroup?} H -->|No| I[groupCostMultiplier = 1.0] H -->|Yes| J[getGroupCostMultiplier - TTL cache] J --> K{Cache hit?} K -->|Yes| L[return cached value] K -->|No| M[findProviderGroupByName per group] M --> N{Row found?} N -->|Yes - cache it| L N -->|No| O[return 1.0 uncached] L --> P[session.setGroupCostMultiplier] O --> P I --> P P --> Q[ProxyForwarder - upstream API] Q --> R[ProxyResponseHandler] R --> S[calculateRequestCost base x providerMult x groupMult] S --> T[calculateRequestCostBreakdown per-component base costs] T --> U[updateMessageRequestCostWithBreakdown costUsd + costBreakdown jsonb] U --> V[DB: message_request + usage_ledger trigger]Comments Outside Diff (1)
src/lib/ledger-backfill/trigger.sql, line 26-46 (link)usage_ledger.group_cost_multiplieris populated entirely by this trigger. The standard migration workflow (bun run db:generate→bun run db:migrate) will add the column to the table but will not re-apply this trigger function. Iftrigger.sqlis not executed against the database separately, every newusage_ledgerrow will havegroup_cost_multiplier = NULL— silently, with no error.The
CREATE OR REPLACE FUNCTIONis idempotent, so the fix is to run this file after the migration, but that step is absent from the PR description's checklist. Please add it explicitly:Note: the
CREATE TRIGGERline at the bottom will fail if the trigger already exists, but that is harmless — the important part is theCREATE OR REPLACE FUNCTIONthat precedes it.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix: address second round of PR review c..." | Re-trigger Greptile