feat(providers): recluster vendors by host:port when website_url empty#670
feat(providers): recluster vendors by host:port when website_url empty#670
Conversation
- Add computeVendorKey helper with host:port support for IP-based providers - When website_url is empty, use host:port as vendor key (different ports = different vendors) - Support IPv6 addresses with [ipv6]:port format - Use protocol default ports (http=80, https=443) when port not specified - Add reclusterProviderVendors action with preview/apply mode - Add ReclusterVendorsDialog UI component - Add i18n support for 5 languages (zh-CN, zh-TW, en, ja, ru) - Existing behavior unchanged when website_url is present (hostname only) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthrough新增供应商“recluster”功能：包含多语言文案、后端重新聚类操作、供应商键计算工具、前端对话框组件以及对应的单元测试，支持预览与应用两种模式并在应用时进行事务性数据库更新与回填/清理操作。 Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ding113, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines how providers are grouped into vendors, especially for those lacking a direct website URL. By implementing a new clustering logic that leverages the host and port of a provider's URL, the system can now more accurately differentiate and group IP-based services. This enhancement is exposed through a new administrative action and a dedicated user interface, enabling administrators to preview and apply these re-clustering operations for improved organization and management of provider vendors. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
github-actions
bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for reclustering provider vendors using host:port when a website_url is absent. The implementation is robust, featuring a new server action, a user-friendly dialog for previewing and applying changes, and thorough unit tests. The core logic in computeVendorKey is well-designed and extensively tested. I've identified a couple of opportunities to enhance performance in the reclusterProviderVendors action by optimizing database queries and data lookups within loops. Overall, this is a well-executed and significant improvement.
| for (const provider of allProviders) { | ||
| const newVendorKey = computeVendorKey({ | ||
| providerUrl: provider.url, | ||
| websiteUrl: provider.websiteUrl, | ||
| }); | ||
|
|
||
| if (!newVendorKey) { | ||
| skippedInvalidUrl++; | ||
| continue; | ||
| } | ||
|
|
||
| // Get current vendor domain | ||
| const currentVendor = provider.providerVendorId | ||
| ? await findProviderVendorById(provider.providerVendorId) | ||
| : null; | ||
| const currentDomain = currentVendor?.websiteDomain ?? ""; | ||
|
|
||
| // If key changed, record the change | ||
| if (currentDomain !== newVendorKey) { | ||
| newVendorKeys.add(newVendorKey); | ||
| if (provider.providerVendorId) { | ||
| oldVendorIds.add(provider.providerVendorId); | ||
| } | ||
| changes.push({ | ||
| providerId: provider.id, | ||
| providerName: provider.name, | ||
| oldVendorId: provider.providerVendorId ?? 0, | ||
| oldVendorDomain: currentDomain, | ||
| newVendorDomain: newVendorKey, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation uses await inside a for loop to fetch vendor information, which leads to sequential database calls. This can be inefficient if there are many providers. To improve performance, you can parallelize these database lookups using Promise.all before iterating through the providers.
const vendorLookups = await Promise.all(
allProviders.map((p) =>
p.providerVendorId ? findProviderVendorById(p.providerVendorId) : null,
),
);
const providerToVendorMap = new Map(
allProviders.map((p, i) => [p.id, vendorLookups[i]]),
);
// Calculate new vendor key for each provider
for (const provider of allProviders) {
const newVendorKey = computeVendorKey({
providerUrl: provider.url,
websiteUrl: provider.websiteUrl,
});
if (!newVendorKey) {
skippedInvalidUrl++;
continue;
}
// Get current vendor domain
const currentVendor = providerToVendorMap.get(provider.id);
const currentDomain = currentVendor?.websiteDomain ?? "";
// If key changed, record the change
if (currentDomain !== newVendorKey) {
newVendorKeys.add(newVendorKey);
if (provider.providerVendorId) {
oldVendorIds.add(provider.providerVendorId);
}
changes.push({
providerId: provider.id,
providerName: provider.name,
oldVendorId: provider.providerVendorId ?? 0,
oldVendorDomain: currentDomain,
newVendorDomain: newVendorKey,
});
}
}| if (changes.length > 0) { | ||
| await db.transaction(async (tx) => { | ||
| for (const change of changes) { | ||
| // Get or create new vendor | ||
| const newVendorId = await getOrCreateProviderVendorIdFromUrls({ | ||
| providerUrl: allProviders.find((p) => p.id === change.providerId)?.url ?? "", | ||
| websiteUrl: allProviders.find((p) => p.id === change.providerId)?.websiteUrl ?? null, | ||
| }); | ||
|
|
||
| // Update provider's vendorId | ||
| await tx | ||
| .update(providersTable) | ||
| .set({ providerVendorId: newVendorId, updatedAt: new Date() }) | ||
| .where(eq(providersTable.id, change.providerId)); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Inside the transaction, allProviders.find() is called within a loop, which has a time complexity of O(N^2) in the worst case. This can be optimized to O(N) by creating a Map of providers for quick lookups before entering the transaction block.
| if (changes.length > 0) { | |
| await db.transaction(async (tx) => { | |
| for (const change of changes) { | |
| // Get or create new vendor | |
| const newVendorId = await getOrCreateProviderVendorIdFromUrls({ | |
| providerUrl: allProviders.find((p) => p.id === change.providerId)?.url ?? "", | |
| websiteUrl: allProviders.find((p) => p.id === change.providerId)?.websiteUrl ?? null, | |
| }); | |
| // Update provider's vendorId | |
| await tx | |
| .update(providersTable) | |
| .set({ providerVendorId: newVendorId, updatedAt: new Date() }) | |
| .where(eq(providersTable.id, change.providerId)); | |
| } | |
| }); | |
| if (changes.length > 0) { | |
| const providerMap = new Map(allProviders.map((p) => [p.id, p])); | |
| await db.transaction(async (tx) => { | |
| for (const change of changes) { | |
| const provider = providerMap.get(change.providerId); | |
| // Get or create new vendor | |
| const newVendorId = await getOrCreateProviderVendorIdFromUrls({ | |
| providerUrl: provider?.url ?? "", | |
| websiteUrl: provider?.websiteUrl ?? null, | |
| }); | |
| // Update provider's vendorId | |
| await tx | |
| .update(providersTable) | |
| .set({ providerVendorId: newVendorId, updatedAt: new Date() }) | |
| .where(eq(providersTable.id, change.providerId)); | |
| } | |
| }); |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| */ | ||
| export async function reclusterProviderVendors(args: { | ||
| confirm: boolean; | ||
| }): Promise<ActionResult<ReclusterResult>> { | ||
| try { | ||
| const session = await getSession(); | ||
| if (!session || session.user.role !== "admin") { | ||
| return { ok: false, error: "NO_PERMISSION" }; |
There was a problem hiding this comment.
potential performance issue: this loops through all providers and makes a DB call (findProviderVendorById) for each one
consider batch-loading all vendor data upfront:
| */ | |
| export async function reclusterProviderVendors(args: { | |
| confirm: boolean; | |
| }): Promise<ActionResult<ReclusterResult>> { | |
| try { | |
| const session = await getSession(); | |
| if (!session || session.user.role !== "admin") { | |
| return { ok: false, error: "NO_PERMISSION" }; | |
| // Get current vendor domain | |
| const allVendorIds = allProviders | |
| .map(p => p.providerVendorId) | |
| .filter((id): id is number => id !== null && id !== undefined); | |
| const vendors = await Promise.all( | |
| [...new Set(allVendorIds)].map(id => findProviderVendorById(id)) | |
| ); | |
| const vendorMap = new Map(vendors.filter((v): v is NonNullable<typeof v> => v !== null).map(v => [v.id, v])); |
then in the loop:
const currentVendor = provider.providerVendorId ? vendorMap.get(provider.providerVendorId) : null;
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/providers.ts
Line: 3589:3596
Comment:
potential performance issue: this loops through all providers and makes a DB call (`findProviderVendorById`) for each one
consider batch-loading all vendor data upfront:
```suggestion
// Get current vendor domain
const allVendorIds = allProviders
.map(p => p.providerVendorId)
.filter((id): id is number => id !== null && id !== undefined);
const vendors = await Promise.all(
[...new Set(allVendorIds)].map(id => findProviderVendorById(id))
);
const vendorMap = new Map(vendors.filter((v): v is NonNullable<typeof v> => v !== null).map(v => [v.id, v]));
```
then in the loop:
```
const currentVendor = provider.providerVendorId ? vendorMap.get(provider.providerVendorId) : null;
```
How can I resolve this? If you propose a fix, please make it concise.| // Get or create new vendor | ||
| const newVendorId = await getOrCreateProviderVendorIdFromUrls({ | ||
| providerUrl: allProviders.find((p) => p.id === change.providerId)?.url ?? "", | ||
| websiteUrl: allProviders.find((p) => p.id === change.providerId)?.websiteUrl ?? null, | ||
| }); | ||
|
|
||
| // Update provider's vendorId | ||
| await tx | ||
| .update(providersTable) | ||
| .set({ providerVendorId: newVendorId, updatedAt: new Date() }) |
There was a problem hiding this comment.
potential N+1 query issue: loops through changes and makes DB calls for each provider's URL lookup
the allProviders.find() calls in the loop are unnecessary since you already have the provider data from earlier
| // Get or create new vendor | |
| const newVendorId = await getOrCreateProviderVendorIdFromUrls({ | |
| providerUrl: allProviders.find((p) => p.id === change.providerId)?.url ?? "", | |
| websiteUrl: allProviders.find((p) => p.id === change.providerId)?.websiteUrl ?? null, | |
| }); | |
| // Update provider's vendorId | |
| await tx | |
| .update(providersTable) | |
| .set({ providerVendorId: newVendorId, updatedAt: new Date() }) | |
| // Build a map for quick provider lookup | |
| const providerMap = new Map(allProviders.map(p => [p.id, p])); | |
| for (const change of changes) { | |
| const provider = providerMap.get(change.providerId); | |
| if (!provider) continue; | |
| // Get or create new vendor | |
| const newVendorId = await getOrCreateProviderVendorIdFromUrls({ | |
| providerUrl: provider.url, | |
| websiteUrl: provider.websiteUrl ?? null, | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/actions/providers.ts
Line: 3679:3688
Comment:
potential N+1 query issue: loops through changes and makes DB calls for each provider's URL lookup
the `allProviders.find()` calls in the loop are unnecessary since you already have the provider data from earlier
```suggestion
// Build a map for quick provider lookup
const providerMap = new Map(allProviders.map(p => [p.id, p]));
for (const change of changes) {
const provider = providerMap.get(change.providerId);
if (!provider) continue;
// Get or create new vendor
const newVendorId = await getOrCreateProviderVendorIdFromUrls({
providerUrl: provider.url,
websiteUrl: provider.websiteUrl ?? null,
});
```
How can I resolve this? If you propose a fix, please make it concise.![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR adds a new feature to recluster provider vendors by host:port when website_url is empty. The implementation is generally well-structured with good test coverage.
PR Size: L
- Lines changed: 1124 (1117 additions, 7 deletions)
- Files changed: 17
Note: This is a large PR. Consider splitting future similar changes into smaller PRs for easier review.
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 |
| Performance | 0 | 1 | 0 | 0 |
High Priority Issues (Should Fix)
1. [PERFORMANCE-ISSUE] Inefficient O(n*m) lookup inside transaction loop (src/actions/providers.ts:3681-3682)
The allProviders.find() is called twice per change inside the loop. For a large number of providers and changes, this results in O(n*m) complexity.
Suggested fix: Build a Map for O(1) lookups before the loop:
const providerMap = new Map(allProviders.map((p) => [p.id, p]));
await db.transaction(async (tx) => {
for (const change of changes) {
const provider = providerMap.get(change.providerId);
const newVendorId = await getOrCreateProviderVendorIdFromUrls({
providerUrl: provider?.url ?? "",
websiteUrl: provider?.websiteUrl ?? null,
});
// ...
}
});Review Coverage
- Logic and correctness - Clean
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - Adequate (31 test cases)
- Code clarity - Good
Automated review by Claude AI
… lookup - Batch load all vendor data upfront with Promise.all to avoid N+1 queries - Use Map for O(1) provider lookup instead of O(N) find() in transaction loop - Addresses bugbot review comments from gemini-code-assist and greptile-apps Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
github-project-automation
bot
moved this from Backlog
to Done
in Claude Code Hub Roadmap
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/repository/provider-endpoints.ts`:
- Around line 63-92: normalizeHostWithPort currently returns host:port and for
raw IPv6 hostnames like "2001:db8::1" produces "2001:db8::1:443", which is
invalid; update normalizeHostWithPort to detect IPv6 hostnames (hostname
includes ':' and does not already start with '[') and wrap the hostname in
brackets before appending the port so the returned value follows the
"[ipv6]:port" form; use parsed.hostname/parsed.port/parsed.protocol and preserve
existing bracketed addresses (i.e., only add brackets when needed) so all return
values are normalizedHostnameWithPort in the expected format.
🧹 Nitpick comments (1)
tests/unit/repository/provider-endpoints-vendor-key.test.ts (1)
160-205: 建议：考虑增加 websiteUrl 无效格式的测试用例当前边缘用例测试了
providerUrl无效的情况，但未测试websiteUrl格式无效时的行为。建议补充此场景以确保完整覆盖。💡 建议添加的测试用例
test("returns null for truly invalid URL", () => { expect( computeVendorKey({ providerUrl: "://invalid", websiteUrl: null, }) ).toBeNull(); }); + + test("falls back to providerUrl when websiteUrl is invalid", () => { + expect( + computeVendorKey({ + providerUrl: "http://192.168.1.1:8080/v1/messages", + websiteUrl: "://invalid", + }) + ).toBe("192.168.1.1:8080"); + }); }); });
| function normalizeHostWithPort(rawUrl: string): string | null { | ||
| const trimmed = rawUrl.trim(); | ||
| if (!trimmed) return null; | ||
|
|
||
| // Add https:// if no scheme present | ||
| let urlString = trimmed; | ||
| if (!/^[a-zA-Z][a-zA-Z\d+.-]*:\/\//.test(trimmed)) { | ||
| urlString = `https://${trimmed}`; | ||
| } | ||
|
|
||
| try { | ||
| const parsed = new URL(urlString); | ||
| const hostname = parsed.hostname?.toLowerCase(); | ||
| if (!hostname) return null; | ||
|
|
||
| // Strip www. prefix | ||
| const normalizedHostname = hostname.startsWith("www.") ? hostname.slice(4) : hostname; | ||
|
|
||
| // Determine port | ||
| let port: string; | ||
| if (parsed.port) { | ||
| port = parsed.port; | ||
| } else { | ||
| // Use protocol default port | ||
| port = parsed.protocol === "http:" ? "80" : "443"; | ||
| } | ||
|
|
||
| // IPv6 addresses already have brackets from URL parser (e.g., "[::1]") | ||
| // Just append the port directly | ||
| return `${normalizedHostname}:${port}`; |
There was a problem hiding this comment.
IPv6 主机未加方括号会生成错误的 vendor key
当前拼接会在 IPv6 情况下生成 2001:db8::1:443 这类非标准格式，与注释的 [ipv6]:port 规则不一致，可能导致聚类键冲突或显示异常。建议在包含冒号的主机名上补 []。
建议修复
- // IPv6 addresses already have brackets from URL parser (e.g., "[::1]")
- // Just append the port directly
- return `${normalizedHostname}:${port}`;
+ // Wrap IPv6 hosts in brackets to keep host:port unambiguous
+ const hostForKey = normalizedHostname.includes(":")
+ ? `[${normalizedHostname}]`
+ : normalizedHostname;
+ return `${hostForKey}:${port}`;🤖 Prompt for AI Agents
In `@src/repository/provider-endpoints.ts` around lines 63 - 92,
normalizeHostWithPort currently returns host:port and for raw IPv6 hostnames
like "2001:db8::1" produces "2001:db8::1:443", which is invalid; update
normalizeHostWithPort to detect IPv6 hostnames (hostname includes ':' and does
not already start with '[') and wrap the hostname in brackets before appending
the port so the returned value follows the "[ipv6]:port" form; use
parsed.hostname/parsed.port/parsed.protocol and preserve existing bracketed
addresses (i.e., only add brackets when needed) so all return values are
normalizedHostnameWithPort in the expected format.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/actions/providers.ts`:
- Around line 3649-3654: The line declaring currentDomain violates Biome
line-wrapping rules; modify the const declaration for currentDomain (currently
using currentVendor?.websiteDomain ?? "") to span multiple lines so it wraps
properly — e.g., break after the optional chaining or the nullish coalescing
operator and indent the fallback string on the next line; adjust the const in
the same block where provider, vendorMap and currentVendor are used to preserve
semantics.
- Around line 3631-3675: The preview's vendorsCreated can be inflated because
newVendorKeys currently includes domains that already exist as
vendor.websiteDomain even if they have no providers; to fix, build a set of
existing vendor domains from vendors (e.g., existingVendorDomains) and when
iterating allProviders and computing newVendorKey (via computeVendorKey), only
add newVendorKey to newVendorKeys if newVendorKey is truthy and not present in
existingVendorDomains (and of course not already in newVendorKeys); reference
vendorMap, newVendorKeys, computeVendorKey, allProviders and ensure
preview.vendorsCreated uses the filtered newVendorKeys.size.
- Around line 3594-3597: Replace the hard-coded "NO_PERMISSION" error string
with the project-standard error code AUTH_ERRORS.PERMISSION_DENIED (imported
from src/lib/utils/error-messages.ts) so the returned ActionResult uses the
correct errorCode and supports translations; update the return in the
getSession() check around the session.user.role !== "admin" guard to return {
ok: false, error: AUTH_ERRORS.PERMISSION_DENIED } and add the necessary import
for AUTH_ERRORS at the top of the file, and then propagate the same replacement
for other permission checks in this file that currently return "NO_PERMISSION"
(the other similar checks noted in the review).
🧹 Nitpick comments (1)
src/actions/providers.ts (1)
3691-3707: 建议让 vendor 创建与 provider 更新同一事务
getOrCreateProviderVendorIdFromUrls目前使用全局db实例创建 vendor，而 provider 更新在事务内执行。若事务回滚，vendor 会被遗留在数据库中（孤立 vendor）。建议将tx参数传入该函数，并在创建 vendor 时使用tx而非全局db。此修改需要更新函数签名及其 5 个调用点（provider.ts 2 处、provider-endpoints.ts 1 处、actions/providers.ts 1 处及本处）。由于部分调用点不在事务context中，建议将 tx 参数设为可选，或采用函数重载处理。
| const session = await getSession(); | ||
| if (!session || session.user.role !== "admin") { | ||
| return { ok: false, error: "NO_PERMISSION" }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 查找 NO_PERMISSION 的前端映射或显示位置
rg -n -C3 "NO_PERMISSION" -g'*.{ts,tsx,js,jsx}'Repository: ding113/claude-code-hub
Length of output: 492
🏁 Script executed:
#!/bin/bash
# 查找 i18n/翻译相关的文件
fd -e json -e ts -e tsx | grep -i -E '(i18n|messages|locale|translation|lang)' | head -20Repository: ding113/claude-code-hub
Length of output: 1134
🏁 Script executed:
#!/bin/bash
# 查看 src/actions/providers.ts 中其他的 error 返回方式
rg -n 'return.*error' src/actions/providers.ts | head -20Repository: ding113/claude-code-hub
Length of output: 1092
🏁 Script executed:
#!/bin/bash
# 查找i18n翻译文件的位置
fd -type f -E 'node_modules' -E 'dist' -E 'build' | grep -E '(messages|locale|lang)' | grep -v test | head -20Repository: ding113/claude-code-hub
Length of output: 237
🏁 Script executed:
#!/bin/bash
# 查看 src/actions/providers.ts 中 NO_PERMISSION 行的完整上下文
sed -n '3590,3610p' src/actions/providers.tsRepository: ding113/claude-code-hub
Length of output: 651
🏁 Script executed:
#!/bin/bash
# 查找翻译文件中是否有 NO_PERMISSION
rg "NO_PERMISSION" -E '.json$|.ts$' --type-list | grep -v node_modules | grep -v testRepository: ding113/claude-code-hub
Length of output: 456
🏁 Script executed:
#!/bin/bash
# 查找i18n翻译文件的位置
find src -type f \( -name "*messages*" -o -name "*locale*" -o -name "*i18n*" \) 2>/dev/null | head -20Repository: ding113/claude-code-hub
Length of output: 749
🏁 Script executed:
#!/bin/bash
# 搜索NO_PERMISSION在整个项目中的使用
rg "NO_PERMISSION" -nRepository: ding113/claude-code-hub
Length of output: 148
🏁 Script executed:
#!/bin/bash
# 查看src目录的结构
ls -la src/ | head -30Repository: ding113/claude-code-hub
Length of output: 834
🏁 Script executed:
#!/bin/bash
# 查看 src/i18n 目录的内容
ls -la src/i18n/Repository: ding113/claude-code-hub
Length of output: 481
🏁 Script executed:
#!/bin/bash
# 查看 error-messages.ts 文件内容
cat src/lib/utils/error-messages.tsRepository: ding113/claude-code-hub
Length of output: 8132
🏁 Script executed:
#!/bin/bash
# 查找翻译文件中是否有 permission 相关的内容
rg -i "permission" src/i18n/ -lRepository: ding113/claude-code-hub
Length of output: 49
🏁 Script executed:
#!/bin/bash
# 查看 src/actions/providers.ts 中其他的错误返回是否都使用硬编码中文
grep -n 'error:' src/actions/providers.ts | grep -v ERROR_CODES | grep -v PERMISSION_DENIED | head -10Repository: ding113/claude-code-hub
Length of output: 722
🏁 Script executed:
#!/bin/bash
# 检查项目中是否有其他地方使用了 NO_PERMISSION 这样的非标准错误代码
rg '"[A-Z_]+"\s*}' src/actions/ -A1 -B1 | grep error | head -20Repository: ding113/claude-code-hub
Length of output: 222
改用项目标准错误代码 PERMISSION_DENIED
"NO_PERMISSION" 不符合项目的错误代码规范。应根据 src/lib/utils/error-messages.ts 的定义，改用 AUTH_ERRORS.PERMISSION_DENIED，以便：
- 支持多语言翻译映射
- 符合
ActionResult的errorCode规范
其他权限检查（如 490、658 行）也存在同样的硬编码中文问题，建议同步修正。
🤖 Prompt for AI Agents
In `@src/actions/providers.ts` around lines 3594 - 3597, Replace the hard-coded
"NO_PERMISSION" error string with the project-standard error code
AUTH_ERRORS.PERMISSION_DENIED (imported from src/lib/utils/error-messages.ts) so
the returned ActionResult uses the correct errorCode and supports translations;
update the return in the getSession() check around the session.user.role !==
"admin" guard to return { ok: false, error: AUTH_ERRORS.PERMISSION_DENIED } and
add the necessary import for AUTH_ERRORS at the top of the file, and then
propagate the same replacement for other permission checks in this file that
currently return "NO_PERMISSION" (the other similar checks noted in the review).
| const vendorMap = new Map( | ||
| vendors.filter((v): v is NonNullable<typeof v> => v !== null).map((v) => [v.id, v]) | ||
| ); | ||
|
|
||
| // Build provider map for quick lookup in transaction | ||
| const providerMap = new Map(allProviders.map((p) => [p.id, p])); | ||
|
|
||
| // Calculate new vendor key for each provider | ||
| for (const provider of allProviders) { | ||
| const newVendorKey = computeVendorKey({ | ||
| providerUrl: provider.url, | ||
| websiteUrl: provider.websiteUrl, | ||
| }); | ||
|
|
||
| if (!newVendorKey) { | ||
| skippedInvalidUrl++; | ||
| continue; | ||
| } | ||
|
|
||
| // Get current vendor domain from pre-loaded map | ||
| const currentVendor = provider.providerVendorId ? vendorMap.get(provider.providerVendorId) : null; | ||
| const currentDomain = currentVendor?.websiteDomain ?? ""; | ||
|
|
||
| // If key changed, record the change | ||
| if (currentDomain !== newVendorKey) { | ||
| newVendorKeys.add(newVendorKey); | ||
| if (provider.providerVendorId) { | ||
| oldVendorIds.add(provider.providerVendorId); | ||
| } | ||
| changes.push({ | ||
| providerId: provider.id, | ||
| providerName: provider.name, | ||
| oldVendorId: provider.providerVendorId ?? 0, | ||
| oldVendorDomain: currentDomain, | ||
| newVendorDomain: newVendorKey, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| const preview = { | ||
| providersMoved: changes.length, | ||
| vendorsCreated: newVendorKeys.size, | ||
| vendorsToDelete: oldVendorIds.size, | ||
| skippedInvalidUrl, | ||
| }; |
There was a problem hiding this comment.
vendorsCreated 预览可能被高估
新 key 可能已存在 vendor（当前无 provider 绑定），统计会偏大；建议扣除已存在域名。
修复示例
const vendorMap = new Map(
vendors.filter((v): v is NonNullable<typeof v> => v !== null).map((v) => [v.id, v])
);
+ const existingVendorDomains = new Set(
+ Array.from(vendorMap.values()).map((vendor) => vendor.websiteDomain)
+ );
@@
const preview = {
providersMoved: changes.length,
- vendorsCreated: newVendorKeys.size,
+ vendorsCreated: Array.from(newVendorKeys).filter(
+ (key) => !existingVendorDomains.has(key)
+ ).length,
vendorsToDelete: oldVendorIds.size,
skippedInvalidUrl,
};📝 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.
| const vendorMap = new Map( | |
| vendors.filter((v): v is NonNullable<typeof v> => v !== null).map((v) => [v.id, v]) | |
| ); | |
| // Build provider map for quick lookup in transaction | |
| const providerMap = new Map(allProviders.map((p) => [p.id, p])); | |
| // Calculate new vendor key for each provider | |
| for (const provider of allProviders) { | |
| const newVendorKey = computeVendorKey({ | |
| providerUrl: provider.url, | |
| websiteUrl: provider.websiteUrl, | |
| }); | |
| if (!newVendorKey) { | |
| skippedInvalidUrl++; | |
| continue; | |
| } | |
| // Get current vendor domain from pre-loaded map | |
| const currentVendor = provider.providerVendorId ? vendorMap.get(provider.providerVendorId) : null; | |
| const currentDomain = currentVendor?.websiteDomain ?? ""; | |
| // If key changed, record the change | |
| if (currentDomain !== newVendorKey) { | |
| newVendorKeys.add(newVendorKey); | |
| if (provider.providerVendorId) { | |
| oldVendorIds.add(provider.providerVendorId); | |
| } | |
| changes.push({ | |
| providerId: provider.id, | |
| providerName: provider.name, | |
| oldVendorId: provider.providerVendorId ?? 0, | |
| oldVendorDomain: currentDomain, | |
| newVendorDomain: newVendorKey, | |
| }); | |
| } | |
| } | |
| const preview = { | |
| providersMoved: changes.length, | |
| vendorsCreated: newVendorKeys.size, | |
| vendorsToDelete: oldVendorIds.size, | |
| skippedInvalidUrl, | |
| }; | |
| const vendorMap = new Map( | |
| vendors.filter((v): v is NonNullable<typeof v> => v !== null).map((v) => [v.id, v]) | |
| ); | |
| const existingVendorDomains = new Set( | |
| Array.from(vendorMap.values()).map((vendor) => vendor.websiteDomain) | |
| ); | |
| // Build provider map for quick lookup in transaction | |
| const providerMap = new Map(allProviders.map((p) => [p.id, p])); | |
| // Calculate new vendor key for each provider | |
| for (const provider of allProviders) { | |
| const newVendorKey = computeVendorKey({ | |
| providerUrl: provider.url, | |
| websiteUrl: provider.websiteUrl, | |
| }); | |
| if (!newVendorKey) { | |
| skippedInvalidUrl++; | |
| continue; | |
| } | |
| // Get current vendor domain from pre-loaded map | |
| const currentVendor = provider.providerVendorId ? vendorMap.get(provider.providerVendorId) : null; | |
| const currentDomain = currentVendor?.websiteDomain ?? ""; | |
| // If key changed, record the change | |
| if (currentDomain !== newVendorKey) { | |
| newVendorKeys.add(newVendorKey); | |
| if (provider.providerVendorId) { | |
| oldVendorIds.add(provider.providerVendorId); | |
| } | |
| changes.push({ | |
| providerId: provider.id, | |
| providerName: provider.name, | |
| oldVendorId: provider.providerVendorId ?? 0, | |
| oldVendorDomain: currentDomain, | |
| newVendorDomain: newVendorKey, | |
| }); | |
| } | |
| } | |
| const preview = { | |
| providersMoved: changes.length, | |
| vendorsCreated: Array.from(newVendorKeys).filter( | |
| (key) => !existingVendorDomains.has(key) | |
| ).length, | |
| vendorsToDelete: oldVendorIds.size, | |
| skippedInvalidUrl, | |
| }; |
🧰 Tools
🪛 GitHub Actions: 🧪 Test Suite
[error] 3649-3653: Formatter would have printed different content. Run the formatter to fix code style issues.
🪛 GitHub Actions: PR Build Check
[error] 3649-3654: Code formatter would reformat the following content. The format:check failed (exit code 1). Run 'bun run format' or 'bun run format:write' to fix formatting in this file.
🤖 Prompt for AI Agents
In `@src/actions/providers.ts` around lines 3631 - 3675, The preview's
vendorsCreated can be inflated because newVendorKeys currently includes domains
that already exist as vendor.websiteDomain even if they have no providers; to
fix, build a set of existing vendor domains from vendors (e.g.,
existingVendorDomains) and when iterating allProviders and computing
newVendorKey (via computeVendorKey), only add newVendorKey to newVendorKeys if
newVendorKey is truthy and not present in existingVendorDomains (and of course
not already in newVendorKeys); reference vendorMap, newVendorKeys,
computeVendorKey, allProviders and ensure preview.vendorsCreated uses the
filtered newVendorKeys.size.
|
|
||
| // Get current vendor domain from pre-loaded map | ||
| const currentVendor = provider.providerVendorId ? vendorMap.get(provider.providerVendorId) : null; | ||
| const currentDomain = currentVendor?.websiteDomain ?? ""; | ||
|
|
||
| // If key changed, record the change |
There was a problem hiding this comment.
该行触发格式化失败
CI 已提示此处需要按 Biome 规则换行。
修复示例
- const currentVendor = provider.providerVendorId ? vendorMap.get(provider.providerVendorId) : null;
+ const currentVendor = provider.providerVendorId
+ ? vendorMap.get(provider.providerVendorId)
+ : 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.
| // Get current vendor domain from pre-loaded map | |
| const currentVendor = provider.providerVendorId ? vendorMap.get(provider.providerVendorId) : null; | |
| const currentDomain = currentVendor?.websiteDomain ?? ""; | |
| // If key changed, record the change | |
| // Get current vendor domain from pre-loaded map | |
| const currentVendor = provider.providerVendorId | |
| ? vendorMap.get(provider.providerVendorId) | |
| : null; | |
| const currentDomain = currentVendor?.websiteDomain ?? ""; | |
| // If key changed, record the change |
🧰 Tools
🪛 GitHub Actions: 🧪 Test Suite
[error] 3649-3653: Formatter would have printed different content. Run the formatter to fix code style issues.
🪛 GitHub Actions: PR Build Check
[error] 3649-3654: Code formatter would reformat the following content. The format:check failed (exit code 1). Run 'bun run format' or 'bun run format:write' to fix formatting in this file.
🤖 Prompt for AI Agents
In `@src/actions/providers.ts` around lines 3649 - 3654, The line declaring
currentDomain violates Biome line-wrapping rules; modify the const declaration
for currentDomain (currently using currentVendor?.websiteDomain ?? "") to span
multiple lines so it wraps properly — e.g., break after the optional chaining or
the nullish coalescing operator and indent the fallback string on the next line;
adjust the const in the same block where provider, vendorMap and currentVendor
are used to preserve semantics.
1 participant
Summary
Add
computeVendorKeyhelper with host:port support for IP-based providers. Whenwebsite_urlis empty, use host:port as vendor key (different ports = different vendors).Related:
Problem
When providers use IP addresses without a
website_url, they were all grouped under the same vendor based on hostname alone. This caused issues when running multiple services on the same IP with different ports (e.g.,192.168.1.1:8080and192.168.1.1:9090should be separate vendors).Solution
computeVendorKeyhelper that uses host:port as vendor key whenwebsite_urlis empty[ipv6]:portformatreclusterProviderVendorsaction with preview/apply mode for migrating existing providersReclusterVendorsDialogUI component to settings and dashboard providers pagesChanges
Core Changes
src/repository/provider-endpoints.ts- AddcomputeVendorKeyhelper andnormalizeHostWithPortfunctionsrc/actions/providers.ts- AddreclusterProviderVendorsaction with preview/apply modesUI Changes
src/app/[locale]/settings/providers/_components/recluster-vendors-dialog.tsx- New dialog componentsrc/app/[locale]/settings/providers/page.tsx- Add recluster buttonsrc/app/[locale]/dashboard/providers/page.tsx- Add recluster buttoni18n (5 languages)
messages/*/settings/providers/recluster.json- New translation filesmessages/*/settings/index.ts- Export new translationsTests
tests/unit/repository/provider-endpoints-vendor-key.test.ts- 21 test cases forcomputeVendorKeytests/unit/actions/providers-recluster.test.ts- 10 test cases forreclusterProviderVendorsTesting
Automated Tests
computeVendorKey(21 test cases)reclusterProviderVendorsaction (10 test cases)Manual Testing
Checklist
Description enhanced by Claude AI
Greptile Overview
Greptile Summary
This PR enhances vendor clustering by using
host:portas the vendor key whenwebsite_urlis empty, solving the problem where multiple services on the same IP with different ports were incorrectly grouped together.Key Changes:
computeVendorKey()helper function that useshost:portformat whenwebsite_urlis empty (supports IPv6 with bracket notation)reclusterProviderVendors()server action with preview/apply modes for migrating existing providersReclusterVendorsDialogUI component with statistics and change preview tablePerformance Improvements:
The implementation addresses previous review feedback by batch-loading all vendor data upfront using
Promise.all()and aMapfor O(1) lookups (lines 3623-3633 inproviders.ts), avoiding N+1 query issues.Code Quality:
Confidence Score: 5/5
Important Files Changed
computeVendorKeyandnormalizeHostWithPorthelpers for host:port-based vendor clustering when websiteUrl is empty. Well-tested with comprehensive unit tests.reclusterProviderVendorsaction with preview/apply modes. Implements batch vendor loading optimization (addresses previous review comment).computeVendorKeywith 21 test cases covering websiteUrl priority, host:port format, IPv6, and edge cases.reclusterProviderVendorsaction with 10 test cases covering permissions, preview mode, apply mode, and error handling.Sequence Diagram
sequenceDiagram participant User participant UI as ReclusterVendorsDialog participant Action as reclusterProviderVendors participant Repo as provider-endpoints participant DB as Database User->>UI: Click "Recluster" button UI->>Action: reclusterProviderVendors({confirm: false}) Action->>Repo: findAllProvidersFresh() Repo->>DB: SELECT * FROM providers DB-->>Repo: All providers Repo-->>Action: providers[] Action->>Repo: findProviderVendorById(id) [batch] Repo->>DB: SELECT * FROM provider_vendors WHERE id IN (...) DB-->>Repo: Vendor records Repo-->>Action: vendorMap loop For each provider Action->>Repo: computeVendorKey({providerUrl, websiteUrl}) Repo-->>Action: newVendorKey (host:port if websiteUrl empty) Action->>Action: Compare with current vendor domain alt Vendor key changed Action->>Action: Record change end end Action-->>UI: {preview: stats, changes: [], applied: false} UI->>User: Display preview table with changes User->>UI: Click "Apply Changes" UI->>Action: reclusterProviderVendors({confirm: true}) Action->>Action: Recalculate changes (same as preview) alt Changes exist Action->>DB: BEGIN TRANSACTION loop For each change Action->>Repo: getOrCreateProviderVendorIdFromUrls() Repo->>DB: INSERT INTO provider_vendors (upsert) DB-->>Repo: newVendorId Action->>DB: UPDATE providers SET provider_vendor_id = newVendorId end Action->>DB: COMMIT TRANSACTION Action->>Repo: backfillProviderEndpointsFromProviders() Repo->>DB: INSERT INTO provider_endpoints (batch) loop For each old vendor Action->>Repo: tryDeleteProviderVendorIfEmpty(oldVendorId) Repo->>DB: DELETE FROM provider_vendors (if no active providers) end Action->>Action: publishProviderCacheInvalidation() end Action-->>UI: {applied: true, preview: stats} UI->>UI: Invalidate React Query cache UI->>User: Show success toast