Conversation
📝 WalkthroughWalkthrough该 PR 在多语言文案中新增精确匹配模型选择相关键，新增并集成了 ModelMultiSelect 与 RuleTesterDialogTrigger 组件，扩展并重构了模型价格/目录接口，更新了允许模型规则编辑器与路由表单，并为这些改动新增/调整单元测试。 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
enhancement
![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 ModelMultiSelect component and a RuleTesterDialogTrigger to streamline provider model allowlist management, allowing users to select exact models from upstream or local catalogs. The changes include multi-language support, a new backend action for model retrieval, and expanded unit tests. Feedback identifies a bug where masked API keys hinder model fetching and suggests improvements for the loadModels effect, specifically using the useEffectEvent pattern and implementing race condition handling.
|
|
||
| try { | ||
| let resolvedKey = apiKey?.trim() || ""; | ||
| if (!resolvedKey && providerId) { |
There was a problem hiding this comment.
The current logic only attempts to unmask the API key if apiKey is empty. However, when editing an existing provider, the apiKey prop (passed from state.basic.key) typically contains the masked version (e.g., sk-****). This causes fetchUpstreamModels to be called with an invalid key, making the upstream model list unavailable.
You should check if the key is masked (e.g., contains ***) and trigger the unmasking action in that case as well.
| if (!resolvedKey && providerId) { | |
| const isMasked = resolvedKey.includes("***"); | |
| if ((!resolvedKey || isMasked) && providerId) { |
| useEffect(() => { | ||
| void loadModels(); | ||
| }, [loadModels]); |
There was a problem hiding this comment.
The useEffect hook triggers loadModels whenever its dependencies change, regardless of whether the model picker popover is actually open. To optimize performance and follow repository patterns, use the useEffectEvent pattern. This separates the trigger logic (the open state) from the action (loadModels), ensuring the action only runs when intended and avoiding unnecessary re-runs if loadModels is unstable.
| useEffect(() => { | |
| void loadModels(); | |
| }, [loadModels]); | |
| const onLoadModels = useEffectEvent(() => { | |
| void loadModels(); | |
| }); | |
| useEffect(() => { | |
| if (open) { | |
| onLoadModels(); | |
| } | |
| }, [open]); |
References
- Use the useEffectEvent pattern to separate the effect's trigger logic from the action, ensuring the action only runs when its specific dependencies change and preventing unnecessary re-runs from unstable function dependencies.
| const loadModels = useCallback(async () => { | ||
| setLoading(true); | ||
| setModelSource("loading"); | ||
|
|
||
| try { | ||
| let resolvedKey = apiKey?.trim() || ""; | ||
| if (!resolvedKey && providerId) { | ||
| const keyResult = await getUnmaskedProviderKey(providerId); | ||
| if (keyResult.ok && keyResult.data?.key) { | ||
| resolvedKey = keyResult.data.key; | ||
| } | ||
| } | ||
|
|
||
| if (providerUrl && resolvedKey) { | ||
| const upstreamResult = await fetchUpstreamModels({ | ||
| providerUrl, | ||
| apiKey: resolvedKey, | ||
| providerType, | ||
| proxyUrl, | ||
| proxyFallbackToDirect, | ||
| }); | ||
|
|
||
| if (upstreamResult.ok && upstreamResult.data?.models?.length) { | ||
| const upstreamModels = upstreamResult.data.models.map((modelName) => | ||
| buildLocalOption({ | ||
| modelName, | ||
| litellmProvider: null, | ||
| updatedAt: "", | ||
| }) | ||
| ); | ||
| setAvailableModels(upstreamModels); | ||
| setModelSource("upstream"); | ||
| setProviderFilter("__all__"); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| const localCatalog = await getAvailableModelCatalog(); | ||
| setAvailableModels(localCatalog.map(buildLocalOption)); | ||
| setModelSource("fallback"); | ||
| setProviderFilter("__all__"); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [apiKey, providerId, providerType, providerUrl, proxyFallbackToDirect, proxyUrl]); |
There was a problem hiding this comment.
The loadModels function is an asynchronous operation called within a useEffect hook, but it lacks a mechanism to handle race conditions. If the dependencies (like apiKey) change rapidly, multiple requests might be in flight simultaneously, and a slower, stale request could overwrite the results of a newer one.
Consider using an AbortController or a local boolean flag within the useEffect to ignore results from stale executions.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const unknownProviders = availableModels | ||
| .map((model) => model.litellmProvider) | ||
| .filter((provider): provider is string => !!provider && !seen.has(provider)) | ||
| .sort((left, right) => left.localeCompare(right)); | ||
|
|
||
| for (const provider of unknownProviders) { | ||
| seen.add(provider); | ||
| options.push({ | ||
| value: provider, | ||
| label: knownProviders.has(provider) | ||
| ? tPrices(`filters.${knownProviders.get(provider)}`) | ||
| : provider, | ||
| }); | ||
| } |
There was a problem hiding this comment.
unknownProviders is built by mapping every model to its litellmProvider and filtering for values not yet in seen — but at that snapshot moment seen only contains PRICE_FILTER_VENDORS entries. Multiple models that share the same unknown provider (e.g. "gemini", "sagemaker") each pass the filter independently. The subsequent for loop then pushes every occurrence into options without re-checking seen, producing duplicate <SelectItem> nodes with the same key value. This triggers React key warnings and shows the same provider twice in the dropdown.
| const unknownProviders = availableModels | |
| .map((model) => model.litellmProvider) | |
| .filter((provider): provider is string => !!provider && !seen.has(provider)) | |
| .sort((left, right) => left.localeCompare(right)); | |
| for (const provider of unknownProviders) { | |
| seen.add(provider); | |
| options.push({ | |
| value: provider, | |
| label: knownProviders.has(provider) | |
| ? tPrices(`filters.${knownProviders.get(provider)}`) | |
| : provider, | |
| }); | |
| } | |
| const unknownProviders = availableModels | |
| .map((model) => model.litellmProvider) | |
| .filter((provider): provider is string => !!provider && !seen.has(provider)) | |
| .sort((left, right) => left.localeCompare(right)); | |
| for (const provider of unknownProviders) { | |
| if (seen.has(provider)) { | |
| continue; | |
| } | |
| seen.add(provider); | |
| options.push({ | |
| value: provider, | |
| label: knownProviders.has(provider) | |
| ? tPrices(`filters.${knownProviders.get(provider)}`) | |
| : provider, | |
| }); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/model-multi-select.tsx
Line: 133-146
Comment:
**Duplicate provider entries in filter dropdown**
`unknownProviders` is built by mapping every model to its `litellmProvider` and filtering for values not yet in `seen` — but at that snapshot moment `seen` only contains `PRICE_FILTER_VENDORS` entries. Multiple models that share the same unknown provider (e.g. `"gemini"`, `"sagemaker"`) each pass the filter independently. The subsequent `for` loop then pushes every occurrence into `options` without re-checking `seen`, producing duplicate `<SelectItem>` nodes with the same `key` value. This triggers React key warnings and shows the same provider twice in the dropdown.
```suggestion
const unknownProviders = availableModels
.map((model) => model.litellmProvider)
.filter((provider): provider is string => !!provider && !seen.has(provider))
.sort((left, right) => left.localeCompare(right));
for (const provider of unknownProviders) {
if (seen.has(provider)) {
continue;
}
seen.add(provider);
options.push({
value: provider,
label: knownProviders.has(provider)
? tPrices(`filters.${knownProviders.get(provider)}`)
: provider,
});
}
```
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: 514973eb44
ℹ️ 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 unknownProviders = availableModels | ||
| .map((model) => model.litellmProvider) | ||
| .filter((provider): provider is string => !!provider && !seen.has(provider)) | ||
| .sort((left, right) => left.localeCompare(right)); |
There was a problem hiding this comment.
When the fallback catalog contains multiple models from the same litellmProvider that is not in PRICE_FILTER_VENDORS, this pipeline keeps every occurrence and later renders duplicate SelectItem values/keys for the provider filter. That produces repeated options and can make selection behavior ambiguous for those providers; convert unknownProviders to a unique set before building options.
Useful? React with 👍 / 👎.
| setAvailableModels(upstreamModels); | ||
| setModelSource("upstream"); | ||
| setProviderFilter("__all__"); |
There was a problem hiding this comment.
loadModels can run concurrently (initial effect, dependency changes, and manual refresh), but each completion unconditionally calls setAvailableModels/setModelSource. If an earlier request resolves after a later one (for example while editing URL/key or clicking refresh twice), stale data can replace the most recent catalog in the picker. Add a request token/abort guard so only the latest invocation is allowed to commit state.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/actions/model-prices.ts (1)
377-380: 同步更新这个 helper 的注释。这里已经直接复用
getAvailableModelCatalog()的全部聊天模型，不再按 provider type 过滤；上面的“根据供应商类型”/@param providerType说明会继续误导后续调用方。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/actions/model-prices.ts` around lines 377 - 380, Update the JSDoc/comment for the helper getAvailableModelsByProviderType to reflect its current behavior: it now returns all chat model names by reusing getAvailableModelCatalog() without filtering by provider type, so remove or correct any "by provider type" description and delete the stale `@param` providerType tag; keep the function name and implementation as-is but make the comment clearly state it returns catalog.map(item => item.modelName) for all providers.src/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx (1)
27-27: 建议把新增导入改为@/别名路径。当前新增导入使用了相对路径，和仓库既定规范不一致。
♻️ 建议修改
-import { RuleTesterDialogTrigger } from "../../../rule-tester-dialog-trigger"; +import { RuleTesterDialogTrigger } from "@/app/[locale]/settings/providers/_components/rule-tester-dialog-trigger";As per coding guidelines: "**/*.{ts,tsx,js,jsx}: Use path alias
@/mapped to ./src/ for imports".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx at line 27, In routing-section.tsx update the import of RuleTesterDialogTrigger to use the project path alias '@/...' (which maps to ./src/) instead of the current relative import; locate the import line that references RuleTesterDialogTrigger, replace the relative path with the corresponding '@/...' module path, and ensure the project builds/typechecks after the change.
🤖 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/allowed-model-rule-editor.tsx:
- Around line 215-247: The exact-models handler handleExactModelsChange
currently rebuilds exact rules from selectedModels without enforcing the
100-rule cap, allowing value to grow beyond the component's limit; modify
handleExactModelsChange to apply the same max-rule check used elsewhere (the
logic at the other entry around lines 123-126) before adding new normalized
rules: compute currentCount = nextRules.length, and when iterating
normalizedSelections / creating nextRule via normalizeRule and identity via
getRuleIdentity only push new rules while currentCount < MAX_RULES (e.g., 100),
skipping or truncating excess selections, then call onChange with the capped
nextRules so value never exceeds the unified limit.
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx:
- Line 244: The current providerUrl prop uses the || operator which treats an
empty string as falsy and falls back to provider?.url, losing the user's
explicit "cleared" URL; change the fallback to use nullish coalescing so empty
string is preserved (e.g. replace providerUrl={state.basic.url || provider?.url}
with providerUrl={state.basic.url ?? provider?.url} or otherwise check for
null/undefined explicitly) so state.basic.url is honored even when it's "".
In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx:
- Around line 205-249: loadModels lacks concurrency protection so stale async
responses can overwrite newer state; introduce a per-call token (e.g., requestId
counter or AbortController stored in a ref) and capture it at the start of
loadModels, then verify the token is still current before calling setters like
setAvailableModels, setModelSource, setProviderFilter and before clearing
setLoading in the finally block; ensure the same guard is applied to the other
loadModels invocation site so only the latest request can mutate state.
- Around line 133-145: unknownProviders can contain duplicates because
availableModels is mapped and sorted but not deduped, causing duplicate
SelectItem entries and repeated keys; fix by deduplicating provider values
before pushing into options (e.g., build a Set or use Array.from(new Set(...))
or filter to only include the first occurrence) so that the loop over
unknownProviders (and use of seen, options, knownProviders, tPrices and the
SelectItem key={option.value}) only processes each provider once.
- Around line 210-225: The client currently calls
getUnmaskedProviderKey(providerId) and passes the returned plaintext into
fetchUpstreamModels(apiKey: resolvedKey), exposing secrets to the browser;
instead, change fetchUpstreamModels to accept an optional providerId and move
secret resolution to the server-side action (similar to resetProviderCircuit and
getProviderLimitUsage) so the client never requests or receives the unmasked
key. Update the call site in model-multi-select (remove getUnmaskedProviderKey
usage and pass providerId when apiKey is empty) and make the analogous change in
api-test-button.tsx so both call fetchUpstreamModels({ providerId, providerUrl,
providerType, proxyUrl, proxyFallbackToDirect }) rather than fetching and
sending the plaintext apiKey.
In `@tests/unit/settings/providers/vendor-keys-compact-list-ui.test.tsx`:
- Around line 66-69: The mock for getAvailableModelsByProviderType does not
match the real function signature (it currently returns an { ok, data } object)
— change the hoisted mock for getAvailableModelsByProviderType to return a
string[] (e.g. async () => [] or a list of model ids) so it matches the
production signature; keep getAvailableModelCatalog as-is. Update the vi.hoisted
block that defines modelPricesActionMocks and adjust only
getAvailableModelsByProviderType to return the correct type so
VendorKeysCompactList / ProviderForm tests behave like production.
---
Nitpick comments:
In `@src/actions/model-prices.ts`:
- Around line 377-380: Update the JSDoc/comment for the helper
getAvailableModelsByProviderType to reflect its current behavior: it now returns
all chat model names by reusing getAvailableModelCatalog() without filtering by
provider type, so remove or correct any "by provider type" description and
delete the stale `@param` providerType tag; keep the function name and
implementation as-is but make the comment clearly state it returns
catalog.map(item => item.modelName) for all providers.
In
`@src/app/`[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx:
- Line 27: In routing-section.tsx update the import of RuleTesterDialogTrigger
to use the project path alias '@/...' (which maps to ./src/) instead of the
current relative import; locate the import line that references
RuleTesterDialogTrigger, replace the relative path with the corresponding
'@/...' module path, and ensure the project builds/typechecks after the change.
🪄 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: 90b2b5ab-4a62-4cff-ae06-2a1a856e13f2
📒 Files selected for processing (21)
messages/en/settings/providers/form/allowedModelRules.jsonmessages/en/settings/providers/form/modelSelect.jsonmessages/ja/settings/providers/form/allowedModelRules.jsonmessages/ja/settings/providers/form/modelSelect.jsonmessages/ru/settings/providers/form/allowedModelRules.jsonmessages/ru/settings/providers/form/modelSelect.jsonmessages/zh-CN/settings/providers/form/allowedModelRules.jsonmessages/zh-CN/settings/providers/form/modelSelect.jsonmessages/zh-TW/settings/providers/form/allowedModelRules.jsonmessages/zh-TW/settings/providers/form/modelSelect.jsonsrc/actions/model-prices.tssrc/app/[locale]/settings/providers/_components/allowed-model-rule-editor.tsxsrc/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsxsrc/app/[locale]/settings/providers/_components/model-multi-select.tsxsrc/app/[locale]/settings/providers/_components/rule-tester-dialog-trigger.tsxtests/unit/settings/providers/allowed-model-rule-editor.test.tsxtests/unit/settings/providers/model-multi-select.test.tsxtests/unit/settings/providers/provider-form-endpoint-pool.test.tsxtests/unit/settings/providers/provider-form-total-limit-ui.test.tsxtests/unit/settings/providers/rule-tester-dialog-trigger.test.tsxtests/unit/settings/providers/vendor-keys-compact-list-ui.test.tsx
src/app/[locale]/settings/providers/_components/allowed-model-rule-editor.tsx
Show resolved
Hide resolved
...app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx
Outdated
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/model-multi-select.tsx
Outdated
Show resolved
Hide resolved
src/app/[locale]/settings/providers/_components/model-multi-select.tsx
Outdated
Show resolved
Hide resolved
| let resolvedKey = apiKey?.trim() || ""; | ||
| if (!resolvedKey && providerId) { | ||
| const keyResult = await getUnmaskedProviderKey(providerId); | ||
| if (keyResult.ok && keyResult.data?.key) { | ||
| resolvedKey = keyResult.data.key; | ||
| } | ||
| } | ||
|
|
||
| if (providerUrl && resolvedKey) { | ||
| const upstreamResult = await fetchUpstreamModels({ | ||
| providerUrl, | ||
| apiKey: resolvedKey, | ||
| providerType, | ||
| proxyUrl, | ||
| proxyFallbackToDirect, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
path = Path("src/actions/providers.ts")
lines = path.read_text().splitlines()
for name in ("getUnmaskedProviderKey", "fetchUpstreamModels"):
for i, line in enumerate(lines, 1):
if name in line:
start = max(1, i - 5)
end = min(len(lines), i + 80)
print(f"== {name} around line {i} ==")
for n in range(start, end + 1):
print(f"{n}:{lines[n-1]}")
print()
break
PY
rg -nC3 --type=tsx '\b(getUnmaskedProviderKey|fetchUpstreamModels)\s*\(' \
'src/app/[locale]/settings/providers/_components/model-multi-select.tsx'Repository: ding113/claude-code-hub
Length of output: 5583
🏁 Script executed:
# First, let's look at the model-multi-select.tsx file to understand the actual usage
cat -n src/app/[locale]/settings/providers/_components/model-multi-select.tsx | sed -n '200,235p'Repository: ding113/claude-code-hub
Length of output: 1449
🏁 Script executed:
# Now let's find the FetchUpstreamModelsArgs type definition
rg -n "type FetchUpstreamModelsArgs|interface FetchUpstreamModelsArgs" src/actions/providers.ts -A 15Repository: ding113/claude-code-hub
Length of output: 492
🏁 Script executed:
# Check if there's any providerId handling in fetchUpstreamModels or related functions
rg -n "providerId" src/actions/providers.ts | head -20Repository: ding113/claude-code-hub
Length of output: 1162
🏁 Script executed:
# Check if model-multi-select.tsx is a client component
head -5 src/app/[locale]/settings/providers/_components/model-multi-select.tsxRepository: ding113/claude-code-hub
Length of output: 117
🏁 Script executed:
# Check for other usages of getUnmaskedProviderKey
rg -n "getUnmaskedProviderKey" --type ts --type tsxRepository: ding113/claude-code-hub
Length of output: 94
🏁 Script executed:
# Check the imports and understand the component's context
sed -n '1,30p' src/app/[locale]/settings/providers/_components/model-multi-select.tsxRepository: ding113/claude-code-hub
Length of output: 922
🏁 Script executed:
# Search for getUnmaskedProviderKey usages with fd and grep
fd -e ts -e tsx -x grep -l "getUnmaskedProviderKey" {}Repository: ding113/claude-code-hub
Length of output: 1000
🏁 Script executed:
# Also check if there are other server actions that might accept providerId pattern
rg -n "export async function" src/actions/providers.ts | grep -E "(providerId|provider\s+id)" | head -10Repository: ding113/claude-code-hub
Length of output: 334
🏁 Script executed:
# Check the api-test-button.tsx usage to see if it has same pattern
grep -n -A 10 -B 5 "getUnmaskedProviderKey\|fetchUpstreamModels" src/app/[locale]/settings/providers/_components/forms/api-test-button.tsx | head -40Repository: ding113/claude-code-hub
Length of output: 1070
🏁 Script executed:
# Check vendor-keys-compact-list.tsx
grep -n -A 5 -B 5 "getUnmaskedProviderKey" src/app/[locale]/settings/providers/_components/vendor-keys-compact-list.tsx | head -30Repository: ding113/claude-code-hub
Length of output: 869
🏁 Script executed:
# Let's check what testProviderUnified and testProviderGemini signatures look like
rg -n "export async function testProviderUnified|export async function testProviderGemini" src/actions/providers.ts -A 3Repository: ding113/claude-code-hub
Length of output: 432
🏁 Script executed:
# Check if there's a pattern of server actions that accept providerId
rg -n "providerId.*:" src/actions/providers.ts | grep "export async function" -A 20 | head -30Repository: ding113/claude-code-hub
Length of output: 1534
不要在客户端补拿已保存的 provider 密钥。
Lines 210-225 中，当 apiKey 为空且 providerId 存在时，会先调用 getUnmaskedProviderKey(providerId) 获取明文密钥，再把返回值传给 fetchUpstreamModels() 作为 apiKey 参数。这会导致明文密钥被序列化到浏览器内存，再通过网络请求原样回传给服务器，掩码保护等于失效。
更安全的做法是在 fetchUpstreamModels() 中添加可选的 providerId 参数，当提供 providerId 时在服务端内部解析已保存的密钥，而不是将密钥暴露给客户端。这与现有的 resetProviderCircuit(providerId)、getProviderLimitUsage(providerId) 等 server action 的设计模式一致。
同样的问题也存在于 src/app/[locale]/settings/providers/_components/forms/api-test-button.tsx 中。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/`[locale]/settings/providers/_components/model-multi-select.tsx
around lines 210 - 225, The client currently calls
getUnmaskedProviderKey(providerId) and passes the returned plaintext into
fetchUpstreamModels(apiKey: resolvedKey), exposing secrets to the browser;
instead, change fetchUpstreamModels to accept an optional providerId and move
secret resolution to the server-side action (similar to resetProviderCircuit and
getProviderLimitUsage) so the client never requests or receives the unmasked
key. Update the call site in model-multi-select (remove getUnmaskedProviderKey
usage and pass providerId when apiKey is empty) and make the analogous change in
api-test-button.tsx so both call fetchUpstreamModels({ providerId, providerUrl,
providerType, proxyUrl, proxyFallbackToDirect }) rather than fetching and
sending the plaintext apiKey.
| const modelPricesActionMocks = vi.hoisted(() => ({ | ||
| getAvailableModelCatalog: vi.fn(async () => []), | ||
| getAvailableModelsByProviderType: vi.fn(async () => ({ ok: true, data: [] })), | ||
| })); |
There was a problem hiding this comment.
顺手把旧 action 的 mock 也对齐真实签名。
getAvailableModelsByProviderType() 在生产代码里返回的是 string[]，这里仍然 mock 成 { ok, data }。当前分支没踩到时问题不显，但一旦 VendorKeysCompactList 或 ProviderForm 走回这个路径，测试行为就会和生产不一致。
建议修改
const modelPricesActionMocks = vi.hoisted(() => ({
getAvailableModelCatalog: vi.fn(async () => []),
- getAvailableModelsByProviderType: vi.fn(async () => ({ ok: true, data: [] })),
+ getAvailableModelsByProviderType: vi.fn(async () => []),
}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/settings/providers/vendor-keys-compact-list-ui.test.tsx` around
lines 66 - 69, The mock for getAvailableModelsByProviderType does not match the
real function signature (it currently returns an { ok, data } object) — change
the hoisted mock for getAvailableModelsByProviderType to return a string[] (e.g.
async () => [] or a list of model ids) so it matches the production signature;
keep getAvailableModelCatalog as-is. Update the vi.hoisted block that defines
modelPricesActionMocks and adjust only getAvailableModelsByProviderType to
return the correct type so VendorKeysCompactList / ProviderForm tests behave
like production.
![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 restores the model picker UI for provider allowlists, implementing upstream fetching with fallback to local catalog. While the feature is well-structured and includes comprehensive tests, there are several issues that should be addressed.
PR Size: XL
- Lines changed: 1361 (1289 additions, 72 deletions)
- Files changed: 21
- Recommendation: This is a large PR touching multiple areas (UI, i18n, tests). Consider splitting future refactors into smaller chunks.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 1 |
| Tests | 0 | 0 | 0 | 1 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
[LOGIC-BUG] Duplicate provider entries in (line ~133)
The calculation filters by where only contains entries. Multiple models sharing the same unknown provider each pass the filter independently, causing duplicate dropdown entries and React key warnings.
Fix: Deduplicate before processing:
High Priority Issues (Should Fix)
[ERROR-SILENT] Upstream fetch errors not surfaced to user (line ~284)
When fails, the error is only logged internally and silently falls back. Users aren't informed why upstream fetch failed.
Fix: Surface the error to users before falling back, or at least log it clearly.
Low Priority Issues
[COMMENT-INACCURATE] Stale i18n keys (messages/*/settings/providers/form/modelSelect.json)
The locale files contain 7 unused keys (allowAllModels, manualAdd, manualDesc, manualPlaceholder, selectedEditEmpty, selectedEditExists, selectedListLabel) that should be removed.
[TEST-BRITTLE] Timing-based test waits (model-multi-select.test.tsx)
Tests rely on magic counts which are inherently brittle. Use explicit conditions or waitFor patterns instead.
Review Coverage
- Logic and correctness - 1 critical bug found
- Security (OWASP Top 10) - No issues detected
- Error handling - 1 silent failure identified
- Type safety - Clean
- Documentation accuracy - Stale i18n keys noted
- Test coverage - Adequate but brittle patterns
- Code clarity - Generally good
Automated review by Claude AI
There was a problem hiding this comment.
Code Review Summary
This PR restores the model picker UI for provider allowlists, implementing upstream fetching with fallback to local catalog. While the feature is well-structured and includes comprehensive tests, there are several issues that should be addressed.
PR Size: XL
- Lines changed: 1361 (1289 additions, 72 deletions)
- Files changed: 21
- Recommendation: This is a large PR touching multiple areas (UI, i18n, tests). Consider splitting future refactors into smaller chunks.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 1 | 0 | 0 | 0 |
| Error Handling | 0 | 1 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 1 |
| Tests | 0 | 0 | 0 | 1 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
[LOGIC-BUG] Duplicate provider entries in model-multi-select.tsx (line ~133)
The unknownProviders calculation has a deduplication bug. Multiple models sharing the same unknown provider cause duplicate dropdown entries and React key warnings.
Fix: Deduplicate with [...new Set(unknownProviders)] before processing.
High Priority Issues (Should Fix)
[ERROR-SILENT] Upstream fetch errors not surfaced to user (line ~284)
When fetchUpstreamModels fails, the error is only logged internally and silently falls back. Users are not informed why upstream fetch failed.
Fix: Surface the error to users before falling back.
Low Priority Issues
[COMMENT-INACCURATE] Stale i18n keys in all 5 locale files
The modelSelect.json files contain 7 unused keys that should be removed: allowAllModels, manualAdd, manualDesc, manualPlaceholder, selectedEditEmpty, selectedEditExists, selectedListLabel
[TEST-BRITTLE] Timing-based test waits in model-multi-select.test.tsx
Tests rely on magic flushTicks(5) counts which are inherently brittle. Use explicit conditions instead.
Review Coverage
- Logic and correctness - 1 critical bug found
- Security (OWASP Top 10) - No issues detected
- Error handling - 1 silent failure identified
- Type safety - Clean
- Documentation accuracy - Stale i18n keys noted
- Test coverage - Adequate but brittle patterns
- Code clarity - Generally good
Automated review by Claude AI
| } | ||
| } | ||
|
|
||
| const unknownProviders = availableModels |
There was a problem hiding this comment.
[Critical] [LOGIC-BUG] Duplicate provider entries in filter dropdown
The unknownProviders calculation filters by !seen.has(provider) where seen only contains PRICE_FILTER_VENDORS entries. If multiple models share the same unknown provider, each occurrence passes the filter independently because they all check against the same initial seen state. The subsequent for loop pushes all occurrences to options without checking for duplicates, causing React key warnings and duplicate dropdown entries.
Fix: Deduplicate before processing:
const uniqueUnknownProviders = [...new Set(unknownProviders)].sort(...);
|
|
||
| onChange(next); | ||
| }; | ||
|
|
There was a problem hiding this comment.
[High] [ERROR-SILENT] Upstream fetch errors not surfaced to user
When fetchUpstreamModels fails, the error is only logged internally and the code silently falls back to local catalog. Users are not informed that their upstream fetch failed or why, making debugging difficult.
Fix: Surface error to user before fallback:
if (!upstreamResult.ok && upstreamResult.error) {
logger.warn('Upstream fetch failed:', upstreamResult.error);
// setErrorState(upstreamResult.error) to display in UI
}
| @@ -2,20 +2,25 @@ | |||
| "allowAllModels": "Allow all {type} models", | |||
There was a problem hiding this comment.
[Low] [COMMENT-INACCURATE] Stale i18n keys not used by component
This file contains 7 unused keys: allowAllModels, manualAdd, manualDesc, manualPlaceholder, selectedEditEmpty, selectedEditExists, selectedListLabel. These appear to be from a previous implementation and should be removed to reduce translation maintenance burden.
Fix: Remove these keys from all 5 locale files (en, ja, ru, zh-CN, zh-TW).
| document.body.innerHTML = ""; | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Low] [TEST-BRITTLE] Test uses arbitrary flushTicks(5) timing-based waits
Tests rely on magic tick counts (5, 4, 2) to wait for async operations. This is inherently brittle and can cause flaky tests if timing changes.
Fix: Replace arbitrary tick counts with explicit conditions or waitFor pattern from testing-library.
| .filter(Boolean); | ||
|
|
||
| const handleExactModelsChange = (selectedModels: string[]) => { | ||
| const normalizedSelections = selectedModels.map((model) => model.trim()).filter(Boolean); |
There was a problem hiding this comment.
[HIGH] [LOGIC-BUG] Exact-model picker bypasses 100-rule cap
Evidence: src/app/[locale]/settings/providers/_components/allowed-model-rule-editor.tsx adds exact rules in handleExactModelsChange without enforcing the existing cap:
if (value.length >= 100) {
setError(t("maxRules"));
return;
}Why this is a problem: The picker supports bulk actions (Select All / Invert) and can push the allowlist past 100 rules, contradicting the UI contract and risking oversized provider configs.
Suggested fix:
const MAX_RULES = 100;
const nonExactRules = value.filter((rule) => rule.matchType !== "exact");
const availableSlots = Math.max(0, MAX_RULES - nonExactRules.length);
const limitedSelections = normalizedSelections.slice(0, availableSlots);
setError(limitedSelections.length < normalizedSelections.length ? t("quickAddReachedLimit") : null);
// Build `nextRules` using `limitedSelections` instead of `normalizedSelections`.
| let resolvedKey = apiKey?.trim() || ""; | ||
| if (!resolvedKey && providerId) { | ||
| const keyResult = await getUnmaskedProviderKey(providerId); | ||
| if (keyResult.ok && keyResult.data?.key) { |
There was a problem hiding this comment.
[HIGH] [SECURITY-VULNERABILITY] Unmasked provider key is fetched on mount to load models
Evidence: src/app/[locale]/settings/providers/_components/model-multi-select.tsx calls getUnmaskedProviderKey(providerId) inside loadModels() and useEffect(() => void loadModels(), ...) runs on mount, so simply rendering the routing section can fetch the full API key into the browser.
This also triggers the audit log in src/actions/providers.ts:2898 ("Admin viewed provider key") even when the user never explicitly views/copies the key.
Why this is a problem: It expands secret exposure surface area and creates noisy/misleading audit logs; it also adds unnecessary network work for initial render when the picker may never be opened.
Suggested fix (lazy-load + avoid key fetch unless needed):
useEffect(() => {
if (!open) return;
void loadModels();
}, [loadModels, open]);
// In loadModels(), only resolve the key when you will actually attempt upstream fetch.
if (!providerUrl) {
const localCatalog = await getAvailableModelCatalog();
setAvailableModels(localCatalog.map(buildLocalOption));
setModelSource("fallback");
return;
}If you want upstream-by-default without exposing keys to the client, consider updating fetchUpstreamModels to accept providerId and resolve the stored key server-side.
|
|
||
| const providerActionMocks = vi.hoisted(() => ({ | ||
| fetchUpstreamModels: vi.fn(async () => ({ ok: false, error: "upstream unavailable" })), | ||
| getUnmaskedProviderKey: vi.fn(async () => ({ ok: false })), |
There was a problem hiding this comment.
[HIGH] [TEST-MISSING-CRITICAL] Upstream-success branch is untested
Evidence: tests/unit/settings/providers/model-multi-select.test.tsx currently mocks fetchUpstreamModels as always { ok: false }, so the upstream-preferred path in src/app/[locale]/settings/providers/_components/model-multi-select.tsx is never exercised.
Why this is a problem: Preferring upstream models is the core behavior of the picker. Without a test, regressions like "still fetching local catalog on upstream success" or "provider filter shown for upstream" can slip in.
Suggested fix (add an upstream test):
test("prefers upstream models when available", async () => {
const messages = loadMessages();
providerActionMocks.fetchUpstreamModels.mockResolvedValueOnce({
ok: true,
data: { models: ["claude-opus-4-1", "claude-sonnet-4-1"], source: "upstream" },
});
const { unmount } = render(
<NextIntlClientProvider locale="en" messages={messages} timeZone="UTC">
<ModelMultiSelect
providerType="claude"
providerUrl="https://api.example.com"
apiKey="sk-test"
selectedModels={[]}
onChange={vi.fn()}
/>
</NextIntlClientProvider>
);
// If you change to lazy-load on popover open, open the trigger before awaiting ticks.
await flushTicks(5);
expect(modelPricesActionMocks.getAvailableModelCatalog).not.toHaveBeenCalled();
expect(providerActionMocks.fetchUpstreamModels).toHaveBeenCalled();
unmount();
});
There was a problem hiding this comment.
Code Review Summary
This PR introduces a new exact-model picker (upstream-first with local fallback) and refactors rule testing UI into dialog triggers in the provider routing settings. The change set is XL and spans UI, server actions, translations, and tests; a couple of issues should be addressed to avoid exceeding the allowlist rule cap and to reduce unnecessary unmasked-key retrieval/audit logging.
PR Size: XL
- Lines changed: 1361
- Files changed: 21
- Split suggestion (recommended for reviewability): (1) server action + i18n message updates, (2) UI components/refactor (picker + dialog trigger + routing section wiring), (3) test additions/mocks.
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 0 | 0 |
| Security | 0 | 1 | 0 | 0 |
| Error Handling | 0 | 0 | 0 | 0 |
| Types | 0 | 0 | 0 | 0 |
| Comments/Docs | 0 | 0 | 0 | 0 |
| Tests | 0 | 1 | 0 | 0 |
| Simplification | 0 | 0 | 0 | 0 |
Critical Issues (Must Fix)
src/app/[locale]/settings/providers/_components/allowed-model-rule-editor.tsxexact-model picker can exceed the 100-rule cap (confidence 98).src/app/[locale]/settings/providers/_components/model-multi-select.tsxfetches unmasked provider key on mount (secret exposure surface + noisy audit logging) (confidence 97).
High Priority Issues (Should Fix)
tests/unit/settings/providers/model-multi-select.test.tsxno unit test covers upstream-success branch of the picker (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.
PR #997 review completed.
- Applied label
size/XL. - Left inline comments on:
src/app/[locale]/settings/providers/_components/allowed-model-rule-editor.tsx#L215– exact-model picker can bypass the 100-rule cap.src/app/[locale]/settings/providers/_components/model-multi-select.tsx#L211– unmasked provider key fetched on mount (secret exposure surface + noisy audit logging); recommend lazy-load and/or server-side key resolution.tests/unit/settings/providers/model-multi-select.test.tsx#L38– upstream-success branch is untested; add a focused unit test.
- Submitted the required markdown summary review via
gh pr review --comment.
|
第二轮修复已推送，已处理这轮 review 里关于 allowlist 规则上限、provider 去重、lazy-load upstream 拉取、以及 upstream 成功分支测试的建议。 当前 PR 剩余唯一红灯仍是
这两条我已在当前 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 728c51c4ae
ℹ️ 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".
| if ((!resolvedKey || resolvedKey.includes("***")) && providerId) { | ||
| const keyResult = await getUnmaskedProviderKey(providerId); | ||
| if (keyResult.ok && keyResult.data?.key) { | ||
| resolvedKey = keyResult.data.key; |
There was a problem hiding this comment.
This branch unconditionally calls getUnmaskedProviderKey(providerId) whenever providerId is present and the form key is blank/masked, before checking whether providerUrl is even configured. In edit mode this means simply opening the picker can pull the full secret key into client memory and emit a false "Admin viewed provider key" audit event, even when the component will immediately fall back to local catalog data and never attempt an upstream fetch. Move key unmasking inside the providerUrl && ... path (or perform upstream fetch server-side by provider id) so secrets are only retrieved when actually needed.
Useful? React with 👍 / 👎.
1 participant
Summary
Restores a model picker in the provider allowlist editing UI that integrates with the new rule-based allowlist system from #996. Models selected from the picker are added as exact-match rules, while advanced match types (prefix, suffix, contains, regex) remain in the detailed rule editor below. The implementation prefers upstream provider model lists when available, falling back to the local price catalog sorted by latest update.
Problem
PR #996 upgraded provider allowlists from simple
string[]matching to a powerful rule-based system with 5 match types (exact/prefix/suffix/contains/regex). While this was a major improvement, the transition removed the convenient model picker UI that allowed users to quickly select models from a list. Users had to manually type every model name, even for exact matches.Additionally, debugging allowlist/redirect rules was done via inline collapsible sections that consumed significant vertical space and created visual clutter.
Related Issues and PRs:
Solution
Model Multi-Select Component (
model-multi-select.tsx): New component that fetches models from upstream provider APIs when credentials are available, falling back to the local price catalog sorted by latest update. Features include:Integrated Exact-Match Picker: The
AllowedModelRuleEditornow includes a dedicated section at the top for adding exact-match rules via the model picker, clearly separated from the advanced rule editor below.Compact Rule Testing: Replaced inline collapsible testers with
RuleTesterDialogTrigger- a tooltip-triggered dialog pattern that reduces visual clutter while keeping testing functionality accessible.Changes
Core Changes
src/app/[locale]/settings/providers/_components/model-multi-select.tsx(+502): New model picker component with upstream fetching, provider filtering, and bulk selectionsrc/app/[locale]/settings/providers/_components/rule-tester-dialog-trigger.tsx(+68): New compact dialog trigger component for rule testingsrc/app/[locale]/settings/providers/_components/allowed-model-rule-editor.tsx(+70/-0): Integrated model picker for exact-match rulessrc/app/[locale]/settings/providers/_components/forms/provider-form/sections/routing-section.tsx(+35/-47): Replaced collapsible testers with dialog triggers, connected provider credentials to model pickersrc/actions/model-prices.ts(+44/-15): AddedgetAvailableModelCatalog()for fallback catalog with provider and update time metadatai18n (5 languages: en, ja, ru, zh-CN, zh-TW)
allowedModelRules.json(+2 each): AddedexactPickerTitleandexactPickerDescriptionmodelSelect.json(+7/-2 each): AddedemptyLabel,exactMatchHint,invertSelection,providerFilterAll,providerFilterPlaceholder, updated selected group labels for clarityTests
tests/unit/settings/providers/model-multi-select.test.tsx(+279): Comprehensive tests for model picker componenttests/unit/settings/providers/rule-tester-dialog-trigger.test.tsx(+118): Tests for dialog trigger componenttests/unit/settings/providers/allowed-model-rule-editor.test.tsx(+125): Extended tests for integrated model picker functionalityTesting
Automated Tests
model-multi-select,rule-tester-dialog-trigger)Manual Testing Steps
Breaking Changes
None - this is a pure UI enhancement that builds on the rule-based allowlist system from #996. The underlying data format remains
AllowedModelRule[].Screenshots / Preview
Model Picker Integration
The exact-match model picker now appears at the top of the allowlist editor, clearly separated from the advanced rule editor:
Compact Rule Testing
Rule testers now open in dialogs triggered by tooltip buttons, reducing visual clutter:
Checklist
Description enhanced by Claude AI
Greptile Summary
This PR restores the model picker UI for provider allowlist editing, adding upstream model fetching with a local price catalog fallback and replacing inline rule testers with compact
RuleTesterDialogTriggerdialogs. The duplicate-provider-entry bug in the filter dropdown (flagged in a previous review) is correctly resolved withArray.from(new Set(...))at line 135–141 ofmodel-multi-select.tsx. No blocking issues were found.Confidence Score: 5/5
Safe to merge — no P0/P1 issues; only minor style findings remain
All findings are P2: six orphaned i18n keys and one duplicate hint label. Core logic is sound — request-ID race protection, provider deduplication, exact-rule sync with the 100-rule cap, and the admin auth guard on getAvailableModelCatalog are all implemented correctly. The previously flagged duplicate-provider-entry bug is fixed.
No files require special attention
Important Files Changed
Sequence Diagram
sequenceDiagram participant U as User participant MM as ModelMultiSelect participant PA as providers action participant MP as model-prices action U->>MM: Opens popover MM->>MM: requestId++, setLoading(true) alt providerId present & key is masked MM->>PA: getUnmaskedProviderKey(providerId) PA-->>MM: resolvedKey end alt providerUrl && resolvedKey available MM->>PA: fetchUpstreamModels() alt upstream success PA-->>MM: models[] MM->>MM: setAvailableModels(upstream) MM->>MM: setModelSource(upstream) else upstream failed PA-->>MM: error MM->>MM: setFallbackNotice() MM->>MP: getAvailableModelCatalog() MP-->>MM: catalog[] MM->>MM: setAvailableModels(catalog) MM->>MM: setModelSource(fallback) end else no URL or key MM->>MP: getAvailableModelCatalog() MP-->>MM: catalog[] MM->>MM: setAvailableModels(catalog) MM->>MM: setModelSource(fallback) end MM->>MM: setLoading(false) [only if requestId still current]Comments Outside Diff (1)
messages/en/settings/providers/form/modelSelect.json, line 2-30 (link)Seven keys —
allowAllModels,manualAdd,manualDesc,manualPlaceholder,selectedEditEmpty,selectedEditExists, andselectedListLabel— are defined here and mirrored across all five locale files, but none of them are referenced inModelMultiSelector any other component in this PR. If they were removed during an earlier refactor they should be pruned from all locale files to avoid dead translation maintenance burden.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "fix: harden provider whitelist model pic..." | Re-trigger Greptile