feat(providers): add group-level priority override #641
Conversation
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
- Add group_priorities JSONB field for per-group priority settings - Update provider selection algorithm to use effective priority based on user group - Add GroupPriorityPopover component for editing priorities on multi-group providers - Add GroupTagEditPopover component for inline group tag editing - Full i18n support (zh-CN, zh-TW, en, ja, ru)
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthrough新增对提供商的群组优先级覆盖支持：数据库新增 Changes
Estimated code review effort🎯 3 (中等) | ⏱️ ~25 分钟 Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
size/XL
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+119
to
+128
| try { | ||
| const newPriority = Number(draft.default); | ||
|
|
||
| // Build new groupPriorities | ||
| let newGroupPriorities: Record<string, number> | null = null; | ||
| for (const group of groups) { | ||
| const value = draft.groups[group]?.trim(); | ||
| if (value && value.length > 0) { | ||
| if (!newGroupPriorities) newGroupPriorities = {}; | ||
| newGroupPriorities[group] = Number(value); |
There was a problem hiding this comment.
Empty string values could bypass validation. When a user clears an input field (empty string), the validation passes on line 36 (if (value.length === 0) return true), but here on line 124 the check if (value && value.length > 0) filters it out correctly. However, the logic is slightly inconsistent - value.length > 0 already implies truthiness. Consider simplifying to if (value.length > 0) for clarity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/group-priority-popover.tsx
Line: 119:128
Comment:
Empty string values could bypass validation. When a user clears an input field (empty string), the validation passes on line 36 (`if (value.length === 0) return true`), but here on line 124 the check `if (value && value.length > 0)` filters it out correctly. However, the logic is slightly inconsistent - `value.length > 0` already implies truthiness. Consider simplifying to `if (value.length > 0)` for clarity.
How can I resolve this? If you propose a fix, please make it concise.
Author
There was a problem hiding this comment.
Thanks for the feedback! The validation logic works correctly:
- Line 36 (
if (value.length === 0) return true) allows empty input during typing - Line 124 filters out empty values before submission
I've simplified the condition as suggested. See commit 8abd137.
Comment on lines
+74
to
+78
| // Return first matching group's priority | ||
| for (const group of userGroups) { | ||
| if (group in provider.groupPriorities) { | ||
| return provider.groupPriorities[group]; | ||
| } |
There was a problem hiding this comment.
First-match priority resolution may cause unexpected behavior. If a user belongs to groups ["cli", "chat"] and both have overrides, only cli's priority is used. Consider documenting this behavior or implementing a "lowest priority wins" strategy.
For example, with user groups "cli,chat":
- If
clipriority = 1 andchatpriority = 5 - Current: returns 1 (first match)
- Alternative:
Math.min(1, 5) = 1(highest priority across all groups)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/v1/_lib/proxy/provider-selector.ts
Line: 74:78
Comment:
First-match priority resolution may cause unexpected behavior. If a user belongs to groups `["cli", "chat"]` and both have overrides, only `cli`'s priority is used. Consider documenting this behavior or implementing a "lowest priority wins" strategy.
For example, with user groups `"cli,chat"`:
- If `cli` priority = 1 and `chat` priority = 5
- Current: returns 1 (first match)
- Alternative: `Math.min(1, 5) = 1` (highest priority across all groups)
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Author
There was a problem hiding this comment.
This is intentional behavior - first-match priority is documented in the function's JSDoc comments:
// Return first matching group's priority
for (const group of userGroups) {
if (group in provider.groupPriorities) {
return provider.groupPriorities[group];
}
}The rationale: user groups are ordered by preference (e.g., "cli,chat" means cli is primary). First-match respects this ordering.
Added unit tests to document this behavior. See commit 8abd137.
Comment on lines
+414
to
+422
| // Calculate display priority based on selected group filter | ||
| const displayPriority = (() => { | ||
| if (!selectedGroup || selectedGroup === "default") { | ||
| return provider.priority; | ||
| } | ||
| if (provider.groupPriorities?.[selectedGroup] !== undefined) { | ||
| return provider.groupPriorities[selectedGroup]; | ||
| } | ||
| return provider.priority; |
There was a problem hiding this comment.
Display priority calculation mirrors backend first-match logic - if selected group filter has no override, falls back to default priority.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/provider-rich-list-item.tsx
Line: 414:422
Comment:
Display priority calculation mirrors backend first-match logic - if selected group filter has no override, falls back to default priority.
How can I resolve this? If you propose a fix, please make it concise.
Author
There was a problem hiding this comment.
Thanks for confirming! Yes, the frontend display logic intentionally mirrors the backend first-match behavior for consistency.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR adds a group-level priority override feature for providers, allowing different priorities for different user groups. The implementation is well-structured with proper i18n support and UI components. However, there is one critical issue that must be addressed.
PR Size: XL
- Lines changed: 3739 (3711 additions + 28 deletions)
- Files changed: 30
Split Suggestions for XL PR:
This PR could be split into smaller, more reviewable PRs:
- Database schema + types (migration, schema.ts, types, repository layer)
- Core business logic (provider-selector.ts with getEffectivePriority)
- UI components (group-priority-popover, group-tag-edit-popover)
- i18n translations (all 5 language files)
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 1 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
1. [TEST-MISSING-CRITICAL] No test coverage for group-level priority override feature
Location: src/app/v1/_lib/proxy/provider-selector.ts - getEffectivePriority() function and selectTopPriority() modifications
Why this is a problem:
This PR introduces a new group-level priority override feature that modifies critical provider selection logic. The getEffectivePriority() function and selectTopPriority() changes directly affect which provider is selected for each request, which is a critical code path.
Violated guideline: CLAUDE.md line 12 states: "Test Coverage - All new features must have unit test coverage of at least 80%"
Without tests, there is no verification that:
- Group-specific priorities correctly override default priorities
- The first matching group is used when a user belongs to multiple groups
- Default priority is used when no group override exists
- Edge cases are handled (null groups, empty groupPriorities, invalid data)
Suggested fix:
Add unit tests in tests/unit/ or source-adjacent tests. Minimum test cases:
// tests/unit/provider-selector.test.ts or src/app/v1/_lib/proxy/provider-selector.test.ts
describe('getEffectivePriority', () => {
it('should return default priority when user has no group', () => {
const provider = { priority: 5, groupPriorities: { cli: 1 } };
expect(getEffectivePriority(provider, null)).toBe(5);
});
it('should return group-specific priority when user group matches', () => {
const provider = { priority: 5, groupPriorities: { cli: 1, chat: 3 } };
expect(getEffectivePriority(provider, 'cli')).toBe(1);
});
it('should return first matching group priority for comma-separated groups', () => {
const provider = { priority: 5, groupPriorities: { cli: 1, chat: 3 } };
expect(getEffectivePriority(provider, 'chat,cli')).toBe(3);
});
it('should return default priority when no group override exists', () => {
const provider = { priority: 5, groupPriorities: { cli: 1 } };
expect(getEffectivePriority(provider, 'web')).toBe(5);
});
it('should return default priority when groupPriorities is null', () => {
const provider = { priority: 5, groupPriorities: null };
expect(getEffectivePriority(provider, 'cli')).toBe(5);
});
});
describe('selectTopPriority with group overrides', () => {
it('should select providers with lowest effective priority for user group', () => {
const providers = [
{ id: 1, priority: 5, groupPriorities: { cli: 1 } },
{ id: 2, priority: 3, groupPriorities: { cli: 2 } },
{ id: 3, priority: 1, groupPriorities: null }
];
const result = selectTopPriority(providers, 'cli');
expect(result).toEqual([providers[0]]); // cli group priority 1 is lowest
});
});Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean (proper try/finally patterns, errors propagate correctly)
- Type safety - Clean (proper TypeScript types, validation schemas)
- Documentation accuracy - Clean (comments match implementation)
- Test coverage - CRITICAL: Missing tests for new feature
- Code clarity - Good (well-structured, clear naming)
Positive Observations
- Proper i18n implementation across all 5 languages
- Good validation logic in UI components (validatePriority function)
- Proper error handling with try/finally blocks
- Clean separation of concerns (UI, business logic, data layer)
- Well-documented code with clear comments
- Follows existing code patterns and conventions
Automated review by Claude AI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/app/`[locale]/settings/providers/_components/group-tag-edit-popover.tsx:
- Around line 141-144: Replace the hard-coded English label
PROVIDER_GROUP.DEFAULT used inside the Badge in group-tag-edit-popover.tsx with
a next-intl translation key (e.g. use intl.formatMessage or the
<FormattedMessage> from next-intl) and add the corresponding translation entries
for that key in the five locale JSON/TS files; update the Badge rendering to
call the intl lookup (keeping the same Badge variant and classes) so the
default-group badge text is localized across all supported languages.
- Around line 90-96: The handleAddGroup function should validate newGroup before
adding: trim newGroup, reject values containing commas and the reserved names
"default" and "*" (case-insensitive), and prevent adding empty or duplicate
entries to selectedGroups; on validation failure surface a user-facing message
(e.g., via an existing error state or toast) and do not call setSelectedGroups
or clear the input. Update handleAddGroup to perform these checks (referencing
newGroup, selectedGroups, setSelectedGroups, setNewGroup) and return early on
invalid input.
In `@src/lib/validation/schemas.ts`:
- Around line 556-560: CreateProviderSchema is missing the group_priorities
field that UpdateProviderSchema already allows; add a group_priorities property
to CreateProviderSchema using the same shape as in UpdateProviderSchema but
constrain the record keys to match group_tag (max 50 chars). Specifically, in
CreateProviderSchema add group_priorities: z.record(z.string().max(50),
z.number().int().min(0).max(2147483647)).nullable().optional(), matching the
UpdateProviderSchema definition and keeping the same null/optional semantics.
🧹 Nitpick comments (2)
drizzle/0057_jazzy_blink.sql (1)
2-2:DEFAULT 'null'::jsonb可能导致查询混淆当前默认值设置为 JSON
null字面量，而非 SQLNULL。这会导致：
group_priorities IS NULL查询不会匹配这些行- 需要用
group_priorities = 'null'::jsonb或group_priorities::text = 'null'来检测建议改为
DEFAULT NULL或直接不设置默认值，让列自然为 SQL NULL。建议修改
-ALTER TABLE "providers" ADD COLUMN "group_priorities" jsonb DEFAULT 'null'::jsonb; +ALTER TABLE "providers" ADD COLUMN "group_priorities" jsonb DEFAULT NULL;src/types/provider.ts (1)
55-56: 建议明确 groupPriorities 的 null/undefined 语义。
可在注释中约定：null表示显式清空/无覆盖，undefined（在 Create/Update 中）表示不修改或继承默认，以保持与其它额度字段的语义一致。Based on learnings, 建议明确 null 与 undefined 的语义边界。
Also applies to: 160-161, 252-253, 323-324
src/app/[locale]/settings/providers/_components/group-tag-edit-popover.tsx
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/group-tag-edit-popover.tsx
Show resolved
Hide resolved
- Fix lint errors: remove unused imports (Power, cn) and params (allowedProviderTypes) - Add i18n for default group badge text in group-tag-edit-popover - Add group name validation: reject commas and reserved names (default, *) - Add unit tests for getEffectivePriority() function (12 test cases) - Export getEffectivePriority for testing
Author
Addressed Code Review FeedbackAll critical and high-priority issues from the code review bots have been addressed in commit 8abd137: Fixed Issues
Intentional Design Decisions
Test Results
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/app/`[locale]/settings/providers/_components/group-tag-edit-popover.tsx:
- Around line 130-132: allDisplayGroups currently only filters out "default"
literally, which allows reserved names like "*" or case variants (e.g.
"DEFAULT", "*") from availableGroups/selectedGroups to be shown and selected,
bypassing handleAddGroup validation; update the creation of allDisplayGroups to
normalize group names to a consistent case and filter against a constant set of
reserved names (e.g., RESERVED_GROUPS = new Set(["default","*"]) ) using
case-insensitive comparison (toLowerCase) so any variant is excluded before
rendering/selection and cannot bypass handleAddGroup.
- Around line 221-230: The icon-only Add Group Button (Button with
onClick={handleAddGroup}, disabled={saving || !newGroup.trim()}) lacks an
accessible name; add an aria-label that uses the i18n translation key (e.g.
"settings.group.add") instead of hardcoding text, wire it to the app's
translation function (e.g. t('settings.group.add')) and ensure the new key is
added to the translations for all five locales; keep the visible UI unchanged
(icon only) and pass the aria-label prop to the Button component.
In `@src/app/v1/_lib/proxy/provider-selector.ts`:
- Around line 65-79: Replace the prototype-sensitive "in" check inside
getEffectivePriority with an own-property check (e.g.,
Object.hasOwn(provider.groupPriorities, group) or
provider.groupPriorities.hasOwnProperty(group)) so only direct keys are matched,
and ensure getEffectivePriority falls back to returning provider.priority if no
group match is found; update the loop that iterates userGroups and the
function's final return accordingly.
In `@tests/unit/proxy/provider-selector-group-priority.test.ts`:
- Around line 5-31: The test helper createProvider returns an object that no
longer matches the Provider interface (uses old fields like apiKey/enabled and
omits required fields such as key, isEnabled, and other mandatory properties);
update createProvider (or reuse the existing Provider test factory) to construct
and return a fully compliant Provider object: rename/remove deprecated fields
(apiKey → key, enabled → isEnabled), add all required properties from the
current Provider type (including key, isEnabled, providerVendorId, limits,
timestamps, and any other non-optional fields), and ensure groupPriorities and
other provided params are applied; verify the returned shape satisfies the
Provider interface used by the tests.
Comment on lines
+130
to
+132
| const allDisplayGroups = Array.from(new Set([...availableGroups, ...selectedGroups])).filter( | ||
| (g) => g !== "default" | ||
| ); |
There was a problem hiding this comment.
避免保留分组通过列表选择绕过校验
allDisplayGroups 只排除了 "default"，若 availableGroups 含 "*" 或大小写变体，会被渲染并可勾选保存，绕过 handleAddGroup 的保留名校验。建议用常量并做大小写无关过滤。
Proposed fix
- const allDisplayGroups = Array.from(new Set([...availableGroups, ...selectedGroups])).filter(
- (g) => g !== "default"
- );
+ const allDisplayGroups = Array.from(new Set([...availableGroups, ...selectedGroups])).filter(
+ (g) => {
+ const normalized = g.trim().toLowerCase();
+ return (
+ normalized !== PROVIDER_GROUP.DEFAULT &&
+ normalized !== PROVIDER_GROUP.ALL
+ );
+ }
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const allDisplayGroups = Array.from(new Set([...availableGroups, ...selectedGroups])).filter( | |
| (g) => g !== "default" | |
| ); | |
| const allDisplayGroups = Array.from(new Set([...availableGroups, ...selectedGroups])).filter( | |
| (g) => { | |
| const normalized = g.trim().toLowerCase(); | |
| return ( | |
| normalized !== PROVIDER_GROUP.DEFAULT && | |
| normalized !== PROVIDER_GROUP.ALL | |
| ); | |
| } | |
| ); |
🤖 Prompt for AI Agents
In `@src/app/`[locale]/settings/providers/_components/group-tag-edit-popover.tsx
around lines 130 - 132, allDisplayGroups currently only filters out "default"
literally, which allows reserved names like "*" or case variants (e.g.
"DEFAULT", "*") from availableGroups/selectedGroups to be shown and selected,
bypassing handleAddGroup validation; update the creation of allDisplayGroups to
normalize group names to a consistent case and filter against a constant set of
reserved names (e.g., RESERVED_GROUPS = new Set(["default","*"]) ) using
case-insensitive comparison (toLowerCase) so any variant is excluded before
rendering/selection and cannot bypass handleAddGroup.
Comment on lines
+221
to
+230
| <Button | ||
| type="button" | ||
| size="sm" | ||
| variant="outline" | ||
| onClick={handleAddGroup} | ||
| disabled={saving || !newGroup.trim()} | ||
| className="h-8 px-2" | ||
| > | ||
| <Plus className="h-4 w-4" /> | ||
| </Button> |
There was a problem hiding this comment.
为仅图标按钮补充可访问名称并走 i18n
当前按钮只有图标，屏幕阅读器无法获得名称。建议添加 aria-label 并新增对应翻译键，补齐 5 语种。
Proposed fix
<Button
type="button"
size="sm"
variant="outline"
onClick={handleAddGroup}
disabled={saving || !newGroup.trim()}
className="h-8 px-2"
+ aria-label={t("addGroup")}
>
<Plus className="h-4 w-4" />
</Button>🤖 Prompt for AI Agents
In `@src/app/`[locale]/settings/providers/_components/group-tag-edit-popover.tsx
around lines 221 - 230, The icon-only Add Group Button (Button with
onClick={handleAddGroup}, disabled={saving || !newGroup.trim()}) lacks an
accessible name; add an aria-label that uses the i18n translation key (e.g.
"settings.group.add") instead of hardcoding text, wire it to the app's
translation function (e.g. t('settings.group.add')) and ensure the new key is
added to the translations for all five locales; keep the visible UI unchanged
(icon only) and pass the aria-label prop to the Button component.
Comment on lines
+65
to
+79
| function getEffectivePriority(provider: Provider, userGroup: string | null): number { | ||
| // No group or no overrides: use global priority | ||
| if (!userGroup || !provider.groupPriorities) { | ||
| return provider.priority; | ||
| } | ||
|
|
||
| // Parse user groups (may be comma-separated) | ||
| const userGroups = parseGroupString(userGroup); | ||
|
|
||
| // Return first matching group's priority | ||
| for (const group of userGroups) { | ||
| if (group in provider.groupPriorities) { | ||
| return provider.groupPriorities[group]; | ||
| } | ||
| } |
There was a problem hiding this comment.
避免使用 in 触发原型链命中
in 会命中原型链键（如 toString），在少数分组名场景下可能返回 undefined，进而影响后续 Math.min 计算。建议用 Object.hasOwn（或 hasOwnProperty）确保只匹配自有属性。
可选修正
- for (const group of userGroups) {
- if (group in provider.groupPriorities) {
- return provider.groupPriorities[group];
- }
- }
+ for (const group of userGroups) {
+ if (Object.hasOwn(provider.groupPriorities, group)) {
+ return provider.groupPriorities[group];
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function getEffectivePriority(provider: Provider, userGroup: string | null): number { | |
| // No group or no overrides: use global priority | |
| if (!userGroup || !provider.groupPriorities) { | |
| return provider.priority; | |
| } | |
| // Parse user groups (may be comma-separated) | |
| const userGroups = parseGroupString(userGroup); | |
| // Return first matching group's priority | |
| for (const group of userGroups) { | |
| if (group in provider.groupPriorities) { | |
| return provider.groupPriorities[group]; | |
| } | |
| } | |
| function getEffectivePriority(provider: Provider, userGroup: string | null): number { | |
| // No group or no overrides: use global priority | |
| if (!userGroup || !provider.groupPriorities) { | |
| return provider.priority; | |
| } | |
| // Parse user groups (may be comma-separated) | |
| const userGroups = parseGroupString(userGroup); | |
| // Return first matching group's priority | |
| for (const group of userGroups) { | |
| if (Object.hasOwn(provider.groupPriorities, group)) { | |
| return provider.groupPriorities[group]; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/app/v1/_lib/proxy/provider-selector.ts` around lines 65 - 79, Replace the
prototype-sensitive "in" check inside getEffectivePriority with an own-property
check (e.g., Object.hasOwn(provider.groupPriorities, group) or
provider.groupPriorities.hasOwnProperty(group)) so only direct keys are matched,
and ensure getEffectivePriority falls back to returning provider.priority if no
group match is found; update the loop that iterates userGroups and the
function's final return accordingly.
Comment on lines
+5
to
+31
| // Helper to create a minimal Provider for testing | ||
| function createProvider( | ||
| priority: number, | ||
| groupPriorities: Record<string, number> | null = null | ||
| ): Provider { | ||
| return { | ||
| id: 1, | ||
| name: "test-provider", | ||
| providerType: "claude", | ||
| url: "https://api.anthropic.com", | ||
| apiKey: "test-key", | ||
| enabled: true, | ||
| priority, | ||
| weight: 1, | ||
| costMultiplier: 1, | ||
| groupTag: null, | ||
| allowedModels: null, | ||
| modelRedirects: null, | ||
| joinClaudePool: false, | ||
| groupPriorities, | ||
| providerVendorId: null, | ||
| limitTotalUsd: null, | ||
| usedTotalUsd: 0, | ||
| createdAt: new Date(), | ||
| updatedAt: new Date(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
createProvider 未符合 Provider 接口字段，可能导致类型检查失败
当前对象使用了 apiKey/enabled 等旧字段名，并缺少 key、isEnabled 等必填字段（以及多项必需属性）。如果未来把 tests 目录纳入 TS 检查，这里会直接报错，也会降低测试数据的代表性。建议对齐 Provider 接口（或复用现有的 Provider 测试工厂/构造器）。
🤖 Prompt for AI Agents
In `@tests/unit/proxy/provider-selector-group-priority.test.ts` around lines 5 -
31, The test helper createProvider returns an object that no longer matches the
Provider interface (uses old fields like apiKey/enabled and omits required
fields such as key, isEnabled, and other mandatory properties); update
createProvider (or reuse the existing Provider test factory) to construct and
return a fully compliant Provider object: rename/remove deprecated fields
(apiKey → key, enabled → isEnabled), add all required properties from the
current Provider type (including key, isEnabled, providerVendorId, limits,
timestamps, and any other non-optional fields), and ensure groupPriorities and
other provided params are applied; verify the returned shape satisfies the
Provider interface used by the tests.
Transform the group selection dropdown from vertical list to horizontal flow layout (tag cloud style) for better space utilization and easier selection when there are many groups. Changes: - TagInput: suggestions panel uses flex-wrap horizontal layout - TagInput: click toggles selection instead of add-only - GroupTagEditPopover: checkbox labels use horizontal flow layout
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/ui/tag-input.tsx`:
- Around line 308-322: handleSuggestionClick currently calls onChangeCommit when
removing a tag but not when adding one (it calls addTag(suggestionValue, true)
which updates state via onChange only), causing inconsistent commit behavior;
fix by invoking onChangeCommit for the add-path as well: either have
addTag(suggestionValue, true) return or accept a callback/flag that triggers
onChangeCommit internally, or after calling addTag compute the new tags array
and call onChangeCommit(newTags); update handleSuggestionClick to call
onChangeCommit with the same signature used in the removal branch so both
branches commit consistently (references: handleSuggestionClick, addTag,
onChange, onChangeCommit).
♻️ Duplicate comments (2)
src/app/[locale]/settings/providers/_components/group-tag-edit-popover.tsx (2)
130-132: 改进分组列表过滤以避免保留名绕过Line 130-132 仅过滤了字面量
"default"，"*"或大小写/空白变体仍可能被渲染并选中，绕过保留名校验。建议规范化并过滤PROVIDER_GROUP.DEFAULT/PROVIDER_GROUP.ALL。建议修改
- const allDisplayGroups = Array.from(new Set([...availableGroups, ...selectedGroups])).filter( - (g) => g !== "default" - ); + const allDisplayGroups = Array.from(new Set([...availableGroups, ...selectedGroups])).filter( + (g) => { + const normalized = g.trim().toLowerCase(); + return normalized !== PROVIDER_GROUP.DEFAULT && normalized !== PROVIDER_GROUP.ALL; + } + );
226-235: 为仅图标按钮补充可访问名称Line 226-235 的按钮仅图标显示，缺少可访问名称。建议添加
aria-label并通过 i18n 文案提供，同时补齐 5 语种翻译。As per coding guidelines, 请确保新增文案走 i18n 并补齐 5 语种。建议修改
<Button type="button" size="sm" variant="outline" onClick={handleAddGroup} disabled={saving || !newGroup.trim()} className="h-8 px-2" + aria-label={t("addGroup")} > <Plus className="h-4 w-4" /> </Button>
🧹 Nitpick comments (1)
src/components/ui/tag-input.tsx (1)
103-112: 注释已过时，需更新。第 103 行的注释
// 过滤建议列表：匹配输入值且未被选中与当前实现不符。代码已改为仅按搜索词过滤，不再排除已选中项（选中状态通过视觉样式展示）。建议更新注释以准确描述当前行为：
建议修改
- // 过滤建议列表：匹配输入值且未被选中 + // 过滤建议列表：按输入值匹配（已选中项通过样式区分） const filteredSuggestions = React.useMemo(() => {
Comment on lines
308
to
322
| const handleSuggestionClick = React.useCallback( | ||
| (suggestionValue: string) => { | ||
| addTag(suggestionValue, true); // keepOpen=true 保持下拉展开 | ||
| if (value.includes(suggestionValue)) { | ||
| // Already selected -> deselect (remove tag) | ||
| const nextTags = value.filter((v) => v !== suggestionValue); | ||
| onChange(nextTags); | ||
| onChangeCommit?.(nextTags); | ||
| } else { | ||
| // Not selected -> select (add tag) | ||
| addTag(suggestionValue, true); | ||
| } | ||
| inputRef.current?.focus(); | ||
| }, | ||
| [addTag] | ||
| [value, onChange, onChangeCommit, addTag] | ||
| ); |
There was a problem hiding this comment.
onChangeCommit 调用行为不一致。
切换逻辑中，取消选择时调用了 onChangeCommit（第 314 行），但选择时通过 addTag 仅调用 onChange，未触发 onChangeCommit。这可能导致依赖 onChangeCommit 进行持久化的调用方出现不一致行为。
建议在选择路径中也触发 onChangeCommit，或在 addTag 中增加可选的 commit 回调：
建议修改
if (value.includes(suggestionValue)) {
// Already selected -> deselect (remove tag)
const nextTags = value.filter((v) => v !== suggestionValue);
onChange(nextTags);
onChangeCommit?.(nextTags);
} else {
// Not selected -> select (add tag)
- addTag(suggestionValue, true);
+ const nextTags = [...value, suggestionValue];
+ onChange(nextTags);
+ onChangeCommit?.(nextTags);
}🤖 Prompt for AI Agents
In `@src/components/ui/tag-input.tsx` around lines 308 - 322,
handleSuggestionClick currently calls onChangeCommit when removing a tag but not
when adding one (it calls addTag(suggestionValue, true) which updates state via
onChange only), causing inconsistent commit behavior; fix by invoking
onChangeCommit for the add-path as well: either have addTag(suggestionValue,
true) return or accept a callback/flag that triggers onChangeCommit internally,
or after calling addTag compute the new tags array and call
onChangeCommit(newTags); update handleSuggestionClick to call onChangeCommit
with the same signature used in the removal branch so both branches commit
consistently (references: handleSuggestionClick, addTag, onChange,
onChangeCommit).
| providerType: "claude", | ||
| url: "https://api.anthropic.com", | ||
| apiKey: "test-key", | ||
| enabled: true, |
There was a problem hiding this comment.
enabled field doesn't exist in the Provider type. Should be isEnabled instead.
Suggested change
| enabled: true, | |
| isEnabled: true, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/proxy/provider-selector-group-priority.test.ts
Line: 16:16
Comment:
`enabled` field doesn't exist in the Provider type. Should be `isEnabled` instead.
```suggestion
isEnabled: true,
```
How can I resolve this? If you propose a fix, please make it concise.| name: "test-provider", | ||
| providerType: "claude", | ||
| url: "https://api.anthropic.com", | ||
| apiKey: "test-key", |
There was a problem hiding this comment.
Property name mismatch with Provider interface - should be key not apiKey
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/proxy/provider-selector-group-priority.test.ts
Line: 15:15
Comment:
Property name mismatch with Provider interface - should be `key` not `apiKey`
How can I resolve this? If you propose a fix, please make it concise.… key Address CodeRabbit feedback: constrain group_priorities keys to max 50 chars, consistent with group_tag field validation.
Comment on lines
+14
to
+16
| url: "https://api.anthropic.com", | ||
| apiKey: "test-key", | ||
| enabled: true, |
There was a problem hiding this comment.
Property names don't match the Provider interface
The test uses apiKey and enabled, but the Provider interface expects key and isEnabled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/proxy/provider-selector-group-priority.test.ts
Line: 14:16
Comment:
Property names don't match the Provider interface
The test uses `apiKey` and `enabled`, but the Provider interface expects `key` and `isEnabled`.
How can I resolve this? If you propose a fix, please make it concise.
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
Add group-level priority override feature for providers, allowing different priorities for different user groups.
Changes:
group_prioritiesJSONB field for per-group priority settingsGroupPriorityPopovercomponent for editing priorities on multi-group providersGroupTagEditPopovercomponent for inline group tag editingUse Case
When a provider serves multiple user groups, administrators can configure different priorities per group. For example:
cligroup: priority 1 (highest)chatgroup: priority 5 (lower)Test Plan
bun run build)bun run typecheck)Screenshots
N/A (backend + existing UI patterns)
Greptile Overview
Greptile Summary
This PR adds group-level priority overrides for providers, allowing administrators to configure different priorities per user group (e.g.,
cligroup gets priority 1,chatgroup gets priority 5).Key Changes:
group_prioritiesJSONB field to store per-group priority mappingsgetEffectivePriority()function using first-match logic for comma-separated user groupsGroupPriorityPopovercomponent for inline editing with proper validationIssues Found:
apiKeyandenabledinstead ofkeyandisEnabled) - will cause type errorsConfidence Score: 4/5
Important Files Changed
groupPrioritiesfield on providers table with proper typing - schema definition is correctgetEffectivePriority()with first-match logic for comma-separated groups - implementation is correct and well-testedenabledandapiKeyproperties instead ofisEnabledandkeygroup_prioritiesas record with max 50 char keys and integer values 0-2147483647Sequence Diagram
sequenceDiagram participant Client participant ProviderSelector participant Database participant Provider Client->>ProviderSelector: Request with user group "cli,chat" ProviderSelector->>Database: Fetch all providers Database-->>ProviderSelector: Return providers with groupPriorities Note over ProviderSelector: Filter by group match Note over ProviderSelector: For each provider, call getEffectivePriority() ProviderSelector->>ProviderSelector: Parse user groups ["cli", "chat"] ProviderSelector->>ProviderSelector: Check provider.groupPriorities alt Provider has groupPriorities ProviderSelector->>ProviderSelector: Iterate user groups in order ProviderSelector->>ProviderSelector: Find first match (e.g., "cli": 1) ProviderSelector->>ProviderSelector: Return group priority (1) else No groupPriorities or no match ProviderSelector->>ProviderSelector: Return default priority (5) end Note over ProviderSelector: Select provider with highest priority (lowest value) ProviderSelector->>Provider: Route request to selected provider Provider-->>Client: Response