fix(provider-groups): sync provider_groups table with groupTag source of truth#1030
fix(provider-groups): sync provider_groups table with groupTag source of truth#1030
Conversation
… of truth Provider group management tab displayed "no groups configured" even when groups existed, because `provider_groups` metadata table was never populated from legacy `providers.groupTag` strings. Also adds per-member priority editing so the tab can manage `groupPriorities` overrides. - repo: add ensureProviderGroupsExist() for idempotent batch upsert with ON CONFLICT DO NOTHING + cache invalidation - action: getProviderGroups read-time self-heal — backfill any groups referenced by providers.groupTag but missing from the table - action: write-time sync in addProvider / single-field edit / batch updateProvider, so new tags immediately materialize group rows - ui: expandable group rows list members with inline per-group priority editing; saves via editProvider.group_priorities merge (only touches the target key, preserving other overrides and global priority) - i18n: add groupMembers / effectivePriority / noMembers / savePriorityFailed / savePrioritySuccess keys across 5 locales Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough扩展提供者组相关本地化字符串（五种语言），新增后端批量保证提供者组存在的仓库函数，并在服务层与前端组件中加入提供者组同步、成员展开与按组优先级编辑功能。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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
bug
🧪 测试结果
总体结果: ✅ 所有测试通过 |
![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
🧹 Nitpick comments (5)
src/repository/provider-groups.ts (1)
179-184: 即使无新增行也会清缓存
onConflictDoNothing不返回受影响行数，当前实现无论是否真的插入了新行都会调用invalidateGroupMultiplierCache()。在批量回填、并发调用或写热路径（addProvider/editProvider/batchUpdateProviders每次写group_tag都会触发）下，会频繁使 60 秒 TTL 的multiplierCache失效，抵消缓存价值。可用.returning({ id: providerGroups.id })得到实际插入条数后再决定是否失效。♻️ 建议改动
- await db + const inserted = await db .insert(providerGroups) .values(unique.map((name) => ({ name }))) - .onConflictDoNothing({ target: providerGroups.name }); - - invalidateGroupMultiplierCache(); + .onConflictDoNothing({ target: providerGroups.name }) + .returning({ id: providerGroups.id }); + + if (inserted.length > 0) { + invalidateGroupMultiplierCache(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/repository/provider-groups.ts` around lines 179 - 184, The current insert into providerGroups always calls invalidateGroupMultiplierCache() even when no rows were inserted; change the db.insert(...).values(...).onConflictDoNothing({ target: providerGroups.name }) chain to include .returning({ id: providerGroups.id }) and capture the returned rows, then only call invalidateGroupMultiplierCache() when the returned rows array is non-empty (i.e. at least one new id was inserted) so the multiplier cache is only invalidated on actual inserts.src/actions/providers.ts (1)
641-649: 三处写时同步可抽小工具
addProvider/editProvider/batchUpdateProviders里三段try { await ensureProviderGroupsExist(parseProviderGroups(tag)) } catch { logger.warn(...) }完全重复。可以抽一个内部 helper（如syncProviderGroupsFromTag(tag, context)）统一处理，降低后续三处漂移的风险。非阻塞。Also applies to: 842-852, 2386-2395
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/providers.ts` around lines 641 - 649, Several places (addProvider, editProvider, batchUpdateProviders) repeat the same try/await ensureProviderGroupsExist(parseProviderGroups(...)) with identical catch logging; extract that logic into a small internal helper syncProviderGroupsFromTag(tag, context) that calls parseProviderGroups(tag) then await ensureProviderGroupsExist(...), catches errors and calls logger.warn("addProvider:provider_groups_sync_failed", { providerId: context?.provider?.id, error: error instanceof Error ? error.message : String(error) }); replace the three duplicated blocks with calls to syncProviderGroupsFromTag(payload.group_tag, { provider }) (or the appropriate context) so all parsing, awaiting and standardized logging live in one place to avoid drift.src/actions/provider-groups.ts (1)
50-82: 重复遍历 providers 可合并第 52-59 行聚合
referenced与第 73-82 行统计groupCounts是两次几乎相同的循环（都parseProviderGroups(provider.groupTag)+ 空列表落到 DEFAULT）。可合并为一次遍历，referenced直接取自groupCounts.keys()，顺便减少一次数组创建。非必须，但能去重逻辑减少未来漂移。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/provider-groups.ts` around lines 50 - 82, The code currently loops providers twice (building referenced set then groupCounts); merge these into one pass over providers where you call parseProviderGroups(provider.groupTag) once, update groupCounts (Map) and also collect referenced names (or derive referenced from groupCounts.keys()), falling back to PROVIDER_GROUP.DEFAULT when parsedGroups is empty; then use referenced to compute missing and call ensureProviderGroupsExist/maybe findAllProviderGroups as before (keep symbols: providers, parseProviderGroups, referenced, groupCounts, PROVIDER_GROUP.DEFAULT, ensureProviderGroupsExist, findAllProviderGroups, initialGroups).src/app/[locale]/settings/providers/_components/provider-group-tab.tsx (2)
286-299: 展开/收起按钮的 aria 语义建议优化
aria-label固定为t("groupMembers")（"组成员"），不随展开/收起状态变化，屏幕阅读器无法感知当前是展开还是收起。建议改用aria-expanded（+ 可选的aria-controls），并把 label 也切换成"展开/收起"语义。属于 a11y 小优化。♿ 建议的改动
<Button variant="ghost" size="icon" className="h-8 w-8" onClick={() => toggleExpand(group.id)} - aria-label={t("groupMembers")} + aria-expanded={isExpanded} + aria-label={isExpanded ? t("collapseMembers") : t("expandMembers")} >需要在 5 种语言的
providerGroups.json中补充expandMembers/collapseMembers文案。🤖 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 286 - 299, The expand/collapse button currently uses a static aria-label t("groupMembers") so screen readers can’t tell state; update the Button rendered in provider-group-tab.tsx to use aria-expanded={isExpanded} (and optionally aria-controls={`group-${group.id}-members`}) and switch the aria-label to t(isExpanded ? "collapseMembers" : "expandMembers") so the label reflects state; also add the new keys "expandMembers" and "collapseMembers" to providerGroups.json in all five languages.
279-282: 可将分组成员索引预先聚合，避免 O(G·P) 遍历每次渲染时，外层
groups.map对每个分组都会在filterGroupMembers内对整个providers做一遍parseProviderGroups+filter。当前规模下没问题，但可以用useMemo把providers一次性按组名聚合成Map<string, ProviderDisplay[]>，渲染时按group.name直接取。纯优化项，可按需延后。♻️ 建议示例
+ const membersByGroup = useMemo(() => { + const map = new Map<string, ProviderDisplay[]>(); + for (const p of providers) { + const tags = parseProviderGroups(p.groupTag); + const keys = tags.length === 0 ? [PROVIDER_GROUP.DEFAULT] : tags; + for (const k of keys) { + const list = map.get(k); + if (list) list.push(p); + else map.set(k, [p]); + } + } + return map; + }, [providers]); ... - const members = filterGroupMembers(providers, group.name); + const members = membersByGroup.get(group.name) ?? [];🤖 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 279 - 282, The render currently calls filterGroupMembers(providers, group.name) inside groups.map causing O(G·P) work because parseProviderGroups runs for each group; fix by computing a Map<string, ProviderDisplay[]> once with useMemo (keyed on providers) that runs parseProviderGroups and groups providers by group.name, then inside the groups.map replace filterGroupMembers(...) with a direct lookup like groupedProviders.get(group.name); keep existing refs such as groups.map, filterGroupMembers, providers, expandedGroups and PROVIDER_GROUP.DEFAULT but remove per-group parsing to avoid repeated work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/actions/provider-groups.ts`:
- Around line 45-69: The read-path healing (in getProviderGroups) currently
calls ensureProviderGroupsExist(missing) and then re-fetches groups via
findAllProviderGroups() without error handling, which causes the whole request
to fail on transient DB errors; wrap the ensureProviderGroupsExist(missing) and
the subsequent groups = await findAllProviderGroups() in a try/catch, log the
error (logger.warn/error) and on catch keep groups = initialGroups so the
function falls back to the original set (still allow aggregation over
providers/parseProviderGroups to include missing names in counts) instead of
throwing.
In `@src/actions/providers.ts`:
- Around line 2386-2395: The log prefix used in the catch for provider group
sync is inconsistent with the surrounding function: change the logger.warn key
from "updateProvider:provider_groups_sync_failed" to
"batchUpdateProviders:provider_groups_sync_failed" inside the
batchUpdateProviders flow; update the call near
ensureProviderGroupsExist(parseProviderGroups(repositoryUpdates.groupTag)) where
logger.warn is invoked so its first argument matches the function name and keep
the existing payload (error message extraction) unchanged.
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 517-523: handleSave currently converts draft to Number which
treats empty or whitespace-only strings as 0; change the validation to first
reject empty/whitespace drafts and non-numeric input before calling Number().
Specifically, inside handleSave (the function using draft, trimmed, value and
calling toast.error(t("savePriorityFailed"))), check trimmed === "" or that
trimmed does not match /^\d+$/ (or otherwise is not an integer string) and call
toast.error/return in that case; only then parse Number(trimmed) and proceed
with the existing Number.isFinite/Number.isInteger/value < 0 checks.
---
Nitpick comments:
In `@src/actions/provider-groups.ts`:
- Around line 50-82: The code currently loops providers twice (building
referenced set then groupCounts); merge these into one pass over providers where
you call parseProviderGroups(provider.groupTag) once, update groupCounts (Map)
and also collect referenced names (or derive referenced from
groupCounts.keys()), falling back to PROVIDER_GROUP.DEFAULT when parsedGroups is
empty; then use referenced to compute missing and call
ensureProviderGroupsExist/maybe findAllProviderGroups as before (keep symbols:
providers, parseProviderGroups, referenced, groupCounts, PROVIDER_GROUP.DEFAULT,
ensureProviderGroupsExist, findAllProviderGroups, initialGroups).
In `@src/actions/providers.ts`:
- Around line 641-649: Several places (addProvider, editProvider,
batchUpdateProviders) repeat the same try/await
ensureProviderGroupsExist(parseProviderGroups(...)) with identical catch
logging; extract that logic into a small internal helper
syncProviderGroupsFromTag(tag, context) that calls parseProviderGroups(tag) then
await ensureProviderGroupsExist(...), catches errors and calls
logger.warn("addProvider:provider_groups_sync_failed", { providerId:
context?.provider?.id, error: error instanceof Error ? error.message :
String(error) }); replace the three duplicated blocks with calls to
syncProviderGroupsFromTag(payload.group_tag, { provider }) (or the appropriate
context) so all parsing, awaiting and standardized logging live in one place to
avoid drift.
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 286-299: The expand/collapse button currently uses a static
aria-label t("groupMembers") so screen readers can’t tell state; update the
Button rendered in provider-group-tab.tsx to use aria-expanded={isExpanded} (and
optionally aria-controls={`group-${group.id}-members`}) and switch the
aria-label to t(isExpanded ? "collapseMembers" : "expandMembers") so the label
reflects state; also add the new keys "expandMembers" and "collapseMembers" to
providerGroups.json in all five languages.
- Around line 279-282: The render currently calls filterGroupMembers(providers,
group.name) inside groups.map causing O(G·P) work because parseProviderGroups
runs for each group; fix by computing a Map<string, ProviderDisplay[]> once with
useMemo (keyed on providers) that runs parseProviderGroups and groups providers
by group.name, then inside the groups.map replace filterGroupMembers(...) with a
direct lookup like groupedProviders.get(group.name); keep existing refs such as
groups.map, filterGroupMembers, providers, expandedGroups and
PROVIDER_GROUP.DEFAULT but remove per-group parsing to avoid repeated work.
In `@src/repository/provider-groups.ts`:
- Around line 179-184: The current insert into providerGroups always calls
invalidateGroupMultiplierCache() even when no rows were inserted; change the
db.insert(...).values(...).onConflictDoNothing({ target: providerGroups.name })
chain to include .returning({ id: providerGroups.id }) and capture the returned
rows, then only call invalidateGroupMultiplierCache() when the returned rows
array is non-empty (i.e. at least one new id was inserted) so the multiplier
cache is only invalidated on actual inserts.
🪄 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: 73e2b426-3fc2-4b06-a44f-9a560b1d59da
📒 Files selected for processing (9)
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/actions/provider-groups.tssrc/actions/providers.tssrc/app/[locale]/settings/providers/_components/provider-group-tab.tsxsrc/repository/provider-groups.ts
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| <Table> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHead>{t("groupName")}</TableHead> |
There was a problem hiding this comment.
t("groupName") resolves to "Group Name" / "分组名称" in every locale, but this column in GroupMembersTable displays the provider name (member.name), not a group name. Admins expanding a group will see a column header that says "Group Name" for a column listing providers — the opposite of what's shown.
A dedicated i18n key (providerName) should be added to all 5 locale files, or a generic "name" key if it already exists.
| <TableHead>{t("groupName")}</TableHead> | |
| <TableHead>{t("providerName")}</TableHead> |
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: 482
Comment:
**Wrong column header in members sub-table**
`t("groupName")` resolves to "Group Name" / "分组名称" in every locale, but this column in `GroupMembersTable` displays the **provider name** (`member.name`), not a group name. Admins expanding a group will see a column header that says "Group Name" for a column listing providers — the opposite of what's shown.
A dedicated i18n key (`providerName`) should be added to all 5 locale files, or a generic `"name"` key if it already exists.
```suggestion
<TableHead>{t("providerName")}</TableHead>
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const existing = (member.groupPriorities ?? null) as Record<string, number> | null; | ||
| const merged: Record<string, number> = { ...(existing ?? {}), [groupName]: value }; | ||
|
|
||
| startSaveTransition(async () => { | ||
| const result = await editProvider(member.id, { group_priorities: merged }); | ||
| if (result.ok) { | ||
| toast.success(t("savePrioritySuccess")); | ||
| onSaved(); | ||
| } else { | ||
| toast.error(result.error ?? t("savePriorityFailed")); | ||
| } | ||
| }); | ||
| }, [draft, member.groupPriorities, member.id, groupName, onSaved, t]); |
There was a problem hiding this comment.
handleSave reads member.groupPriorities from the component's stale prop to build the merged object, then calls editProvider which replaces the field entirely. If the same provider is visible in two expanded group rows simultaneously and both saves race (e.g., admin sets group A priority → sets group B priority before the first fetchGroups() completes), the second save sends a merged object built from the pre-save snapshot, which no longer includes the first save's key — silently losing it.
Consider sending only the delta ({ groupName: value }) and performing the merge atomically on the server (a JSON patch / jsonb_set in Postgres), or at minimum document that concurrent edits on the same provider from two group panels are unsupported.
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: 524-537
Comment:
**Client-side merge can silently clobber concurrent group-priority saves**
`handleSave` reads `member.groupPriorities` from the component's stale prop to build the merged object, then calls `editProvider` which **replaces** the field entirely. If the same provider is visible in two expanded group rows simultaneously and both saves race (e.g., admin sets group A priority → sets group B priority before the first `fetchGroups()` completes), the second save sends a `merged` object built from the pre-save snapshot, which no longer includes the first save's key — silently losing it.
Consider sending only the delta (`{ groupName: value }`) and performing the merge atomically on the server (a JSON patch / `jsonb_set` in Postgres), or at minimum document that concurrent edits on the same provider from two group panels are unsupported.
How can I resolve this? If you propose a fix, please make it concise.| if (missing.length > 0) { | ||
| await ensureProviderGroupsExist(missing); | ||
| // 重新拉取一次，拿到新插入行的完整字段（id/timestamps/默认倍率） | ||
| groups = await findAllProviderGroups(); |
There was a problem hiding this comment.
Unlike the write paths in providers.ts (which wrap ensureProviderGroupsExist in try/catch and log a warning without blocking), the call here is inside the outer try/catch of getProviderGroups. A transient DB error during self-heal returns { ok: false, error: "Failed to fetch provider groups" }, causing the whole tab to show an error. For a best-effort background sync, consider wrapping this block independently so a heal failure degrades gracefully:
try {
await ensureProviderGroupsExist(missing);
groups = await findAllProviderGroups();
} catch (err) {
logger.warn("getProviderGroups:self_heal_failed", { missing, err });
// fall through with initialGroups; next load will retry
}Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/provider-groups.ts
Line: 65-68
Comment:
**Self-heal failure in read path silently breaks the entire response**
Unlike the write paths in `providers.ts` (which wrap `ensureProviderGroupsExist` in try/catch and log a warning without blocking), the call here is inside the outer try/catch of `getProviderGroups`. A transient DB error during self-heal returns `{ ok: false, error: "Failed to fetch provider groups" }`, causing the whole tab to show an error. For a best-effort background sync, consider wrapping this block independently so a heal failure degrades gracefully:
```ts
try {
await ensureProviderGroupsExist(missing);
groups = await findAllProviderGroups();
} catch (err) {
logger.warn("getProviderGroups:self_heal_failed", { missing, err });
// fall through with initialGroups; next load will retry
}
```
How can I resolve this? If you propose a fix, please make it concise.![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: 91c9b445bb
ℹ️ 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".
| const missing = [...referenced].filter((n) => !existingNames.has(n)); | ||
| let groups = initialGroups; | ||
| if (missing.length > 0) { | ||
| await ensureProviderGroupsExist(missing); |
There was a problem hiding this comment.
getProviderGroups now writes missing names from providers.groupTag directly into provider_groups via ensureProviderGroupsExist, but provider tags can be up to 255 chars while provider_groups.name is capped at 200 (providers.groupTag vs provider_groups.name schema limits). When an existing provider has a tag longer than 200 chars, this insert throws and the whole action falls into the catch path, so the group management tab fails to load instead of returning existing groups. Please filter/validate oversized names before calling the sync helper (or skip invalid names) so read-time self-heal cannot take down the page.
Useful? React with 👍 / 👎.
![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 implements a self-healing mechanism for provider groups and enhances the management UI to support viewing group members and editing group-specific priorities. Feedback highlights a potential race condition when updating priorities, redundant database queries and iterations in the backend actions, and opportunities to optimize the frontend rendering performance by memoizing provider-to-group mappings. Additionally, it is suggested to sync group names from the priorities object as well as the tags.
| } | ||
|
|
||
| const existing = (member.groupPriorities ?? null) as Record<string, number> | null; | ||
| const merged: Record<string, number> = { ...(existing ?? {}), [groupName]: value }; |
There was a problem hiding this comment.
There is a potential race condition here. Since merged is constructed using the member object from the component's state, if a user saves priorities for the same provider in two different groups in rapid succession, the second save might overwrite the first one if the state hasn't been refreshed yet. A safer approach would be to perform an atomic JSONB merge on the server side in the editProvider action.
| const referenced = new Set<string>(); | ||
| for (const provider of providers) { | ||
| const parsed = parseProviderGroups(provider.groupTag); | ||
| if (parsed.length === 0) { | ||
| referenced.add(PROVIDER_GROUP.DEFAULT); | ||
| } else { | ||
| for (const name of parsed) referenced.add(name); | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop over providers to aggregate referenced group names is redundant because a similar loop is executed later (lines 73-82) to calculate groupCounts. You can derive the referenced set directly from the keys of the groupCounts map after it is populated, avoiding an extra iteration over the entire provider list.
| const [groupsResult, providersResult] = await Promise.all([ | ||
| getProviderGroups(), | ||
| getProviders(), | ||
| ]); |
There was a problem hiding this comment.
Calling getProviderGroups() and getProviders() in parallel results in redundant database queries. Both server actions internally call findAllProvidersFresh() to fetch the complete list of providers. Consider optimizing this by having getProviderGroups return the provider list as well, or by fetching the providers once and deriving the group metadata from them.
| const [groups, setGroups] = useState<ProviderGroupWithCount[]>([]); | ||
| const [providers, setProviders] = useState<ProviderDisplay[]>([]); | ||
| const [expandedGroups, setExpandedGroups] = useState<Set<number>>(new Set()); | ||
| const [isLoading, startLoadTransition] = useTransition(); |
There was a problem hiding this comment.
To improve performance in the render loop, consider pre-calculating a mapping of group names to their member providers. Currently, filterGroupMembers is called for every group row on every render, which is an O(G * P) operation.
| const [isLoading, startLoadTransition] = useTransition(); | |
| const [isLoading, startLoadTransition] = useTransition(); | |
| const providersByGroup = useMemo(() => { | |
| const map = new Map<string, ProviderDisplay[]>(); | |
| providers.forEach((p) => { | |
| const tags = parseProviderGroups(p.groupTag); | |
| const effectiveTags = tags.length > 0 ? tags : [PROVIDER_GROUP.DEFAULT]; | |
| effectiveTags.forEach((tag) => { | |
| if (!map.has(tag)) map.set(tag, []); | |
| map.get(tag)!.push(p); | |
| }); | |
| }); | |
| return map; | |
| }, [providers]); |
| {groups.map((group) => { | ||
| const isDefault = group.name === PROVIDER_GROUP.DEFAULT; | ||
| const isExpanded = expandedGroups.has(group.id); | ||
| const members = filterGroupMembers(providers, group.name); |
There was a problem hiding this comment.
Use the memoized group mapping to avoid re-filtering the entire provider list for every group row.
| const members = filterGroupMembers(providers, group.name); | |
| const members = providersByGroup.get(group.name) || []; |
| if (payload.group_tag !== undefined) { | ||
| try { | ||
| await ensureProviderGroupsExist(parseProviderGroups(payload.group_tag)); | ||
| } catch (error) { | ||
| logger.warn("editProvider:provider_groups_sync_failed", { | ||
| providerId, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
When updating a provider, we should also ensure that any group names referenced in group_priorities exist in the provider_groups table, not just the ones in group_tag.
| if (payload.group_tag !== undefined) { | |
| try { | |
| await ensureProviderGroupsExist(parseProviderGroups(payload.group_tag)); | |
| } catch (error) { | |
| logger.warn("editProvider:provider_groups_sync_failed", { | |
| providerId, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } | |
| } | |
| // 同步 provider_groups 表（系统级，失败不影响主流程） | |
| const groupsToSync = new Set<string>(); | |
| if (payload.group_tag !== undefined) { | |
| parseProviderGroups(payload.group_tag).forEach((g) => groupsToSync.add(g)); | |
| } | |
| if (payload.group_priorities) { | |
| Object.keys(payload.group_priorities).forEach((g) => groupsToSync.add(g)); | |
| } | |
| if (groupsToSync.size > 0) { | |
| try { | |
| await ensureProviderGroupsExist(Array.from(groupsToSync)); | |
| } catch (error) { | |
| logger.warn("editProvider:provider_groups_sync_failed", { | |
| providerId, | |
| error: error instanceof Error ? error.message : String(error), | |
| }); | |
| } | |
| } |
- Wrap read-time self-heal in try/catch so Tab stays usable when sync
fails (e.g. historical groupTag exceeds provider_groups.name 200-char
cap); filter oversized names before upsert
- Merge the two provider-iteration passes in getProviderGroups into one
- editProvider now also materializes groups referenced only in
group_priorities (previously only group_tag triggered sync)
- batchUpdateProviders log key mismatch (updateProvider -> batchUpdateProviders)
- Member priority editor rejects empty/whitespace input instead of
silently saving 0 (Number("") === 0 bypasses all numeric validators)
- Members sub-table column header fixed: providerName, not groupName
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| <Table> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHead>{t("groupName")}</TableHead> |
There was a problem hiding this comment.
[Medium] [LOGIC-BUG] Group members table header labels provider names as "Group Name"
Why this is a problem: This column renders provider names ({member.name}), but the header uses t("groupName") which resolves to "Group Name" (e.g. messages/en/settings/providers/providerGroups.json:7). This is user-visible and misleading when expanding a group.
Suggested fix:
// src/app/[locale]/settings/providers/_components/provider-group-tab.tsx
<TableHead>{t("providerName")}</TableHead>// messages/en/settings/providers/providerGroups.json
"providerName": "Provider"
| const missing = [...referenced].filter((n) => !existingNames.has(n)); | ||
| let groups = initialGroups; | ||
| if (missing.length > 0) { | ||
| await ensureProviderGroupsExist(missing); |
There was a problem hiding this comment.
[High] [TEST-MISSING-CRITICAL] New getProviderGroups() self-heal branch has no unit coverage
Why this is a problem: This PR adds a new behavior branch that mutates state during reads:
const missing = [...referenced].filter((n) => !existingNames.has(n));await ensureProviderGroupsExist(missing);
Repo rule: **Test Coverage** - All new features must have unit test coverage of at least 80%.
Without a test, regressions in the "missing referenced groups" path (and default-group counting) can ship silently.
Suggested fix:
// tests/unit/actions/provider-groups-get-provider-groups-self-heal.test.ts
import { beforeEach, describe, expect, it, vi } from "vitest";
const getSessionMock = vi.fn();
const findAllProviderGroupsMock = vi.fn();
const findAllProvidersFreshMock = vi.fn();
const ensureProviderGroupsExistMock = vi.fn();
vi.mock("@/lib/auth", () => ({ getSession: getSessionMock }));
vi.mock("@/repository/provider", () => ({ findAllProvidersFresh: findAllProvidersFreshMock }));
vi.mock("@/repository/provider-groups", () => ({
countProvidersUsingGroup: vi.fn(),
ensureProviderGroupsExist: ensureProviderGroupsExistMock,
findAllProviderGroups: findAllProviderGroupsMock,
findProviderGroupById: vi.fn(),
findProviderGroupByName: vi.fn(),
createProviderGroup: vi.fn(),
deleteProviderGroup: vi.fn(),
updateProviderGroup: vi.fn(),
}));
vi.mock("@/lib/audit/emit", () => ({ emitActionAudit: vi.fn() }));
vi.mock("@/lib/logger", () => ({ logger: { error: vi.fn() } }));
describe("getProviderGroups self-heal", () => {
beforeEach(() => {
vi.clearAllMocks();
getSessionMock.mockResolvedValue({ user: { role: "admin" } });
});
it("inserts missing referenced groups and returns refreshed list", async () => {
findAllProvidersFreshMock.mockResolvedValue([{ groupTag: "premium" }, { groupTag: null }]);
findAllProviderGroupsMock
.mockResolvedValueOnce([
{ id: 1, name: "default", costMultiplier: 1, description: null, createdAt: new Date(), updatedAt: new Date() },
])
.mockResolvedValueOnce([
{ id: 1, name: "default", costMultiplier: 1, description: null, createdAt: new Date(), updatedAt: new Date() },
{ id: 2, name: "premium", costMultiplier: 1, description: null, createdAt: new Date(), updatedAt: new Date() },
]);
const { getProviderGroups } = await import("@/actions/provider-groups");
const result = await getProviderGroups();
expect(ensureProviderGroupsExistMock).toHaveBeenCalledWith(["premium"]);
expect(result.ok).toBe(true);
expect(result.data?.find((g) => g.name === "premium")?.providerCount).toBe(1);
expect(result.data?.find((g) => g.name === "default")?.providerCount).toBe(1);
});
});
🧪 测试结果
总体结果: ✅ 所有测试通过 |
| * | ||
| * 不触发 audit——这是系统级同步，非用户显式操作。 | ||
| */ | ||
| export async function ensureProviderGroupsExist(names: string[]): Promise<void> { |
There was a problem hiding this comment.
[High] [TEST-MISSING-CRITICAL] ensureProviderGroupsExist() added without unit tests
Why this is a problem: This PR introduces a new repository helper that writes to provider_groups:
export async function ensureProviderGroupsExist(names: string[]): Promise<void> { ... }await db.insert(providerGroups).values(...).onConflictDoNothing({ target: providerGroups.name });
Repo rule: **Test Coverage** - All new features must have unit test coverage of at least 80%.
There is already a dedicated repository test file (tests/unit/repository/provider-groups.test.ts) that can be extended to cover trimming/deduping and the ON CONFLICT DO NOTHING behavior.
Suggested fix:
// tests/unit/repository/provider-groups.test.ts
// 1) Extend the insert mock chain to support .onConflictDoNothing()
const onConflictDoNothingMock = vi.fn();
function resetChainMocks() {
// ...existing...
insertMock.mockReturnValue({ values: valuesMock });
valuesMock.mockReturnValue({ returning: returningMock, onConflictDoNothing: onConflictDoNothingMock });
onConflictDoNothingMock.mockResolvedValue(undefined);
}
it("ensureProviderGroupsExist() trims/dedupes and skips empties", async () => {
const { ensureProviderGroupsExist } = await import("@/repository/provider-groups");
await ensureProviderGroupsExist([" premium ", "", "premium", "enterprise"]);
expect(valuesMock).toHaveBeenCalledWith([{ name: "premium" }, { name: "enterprise" }]);
expect(onConflictDoNothingMock).toHaveBeenCalledWith({ target: "name" });
});
| <Table> | ||
| <TableHeader> | ||
| <TableRow> | ||
| <TableHead>{t("groupName")}</TableHead> |
There was a problem hiding this comment.
[High] [LOGIC-BUG] Wrong column header in members sub-table
Why this is a problem: The <TableHead> at line 482 uses t("groupName") (renders as "Group Name" / "分组名称" in all 5 locales), but the column actually displays member.name (the provider name) in the MemberRow below. Admins expanding a group will see a column labeled "Group Name" listing provider names — confusing and incorrect across every language.
Suggested fix: Add a new i18n key (e.g. providerName) to all 5 locale files and use it here:
<TableHead>{t("providerName")}</TableHead>
| if (missing.length > 0) { | ||
| await ensureProviderGroupsExist(missing); | ||
| // 重新拉取一次，拿到新插入行的完整字段（id/timestamps/默认倍率） | ||
| groups = await findAllProviderGroups(); |
There was a problem hiding this comment.
[Medium] [ERROR-BROAD-CATCH] Self-heal failure breaks the entire response
Why this is a problem: The write paths in src/actions/providers.ts wrap ensureProviderGroupsExist in their own try/catch with a warn log, so a self-heal failure never blocks the primary action. Here in the read path, the call sits inside the outer try/catch of getProviderGroups (line 90: catch (error) -> returns { ok: false, error: "Failed to fetch provider groups" }). A transient DB hiccup during the best-effort backfill will surface as a full tab error, even though initialGroups was already fetched successfully — the user sees "Failed to fetch provider groups" instead of the (slightly stale) data the system already has.
Suggested fix: Wrap the heal block independently so failures degrade gracefully:
if (missing.length > 0) {
try {
await ensureProviderGroupsExist(missing);
groups = await findAllProviderGroups();
} catch (err) {
logger.warn("getProviderGroups:self_heal_failed", {
missing,
error: err instanceof Error ? err.message : String(err),
});
// fall through with initialGroups; next load retries
}
}
There was a problem hiding this comment.
Code Review Summary
The dual-sync approach (read-time self-heal + write-time propagation) is sound and the idempotent ON CONFLICT DO NOTHING upsert is correctly implemented. Two real issues identified: one user-facing i18n bug (wrong column header across all 5 locales) and one error-handling asymmetry where the read-path self-heal is not isolated from the outer catch.
PR Size: M
- Lines changed: 364 (+318 / -46)
- Files changed: 9
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 0 | 0 |
| Security | 0 | 0 | 0 | 0 |
| Error Handling | 0 | 0 | 1 | 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)
- Wrong column header in members sub-table (
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:482) —t("groupName")labels the provider-name column as "Group Name" / "分组名称" in all 5 locales.
Medium Priority Issues (Consider Fixing)
- Self-heal failure breaks the entire response (
src/actions/provider-groups.ts:65-68) — unlike the symmetric write-path wrappers inproviders.ts, the read-path self-heal is not isolated, so a transient DB error during backfill turns into a full tab error even thoughinitialGroupsalready loaded.
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Claude AI
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b4d433f08
ℹ️ 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".
| const existing = (member.groupPriorities ?? null) as Record<string, number> | null; | ||
| const merged: Record<string, number> = { ...(existing ?? {}), [groupName]: value }; | ||
|
|
||
| startSaveTransition(async () => { | ||
| const result = await editProvider(member.id, { group_priorities: merged }); |
There was a problem hiding this comment.
handleSave computes merged from the row's current member.groupPriorities snapshot and then sends that whole object to editProvider, but group_priorities is stored as a full-field replacement. If the same provider is edited from another expanded group (or another admin session) before fetchGroups() refreshes this row, the later save can overwrite and drop keys written by the earlier save. This creates silent config data loss for concurrent edits; the update should be merged server-side (or refreshed from latest state before submit).
Useful? React with 👍 / 👎.
| await db | ||
| .insert(providerGroups) | ||
| .values(unique.map((name) => ({ name }))) | ||
| .onConflictDoNothing({ target: providerGroups.name }); |
There was a problem hiding this comment.
ensureProviderGroupsExist only trims and de-duplicates names, then bulk inserts all of them in one statement. Since provider_groups.name is limited to 200 chars while source tags can be longer, one oversized value causes the entire insert to fail, so valid shorter names in the same batch are skipped too. In write paths this exception is swallowed with a warning, so provider edits succeed while metadata silently remains unsynced.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review Summary
This PR adds new provider group synchronization behavior (read-time self-heal + write-time sync) and expands the settings UI with per-group member priority editing. The highest risk is regression in the new sync paths because no unit tests were added/updated to lock in the behavior. There is also a user-visible labeling bug in the new group members table.
PR Size: M
- Lines changed: 364
- Files changed: 9
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 | 2 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
None.
High Priority Issues (Should Fix)
- [High] [TEST-MISSING-CRITICAL]
getProviderGroups()self-heal branch lacks unit coverage (src/actions/provider-groups.ts:66, Confidence: 90) - [High] [TEST-MISSING-CRITICAL]
ensureProviderGroupsExist()added without unit coverage (src/repository/provider-groups.ts:175, Confidence: 90) - [Medium] [LOGIC-BUG] Group members table header labels provider names as "Group Name" (
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:482, Confidence: 85)
Review Coverage
- Logic and correctness
- Security (OWASP Top 10)
- Error handling
- Type safety
- Documentation accuracy
- Test coverage
- Code clarity
Automated review by Codex AI
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/actions/provider-groups.ts (1)
70-73:PROVIDER_GROUP_NAME_MAX建议集中定义硬编码的
200依赖注释来与 schema 保持同步，后续 schema 调整时容易漏改。可在@/lib/constants/provider.constants或@/types/provider-group中统一导出，让 repository（upsert 校验）与 action（预过滤）共用同一个常量。非阻塞。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/provider-groups.ts` around lines 70 - 73, The local constant PROVIDER_GROUP_NAME_MAX (used in the missing filter that references referenced and existingNames) should be centralized: add and export a single MAX_NAME (or PROVIDER_GROUP_NAME_MAX) from the shared constants module (e.g., provider.constants or types/provider-group) and replace the local const in provider-groups.ts with an import of that exported constant; also update the repository/upsert validation to consume the same exported constant so both the pre-filter in the missing computation and the upsert validation use the identical source of truth.src/app/[locale]/settings/providers/_components/provider-group-tab.tsx (1)
279-353: 每次渲染都重算所有分组成员 (可选)
filterGroupMembers(providers, group.name)在 groups.map 里每渲染都会对每个组全量遍历 providers。toggleExpand / dialog 开关 / 父组件更新都会触发。当 provider 量级上来后有轻微开销。可在循环外一次性构建Map<groupName, ProviderDisplay[]>，或用useMemo按providers做记忆。非阻塞。🤖 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 279 - 353, The code repeatedly calls filterGroupMembers(providers, group.name) inside the groups.map loop which re-traverses providers on every render; change this to compute a single lookup (e.g., Map<string, ProviderDisplay[]>) for group members outside the map or memoize it with useMemo dependent on providers (and groups if needed), then replace filterGroupMembers calls in the loop with a quick map lookup (used by GroupMembersTable's members prop) to avoid full scans on each render.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 81-94: fetchGroups currently awaits
Promise.all([getProviderGroups(), getProviders()]) inside startLoadTransition
without a try/catch so any rejection (e.g. getProviders) aborts the transition
and hides group results; wrap the awaited calls in error boundaries (either use
try/catch around separate awaits or use Promise.allSettled) so getProviderGroups
and getProviders degrade independently: call getProviderGroups() and if
groupsResult.ok call setGroups(groupsResult.data) else
toast.error(groupsResult.error); for providers, if the call succeeds call
setProviders(...) otherwise toast.error(...) — keep this logic inside the
existing startLoadTransition and ensure rejections are caught to prevent
unhandled promise rejections and to allow partial success to render.
- Around line 505-541: The save handler currently always writes
group_priorities["default"] which downstream routing ignores; update handleSave
so when groupName === "default" it instead writes the provider's top-level
priority: detect the default group name (string "default" or the DEFAULT_GROUP
constant), and call editProvider(member.id, { priority: value }) (optionally
removing any "default" key from group_priorities) rather than merging into
group_priorities; keep the existing behavior for non-default groups (use merged
= { ...(existing ?? {}), [groupName]: value }) and leave toast/onSaved logic
unchanged.
---
Nitpick comments:
In `@src/actions/provider-groups.ts`:
- Around line 70-73: The local constant PROVIDER_GROUP_NAME_MAX (used in the
missing filter that references referenced and existingNames) should be
centralized: add and export a single MAX_NAME (or PROVIDER_GROUP_NAME_MAX) from
the shared constants module (e.g., provider.constants or types/provider-group)
and replace the local const in provider-groups.ts with an import of that
exported constant; also update the repository/upsert validation to consume the
same exported constant so both the pre-filter in the missing computation and the
upsert validation use the identical source of truth.
In `@src/app/`[locale]/settings/providers/_components/provider-group-tab.tsx:
- Around line 279-353: The code repeatedly calls filterGroupMembers(providers,
group.name) inside the groups.map loop which re-traverses providers on every
render; change this to compute a single lookup (e.g., Map<string,
ProviderDisplay[]>) for group members outside the map or memoize it with useMemo
dependent on providers (and groups if needed), then replace filterGroupMembers
calls in the loop with a quick map lookup (used by GroupMembersTable's members
prop) to avoid full scans on each render.
🪄 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: f2009015-31ce-4977-a0ed-159b9e5261b3
📒 Files selected for processing (8)
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/actions/provider-groups.tssrc/actions/providers.tssrc/app/[locale]/settings/providers/_components/provider-group-tab.tsx
✅ Files skipped from review due to trivial changes (4)
- messages/zh-TW/settings/providers/providerGroups.json
- messages/ja/settings/providers/providerGroups.json
- messages/zh-CN/settings/providers/providerGroups.json
- messages/ru/settings/providers/providerGroups.json
🚧 Files skipped from review as they are similar to previous changes (2)
- messages/en/settings/providers/providerGroups.json
- src/actions/providers.ts
| const fetchGroups = useCallback(() => { | ||
| startLoadTransition(async () => { | ||
| const result = await getProviderGroups(); | ||
| if (result.ok) { | ||
| setGroups(result.data); | ||
| const [groupsResult, providersResult] = await Promise.all([ | ||
| getProviderGroups(), | ||
| getProviders(), | ||
| ]); | ||
| if (groupsResult.ok) { | ||
| setGroups(groupsResult.data); | ||
| } else { | ||
| toast.error(result.error); | ||
| toast.error(groupsResult.error); | ||
| } | ||
| setProviders(providersResult); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
fetchGroups 缺失错误边界
Promise.all([getProviderGroups(), getProviders()]) 中若 getProviders() 抛错（网络/鉴权异常），整个 transition 会以未捕获 rejection 结束：既不会走 groupsResult.ok 分支展示分组，也不会提示用户，Tab 会停留在上一状态且没有反馈。即便分组侧成功，也会被整体吞掉。
建议包一层 try/catch，分组与 providers 独立降级：
建议修复
const fetchGroups = useCallback(() => {
startLoadTransition(async () => {
- const [groupsResult, providersResult] = await Promise.all([
- getProviderGroups(),
- getProviders(),
- ]);
- if (groupsResult.ok) {
- setGroups(groupsResult.data);
- } else {
- toast.error(groupsResult.error);
- }
- setProviders(providersResult);
+ const [groupsSettled, providersSettled] = await Promise.allSettled([
+ getProviderGroups(),
+ getProviders(),
+ ]);
+ if (groupsSettled.status === "fulfilled") {
+ if (groupsSettled.value.ok) setGroups(groupsSettled.value.data);
+ else toast.error(groupsSettled.value.error);
+ } else {
+ toast.error(t("loadFailed"));
+ }
+ if (providersSettled.status === "fulfilled") {
+ setProviders(providersSettled.value);
+ }
});
- }, []);
+ }, [t]);🤖 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 81 - 94, fetchGroups currently awaits
Promise.all([getProviderGroups(), getProviders()]) inside startLoadTransition
without a try/catch so any rejection (e.g. getProviders) aborts the transition
and hides group results; wrap the awaited calls in error boundaries (either use
try/catch around separate awaits or use Promise.allSettled) so getProviderGroups
and getProviders degrade independently: call getProviderGroups() and if
groupsResult.ok call setGroups(groupsResult.data) else
toast.error(groupsResult.error); for providers, if the call succeeds call
setProviders(...) otherwise toast.error(...) — keep this logic inside the
existing startLoadTransition and ensure rejections are caught to prevent
unhandled promise rejections and to allow partial success to render.
| const effective = useMemo(() => { | ||
| const groupPriorities = (member.groupPriorities ?? null) as Record<string, number> | null; | ||
| return groupPriorities?.[groupName] ?? member.priority; | ||
| }, [member.groupPriorities, member.priority, groupName]); | ||
|
|
||
| const [draft, setDraft] = useState<string>(String(effective)); | ||
| const [isSaving, startSaveTransition] = useTransition(); | ||
|
|
||
| useEffect(() => { | ||
| setDraft(String(effective)); | ||
| }, [effective]); | ||
|
|
||
| const handleSave = useCallback(() => { | ||
| const trimmed = draft.trim(); | ||
| if (trimmed === "") { | ||
| toast.error(t("savePriorityFailed")); | ||
| return; | ||
| } | ||
| const value = Number(trimmed); | ||
| if (!Number.isFinite(value) || !Number.isInteger(value) || value < 0) { | ||
| toast.error(t("savePriorityFailed")); | ||
| return; | ||
| } | ||
|
|
||
| const existing = (member.groupPriorities ?? null) as Record<string, number> | null; | ||
| const merged: Record<string, number> = { ...(existing ?? {}), [groupName]: value }; | ||
|
|
||
| startSaveTransition(async () => { | ||
| const result = await editProvider(member.id, { group_priorities: merged }); | ||
| if (result.ok) { | ||
| toast.success(t("savePrioritySuccess")); | ||
| onSaved(); | ||
| } else { | ||
| toast.error(result.error ?? t("savePriorityFailed")); | ||
| } | ||
| }); | ||
| }, [draft, member.groupPriorities, member.id, groupName, onSaved, t]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 查找 groupPriorities 在运行时路径中的读取点，确认对 DEFAULT/无 tag 的回退规则
rg -nP --type=ts -C4 '\bgroupPriorities\b' -g '!**/*.test.*' -g '!**/__tests__/**'
# 额外查一下 selector/router 处可能按 groupName 查优先级的关键词
rg -nP --type=ts -C3 '(selectProvider|pickProvider|providerSelector|chooseProvider)' -g '!**/*.test.*'Repository: ding113/claude-code-hub
Length of output: 50046
🏁 Script executed:
rg -nP --type=tsx 'MemberRow' -A5 -B5 -g '!**/*.test.*' | head -100Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
# Look for DEFAULT group references and how provider-group-tab uses groups
rg -nP --type=tsx '"DEFAULT"|\'DEFAULT\'' -C3 src/app/[locale]/settings/providers/Repository: ding113/claude-code-hub
Length of output: 135
🏁 Script executed:
# Check where provider-group-tab is used and what groups are passed
rg -nP --type=tsx 'provider-group-tab|ProviderGroupTab' -C3 -g '!**/*.test.*'Repository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
rg -nP --type=ts 'MemberRow' -A5 -B5 -g '!**/*.test.*' | head -150Repository: ding113/claude-code-hub
Length of output: 2745
🏁 Script executed:
# Look for where MemberRow is instantiated with groupName parameter
rg -nP --type=ts 'MemberRow.*groupName' -B10 -A2 -g '!**/*.test.*'Repository: ding113/claude-code-hub
Length of output: 2787
🏁 Script executed:
# Check provider-group-tab for group iteration and how groupName is determined
rg -nP --type=ts 'groupName.*DEFAULT|DEFAULT.*group|state\.routing\.groupTag' src/app/[locale]/settings/providers/ -C3Repository: ding113/claude-code-hub
Length of output: 5839
🏁 Script executed:
rg -nP --type=ts 'PROVIDER_GROUP\s*[=:]|const.*PROVIDER_GROUP' -A3 -g '!**/*.test.*' | head -50Repository: ding113/claude-code-hub
Length of output: 3778
🏁 Script executed:
# Find where PROVIDER_GROUP is defined
fd -t f -name '*.ts' | xargs grep -l 'PROVIDER_GROUP.*=' | head -5Repository: ding113/claude-code-hub
Length of output: 297
🏁 Script executed:
# Check the full resolveEffectivePriority logic in provider-selector
rg -nP --type=ts 'resolveEffectivePriority' -A15 src/app/v1/_lib/proxy/provider-selector.tsRepository: ding113/claude-code-hub
Length of output: 2784
🏁 Script executed:
# Confirm by checking how effectiveGroupPick is computed when user has no group
rg -nP --type=ts 'effectiveGroupPick\s*=' -B5 -A2 src/app/v1/_lib/proxy/provider-selector.ts | head -60Repository: ding113/claude-code-hub
Length of output: 782
🏁 Script executed:
# Check if there's special handling for "default" string in selector
rg -nP --type=ts '"default"|parseProviderGroups.*default' src/app/v1/_lib/proxy/provider-selector.ts -C2Repository: ding113/claude-code-hub
Length of output: 49
DEFAULT 组优先级写入与下游消费不一致——引发路由静默失效
验证已确认：对未打 tag 的 provider（DEFAULT 组），当前逻辑存在下游消费不匹配问题。
问题详情：
- MemberRow 编辑 DEFAULT 组优先级时，会写入
groupPriorities["default"] = value - 但 provider-selector 的
resolveEffectivePriority()方法（1126-1137 行）只在if (userGroup && provider.groupPriorities)成立时才查询 groupPriorities - 当用户/密钥未分配 providerGroup 时，
effectiveGroupPick = null，条件失败，总是回退到provider.priority - 结果：UI 层显示保存成功（因为 effective 计算为
groupPriorities["default"] ?? priority），但实际路由调度完全忽略groupPriorities["default"]，继续使用原始provider.priority
建议修复：
对 DEFAULT 组编辑时，改为直接更新 priority 字段，而非 groupPriorities["default"]；或在 selector 侧对 DEFAULT 组添加特殊处理，使其不依赖 userGroup 存在的条件。
🤖 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 505 - 541, The save handler currently always writes
group_priorities["default"] which downstream routing ignores; update handleSave
so when groupName === "default" it instead writes the provider's top-level
priority: detect the default group name (string "default" or the DEFAULT_GROUP
constant), and call editProvider(member.id, { priority: value }) (optionally
removing any "default" key from group_priorities) rather than merging into
group_priorities; keep the existing behavior for non-default groups (use merged
= { ...(existing ?? {}), [groupName]: value }) and leave toast/onSaved logic
unchanged.
There was a problem hiding this comment.
- Identified open PR
#1030and applied labelsize/M(318 additions, 46 deletions; 9 files; 364 lines changed total). - Posted inline review comments on diff lines:
src/actions/provider-groups.ts:66— [High][TEST-MISSING-CRITICAL]missing unit coverage for new read-time self-heal branch.src/repository/provider-groups.ts:175— [High][TEST-MISSING-CRITICAL]missing unit coverage for newensureProviderGroupsExist().src/app/[locale]/settings/providers/_components/provider-group-tab.tsx:482— [Medium][LOGIC-BUG]members table header usesgroupNamewhile rendering provider names.
- Submitted the required PR review summary via
gh pr review --comment.
1 participant
Summary
provider_groups元数据表从未与 legacyproviders.groupTag字符串同步groupPriorities[name]（只改 target key，不影响全局 priority 和其他分组覆盖）Related PRs:
provider_groupstable and group management tab; this PR fixes the data sync gap where existinggroupTagvalues were never backfilled into the new tableChanges
后端
src/repository/provider-groups.ts：新增ensureProviderGroupsExist()，ON CONFLICT DO NOTHING幂等批量 upsert，并触发invalidateGroupMultiplierCache()src/actions/provider-groups.ts：getProviderGroups加读时自愈——从providers.groupTag聚合被引用的分组名，补齐缺失的表行（costMultiplier使用默认 1.0，与历史 fallback 行为一致，不改账单）src/actions/providers.ts：3 处写groupTag的路径（addProvider/ 单字段编辑 / 批量updateProvider）成功后调ensureProviderGroupsExist，try/catch 包裹避免阻塞主流程前端
src/app/[locale]/settings/providers/_components/provider-group-tab.tsx：分组行增加展开/折叠；展开后列出成员（过滤parseProviderGroups(groupTag).includes(groupName)+ default 分组兜底），显示当前有效优先级（groupPriorities[name] ?? priority），行内编辑并通过editProvider的group_priorities合并提交messages/{zh-CN,zh-TW,en,ja,ru}/settings/providers/providerGroups.json：补全 5 语言 i18n 键关键设计原则
providers.groupTag仍是路由/下拉/计费的权威来源，provider_groups只是元数据侧表groupTag字符串 +groupPrioritiesJSON key 双重级联问题）costMultiplier/description，用户可在providerCount=0时手动删除editProvideraction：自动触发publishProviderCacheInvalidation，30s provider cache + Redis pub/sub 跨实例生效Test plan
bun run typecheck— 无错误bun run build— 通过bun run lint— 无新增 warningbun run test— 606 provider 相关测试全部通过（1 个 pre-existing 的 API 健康检查失败与本 PR 无关，需要本地 DB/Redis）/dashboard/providers→ 分组管理 Tab → 确认显示所有实际在用分组（至少 default）groupPriorities只有该 key 变化provider_groups应已存在该行🤖 Generated with Claude Code
Greptile Summary
This PR fixes the empty "Provider Groups" tab by introducing a dual consistency mechanism: read-time self-healing in
getProviderGroups(backfills anygroupTag-referenced groups missing fromprovider_groups) and write-time sync on the three main write paths (addProvider,editProvider,batchUpdateProviders). It also adds an inline priority editor per group member. Previous review concerns (wrong column header, self-heal swallowing the full response) are addressed.Confidence Score: 5/5
Safe to merge; the core fix is correct and all prior feedback has been addressed.
The only new finding is a P2 gap in
applyProviderBatchPatch— it doesn't callensureProviderGroupsExistafter writinggroupTag, but the read-time self-heal ingetProviderGroupsalready covers this case on the next tab load, so there is no data-loss or routing/billing impact. All three previously-raised concerns are resolved.src/actions/providers.ts — the
applyProviderBatchPatchfunction is the only write path missing the eager group-sync call.Important Files Changed
ensureProviderGroupsExisttoaddProvider,editProvider, andbatchUpdateProviders;applyProviderBatchPatchis the one write path that can changegroupTagbut lacks the sync call.getProviderGroupswith proper try/catch isolation; correctly filters names exceeding the 200-char schema limit before insertion.ensureProviderGroupsExistwith idempotentON CONFLICT DO NOTHINGbatch insert andinvalidateGroupMultiplierCachecall; clean and correct.GroupMembersTablewith inline priority editing; usesproviderNamei18n key (previous concern addressed);filterGroupMemberscorrectly handles the default-group fallback.providerName,effectivePriority,groupMembers,noMembers,savePriorityFailed,savePrioritySuccess) matching the component usage.Sequence Diagram
sequenceDiagram participant Admin participant ProviderGroupTab participant getProviderGroups participant provider_groups_table participant providers_table Note over Admin,providers_table: Read-time self-heal (tab open) Admin->>ProviderGroupTab: opens Groups tab ProviderGroupTab->>getProviderGroups: call getProviderGroups->>provider_groups_table: findAllProviderGroups() getProviderGroups->>providers_table: findAllProvidersFresh() getProviderGroups->>getProviderGroups: diff referenced vs existing names alt missing groups exist getProviderGroups->>provider_groups_table: ensureProviderGroupsExist(missing) ON CONFLICT DO NOTHING getProviderGroups->>provider_groups_table: findAllProviderGroups() (refresh) end getProviderGroups-->>ProviderGroupTab: groups + providerCount Note over Admin,providers_table: Write-time sync (provider form) Admin->>addProvider: save provider with groupTag addProvider->>providers_table: createProvider() addProvider->>provider_groups_table: ensureProviderGroupsExist(tags) Admin->>editProvider: update groupTag / groupPriorities editProvider->>providers_table: updateProvider() editProvider->>provider_groups_table: ensureProviderGroupsExist(names)Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix(provider-groups): address bugbot rev..." | Re-trigger Greptile