feat: align provider group management with provider list#1033
Merged
Conversation
📝 WalkthroughWalkthrough在供应商分组与管理功能中新增/扩展多语言文案键，重构 ProviderGroupTab 以接收外部 providers 并支持内联/多选编辑，添加 ProviderManager 的单供应商编辑对话流，并为相关交互新增单元测试与可选渲染属性。 Changes
代码审查预估工作量🎯 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
enhancement
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f39a05634e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+804
to
+808
| <Button | ||
| size="sm" | ||
| variant="ghost" | ||
| className="h-8 px-2" | ||
| onClick={() => onRequestEditProvider(member.id)} |
There was a problem hiding this comment.
When canEdit is false (non-admin users), this button still calls onRequestEditProvider, which opens the shared ProviderForm dialog through ProviderManager. That bypasses the existing UI permission model used in the provider list (where edit entry points are hidden for non-admins) and exposes an edit workflow to unauthorized users. Even if backend saves are rejected, non-admins can still access sensitive configuration screens and trigger edit attempts; the edit entry points in MemberRow should be gated by canEdit.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
已处理。成员行的名称点击入口和右侧编辑按钮现在都按 canEdit 门控；非管理员只会看到只读名称，不会再触发共享编辑弹窗。对应回归已补到 provider-group-tab.test.tsx。
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+366
to
+378
| const handleOpenProviderEditor = useCallback((provider: ProviderDisplay) => { | ||
| setViewMode("list"); | ||
| setEditingProviderId(provider.id); | ||
| }, []); | ||
|
|
||
| const handleRequestEditProvider = useCallback( | ||
| (providerId: number) => { | ||
| const provider = providers.find((item) => item.id === providerId); | ||
| if (!provider) return; | ||
| handleOpenProviderEditor(provider); | ||
| }, | ||
| [handleOpenProviderEditor, providers] | ||
| ); |
There was a problem hiding this comment.
handleOpenProviderEditor always calls setViewMode("list"), so clicking the edit entry in the groups panel switches the view to list. After the user closes the dialog, they land on the list tab rather than returning to the groups tab they came from. Since the editor dialog is a modal overlay mounted at ProviderManager level and independent of viewMode, the setViewMode("list") call isn't needed for the dialog to appear.
Consider keeping the view state when editing from the groups context:
Suggested change
| const handleOpenProviderEditor = useCallback((provider: ProviderDisplay) => { | |
| setViewMode("list"); | |
| setEditingProviderId(provider.id); | |
| }, []); | |
| const handleRequestEditProvider = useCallback( | |
| (providerId: number) => { | |
| const provider = providers.find((item) => item.id === providerId); | |
| if (!provider) return; | |
| handleOpenProviderEditor(provider); | |
| }, | |
| [handleOpenProviderEditor, providers] | |
| ); | |
| const handleRequestEditProvider = useCallback( | |
| (providerId: number) => { | |
| const provider = providers.find((item) => item.id === providerId); | |
| if (!provider) return; | |
| // Open the editor without forcibly switching away from groups view | |
| setEditingProviderId(provider.id); | |
| }, | |
| [providers] | |
| ); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/provider-manager.tsx
Line: 366-378
Comment:
**View mode switches away from groups after editing a provider**
`handleOpenProviderEditor` always calls `setViewMode("list")`, so clicking the edit entry in the groups panel switches the view to list. After the user closes the dialog, they land on the list tab rather than returning to the groups tab they came from. Since the editor dialog is a modal overlay mounted at `ProviderManager` level and independent of `viewMode`, the `setViewMode("list")` call isn't needed for the dialog to appear.
Consider keeping the view state when editing from the groups context:
```suggestion
const handleRequestEditProvider = useCallback(
(providerId: number) => {
const provider = providers.find((item) => item.id === providerId);
if (!provider) return;
// Open the editor without forcibly switching away from groups view
setEditingProviderId(provider.id);
},
[providers]
);
```
How can I resolve this? If you propose a fix, please make it concise.
Owner
Author
There was a problem hiding this comment.
已处理。handleOpenProviderEditor 不再切换 viewMode，现在只设置 editingProviderId，因此从 groups 视图打开编辑弹窗后会保留当前上下文。对应测试已更新。
Comment on lines
+818
to
+1011
| interface InlineTextEditPopoverProps { | ||
| value: string; | ||
| emptyLabel: string; | ||
| label: string; | ||
| placeholder: string; | ||
| validator: (value: string) => string | null; | ||
| onSave: (value: string) => Promise<boolean>; | ||
| } | ||
|
|
||
| function InlineTextEditPopover({ | ||
| value, | ||
| emptyLabel, | ||
| label, | ||
| placeholder, | ||
| validator, | ||
| onSave, | ||
| }: InlineTextEditPopoverProps) { | ||
| const t = useTranslations("settings.providers.providerGroups"); | ||
| const isDesktop = useMediaQuery("(min-width: 768px)"); | ||
| const [open, setOpen] = useState(false); | ||
| const [draft, setDraft] = useState(value); | ||
| const [saving, setSaving] = useState(false); | ||
| const inputRef = useRef<HTMLInputElement>(null); | ||
|
|
||
| const trimmedDraft = draft.trim(); | ||
| const validationError = useMemo(() => validator(trimmedDraft), [trimmedDraft, validator]); | ||
| const canSave = !saving && validationError == null; | ||
|
|
||
| useEffect(() => { | ||
| if (!open) return; | ||
| const raf = requestAnimationFrame(() => { | ||
| inputRef.current?.focus(); | ||
| inputRef.current?.select(); | ||
| }); | ||
| return () => cancelAnimationFrame(raf); | ||
| }, [open]); | ||
|
|
||
| const resetDraft = useCallback(() => { | ||
| setDraft(value); | ||
| }, [value]); | ||
|
|
||
| const handleOpenChange = useCallback( | ||
| (nextOpen: boolean) => { | ||
| if (nextOpen) { | ||
| setDraft(value); | ||
| } else { | ||
| resetDraft(); | ||
| setSaving(false); | ||
| } | ||
| setOpen(nextOpen); | ||
| }, | ||
| [resetDraft, value] | ||
| ); | ||
|
|
||
| const handleCancel = useCallback(() => { | ||
| resetDraft(); | ||
| setOpen(false); | ||
| }, [resetDraft]); | ||
|
|
||
| const handleSave = useCallback(async () => { | ||
| if (!canSave) return; | ||
| setSaving(true); | ||
| try { | ||
| const ok = await onSave(trimmedDraft); | ||
| if (ok) { | ||
| setOpen(false); | ||
| } | ||
| } finally { | ||
| setSaving(false); | ||
| } | ||
| }, [canSave, onSave, trimmedDraft]); | ||
|
|
||
| const stopPropagation = (event: React.SyntheticEvent) => { | ||
| event.stopPropagation(); | ||
| }; | ||
|
|
||
| const trigger = ( | ||
| <button | ||
| type="button" | ||
| className={cn( | ||
| "max-w-full rounded-sm text-left underline-offset-4 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-1", | ||
| value | ||
| ? "text-muted-foreground hover:underline" | ||
| : "text-muted-foreground/80 italic hover:underline" | ||
| )} | ||
| onPointerDown={stopPropagation} | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| if (!isDesktop) handleOpenChange(true); | ||
| }} | ||
| > | ||
| <span className="line-clamp-2">{value || emptyLabel}</span> | ||
| </button> | ||
| ); | ||
|
|
||
| const inputProps = { | ||
| ref: inputRef, | ||
| value: draft, | ||
| placeholder, | ||
| onChange: (event: React.ChangeEvent<HTMLInputElement>) => setDraft(event.target.value), | ||
| disabled: saving, | ||
| "aria-label": label, | ||
| "aria-invalid": validationError != null, | ||
| onPointerDown: stopPropagation, | ||
| onClick: stopPropagation, | ||
| onKeyDown: (event: React.KeyboardEvent<HTMLInputElement>) => { | ||
| event.stopPropagation(); | ||
| if (event.key === "Escape") { | ||
| event.preventDefault(); | ||
| handleCancel(); | ||
| } | ||
| if (event.key === "Enter") { | ||
| event.preventDefault(); | ||
| void handleSave(); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| const content = ( | ||
| <div className="grid gap-2"> | ||
| <div className="hidden text-xs font-medium md:block">{label}</div> | ||
| <Input {...inputProps} className="w-full md:w-[320px]" /> | ||
| {validationError ? <div className="text-xs text-destructive">{validationError}</div> : null} | ||
| <div className="flex items-center justify-end gap-2 pt-1"> | ||
| <Button type="button" size="sm" variant="outline" onClick={handleCancel} disabled={saving}> | ||
| {t("cancel")} | ||
| </Button> | ||
| <Button type="button" size="sm" onClick={handleSave} disabled={!canSave}> | ||
| {saving ? <Loader2 className="mr-2 h-4 w-4 animate-spin" /> : null} | ||
| {t("save")} | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| ); | ||
|
|
||
| if (!isDesktop) { | ||
| return ( | ||
| <> | ||
| {trigger} | ||
| <Drawer open={open} onOpenChange={handleOpenChange}> | ||
| <DrawerContent> | ||
| <DrawerHeader> | ||
| <DrawerTitle>{label}</DrawerTitle> | ||
| </DrawerHeader> | ||
| <div className="px-4 pb-6"> | ||
| <div className="grid gap-3"> | ||
| <Input | ||
| {...inputProps} | ||
| className="text-base" | ||
| onPointerDown={undefined} | ||
| onClick={undefined} | ||
| /> | ||
| {validationError ? ( | ||
| <div className="text-sm text-destructive">{validationError}</div> | ||
| ) : null} | ||
| <div className="flex gap-2 pt-2"> | ||
| <Button | ||
| variant="outline" | ||
| onClick={handleCancel} | ||
| disabled={saving} | ||
| className="flex-1" | ||
| size="lg" | ||
| > | ||
| {t("cancel")} | ||
| </Button> | ||
| <Button onClick={handleSave} disabled={!canSave} className="flex-1" size="lg"> | ||
| {saving ? <Loader2 className="mr-2 h-4 w-4 animate-spin" /> : null} | ||
| {t("save")} | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </DrawerContent> | ||
| </Drawer> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Popover open={open} onOpenChange={handleOpenChange}> | ||
| <PopoverTrigger asChild>{trigger}</PopoverTrigger> | ||
| <PopoverContent | ||
| align="start" | ||
| side="bottom" | ||
| sideOffset={6} | ||
| className="w-auto p-3" | ||
| onPointerDown={stopPropagation} | ||
| onClick={stopPropagation} | ||
| > | ||
| {content} | ||
| </PopoverContent> | ||
| </Popover> | ||
| ); | ||
| } |
There was a problem hiding this comment.
InlineTextEditPopover duplicates the existing InlineEditPopover pattern
InlineTextEditPopover (lines 818–1011) re-implements the same popover/drawer responsive pattern as InlineEditPopover in inline-edit-popover.tsx, including identical requestAnimationFrame focus logic, resetDraft, handleOpenChange, Drawer vs Popover branching, and keyboard shortcut handling (Enter/Escape). The only difference is that it works on string instead of number.
Extracting this into a shared InlineTextEditPopover file (or generalising the existing InlineEditPopover with a generic type parameter) would avoid maintaining two parallel implementations of the same UI pattern.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/provider-group-tab.tsx
Line: 818-1011
Comment:
**`InlineTextEditPopover` duplicates the existing `InlineEditPopover` pattern**
`InlineTextEditPopover` (lines 818–1011) re-implements the same popover/drawer responsive pattern as `InlineEditPopover` in `inline-edit-popover.tsx`, including identical `requestAnimationFrame` focus logic, `resetDraft`, `handleOpenChange`, Drawer vs Popover branching, and keyboard shortcut handling (`Enter`/`Escape`). The only difference is that it works on `string` instead of `number`.
Extracting this into a shared `InlineTextEditPopover` file (or generalising the existing `InlineEditPopover` with a generic type parameter) would avoid maintaining two parallel implementations of the same UI pattern.
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx (2)
276-279:⚠️ Potential issue | 🟠 Major把所有编辑入口都按
isAdmin/canEdit门控。现在非管理员仍能看到“新增群组”、群组编辑/删除按钮，以及成员行的 provider 编辑入口；这些都会触发修改流程。既然该组件已经传入
isAdmin并向成员面板传入canEdit，这些入口也应一致隐藏或禁用。建议调整
- <Button onClick={openCreateDialog} size="sm"> - <Plus className="mr-1.5 h-4 w-4" /> - {t("addGroup")} - </Button> + {isAdmin ? ( + <Button onClick={openCreateDialog} size="sm"> + <Plus className="mr-1.5 h-4 w-4" /> + {t("addGroup")} + </Button> + ) : null}- <div className="flex items-center justify-end gap-1"> - <Button - variant="ghost" - size="icon" - className="h-8 w-8" - onClick={() => openEditDialog(group)} - title={t("editGroup")} - > - <Pencil className="h-3.5 w-3.5" /> - </Button> - <Button - variant="ghost" - size="icon" - className="h-8 w-8 text-destructive hover:text-destructive" - onClick={() => openDeleteConfirm(group)} - disabled={isDefault} - title={isDefault ? t("cannotDeleteDefault") : t("deleteGroup")} - > - <Trash2 className="h-3.5 w-3.5" /> - </Button> - </div> + {isAdmin ? ( + <div className="flex items-center justify-end gap-1"> + <Button + variant="ghost" + size="icon" + className="h-8 w-8" + onClick={() => openEditDialog(group)} + title={t("editGroup")} + > + <Pencil className="h-3.5 w-3.5" /> + </Button> + <Button + variant="ghost" + size="icon" + className="h-8 w-8 text-destructive hover:text-destructive" + onClick={() => openDeleteConfirm(group)} + disabled={isDefault} + title={isDefault ? t("cannotDeleteDefault") : t("deleteGroup")} + > + <Trash2 className="h-3.5 w-3.5" /> + </Button> + </div> + ) : null}- <button - type="button" - className="truncate text-left font-medium underline-offset-4 hover:underline" - onClick={() => onRequestEditProvider(member.id)} - > - {member.name} - </button> + {canEdit ? ( + <button + type="button" + className="truncate text-left font-medium underline-offset-4 hover:underline" + onClick={() => onRequestEditProvider(member.id)} + > + {member.name} + </button> + ) : ( + <span className="truncate text-left font-medium">{member.name}</span> + )} @@ - <Button - size="sm" - variant="ghost" - className="h-8 px-2" - onClick={() => onRequestEditProvider(member.id)} - title={t("openProviderEditor")} - > - <Edit className="h-4 w-4" /> - </Button> + {canEdit ? ( + <Button + size="sm" + variant="ghost" + className="h-8 px-2" + onClick={() => onRequestEditProvider(member.id)} + title={t("openProviderEditor")} + aria-label={t("openProviderEditor")} + > + <Edit className="h-4 w-4" /> + </Button> + ) : null}Also applies to: 373-393, 773-812
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx around lines 276 - 279, The Add Group button and all edit/delete provider controls must be gated by the passed isAdmin/canEdit props: update the component in provider-group-tab.tsx so the Plus/Add Group Button (openCreateDialog), group edit/delete buttons, and the provider edit entry in the member row check isAdmin or canEdit before rendering or enablement; use the existing isAdmin prop for top-level controls and the canEdit prop passed down to the members panel (e.g. where provider edit links/buttons are rendered) to conditionally hide or disable those elements so non-admins cannot see or trigger modification flows (also apply same gating logic to the other edit/delete button blocks referenced in the file).
178-216:⚠️ Potential issue | 🟡 Minor让弹窗保存路径也校验描述长度。
validateDescription()只用于 inline 编辑；通过新增/编辑群组弹窗保存时，form.description可以超过 500 字符并直接提交，和新增的descriptionTooLong约束不一致。建议调整
const trimmedName = form.name.trim(); + const trimmedDescription = form.description.trim(); if (!editingGroup && !trimmedName) { toast.error(t("nameRequired")); return; } + if (trimmedDescription.length > 500) { + toast.error(t("descriptionTooLong")); + return; + } startSaveTransition(async () => { if (editingGroup) { const ok = await saveGroupPatch(editingGroup.id, { costMultiplier, - description: form.description.trim() || null, + description: trimmedDescription || null, }); @@ const result = await createProviderGroup({ name: trimmedName, costMultiplier, - description: form.description.trim() || undefined, + description: trimmedDescription || undefined, });Also applies to: 255-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx around lines 178 - 216, handleSave currently doesn't validate description length so form.description can exceed the 500-char constraint enforced elsewhere (validateDescription). Add a length check in handleSave before calling saveGroupPatch/createProviderGroup: compute const desc = form.description.trim(); if (desc.length > 500) { toast.error(t("descriptionTooLong")); return; } then use desc in the payload (description: editingGroup ? (desc || null) : (desc || undefined)) so both edit and create paths respect the same 500-character constraint and preserve existing null/undefined handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/settings/providers/_components/provider-manager.tsx:
- Around line 366-369: The handler handleOpenProviderEditor currently forces the
UI back to list view by calling setViewMode("list"); remove that call so opening
the provider editor only sets the editing id (setEditingProviderId(provider.id))
and leaves viewMode unchanged; update handleOpenProviderEditor to only call
setEditingProviderId(provider.id) to preserve the group context and expanded
state when the Dialog (provider editor) is rendered independently.
In `@tests/unit/settings/providers/provider-group-tab.test.tsx`:
- Around line 39-93: makeProvider 的返回对象缺少若干 ProviderDisplay 的必填字段，导致类型检查失败；在
makeProvider(...) 返回的字面量中添加缺失字段
disableSessionReuse、activeTimeStart、activeTimeEnd、allowedClients、blockedClients、swapCacheTtlBilling、codexServiceTierPreference、anthropicAdaptiveThinking（使用合理的默认值，例如布尔/字符串/数组/数字/null
等），并保留 ...overrides 以允许覆盖，以确保 ProviderDisplay 类型完整且测试 fixture 正常通过类型检查。
---
Outside diff comments:
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 276-279: The Add Group button and all edit/delete provider
controls must be gated by the passed isAdmin/canEdit props: update the component
in provider-group-tab.tsx so the Plus/Add Group Button (openCreateDialog), group
edit/delete buttons, and the provider edit entry in the member row check isAdmin
or canEdit before rendering or enablement; use the existing isAdmin prop for
top-level controls and the canEdit prop passed down to the members panel (e.g.
where provider edit links/buttons are rendered) to conditionally hide or disable
those elements so non-admins cannot see or trigger modification flows (also
apply same gating logic to the other edit/delete button blocks referenced in the
file).
- Around line 178-216: handleSave currently doesn't validate description length
so form.description can exceed the 500-char constraint enforced elsewhere
(validateDescription). Add a length check in handleSave before calling
saveGroupPatch/createProviderGroup: compute const desc =
form.description.trim(); if (desc.length > 500) {
toast.error(t("descriptionTooLong")); return; } then use desc in the payload
(description: editingGroup ? (desc || null) : (desc || undefined)) so both edit
and create paths respect the same 500-character constraint and preserve existing
null/undefined handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb13840d-f37e-4b1a-976d-4669d1cb5483
📒 Files selected for processing (12)
messages/en/settings/providers/providerGroups.jsonmessages/ja/settings/providers/providerGroups.jsonmessages/ru/settings/providers/providerGroups.jsonmessages/zh-CN/settings/providers/providerGroups.jsonmessages/zh-TW/settings/providers/providerGroups.jsonsrc/app/[locale]/settings/providers/_components/batch-edit/provider-batch-toolbar.tsxsrc/app/[locale]/settings/providers/_components/provider-group-tab.tsxsrc/app/[locale]/settings/providers/_components/provider-list.tsxsrc/app/[locale]/settings/providers/_components/provider-manager.tsxtests/unit/settings/providers/provider-batch-toolbar-selection.test.tsxtests/unit/settings/providers/provider-group-tab.test.tsxtests/unit/settings/providers/provider-manager.test.tsx
Comment on lines
+39
to
+93
| function makeProvider(overrides: Partial<ProviderDisplay> = {}): ProviderDisplay { | ||
| return { | ||
| id: 1, | ||
| name: "Provider A", | ||
| url: "https://api.example.com", | ||
| maskedKey: "sk-***", | ||
| isEnabled: true, | ||
| weight: 1, | ||
| priority: 1, | ||
| costMultiplier: 1.5, | ||
| groupTag: "premium", | ||
| groupPriorities: { premium: 3 }, | ||
| providerType: "claude", | ||
| providerVendorId: null, | ||
| preserveClientIp: false, | ||
| modelRedirects: null, | ||
| allowedModels: null, | ||
| mcpPassthroughType: "none", | ||
| mcpPassthroughUrl: null, | ||
| limit5hUsd: null, | ||
| limitDailyUsd: null, | ||
| dailyResetMode: "fixed", | ||
| dailyResetTime: "00:00", | ||
| limitWeeklyUsd: null, | ||
| limitMonthlyUsd: null, | ||
| limitTotalUsd: null, | ||
| limitConcurrentSessions: 1, | ||
| maxRetryAttempts: null, | ||
| circuitBreakerFailureThreshold: 1, | ||
| circuitBreakerOpenDuration: 60, | ||
| circuitBreakerHalfOpenSuccessThreshold: 1, | ||
| proxyUrl: null, | ||
| proxyFallbackToDirect: false, | ||
| firstByteTimeoutStreamingMs: 0, | ||
| streamingIdleTimeoutMs: 0, | ||
| requestTimeoutNonStreamingMs: 0, | ||
| websiteUrl: null, | ||
| faviconUrl: null, | ||
| cacheTtlPreference: null, | ||
| context1mPreference: null, | ||
| codexReasoningEffortPreference: null, | ||
| codexReasoningSummaryPreference: null, | ||
| codexTextVerbosityPreference: null, | ||
| codexParallelToolCallsPreference: null, | ||
| anthropicMaxTokensPreference: null, | ||
| anthropicThinkingBudgetPreference: null, | ||
| geminiGoogleSearchPreference: null, | ||
| tpm: null, | ||
| rpm: null, | ||
| rpd: null, | ||
| cc: null, | ||
| createdAt: "2026-01-01", | ||
| updatedAt: "2026-01-01", | ||
| ...overrides, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 查看 ProviderDisplay 必填字段与测试 fixture 字段，确认是否仍有遗漏。
rg -n -A90 'export interface ProviderDisplay' --glob 'src/types/provider.ts'
rg -n -A70 'function makeProvider' --glob 'tests/unit/settings/providers/provider-group-tab.test.tsx'Repository: ding113/claude-code-hub
Length of output: 11803
补齐 ProviderDisplay 的必填字段，避免类型检查失败。
makeProvider() 缺少以下必填字段：disableSessionReuse、activeTimeStart、activeTimeEnd、allowedClients、blockedClients、swapCacheTtlBilling、codexServiceTierPreference、anthropicAdaptiveThinking。fixture 不完整会导致 TypeScript 类型检查失败。
建议补齐 fixture
providerVendorId: null,
preserveClientIp: false,
+ disableSessionReuse: false,
modelRedirects: null,
+ activeTimeStart: null,
+ activeTimeEnd: null,
allowedModels: null,
+ allowedClients: [],
+ blockedClients: [],
mcpPassthroughType: "none",
mcpPassthroughUrl: null,
@@
faviconUrl: null,
cacheTtlPreference: null,
+ swapCacheTtlBilling: false,
context1mPreference: null,
@@
codexTextVerbosityPreference: null,
codexParallelToolCallsPreference: null,
+ codexServiceTierPreference: null,
anthropicMaxTokensPreference: null,
anthropicThinkingBudgetPreference: null,
+ anthropicAdaptiveThinking: null,
geminiGoogleSearchPreference: null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function makeProvider(overrides: Partial<ProviderDisplay> = {}): ProviderDisplay { | |
| return { | |
| id: 1, | |
| name: "Provider A", | |
| url: "https://api.example.com", | |
| maskedKey: "sk-***", | |
| isEnabled: true, | |
| weight: 1, | |
| priority: 1, | |
| costMultiplier: 1.5, | |
| groupTag: "premium", | |
| groupPriorities: { premium: 3 }, | |
| providerType: "claude", | |
| providerVendorId: null, | |
| preserveClientIp: false, | |
| modelRedirects: null, | |
| allowedModels: null, | |
| mcpPassthroughType: "none", | |
| mcpPassthroughUrl: null, | |
| limit5hUsd: null, | |
| limitDailyUsd: null, | |
| dailyResetMode: "fixed", | |
| dailyResetTime: "00:00", | |
| limitWeeklyUsd: null, | |
| limitMonthlyUsd: null, | |
| limitTotalUsd: null, | |
| limitConcurrentSessions: 1, | |
| maxRetryAttempts: null, | |
| circuitBreakerFailureThreshold: 1, | |
| circuitBreakerOpenDuration: 60, | |
| circuitBreakerHalfOpenSuccessThreshold: 1, | |
| proxyUrl: null, | |
| proxyFallbackToDirect: false, | |
| firstByteTimeoutStreamingMs: 0, | |
| streamingIdleTimeoutMs: 0, | |
| requestTimeoutNonStreamingMs: 0, | |
| websiteUrl: null, | |
| faviconUrl: null, | |
| cacheTtlPreference: null, | |
| context1mPreference: null, | |
| codexReasoningEffortPreference: null, | |
| codexReasoningSummaryPreference: null, | |
| codexTextVerbosityPreference: null, | |
| codexParallelToolCallsPreference: null, | |
| anthropicMaxTokensPreference: null, | |
| anthropicThinkingBudgetPreference: null, | |
| geminiGoogleSearchPreference: null, | |
| tpm: null, | |
| rpm: null, | |
| rpd: null, | |
| cc: null, | |
| createdAt: "2026-01-01", | |
| updatedAt: "2026-01-01", | |
| ...overrides, | |
| }; | |
| function makeProvider(overrides: Partial<ProviderDisplay> = {}): ProviderDisplay { | |
| return { | |
| id: 1, | |
| name: "Provider A", | |
| url: "https://api.example.com", | |
| maskedKey: "sk-***", | |
| isEnabled: true, | |
| weight: 1, | |
| priority: 1, | |
| costMultiplier: 1.5, | |
| groupTag: "premium", | |
| groupPriorities: { premium: 3 }, | |
| providerType: "claude", | |
| providerVendorId: null, | |
| preserveClientIp: false, | |
| disableSessionReuse: false, | |
| modelRedirects: null, | |
| activeTimeStart: null, | |
| activeTimeEnd: null, | |
| allowedModels: null, | |
| allowedClients: [], | |
| blockedClients: [], | |
| mcpPassthroughType: "none", | |
| mcpPassthroughUrl: null, | |
| limit5hUsd: null, | |
| limitDailyUsd: null, | |
| dailyResetMode: "fixed", | |
| dailyResetTime: "00:00", | |
| limitWeeklyUsd: null, | |
| limitMonthlyUsd: null, | |
| limitTotalUsd: null, | |
| limitConcurrentSessions: 1, | |
| maxRetryAttempts: null, | |
| circuitBreakerFailureThreshold: 1, | |
| circuitBreakerOpenDuration: 60, | |
| circuitBreakerHalfOpenSuccessThreshold: 1, | |
| proxyUrl: null, | |
| proxyFallbackToDirect: false, | |
| firstByteTimeoutStreamingMs: 0, | |
| streamingIdleTimeoutMs: 0, | |
| requestTimeoutNonStreamingMs: 0, | |
| websiteUrl: null, | |
| faviconUrl: null, | |
| cacheTtlPreference: null, | |
| swapCacheTtlBilling: false, | |
| context1mPreference: null, | |
| codexReasoningEffortPreference: null, | |
| codexReasoningSummaryPreference: null, | |
| codexTextVerbosityPreference: null, | |
| codexParallelToolCallsPreference: null, | |
| codexServiceTierPreference: null, | |
| anthropicMaxTokensPreference: null, | |
| anthropicThinkingBudgetPreference: null, | |
| anthropicAdaptiveThinking: null, | |
| geminiGoogleSearchPreference: null, | |
| tpm: null, | |
| rpm: null, | |
| rpd: null, | |
| cc: null, | |
| createdAt: "2026-01-01", | |
| updatedAt: "2026-01-01", | |
| ...overrides, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/providers/provider-group-tab.test.tsx` around lines 39 -
93, makeProvider 的返回对象缺少若干 ProviderDisplay 的必填字段，导致类型检查失败；在 makeProvider(...)
返回的字面量中添加缺失字段
disableSessionReuse、activeTimeStart、activeTimeEnd、allowedClients、blockedClients、swapCacheTtlBilling、codexServiceTierPreference、anthropicAdaptiveThinking（使用合理的默认值，例如布尔/字符串/数组/数字/null
等），并保留 ...overrides 以允许覆盖，以确保 ProviderDisplay 类型完整且测试 fixture 正常通过类型检查。
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the provider groups management interface. Key changes include the introduction of inline editing for group cost multipliers and descriptions, the addition of multi-select and batch action capabilities for group members, and the integration of a shared provider editor dialog within the manager. The UI has been refined with better spacing, icons, and badges, supported by updated translations across multiple locales and new unit tests. Feedback is provided regarding the extraction of a large sub-component to improve file maintainability.
Comment on lines
+818
to
+1011
| interface InlineTextEditPopoverProps { | ||
| value: string; | ||
| emptyLabel: string; | ||
| label: string; | ||
| placeholder: string; | ||
| validator: (value: string) => string | null; | ||
| onSave: (value: string) => Promise<boolean>; | ||
| } | ||
|
|
||
| function InlineTextEditPopover({ | ||
| value, | ||
| emptyLabel, | ||
| label, | ||
| placeholder, | ||
| validator, | ||
| onSave, | ||
| }: InlineTextEditPopoverProps) { | ||
| const t = useTranslations("settings.providers.providerGroups"); | ||
| const isDesktop = useMediaQuery("(min-width: 768px)"); | ||
| const [open, setOpen] = useState(false); | ||
| const [draft, setDraft] = useState(value); | ||
| const [saving, setSaving] = useState(false); | ||
| const inputRef = useRef<HTMLInputElement>(null); | ||
|
|
||
| const trimmedDraft = draft.trim(); | ||
| const validationError = useMemo(() => validator(trimmedDraft), [trimmedDraft, validator]); | ||
| const canSave = !saving && validationError == null; | ||
|
|
||
| useEffect(() => { | ||
| if (!open) return; | ||
| const raf = requestAnimationFrame(() => { | ||
| inputRef.current?.focus(); | ||
| inputRef.current?.select(); | ||
| }); | ||
| return () => cancelAnimationFrame(raf); | ||
| }, [open]); | ||
|
|
||
| const resetDraft = useCallback(() => { | ||
| setDraft(value); | ||
| }, [value]); | ||
|
|
||
| const handleOpenChange = useCallback( | ||
| (nextOpen: boolean) => { | ||
| if (nextOpen) { | ||
| setDraft(value); | ||
| } else { | ||
| resetDraft(); | ||
| setSaving(false); | ||
| } | ||
| setOpen(nextOpen); | ||
| }, | ||
| [resetDraft, value] | ||
| ); | ||
|
|
||
| const handleCancel = useCallback(() => { | ||
| resetDraft(); | ||
| setOpen(false); | ||
| }, [resetDraft]); | ||
|
|
||
| const handleSave = useCallback(async () => { | ||
| if (!canSave) return; | ||
| setSaving(true); | ||
| try { | ||
| const ok = await onSave(trimmedDraft); | ||
| if (ok) { | ||
| setOpen(false); | ||
| } | ||
| } finally { | ||
| setSaving(false); | ||
| } | ||
| }, [canSave, onSave, trimmedDraft]); | ||
|
|
||
| const stopPropagation = (event: React.SyntheticEvent) => { | ||
| event.stopPropagation(); | ||
| }; | ||
|
|
||
| const trigger = ( | ||
| <button | ||
| type="button" | ||
| className={cn( | ||
| "max-w-full rounded-sm text-left underline-offset-4 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring focus-visible:ring-offset-1", | ||
| value | ||
| ? "text-muted-foreground hover:underline" | ||
| : "text-muted-foreground/80 italic hover:underline" | ||
| )} | ||
| onPointerDown={stopPropagation} | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| if (!isDesktop) handleOpenChange(true); | ||
| }} | ||
| > | ||
| <span className="line-clamp-2">{value || emptyLabel}</span> | ||
| </button> | ||
| ); | ||
|
|
||
| const inputProps = { | ||
| ref: inputRef, | ||
| value: draft, | ||
| placeholder, | ||
| onChange: (event: React.ChangeEvent<HTMLInputElement>) => setDraft(event.target.value), | ||
| disabled: saving, | ||
| "aria-label": label, | ||
| "aria-invalid": validationError != null, | ||
| onPointerDown: stopPropagation, | ||
| onClick: stopPropagation, | ||
| onKeyDown: (event: React.KeyboardEvent<HTMLInputElement>) => { | ||
| event.stopPropagation(); | ||
| if (event.key === "Escape") { | ||
| event.preventDefault(); | ||
| handleCancel(); | ||
| } | ||
| if (event.key === "Enter") { | ||
| event.preventDefault(); | ||
| void handleSave(); | ||
| } | ||
| }, | ||
| }; | ||
|
|
||
| const content = ( | ||
| <div className="grid gap-2"> | ||
| <div className="hidden text-xs font-medium md:block">{label}</div> | ||
| <Input {...inputProps} className="w-full md:w-[320px]" /> | ||
| {validationError ? <div className="text-xs text-destructive">{validationError}</div> : null} | ||
| <div className="flex items-center justify-end gap-2 pt-1"> | ||
| <Button type="button" size="sm" variant="outline" onClick={handleCancel} disabled={saving}> | ||
| {t("cancel")} | ||
| </Button> | ||
| <Button type="button" size="sm" onClick={handleSave} disabled={!canSave}> | ||
| {saving ? <Loader2 className="mr-2 h-4 w-4 animate-spin" /> : null} | ||
| {t("save")} | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| ); | ||
|
|
||
| if (!isDesktop) { | ||
| return ( | ||
| <> | ||
| {trigger} | ||
| <Drawer open={open} onOpenChange={handleOpenChange}> | ||
| <DrawerContent> | ||
| <DrawerHeader> | ||
| <DrawerTitle>{label}</DrawerTitle> | ||
| </DrawerHeader> | ||
| <div className="px-4 pb-6"> | ||
| <div className="grid gap-3"> | ||
| <Input | ||
| {...inputProps} | ||
| className="text-base" | ||
| onPointerDown={undefined} | ||
| onClick={undefined} | ||
| /> | ||
| {validationError ? ( | ||
| <div className="text-sm text-destructive">{validationError}</div> | ||
| ) : null} | ||
| <div className="flex gap-2 pt-2"> | ||
| <Button | ||
| variant="outline" | ||
| onClick={handleCancel} | ||
| disabled={saving} | ||
| className="flex-1" | ||
| size="lg" | ||
| > | ||
| {t("cancel")} | ||
| </Button> | ||
| <Button onClick={handleSave} disabled={!canSave} className="flex-1" size="lg"> | ||
| {saving ? <Loader2 className="mr-2 h-4 w-4 animate-spin" /> : null} | ||
| {t("save")} | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </DrawerContent> | ||
| </Drawer> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <Popover open={open} onOpenChange={handleOpenChange}> | ||
| <PopoverTrigger asChild>{trigger}</PopoverTrigger> | ||
| <PopoverContent | ||
| align="start" | ||
| side="bottom" | ||
| sideOffset={6} | ||
| className="w-auto p-3" | ||
| onPointerDown={stopPropagation} | ||
| onClick={stopPropagation} | ||
| > | ||
| {content} | ||
| </PopoverContent> | ||
| </Popover> | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
The new InlineTextEditPopover component is quite large and self-contained. To improve maintainability and reusability, consider extracting it into its own file, similar to how InlineEditPopover is handled. This will make provider-group-tab.tsx shorter and more focused on its primary responsibility.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const handleRequestEditProvider = useCallback( | ||
| (providerId: number) => { | ||
| const provider = providers.find((item) => item.id === providerId); | ||
| if (!provider) return; |
Contributor
There was a problem hiding this comment.
[HIGH] [UX-ISSUE] View mode switches away from groups after editing a provider
Why this is a problem:
When a user clicks to edit a provider from the groups tab, handleOpenProviderEditor forcibly calls setViewMode("list"). This causes the user to lose their context - after closing the editor dialog, they land on the list tab instead of returning to the groups tab they came from.
Since the editor dialog is a modal overlay mounted at ProviderManager level and independent of viewMode, the setViewMode call is not required for the dialog to appear.
Suggested fix:
Remove the forced view mode switch when editing from groups context:
Suggested change
| if (!provider) return; | |
| const handleOpenProviderEditor = useCallback((provider: ProviderDisplay) => { | |
| setEditingProviderId(provider.id); | |
| }, []); |
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR aligns the provider group tab with the provider list interaction model by adding inline editing, provider type icons, batch management, and shared provider editor support. The implementation is well-structured with proper i18n coverage (5 languages) and test coverage for new functionality. One high-priority UX issue was identified.
PR Size: XL
- Lines changed: 1160 (976 additions + 184 deletions)
- Files changed: 12
Note: Despite the XL label, most of the line count comes from i18n JSON files (5 locales with identical structure) and test files. The core logic changes are concentrated in 2 files (provider-group-tab.tsx and provider-manager.tsx).
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| UX | 0 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
High Priority Issues (Should Fix)
- View mode switches away from groups after editing (
provider-manager.tsx:374)handleOpenProviderEditorforcibly callssetViewMode("list")when opening the provider editor, even when triggered from the groups tab. After closing the dialog, the user lands on the list tab instead of returning to groups. The dialog is a modal overlay independent of viewMode, so the view switch is unnecessary. Confidence: 95
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Claude AI
Owner
Author
|
已根据上一轮 review 更新：
本地已重新执行：
|
Contributor
🧪 测试结果
总体结果: ✅ 所有测试通过 |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/unit/settings/providers/provider-group-tab.test.tsx (1)
39-93:⚠️ Potential issue | 🟠 Major补齐
ProviderDisplay必填字段，避免测试类型检查失败。
makeProvider()返回值标注为ProviderDisplay，但 fixture 仍少了这些字段：disableSessionReuse、activeTimeStart、activeTimeEnd、allowedClients、blockedClients、swapCacheTtlBilling、codexServiceTierPreference、anthropicAdaptiveThinking。严格类型检查下这个测试文件会直接失败。建议补齐 fixture 字段
providerVendorId: null, preserveClientIp: false, + disableSessionReuse: false, modelRedirects: null, + activeTimeStart: null, + activeTimeEnd: null, allowedModels: null, + allowedClients: [], + blockedClients: [], mcpPassthroughType: "none", mcpPassthroughUrl: null, @@ websiteUrl: null, faviconUrl: null, cacheTtlPreference: null, + swapCacheTtlBilling: false, context1mPreference: null, codexReasoningEffortPreference: null, codexReasoningSummaryPreference: null, codexTextVerbosityPreference: null, codexParallelToolCallsPreference: null, + codexServiceTierPreference: null, anthropicMaxTokensPreference: null, anthropicThinkingBudgetPreference: null, + anthropicAdaptiveThinking: null, geminiGoogleSearchPreference: null,可用只读命令核对当前接口与 fixture：
#!/bin/bash rg -n -A120 'export interface ProviderDisplay' --iglob 'provider.ts' rg -n -A80 'function makeProvider' tests/unit/settings/providers/provider-group-tab.test.tsx🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/settings/providers/provider-group-tab.test.tsx` around lines 39 - 93, The fixture function makeProvider returns a value typed as ProviderDisplay but is missing required fields (disableSessionReuse, activeTimeStart, activeTimeEnd, allowedClients, blockedClients, swapCacheTtlBilling, codexServiceTierPreference, anthropicAdaptiveThinking), causing TS failures; update the object returned by makeProvider to include these missing properties with appropriate defaults (e.g., false / null / empty arrays / sensible numeric or string defaults) so it satisfies the ProviderDisplay interface and keep the function signature and overrides spread intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/settings/providers/provider-group-tab.test.tsx`:
- Around line 39-93: The fixture function makeProvider returns a value typed as
ProviderDisplay but is missing required fields (disableSessionReuse,
activeTimeStart, activeTimeEnd, allowedClients, blockedClients,
swapCacheTtlBilling, codexServiceTierPreference, anthropicAdaptiveThinking),
causing TS failures; update the object returned by makeProvider to include these
missing properties with appropriate defaults (e.g., false / null / empty arrays
/ sensible numeric or string defaults) so it satisfies the ProviderDisplay
interface and keep the function signature and overrides spread intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bbabf94-246a-45e8-b34d-040fca672c03
📒 Files selected for processing (4)
src/app/[locale]/settings/providers/_components/provider-group-tab.tsxsrc/app/[locale]/settings/providers/_components/provider-manager.tsxtests/unit/settings/providers/provider-group-tab.test.tsxtests/unit/settings/providers/provider-manager.test.tsx
✅ Files skipped from review due to trivial changes (1)
- tests/unit/settings/providers/provider-manager.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/[locale]/settings/providers/_components/provider-manager.tsx
- src/app/[locale]/settings/providers/_components/provider-group-tab.tsx
| </Badge> | ||
| </TableCell> | ||
| <TableCell> | ||
| {canEdit ? ( |
Contributor
There was a problem hiding this comment.
[Medium] [LOGIC-BUG] Provider edit entry ignores canEdit
Why this is a problem: In src/app/[locale]/settings/providers/_components/provider-group-tab.tsx the provider name button and edit icon both call onRequestEditProvider(member.id) even when canEdit is false (e.g. onClick={() => onRequestEditProvider(member.id)} at the name button and the edit icon). Priority editing is correctly gated behind canEdit, so this creates an inconsistent read-only vs edit surface and can lead to unauthorized edit attempts / confusing UX.
Suggested fix:
// Only allow opening the editor when edits are allowed
{canEdit ? (
<button
type="button"
className="truncate text-left font-medium underline-offset-4 hover:underline"
onClick={() => onRequestEditProvider(member.id)}
>
{member.name}
</button>
) : (
<span className="truncate font-medium">{member.name}</span>
)}
{canEdit ? (
<Button
size="sm"
variant="ghost"
className="h-8 px-2"
onClick={() => onRequestEditProvider(member.id)}
title={t("openProviderEditor")}
>
<Edit className="h-4 w-4" />
</Button>
) : null}
Owner
Author
There was a problem hiding this comment.
已处理。这里现在和上面的优先级编辑一致，只有 canEdit 时才渲染供应商编辑入口；非管理员场景新增了单测覆盖。
Contributor
There was a problem hiding this comment.
Code Review Summary
This PR makes large UI changes to provider group management and routes provider editing through a shared editor dialog. One permission-gating inconsistency was found in the new group members row edit entry.
PR Size: XL
- Lines changed: 1160
- Files changed: 12
- Split suggestion: Separate into (1)
ProviderGroupTabUI refactor + i18n updates, (2) shared provider editor wiring (ProviderManager/ProviderList), (3) test updates.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 0 | 1 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 0 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
- None.
High Priority Issues (Should Fix)
- (Confidence 90) [Medium] [LOGIC-BUG] Provider edit entry ignores
canEdit(src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:773,src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:804).
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
Contributor
There was a problem hiding this comment.
- PR
#1033labeledsize/XL. - Posted 1 inline review comment:
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:773/:804— provider edit entry callsonRequestEditProvider(...)even whencanEditis false; suggested gating the name click + edit icon behindcanEdit. - Submitted the summary review on PR
#1033.
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related PRs
Changes
Core Changes (provider-group-tab.tsx)
Supporting Changes
showSelectByGroupprop to hide group dropdown when inside a group contextonEditProvidercallback prop for external edit triggersTest Coverage
provider-group-tab.test.tsx- Tests provider edit request forwarding from group tabTesting
Automated Tests
bun run build bun run lint bun run lint:fix bun run typecheck bun run test bunx vitest run tests/unit/settings/providers/provider-manager.test.tsx tests/unit/settings/providers/provider-batch-toolbar-selection.test.tsx tests/unit/settings/providers/provider-group-tab.test.tsxManual Testing
Checklist
Description enhanced by Claude AI
Greptile Summary
This PR extends the provider group management tab with inline editing for cost multiplier and description, provider type icons/badges in member rows, shared provider editor forwarding, and batch selection within expanded group panels — aligning the group tab UX with the main provider list.
The view-mode preservation bug noted in the previous review thread (where opening the provider editor from the groups tab would switch the view to "list") has been addressed in the accompanying fix commit by removing the
setViewMode("list")call fromhandleOpenProviderEditor.Confidence Score: 5/5
Safe to merge — the view-mode preservation bug from the prior review thread is resolved, all i18n keys are present across 5 languages, and the new inline editing and batch-management paths are logically consistent with the existing provider list patterns.
Both remaining findings are P2: an i18n namespace reuse nit in InlineTextEditPopover and a potential test-flakiness note for the async useTransition flush. Neither affects production correctness or data integrity.
No files require special attention. provider-group-tab.tsx is the largest change but is well-structured and correctly mirrors existing patterns.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant ProviderGroupTab participant GroupMembersPanel participant MemberRow participant ProviderManager participant ProviderForm User->>ProviderGroupTab: Click chevron (expand group) ProviderGroupTab->>GroupMembersPanel: Render with members[] User->>MemberRow: Click provider name / Edit icon MemberRow->>GroupMembersPanel: onRequestEditProvider(providerId) GroupMembersPanel->>ProviderGroupTab: onRequestEditProvider(providerId) ProviderGroupTab->>ProviderManager: onRequestEditProvider(providerId) ProviderManager->>ProviderManager: setEditingProviderId(id) [viewMode stays groups] ProviderManager->>ProviderForm: Open Dialog with provider User->>MemberRow: Click cost multiplier cell MemberRow->>MemberRow: InlineEditPopover opens User->>MemberRow: Enter new value, Save MemberRow->>MemberRow: editProvider(id, group_priorities) MemberRow->>ProviderGroupTab: onSaved() → fetchGroups() User->>GroupMembersPanel: Enter batch mode, select members GroupMembersPanel->>GroupMembersPanel: ProviderBatchToolbar showSelectByGroup=false User->>GroupMembersPanel: Open batch edit GroupMembersPanel->>GroupMembersPanel: ProviderBatchDialog providers=members GroupMembersPanel->>ProviderGroupTab: onSuccess → onSaved()Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: preserve group view context in prov..." | Re-trigger Greptile