Repository navigation
fix(web-shell): encode vision model picker selection & polish dispatch - #6236
Conversation
Address Wenshao's review comments on #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 #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 |
🔬 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
✅ 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.
- 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 |
3 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 的审查意见）。