fix(web-shell): encode vision model picker selection & polish dispatch#6236
Conversation
Address Wenshao's review comments on QwenLM#6209: [Critical] Encode vision model selection before persisting - handleVisionModelSelect now strips ACP (authType) suffix and stores as authType:modelId format expected by core's resolveVisionModelSelection() - Without this, picker selections silently fail to resolve when the same model ID appears on multiple providers [Suggestion] Add currentVisionModel derivation - Mirror currentVoiceModel pattern so the picker highlights the active vision model instead of falling back to the main model [Suggestion] Extract MODE_TITLE_KEY record for exhaustive dispatch - Replace duplicated 4-way ternary in App.tsx dialog title with a single Record<ModelDialogMode, string> lookup - Replace if/else if/else onSelect chain with a handlers record that would fail at compile time if a new mode is added without a handler [Suggestion] Add settings.label/description.visionModel i18n keys - Add to both EN and ZH locales so Chinese users see proper labels in the Settings dialog Files changed: - App.tsx: encoding fix, currentVisionModel, MODE_TITLE_KEY, handlers record - i18n.tsx: visionModel label + description (EN + ZH)
Address Wenshao's review comments on QwenLM#6209: [Critical] Encode vision model selection before persisting - handleVisionModelSelect now strips ACP (authType) suffix and stores as authType:modelId format expected by core's resolveVisionModelSelection() - Without this, picker selections silently fail to resolve when the same model ID appears on multiple providers [Suggestion] Add currentVisionModel derivation - Mirror currentVoiceModel pattern so the picker highlights the active vision model instead of falling back to the main model [Suggestion] Extract MODE_TITLE_KEY record for exhaustive dispatch - Replace duplicated 4-way ternary in App.tsx dialog title with a single Record<ModelDialogMode, string> lookup - Replace if/else if/else onSelect chain with a handlers record that would fail at compile time if a new mode is added without a handler [Suggestion] Add settings.label/description.visionModel i18n keys - Add to both EN and ZH locales so Chinese users see proper labels in the Settings dialog Files changed: - App.tsx: encoding fix, currentVisionModel, MODE_TITLE_KEY, handlers record - i18n.tsx: visionModel label + description (EN + ZH)
|
@qwen-code-ci-bot & @wenshao kindly review |
|
Thanks for the PR, @AmariahAK! Template looks good ✓ On direction: this fixes a genuine data-loss bug — users pick a vision model from the web-shell picker, see a success toast, but their choice is silently discarded because the stored format doesn't match what core expects. That's a real user problem and squarely within qwen-code's web-shell + model selection scope. The polish items (exhaustive handlers, i18n keys, active model highlighting) are natural follow-ups from #6209's review. Aligned. On approach: the scope feels right for a fix + polish PR. The encoding fix is a targeted 3-line change. The One minor note: the regex Moving on to code review. 🔍 中文说明感谢贡献，@AmariahAK！ 模板完整 ✓ 方向：这修复了一个真实的数据丢失 bug——用户从 web-shell 选择器中选择视觉模型后看到成功提示，但选择被静默丢弃，因为存储格式与 core 期望的格式不匹配。这是真实的用户问题，完全在 qwen-code 的 web-shell + 模型选择范围内。优化项（穷举 handlers、i18n 键、活动模型高亮）是 #6209 审查中的自然后续。方向对齐。 方案：作为修复+优化的 PR，范围合理。编码修复是一个精准的 3 行改动。 一个小注：正则 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading the diff): I would have done exactly what this PR does — add a conversion step in Comparison: The PR's approach matches my independent proposal point for point. The regex Reuse check: The CLI has No critical blockers found. No security issues. Follows project conventions. One minor nit (non-blocking): in the Real-Scenario TestingThis PR modifies a browser-based React component (web-shell model picker dialog). It cannot be meaningfully tested via tmux/CLI — there's no terminal interaction to drive. The feature requires CI status at time of review: Verdict: Code review clean. Real-scenario testing deferred to the reviewer's local environment (browser) or CI once it completes. 中文说明代码审查独立方案（阅读 diff 之前）：我会做和这个 PR 完全一样的事情——在 对比：PR 的方案与我的独立方案逐点吻合。正则 复用检查：CLI 有 未发现关键阻塞问题。 无安全问题。遵循项目约定。 一个小 nit（不阻塞）： 真实场景测试此 PR 修改的是浏览器端 React 组件（web-shell 模型选择对话框）。无法通过 tmux/CLI 进行有意义的测试——没有终端交互可驱动。该功能需要 CI 状态： 结论：代码审查通过。真实场景测试留待审查者在本地环境（浏览器）中验证，或等 CI 完成后确认。 — Qwen Code · qwen3.7-max |
ReflectionThis is a clean, focused PR that solves a real problem: users select a vision model from the web-shell picker and their choice gets silently discarded because of a format mismatch between the picker's ACP encoding and core's expected format. The fix is 3 lines of conversion logic that correctly bridges the two formats. The polish items are all natural follow-ups from #6209's review — exhaustive handlers for compile-time safety, a single-source-of-truth title map, active model highlighting, and missing i18n keys. Nothing here is speculative or over-engineered. The approach matches my independent proposal exactly. The code is straightforward — no abstractions, no unnecessary indirection, no drive-by refactors. If I had to maintain this in six months, I'd thank the author for making the dispatch exhaustive and the encoding conversion obvious. One non-blocking nit: Can't verify in browser from this environment (web-shell is a React UI, not a CLI feature), but the code review is clean and the logic is correct. Approving. 中文说明总结这是一个干净、聚焦的 PR，解决了一个真实问题：用户从 web-shell 选择器中选择视觉模型后，选择因选择器的 ACP 编码与 core 期望格式不匹配而被静默丢弃。修复是 3 行转换逻辑，正确桥接了两种格式。 优化项都是 #6209 审查中的自然后续——穷举 handlers 确保编译时安全、单一来源的标题映射、活动模型高亮、缺失的 i18n 键。没有投机性代码或过度工程。 方案与我的独立方案完全吻合。代码直截了当——没有抽象、没有不必要的间接、没有顺手重构。如果六个月后我要维护这段代码，我会感谢作者让分发穷举且编码转换一目了然。 一个不阻塞的 nit： 无法在此环境中通过浏览器验证（web-shell 是 React UI，不是 CLI 功能），但代码审查通过，逻辑正确。批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings from independent multi-agent analysis. The encoding fix correctly converts ACP format to authType:modelId, the MODE_TITLE_KEY / handlers record extraction is type-safe and exhaustive, and 694/699 functional tests pass (5 build-artifact test failures are environment-only — require npm run build first). Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
🔬 Maintainer verification — built & tested locally (as a merge reference)I built PR #6236 at its head ( TL;DR
Evidence (real
|
| 声明 | 结果 |
|---|---|
编码修复 modelId(authType) → authType:modelId 能被 core 解析 |
✅ 正确（运行时往返测试，含 id 内含冒号的情形） |
web-shell 测试套件（699）+ tsc -p tsconfig.lib.json |
✅ 703 通过（699 + 我新增 4 个），tsc 退出码 0 |
| 新增 EN/ZH i18n 键能解析（不回退到裸键） | ✅ 确认 |
| 第 2 步：选择器高亮当前视觉模型 | ❌ 失败——"current" ✓ 从不出现 |
| CI 机器人称 "voice/fast 同样损坏" |
证据（真实 ModelDialog 无头渲染，暗色主题）
- 左（BUG）：
currentModelId= PR 实际持久化的值（qwen-oauth:qwen-vl-max）。任何位置都没有 ✓ 标记；漫游高亮甚至回退到第 1 行（coder-model）——错误的模型。 - 中（对照）：
currentModelId= ACP 形式（qwen-vl-max(qwen-oauth)）。✓ 正确标记第 2 行。→ 说明选择器能高亮，问题纯粹出在喂给它的格式。 - 右（i18n）：
视觉模型+ 中文描述通过真实的formatSettingLabel/getTranslator('zh-CN')解析得到。
🔴 唯一真正的问题：高亮被喂了错误的格式
修复把值存为 authType:modelId（对 core 是正确的）。但同一个存储字符串又被作为 currentModelId 传回选择器，而选择器用严格 === 去匹配 ACP 格式的 id：
ModelDialog.tsx:111→availableModels.findIndex((m) => m.id === currentModel)m.id是 ACP 格式modelId(authType)（构造于packages/cli/src/utils/acpModelUtils.ts:20）currentVisionModel是authType:modelId（App.tsx，编码修复之后）
'qwen-vl-max(qwen-oauth)' === 'qwen-oauth:qwen-vl-max' 永远为假 → currentIdx = -1 → ✓ 标记（ModelDialog.tsx:189、aria-selected）从不出现，activeIndex 回退到 0。
这恰恰是该优化项本想消除的困惑。 最小修复（与 diff 上已有的行内建议一致）是在传给选择器前解码回 ACP：
const colonIdx = value.indexOf(':');
const currentVisionModel = colonIdx > 0
? `${value.slice(colonIdx + 1)}(${value.slice(0, colonIdx)})`
: value;我确认即便 id 内含冒号，这个逆变换也正确（openai:gpt-4o:online → gpt-4o:online(openai)）。
关于自动化审查的评论（我已代为核验）
- 编码正确性 —— ✅ 属实。
parseModelIdSelector（modelId.ts:90-113）按第一个冒号切分，且当前缀是已知AuthType时接受它（qwen-oauth就是其一）。往返测试通过；修复前的裸 ACP 串确实会丢掉 authType。 - "voice/fast 同样损坏" ——
⚠️ 过度概括。 我核对了两条路径：handleFastModelSelect不写裸设置——它发送/model --fast <id>（sendPrompt），走命令处理器，是另一条路径。voiceModel是被use-voice-input.ts→transcribe(...)消费的 ASR id，不经过resolveModelId。- 所以这个格式不匹配确实是视觉专属的，PR 的窄范围是站得住脚的。（一个不阻塞的开放问题：ASR 端点是否容忍 ACP 包裹的 id，值得单独看看。）
- "镜像 CLI 的
encodeVisionModelSelector" —— 大体如此，但该正则未处理 CLI 的$runtime|authType|modelId快照 id 和\0baseUrl。对标准模型没问题；选中运行时快照会编码出畸形 selector。属边缘情形，不阻塞。 - 零测试覆盖 —— ✅ 属实；我已在本地写好相应测试（编码 + 高亮 A/B + i18n），建议纳入仓库。
建议
编码修复（真正的数据丢失 bug）是正确的——👍 可以合并。 请补上约 3 行的 currentVisionModel 解码，让第 2 步真正生效（很简单，且 PR 明确声称了它），并为编码补一个测试。分发清理（MODE_TITLE_KEY / handlers）与 i18n 键保持现状即可。
Verified locally against head 05c6c71: real worktree build, vitest/tsc, and real-component screenshots via Playwright/Chromium. Not a CI run.
|
@qwen-code-ci-bot & @wenshao all changes requested have been correctly implemented, kindly take a look |
- Extract encodeVisionModelForSetting / decodeVisionModelForPicker into shared utils/modelEncoding.ts so they can be tested in isolation - Add 17 unit tests covering ACP encoding, colon-bearing IDs, empty parens passthrough, and round-trip identity - Memoize modelHandlers record with useMemo to avoid re-allocation on every model picker click - Replace dead fallback (?? 'main') — the outer modelDialogMode guard already ensures non-null, so use an explicit if-guard instead
- Add passthrough tests for already-encoded colon format - Add malformed input tests (bare authType, unclosed paren, double-parens) - Add leadin-colon malformed input test for decode - Add empty string passthrough test - 23 encoding tests passing (up from 17), full suite: 735 passing
|
@qwen-code-ci-bot & @wenshao kindly review |
| // Model IDs from the picker arrive in ACP format: `modelId(authType)`. | ||
| // Core's resolveFastModelSelector() expects `authType:modelId`. | ||
| const encoded = encodeFastModelForSetting(modelId); | ||
| sendPrompt(`/model --fast ${encoded}`).catch((error: unknown) => { |
There was a problem hiding this comment.
[Suggestion] encodeFastModelForSetting is dead code here. getModelSelectId() in ModelDialog.tsx strips the (authType) suffix when isFastMode is true (return model.baseModelId ?? model.id.replace(/\([^()]+\)$/, '')), so this handler receives a bare model ID like qwen-max. The regex in encodeFastModelForSetting never matches, making it a pass-through.
The comment "Model IDs from the picker arrive in ACP format" is incorrect for fast mode. The code works because core's resolveModelId has a fallback for bare IDs, but the dead encode call and misleading comment suggest the fast-model path wasn't traced end-to-end.
Consider either removing the dead encode call (and fixing the comment), or changing getModelSelectId to NOT strip authType in fast mode — mirroring the vision path.
— qwen3.7-max via Qwen Code /review
| currentModelId={ | ||
| modelDialogMode === 'voice' ? currentVoiceModel : undefined | ||
| modelDialogMode === 'voice' | ||
| ? currentVoiceModel |
There was a problem hiding this comment.
[Suggestion] The currentModelId ternary has no 'fast' branch. When modelDialogMode === 'fast', it falls through to undefined, and ModelDialog falls back to connection.currentModel (the main chat model) — highlighting the wrong model in the fast picker.
This PR added currentVisionModel (great!) but missed currentFastModel. Consider adding it following the same derivation pattern.
— qwen3.7-max via Qwen Code /review
| (modelId: string) => { | ||
| setWorkspaceSetting('workspace', 'voiceModel', modelId).catch( | ||
| // Model IDs from the picker arrive in ACP format: `modelId(authType)`. | ||
| // Voice model resolution expects bare model IDs (not authType:modelId). |
There was a problem hiding this comment.
[Nice to have] The comment says "Model IDs from the picker arrive in ACP format" but this is incorrect for voice mode — extractVoiceModels() sets id: model.baseModelId (a bare ID with no (authType) suffix). extractBareModelId is a no-op on the already-bare input. The behavior is correct, but the comment documents the wrong contract.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Two suggestions below — no blocking issues found.
— qwen3.7-max via Qwen Code /review
| /** | ||
| * Encodes a fast model ID from ACP format (modelId(authType)) to storage format | ||
| * (authType:modelId). Core's resolveFastModelSelector() expects this format. | ||
| * If the input is not in ACP format, returns it unchanged. |
There was a problem hiding this comment.
[Suggestion] encodeVisionModelForSetting and encodeFastModelForSetting have byte-identical implementations (same regex, same template string). Two named exports with the same body is a maintenance trap — a future fix in one could be missed in the other.
Since encodeFastModelForSetting is effectively dead code anyway (the fast-mode picker strips the ACP suffix before calling the handler), consider removing it entirely and using encodeVisionModelForSetting at both call sites — or extracting a single encodeAcpToStorage(modelId) function.
| * If the input is not in ACP format, returns it unchanged. | |
| /** | |
| * Encodes a model ID from ACP format (modelId(authType)) to storage format | |
| * (authType:modelId). Used by vision and fast model handlers. | |
| * If the input is not in ACP format, returns it unchanged. | |
| */ | |
| export function encodeAcpToStorage(modelId: string): string { | |
| const match = modelId.match(/^(.+)\(([^()]+)\)$/); | |
| return match ? `${match[2]}:${match[1]}` : modelId; | |
| } |
— qwen3.7-max via Qwen Code /review
| const modelHandlers: Record<ModelDialogMode, (id: string) => void> = useMemo( | ||
| () => ({ | ||
| main: handleModelSelect, | ||
| fast: handleFastModelSelect, |
There was a problem hiding this comment.
[Nice to have] The useMemo wrapping modelHandlers provides no benefit here — it's only consumed inside an inline onSelect closure (which already creates a new function reference each render), and ModelDialog is not React.memo-wrapped. The four useCallback dependencies are stable references, so the useMemo dependency-array comparison overhead exceeds the cost of a plain object literal.
A plain const modelHandlers = { main: handleModelSelect, ... } without useMemo would be equivalent and less noisy.
— qwen3.7-max via Qwen Code /review
| * (e.g., 'openai:gpt-4o:online' → 'gpt-4o:online(openai)'). | ||
| * If the input has no colon, returns it unchanged. | ||
| */ | ||
| export function decodeVisionModelForPicker(storedValue: string): string { |
There was a problem hiding this comment.
[Suggestion] decodeVisionModelForPicker doesn't handle the \0baseUrl suffix that the CLI stores for vision models with custom endpoints. In modelCommand.ts:590-596, the CLI constructs authType:modelId\0baseUrl when the matched model has a baseUrl. This decoder splits on the first : and produces a corrupted ACP string like modelId\0https://endpoint(authType), which won't match any picker model ID — the "current" highlight silently fails for custom-endpoint vision models.
| export function decodeVisionModelForPicker(storedValue: string): string { | |
| export function decodeVisionModelForPicker(storedValue: string): string { | |
| const nullIdx = storedValue.indexOf('\0'); | |
| const selector = nullIdx >= 0 ? storedValue.slice(0, nullIdx) : storedValue; | |
| const colonIdx = selector.indexOf(':'); | |
| if (colonIdx > 0) { | |
| return `${selector.slice(colonIdx + 1)}(${selector.slice(0, colonIdx)})`; | |
| } | |
| return selector; | |
| } |
— qwen3.7-max via Qwen Code /review
✅ Local verification — real build + tests + screenshot (HEAD
|
| Stored value | resolveModelId result |
Matches a configured model? |
|---|---|---|
qwen-oauth:qwen-vl-max (after fix) |
{authType:'qwen-oauth', modelId:'qwen-vl-max'} |
✅ yes |
qwen-vl-max(qwen-oauth) (old ACP) |
{modelId:'qwen-vl-max(qwen-oauth)'} (no authType) |
❌ no → silent auto-select fallback |
The old ACP string has no authType: prefix, so parseModelIdSelector treats the whole thing (suffix included) as a bare id that can never equal a configured id — exactly the "success toast, choice silently discarded" bug. 5/5 core round-trip tests pass, plus the PR's own 23/23 modelEncoding.test.ts.
2. Highlight polish — ✅ works at HEAD (the [Critical] is stale)
The [Critical] says "currentVisionModel reads the stored setting in authType:modelId format … these two formats never match … the indicator will never light up." That premise is no longer true at HEAD: App.tsx:1667-1673 decodes the stored value back to ACP via decodeVisionModelForPicker before handing it to the picker, and App.tsx:3521-3522 wires it to currentModelId. The decode was added in 4ad0e11a ("address review comments"); GitHub re-anchored the older inline comment onto the latest commit.
I proved it three ways (real component render + real browser), including the counterfactual that the decode is load-bearing:
| Case | currentModelId passed |
Result |
|---|---|---|
| A — with fix | decode('qwen-oauth:qwen-vl-max') → qwen-vl-max(qwen-oauth) |
exactly 1 highlight, on the qwen-vl-max row ✅ |
| B — no decode | raw qwen-oauth:qwen-vl-max |
0 highlights, roving falls to row 0 (⇐ the bug the [Critical] describes) |
| C — no derivation | undefined |
falls back to the main model row (the pre-PR regression) |
Real-Chromium assertion (computed ::after content = the ✓):
withFix : rows=3, checkMarkRows=["2.[qwen-oauth]qwen-vl-max"], ariaSelected=["2.[qwen-oauth]qwen-vl-max"]
withoutFix : rows=3, checkMarkRows=[], ariaSelected=[]
Left = HEAD (decode wired): the blue ✓ + rail sit on qwen-vl-max, and its detail panel shows the true vision modality (text·image·pdf). Right = the same picker if the raw authType:modelId were handed to it undecoded: no ✓ anywhere (row-1's faint tint is just default keyboard focus on the main model).
3. i18n — ✅ present in both locales
settings.label.visionModel / settings.description.visionModel and model.setVision exist in EN (i18n.tsx:966,1219-1220) and ZH (:2158,2462-2463) — no bare-key fallback.
4. Tests & types
- web-shell vitest: 760/760 green. (In a fresh worktree,
build-artifact.test.ts's 5 checks readdist/index.js; afternpm run buildthey pass.) tsc -p tsconfig.lib.json: 0 new errors. The 12 errors seen in a symlinked worktree (loadingTranscript,parentAgentId/parentName/depth) are identical at the merge-base without this PR — including inTasksStatusMessage.tsx, a file this PR never touches. They're stale symlinked-SDK declarations, not a PR regression; a clean CI checkout resolves them.
Minor / non-blocking (optional follow-ups)
\0baseUrldecode gap (bot [Suggestion], confirmed real but narrow):decodeVisionModelForPickersplits only on:and doesn't strip theauthType:modelId\0baseUrlform the CLI persists for custom-endpoint vision pins → the✓silently won't light up for those (highlight-only, cross-path CLI→web; the web-shell's own encode never produces\0baseUrl, so its round-trip is fine).encodeFastModelForSettingis effectively dead (App.tsx:3330):getModelSelectIdalready strips(authType)for fast mode, so this handler receives a bare id and the regex never fires — harmless pass-through.currentModelIdternary has no'fast'branch (App.tsx:3518): the fast picker highlights the main model, not the pinned fast model. Pre-existing (voice/vision only); worth a follow-up.encodeVisionModelForSetting/encodeFastModelForSettingare byte-identical — a maintenance trap; fine to collapse later.
None of these block merge. LGTM — the data-loss fix and the highlight both verified working.
中文版（完整对应）
✅ 本地验证 —— 真实构建 + 测试 + 截图（HEAD f710b40d）
我针对真实 HEAD 做了验证（真跑组件/浏览器测试，不只是重读 diff）。结论：正确，可合并。 两个交付物 —— 视觉模型编码数据丢失修复与高亮 polish —— 都端到端可用。唯一仍未关闭的机器人 [Critical]「当前指示器永不点亮」是过期评论（其前提在 HEAD 已不成立）；下面我把「坏」和「好」两种状态都复现出来。
方法
- 在 PR head
f710b40d新建 worktree。 - 用真实
ModelDialog做组件测试（jsdom +createRoot）、用真实 Chromium Playwright 渲染选择器、并在主克隆（原生安装，避开 ajv 深导入 split-brain）跑一遍 core 的真实resolveModelId/parseVisionModelSetting往返。
1. 编码修复（数据丢失 bug）—— ✅ 端到端验证
handleVisionModelSelect（App.tsx:3350）在持久化前把选择器的 ACP id modelId(authType) 编码成 core 的 authType:modelId。我把 PR 真实的 encodeVisionModelForSetting 输出喂给 core 的真实解析器：
| 存储值 | resolveModelId 结果 |
是否匹配已配置模型 |
|---|---|---|
qwen-oauth:qwen-vl-max（修复后） |
{authType:'qwen-oauth', modelId:'qwen-vl-max'} |
✅ 匹配 |
qwen-vl-max(qwen-oauth)（旧 ACP） |
{modelId:'qwen-vl-max(qwen-oauth)'}（无 authType） |
❌ 不匹配 → 静默回退自动选择 |
旧 ACP 串没有 authType: 前缀，parseModelIdSelector 会把整串（含后缀）当作裸 id，永远等不上任何已配置 id —— 正是「弹成功提示但选择被静默丢弃」的 bug。core 往返 5/5 通过，PR 自带 modelEncoding.test.ts 23/23 通过。
2. 高亮 polish —— ✅ HEAD 上有效（[Critical] 已过期）
[Critical] 称*「currentVisionModel 以 authType:modelId 格式读取……两种格式永不相等……指示器永不点亮」*。该前提在 HEAD 已不成立：App.tsx:1667-1673 先用 decodeVisionModelForPicker 把存储值解码回 ACP，再由 App.tsx:3521-3522 接到 currentModelId。解码是在 4ad0e11a（"address review comments"）加入的；GitHub 把旧的行内评论重新锚定到了最新 commit。
我用三种方式（真实组件渲染 + 真实浏览器）证明，并给出「解码不可或缺」的反事实：
| 用例 | 传入的 currentModelId |
结果 |
|---|---|---|
| A —— 有修复 | decode('qwen-oauth:qwen-vl-max') → qwen-vl-max(qwen-oauth) |
恰好 1 处高亮，落在 qwen-vl-max 行 ✅ |
| B —— 无解码 | 原始 qwen-oauth:qwen-vl-max |
0 处高亮，roving 回退到第 0 行（⇐ [Critical] 描述的 bug） |
| C —— 无派生 | undefined |
回退高亮主模型行（PR 之前的回归） |
真实 Chromium 断言（计算得到的 ::after 内容 = ✓）：
withFix : rows=3, checkMarkRows=["2.[qwen-oauth]qwen-vl-max"], ariaSelected=["2.[qwen-oauth]qwen-vl-max"]
withoutFix : rows=3, checkMarkRows=[], ariaSelected=[]
（截图见上方英文部分）左 = HEAD（已接解码）：蓝色 ✓ + 左侧竖条落在 qwen-vl-max，其详情面板显示真正的视觉模态（text·image·pdf）。右 = 若把原始 authType:modelId 未解码直接交给选择器：任何行都没有 ✓（第 1 行淡淡的底色只是键盘焦点默认落在主模型上）。
3. i18n —— ✅ 双语齐全
settings.label.visionModel / settings.description.visionModel 与 model.setVision 在 EN（i18n.tsx:966,1219-1220）与 ZH（:2158,2462-2463）均存在，无裸键回退。
4. 测试与类型
- web-shell vitest：760/760 全绿。（新 worktree 里
build-artifact.test.ts的 5 项要读dist/index.js；npm run build后即通过。） tsc -p tsconfig.lib.json：0 个新增错误。 符号链接 worktree 里看到的 12 个错误（loadingTranscript、parentAgentId/parentName/depth）在不含本 PR 的 merge-base 上完全一致 —— 包括本 PR 从未改动的TasksStatusMessage.tsx。这是陈旧的符号链接 SDK 声明导致的 split-brain，不是 PR 回归；干净的 CI 检出即可消除。
次要 / 不阻塞（可选后续）
\0baseUrl解码缺口（机器人 [Suggestion]，确属真实但很窄）：decodeVisionModelForPicker只按:切分，不处理 CLI 为自定义端点视觉 pin 持久化的authType:modelId\0baseUrl，导致这类情况✓静默不亮（仅影响高亮，且是 CLI→web 跨路径；web-shell 自身编码从不产生\0baseUrl，故其往返没问题）。encodeFastModelForSetting实为死代码（App.tsx:3330）：getModelSelectId在 fast 模式已剥掉(authType)，该 handler 收到裸 id，正则永不命中 —— 无害透传。currentModelId三元表达式没有'fast'分支（App.tsx:3518）：fast 选择器高亮的是主模型而非固定的 fast 模型。属既有行为（只有 voice/vision），可后续补。encodeVisionModelForSetting/encodeFastModelForSetting字节完全相同 —— 维护陷阱，日后可合并。
以上均不阻塞合并。LGTM —— 数据丢失修复与高亮均已验证有效。
Verified locally by the maintainer: real worktree @ f710b40d, real ModelDialog component tests, real-Chromium Playwright render, and a core resolveModelId/parseVisionModelSetting round-trip.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No new review findings beyond the existing discussion by @qwen-code-ci-bot and @wenshao. The core data-loss bug (ACP→authType:modelId encoding for the vision picker) is correctly fixed, the new modelEncoding helpers have solid test coverage (23 tests, round-trip + edge cases), i18n keys are added for both EN and ZH, and the MODE_TITLE_KEY/modelHandlers record extraction improves compile-time safety. Deterministic analysis (tsc, eslint, vitest) passes for all changed files. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
…ghlight - decodeVisionModelForPicker: strip \0baseUrl suffix before decoding to ACP - Remove dead encodeFastModelForSetting (fast picker strips ACP suffix before handler) - Add currentFastModel derivation + 'fast' branch to currentModelId ternary - Fix misleading voice handler comment (bare IDs, not ACP) - Replace unnecessary useMemo on modelHandlers with plain object Co-authored-by: atlarix-agent <agent@atlarix.dev>
|
@DragonnZhang @qwen-code-ci-bot @wenshao kindly review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No new issues found beyond the existing discussion. The encoding fix correctly converts ACP format to authType:modelId, the MODE_TITLE_KEY/handlers record extraction is type-safe and exhaustive, and the new modelEncoding.ts utility has solid test coverage (round-trip, edge cases, \0baseUrl stripping). 761 tests pass, eslint clean.
— qwen3.7-max via Qwen Code /review
4 participants
What this PR does
Fixes a critical bug where selecting a vision model from the web-shell picker silently fails to pin the model. The picker passes model IDs in ACP format (
modelId(authType)), but core'sresolveVisionModelSelection()expectsauthType:modelId— so the stored value never resolves and the vision bridge falls back to auto-select. The fix strips the ACP suffix and re-encodes before persisting, mirroring what the CLI'sencodeVisionModelSelectoralready does.Also addresses three polish items from Wenshao's review on #6209: derives
currentVisionModelso the picker highlights the active vision model (instead of falling back to the main model), extracts aMODE_TITLE_KEYrecord and exhaustivehandlersrecord to replace a duplicated 4-way ternary and if/else chain, and adds missingsettings.label.visionModel/settings.description.visionModeli18n keys to both EN and ZH locales.Why it's needed
Without the encoding fix, users who select a vision model from the picker or Settings button see a success toast but their choice is silently discarded — the vision bridge can't resolve the stored
coder-model(qwen-oauth)string and falls back to auto-select. The Settings page still shows the value, which masks the failure. This is a data-loss bug: the user's explicit choice has no effect.The polish items prevent future regressions: the exhaustive
handlersrecord means adding a newModelDialogModemember would fail at compile time if no handler is wired, and the i18n keys ensure Chinese users see localized labels instead of English fallbacks.Reviewer Test Plan
How to verify
npm run dev:daemon) and open the web UI/model --vision— confirm the picker opens and the currently active vision model (if any) is highlighted with the "current" indicator.qwen/settings.jsonshowsauthType:modelIdformat, notmodelId(authType))cd packages/web-shell && npx vitest run— confirm all 699 tests passcd packages/web-shell && npx tsc --noEmit -p tsconfig.lib.json— confirm zero type errorsEvidence (Before & After)
N/A (non-visual change; verified via unit tests — 699 tests pass, 0 failures; tsc clean)
Tested on
Environment (optional)
npm run dev:daemon(local),cd packages/web-shell && npx vitest run(unit tests)Risk & Scope
/model --vision <bare-id>inline command is unchanged (bare IDs already resolve correctly).visionModelsettings stored as bare IDs continue to resolve correctly.Linked Issues
Follow-up to #6209 (addresses Wenshao's review comments).
中文说明
本 PR 做了什么
修复了一个关键 bug：从 web-shell 选择器中选择视觉模型时，选择会被静默丢弃。选择器传递的模型 ID 是 ACP 格式（
modelId(authType)），但 core 的resolveVisionModelSelection()期望的是authType:modelId格式——因此存储的值永远无法解析，视觉桥接会回退到自动选择。修复方案是去除 ACP 后缀并在持久化前重新编码，与 CLI 的encodeVisionModelSelector行为一致。同时处理了 Wenshao 在 #6209 审查中提出的三个优化项：派生
currentVisionModel使选择器能高亮当前活动的视觉模型（而非回退到主模型）；提取MODE_TITLE_KEY记录和穷举handlers记录，替换重复的四元条件表达式和 if/else 链；为 EN 和 ZH 两个语言环境添加缺失的settings.label.visionModel/settings.description.visionModeli18n 键。为什么需要
如果不修复编码问题，用户从选择器或设置按钮中选择视觉模型后会看到成功提示，但他们的选择实际上被静默丢弃——视觉桥接无法解析存储的
coder-model(qwen-oauth)字符串，回退到自动选择。设置页面仍然显示该值，掩盖了故障。这是一个数据丢失 bug：用户的明确选择不生效。优化项可防止未来回归：穷举
handlers记录意味着添加新的ModelDialogMode成员时，如果未连接处理器，编译时就会报错；i18n 键确保中文用户看到本地化标签而非英文回退。审查者测试计划
如何验证
npm run dev:daemon）并打开 web UI/model --vision——确认选择器打开，且当前活动的视觉模型（如有）以"current"指示器高亮.qwen/settings.json显示authType:modelId格式，而非modelId(authType)）cd packages/web-shell && npx vitest run——确认全部 699 个测试通过cd packages/web-shell && npx tsc --noEmit -p tsconfig.lib.json——确认零类型错误证据（修改前与修改后）
N/A（非可视化变更；通过单元测试验证——699 个测试全部通过，0 失败；tsc 无错误）
测试平台
环境（可选）
npm run dev:daemon（本地），cd packages/web-shell && npx vitest run（单元测试）风险与范围
/model --vision <bare-id>内联命令未更改（裸 ID 已能正确解析）。visionModel设置仍可正确解析。关联 Issue
#6209 的后续（处理 Wenshao 的审查意见）。