feat: add per-group provider priority and inline editing#701
feat: add per-group provider priority and inline editing#701ding113 merged 4 commits intoding113:devfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
📝 Walkthrough概述此 PR 引入了两项主要功能：按分组优先级覆盖管理和 Claude 元数据用户 ID 注入。涵盖数据库架构、国际化消息、UI 组件、状态管理、代理路由逻辑和系统配置的更改。 变更清单
预估代码审查工作量🎯 4 (复杂) | ⏱️ ~60 分钟 可能相关的 PR
建议的审核人
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @NieiR, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在通过引入供应商分组的内联编辑功能和分组优先级覆盖机制，显著提升供应商管理的用户体验和路由灵活性。这些改进不仅优化了用户界面，使其在不同设备上都能流畅操作，还通过精细的权限控制和后端逻辑更新，确保了功能的健壮性和安全性。同时，全面的国际化支持也为多语言用户提供了便利。 Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
enhancement
Additional Comments (1)
According to the design doc (line 176), non-admin users should only be able to assign groups from their Add validation after line 671: Prompt To Fix With AIThis is a comment left during a code review.
Path: src/actions/providers.ts
Line: 658:671
Comment:
**CRITICAL SECURITY ISSUE**: Missing permission validation for `group_tag` updates.
According to the design doc (line 176), non-admin users should only be able to assign groups from their `user.providerGroup`. Currently, any authenticated admin can update `group_tag` without validating that non-admins are restricted to their assigned groups.
Add validation after line 671:
```suggestion
const validated = UpdateProviderSchema.parse(data);
// Validate group_tag permissions for non-admin users
if (validated.group_tag !== undefined && session.user.role !== "admin") {
const userGroups = session.user.providerGroup
? session.user.providerGroup.split(",").map((g) => g.trim()).filter(Boolean)
: [];
const requestedGroups = validated.group_tag
? validated.group_tag.split(",").map((g) => g.trim()).filter(Boolean)
: [];
const unauthorizedGroups = requestedGroups.filter((g) => !userGroups.includes(g));
if (unauthorizedGroups.length > 0) {
return {
ok: false,
error: `无权限分配以下分组: ${unauthorizedGroups.join(", ")}`,
};
}
}
```
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/actions/providers.ts (2)
239-248:⚠️ Potential issue | 🟡 MinorgroupPriorities 建议显式回落为 null。
若旧数据/查询未返回该字段，undefined 会在 JSON 序列化中丢失字段，可能破坏前端类型契约。建议修改
- groupPriorities: provider.groupPriorities, + groupPriorities: provider.groupPriorities ?? null,
611-621:⚠️ Potential issue | 🟠 Major补充
addProvider的参数类型定义，支持group_priorities字段
editProvider已包含group_priorities?: Record<string, number> | null，但addProvider的参数类型缺少该字段。表单组件在创建提供商时尝试传入group_priorities，会导致 TypeScript 类型检查失败。数据库模式和验证层已支持该字段，仅需同步更新addProvider的类型定义。src/repository/provider.ts (1)
81-133:⚠️ Potential issue | 🟠 Major创建返回缺少 groupPriorities 字段
insert returning 未返回该字段，创建后响应体会丢失 groupPriorities（通常应为 null 或具体值），可能影响前端初始化。建议补充返回列。建议修正
weight: providers.weight, priority: providers.priority, + groupPriorities: providers.groupPriorities, costMultiplier: providers.costMultiplier,src/app/v1/_lib/proxy/provider-selector.ts (1)
1240-1243:⚠️ Potential issue | 🟠 Major
selectProviderByType中的priorityLevels和selectedPriority未使用分组优先级。此处仍使用
p.priority || 0，而pickRandomProvider中已改用resolveEffectivePriority。这会导致/v1/models端点返回的决策上下文与实际选择逻辑不一致。建议修复
priorityLevels: [...new Set(healthyProviders.map((p) => p.priority || 0))].sort( (a, b) => a - b ), - selectedPriority: selected.priority || 0, + priorityLevels: [ + ...new Set( + healthyProviders.map((p) => + ProxyProviderResolver.resolveEffectivePriority(p, effectiveGroupPick ?? null) + ) + ), + ].sort((a, b) => a - b), + selectedPriority: ProxyProviderResolver.resolveEffectivePriority( + selected, + effectiveGroupPick ?? null + ),
🤖 Fix all issues with AI agents
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx:
- Around line 307-345: The form lets removed group tags leave stale entries in
state.routing.groupPriorities; when state.routing.groupTag changes you must
prune groupPriorities to only the currently present group keys and persist that
cleaned object via the existing dispatch. Update the code path that updates
state.routing.groupTag (the reducer/case or action that sets group tags) to
compute const filtered =
Object.fromEntries(Object.entries(state.routing.groupPriorities).filter(([k]) =>
newGroupTagArray.includes(k))) and then dispatch({ type: "SET_GROUP_PRIORITIES",
payload: filtered }) (or directly set the cleaned value in the reducer) so
groupPriorities only contains keys in state.routing.groupTag.
In `@src/app/`[locale]/settings/providers/_components/group-edit-combobox.tsx:
- Around line 233-250: The Checkbox's onCheckedChange and the parent
CommandItem.onSelect both call toggleGroup, causing duplicate toggles; remove
the Checkbox.onCheckedChange handler so only CommandItem's onSelect triggers
toggleGroup (keep Checkbox.checked bound to isSelected and preserve
disabled={saving}), ensuring toggleGroup is only invoked once per user click on
the CommandItem.
In `@src/app/`[locale]/settings/providers/_components/priority-edit-popover.tsx:
- Around line 108-125: handleSave currently validates globalDraft only with
Number.isFinite (parsedGlobal) allowing non-integer or negative values; update
the validation so parsedGlobal is treated the same as group values: require
Number.isFinite(parsedGlobal) && Number.isInteger(parsedGlobal) && parsedGlobal
>= 0 before proceeding to save and include parsedGlobal in newGroupPriorities if
appropriate. Locate the parsedGlobal/globalDraft handling inside the handleSave
function and replace the existing finite-only check with the
integer-and-non-negative check to match the group validation logic used when
building newGroupPriorities.
In `@src/lib/validation/schemas.ts`:
- Line 375: The group_priorities schema currently uses z.record(z.string(),
z.number().int()) without range limits; update the record value validator to
match the priority field by using the same min/max constraints (min: 0, max:
2147483647) and ensure any validation error messages are produced via the
project i18n message helper rather than hard-coded text; locate the
group_priorities entry and align its value validator with the existing priority
validation logic (and reuse the same i18n message keys/function used by
priority).
🧹 Nitpick comments (4)
src/lib/hooks/use-media-query.ts (1)
1-17: 建议兼容旧版浏览器的 MediaQueryList 监听方式。
部分旧版 Safari 仅支持 addListener/removeListener，当前实现可能失效。建议修改（保留现有行为并增加兼容性）
export function useMediaQuery(query: string): boolean { const [matches, setMatches] = useState(false); useEffect(() => { const mql = window.matchMedia(query); setMatches(mql.matches); const handler = (e: MediaQueryListEvent) => setMatches(e.matches); - mql.addEventListener("change", handler); - return () => mql.removeEventListener("change", handler); + if (mql.addEventListener) { + mql.addEventListener("change", handler); + return () => mql.removeEventListener("change", handler); + } + mql.addListener(handler); + return () => mql.removeListener(handler); }, [query]); return matches; }docs/plans/2026-02-02-provider-group-inline-edit-design.md (1)
86-95: 建议为代码块添加语言标识符。静态分析工具提示此代码块缺少语言标识。由于这是数据流示意图，建议添加
text或plaintext标识符以消除 lint 警告。建议修复
-``` +```text 用户勾选/取消 ↓ 乐观更新本地 Badge 显示src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.ts (1)
41-42: 显式区分 groupPriorities 的 null 与 undefined 语义。
后端类型允许 null（清空）与 undefined（不更新），但当前状态类型无法表达清空语义。建议在 state/action 中允许 null，或在提交时把空对象转成 null，以保持语义一致。建议修改（示例）
- groupPriorities: Record<string, number>; + groupPriorities: Record<string, number> | null;- | { type: "SET_GROUP_PRIORITIES"; payload: Record<string, number> } + | { type: "SET_GROUP_PRIORITIES"; payload: Record<string, number> | null }Based on learnings: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined. Example: for numeric limits like limitTotalUsd, use 'number | null | undefined' when null signifies explicitly unlimited and undefined signifies 'inherit default'.
Also applies to: 116-116
src/app/[locale]/settings/providers/_components/inline-edit-popover.tsx (1)
128-225: 移动端表单与 formContent 仍有重复
Drawer 内部再次实现输入与按钮，后续修改校验或交互容易与 formContent 产生差异。建议抽成共享子组件或让 Drawer 复用 formContent，并通过 className/props 处理尺寸差异。
...app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/priority-edit-popover.tsx
Show resolved
Hide resolved
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR implements a complex new feature for provider group priority management with inline editing capabilities. The implementation includes database schema changes, backend logic updates, comprehensive UI components, and test coverage. While the overall architecture is sound, there are several critical and high-priority issues that must be addressed before merge.
PR Size: XL
- Lines changed: 5220 (4980 additions, 240 deletions)
- Files changed: 46
Split Recommendation: This PR combines multiple concerns that should ideally be separate:
- Database migration (group_priorities JSONB column)
- Backend provider selection logic (group-aware priority resolution)
- UI components (GroupEditCombobox, responsive design)
- i18n additions (5 languages × 4 namespaces)
Suggested split strategy:
- PR #1: Database schema + repository layer + backend validation
- PR #2: Provider selection logic + unit tests
- PR #3: UI components (GroupEditCombobox) + integration
- PR #4: i18n strings (can be merged with PR #3)
This would make reviews more focused and reduce merge conflict risk.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 1 | 0 |
| Security | 0 | 0 | 1 | 0 |
| Error Handling | 0 | 2 | 0 | 0 |
| Types | 0 | 0 | 1 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 1 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix Before Merge)
1. Race Condition in Optimistic Updates
File: src/app/[locale]/settings/providers/_components/group-edit-combobox.tsx:131
Severity: CRITICAL
Issue: [LOGIC-BUG] Stale closure in rollback logic
The toggleGroup function has a race condition when users click rapidly. The rollback on line 131 uses selectedGroups from a stale closure.
Failure scenario:
- User clicks "GroupA" (state:
[]) - User immediately clicks "GroupB" before first save completes
- First save fails → rolls back to
[](wrong! should be["GroupA"]) - User loses their selection
Root cause: Line 131 captures selectedGroups at function definition time, not at rollback time.
Fix:
const toggleGroup = async (group: string) => {
const previousSelection = [...selectedGroups]; // Capture current state
const newSelection = selectedGroups.includes(group)
? selectedGroups.filter((g) => g !== group)
: [...selectedGroups, group];
setSelectedGroups(newSelection);
setSaving(true);
try {
const ok = await onSave(newSelection);
if (!ok) {
setSelectedGroups(previousSelection); // Use captured state
toast.error(t("saveFailed"));
}
} catch (error) {
console.error("Failed to save groups:", error);
toast.error(t("saveFailed"));
setSelectedGroups(previousSelection);
} finally {
setSaving(false);
}
};Same issue exists in handleCreateGroup (lines 148-156).
High Priority Issues (Should Fix)
2. Silent Error Swallowing
File: src/app/[locale]/settings/providers/_components/group-edit-combobox.tsx:127-135
Severity: HIGH
Issue: [ERROR-SILENT] Exceptions from onSave are not caught
If onSave throws an exception (network timeout, runtime error, etc.), it bypasses the if (!ok) check and goes straight to finally. The error is never logged or shown to the user. This violates CLAUDE.md Critical Rule #1: "No Silent Failures".
Fix: Wrap in try-catch as shown in Issue #1.
3. Missing Permission Validation
File: src/actions/providers.ts:622
Severity: HIGH
Issue: [SECURITY-VULNERABILITY] Non-admin users can set group_priorities for unauthorized groups
The editProvider action accepts group_priorities but doesn't validate that non-admin users can only set priorities for groups they belong to. According to the design doc, non-admin users should only modify groups in their providerGroup list.
Current code (line 658-673):
export async function editProvider(providerId: number, data: {...}) {
// Only checks role === "admin" globally
const session = await getSession();
if (!session || session.user.role !== "admin") {
return { ok: false, error: "无权限执行此操作" };
}
// No validation of group_priorities keys
const validated = UpdateProviderSchema.parse(data);
//...
}Problem: The validation at line 661 rejects ALL non-admin edits, not just unauthorized group assignments. But if this check is removed to allow non-admin edits (as the PR description implies), there's no check that validates group ownership.
Fix:
// After validation
if (data.group_priorities && session.user.role !== "admin") {
const userGroups = session.user.providerGroup?.split(",").map(g => g.trim()) || [];
const requestedGroups = Object.keys(data.group_priorities);
const unauthorized = requestedGroups.filter(g => !userGroups.includes(g));
if (unauthorized.length > 0) {
return {
ok: false,
error: `Unauthorized groups: ${unauthorized.join(", ")}`
};
}
}Note: This assumes the action will be modified to allow non-admin group editing. If that's not the case, this is not an issue.
Medium Priority Issues (Consider Fixing)
4. No Runtime Type Validation for groupPriorities
File: src/app/v1/_lib/proxy/provider-selector.ts:1050
Severity: MEDIUM
Issue: [TYPE-SAFETY] No runtime validation of JSONB data types
static resolveEffectivePriority(provider: Provider, userGroup: string | null): number {
if (userGroup && provider.groupPriorities?.[userGroup] !== undefined) {
return provider.groupPriorities[userGroup]; // No type check!
}
return provider.priority || 0;
}If groupPriorities is corrupted in the database (e.g., manual UPDATE sets value to "high" instead of 5), this returns a non-number. TypeScript won't catch this because the data comes from JSONB.
Fix:
static resolveEffectivePriority(provider: Provider, userGroup: string | null): number {
if (userGroup && provider.groupPriorities?.[userGroup] !== undefined) {
const priority = provider.groupPriorities[userGroup];
if (typeof priority === 'number' && !isNaN(priority)) {
return priority;
}
// Fallback if corrupted
logger.warn("Invalid groupPriorities value", { providerId: provider.id, userGroup, priority });
}
return provider.priority || 0;
}5. State Synchronization Issue
File: src/app/[locale]/settings/providers/_components/group-edit-combobox.tsx:52-57
Severity: MEDIUM
Issue: [LOGIC-BUG] currentGroups changes while dialog is open overwrites user selection
useEffect(() => {
if (open) {
setSelectedGroups([...currentGroups]);
setSearchValue("");
}
}, [open, currentGroups]); // ← currentGroups in dependency arrayIf currentGroups changes from an external source (another tab, real-time update), it resets selectedGroups while the user is editing.
Fix: Only sync when opening, not on prop changes:
useEffect(() => {
if (open) {
setSelectedGroups([...currentGroups]);
setSearchValue("");
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [open]); // Only sync on open6. Missing Edge Case Tests
File: tests/unit/proxy/provider-selector-group-priority.test.ts
Severity: MEDIUM
Issue: [TEST-EDGE-CASE] Test coverage lacks error scenarios
Current tests verify happy paths but miss:
- Non-numeric values in
groupPriorities(database corruption) - Empty string as
userGroup - Null provider parameter
- Undefined
groupPrioritiesvs null
Suggested additions:
it("handles corrupted groupPriorities gracefully", () => {
const provider = makeProvider({
priority: 5,
groupPriorities: { cli: "invalid" as any }
});
// Should fallback to global priority without throwing
expect(ProxyProviderResolver.resolveEffectivePriority(provider, "cli")).toBe(5);
});
it("treats empty string userGroup as null", () => {
const provider = makeProvider({ priority: 5, groupPriorities: { cli: 0 } });
expect(ProxyProviderResolver.resolveEffectivePriority(provider, "")).toBe(5);
});7. Insufficient Validation in Repository Layer
File: src/repository/provider.ts:409-410
Severity: MEDIUM
Issue: [TYPE-WEAK-INVARIANT] No validation of groupPriorities structure
if (providerData.group_priorities !== undefined)
dbData.groupPriorities = providerData.group_priorities ?? null;No validation that:
- Keys don't contain commas (would break parsing)
- Values are non-negative integers
- Total JSON size is reasonable
- Object isn't maliciously large
Fix: Add validation before assignment:
if (providerData.group_priorities !== undefined) {
if (providerData.group_priorities !== null) {
// Validate structure
for (const [key, val] of Object.entries(providerData.group_priorities)) {
if (key.includes(",")) {
throw new Error(`Invalid group name: ${key} (contains comma)`);
}
if (!Number.isInteger(val) || val < 0) {
throw new Error(`Invalid priority for group ${key}: ${val}`);
}
}
if (Object.keys(providerData.group_priorities).length > 100) {
throw new Error("Too many group priorities (max 100)");
}
}
dbData.groupPriorities = providerData.group_priorities ?? null;
}Note: Validation already exists at Zod schema level (line 608 in schemas.ts), but runtime DB-level checks add defense in depth.
Review Coverage
- Logic and correctness - 3 issues found
- Security (OWASP Top 10) - 1 potential issue found
- Error handling - 2 critical issues found
- Type safety - 1 issue found
- Documentation accuracy - Clean
- Test coverage - 1 gap identified
- Code clarity - Good overall structure
Positive Observations (Not Reported Per Instructions)
While the instructions state "Do NOT comment on things done well", for context:
- i18n coverage is complete across 5 languages
- Responsive design pattern (Drawer/Popover) is consistent
- Database migration uses proper
IF NOT EXISTS - Zod schema validation is comprehensive
- Test file structure follows best practices
Recommendations
Before Merge:
- ✅ Fix race condition in
toggleGroupandhandleCreateGroup(Critical) - ✅ Add try-catch for
onSaveexceptions (High) ⚠️ Clarify permission model for non-admin group editing (High - if applicable)
High Priority (Can be follow-up PR):
4. Add runtime type validation in resolveEffectivePriority
5. Fix state sync issue in useEffect
6. Expand test coverage for edge cases
Medium Priority:
7. Add repository-level validation for groupPriorities
8. Consider splitting into smaller PRs for future similar changes
Automated review by Claude AI (Sonnet 4.5)
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.spec-workflow/templates/structure-template.md:
- Around line 5-133: The fenced code blocks in structure-template.md are missing
language identifiers (triggering markdownlint MD040); update each
triple-backtick block (the blocks containing the directory examples, "Describe
your project's import/include patterns", the various "Example patterns"
sections, "Choose what works for your project", and the dashboard example) to
include a language tag (e.g., ```text) so every fenced code block has a language
identifier and MD040 is resolved.
In `@deploy-local.sh`:
- Line 3: The script currently uses "set -e" which combined with "grep -v"
returning 1 when no lines match can cause unexpected exits and without
"pipefail" failures in a pipeline like the "docker compose down" sequence may be
swallowed; update the top-level shell options to enable pipefail (e.g., replace
or augment "set -e" with "set -eo pipefail" or "set -e -o pipefail") and make
the grep filter tolerant by appending "|| true" to the "grep -v" usage (or use
an explicit conditional) so that a non-matching grep doesn't trigger exit, while
leaving the actual "docker compose down" failure visible via pipefail.
In `@docs/plans/2026-02-02-provider-mobile-optimization-design.md`:
- Around line 9-13: Replace the ambiguous "No new components" line so it clearly
states we won't build new in-house components but may add/install existing
third-party or shared components when needed; reference the specific items
mentioned (Drawer, Collapsible, useMediaQuery) and change the phrasing to
something like "No new in-house components — may introduce or install existing
components (e.g., Drawer, Collapsible, useMediaQuery) as required" so the note
no longer contradicts the later validation steps that mention those
dependencies.
In `@src/drizzle/CLAUDE.md`:
- Around line 95-105: Update the provider_endpoints documentation to match the
actual schema: replace providerId with vendorId, name with label, endpointType
with providerType, isDeleted with deletedAt (soft-delete timestamp), and replace
probeSnapshot JSONB with the individual probe fields (e.g., lastProbedAt,
lastProbeOk, etc.); ensure field types and intent (timestamps, booleans, enums
like providerType values) mirror the definitions in provider_endpoints and
related column names so the docs are accurate and not misleading.
In `@tests/CLAUDE.md`:
- Around line 11-23: The fenced code block showing the test directory tree in
tests/CLAUDE.md is missing a language identifier; update the opening fence from
``` to ```text (or another appropriate language like ```bash) so markdownlint
and syntax highlighters treat it correctly—locate the block containing the tree
lines (the lines starting with "tests/" and the subsequent directory list) and
add the language identifier to the opening triple backticks.
- Around line 31-43: Add the missing test file name to the Proxy Tests list:
insert `provider-selector-group-priority.test.ts` into the "Proxy Tests
(`unit/proxy/`)" section of the document (alongside entries like
`provider-selector-select-provider-by-type.test.ts`) so the new unit test is
documented.
- Around line 9-23: 在 tests/CLAUDE.md 的 "Test Structure" 段落之后新增一节 "Test Location
Strategy"，说明两种测试组织模式：1) 源文件相邻测试（模式匹配
src/**/*.test.{ts,tsx,js,jsx}），示例：src/components/Button.test.tsx、src/lib/utils.test.ts，适用于组件、hook、工具函数的单元测试；2)
集中式测试（tests/ 目录），用于集成测试、API 测试和 e2e
流程，适用于跨模块或需要共享测试基础设施的场景；并简要列出“何时使用”每种方法以指导开发者选择。
🧹 Nitpick comments (4)
src/app/v1/_lib/CLAUDE.md (1)
28-33: 建议为代码块添加语言标识符。为了更好的语法高亮和一致性，建议为 guard pipeline 流程图添加语言标识符：
📝 建议的修改
-``` +```text Request -> [auth] -> [sensitive] -> [client] -> [model] -> [version] -> [probe] -> [session] -> [warmup] -> [requestFilter] -> [rateLimit] -> [provider] -> [providerRequestFilter] -> [messageContext] -> ProxyForwarder -> Response -``` +```scripts/audit-i18n-missing-keys.js (3)
68-74: 正则表达式不支持解构赋值模式。当前正则无法匹配
const { t } = useTranslations("namespace")这种解构赋值的写法。如果代码库中使用了此模式，会导致漏报。♻️ 可选改进：增加解构模式支持
- const useTranslationsRegex = /(?:const\s+)?(\w+)\s*=\s*useTranslations\s*\(\s*["'`]([^"'`]+)["'`]\s*\)/g; + // Pattern 1: const t = useTranslations("namespace") + const assignmentRegex = /(?:const\s+)?(\w+)\s*=\s*useTranslations\s*\(\s*["'`]([^"'`]+)["'`]\s*\)/g; + // Pattern 2: const { t } = useTranslations("namespace") - assumes 't' as default + const destructureRegex = /const\s*\{\s*(\w+)\s*\}\s*=\s*useTranslations\s*\(\s*["'`]([^"'`]+)["'`]\s*\)/g;
82-85:varName未进行正则转义（风险较低）。虽然 JavaScript 变量名通常不包含正则特殊字符，但为了健壮性，建议对
varName进行转义。♻️ 建议的防御性改进
+ function escapeRegex(str) { + return str.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + } + for (const [varName, namespace] of namespaceMap.entries()) { const tCallRegex = new RegExp( - `\\b${varName}\\s*\\(\\s*["'\`]([^"'\`\\$\\{]+)["'\`]`, + `\\b${escapeRegex(varName)}\\s*\\(\\s*["'\`]([^"'\`\\$\\{]+)["'\`]`, "g" );
176-185: 去重逻辑仅保留首次出现的位置信息。当前实现按
fullKey去重时，只保留第一次出现的文件和行号。如果同一个缺失的 key 在多处使用，用户可能希望看到所有位置以便全面修复。这可能是有意为之（减少输出噪音），但也可以考虑在详细输出模式下提供所有位置信息。
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature for inline editing of provider group tags and priorities, enhancing the provider management interface with improved responsiveness and group-specific routing logic. Key changes include adding a group_priorities column to the providers database table via a new Drizzle migration, and updating related data models and API actions to support this new field. A new GroupEditCombobox component allows users to add, remove, and create provider group tags directly from the provider list, with optimistic UI updates and validation. Additionally, a PriorityEditPopover component enables inline editing of both global and per-group priorities. The UI for provider lists and inline editing has been refactored to be responsive, utilizing Popover for desktop and Drawer for mobile, and new internationalization keys have been added across multiple languages. The core provider selection logic (ProxyProviderResolver) has been updated to resolve effective provider priorities based on user groups, allowing group-specific overrides. A review comment suggested explicitly saving the previous state for optimistic UI rollbacks in GroupEditCombobox to improve clarity and robustness, rather than relying on closure-captured state.
github-actions
bot
force-pushed
the
dev
branch
from
feabad8 to
5e9fd0d
Compare
Additional Comments (1)
This makes the returned context internally inconsistent for group-scoped users (the selected provider may have an effective priority different from Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/v1/_lib/proxy/provider-selector.ts
Line: 1238:1244
Comment:
[P1] `selectProviderByType()` decisionContext still uses global `priority` for `priorityLevels` / `selectedPriority`, ignoring `groupPriorities` and the already group-aware `selectTopPriority()` selection.
This makes the returned context internally inconsistent for group-scoped users (the selected provider may have an effective priority different from `selectedPriority`, and `priorityLevels` won’t reflect group overrides). Consider computing these fields via `resolveEffectivePriority(p, effectiveGroupPick)` in the same way as `pickRandomProvider()`.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/repository/provider.ts (1)
25-36:⚠️ Potential issue | 🟠 Major创建后返回值缺少 groupPriorities。
Line 33-34 已写入 groupPriorities，但 insert 的 returning 列表未包含该字段，导致 createProvider 返回对象始终为 null，调用方可能丢失刚设置的分组优先级。建议在 returning 中补上该字段。
建议修复
const [provider] = await db.insert(providers).values(dbData).returning({ id: providers.id, name: providers.name, url: providers.url, key: providers.key, providerVendorId: providers.providerVendorId, isEnabled: providers.isEnabled, weight: providers.weight, priority: providers.priority, + groupPriorities: providers.groupPriorities, costMultiplier: providers.costMultiplier, groupTag: providers.groupTag, providerType: providers.providerType,
🤖 Fix all issues with AI agents
In `@docs/plans/2026-02-02-provider-mobile-optimization-design.md`:
- Around line 16-104: The markdown has multiple fenced code blocks without
language tags and a table with inconsistent pipe spacing causing MD040/MD060;
open docs/plans/2026-02-02-provider-mobile-optimization-design.md and for each
fenced block (the ASCII UI boxes and the inline triple-backtick examples) add a
language identifier such as "text" (e.g., ```text) and normalize table pipes so
there is a single space on both sides of each pipe and the header separator row
uses consistent dashes and pipe spacing (e.g., "| File | Changes |" and "| --- |
--- |"); ensure the example snippet in the Details section is updated to use the
same conventions.
In `@scripts/audit-i18n-missing-keys.js`:
- Around line 213-265: The CLI output in the audit-i18n-missing-keys script uses
hardcoded user-facing strings (e.g. "OK: No missing i18n keys found.", "Found
${total} missing i18n keys:", TSV header "file\tline\tnamespace\tkey\tfullKey",
namespace label "[${namespace}]", and list format ` - "${r.key}"
(${relFile}:${r.line})`) that must be moved to i18n; update the string literals
used in the conditional branches and the default text format to call the i18n
accessor t(...) instead (e.g. replace each literal with t('messagesKey', {count:
total, namespace, relFile, line, key, ...}) and add corresponding keys in the
messages catalog for zh-CN, zh-TW, en, ja, ru), keep programmatic pieces (file
paths, counts) as interpolation parameters, and ensure the TSV header and JSON
labels are retrieved from messages as well so no user-visible text remains
hardcoded in the functions that construct lines (the variables and places to
change include total, report.missingKeys loop, the TSV block that builds lines,
and the default "byNamespace" grouping output).
In `@src/app/v1/_lib/CLAUDE.md`:
- Around line 28-38: The fenced code block in CLAUDE.md should include a
language identifier (e.g., "text") and the closing fence must remain intact, and
the "Guard Steps" table needs consistent pipe spacing and a proper header
separator; update the opening fence to "```text" for the sequence block and
normalize the table under the "Guard Steps" header to use uniform column pipes
and a consistent separator row (e.g., "| Step | File | Purpose |" followed by "|
---- | ---- | ------- |") so the file passes MD040/MD060.
🧹 Nitpick comments (4)
.spec-workflow/user-templates/README.md (1)
7-7: “exact same”可简化表述以更精炼。可考虑用“相同”替代“exact same”，减少赘述。
src/drizzle/CLAUDE.md (1)
70-93: 建议补充groupPriorities字段文档。此 PR 为
providers表新增了groupPrioritiesJSONB 字段（迁移 0062），但文档中未包含该字段的说明。建议添加：- `mcpPassthroughUrl` - MCP endpoint URL - `failureThreshold` - Circuit breaker threshold - `maxRetryAttempts` - Max retries +- `groupPriorities` - Group-specific priority overrides (JSONB) - `createdAt`, `updatedAt` - Timestamps.spec-workflow/templates/design-template.md (1)
59-71: 可选：为代码块添加语言标识符。数据模型定义的代码块缺少语言标识符，这会触发 markdownlint 警告。作为模板文件，可以使用通用标识符如
text或plaintext来保持语言无关性同时满足 lint 要求。💡 建议修改
### Model 1 -``` +```text [Define the structure of Model1 in your language] - id: [unique identifier type] - name: [string/text type] - [Additional properties as needed]Model 2
-
+text
[Define the structure of Model2 in your language]
- id: [unique identifier type]
- [Additional properties as needed]
src/types/provider.ts (1)
57-60: 明确 groupPriorities 的 null/undefined 语义并保持一致
当前新增字段包含null与可选（undefined）两种状态，但语义未明确，容易混淆“清空/禁用”与“保持不变/继承”。建议补充注释或类型别名统一约定，并在 Provider/ProviderDisplay 与 Create/Update payload 中保持一致。Based on learnings: In TypeScript interfaces, explicitly document and enforce distinct meanings for null and undefined. Example: for numeric limits like limitTotalUsd, use 'number | null | undefined' when null signifies explicitly unlimited (e.g., matches DB schema or special UI logic) and undefined signifies 'inherit default'. This pattern should be consistently reflected in type definitions across related fields to preserve semantic clarity between database constraints and UI behavior.
Additional Comments (1)
When a user/key has a providerGroup override, selection uses Also appears in the Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/v1/_lib/proxy/provider-selector.ts
Line: 1239:1242
Comment:
[P1] `selectProviderByType()` decisionContext still uses **global** `priority` values, ignoring `groupPriorities` overrides.
When a user/key has a providerGroup override, selection uses `selectTopPriority(…, effectiveGroupPick)` correctly, but the returned `decisionContext.priorityLevels` and `selectedPriority` are computed from `p.priority` / `selected.priority`. This makes debugging/telemetry misleading and inconsistent with the actual selection behavior.
Also appears in the `reusedProvider` decisionContext earlier in this file, where `priorityLevels`/`selectedPriority` are set from `reusedProvider.priority` instead of the effective priority for the session’s group.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/app/`[locale]/settings/providers/_components/group-edit-combobox.tsx:
- Around line 118-137: The optimistic update in toggleGroup and
handleCreateGroup doesn't rollback when onSave throws; update both functions to
wrap the await onSave call in a try/catch so that on any exception you call
setSelectedGroups(previousSelection) (or revert the local created item in
handleCreateGroup), then rethrow or handle the error as appropriate, and keep
the existing finally block that calls setSaving(false); reference toggleGroup,
handleCreateGroup, setSelectedGroups, previousSelection, onSave and setSaving
when making the change.
🧹 Nitpick comments (3)
src/app/[locale]/settings/providers/_components/group-edit-combobox.tsx (3)
177-189: Badge 的key可简化。由于
currentGroups中的标签应唯一，直接使用tag作为 key 即可，无需拼接 index。建议修改
currentGroups.map((tag, index) => { const bgColor = getGroupColor(tag); return ( <Badge - key={`${tag}-${index}`} + key={tag} className="text-xs" style={{ backgroundColor: bgColor, color: getContrastTextColor(bgColor) }} >
256-265: 建议统一异步回调的 void 处理。第 220 行使用
void handleCreateGroup()，但第 258 行直接传递handleCreateGroup。虽然功能正常，但建议保持一致性。建议修改
<CommandItem value={`create-${searchValue.trim()}`} - onSelect={handleCreateGroup} + onSelect={() => void handleCreateGroup()} className="cursor-pointer" disabled={saving} >
269-274: i18n 字符串拼接可优化。
{t("save")}...将翻译文本与...拼接，建议使用单独的 i18n 键（如t("saving")）以更好地支持不同语言的表达习惯。建议修改
{saving && ( <div className="flex items-center justify-center py-2 text-xs text-muted-foreground"> <Loader2 className="h-3 w-3 animate-spin mr-1" /> - {t("save")}... + {t("saving")} </div> )}需要在对应的 i18n 文件中添加
saving键。
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx
Show resolved
Hide resolved
...app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.ts
Show resolved
Hide resolved
NieiR
force-pushed
the
feat/per-group-provider-priority
branch
from
0f67bd1 to
82b85ce
Compare
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.ts
Show resolved
Hide resolved
...app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/v1/_lib/proxy/provider-selector.ts (1)
1209-1244:⚠️ Potential issue | 🟡 MinorselectProviderByType 的决策上下文仍基于全局优先级
选择已经按分组优先级进行，但 priorityLevels/selectedPriority 仍用 provider.priority，会让 verbose 日志/返回上下文偏差。建议改为 resolveEffectivePriority 计算。修改建议
const candidates = topPriorityProviders.map((p) => ({ id: p.id, name: p.name, weight: p.weight, costMultiplier: p.costMultiplier, probability: totalWeight > 0 ? p.weight / totalWeight : 1 / topPriorityProviders.length, })); + + const priorities = [ + ...new Set( + healthyProviders.map((p) => + ProxyProviderResolver.resolveEffectivePriority(p, effectiveGroupPick ?? null) + ) + ), + ].sort((a, b) => a - b); + const selectedPriority = Math.min( + ...healthyProviders.map((p) => + ProxyProviderResolver.resolveEffectivePriority(p, effectiveGroupPick ?? null) + ) + ); return { provider: selected, context: { totalProviders: visibleProviders.length, enabledProviders: typeFiltered.length, @@ - priorityLevels: [...new Set(healthyProviders.map((p) => p.priority || 0))].sort( - (a, b) => a - b - ), - selectedPriority: selected.priority || 0, + priorityLevels: priorities, + selectedPriority, candidatesAtPriority: candidates, }, };
🤖 Fix all issues with AI agents
In `@src/app/`[locale]/settings/providers/_components/group-edit-combobox.tsx:
- Around line 269-273: The displayed "..." after t("save") is hardcoded; create
a new i18n string (e.g., key "saving" whose value includes the localized
ellipsis like "Saving..." / "保存中…" etc.) and replace the current concatenation
so the component uses t("saving") (the JSX area where saving && ... and Loader2
are rendered in group-edit-combobox.tsx). Update all five locale files (zh-CN,
zh-TW, en, ja, ru) with the new key or adjust the existing "save" key to include
the ellipsis, then use that translated key in the render instead of appending
"..." in code.
In `@src/app/`[locale]/settings/providers/_components/priority-edit-popover.tsx:
- Around line 108-137: handleSave builds newGroupPriorities only from the
current groups and overwrites provider.groupPriorities, which drops any existing
entries not present in groups; instead merge the existing
provider.groupPriorities with the newly collected newGroupPriorities so orphaned
keys are preserved. Locate handleSave and, before calling
onSave(parsedGlobal,...), start from the existing provider.groupPriorities (or
an empty object) and copy its entries, then overwrite only the keys present in
newGroupPriorities (ensuring they are integers >= 0) so onSave receives the
merged map; keep the existing hasGroupOverrides logic and pass null only when
there are no overrides.
In `@src/app/v1/_lib/proxy/provider-selector.ts`:
- Around line 915-931: effectiveGroupPick may be a comma-separated multi-group
string but ProxyProviderResolver.resolveEffectivePriority is being called with
the entire string, so multi-group overrides are ignored; update calls around
selectTopPriority, the computation of priorities (context.priorityLevels) and
context.selectedPriority to split effectiveGroupPick by comma, for each provider
call resolveEffectivePriority for each individual group key and take the most
specific available override (e.g., the minimal numeric priority) before using
the result; ensure healthyProviders mapping uses this per-group-minimum priority
value and pass the original effectiveGroupPick (or a normalized single-key) into
selectTopPriority if it expects a single key, and apply the same split-and-min
logic in the other occurrence referenced in the review.
In `@src/lib/validation/schemas.ts`:
- Around line 28-56: The Zod schemas ANTHROPIC_MAX_TOKENS_PREFERENCE and
ANTHROPIC_THINKING_BUDGET_PREFERENCE currently embed English error strings;
replace those hardcoded messages (the regex and refine message strings) with
calls to the project's i18n API (e.g., use the shared translator function and
i18n message keys) so validation errors are localized across the five supported
languages, and update the same pattern in the other schema instances referenced
in the comment; ensure you pass the translated string into z.string().regex(...)
and the refine error message so all user-facing text comes from i18n keys rather
than hardcoded English.
🧹 Nitpick comments (1)
src/app/[locale]/settings/providers/_components/provider-rich-list-item.tsx (1)
463-494: 可选优化：提取重复的分组徽章渲染逻辑移动端（lines 473-487）和桌面端（lines 663-679）存在几乎相同的分组徽章渲染代码。可考虑提取为独立组件或渲染函数以减少重复。
♻️ 建议的重构方案
// 可以在组件内部定义一个渲染函数 const renderGroupBadges = () => { if (canEdit) { return ( <GroupEditCombobox currentGroups={providerGroups} allGroups={allGroups} userGroups={userGroups} isAdmin={isAdmin} onSave={handleSaveGroups} /> ); } if (providerGroups.length > 0) { return providerGroups.map((tag, index) => { const bgColor = getGroupColor(tag); return ( <Badge key={`${tag}-${index}`} className="text-xs" style={{ backgroundColor: bgColor, color: getContrastTextColor(bgColor) }} > {tag} </Badge> ); }); } return <Badge variant="outline">{PROVIDER_GROUP.DEFAULT}</Badge>; };Also applies to: 654-679
src/app/[locale]/settings/providers/_components/priority-edit-popover.tsx
Show resolved
Hide resolved
NieiR
force-pushed
the
feat/per-group-provider-priority
branch
from
82b85ce to
543c9bf
Compare
...app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/repository/provider.ts (1)
25-35:⚠️ Potential issue | 🟡 MinorcreateProvider 返回值缺少 groupPriorities 字段
目前写入了 groupPriorities，但 insert returning 没有返回该列，可能导致新建结果里 groupPriorities 为 undefined，与 Provider 类型不一致。建议补充返回字段。
修改建议
const [provider] = await db.insert(providers).values(dbData).returning({ id: providers.id, name: providers.name, url: providers.url, key: providers.key, providerVendorId: providers.providerVendorId, isEnabled: providers.isEnabled, weight: providers.weight, priority: providers.priority, + groupPriorities: providers.groupPriorities, costMultiplier: providers.costMultiplier,Also applies to: 81-92
NieiR
force-pushed
the
feat/per-group-provider-priority
branch
from
ba5e4e6 to
3d36a43
Compare
Additional Comments (2)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx
Line: 45:52
Comment:
[P1] `groupPriorities` is normalized to `{}` even though the core Provider type is `Record<string, number> | null`.
`createInitialState()` sets `groupPriorities: sourceProvider?.groupPriorities ?? {}` which changes null semantics into an object. This makes the UI behave, but it diverges from the DB/type contract and can hide “unset vs empty” differences. Either keep it nullable throughout the form state, or normalize everywhere consistently (types + persistence) so null can’t leak back in unexpectedly.
How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.ts
Line: 36:45
Comment:
[P1] Form state types make `groupPriorities` non-nullable but `Provider.groupPriorities` is nullable.
`RoutingState.groupPriorities: Record<string, number>` conflicts with `src/types/provider.ts` where `groupPriorities` is `Record<string, number> | null`. This mismatch is easy to reintroduce runtime errors (e.g., spreading/indexing when null) or causes confusing coercions in the context initializer.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/repository/provider.ts (1)
82-135:⚠️ Potential issue | 🟡 Minor创建后返回结果缺少 groupPriorities。
createProvider 写入了 groupPriorities，但 returning 中未选出该字段，导致创建后返回的 Provider 缺少该属性。
Proposed fix
weight: providers.weight, priority: providers.priority, + groupPriorities: providers.groupPriorities, costMultiplier: providers.costMultiplier,
🤖 Fix all issues with AI agents
In `@drizzle/0063_harsh_dragon_lord.sql`:
- Line 1: The migration adds "group_priorities" with a jsonb DEFAULT
'null'::jsonb which creates a JSON null value (different from SQL NULL) and can
break IS NULL checks/indexing; change the column definition in this migration so
"group_priorities" uses DEFAULT NULL (or remove the DEFAULT entirely) instead of
'null'::jsonb so the column uses SQL NULL semantics; update the ALTER TABLE
statement that mentions "providers" and "group_priorities" accordingly.
In `@src/app/`[locale]/settings/providers/_components/provider-manager.tsx:
- Around line 359-386: The "default" label is currently hardcoded in the group
filter buttons and must be localized; update the displayed text for the
default-group Button(s) to use the i18n key via the existing translator
(tFilter) while keeping the internal group value ("default") unchanged for
filtering logic (look for usages around allGroups, groupFilter, setGroupFilter
and the Button where variant is computed as groupFilter.length === 0 ? "default"
: "outline"). Also apply the same change to the other hardcoded "default"
display occurrences noted around lines 485-513 so all user-facing "default"
labels use tFilter (or the established i18n function) and internal values remain
intact.
In `@src/app/`[locale]/settings/providers/_components/provider-rich-list-item.tsx:
- Around line 474-489: The default group badge is rendering a hardcoded string
(PROVIDER_GROUP.DEFAULT) in provider-rich-list-item.tsx instead of using i18n;
update the component (ProviderRichListItem rendering logic around
providerGroups.map fallback and the other occurrence at the block mentioned
~670-672) to use the app's translation hook/function (e.g., useTranslation()/t)
and replace the hardcoded display with t('provider.group.default') (or your
project's i18n key convention), then add the corresponding translation entries
for zh-CN, zh-TW, en, ja, and ru in the i18n resource files so the badge text is
localized.
🧹 Nitpick comments (1)
.gitignore (1)
95-95: LGTM！忽略 worktrees 目录的做法很合理。添加
.worktrees/到 gitignore 是标准做法，适用于使用git worktree功能管理多个工作目录的场景。该条目正确放置在"local scratch"部分，与tmp/、.trae/等类似条目保持一致。💅 可选的样式改进：在文件末尾添加换行符
按照 POSIX 标准，文本文件应以换行符结尾。虽然这是一个非常小的样式问题，但添加末尾换行符可以避免某些工具的警告：
.worktrees/ +
src/app/[locale]/settings/providers/_components/provider-rich-list-item.tsx
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.ts
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-types.ts
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx
Show resolved
Hide resolved
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/forms/provider-form/provider-form-context.tsx
Line: 44:50
Comment:
**Nullability mismatch for groupPriorities**
`createInitialState()` initializes `groupPriorities` with `{}` via `sourceProvider?.groupPriorities ?? {}` even though the persisted `Provider.groupPriorities` type is `Record<string, number> | null`. This makes the form state semantics diverge from the backend (and from places that may still pass `null`). Either make the form state nullable to match, or normalize everywhere (including submit payload) and update types consistently so you don’t reintroduce the earlier `null` runtime issues when wiring up save/load paths.
How can I resolve this? If you propose a fix, please make it concise. |
NieiR
changed the title
NieiR
force-pushed
the
feat/per-group-provider-priority
branch
from
036d10b to
579c82e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@drizzle/0064_harsh_dragon_lord.sql`:
- Line 1: The migration sets the "group_priorities" column default to the JSON
literal 'null'::jsonb which mismatches the Drizzle schema field groupPriorities:
jsonb('group_priorities').$type<Record<string, number> | null>().default(null);
fix by ensuring the Drizzle schema is correct (groupPriorities with
default(null)) and then regenerate the migration so the ALTER TABLE uses DEFAULT
NULL (SQL NULL) instead of 'null'::jsonb; if you cannot regenerate, manually
edit the migration ALTER TABLE statement to use DEFAULT NULL for
group_priorities and re-run tests/DB migrations to confirm behavior.
In `@src/app/`[locale]/settings/providers/_components/provider-rich-list-item.tsx:
- Around line 598-626: The AlertDialog is nested inside DropdownMenuContent
which unmounts on menu close; move the AlertDialog (including
AlertDialogTrigger/AlertDialogContent/AlertDialogAction) out of
DropdownMenuContent so it isn’t destroyed when the menu closes, and convert it
to a controlled dialog using an open state (e.g., dialogOpen / setDialogOpen)
wired from the DropdownMenuItem click handler (replace implicit
AlertDialogTrigger behavior with onClick that sets dialogOpen = true); keep
existing handlers like handleDelete and props like disabled={deletePending} on
AlertDialogAction so deletion logic remains unchanged.
🧹 Nitpick comments (6)
drizzle/0063_slippery_sharon_carter.sql (1)
1-1: 缺少IF NOT EXISTS幂等性保护迁移 0064 使用了
ADD COLUMN IF NOT EXISTS来确保幂等性，但此迁移未使用。如果在某些部署场景下该迁移被重复执行（例如 Docker 容器重启期间的竞争条件），ADD COLUMN会因列已存在而报错。建议保持一致：
建议的修改
-ALTER TABLE "system_settings" ADD COLUMN "enable_claude_metadata_user_id_injection" boolean DEFAULT true NOT NULL; +ALTER TABLE "system_settings" ADD COLUMN IF NOT EXISTS "enable_claude_metadata_user_id_injection" boolean DEFAULT true NOT NULL;src/app/v1/_lib/proxy/forwarder.ts (3)
295-410: 跳过场景的日志级别可考虑降级。
applyClaudeMetadataUserIdInjectionWithAudit中，already_exists、missing_key_id、missing_session_id等非命中场景均使用logger.info。在高流量 Claude 请求场景下（例如客户端已携带metadata.user_id），这些日志可能产生大量输出。建议将跳过场景降级为logger.debug，仅保留injected场景为logger.info。建议：将 skip 场景日志降级为 debug
if (existingMetadata?.user_id !== undefined && existingMetadata?.user_id !== null) { - logger.info("[ProxyForwarder] Claude metadata.user_id injection skipped", { + logger.debug("[ProxyForwarder] Claude metadata.user_id injection skipped", { enabled, hit: false, reason: "already_exists",对
missing_key_id和missing_session_id分支做同样调整。
1828-1851: 注入逻辑在doForward中执行，重试时会产生重复审计记录。
doForward每次调用都会执行注入和persistSpecialSettings。当请求被重试时（内层循环多次调用doForward），session.addSpecialSetting(injection.audit)会累加多条相同的注入审计记录。这不会影响功能，但会使审计数据冗余。对比 thinking rectifier 的做法（通过
thinkingSignatureRectifierRetried标志控制只执行一次），如果后续发现审计噪声过大，可考虑在send层级做一次性注入，或在doForward中添加去重标志。当前实现不阻塞合并。
214-243:persistSpecialSettings封装了通用的持久化模式，建议后续复用。该方法很好地抽象了 "session 特殊设置存储 + 消息请求详情更新" 的逻辑，目前只用于新增的 metadata 注入。文件中 thinking signature/budget rectifier 及 Gemini/Codex/Anthropic override 的持久化代码（如 Line 833-860, 965-992 等）使用了相同的模式但以内联方式实现。可在后续 PR 中统一替换以减少重复代码。
src/app/[locale]/settings/providers/_components/provider-rich-list-item.tsx (1)
839-851:disabled={!canEdit}冗余。Line 839 的
canEdit &&已保证按钮只在有编辑权限时渲染，Line 847 再加disabled={!canEdit}永远为false，属于无效代码。同样的模式出现在 Line 860 和 Line 899。可以移除这些冗余的disabled属性。示例修改
- <Button - size="icon" - variant="ghost" - onClick={(e) => { - e.stopPropagation(); - handleEdit(); - }} - disabled={!canEdit} - > + <Button + size="icon" + variant="ghost" + onClick={(e) => { + e.stopPropagation(); + handleEdit(); + }} + >src/lib/validation/schemas.ts (1)
412-416:group_priorities的 record key 缺少长度约束，与group_tag的 50 字符限制不一致。
group_tag（Line 418）限制了 50 个字符（整个逗号分隔字符串），但group_priorities的 key 使用裸z.string()没有长度限制。恶意或误操作的请求可以插入超长的 key。建议对 key 也加上合理的长度约束：建议修改
group_priorities: z - .record(z.string(), z.number().int().min(0).max(2147483647)) + .record(z.string().max(50), z.number().int().min(0).max(2147483647)) .nullable() .optional() .default(null),同样适用于
UpdateProviderSchema（Line 618-621）。
src/app/[locale]/settings/providers/_components/provider-rich-list-item.tsx
Show resolved
Hide resolved
| healthcheck: | ||
| test: ["CMD-SHELL", "curl -f http://localhost:3000/api/actions/health || exit 1"] | ||
| test: | ||
| [ | ||
| "CMD", | ||
| "node", | ||
| "-e", | ||
| "fetch('http://' + (process.env.HOSTNAME || '127.0.0.1') + ':3000/api/actions/health').then((r)=>process.exit(r.ok?0:1)).catch(()=>process.exit(1))", | ||
| ] |
There was a problem hiding this comment.
Healthcheck uses missing node
The app service healthcheck runs node -e fetch(...), but the container image is ghcr.io/ding113/claude-code-hub:latest (and in this repo’s Dockerfile the runtime command is node server.js only if Node exists in the image). If the published image is bun-based or otherwise lacks the node binary, the healthcheck will fail continuously and the service will be marked unhealthy. Consider switching back to a healthcheck that relies on tools guaranteed to exist in the image (or ensure node is present in the runtime image).
Prompt To Fix With AI
This is a comment left during a code review.
Path: docker-compose.yaml
Line: 70:77
Comment:
**Healthcheck uses missing `node`**
The `app` service healthcheck runs `node -e fetch(...)`, but the container image is `ghcr.io/ding113/claude-code-hub:latest` (and in this repo’s Dockerfile the runtime command is `node server.js` only if Node exists in the image). If the published image is bun-based or otherwise lacks the `node` binary, the healthcheck will fail continuously and the service will be marked unhealthy. Consider switching back to a healthcheck that relies on tools guaranteed to exist in the image (or ensure `node` is present in the runtime image).
How can I resolve this? If you propose a fix, please make it concise.| ENV CI=true | ||
| RUN bun run build | ||
| RUN --mount=type=cache,target=/app/.next/cache bun run build | ||
|
|
||
| FROM node:20-slim AS runner | ||
| WORKDIR /app | ||
| ENV NODE_ENV=production | ||
| ENV PORT=8080 | ||
| EXPOSE 8080 | ||
| ENV PORT=3000 | ||
| EXPOSE 3000 |
There was a problem hiding this comment.
Runtime port mismatch
This Dockerfile changes ENV PORT/EXPOSE from 8080 to 3000, but the final command is node server.js from Next standalone output. If the generated server.js is configured to bind process.env.PORT (common) this is fine, but if any deployment still expects 8080 (e.g., existing infra / docs / helm values), this change will break connectivity. At minimum ensure the app actually listens on 3000 in standalone mode and update any references that still assume 8080.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Dockerfile
Line: 12:19
Comment:
**Runtime port mismatch**
This Dockerfile changes `ENV PORT`/`EXPOSE` from `8080` to `3000`, but the final command is `node server.js` from Next standalone output. If the generated `server.js` is configured to bind `process.env.PORT` (common) this is fine, but if any deployment still expects `8080` (e.g., existing infra / docs / helm values), this change will break connectivity. At minimum ensure the app actually listens on 3000 in standalone mode and update any references that still assume 8080.
How can I resolve this? If you propose a fix, please make it concise.| # 关键：确保复制了所有必要的文件，特别是 drizzle 文件夹 | ||
| COPY --from=builder /app/public ./public | ||
| COPY --from=builder /app/.next ./.next | ||
| COPY --from=builder /app/node_modules ./node_modules | ||
| COPY --from=builder /app/package.json ./package.json | ||
| COPY --from=builder /app/drizzle ./drizzle | ||
| COPY --from=builder /app/.next/standalone ./ |
There was a problem hiding this comment.
Possible missing server.js
The runner stage copies /.next/standalone into ./ and then runs CMD ["node","server.js"]. This only works if server.js is actually present at the container workdir root after the copy. If Next’s standalone output layout changes or output: 'standalone' isn’t enabled in some build contexts, this will crash at runtime with Cannot find module '/app/server.js'. It would be safer to ensure the build always produces standalone output (and/or validate the file exists in CI) before switching the runtime command.
Prompt To Fix With AI
This is a comment left during a code review.
Path: Dockerfile
Line: 21:23
Comment:
**Possible missing `server.js`**
The runner stage copies `/.next/standalone` into `./` and then runs `CMD ["node","server.js"]`. This only works if `server.js` is actually present at the container workdir root after the copy. If Next’s standalone output layout changes or `output: 'standalone'` isn’t enabled in some build contexts, this will crash at runtime with `Cannot find module '/app/server.js'`. It would be safer to ensure the build always produces standalone output (and/or validate the file exists in CI) before switching the runtime command.
How can I resolve this? If you propose a fix, please make it concise.| * 支持逗号分隔的多分组（如 "cli,admin"），取匹配到的最小优先级 | ||
| */ | ||
| static resolveEffectivePriority(provider: Provider, userGroup: string | null): number { | ||
| if (userGroup && provider.groupPriorities) { | ||
| const groups = parseGroupString(userGroup); | ||
| const overrides = groups | ||
| .map((g) => provider.groupPriorities?.[g]) | ||
| .filter((v): v is number => v !== undefined); | ||
| if (overrides.length > 0) { |
There was a problem hiding this comment.
Priority override semantics change
resolveEffectivePriority() now parses comma-separated userGroup and returns the minimum matched override (Math.min(...overrides)). This is a behavior change vs the previously-discussed “first matching group wins” semantics from earlier threads. If userGroup can contain multiple groups (e.g. "cli,admin"), this will prefer the numerically smallest override across all matches, not the first match. Please confirm which behavior is intended and align implementation + docs/tests accordingly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/provider-selector.ts
Line: 1039:1047
Comment:
**Priority override semantics change**
`resolveEffectivePriority()` now parses comma-separated `userGroup` and returns the *minimum* matched override (`Math.min(...overrides)`). This is a behavior change vs the previously-discussed “first matching group wins” semantics from earlier threads. If `userGroup` can contain multiple groups (e.g. `"cli,admin"`), this will prefer the numerically smallest override across all matches, not the first match. Please confirm which behavior is intended and align implementation + docs/tests accordingly.
How can I resolve this? If you propose a fix, please make it concise.
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
2 participants
变更摘要
本 PR 聚焦 Provider 路由策略的精细化控制与配置效率提升，核心改动如下：
按分组优先级路由（Per-Group Priority）
providers.group_priorities（JSONB）字段。列表页内联编辑能力
移动端与交互优化
兼容性与质量保障
group_priorities时保持原有全局优先级行为。CI 可靠性修复（fork 场景）
Test Summary评论步骤触发条件，避免 fork PR 因权限限制出现403 Resource not accessible by integration的假失败。测试与验证
已在 GitHub Actions 中通过以下检查：
Code Quality Check📋 Code Quality⚡ Unit Tests🔗 Integration Tests🌐 API TestsDocker Build Test📊 Test Summary验证方式：基于 PR 自动化流水线日志检查（见 PR Checks）。
PR Checklist
devDocker Build Test）upstream/dev）Greptile Overview
Greptile Summary
This PR adds per-group provider priority overrides (
providers.group_prioritiesjsonb) end-to-end (DB schema/migrations, repository transformers, server actions/types) and introduces inline editing for global priority, group priorities, and group tags in the providers settings UI (Drawer on mobile, Popover on desktop). It also adjusts CI to avoid posting Test Summary comments on fork PRs.Main merge-blocking issues found:
docker-compose.yamlhealthcheck now runsnode -e fetch(...), which will fail if the published runtime image does not include thenodebinary (container stays unhealthy).ProxyProviderResolver.resolveEffectivePriority()currently resolves multi-group overrides by taking the numeric minimum across all matching groups, which conflicts with the earlier-threaded “first matching group wins” semantics; confirm intended behavior and align implementation/tests/docs.Other notable changes include switching the Docker runtime to Next standalone output and changing the exposed port to 3000; ensure runtime layout/port expectations are consistent with deployments.
Confidence Score: 3/5
nodeis absent in the runtime image, and the effective priority calculation for multi-group users appears to implement a different rule than previously described. These can cause real production issues (unhealthy container, unexpected routing behavior).Important Files Changed
node -e fetch(...); will fail if the runtime image lacks node (likely unhealthy container loop).Sequence Diagram
sequenceDiagram actor Admin as Admin/User participant UI as Providers UI (List/Form) participant Actions as Server Actions (src/actions/providers.ts) participant Repo as Provider Repository (src/repository/provider.ts) participant DB as Postgres (providers.group_priorities) participant Selector as ProxyProviderResolver Admin->>UI: Inline edit global/group priority UI->>Actions: editProvider(id,{priority,group_priorities}) Actions->>Repo: updateProvider(id, UpdateProviderData) Repo->>DB: UPDATE providers SET priority, group_priorities DB-->>Repo: updated row Repo-->>Actions: Provider Actions-->>UI: ok/error Note over Selector,DB: Runtime routing uses group_priorities Selector->>Repo: findAllProviders() / snapshot Repo->>DB: SELECT providers (incl. group_priorities) DB-->>Repo: provider rows Repo-->>Selector: Provider[] Selector->>Selector: resolveEffectivePriority(provider,userGroup) Selector-->>Admin: Routes request to selected provider