fix(provider): allow removing custom whitelisted models (#592)#593
fix(provider): allow removing custom whitelisted models (#592)#593
Conversation
📝 WalkthroughWalkthrough该拉取请求为 ModelMultiSelect 组件添加了显示派生模型列表的功能,该列表合并了可用模型与已选模型,确保即使在远程列表中不存在的自定义模型也能显示并支持移除操作,并新增相应的单元测试用例。 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
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! 此拉取请求旨在增强模型管理的用户体验,特别是针对自定义模型。它解决了一个关键问题,即用户无法从白名单中移除那些不在供应商默认可用模型列表中的手动添加模型。通过优化模型列表的渲染机制,确保所有已选模型都能在界面上可见并可操作,从而提升了系统的健壮性和用户对模型配置的控制能力。 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
|
| const displayedModels = (() => { | ||
| const seen = new Set<string>(); | ||
| const merged: string[] = []; | ||
|
|
||
| for (const model of availableModels) { | ||
| if (seen.has(model)) continue; | ||
| seen.add(model); | ||
| merged.push(model); | ||
| } | ||
|
|
||
| // 关键:把已选中但不在远端列表的自定义模型也渲染出来,保证可取消选中 | ||
| for (const model of selectedModels) { | ||
| if (seen.has(model)) continue; | ||
| seen.add(model); | ||
| merged.push(model); | ||
| } | ||
|
|
||
| return merged; | ||
| })(); |
There was a problem hiding this comment.
为了提高代码的可读性和性能,建议简化 displayedModels 的实现。
当前的 IIFE 写法有些冗长。我们可以使用 Set 和展开语法 ... 来更简洁地合并和去重数组。
此外,将此计算包装在 useMemo hook 中可以避免在组件因其他状态(如 customModel)更新而重新渲染时进行不必要的重复计算。
请在文件顶部引入 useMemo:
import { useCallback, useEffect, useMemo, useState } from "react";然后可以将 displayedModels 的实现修改如下:
const displayedModels = useMemo(
() => [...new Set([...availableModels, ...selectedModels])],
[availableModels, selectedModels],
);
🧪 测试结果
总体结果: ✅ 所有测试通过 |
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/[locale]/settings/providers/_components/model-multi-select.tsx
Line: 149:149
Comment:
[P1] Clicking "Select All" removes custom models. The `selectAll` function uses `availableModels`, but after your fix, the displayed list includes custom models from `selectedModels`. When a user with custom models clicks "Select All", those custom models get removed since they're not in `availableModels`. Change to `onChange(displayedModels)` to preserve custom models.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx (1)
25-61: 考虑提取测试辅助函数以提高复用性
loadMessages、render和flushTicks是通用的测试辅助函数,建议提取到共享的测试工具文件(如tests/utils/test-helpers.tsx)中,以便在其他测试文件中复用,提高可维护性。♻️ 建议的重构方案
创建
tests/utils/test-helpers.tsx:import fs from "node:fs"; import path from "node:path"; import type { ReactNode } from "react"; import { act } from "react"; import { createRoot } from "react-dom/client"; import { NextIntlClientProvider } from "next-intl"; export function loadMessages() { const base = path.join(process.cwd(), "messages/en"); const read = (name: string) => JSON.parse(fs.readFileSync(path.join(base, name), "utf8")); return { common: read("common.json"), errors: read("errors.json"), ui: read("ui.json"), forms: read("forms.json"), settings: read("settings.json"), }; } export function renderWithIntl(node: ReactNode, locale = "en") { const messages = loadMessages(); const container = document.createElement("div"); document.body.appendChild(container); const root = createRoot(container); act(() => { root.render( <NextIntlClientProvider locale={locale} messages={messages} timeZone="UTC"> {node} </NextIntlClientProvider> ); }); return { unmount: () => { act(() => root.unmount()); container.remove(); }, }; } export async function flushTicks(times = 3) { for (let i = 0; i < times; i++) { await act(async () => { await new Promise((r) => setTimeout(r, 0)); }); } }然后在测试文件中导入使用:
import { loadMessages, renderWithIntl, flushTicks } from "@/tests/utils/test-helpers";
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
📒 Files selected for processing (2)
src/app/[locale]/settings/providers/_components/model-multi-select.tsxtests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use emoji characters in any code, comments, or string literals
Files:
src/app/[locale]/settings/providers/_components/model-multi-select.tsxtests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: All user-facing strings must use i18n (5 languages supported: zh-CN, zh-TW, en, ja, ru). Never hardcode display text
Use path alias@/to reference files in./src/directory
Format code with Biome using: double quotes, trailing commas, 2-space indent, 100 character line width
Files:
src/app/[locale]/settings/providers/_components/model-multi-select.tsxtests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer named exports over default exports
Files:
src/app/[locale]/settings/providers/_components/model-multi-select.tsxtests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
All new features must have unit test coverage of at least 80%
Files:
tests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx
tests/**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Vitest for unit testing and happy-dom for DOM testing
Files:
tests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx
🧠 Learnings (1)
📚 Learning: 2026-01-10T17:53:25.066Z
Learnt from: CR
Repo: ding113/claude-code-hub PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-10T17:53:25.066Z
Learning: Applies to tests/**/*.test.{ts,tsx,js,jsx} : Use Vitest for unit testing and happy-dom for DOM testing
Applied to files:
tests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx
🧬 Code graph analysis (1)
tests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx (1)
src/app/[locale]/settings/providers/_components/model-multi-select.tsx (1)
ModelMultiSelect(43-343)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: pr-review
- GitHub Check: 🌐 API Tests
- GitHub Check: pr-description
- GitHub Check: pr-label
- GitHub Check: check-codex-status
- GitHub Check: Docker Build Test
- GitHub Check: dev-build-deploy
🔇 Additional comments (5)
src/app/[locale]/settings/providers/_components/model-multi-select.tsx (3)
61-79: 合并逻辑实现正确!
displayedModels的去重和合并逻辑清晰且正确:
- 先添加
availableModels中的所有模型- 再追加
selectedModels中不在远程列表的自定义模型- 使用
Set避免重复这很好地解决了自定义模型无法在 UI 中显示和移除的问题。
149-149: 验证"全选"按钮的用户体验当前
selectAll使用availableModels,这意味着点击"全选"会将selectedModels设置为仅包含远程模型列表,可能会意外取消用户已选中的自定义模型。建议确认这是否是期望的行为。如果"全选"应该保留现有自定义模型并添加所有可用模型,考虑改为:
const selectAll = () => { const customModels = selectedModels.filter(m => !availableModels.includes(m)); onChange([...availableModels, ...customModels]); };
290-290: 渲染逻辑更新正确正确使用
displayedModels进行渲染,确保自定义模型可见并可交互。tests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx (2)
1-23: 测试环境和 mock 设置正确
- 正确使用
happy-dom环境- Mock 正确使用
vi.hoisted()提升- 模拟的返回值符合测试场景:
availableModels仅包含remote-model-1,而测试用例选中的是custom-model-x符合 learnings 中关于使用 Vitest 和 happy-dom 的要求。
68-111: 测试用例全面覆盖回归场景测试逻辑清晰且完整:
- ✓ 渲染组件时提供不在
availableModels中的custom-model-x- ✓ 等待异步加载完成
- ✓ 打开下拉列表
- ✓ 验证自定义模型出现在列表中(关键回归点)
- ✓ 模拟点击取消选中
- ✓ 验证
onChange被调用且参数为空数组测试准确验证了 PR 修复的问题:即使模型不在远程列表中,已选中的自定义模型也应该可见并可移除。
There was a problem hiding this comment.
Code Review Summary
This PR successfully fixes the issue where custom whitelisted models couldn't be removed from the UI. However, the implementation introduces a logic inconsistency that affects the "Select All" functionality.
PR Size: XS
- Lines changed: 134 (133 additions, 1 deletion)
- Files changed: 2
Issues Found
| Category | Critical | High | Medium | Low |
|---|---|---|---|---|
| Logic/Bugs | 0 | 1 | 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 | 1 | 0 |
| Performance | 0 | 0 | 1 | 0 |
High Priority Issues (Should Fix)
1. [LOGIC-BUG] selectAll function inconsistent with displayed models
Location: src/app/[locale]/settings/providers/_components/model-multi-select.tsx:149
Problem: The selectAll function uses availableModels, but the UI now renders displayedModels (which includes custom whitelisted models). This creates confusing UX behavior.
Impact:
- User sees N models in the list, but "Select All" button shows a different count
- Clicking "Select All" doesn't select all visible models
- If custom models are already selected, "Select All" will deselect them
Example scenario:
// State:
availableModels = ["model-a", "model-b"]
selectedModels = ["custom-x"]
displayedModels = ["model-a", "model-b", "custom-x"] // 3 visible items
// User clicks "Select All (2)" button
// Result: selectedModels = ["model-a", "model-b"]
// Bug: "custom-x" was DESELECTED instead of remaining selectedSuggested fix:
// Line 149
const selectAll = () => onChange(displayedModels);
// Line 272
{t("selectAll", { count: displayedModels.length })}Medium Priority Issues (Consider Fixing)
2. [PERFORMANCE-ISSUE] displayedModels recalculates on every render
Location: src/app/[locale]/settings/providers/_components/model-multi-select.tsx:61-78
Problem: The displayedModels computation uses an IIFE that runs on every component render, creating unnecessary Set and array allocations.
Why this matters:
- Component already uses
useCallbackforloadModels(line 95) - Inconsistent with React best practices
- Unnecessary recalculation when unrelated state changes (e.g.,
open,customModel)
Suggested fix:
import { useCallback, useEffect, useMemo, useState } from "react";
const displayedModels = useMemo(() => {
const seen = new Set<string>();
const merged: string[] = [];
for (const model of availableModels) {
if (seen.has(model)) continue;
seen.add(model);
merged.push(model);
}
for (const model of selectedModels) {
if (seen.has(model)) continue;
seen.add(model);
merged.push(model);
}
return merged;
}, [availableModels, selectedModels]);3. [TEST-INCOMPLETE] Missing test coverage for "Select All" behavior
Location: tests/unit/settings/providers/model-multi-select-custom-models-ui.test.tsx
Problem: The test file only covers the deselection scenario but doesn't test the "Select All" button behavior with custom models. This would have caught issue #1.
CLAUDE.md requirement: "All new features must have unit test coverage of at least 80%"
Suggested additional test:
test("Select All should select all visible models including custom ones", async () => {
const messages = loadMessages();
const onChange = vi.fn();
const { unmount } = render(
<NextIntlClientProvider locale="en" messages={messages} timeZone="UTC">
<ModelMultiSelect
providerType="claude"
selectedModels={["custom-model-x"]}
onChange={onChange}
/>
</NextIntlClientProvider>
);
await flushTicks(5);
const trigger = document.querySelector("button[role='combobox']") as HTMLButtonElement | null;
await act(async () => {
trigger?.click();
});
await flushTicks(5);
const selectAllBtn = Array.from(document.querySelectorAll("button"))
.find(btn => btn.textContent?.includes("Select All"));
await act(async () => {
selectAllBtn?.click();
});
// Should select ALL visible models (remote-model-1 + custom-model-x)
expect(onChange).toHaveBeenLastCalledWith(["remote-model-1", "custom-model-x"]);
unmount();
});Review Coverage
- Logic and correctness - 1 issue found
- Security (OWASP Top 10) - Clean
- Error handling - Clean
- Type safety - Clean
- Documentation accuracy - Clean
- Test coverage - 1 issue found
- Performance - 1 issue found
Automated review by Claude AI
There was a problem hiding this comment.
- Applied the correct PR size label: removed
size/L, addedsize/XLon PR#591. - Posted 2 inline review comments (both High):
src/lib/validation/schemas.ts:35—[STANDARD-VIOLATION]hardcodedproviderGroupmax-length message bypasses i18n.src/lib/validation/schemas.ts:169—[TEST-MISSING-CRITICAL]missing boundary unit tests for the new 200-char limit.
- Submitted the required review summary via
gh pr review --commenton PR#591.
* PR: 修复 Edge Runtime 下 `process.once` 构建告警(AsyncTaskManager 导入链) (#589) * fix: skip async task manager init on edge * fix: avoid static async task manager import * test: cover edge runtime task scheduling * chore: document edge runtime process.once fix * chore: record edge runtime warning baseline * fix: drop NEXT_PHASE and lazy-init async task manager * test: isolate NEXT_RUNTIME in cloud price sync tests * docs: stabilize edge process.once repro baseline * docs: make rollback instructions hashless * docs: add grep checklist for edge warning audit * chore: run regression gate and align docs * test: cover edge runtime guard on register * Update src/lib/async-task-manager.ts 补充 NEXT_PHASE === "phase-production-build" 检查 Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * chore: format code (fix-edge-runtime-process-once-bee7e19) * PR:i18n settings 拆分与翻译质量门禁 (#588) * refactor(i18n): split settings json into smaller files * refactor(i18n): load settings from split module * refactor(i18n): remove legacy settings.json * chore(i18n): update sync-settings-keys for split layout * test(i18n): add split settings guards * chore: align biome schema version * chore(i18n): document messages loading contract * chore(i18n): add settings split verification notes * chore: format code (refactor-i18n-split-settings-3f48fec) * chore: fix i18n request formatting * chore: format code (refactor-i18n-split-settings-a1eff62) * fix: replace settings placeholder translations * chore: verify settings sync script is idempotent * test: run i18n settings split guards * test: add audit for zh-CN placeholder settings strings * chore: apply biome formatting * chore: document manual i18n settings verification * fix: translate all providers filter in ja * fix: translate all providers filter in zh-TW * fix: translate providers section copy in zh-TW * fix: translate providers section copy in ja * feat: extend placeholder audit output * feat: add allowlist for placeholder audit * docs: define i18n translation quality rules * chore: add i18n audit fail commands * docs: add i18n PR checklist * chore: format i18n audit tests * fix: translate dashboard placeholders * fix: translate myUsage placeholders * fix: enforce locale-specific parentheses * fix: start translating provider form strings * fix: translate provider form strings * fix: translate provider guide content * test: add ja dashboard parentheses guard * test: add zh-TW dashboard parentheses guard * test: add zh-TW myUsage parentheses guard * chore: translate ja provider form strings * chore: translate zh-TW provider form strings * chore: translate ja providers guide * chore: translate zh-TW providers guide * chore: refine zh-TW dashboard strings * chore: translate ja providers strings * chore: translate zh-TW providers strings * chore: refine zh-TW api test strings * chore: translate zh-TW settings small modules * chore: translate ja settings small modules * chore: clear i18n placeholders in settings * chore: format code (refactor-i18n-split-settings-2437d19) * test: fix biome formatting in i18n test * chore: verify Biome lint gate (I18NE-030) * chore: add messages emoji audit script (I18NE-010) * fix: remove emoji from messages warnings (I18NE-040) * test: add messages no-emoji audit gate (I18NE-050) * docs: add zh-CN i18n docs (I18NE-020) * docs: add messages no-emoji rule (I18NE-060) * chore: run full regression checks (I18NE-070) * docs: add i18n PR evidence template (I18NE-080) * fix: make messages no-emoji audit path-sep safe * docs: add bun alias for messages no-emoji audit * fix: detect keycap and flag emoji sequences in i18n message audits * fix(provider): allow removing custom whitelisted models (#592) (#593) * fix(rectifier): detect 'signature: Field required' error and trigger rectifier (#594) - Extend detectThinkingSignatureRectifierTrigger to match 'signature: Field required' - Add Rule 72 for friendly error message when signature field is missing - Add comprehensive test cases for the new detection logic This fixes an issue where switching from non-Anthropic to Anthropic channels with thinking blocks missing signature fields would fail without proper handling. * feat(users): increase provider group length to 200 (#591) close #590 * feat(usage-doc): update OpenCode config example with GPT-5.2 and Gemini v1beta (#597) - Add OpenAI GPT-5.2 model configuration with reasoningEffort options - Add GPT-5.2-small variant using medium reasoning effort - Fix Gemini baseURL to use /v1beta endpoint - Update i18n strings to reflect different baseURLs per provider * feat: auto-complete Codex session identifiers (#599) * fix: Codex session completion must not inject metadata (#601) * feat: auto-complete Codex session identifiers * fix: avoid Codex metadata injection --------- Co-authored-by: YangQing-Lin <56943790+YangQing-Lin@users.noreply.github.com> Co-authored-by: Hwwwww-dev <47653238+Hwwwww-dev@users.noreply.github.com>
问题
当用户通过手动添加模型把模型加入白名单,但该模型不在供应商远端模型列表(availableModels)中时,列表不会渲染该模型,导致无法取消勾选进行单个删除。
相关 Issue:
修复
变更
核心变更
model-multi-select.tsx: 添加displayedModels计算逻辑,合并availableModels和selectedModels,确保自定义白名单模型可见且可取消选中测试覆盖
model-multi-select-custom-models-ui.test.tsx(112 行)验证
Description enhanced by Claude AI