feat(cli): Add model toggle hotkey (Ctrl+F) - #6486
Conversation
|
Thanks for the PR, @Aleks-0! Template looks good ✓ Problem: This is a feature request, not a bugfix — linked to issue #6442 (also filed by the same author). The issue explicitly asks whether the feature is worth building, which is a good sign of thoughtful scoping. The use case (rapid cost-driven model switching) is niche but clearly articulated. Direction: Aligned. Model switching is already a first-class feature ( Size: Not applicable — no core module paths touched. All changes are in Approach: Scope feels right. The diff does exactly what's described: a new One observation before diving into code review: the Moving on to code review. 🔍 中文说明感谢贡献，@Aleks-0！ 模板完整 ✓ 问题： 这是一个功能请求，不是 bug 修复——关联 issue #6442（也是同一作者提交的）。该 issue 主动讨论了这个功能是否值得实现，说明作者对范围有深思熟虑。使用场景（快速成本驱动的模型切换）虽然小众，但表述清晰。 方向： 对齐。模型切换已经是核心功能（ 规模： 不适用——未触及核心模块路径。所有改动在 方案： 范围合理。diff 精确实现了描述的功能：新增 一个观察： 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
2a. Code ReviewIndependent proposal (before reading diff): I would have added a new Comparison: The PR's approach matches my independent proposal. The implementation is clean and follows existing patterns in the codebase. Findings:
No critical blockers. No AGENTS.md violations. The code is minimal, well-structured, and follows project conventions. 2b. Real-Scenario TestingUnit tests: All pass ✅
Interactive test (tmux): Started Limitation: Full toggle verification (model actually switching + info message appearing) requires — Qwen Code · qwen3.7-max |
|
This PR is a clean, minimal implementation of a well-scoped feature request. The linked issue (#6442) is unusually thoughtful for a feature request — the author explicitly questions whether the feature is worth building, identifies the niche audience (power users managing model costs), and acknowledges the tradeoffs. That self-awareness carries through to the implementation: the feature is opt-in (no-op without Going back to my independent proposal from Stage 2a — the PR's approach matches it. I don't see a materially simpler path. The The code review found only minor observations (hardcoded shortcut display keys, silent error catching in keypress handler, integration test naming) — none are blockers. Unit tests all pass, typecheck is clean, and the CLI starts without issues with the PR applied. Approving. ✅ 中文说明这个 PR 是一个范围清晰、实现简洁的功能请求的干净实现。 关联的 issue (#6442) 对功能请求来说异常深思熟虑——作者主动质疑该功能是否值得构建，识别了小众受众（管理模型成本的重度用户），并承认了权衡。这种自觉延续到了实现中：功能是可选的（未配置 回顾 Stage 2a 的独立方案——PR 的方法与之匹配。我没有找到更简单的路径。 代码审查仅发现了次要观察（硬编码的快捷键显示、按键处理器中的静默错误捕获、集成测试命名）——均不构成阻塞。单元测试全部通过，类型检查干净，CLI 在应用 PR 后正常启动。 批准 ✅ — 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. ✅
| if (keyMatchers[Command.TOGGLE_MODEL](key)) { | ||
| const toggleModel = settings.merged.model?.toggleModel; | ||
| if (!toggleModel) { | ||
| return; |
There was a problem hiding this comment.
[Critical] When toggleModel is not configured (the default), the handler matches Ctrl+F, enters this block, hits return, and swallows the keypress — preventing TOGGLE_SHELL_INPUT_FOCUS (line 3565) from ever seeing it. This breaks Ctrl+F for all users who haven't set model.toggleModel.
| return; | |
| if (settings.merged.model?.toggleModel && keyMatchers[Command.TOGGLE_MODEL](key)) { | |
| const toggleModel = settings.merged.model.toggleModel; |
This way, when toggleModel is undefined, the entire branch is skipped and the keypress falls through to subsequent handlers.
— qwen3.7-max via Qwen Code /review
| // Model toggle | ||
| [Command.TOGGLE_MODEL]: [ | ||
| { key: 's', meta: true }, | ||
| { key: 'f', ctrl: true }, |
There was a problem hiding this comment.
[Critical] { key: 'f', ctrl: true } has shift: undefined, which per matchKeyBinding (line 33: "binding undefined = ignore this modifier") means Ctrl+Shift+F also matches. The test's reference lambda at keyMatchers.test.ts:99 explicitly uses !key.shift, confirming the intent was Ctrl+F only.
| { key: 'f', ctrl: true }, | |
| { key: 'f', ctrl: true, shift: false }, |
— qwen3.7-max via Qwen Code /review
| { key: getExternalEditorKey(), description: t('for external editor') }, | ||
| { key: 'ctrl+o', description: t('to toggle compact mode') }, | ||
| { key: 'ctrl+f', description: t('to toggle model') }, | ||
| { key: 'alt+s', description: t('to toggle model') }, |
There was a problem hiding this comment.
[Critical] getShortcuts() now returns 15 entries, but COLUMN_SPLITS (line 62) still sums to 13 ([5,4,4] / [7,6] / [13]). The 2 new model toggle entries (indices 13–14) are silently dropped in every terminal width — they are never rendered in any layout.
Update COLUMN_SPLITS to accommodate 15 entries, e.g.:
| { key: 'alt+s', description: t('to toggle model') }, | |
| const COLUMN_SPLITS: Record<number, number[]> = { | |
| 3: [5, 5, 5], | |
| 2: [8, 7], | |
| 1: [15], | |
| }; |
Also consider adding the third binding (ctrl+shift+m) to the shortcuts list.
— qwen3.7-max via Qwen Code /review
| key.meta && key.name === 't', | ||
| [Command.TOGGLE_MODEL]: (key: Key) => | ||
| (key.meta && key.name === 's') || | ||
| (key.ctrl && !key.shift && key.name === 'f'), |
There was a problem hiding this comment.
[Suggestion] The reference implementation only covers 2 of 3 bindings — the third binding { key: 'm', ctrl: true, shift: true } (Ctrl+Shift+M) is missing from both the lambda and the positive test cases. The data-driven equivalence test therefore cannot detect a regression in Ctrl+Shift+M matching.
| (key.ctrl && !key.shift && key.name === 'f'), | |
| [Command.TOGGLE_MODEL]: (key: Key) => | |
| (key.meta && key.name === 's') || | |
| (key.ctrl && !key.shift && key.name === 'f') || | |
| (key.ctrl && key.shift && key.name === 'm'), |
Also add createKey('m', { ctrl: true, shift: true }) to positive cases and createKey('m', { ctrl: true }), createKey('m', { shift: true }) to negative cases.
— qwen3.7-max via Qwen Code /review
|
|
||
| // Wait for any response — the CLI should respond without errors | ||
| const hasResponse = await rig.waitForText('Hello', 15000); | ||
| expect( |
There was a problem hiding this comment.
[Suggestion] This assertion only checks that the CLI echoes "Hello" back — it does not verify that the model actually toggled. A complete no-op handler would pass this test.
Assert on the INFO messages the toggle handler emits:
| expect( | |
| // After first toggle | |
| const switchedToB = await rig.waitForText('Switched to model-b', 10000); | |
| expect(switchedToB, `Toggle to model-b did not occur. Output:\n${output}`).toBe(true); | |
| // Toggle back to original model (model-a) | |
| ptyProcess.write(CTRL_SHIFT_M); | |
| const switchedToA = await rig.waitForText('Switched to model-a', 10000); | |
| expect(switchedToA, `Toggle back to model-a did not occur. Output:\n${output}`).toBe(true); |
— qwen3.7-max via Qwen Code /review
| // ESLint requires it here because the callback calls settings.setValue(). | ||
| // debugKeystrokeLogging is read at call time, so no stale closure risk. | ||
| settings, | ||
| historyManager, |
There was a problem hiding this comment.
[Suggestion] Depending on historyManager (the whole object) contradicts the guidance in this same file (lines 414–417): "useHistory() returns a fresh memoized object whenever history changes". This causes handleGlobalKeypress to be re-created on every history mutation.
The toggle handler only calls historyManager.addItem(...), which is itself a stable useCallback reference. Depend on that instead:
| historyManager, | |
| historyManager.addItem, |
— qwen3.7-max via Qwen Code /review
| return; | ||
| } | ||
| const current = config.getModel(); | ||
| if (current === toggleModel) { |
There was a problem hiding this comment.
[Suggestion] When current === toggleModel and previousModelRef.current is null (e.g., user manually switched to the toggle model via /model), the hotkey silently does nothing — no info message, no feedback. The keypress is consumed (return at line 3414) so it also doesn't fall through.
Consider falling back to settings.merged.model?.name as the toggle target, or showing an info message:
if (previousModelRef.current) {
// ...existing toggle-back logic...
} else {
const fallback = settings.merged.model?.name;
if (fallback && fallback !== toggleModel) {
config.setModel(fallback).catch(() => {});
historyManager.addItem(
{ type: MessageType.INFO, text: '● Switched to ' + fallback },
Date.now(),
);
}
}— qwen3.7-max via Qwen Code /review
| return; | ||
| } | ||
|
|
||
| // Ctrl+Shift+M: toggle between current and alternate model |
There was a problem hiding this comment.
[Critical] Two issues in this toggle handler:
1. Ctrl+F dual-fire with text-buffer cursor-right: KeypressContext.broadcast() dispatches to ALL subscribers with no stop-propagation. text-buffer.ts:2663 still has else if (key.ctrl && key.name === 'f') move('right') — so pressing Ctrl+F both toggles the model AND moves the cursor one position right. The return here only exits this callback, not the text-buffer's. The PR description says "Ctrl+F replaces cursor-right" but the text-buffer binding was never removed.
2. setModel errors silently swallowed: Both config.setModel(...) calls use .catch(() => {}), but historyManager.addItem('● Switched to ...') fires synchronously before the promise resolves. If setModel rejects (invalid model, provider error), the user sees "Switched to X" but the model never changed — with zero logging to aid debugging.
Fix for (1): Remove or guard the Ctrl+F binding in text-buffer.ts:2663.
Fix for (2): Chain the info message into .then() and log/show an error in .catch():
config.setModel(toggleModel).then(() => {
previousModelRef.current = current;
historyManager.addItem({ type: MessageType.INFO, text: '● Switched to ' + toggleModel }, Date.now());
}).catch((err) => {
debugLogger.debug('[TOGGLE_MODEL] setModel failed:', err);
});— qwen3.7-max via Qwen Code /review
| category: 'Model', | ||
| requiresRestart: false, | ||
| default: undefined as string | undefined, | ||
| description: |
There was a problem hiding this comment.
[Suggestion] Description says "Ctrl+Shift+M hotkey" but the primary bindings are Ctrl+F / Alt+S (Ctrl+Shift+M is the third/legacy binding). Users who read this description won't know about the more accessible shortcuts. Consider: 'Alternate model ID for quick toggle via the model toggle hotkey (Ctrl+F / Alt+S). ...'
Same issue in settings.schema.json.
— qwen3.7-max via Qwen Code /review
| [Command.SCROLL_END]: (key: Key) => key.ctrl && key.name === 'end', | ||
| [Command.TOGGLE_THINKING_EXPANDED]: (key: Key) => | ||
| key.meta && key.name === 't', | ||
| [Command.TOGGLE_MODEL]: (key: Key) => |
There was a problem hiding this comment.
[Suggestion] This reference matcher only covers 2 of 3 default bindings — meta+s and ctrl+f. The third binding { key: 'm', ctrl: true, shift: true } (Ctrl+Shift+M) is absent from both the matcher and the positive test cases. A regression in Ctrl+Shift+M matching would go undetected by the "Data-driven key binding matches original logic" test.
| [Command.TOGGLE_MODEL]: (key: Key) => | |
| [Command.TOGGLE_MODEL]: (key: Key) => | |
| (key.meta && key.name === 's') || | |
| (key.ctrl && !key.shift && key.name === 'f') || | |
| (key.ctrl && key.shift && key.name === 'm'), |
Also add createKey('m', { ctrl: true, shift: true }) to the positive test cases.
— qwen3.7-max via Qwen Code /review
|
Addressed all 3 critical + 2 suggestion findings from CI bot review, plus removed the non-functional Ctrl+Shift+M binding. 🔴 Critical fixes
🟡 Suggestion fixes
🗑️ Removed Ctrl+Shift+M bindingFully removed from bindings and tests — unusable on standard terminals (Ctrl+M = Enter, Shift is ignored by the terminal). Files: keyBindings.ts, keyBindings.test.ts, AppContainer.tsx, KeyboardShortcuts.tsx, settingsSchema.ts QwenCode QwenLLM |
| } | ||
|
|
||
| // Alt+S / Ctrl+F: toggle between current and alternate model | ||
| if ( |
There was a problem hiding this comment.
[Critical] Ctrl+F collides with TOGGLE_SHELL_INPUT_FOCUS
When toggleModel is configured, this handler consumes Ctrl+F and returns early, making TOGGLE_SHELL_INPUT_FOCUS (bound to {key:'f', ctrl:true} at keyBindings.ts:241) unreachable at line ~3573. Users who configure toggleModel silently lose the embedded shell focus toggle. The PR description acknowledges losing cursor-right ("→ still works") but doesn't mention the shell focus regression.
Fix options:
- Drop the Ctrl+F binding — keep only Alt+S (no collision)
- Add a guard:
if (settings.merged.model?.toggleModel && !activePtyId && !embeddedShellFocused && keyMatchers[Command.TOGGLE_MODEL](key)) - Reorder handlers so TOGGLE_SHELL_INPUT_FOCUS has priority when a shell is focused
— qwen3.7-max via Qwen Code /review
| const CTRL_SHIFT_M = '\x1b[109;6u'; | ||
|
|
||
| // Toggle to toggleModel (model-b) | ||
| ptyProcess.write(CTRL_SHIFT_M); |
There was a problem hiding this comment.
[Critical] E2E test sends wrong key sequence — zero feature coverage
This sends \x1b[109;6u which is Ctrl+Shift+M (codepoint 109='m', mods 6=Shift+Ctrl). But the actual bindings are Ctrl+F ({key:'f', ctrl:true, shift:false}) and Alt+S ({key:'s', meta:true}). Ctrl+Shift+M matches neither binding — the toggle handler is never exercised.
The test only asserts the CLI doesn't crash, which is trivially true for any unbound key.
| ptyProcess.write(CTRL_SHIFT_M); | |
| // CSI-u format: ESC [ <code_point> ; <mods> u | |
| // code_point = 102 (lowercase 'f'), mods = 5 (base 1 + ctrl 4) | |
| const CTRL_F = '\x1b[102;5u'; | |
| // Toggle to toggleModel (model-b) | |
| ptyProcess.write(CTRL_F); |
Also update the test assertions to verify the model actually switched (e.g., check for "Switched to" in output) rather than just asserting the CLI is alive.
— qwen3.7-max via Qwen Code /review
| }, | ||
| Date.now(), | ||
| ); | ||
| previousModelRef.current = null; |
There was a problem hiding this comment.
[Critical] previousModelRef cleared before async setModel resolves
config.setModel(previousModelRef.current).catch(() => {}) is fire-and-forget, but previousModelRef.current = null executes synchronously right after. If setModel rejects (model removed from registry, auth expired, provider unreachable), the ref is already lost — the user can never return to their original model via the hotkey.
| previousModelRef.current = null; | |
| try { | |
| await config.setModel(previousModelRef.current); | |
| historyManager.addItem( | |
| { | |
| type: MessageType.INFO, | |
| text: '● Switched to ' + previousModelRef.current, | |
| }, | |
| Date.now(), | |
| ); | |
| previousModelRef.current = null; | |
| } catch (err) { | |
| historyManager.addItem( | |
| { | |
| type: MessageType.INFO, | |
| text: '● Failed to switch to ' + previousModelRef.current, | |
| }, | |
| Date.now(), | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| } | ||
| } else { | ||
| previousModelRef.current = current; | ||
| config.setModel(toggleModel).catch(() => {}); |
There was a problem hiding this comment.
[Critical] Silent error swallowing shows false success message
Both toggle paths use .catch(() => {}) while pushing "● Switched to X" to history unconditionally. setModel can reject — core has explicit rollback-and-rethrow logic (modelsConfig.ts:398). The user sees a success banner while the model never changed.
This is especially concerning when a user configures toggleModel for cost/privacy reasons (e.g., switching to a local model). They believe they're on the local model but the cloud model is still active.
Fix: Replace .catch(() => {}) with error reporting:
config.setModel(toggleModel).catch((err) => {
historyManager.addItem(
{ type: MessageType.INFO, text: '● Failed to switch model: ' + (err?.message ?? err) },
Date.now(),
);
});— qwen3.7-max via Qwen Code /review
| "type": "string" | ||
| }, | ||
| "toggleModel": { | ||
| "description": "Alternate model ID for quick toggle via Ctrl+Shift+M hotkey. When set, pressing the hotkey switches between the current model and this model. Leave empty to disable the hotkey.", |
There was a problem hiding this comment.
[Suggestion] Stale hotkey reference in VS Code schema
Description says "Ctrl+Shift+M hotkey" but the actual bindings are Alt+S / Ctrl+F. The TypeScript schema (settingsSchema.ts:1325) was correctly updated to say "Alt+S / Ctrl+F", but this JSON schema was missed.
| "description": "Alternate model ID for quick toggle via Ctrl+Shift+M hotkey. When set, pressing the hotkey switches between the current model and this model. Leave empty to disable the hotkey.", | |
| "toggleModel": { | |
| "description": "Alternate model ID for quick toggle via Alt+S / Ctrl+F hotkey. When set, pressing the hotkey switches between the current model and this model. Leave empty to disable the hotkey.", | |
| "type": "string" | |
| }, |
— qwen3.7-max via Qwen Code /review
| // Alt+S / Ctrl+F: toggle between current and alternate model | ||
| if ( | ||
| settings.merged.model?.toggleModel && | ||
| keyMatchers[Command.TOGGLE_MODEL](key) |
There was a problem hiding this comment.
[Critical] Toggle handler has no unit tests for its 5 branches
The handler contains 5 distinct code paths: (1) toggle to alternate model, (2) toggle back to previous, (3) already-on-toggleModel with no previous ref, (4) toggleModel not configured, (5) key mismatch. None are directly tested.
The integration test (model-toggle-hotkey.test.ts) sends the wrong key sequence (Ctrl+Shift+M instead of Ctrl+F/Alt+S) and never triggers the handler. Regressions in any branch will go undetected.
Suggested approach: Extract the toggle logic into a pure function (e.g., computeToggleResult(current, toggleModel, previousModel) => { nextModel, newPrevious, message }) and unit test it. Alternatively, mock config.setModel/config.getModel/historyManager.addItem and exercise each branch via synthetic key events.
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Suggestion] docs/users/reference/keyboard-shortcuts.md — Ctrl+F is now documented with two conflicting meanings. The General section (line 19, added by this PR) says "Ctrl+F / Alt+S → toggle model, Ctrl+F replaces cursor-right." But the Input Prompt section (line 43, unchanged) still says "Ctrl+F / Right Arrow → Move cursor one character to the right." Drop Ctrl+F from line 43, leaving only Right Arrow.
— qwen3.7-max via Qwen Code /review
|
Second round: resolved Ctrl+F collisions and addressed crosscut trace feedback. ✅ Fixed since last reviewCtrl+F dual-fire + collision fixes:
Crosscut trace — all stale references updated:
⏸️ Deferred (conscious decisions)
Files: 8 changed, +16/-23 QwenCode QwenLLM |
| | `Ctrl+B` | While a foreground shell command is running: promote it to a background task. The child keeps running, the agent's turn unblocks, and the shell appears in `/tasks` + the Background tasks dialog. No-op when no shell is executing — Ctrl+B then falls through to its prompt-area binding (cursor-left). | | ||
| | `Alt/Option+M` | Toggle Markdown output between rich rendered previews and raw/source mode. On macOS, the terminal must send Option as Meta. | | ||
| | `Shift+Tab` (`Tab` on Windows) | Cycle approval modes (`plan` → `default` → `auto-edit` → `auto` → `yolo`) | | ||
| | `Ctrl+F` | Toggle between the current model and the alternate model set in `model.toggleModel`. Requires `model.toggleModel` to be configured in settings. Only active when no shell is focused. | |
There was a problem hiding this comment.
[Critical] Ctrl+F is documented with two conflicting meanings in this file. The General section (this line) says "Ctrl+F → toggle model", but the Input Prompt section (line 39) still says "Ctrl+F / Right Arrow → move cursor right". Users reading both sections will be confused.
Consider following the Ctrl+B pattern, which documents both behaviors: "promote to background task ... No-op when no shell is executing — Ctrl+B then falls through to its prompt-area binding (cursor-left)." E.g.:
Toggle between the current model and the alternate model set in
model.toggleModel. Requiresmodel.toggleModelto be configured. When not configured, Ctrl+F falls through to its input-prompt binding (cursor-right).
— qwen3.7-max via Qwen Code /review
| { key: getPasteKey(), description: t('to paste images') }, | ||
| { key: getExternalEditorKey(), description: t('for external editor') }, | ||
| { key: 'ctrl+o', description: t('to toggle compact mode') }, | ||
| { key: 'ctrl+f', description: t('to toggle model') }, |
There was a problem hiding this comment.
[Suggestion] This shortcut is always displayed, but model.toggleModel defaults to undefined (disabled). Most users will see a "ctrl+f to toggle model" hint that does nothing when pressed. Consider conditionally including this entry only when the toggle model setting is configured.
— qwen3.7-max via Qwen Code /review
dfab20a to
af4f33b
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
| if ( | ||
| settings.merged.model?.toggleModel && | ||
| !activePtyId && | ||
| !embeddedShellFocused && |
There was a problem hiding this comment.
[Critical] Missing streamingState guard — model can be toggled mid-stream
The toggle handler does not check streamingState. Pressing Ctrl+F while the LLM is actively streaming calls config.setModel(), which mutates contentGeneratorConfig.model in place. The in-flight response was started by the old model, but any continuation request (e.g., after a tool call result returns) reads the new model from the config. This produces a hybrid turn where the first API call uses model A and the continuation uses model B — with potentially different context windows, modalities, and tool-calling capabilities.
Other handlers in this callback correctly guard on streaming state (e.g., Escape at line 3523: streamingState === StreamingState.Responding && !dialogsVisibleRef.current).
| !embeddedShellFocused && | |
| if ( | |
| settings.merged.model?.toggleModel && | |
| !activePtyId && | |
| !embeddedShellFocused && | |
| streamingState === StreamingState.Idle && | |
| keyMatchers[Command.TOGGLE_MODEL](key) | |
| ) { |
— qwen3.7-max via Qwen Code /review
| Date.now(), | ||
| ); | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
[Critical] .catch() in toggle-back path doesn't reset previousModelRef — unrecoverable failure loop
When config.setModel(prevModel) fails, this .catch() handler does not clear previousModelRef.current. The else branch's .catch() at line 3440 correctly clears previousModelRef.current = null, but the reverse path omits this. After a failed toggle-back, the ref retains the stale value, causing every subsequent Ctrl+F press to re-enter this same failing branch with no recovery path other than restarting.
| .catch(() => { | |
| .catch(() => { | |
| previousModelRef.current = null; | |
| historyManager.addItem( | |
| { | |
| type: MessageType.ERROR, | |
| text: '⚠ Failed to switch to ' + prevModel, | |
| }, | |
| Date.now(), | |
| ); | |
| }); |
— qwen3.7-max via Qwen Code /review
| if (previousModelRef.current) { | ||
| const prevModel = previousModelRef.current; | ||
| config | ||
| .setModel(prevModel) |
There was a problem hiding this comment.
[Suggestion] Missing ModelSwitchMetadata in setModel calls — hotkey switches invisible in telemetry
Both config.setModel() calls in the toggle handler omit the second ModelSwitchMetadata argument ({ reason?: string }). Without it, hotkey-triggered model switches are indistinguishable from other programmatic switches (quota-error auto-switch, slash-command, ACP session) in telemetry and audit trails.
| .setModel(prevModel) | |
| config | |
| .setModel(prevModel, { reason: 'toggle-hotkey' }) |
And similarly for the other setModel call (~line 3429):
| .setModel(prevModel) | |
| config | |
| .setModel(toggleModel, { reason: 'toggle-hotkey' }) |
— qwen3.7-max via Qwen Code /review
| return; | ||
| } | ||
|
|
||
| // Ctrl+F: toggle between current and alternate model |
There was a problem hiding this comment.
[Suggestion] No diagnostic logging in toggle handler
The toggle handler has no debugLogger.debug() calls. Neighboring handlers in this callback (e.g., Ctrl+B) use debug logging for post-mortem analysis. When the toggle misbehaves, the debug log has zero entries to correlate with the failure — no record of which model was active, which model was targeted, or what previousModelRef held.
| // Ctrl+F: toggle between current and alternate model | |
| // Ctrl+F: toggle between current and alternate model | |
| if ( | |
| settings.merged.model?.toggleModel && | |
| !activePtyId && | |
| !embeddedShellFocused && | |
| keyMatchers[Command.TOGGLE_MODEL](key) | |
| ) { | |
| const toggleModel = settings.merged.model.toggleModel; | |
| const current = config.getModel(); | |
| debugLogger.debug(`Model toggle: current=${current} target=${toggleModel} prev=${previousModelRef.current}`); |
— qwen3.7-max via Qwen Code /review
|
... holyshit, Guys, add a "proper" autoreview skill to the repository so that you can review it on the fork before the commit ))) |
|
@qwen-code /resolve |
|
Qwen Code attempted to resolve merge conflicts but the run did not complete successfully. address-summary.mdMerge Conflict Resolution Summary — PR #6486Branch: ConflictsThe PR adds a model toggle hotkey (Ctrl+F / 1.
|
| // Thinking expansion | ||
| [Command.TOGGLE_THINKING_EXPANDED]: [{ key: 't', meta: true }], | ||
|
|
||
| // Model toggle |
There was a problem hiding this comment.
[Critical] Alt+S claimed in PR title/description but not implemented
The PR title says "Alt+S / Ctrl+F" but only Ctrl+F is defined here. No Alt+S binding exists anywhere in the diff. The KeyBinding interface has no alt field — Alt+S would need { key: 's', meta: true }.
| // Model toggle | |
| [Command.TOGGLE_MODEL]: [ | |
| { key: 'f', ctrl: true, shift: false }, | |
| { key: 's', meta: true }, | |
| ], |
Or remove Alt+S from the PR title/description and update the settingsSchema.ts description which also references it.
— qwen3.7-max via Qwen Code /review
|
|
||
| // Ctrl+F: toggle between current and alternate model | ||
| if ( | ||
| settings.merged.model?.toggleModel && |
There was a problem hiding this comment.
[Suggestion] Toggle handler missing dialogsVisibleRef guard
Other handlers in this callback (lines 3472, 3492, 3523, 3537, 3563) guard on !dialogsVisibleRef.current. When a dialog is open (e.g., ModelDialog via /model), pressing Ctrl+F silently toggles the model behind the dialog with no user feedback.
| settings.merged.model?.toggleModel && | |
| if ( | |
| settings.merged.model?.toggleModel && | |
| !activePtyId && | |
| !embeddedShellFocused && | |
| !dialogsVisibleRef.current && | |
| keyMatchers[Command.TOGGLE_MODEL](key) | |
| ) { |
— qwen3.7-max via Qwen Code /review
| }, | ||
| Date.now(), | ||
| ); | ||
| }) |
There was a problem hiding this comment.
[Suggestion] Error messages don't tell the user what model they're on after failure
Both .catch() blocks show "Failed to switch to X" but discard the error object and never call config.getModel(). After a failed switch, the user doesn't know what model is active.
| }) | |
| .catch(() => { | |
| const actual = config.getModel(); | |
| historyManager.addItem( | |
| { | |
| type: MessageType.ERROR, | |
| text: '⚠ Failed to switch to ' + prevModel + ' (currently on ' + actual + ')', | |
| }, | |
| Date.now(), | |
| ); | |
| }); |
— qwen3.7-max via Qwen Code /review
| // this event handler into a single commit, so the render-phase reset | ||
| // and the Static remount happen together — no full-history flash. | ||
| const lastNotifiedModelRef = useRef(currentModel); | ||
| const previousModelRef = useRef<string | null>(null); |
There was a problem hiding this comment.
[Suggestion] previousModelRef state machine has 5 coordination points with no consolidated documentation
The toggle-back protocol is spread across: (1) set in toggle handler when switching to toggleModel, (2) cleared in toggle-back .then(), (3-4) cleared in two .catch() paths, (5) cleared in onModelChange when a non-toggle model is detected. Any future maintainer adding a model-switch path must independently discover all 5 locations.
Consider adding a // PROTOCOL: comment here documenting the full state machine, or extracting into a dedicated hook.
— qwen3.7-max via Qwen Code /review
| command: Command.TOGGLE_MODEL, | ||
| positive: [createKey('f', { ctrl: true })], | ||
| negative: [ | ||
| createKey('f'), |
There was a problem hiding this comment.
[Suggestion] Missing Ctrl+Shift+F negative test case
The existing createKey('f', { shift: true }) creates {ctrl: false, shift: true} — it fails because ctrl is missing, not because of the shift guard. The critical test — that Ctrl+Shift+F (which is TOGGLE_SHELL_INPUT_FOCUS) does NOT match TOGGLE_MODEL — is absent.
| createKey('f'), | |
| negative: [ | |
| createKey('f'), | |
| createKey('f', { ctrl: true, shift: true }), | |
| createKey('f', { meta: true }), | |
| createKey('s'), | |
| createKey('s', { meta: true }), | |
| createKey('s', { ctrl: true }), | |
| ], |
— qwen3.7-max via Qwen Code /review
| it('should not crash when Ctrl+F hotkey is pressed', async () => { | ||
| const { ptyProcess } = rig.runInteractive(); | ||
|
|
||
| let output = ''; |
There was a problem hiding this comment.
[Suggestion] E2E test never terminates the PTY process
runInteractive() returns { ptyProcess, promise } but only ptyProcess is destructured. The CLI process is never stopped — rig.cleanup() only deletes the test directory. Orphaned PTY processes accumulate during test runs.
| let output = ''; | |
| const { ptyProcess, promise } = rig.runInteractive(); |
Then at the end of the test, gracefully exit:
ptyProcess.write('\x03'); // Ctrl+C
await new Promise(r => setTimeout(r, 500));
ptyProcess.write('\x03'); // Ctrl+C again
await promise;— 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.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/cli/src/ui/AppContainer.tsx:3618-3625 |
Dead code in toggle-back .then() — onModelChange clears previousModelRef synchronously before .then() runs, making the .then() clear redundant |
Add comment: // Defensive: onModelChange already clears this in the success path or remove the .then() clear |
packages/cli/src/ui/AppContainer.tsx:3605 + :3833 |
Implicit Ctrl+F fallthrough to TOGGLE_SHELL_INPUT_FOCUS — toggle handler guards are 230 lines apart from the fallthrough target with no cross-reference |
Add cross-reference comment documenting the fallthrough coupling |
packages/cli/src/ui/components/KeyboardShortcuts.tsx:60 |
Stale comment — says 5+4+4 / 7+6 but actual values are [5,5,4] / [7,7] / [14] |
Update comment to match actual values |
— qwen3.7-max via Qwen Code /review
|
Both suggestions analyzed via full consumer trace. Declining both: Suggestion 1 (preserve Suggestion 2 (clear QwenCode QwenLLM |
Aleks-0
changed the title
| return; | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
[Critical] Orphan return; } from merge artifact — TypeScript compilation fails
These two lines (3605–3606) have no matching if block. They appear to be a merge/rebase artifact — the closing fragment of a removed guard condition. TypeScript reports TS1005 at line 3609 and cascading TS1128 errors.
| return; |
Remove both orphaned lines ( return; and }) to restore compilation.
— 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.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
Re-review (2026-07-11) — Code Review + Testing2a. Code ReviewIndependent proposal: Same as original triage — I'd add a Findings on current diff (commit
No critical blockers. Two minor issues above. 2b. Real-Scenario TestingUnit tests: All pass ✅
Interactive test (tmux): CLI starts cleanly on the PR branch. Sent Output identical before and after Ctrl+F — the unconfigured-toggleModel guard works correctly. Limitation: Full toggle verification (actual model switch + info message) requires — 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.
Reviewed. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
— qwen3.8-max-preview via Qwen Code /review
b1737c7 to
69ae108
Compare
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite (the only end-to-end coverage of the new hotkey, integration-tests/interactive/model-toggle-hotkey.test.ts) did not run locally either.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite (the only end-to-end coverage of the new hotkey, integration-tests/interactive/model-toggle-hotkey.test.ts) did not run locally either。
— qwen3.8-max-preview via Qwen Code /review
| getLiveOutputs: () => new Map(), | ||
| getShellPids: () => shellPids, | ||
| }; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The makeCore mock is missing runtimeContext and modelConfig. AgentChatContent reads core.runtimeContext.getTargetDir() (line 206) and core.modelConfig.model (line 211) unconditionally during render, so the component throws after the hooks run but before the JSX return. The test passes only because useKeypress is mocked and the test asserts on the captured handler, never on rendered output. — Concrete cost: if a refactor moves core.runtimeContext.getTargetDir() above the useKeypress call (or adds an early guard that skips the hook), this test breaks with a confusing "useKeypress was not called with a handler" error instead of a clear mock-completeness failure.
| getLiveOutputs: () => new Map(), | |
| getShellPids: () => shellPids, | |
| }; | |
| } | |
| getLiveOutputs: () => new Map(), | |
| getShellPids: () => shellPids, | |
| runtimeContext: { getTargetDir: () => '/tmp/test' }, | |
| modelConfig: { model: 'test-model' }, | |
| }; | |
| } |
中文说明
makeCore mock 缺少 runtimeContext 和 modelConfig。AgentChatContent 在渲染期间会无条件读取 core.runtimeContext.getTargetDir()（第 206 行）和 core.modelConfig.model（第 211 行），因此组件会在 hooks 运行之后、JSX 返回之前抛错。测试之所以通过，只是因为 useKeypress 被 mock 了，且测试只对捕获到的 handler 断言，从不对渲染输出断言。
具体代价： 如果某次重构把 core.runtimeContext.getTargetDir() 移到 useKeypress 调用之上（或新增一个会跳过该 hook 的提前返回守卫），这个测试会以一个令人困惑的 "useKeypress was not called with a handler" 错误失败，而不是一个清晰的 mock 不完整失败。
— qwen3.8-max-preview via Qwen Code /review
| requiresRestart: false, | ||
| default: undefined as string | undefined, | ||
| description: | ||
| 'Alternate model ID for quick toggle via the toggleModel hotkey. When set, pressing the configured hotkey switches between the current model and this model. Use the qualified syntax "model-id(authType)" (e.g. "gpt-4(openai)") to disambiguate when multiple providers offer the same model ID; without qualification the first provider in enum order wins. Leave empty to disable the hotkey.', |
There was a problem hiding this comment.
[Suggestion] The toggleModel description says "without qualification the first provider in enum order wins", but resolveToggleTarget (packages/cli/src/ui/resolve-toggle-model.ts:63-72) checks the current provider first and only falls through to enum-order iteration among other providers. The function's own JSDoc documents the correct order ("2. The current auth type, when it already owns the model id"); the user-facing description contradicts it. — Concrete cost: a user whose current provider (e.g. qwen-oauth) and an earlier-in-enum-order provider (e.g. openai) both offer model "X" reads this in the settings UI and expects the unqualified toggle to switch to openai; in reality the current provider wins, so the toggle stays on qwen-oauth — the user adds an unnecessary (openai) qualifier or files a "toggle doesn't switch provider" bug. The same inaccurate wording also appears in packages/vscode-ide-companion/schemas/settings.schema.json (commented there separately).
Suggested fix: replace the clause without qualification the first provider in enum order wins with without qualification the current provider is preferred; if it does not offer the model, the first other provider in enum order wins.
中文说明
toggleModel 的描述写着 "without qualification the first provider in enum order wins"（不加限定符时按枚举顺序取第一个 provider），但 resolveToggleTarget（packages/cli/src/ui/resolve-toggle-model.ts:63-72）会先检查当前 provider，只有在其他 provider 中才按枚举顺序回退。该函数自己的 JSDoc 记录了正确的顺序（"2. The current auth type, when it already owns the model id"）；面向用户的描述与之矛盾。
具体代价： 如果用户当前 provider（如 qwen-oauth）和枚举序更靠前的 provider（如 openai）都提供模型 "X"，用户在设置界面读到这段描述后会以为不加限定符的切换会切到 openai；实际上当前 provider 优先，切换仍停留在 qwen-oauth——用户会多加一个不必要的 (openai) 限定符，或提交一个 "切换没有换 provider" 的 bug。同样的不准确措辞也出现在 packages/vscode-ide-companion/schemas/settings.schema.json（已在那里单独评论）。
建议修复：把子句 without qualification the first provider in enum order wins 替换为 without qualification the current provider is preferred; if it does not offer the model, the first other provider in enum order wins。
— qwen3.8-max-preview via Qwen Code /review
| "type": "string" | ||
| }, | ||
| "toggleModel": { | ||
| "description": "Alternate model ID for quick toggle via the toggleModel hotkey. When set, pressing the configured hotkey switches between the current model and this model. Use the qualified syntax \"model-id(authType)\" (e.g. \"gpt-4(openai)\") to disambiguate when multiple providers offer the same model ID; without qualification the first provider in enum order wins. Leave empty to disable the hotkey.", |
There was a problem hiding this comment.
[Suggestion] Same inaccurate wording as packages/cli/src/config/settingsSchema.ts (commented there): the description says "without qualification the first provider in enum order wins", but resolveToggleTarget (packages/cli/src/ui/resolve-toggle-model.ts:63-72) prefers the current provider first, then enum order among other providers. — Concrete cost: a user whose current provider and an earlier-in-enum-order provider both offer the same model ID reads this in the VS Code settings UI and expects the unqualified toggle to switch provider; in reality the current provider wins, so the toggle stays put — leading to an unnecessary qualifier or a "toggle doesn't switch provider" bug report.
Suggested fix: replace the clause without qualification the first provider in enum order wins with without qualification the current provider is preferred; if it does not offer the model, the first other provider in enum order wins.
中文说明
与 packages/cli/src/config/settingsSchema.ts 中的不准确措辞相同（已在那里评论）：描述写着 "without qualification the first provider in enum order wins"，但 resolveToggleTarget（packages/cli/src/ui/resolve-toggle-model.ts:63-72）会先优先当前 provider，然后在其他 provider 中按枚举顺序回退。
具体代价： 如果用户当前 provider 和枚举序更靠前的 provider 都提供同一个模型 ID，用户在 VS Code 设置界面读到这段描述后会以为不加限定符的切换会换 provider；实际上当前 provider 优先，切换原地不动——导致多加一个不必要的限定符，或一个 "切换没有换 provider" 的 bug 报告。
建议修复：把子句 without qualification the first provider in enum order wins 替换为 without qualification the current provider is preferred; if it does not offer the model, the first other provider in enum order wins。
— qwen3.8-max-preview via Qwen Code /review
| // Ctrl+F: toggle between current and alternate model | ||
| if (handleToggleKeypress(key)) { |
There was a problem hiding this comment.
[Suggestion] The ~100-line toggle handler glue (resolve → compute → switchModel → ref updates → history messages → error handling) has no unit test; the integration test covers only the happy path. The pure helpers (resolveToggleTarget, computeToggleAction, needsCachedCredentials) are well tested, so the gap is confined to this glue. — Concrete cost: the switchModel-rejection path (which clears previousModelRef and posts the error message) is verified by no test; if the previousModelRef.current = null line in the error handler were lost, a failed toggle would leave a stale ref and the next Ctrl+F would run a backward toggle to a model the user never successfully toggled from — and no test would go red.
Suggested fix: add an AppContainer.test.tsx case that mocks config.switchModel to reject, sends Ctrl+F, and asserts (a) an error message is posted, (b) previousModelRef is cleared so the next toggle goes forward, and (c) isTogglingRef is reset so the guard does not permanently block.
中文说明
约 100 行的切换处理「胶水」代码（resolve → compute → switchModel → ref 更新 → 历史消息 → 错误处理）没有单元测试；集成测试只覆盖了 happy path。纯函数辅助（resolveToggleTarget、computeToggleAction、needsCachedCredentials）测试充分，缺口仅在这段胶水代码。
具体代价： switchModel 拒绝路径（会清空 previousModelRef 并发出错误消息）没有任何测试验证；如果错误处理里的 previousModelRef.current = null 这一行丢失，失败的切换会留下一个过期的 ref，下一次 Ctrl+F 会执行一次 backward 切换到一个用户从未成功切换过去的模型——而没有任何测试会变红。
建议修复：新增一个 AppContainer.test.tsx 用例，mock config.switchModel 使其 reject，发送 Ctrl+F，并断言 (a) 发出了错误消息，(b) previousModelRef 被清空使下一次切换为 forward，(c) isTogglingRef 被重置使守卫不会永久阻塞。
— qwen3.8-max-preview via Qwen Code /review
- Complete makeCore mock with runtimeContext and modelConfig in AgentChatContent tests - Correct toggleModel description: current provider is preferred before enum-order fallback (settingsSchema + VS Code schema) - Add AppContainer test covering switchModel rejection path (error message + guard re-arm)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm: [Critical] integration-tests/interactive/model-toggle-hotkey.test.ts:45 (comment 3576700771) — E2E test uses synthetic model IDs (model-a/model-b via modelProviders config); whether the model registry resolves them at runtime cannot be confirmed from static analysis. Test-only; the feature itself has maintainer real-run verification, but this suite did not run in this review.
中文说明
未决，请确认：[Critical] integration-tests/interactive/model-toggle-hotkey.test.ts:45 (comment 3576700771) — E2E test uses synthetic model IDs (model-a/model-b via modelProviders config); whether the model registry resolves them at runtime cannot be confirmed from static analysis. Test-only; the feature itself has maintainer real-run verification, but this suite did not run in this review.
— qwen3.8-max-preview via Qwen Code /review
| agentViewActiveShellPtyRef.current = | ||
| agentViewState.activeView !== 'main' && |
There was a problem hiding this comment.
[Critical] agentViewHasActiveShellPty is a global "any agent has a PTY" boolean (AgentViewContext.tsx:145: agentViewActiveShellPtySet.size > 0), but here it is conjoined with activeView !== 'main', so the model toggle is blocked whenever the user is on any agent tab while any agent has a PTY — even when the currently viewed agent has none. This contradicts the comment directly above ("Block the model toggle only when the user is viewing an agent tab that has an active embedded-shell PTY … A background agent's PTY must not block the toggle"). — Failure scenario: agents A and B; B spawns an embedded shell (gets a PTY); the user switches to A's tab (no PTY) and presses Ctrl+F with toggleModel configured. canToggleModel reads this ref = ('agent-A' !== 'main') && (set.size > 0) = true → returns false; AgentChatContent's local Ctrl+F branch is also a no-op (no PTY, not focused). Ctrl+F does nothing — the toggle should have fired. This confirms the still-open existing blocker (comment 3643679617), verified against the current commit.
Fix: expose the per-agent information (the Set is already keyed by the same agentId that activeView holds) and use it here instead of the global boolean, e.g.:
// in AgentViewProvider, expose a derived per-active-agent flag:
const activeAgentHasShellPty = agentViewActiveShellPtySet.has(activeView);
// ...and here:
agentViewActiveShellPtyRef.current =
agentViewState.activeView !== 'main' && agentViewState.activeAgentHasShellPty;中文说明
agentViewHasActiveShellPty 是一个全局的“任意 agent 有 PTY”布尔值（AgentViewContext.tsx:145：agentViewActiveShellPtySet.size > 0），但这里它与 activeView !== 'main' 组合，导致只要用户位于任意 agent 标签页、且任意 agent 有 PTY，模型切换就会被阻止——即使当前查看的 agent 并没有 PTY。这正上方的注释相矛盾（“仅当用户正在查看一个拥有活动内嵌 shell PTY 的 agent 标签页时才阻止模型切换……后台 agent 的 PTY 不应阻止切换”）。— 失败场景：agent A 与 B；B 启动了内嵌 shell（获得 PTY）；用户切换到 A 的标签页（无 PTY）并在已配置 toggleModel 时按 Ctrl+F。canToggleModel 读到此 ref = ('agent-A' !== 'main') && (set.size > 0) = true → 返回 false；AgentChatContent 的本地 Ctrl+F 分支同样是空操作（无 PTY、未聚焦）。Ctrl+F 毫无反应——本应触发切换。这确认了仍然未解决的现有阻塞问题（评论 3643679617），已在当前提交上验证。
修复：暴露按 agent 的信息（该 Set 已经以与 activeView 相同的 agentId 为键），并在此处用它替代全局布尔值，例如在 AgentViewProvider 中暴露派生的 activeAgentHasShellPty = agentViewActiveShellPtySet.has(activeView)，并在此处使用 agentViewState.activeView !== 'main' && agentViewState.activeAgentHasShellPty。
— qwen3.8-max-preview via Qwen Code /review
| expect(handleKeypress!.toString()).toContain( | ||
| 'previousModelRef.current = null', | ||
| ); |
There was a problem hiding this comment.
[Suggestion] This assertion inspects the keypress handler's source text (expect(handleKeypress!.toString()).toContain('previousModelRef.current = null')) rather than the behavior it describes. — Concrete cost: if the toggle error-handling logic is extracted into a helper in a future refactor, the outer handler's .toString() no longer contains that literal string even though behavior is unchanged, so the test fails on a valid, behavior-preserving refactor. (The getGlobalKeypress() locator near line 5000 has the same source-text dependency.) Prefer a behavioral assertion: after the failed toggle, verify the second switchModel call receives the forward-switch arguments, proving previousModelRef was cleared — e.g. expect(switchModelSpy).toHaveBeenNthCalledWith(2, AuthType.USE_OPENAI, 'model-b', expect.anything()).
中文说明
此断言检查的是按键处理器的源代码文本（expect(handleKeypress!.toString()).toContain('previousModelRef.current = null')），而非其所描述的行为。— 具体代价：如果未来重构将切换的错误处理逻辑抽取到一个辅助函数中，外层处理器的 .toString() 将不再包含该字面字符串，即使行为未变，测试也会在一次有效的、保持行为的重构上失败。（约第 5000 行的 getGlobalKeypress() 定位器有同样的源代码文本依赖。）建议改用行为断言：在失败的切换之后，验证第二次 switchModel 调用收到正向切换参数，从而证明 previousModelRef 已被清除——例如 expect(switchModelSpy).toHaveBeenNthCalledWith(2, AuthType.USE_OPENAI, 'model-b', expect.anything())。
— qwen3.8-max-preview via Qwen Code /review
| const switchOptions = needsCachedCredentials( | ||
| switchTo.authType, | ||
| currentAuthType!, | ||
| ) | ||
| ? { requireCachedCredentials: true } | ||
| : undefined; |
There was a problem hiding this comment.
[Suggestion] The wiring that turns needsCachedCredentials(...) into the switchModel third argument is exercised by no test at any level. The pure helper is tested in resolve-toggle-model.test.ts, but nothing asserts AppContainer actually passes its result through. — Concrete cost: the only two tests reaching this glue both use a single provider, so switchOptions is undefined in both; a mutation deleting the third argument from config.switchModel(switchTo.authType, switchTo.modelId, switchOptions) leaves every test green. A cross-provider toggle into qwen-oauth would then lose the requireCachedCredentials: true flag that the /model command and ModelDialog pass for the same kind of switch, risking an unexpected auth prompt mid-toggle. Add an AppContainer unit case for a cross-provider toggle into AuthType.QWEN_OAUTH asserting switchModel is called with { requireCachedCredentials: true } as its third argument (and undefined for the same-provider case already covered).
中文说明
将 needsCachedCredentials(...) 转换为 switchModel 第三个参数的这段接线，在任何层级的测试中都未被覆盖。该纯函数辅助方法在 resolve-toggle-model.test.ts 中有测试，但没有任何测试断言 AppContainer 确实将其结果传递下去。— 具体代价：唯一到达这段接线的两个测试都使用单一 provider，因此两种情况下 switchOptions 都是 undefined；若变异删除 config.switchModel(switchTo.authType, switchTo.modelId, switchOptions) 的第三个参数，所有测试仍为绿色。这样，跨 provider 切换到 qwen-oauth 时会丢失 /model 命令和 ModelDialog 在同类切换中传递的 requireCachedCredentials: true 标志，可能在切换中途触发意外的认证提示。建议为跨 provider 切换到 AuthType.QWEN_OAUTH 添加一个 AppContainer 单元测试，断言 switchModel 的第三个参数被以 { requireCachedCredentials: true } 调用（已覆盖的同一 provider 情况则为 undefined）。
— qwen3.8-max-preview via Qwen Code /review
| if (handleToggleKeypress(key)) { | ||
| return true; | ||
| } | ||
| return vimHandleInput(key); |
There was a problem hiding this comment.
[Suggestion] This vim-mode Ctrl+F cursor-right suppression (the vimHandleInput wrapper) has no real test coverage. The test that claims to cover it (can-toggle-model.test.ts, describe('vimHandleInput cursor-right suppression')) re-implements the wrapper as a local lambda and asserts against that copy — it never imports or invokes AppContainer's real wrapper; every other vimHandleInput reference in the suite is a prop mock. — Concrete cost: deleting these three guard lines leaves all 347 Ctrl+F-related tests green (verified by a mutation probe), so the regression they prevent — in vim mode a single Ctrl+F press firing the model toggle and moving the text-buffer cursor right on the same keypress — would ship uncaught. Add a test that exercises the real vimHandleInput exposed via UIActions: render in vim mode with the toggle guard passing, press Ctrl+F, and assert the underlying vimHandleInput is NOT called while the toggle fires (or rename the can-toggle-model.test.ts block so it no longer implies coverage of the production wrapper).
中文说明
这个 vim 模式下对 Ctrl+F 光标右移的抑制（vimHandleInput 包装器）没有真正的测试覆盖。声称覆盖它的测试（can-toggle-model.test.ts 中的 describe('vimHandleInput cursor-right suppression')）将该包装器重新实现为一个局部 lambda 并对该副本断言——它从未导入或调用 AppContainer 的真实包装器；测试套件中所有其他 vimHandleInput 引用都是 prop mock。— 具体代价：删除这三行守卫代码后，全部 347 个与 Ctrl+F 相关的测试仍为绿色（已通过变异探针验证），因此它们所防止的回归——在 vim 模式下按一次 Ctrl+F 既触发模型切换又将文本缓冲区光标右移——将会未被发现地发布。建议添加一个测试来验证通过 UIActions 暴露的真实 vimHandleInput：在 vim 模式下且切换守卫通过时渲染，按下 Ctrl+F，断言底层的 vimHandleInput 未被调用、同时切换被触发（或将 can-toggle-model.test.ts 中的该代码块改名，使其不再暗示覆盖了生产包装器）。
— qwen3.8-max-preview via Qwen Code /review
The toggle guard keyed off the global 'any agent has a PTY' boolean, so a background agent's embedded shell blocked Ctrl+F even while viewing a different agent tab (or main) with no PTY. Expose a per-active-agent flag (activeAgentHasShellPty) and use it in the guard. Also addresses review suggestions: behavioral assertion for the toggle error path, and tests for the needsCachedCredentials wiring and the vim-mode cursor-right suppression.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Unresolved, please confirm: [Critical] integration-tests/interactive/model-toggle-hotkey.test.ts:45 (comment 3576700771) — existing [Critical] that the E2E test's synthetic model IDs (model-a/model-b via modelProviders) resolve in the model registry at runtime could not be confirmed from static analysis; the suite is unreachable by the per-workspace unit command and did not run in this review (test-only; the feature has maintainer real-run verification). Ruled cannot tell.
中文说明
已审查。 建议见行内评论。 未决，请确认：[Critical] integration-tests/interactive/model-toggle-hotkey.test.ts:45 (comment 3576700771) — existing [Critical] that the E2E test's synthetic model IDs (model-a/model-b via modelProviders) resolve in the model registry at runtime could not be confirmed from static analysis; the suite is unreachable by the per-workspace unit command and did not run in this review (test-only; the feature has maintainer real-run verification). Ruled cannot tell.
— qwen3.8-max-preview via Qwen Code /review
| it('blocks cursor-right (returns true) when canToggleModel passes', () => { | ||
| // Simulate the vimHandleInput wrapper: when canToggleModel is true, | ||
| // it returns true (consumed) and does NOT forward to vim cursor-right. | ||
| const vimHandleInputWrapper = (key: Key): boolean => { | ||
| if (canToggleModel(key, ALL_OK)) return true; |
There was a problem hiding this comment.
[Suggestion] This vimHandleInput cursor-right suppression block defines an inline vimHandleInputWrapper closure and tests that, not the real wrapper in AppContainer.tsx (~line 4490). The file imports only canToggleModel and Key, so removing or inverting the real wrapper's if (handleToggleKeypress(key)) return true; guard leaves both tests green. — Concrete cost: a future edit breaking the real wrapper's guard would not be caught here, and the two assertions duplicate cases already covered by the canToggleModel guard conditions block. The real wrapper is integration-tested in AppContainer.test.tsx (suppresses the vim cursor-right while the toggle fires), so nothing incorrect ships today — the harm is a false sense of coverage. Fix: remove this block, or import and test the actual wrapper closure.
中文说明
这个 vimHandleInput cursor-right suppression 测试块定义了一个内联的 vimHandleInputWrapper 闭合并测试它本身，而非 AppContainer.tsx（约 4490 行）中的真实 wrapper。该文件只导入了 canToggleModel 和 Key，因此即便删除或反转真实 wrapper 中的 if (handleToggleKeypress(key)) return true; 守卫，这两个测试仍会通过。— 具体代价：未来若破坏了真实 wrapper 的守卫，这里无法捕获；且这两个断言与 canToggleModel guard conditions 块中已有的用例重复。真实 wrapper 已在 AppContainer.test.tsx（suppresses the vim cursor-right while the toggle fires）中做了集成测试，因此当前不会发布错误行为——危害只是虚假的覆盖感。修复：删除该测试块，或导入并测试真实的 wrapper 闭包。
— qwen3.8-max-preview via Qwen Code /review
|
|
||
| const IS_WINDOWS = process.platform === 'win32'; | ||
|
|
||
| (IS_WINDOWS ? describe.skip : describe)('model toggle hotkey', () => { |
There was a problem hiding this comment.
[Suggestion] This integration test lives outside every npm workspace, so the per-workspace vitest run (e.g. npm test in packages/cli) never collects it — the test-efficacy probe marked it unreachable. — Concrete cost: if the CI job that runs the interactive integration suite (npm run test:integration:interactive:sandbox:none) is skipped or misconfigured, the Ctrl+F toggle has no automated regression gate and this test silently never executes. Fix: confirm CI runs that suite and collects this file (no code change needed if confirmed).
中文说明
该集成测试位于所有 npm workspace 之外，因此按 workspace 运行的 vitest run（例如 packages/cli 中的 npm test）永远不会收集它——测试有效性探针将其标记为 unreachable（不可达）。— 具体代价：如果运行交互式集成测试套件的 CI 任务（npm run test:integration:interactive:sandbox:none）被跳过或配置错误，Ctrl+F 切换将没有任何自动化回归门禁，该测试会静默地从不执行。修复：确认 CI 运行了该套件并收集此文件（若已确认则无需改代码）。
— qwen3.8-max-preview via Qwen Code /review
| activeView: 'main', | ||
| agents: new Map(), | ||
| agentViewHasActiveShellPty: false, |
There was a problem hiding this comment.
[Suggestion] The mock adds agentViewHasActiveShellPty: false, but AppContainer.tsx derives the toggle-blocking guard ref from a different field, activeAgentHasShellPty (AppContainer.tsx:2005: agentViewActiveShellPtyRef.current = activeView !== 'main' && activeAgentHasShellPty), which this mock omits. AppContainer never reads the context field agentViewHasActiveShellPty (its only occurrence is the canToggleModel guard property key at :1934). — Concrete cost: a future test for "Ctrl+F blocked while an agent tab's PTY is active" would set agentViewHasActiveShellPty: true here and see the toggle still fire (AppContainer ignores that field; activeView is pinned to 'main'). The pure guard is covered at can-toggle-model.test.ts:72, so nothing incorrect ships today — the harm is a misleading, ineffective mock field. Fix: provide the field AppContainer actually reads.
| activeView: 'main', | |
| agents: new Map(), | |
| agentViewHasActiveShellPty: false, | |
| activeView: 'main', | |
| agents: new Map(), | |
| activeAgentHasShellPty: false, |
中文说明
mock 添加了 agentViewHasActiveShellPty: false，但 AppContainer.tsx 是从另一个字段 activeAgentHasShellPty 推导切换阻塞守卫 ref 的（AppContainer.tsx:2005：agentViewActiveShellPtyRef.current = activeView !== 'main' && activeAgentHasShellPty），而该 mock 缺少这个字段。AppContainer 从不读取上下文字段 agentViewHasActiveShellPty（它唯一出现处是 :1934 的 canToggleModel 守卫属性键）。— 具体代价：未来若编写"agent 标签页有 PTY 时阻塞 Ctrl+F"的测试，会在此设置 agentViewHasActiveShellPty: true，却看到切换仍然触发（AppContainer 忽略该字段，且 activeView 固定为 'main'）。纯守卫已在 can-toggle-model.test.ts:72 覆盖，因此当前不会发布错误行为——危害是一个误导性的、无效的 mock 字段。修复：提供 AppContainer 实际读取的字段。
— qwen3.8-max-preview via Qwen Code /review
| }); | ||
|
|
||
| it('should toggle model on Ctrl+F and show Switched to message', async () => { | ||
| const { ptyProcess, promise } = rig.runInteractive(); |
There was a problem hiding this comment.
[Suggestion] The test never kills the PTY it spawns. rig.runInteractive() spawns a PTY child (test-helper.ts → pty.spawn) whose exit promise only resolves on ptyProcess.onExit; neither the test body nor afterEach (which only restores QWEN_CODE_LANG and calls rig.cleanup() → rm -rf testDir) ever calls ptyProcess.kill() or awaits the promise. — Concrete cost: each run orphans one CLI child process holding memory and an open PTY fd; across the interactive suite in CI these accumulate. Sibling tests (file-system-interactive.test.ts, external-context-auto-recall.test.ts, etc.) wrap the body in try/finally { ptyProcess.kill(); await promise; }. Fix: match that pattern (wrap the body in try { … } finally { ptyProcess.kill(); await promise; }).
中文说明
该测试从不杀死它启动的 PTY。rig.runInteractive() 会启动一个 PTY 子进程（test-helper.ts → pty.spawn），其退出 promise 只在 ptyProcess.onExit 时 resolve；测试主体和 afterEach（仅恢复 QWEN_CODE_LANG 并调用 rig.cleanup() → rm -rf testDir）都没有调用 ptyProcess.kill() 或 await 该 promise。— 具体代价：每次运行都会遗留一个孤儿 CLI 子进程，占用内存和一个打开的 PTY fd；在 CI 的整个交互式套件中这些会累积。同类测试（file-system-interactive.test.ts、external-context-auto-recall.test.ts 等）会用 try/finally { ptyProcess.kill(); await promise; } 包裹主体。修复：采用相同模式（用 try { … } finally { ptyProcess.kill(); await promise; } 包裹主体）。
— qwen3.8-max-preview via Qwen Code /review
| it('should render the last shortcut when toggleModel is not configured', () => { | ||
| const { lastFrame } = renderShortcuts(); |
There was a problem hiding this comment.
[Suggestion] The two "render the last shortcut" regression tests hard-code columns: 40 (via the useTerminalSize mock in renderShortcuts), which forces a 1-column layout deterministically (availableWidth = 36 < the ~65 chars needed for 2 columns). The slice loop then uses columnSplits[1] ([14]/[15]), which always equals shortcuts.length, so the dynamic columnSplits[2] ([8,7]/[7,7]) and columnSplits[3] ([5,5,5]/[5,5,4]) branches have zero effective coverage. — Concrete cost (verified by probe): changing the with-toggle 3-col split to [5,5,4] (sum 14 < 15) slices off the trailing "for external editor" shortcut on a wide terminal, yet both regression tests still pass because they render in 1-column mode. The invariant ships unverified for 2 of 3 layouts. Fix: add a wide-terminal variant (e.g. mock useTerminalSize to { columns: 120, rows: 24 } → 3 columns) and assert the last shortcut renders there too.
中文说明
这两个"渲染最后一个快捷键"的回归测试硬编码了 columns: 40（通过 renderShortcuts 中的 useTerminalSize mock），这会确定性地强制为单列布局（availableWidth = 36 < 双列所需的约 65 字符）。切片循环随后使用 columnSplits[1]（[14]/[15]），它恒等于 shortcuts.length，因此动态的 columnSplits[2]（[8,7]/[7,7]）和 columnSplits[3]（[5,5,5]/[5,5,4]）分支完全没有有效覆盖。— 具体代价（已通过探针验证）：若将带 toggle 的三列拆分改为 [5,5,4]（和 14 < 15），在宽终端下会切掉末尾的 "for external editor" 快捷键，但这两个回归测试仍会通过，因为它们以单列模式渲染。该不变量在 3 种布局中有 2 种未经验证就发布。修复：增加一个宽终端变体（例如将 useTerminalSize mock 为 { columns: 120, rows: 24 } → 3 列），并断言最后一个快捷键在那里也能渲染。
— qwen3.8-max-preview via Qwen Code /review
- Wrap E2E test body in try/finally to kill PTY process - Fix mock field: agentViewHasActiveShellPty → activeAgentHasShellPty - Remove redundant vimHandleInput wrapper tests (covered by integration test) - Add wide-terminal regression tests for 3-column layout
| if (uiActions.handleToggleKeypress(key)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
[Critical] Ctrl+F in !-shell mode (and other in-prompt sub-modes) triggers the model toggle instead of moving the cursor right. uiActions.handleToggleKeypress(key) is consulted here before the shellModeActive / sub-mode guards further down, and canToggleModel has no shellModeActive guard — so when model.toggleModel is configured and the agent is idle, all seven guards pass. This branch then returns true (suppressing cursor-right) while AppContainer's independent global keypress handler fires config.switchModel. The same mechanism applies in reverse-search, command-search, completion-suggestion and attachment sub-modes, none of which canToggleModel knows about. — Failure scenario: user configures model.toggleModel, types ! to enter shell mode, types a command, presses Ctrl+F to move the cursor forward → the cursor does not move and the active model silently changes, affecting every subsequent API call. Before this diff, Ctrl+F moved the cursor right in shell mode.
Fix: gate the toggle on InputPrompt's transient sub-modes here (e.g. !shellModeActive && !reverseSearchActive && !commandSearchActive && !showCompletionSuggestions && !isAttachmentMode && uiActions.handleToggleKeypress(key)), and add the matching guard in AppContainer's global handleGlobalKeypress so the suppression and the toggle cannot disagree — guarding only this side would let cursor-right and model-toggle fire together.
中文说明
[Critical] 在 ! shell 模式（以及其他输入态子模式）下按 Ctrl+F 会触发模型切换，而不是向右移动光标。此处先调用 uiActions.handleToggleKeypress(key)，之后才轮到下方的 shellModeActive / 子模式守卫；而 canToggleModel 并没有 shellModeActive 守卫——因此当配置了 model.toggleModel 且 agent 空闲时，七个守卫全部通过。于是这个分支返回 true（抑制光标右移），同时 AppContainer 独立的全局按键处理器执行 config.switchModel。反向搜索、命令搜索、补全建议、附件等子模式同样如此，canToggleModel 对这些状态一无所知。— 失败场景：用户配置 model.toggleModel，输入 ! 进入 shell 模式，输入命令后按 Ctrl+F 想把光标右移 → 光标不动，当前模型却被悄悄切换，影响之后所有 API 调用。在此 diff 之前，shell 模式下 Ctrl+F 是向右移动光标的。
修复：在此处用 InputPrompt 的临时子模式来门控切换（例如 !shellModeActive && !reverseSearchActive && !commandSearchActive && !showCompletionSuggestions && !isAttachmentMode && uiActions.handleToggleKeypress(key)），并在 AppContainer 的全局 handleGlobalKeypress 中加上对应守卫，使抑制与切换不会彼此矛盾——只在这一侧加守卫会导致光标右移与模型切换同时触发。
— qwen3.8-max-preview via Qwen Code /review
| // Insert after ctrl+o to keep logical grouping | ||
| shortcuts.splice(9, 0, { | ||
| key: 'ctrl+f', | ||
| description: t('to toggle model'), | ||
| }); |
There was a problem hiding this comment.
[Suggestion] The toggle shortcut is inserted with a hard-coded magic index (shortcuts.splice(9, 0, …)) into a separately-maintained literal array. — Concrete cost: if a future edit inserts or removes an entry above index 9, ctrl+f silently lands in a different logical group with no error; the new tests only assert frame.toContain('ctrl+f') / 'to toggle model', not its position relative to ctrl+o, so the drift ships green (plus a stale comment).
| // Insert after ctrl+o to keep logical grouping | |
| shortcuts.splice(9, 0, { | |
| key: 'ctrl+f', | |
| description: t('to toggle model'), | |
| }); | |
| // Insert after ctrl+o to keep logical grouping | |
| const idx = shortcuts.findIndex((s) => s.key === 'ctrl+o'); | |
| shortcuts.splice(idx + 1, 0, { | |
| key: 'ctrl+f', | |
| description: t('to toggle model'), | |
| }); |
中文说明
[Suggestion] 切换快捷键是用硬编码的魔法下标（shortcuts.splice(9, 0, …)）插入到一个单独维护的字面量数组里的。— 具体代价：如果将来有人在 index 9 之前插入或删除条目，ctrl+f 会悄无声息地落到另一个逻辑分组里而不会报错；新增测试只断言 frame.toContain('ctrl+f') / 'to toggle model'，并未断言它相对 ctrl+o 的位置，因此这种漂移会绿灯通过（外加一条过时的注释）。
修复：把插入点锚定到内容而非字面下标（见 suggestion 代码块）。
— qwen3.8-max-preview via Qwen Code /review
| if (action.type === 'already-on') { | ||
| historyManager.addItem( | ||
| { | ||
| type: MessageType.INFO, | ||
| text: t('Already on {{model}}', { model: action.modelId }), | ||
| }, | ||
| Date.now(), | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The already-on handler branch has no AppContainer-level unit test. — Concrete cost: if this branch (or its message) were removed or changed, no test would catch it — a user who is already on the toggle target and presses Ctrl+F would see no feedback (or wrong feedback) instead of the Already on {{model}} info message. The pure computeToggleAction is tested for the already-on return type in resolve-toggle-model.test.ts, but the AppContainer wiring that displays the message is not.
Fix: add a test that sets config.getModel() to the toggle target with no previousModelRef, presses Ctrl+F, and asserts historyManager.addItem is called with MessageType.INFO and text containing Already on.
中文说明
[Suggestion] already-on 处理分支没有 AppContainer 层的单元测试。— 具体代价：如果这个分支（或其文案）被删除或修改，没有任何测试能发现——当用户已经处于切换目标模型时按 Ctrl+F，会看不到反馈（或看到错误反馈），而不是 Already on {{model}} 提示。纯函数 computeToggleAction 在 resolve-toggle-model.test.ts 里对 already-on 返回类型有测试，但 AppContainer 中显示该消息的接线没有被测试。
修复：新增一个测试，把 config.getModel() 设为切换目标模型且没有 previousModelRef，按下 Ctrl+F，断言 historyManager.addItem 以 MessageType.INFO 且文案包含 Already on 被调用。
— qwen3.8-max-preview via Qwen Code /review
| !guards.agentViewHasActiveShellPty && | ||
| !guards.dialogsVisible | ||
| ); |
There was a problem hiding this comment.
[Suggestion] canToggleModel guards only modal dialogs (dialogsVisible) and PTY/shell-focus states; it has no awareness of InputPrompt's transient in-prompt sub-modes (reverse search, command search, completion suggestions, attachment mode), yet AppContainer's global handler fires the toggle on every keypress. — Failure scenario: with model.toggleModel configured and the agent idle, open reverse history search (Ctrl+R) then press Ctrl+F to move the cursor → none of the seven guards catches reverse search, so the global handler calls switchModel (model changes unexpectedly) and InputPrompt's branch at ~line 1092 swallows the key. This is the same defect class as the shell-mode Critical above but broader; a fix that only adds a shellModeActive guard leaves these triggers broken.
Fix: gate the toggle on "InputPrompt is in a transient editing sub-mode" — surface shellModeActive || reverseSearchActive || commandSearchActive || showCompletionSuggestions || isAttachmentMode into the guard set, or have the global handler defer to InputPrompt's local key ownership.
中文说明
[Suggestion] canToggleModel 只守卫模态对话框（dialogsVisible）和 PTY / shell 焦点状态；它完全不知道 InputPrompt 的临时输入态子模式（反向搜索、命令搜索、补全建议、附件模式），而 AppContainer 的全局处理器在每次按键时都会触发切换。— 失败场景：配置了 model.toggleModel 且 agent 空闲时，打开反向历史搜索（Ctrl+R）再按 Ctrl+F 想移动光标 → 七个守卫没有一个能拦住反向搜索，于是全局处理器调用 switchModel（模型被意外切换），而 InputPrompt 约 1092 行的分支把这个键吞掉了。这与上面 shell 模式的 Critical 属于同一类缺陷，但范围更广；只加 shellModeActive 守卫的修复会让这些触发点仍然有问题。
修复：用“InputPrompt 是否处于临时编辑子模式”来门控切换——把 shellModeActive || reverseSearchActive || commandSearchActive || showCompletionSuggestions || isAttachmentMode 纳入守卫集合，或让全局处理器让位于 InputPrompt 的本地按键归属。
— qwen3.8-max-preview via Qwen Code /review
| const agentViewHasActiveShellPty = agentViewActiveShellPtySet.size > 0; | ||
| const activeAgentHasShellPty = agentViewActiveShellPtySet.has(activeView); |
There was a problem hiding this comment.
[Suggestion] agentViewHasActiveShellPty ("any agent has a PTY") is computed and included in the state useMemo dependency array, but no production consumer reads it — AppContainer derives the toggle guard solely from activeAgentHasShellPty (per active view) at AppContainer.tsx:2005. — Concrete cost: when a background agent opens or closes a shell PTY while the user is on the main view, this dead field flips false → true while activeAgentHasShellPty stays false, invalidating the memo and forcing all six AgentViewStateContext consumers (AppContainer, DefaultAppLayout, InputPrompt, AgentTabBar, AgentComposer, AgentChatView) to re-render though none reads the changed field.
Fix: remove agentViewHasActiveShellPty from the AgentViewState interface, DEFAULT_STATE, and the useMemo value/deps; keep the internal agentViewActiveShellPtySet state and the setAgentViewHasActiveShellPty action (both still needed to derive/populate activeAgentHasShellPty).
中文说明
[Suggestion] agentViewHasActiveShellPty（“任意 agent 有 PTY”）被计算出来并放进了 state 的 useMemo 依赖数组，但没有任何生产消费者读取它——AppContainer 在 AppContainer.tsx:2005 仅用 activeAgentHasShellPty（按当前视图）来推导切换守卫。— 具体代价：当用户停留在主视图时，后台 agent 开启或关闭 shell PTY 会让这个无用字段从 false → true 翻转，而 activeAgentHasShellPty 仍为 false，于是 memo 失效，迫使全部六个 AgentViewStateContext 消费者（AppContainer、DefaultAppLayout、InputPrompt、AgentTabBar、AgentComposer、AgentChatView）重新渲染，尽管没有一个读取这个变化的字段。
修复：从 AgentViewState 接口、DEFAULT_STATE 以及 useMemo 的 value/deps 中移除 agentViewHasActiveShellPty；保留内部的 agentViewActiveShellPtySet state 和 setAgentViewHasActiveShellPty action（推导/填充 activeAgentHasShellPty 仍需要它们）。
— qwen3.8-max-preview via Qwen Code /review
- Add inputPromptInTransientMode guard to canToggleModel to prevent toggle in shell/reverse-search/command-search/completion/attachment modes
- Move handleToggleKeypress call after transient mode checks in InputPrompt
- Sync transient mode state to AppContainer via setInputPromptTransientMode
- Replace magic index 9 with findIndex('ctrl+o') in KeyboardShortcuts
- Add test for 'Already on' info message in AppContainer
- Add test for inputPromptInTransientMode guard in can-toggle-model
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| onSubmit(finalValue, { | ||
| deferUntilIdle: options?.deferUntilIdle ?? false, | ||
| submittedPrompt, | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Mutant survivor: dropping the ?? false fallback from options?.deferUntilIdle ?? false leaves every affected test green — the fallback to false when options is undefined is untested. — Failure scenario: if ?? false is removed, deferUntilIdle becomes undefined on the plain Enter-to-submit path (no options object); downstream code distinguishing deferUntilIdle === false from truthiness would behave differently, potentially deferring submission when it should not. Add a test that calls handleSubmitAndClear(text) with no options argument and asserts onSubmit receives deferUntilIdle: false (not undefined).
中文说明
变异测试存活：删除 options?.deferUntilIdle ?? false 中的 ?? false 回退后，所有相关测试仍然通过——options 为 undefined 时回退到 false 的这一行为没有被测试覆盖。
失败场景：如果 ?? false 被移除，在普通的回车提交路径（不传 options 对象）下 deferUntilIdle 会变成 undefined；下游区分 deferUntilIdle === false 与真值判断的代码行为会不同，可能在本不该延迟提交时延迟提交。建议添加测试：调用 handleSubmitAndClear(text) 且不传 options，断言 onSubmit 收到的 deferUntilIdle 为 false（而非 undefined）。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| setAgentViewActiveShellPtySet((prev) => { | ||
| const has = prev.has(agentId); | ||
| if (hasPty === has) return prev; | ||
| const next = new Set(prev); |
There was a problem hiding this comment.
[Suggestion] Mutant survivor: forcing the no-op guard if (hasPty === has) to always-true leaves every test green — no test pins the case where setAgentViewHasActiveShellPty is called with the value the set already holds (which should be a no-op returning prev). — Failure scenario: if the early-return guard is removed or broken, every call creates a new Set and triggers a React re-render even when nothing changed; with frequent PTY status updates this causes unnecessary re-renders of the whole agent-view tree. Add a test that calls setAgentViewHasActiveShellPty(id, true) twice and asserts the state reference is unchanged on the second call (no re-render).
中文说明
变异测试存活：将空操作守卫 if (hasPty === has) 强制改为恒真后，所有测试仍然通过——没有测试覆盖「用集合中已有的值调用 setAgentViewHasActiveShellPty」这一情形（此时应是返回 prev 的空操作）。
失败场景：如果该提前返回守卫被移除或破坏，每次调用都会创建新的 Set 并触发 React 重渲染，即使状态没有变化；在 PTY 状态频繁更新时会导致整棵 agent-view 树的不必要重渲染。建议添加测试：两次调用 setAgentViewHasActiveShellPty(id, true)，断言第二次调用时 state 引用不变（无重渲染）。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
| for (const authType of Object.values(AuthType)) { | ||
| if (authType !== currentAuthType && hasModel(config, authType, modelId)) { | ||
| return { modelId, authType }; | ||
| } | ||
| } |
There was a problem hiding this comment.
[Suggestion] Mutant survivor: forcing the cross-provider loop guard if (authType !== currentAuthType && hasModel(...)) to always-true leaves every test green — no test exercises the case where the loop must skip an auth type (because it IS the current one, or does not own the model). — Failure scenario: if the authType !== currentAuthType check is dropped, the cross-provider search loop could return the current auth type from the loop instead of falling through to the final fallback, changing the resolution order and masking lookup bugs. Add a test to resolve-toggle-model.test.ts where the model is owned only by the current provider, and assert the function returns currentAuthType via the fallback (line 73), not via the loop.
中文说明
变异测试存活：将跨 provider 循环守卫 if (authType !== currentAuthType && hasModel(...)) 强制改为恒真后，所有测试仍然通过——没有测试覆盖「循环必须跳过某个 auth type」的情形（因为它是当前 auth type，或不拥有该模型）。
失败场景：如果 authType !== currentAuthType 检查被移除，跨 provider 搜索循环可能从循环中返回当前 auth type，而不是走到最后的回退分支，从而改变解析顺序并掩盖查找缺陷。建议在 resolve-toggle-model.test.ts 中添加测试：使模型仅由当前 provider 拥有，断言函数通过回退分支（第 73 行）返回 currentAuthType，而非通过循环返回。
— qwen3.8-max-preview via Qwen Code /review (v0.21.3)
Cover three mutant survivor findings from PR review #4835752200: - InputPrompt: assert deferUntilIdle is strictly false (not undefined) - AgentViewContext: verify no-op guard prevents re-render on duplicate call - resolve-toggle-model: cover early-return path for current-provider-only model
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
中文说明
已审查——无阻断问题。 建议见行内评论。
— qwen3.8-max-preview via Qwen Code /review
| !isAttachmentMode && | ||
| uiActions.handleToggleKeypress(key) | ||
| ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The five transient-mode conditions here duplicate the inTransientMode variable defined above (~line 388) — this Ctrl+F guard is exactly !inTransientMode && uiActions.handleToggleKeypress(key). A third copy of the same predicate sits in the pre-existing voice-input guard below (~line 1119). — Failure scenario: adding a sixth transient sub-mode means updating inTransientMode and this guard in lockstep; missing this site lets Ctrl+F toggle the model in that mode instead of moving the cursor.
if (!inTransientMode && uiActions.handleToggleKeypress(key)) {
return true;
}
// also add `inTransientMode` to the handleInput useCallback deps中文说明
此处的五个 transient-mode 条件与上方（约 388 行）定义的 inTransientMode 变量重复——这个 Ctrl+F 守卫恰好就是 !inTransientMode && uiActions.handleToggleKeypress(key)。下方（约 1119 行）既有的语音输入守卫是同一谓词的第三份拷贝。— 失败场景：若新增第六个 transient 子模式，需要同步更新 inTransientMode 和此守卫；遗漏此处会导致在该模式下 Ctrl+F 切换模型而非移动光标。建议改用 !inTransientMode，并把 inTransientMode 加入 handleInput 的 useCallback 依赖。
— qwen3.8-max-preview via Qwen Code /review
| useEffect(() => { | ||
| uiActions.setInputPromptTransientMode(inTransientMode); | ||
| }, [inTransientMode, uiActions]); |
There was a problem hiding this comment.
[Suggestion] No test asserts setInputPromptTransientMode(true) is ever called in a sub-mode, yet the always-active global toggle handler in AppContainer depends entirely on this synced state (inputPromptInTransientMode). — Failure scenario: a mutation dropping any one of the five inTransientMode conditions leaves the synced state false in that sub-mode; AppContainer's global handler (useKeypress(handleGlobalKeypress, { isActive: true }), which fires on every keypress independently) then passes canToggleModel and toggles the model on Ctrl+F while the user is in shell / reverse-search / completion / attachment mode. No unit or integration test turns red under that mutation.
// InputPrompt.test.tsx
// render with shellModeActive (and reverse-search / completion / attachment) true,
// assert setInputPromptTransientMode was called with true;
// plus one asserting false when no sub-mode is active.中文说明
没有任何测试断言 setInputPromptTransientMode(true) 会在某个子模式下被调用，但 AppContainer 中始终激活的全局切换处理器完全依赖这个同步过来的状态（inputPromptInTransientMode）。— 失败场景：若变异删除 inTransientMode 五个条件中的任意一个，该子模式下同步状态会保持为 false；AppContainer 的全局处理器（useKeypress(handleGlobalKeypress, { isActive: true })，对每个按键独立触发）便会通过 canToggleModel，在用户处于 shell / 反向搜索 / 补全 / 附件模式时按 Ctrl+F 切换模型。该变异下没有任何单元或集成测试会变红。建议补充相应测试。
— qwen3.8-max-preview via Qwen Code /review
| const currentAuthType = config.getAuthType(); | ||
| if (!currentAuthType) { | ||
| historyManager.addItem( |
There was a problem hiding this comment.
[Suggestion] The glue's no-auth early-exit (emitting "⚠ Cannot toggle: auth type not available" when config.getAuthType() is falsy) has no AppContainer-level test — every toggle test mocks getAuthType to USE_OPENAI. — Failure scenario: deleting this if (!currentAuthType) guard would go uncaught: control falls into resolveToggleTarget/computeToggleAction with undefined, computeToggleAction returns { type: 'no-auth' }, and the handler's later if (action.type === 'no-auth') return; turns the user-visible error into a silent no-op. The computeToggleAction no-auth branch is unit-tested in isolation, but this message-emitting path (and the new i18n string added across all 9 locales) is exercised by nothing.
// AppContainer.test.tsx
// mock getAuthType -> undefined;
// assert addItem is called with an ERROR item containing
// 'Cannot toggle: auth type not available' and switchModel is never called.中文说明
胶水层的 no-auth 提前退出（当 config.getAuthType() 为假时发出 "⚠ Cannot toggle: auth type not available"）没有 AppContainer 级别的测试——所有切换测试都把 getAuthType mock 成 USE_OPENAI。— 失败场景：删除这个 if (!currentAuthType) 守卫不会被发现：控制流会带着 undefined 进入 resolveToggleTarget/computeToggleAction，computeToggleAction 返回 { type: 'no-auth' }，而处理器后面的 if (action.type === 'no-auth') return; 会把用户可见的错误变成静默无操作。computeToggleAction 的 no-auth 分支虽有独立单测，但这个发出消息的路径（以及新增到全部 9 个语言包的 i18n 字符串）没有任何测试覆盖。建议补充相应测试。
— qwen3.8-max-preview via Qwen Code /review
| inputPromptInTransientMode: inputPromptTransientMode, | ||
| }), | ||
| [inputPromptTransientMode], |
There was a problem hiding this comment.
[Suggestion] inputPromptTransientMode is captured as React state in this useCallback (dep [inputPromptTransientMode]) while the other seven guards are read from refs. — Failure scenario: when InputPrompt enters a transient sub-mode it calls setInputPromptTransientMode(true) in a useEffect; between that effect and AppContainer re-rendering, a Ctrl+F reads the stale closure value (false), so canToggleModel passes and the toggle fires in a sub-mode where Ctrl+F should move the cursor (a one-render window, but deterministic). The [inputPromptTransientMode] dep also recreates handleToggleKeypress on every transient-mode change, cascading through the global keypress handler and the uiActions useMemo to re-render every useUIActions consumer — overhead the ref-based guards avoid.
const inputPromptTransientModeRef = useRef(false);
const setInputPromptTransientMode = useCallback((value: boolean) => {
inputPromptTransientModeRef.current = value;
}, []);
// read inputPromptTransientModeRef.current in handleToggleKeypress; drop the dep.中文说明
inputPromptTransientMode 在这个 useCallback 中以 React state 形式被捕获（依赖 [inputPromptTransientMode]），而其余七个守卫都从 ref 读取。— 失败场景：当 InputPrompt 进入某个 transient 子模式时，它在 useEffect 中调用 setInputPromptTransientMode(true)；在该 effect 与 AppContainer 重新渲染之间，Ctrl+F 会读到闭包中的旧值（false），于是 canToggleModel 通过，在本应移动光标的子模式里触发了模型切换（窗口为一个渲染周期，但机制是确定性的）。[inputPromptTransientMode] 依赖还会在每次 transient 模式变化时重建 handleToggleKeypress，经由全局按键处理器和 uiActions 的 useMemo 级联，导致所有 useUIActions 消费者重新渲染——这是基于 ref 的守卫所能避免的开销。建议改用 ref + 稳定 setter。
— qwen3.8-max-preview via Qwen Code /review
- Deduplicate transient-mode conditions in InputPrompt handleInput (use inTransientMode) - Convert inputPromptTransientMode from useState to useRef to eliminate race condition and cascade re-renders - Add test for setInputPromptTransientMode sync in sub-modes - Add test for no-auth error path in AppContainer toggle
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Unresolved, please confirm: [Critical] Existing blocker (inline comment 3590698595, InputPrompt/AppContainer toggle success path): no post-resolve verification that switchModel actually changed the model — could not statically determine whether config.switchModel can resolve without changing the model, so the 'Switched to {{model}}' message could in theory be shown without a real switch. Fail closed: cannot tell. (Maintainer runtime testing did not reproduce it; treated as a defensive-check concern.)
中文说明
未决，请确认：[Critical] Existing blocker (inline comment 3590698595, InputPrompt/AppContainer toggle success path): no post-resolve verification that switchModel actually changed the model — could not statically determine whether config.switchModel can resolve without changing the model, so the 'Switched to {{model}}' message could in theory be shown without a real switch. Fail closed: cannot tell. (Maintainer runtime testing did not reproduce it; treated as a defensive-check concern.)
— qwen3.8-max-preview via Qwen Code /review
| // takes priority over the global model toggle. Only defer to the | ||
| // model-toggle handler when no agent shell PTY is active. |
There was a problem hiding this comment.
[Critical] The agent-view composer dual-fires Ctrl+F. This new delegation suppresses only the shell-focus toggle in AgentChatContent, but the agent composer's own BaseTextInput is a separate keypress subscriber: AgentComposer.handleKeypress returns false for Ctrl+F, so BaseTextInput.handleKey falls through to text-buffer.ts:2676 (key.ctrl && key.name === 'f' → move('right')). KeypressContext.broadcast invokes every subscriber with no stop-propagation, so while the always-active global handler (AppContainer.tsx:4091) switches the model, the composer cursor also moves right one character. The main InputPrompt was patched to prevent exactly this (InputPrompt.tsx:1108), but the agent composer path was not. — Failure scenario: on an idle agent tab with the input focused and no PTY, press Ctrl+F → the model toggles AND the cursor in the user's in-progress agent message jumps right one character (probe-confirmed: onKeypress(Ctrl+F) === false while isActive === true).
Suggested fix: consume the toggle key in AgentComposer.handleKeypress before it reaches BaseTextInput, mirroring the main InputPrompt — e.g. add if (uiActions.handleToggleKeypress(key)) return true; (via useUIActions) ahead of the tab-bar / navigation logic.
中文说明
代理视图的输入框（AgentComposer）会对 Ctrl+F 双触发。此处新增的委托只在 AgentChatContent 中抑制了 shell 焦点切换，但 agent composer 自己的 BaseTextInput 是另一个独立的按键订阅者：AgentComposer.handleKeypress 对 Ctrl+F 返回 false，因此 BaseTextInput.handleKey 会落到 text-buffer.ts:2676（key.ctrl && key.name === 'f' → move('right')）。KeypressContext.broadcast 会调用所有订阅者且不停止传播，所以常驻的全局处理器（AppContainer.tsx:4091）在切换模型的同时，composer 光标也会向右移动一格。主 InputPrompt 已经为此打了补丁（InputPrompt.tsx:1108），但 agent composer 路径没有。— 触发场景：在一个空闲、输入框聚焦、无 PTY 的 agent 标签页按 Ctrl+F → 模型被切换，同时用户正在编辑的 agent 消息中的光标向右跳一格（已用 probe 验证）。
建议修复：在 AgentComposer.handleKeypress 中、在按键到达 BaseTextInput 之前消费掉切换键，参照主 InputPrompt 的做法 — 例如在 tab-bar / 导航逻辑之前加上 if (uiActions.handleToggleKeypress(key)) return true;（通过 useUIActions）。
— qwen3.8-max-preview via Qwen Code /review
| // In transient sub-modes (shell, reverse/command search, completion, | ||
| // attachment), Ctrl+F should move the cursor, not toggle the model. | ||
| if (!inTransientMode && uiActions.handleToggleKeypress(key)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
[Critical] This Ctrl+F cursor-right suppression is subscriber-order-dependent and can be defeated. KeypressContext.broadcast walks the subscriber Set in insertion order with no consumption flag. The main BaseTextInput subscribes with isActive={!isEmbeddedShellFocused} (InputPrompt.tsx:2270); focusing the embedded shell and returning flips isActive false→true, which unsubscribes and re-subscribes it, re-inserting it AFTER the always-active global handler (AppContainer.tsx:4091). From then on the global handler runs first and sets isTogglingRef.current = true synchronously (AppContainer.tsx:3785), so handleToggleKeypress(key) here returns false and BaseTextInput falls through to text-buffer.ts:2676 move('right'). — Failure scenario: with model.toggleModel configured, focus the embedded shell then return to the input, then press Ctrl+F while idle → the model toggles AND the cursor jumps right one character in the in-progress prompt (probe-confirmed: cursor-right fired 0× in initial subscriber order, 1× after the focus round-trip reorder).
Suggested fix: do not make suppression depend on the mutable isToggling flag the global handler flips mid-dispatch. Either gate the local suppression on a side-effect-free predicate that excludes isToggling (e.g. keyMatchers[TOGGLE_MODEL](key) && toggleModelConfigured && <stable guards except isToggling>), so both handlers agree regardless of order; or have the global handler own cursor suppression (mark the keypress consumed in a ref the text-buffer checks), removing the reliance on subscriber insertion order.
中文说明
这个对 Ctrl+F 光标右移的抑制依赖于订阅者顺序，可能被绕过。KeypressContext.broadcast 按插入顺序遍历订阅者 Set，且没有消费标志。主 BaseTextInput 以 isActive={!isEmbeddedShellFocused} 订阅（InputPrompt.tsx:2270）；聚焦内嵌 shell 再返回会使 isActive 从 false→true，从而先取消订阅再重新订阅，把它重新插入到常驻全局处理器（AppContainer.tsx:4091）之后。此后全局处理器先运行并同步设置 isTogglingRef.current = true（AppContainer.tsx:3785），于是这里的 handleToggleKeypress(key) 返回 false，BaseTextInput 落到 text-buffer.ts:2676 的 move('right')。— 触发场景：配置了 model.toggleModel 时，先聚焦内嵌 shell 再返回输入框，然后在空闲时按 Ctrl+F → 模型被切换，同时正在编辑的输入中的光标向右跳一格（已用 probe 验证：初始订阅者顺序下光标右移触发 0 次，焦点往返重排后触发 1 次）。
建议修复：不要让抑制依赖于全局处理器在分发过程中翻转的可变 isToggling 标志。要么用一个无副作用、不包含 isToggling 的谓词来做本地抑制（例如 keyMatchers[TOGGLE_MODEL](key) && toggleModelConfigured && <除 isToggling 外的稳定守卫>），使两个处理器无论顺序如何都保持一致；要么让全局处理器负责光标抑制（用一个 text-buffer 会检查的 ref 标记该按键已被消费），从而彻底摆脱对订阅者插入顺序的依赖。
— qwen3.8-max-preview via Qwen Code /review
| useEffect(() => { | ||
| uiActions.setInputPromptTransientMode(inTransientMode); |
There was a problem hiding this comment.
[Suggestion] This effect has no unmount cleanup, so inputPromptTransientModeRef (AppContainer.tsx:508) can stay stuck at true after InputPrompt unmounts. DefaultAppLayout renders the main InputPrompt only in the main-view branch — it unmounts on an agent-tab switch or when a dialog opens. If the user leaves the main view while a transient sub-mode is active (e.g. shellModeActive after typing !), the last effect run left the ref true and nothing resets it; the global toggle guard still reads it (AppContainer.tsx:1940), so canToggleModel returns false and Ctrl+F silently does nothing on an agent tab until InputPrompt remounts. — Concrete cost: a reachable user sequence silently disables the model-toggle hotkey.
| useEffect(() => { | |
| uiActions.setInputPromptTransientMode(inTransientMode); | |
| useEffect(() => { | |
| uiActions.setInputPromptTransientMode(inTransientMode); | |
| return () => uiActions.setInputPromptTransientMode(false); |
中文说明
这个 effect 没有卸载清理，因此 InputPrompt 卸载后 inputPromptTransientModeRef（AppContainer.tsx:508）可能一直停留在 true。DefaultAppLayout 只在主视图分支渲染主 InputPrompt —— 切换 agent 标签页或打开对话框时它会卸载。如果用户在某个临时子模式激活时（例如输入 ! 后的 shellModeActive）离开主视图，effect 最后一次运行把 ref 置为 true 后没有任何东西重置它；全局切换守卫仍会读取它（AppContainer.tsx:1940），于是 canToggleModel 返回 false，Ctrl+F 在 agent 标签页上会静默失效，直到 InputPrompt 重新挂载。— 具体代价：一个可达的用户操作序列会静默禁用模型切换快捷键。
上面的 suggestion 在卸载时把标志重置为 false。
— qwen3.8-max-preview via Qwen Code /review
| "integrity": "sha512-H3mcG6ZDLTlYfaSNi0iOKkigqMFvkTKlGUYlD8GW7nNOYRrevuA46iTypPyv+06V3fEmvvazfntkBU34L0azAw==", | ||
| "dev": true, | ||
| "license": "MIT", | ||
| "peer": true, |
There was a problem hiding this comment.
[Suggestion] ~520 lines of package-lock.json churn (adding/removing "peer": true across react, react-dom, vite, vitest, typescript, eslint, zod, the @img/sharp-* binaries, etc.) with no corresponding package.json change in this PR — the feature adds no dependencies. Every hunk in this range is a flag flip with no version / resolved / integrity change, the signature of regeneration under a different npm major version than the project's pinned one (the only substantive addition, sharp@^0.34.5, is already declared in package.json:152). — Concrete cost: inflates a small hotkey PR, creates merge-conflict surface against concurrent dependency PRs, and the next contributor's npm install regenerates the flags back (perpetual churn). It does not break npm ci.
Suggested fix: revert package-lock.json (regenerate with the repo's pinned npm version, or restore the base lockfile) so the diff contains only the feature changes.
中文说明
本 PR 没有修改 package.json，却带来约 520 行 package-lock.json 变动（在 react、react-dom、vite、vitest、typescript、eslint、zod、@img/sharp-* 平台二进制等条目上增删 "peer": true）—— 而这个功能并不新增任何依赖。该范围内的每个 hunk 都只是标志翻转，没有版本 / resolved / integrity 变化，这是用与项目锁定版本不同的 npm 主版本重新生成 lockfile 的典型特征（唯一实质性的新增 sharp@^0.34.5 已经在 package.json:152 中声明）。— 具体代价：膨胀了一个小的快捷键 PR，增加与并行依赖 PR 的合并冲突面，并且下一位贡献者运行 npm install 会把这些标志再翻转回去（持续的来回变动）。它不会破坏 npm ci。
建议修复：还原 package-lock.json（用仓库锁定的 npm 版本重新生成，或恢复 base 的 lockfile），使 diff 只包含功能改动。
— qwen3.8-max-preview via Qwen Code /review
| handler.toString().includes('handleToggleKeypress'), | ||
| ) as ((key: Key) => void) | undefined; | ||
|
|
||
| const ctrlF = makeKey({ name: 'f', ctrl: true, sequence: '\x06' }); |
There was a problem hiding this comment.
[Suggestion] The makeKey, getGlobalKeypress, and ctrlF helpers are copy-pasted verbatim across the two new sibling describe blocks (and a third copy above) — makeKey at ~4911/4985/5158, getGlobalKeypress at ~4922/4999/5169 (both new copies use the fragile handler.toString().includes('handleToggleKeypress') heuristic), ctrlF at ~5009/5179. — Concrete cost: the toggle-key definition and the handler-location heuristic live in multiple places; if the binding changes or the handler is renamed/inlined, each copy must be edited in lockstep — updating one and missing another leaves a describe block silently testing a stale key or returning undefined from getGlobalKeypress.
Suggested fix: hoist makeKey, getGlobalKeypress, and ctrlF into the enclosing describe('AppContainer State Management') scope (or a small shared helper) and reference them from each block.
中文说明
makeKey、getGlobalKeypress 和 ctrlF 这几个辅助函数在两个新增的同级 describe 块中被逐字复制粘贴（上方还有第三份拷贝）—— makeKey 在 ~4911/4985/5158，getGlobalKeypress 在 ~4922/4999/5169（两份新拷贝都使用了脆弱的 handler.toString().includes('handleToggleKeypress') 启发式），ctrlF 在 ~5009/5179。— 具体代价：切换键定义和处理器定位启发式分散在多处；如果绑定改变或处理器被重命名/内联，每份拷贝都必须同步修改 —— 改了一处漏了另一处，就会让某个 describe 块静默地测试一个过期的按键，或从 getGlobalKeypress 返回 undefined。
建议修复：把 makeKey、getGlobalKeypress 和 ctrlF 提升到外层 describe('AppContainer State Management') 作用域（或一个小的共享辅助函数），并在各块中引用它们。
— qwen3.8-max-preview via Qwen Code /review
| useEffect(() => { | ||
| const unsub = config.onModelChange(() => { | ||
| if (!isTogglingRef.current) { | ||
| previousModelRef.current = null; |
There was a problem hiding this comment.
[Suggestion] This external-model-change invalidation of previousModelRef (and its !isTogglingRef.current condition) is exercised by no test. The resolve-toggle-model.test.ts case "returns forward … after external model change" passes null directly into computeToggleAction — it verifies the consumer of the null, not this producer; the pre-existing "Model change refreshStatic wiring" test fires only the last onModelChange subscriber and never this one. Deleting this effect or flipping the guard leaves every test green. — Concrete cost: the regression that would then ship — toggle model-a→model-b (previousModelRef=model-a), then two external /model changes; with invalidation gone previousModelRef stays model-a, so a Ctrl+F while on model-b (== target) yields backward and jumps to the stale model-a instead of "Already on model-b". (The code is currently correct — this is a coverage gap, not a live bug.)
Suggested fix: add an AppContainer-level test — toggle forward to model-b, fire the captured onModelChange listener with a different model (guard not toggling), then assert a subsequent Ctrl+F performs a fresh forward switch (or "Already on") rather than a backward switch to the stale model; also assert the listener is NOT acted on while isTogglingRef is set.
中文说明
这个对 previousModelRef 的外部模型变更失效处理（以及它的 !isTogglingRef.current 条件）没有任何测试覆盖。resolve-toggle-model.test.ts 中 "returns forward … after external model change" 用例直接把 null 传入 computeToggleAction —— 它验证的是 null 的消费者，而非这个生产者；已有的 "Model change refreshStatic wiring" 测试只触发最后一个 onModelChange 订阅者，从不触发这一个。删除这个 effect 或翻转守卫，所有测试仍然为绿。— 具体代价：随后会漏过的回归 —— 切换 model-a→model-b（previousModelRef=model-a），然后两次外部 /model 变更；失效处理消失后 previousModelRef 仍是 model-a，于是在 model-b（== 目标）上按 Ctrl+F 会得到 backward，跳到过期的 model-a 而不是显示 "Already on model-b"。（代码目前是正确 —— 这是覆盖缺口，不是现存 bug。）
建议修复：增加一个 AppContainer 级别的测试 —— 先向前切换到 model-b，用不同的模型触发捕获到的 onModelChange 监听器（此时不在切换中），然后断言随后的 Ctrl+F 执行一次新的向前切换（或 "Already on"），而不是向后切换到过期模型；同时断言当 isTogglingRef 被设置时该监听器不被处理。
— qwen3.8-max-preview via Qwen Code /review
| it('should render the last shortcut in wide terminal (3 columns) without toggleModel', () => { | ||
| const { lastFrame } = renderShortcutsWide(); | ||
| const frame = lastFrame(); |
There was a problem hiding this comment.
[Suggestion] The comment above these wide-terminal tests says they cover the columnSplits[2] and columnSplits[3] branches the narrow tests leave unverified, but only two widths exist in this file: 40 (→ 1 column) and 120 (→ 3 columns). No test lands in the [width2Col, width3Col) band (roughly 68–95 columns) where numColumns === 2, so the 2-column split ({2:[8,7]} with toggle / {2:[7,7]} without) is never exercised. — Concrete cost: a future edit that breaks the 2-column sums (e.g. "rebalancing" [8,7] to [7,7], or copy-pasting the no-toggle entry into the toggle branch) slices off the 15th shortcut (ctrl+x "for external editor") only for terminals ~69–96 columns wide, while every test here (1-column at 40, 3-column at 120) stays green — exactly the regression the comment claims to guard.
Suggested fix: add a pair of tests using a width inside the 2-column band (e.g. useTerminalSizeMock.mockReturnValue({ columns: 80, rows: 24 })) asserting ctrl+x / for external editor render, with and without toggleModel; or correct the comment to state that only the 3-column branch is verified.
中文说明
这些宽终端测试上方的注释说它们覆盖了窄测试未验证的 columnSplits[2] 和 columnSplits[3] 分支，但本文件中只有两种宽度：40（→ 1 列）和 120（→ 3 列）。没有任何测试落在 numColumns === 2 的 [width2Col, width3Col) 区间（约 68–95 列），因此 2 列分配（带 toggle 的 {2:[8,7]} / 不带的 {2:[7,7]}）从未被覆盖。— 具体代价：未来一次破坏 2 列总和的修改（例如把 [8,7] "重新平衡" 成 [7,7]，或把无 toggle 的条目复制到 toggle 分支）只会对约 69–96 列宽的终端切掉第 15 个快捷键（ctrl+x "for external editor"），而这里所有测试（40 时 1 列、120 时 3 列）都仍为绿 —— 正是注释声称要防的回归。
建议修复：增加一对使用 2 列区间内宽度的测试（例如 useTerminalSizeMock.mockReturnValue({ columns: 80, rows: 24 })），断言 ctrl+x / for external editor 被渲染，分别覆盖带和不带 toggleModel 的情况；或者更正注释，说明只验证了 3 列分支。
— qwen3.8-max-preview via Qwen Code /review
| // Behavioral: the error path cleared previousModelRef, so the second | ||
| // toggle is a fresh forward switch to the configured target (model-b), | ||
| // not a backward switch to a stale return model. Same-provider toggle |
There was a problem hiding this comment.
[Suggestion] This comment claims the test verifies the error path cleared previousModelRef, but the assertion cannot distinguish cleared from not-cleared. previousModelRef starts null (AppContainer.tsx:1027) and the first toggle FAILS, so the success handler (the only non-null writer, ~3790) never runs; the error handler's previousModelRef.current = null (~3806) is a no-op on an already-null ref. The second Ctrl+F is forward because currentModel !== target.modelId regardless of the ref, so toHaveBeenNthCalledWith(2, USE_OPENAI, 'model-b', undefined) passes whether or not the clearing exists (probe-confirmed: commenting out the clearing at 3806 leaves this test green). — Concrete cost: removing the clearing ships a regression no test catches — after a successful forward A→B (sets the ref), a failed backward B→A leaves the stale ref, and the next Ctrl+F incorrectly switches back to A instead of "Already on B".
Suggested fix: add a test that first RESOLVES a forward toggle (so the ref is set), then REJECTS the backward toggle, then asserts the third Ctrl+F yields an "Already on" info message rather than a backward switchModel call, e.g. switchModelSpy.mockResolvedValueOnce(undefined).mockRejectedValueOnce(new Error('back failed')).
中文说明
这条注释声称该测试验证了错误路径清除了 previousModelRef，但这个断言无法区分"已清除"和"未清除"。previousModelRef 初始为 null（AppContainer.tsx:1027），且第一次切换失败，所以成功处理器（唯一会写入非 null 的地方，~3790）从不运行；错误处理器的 previousModelRef.current = null（~3806）对一个已为 null 的 ref 是空操作。第二次 Ctrl+F 是 forward，因为无论 ref 如何都有 currentModel !== target.modelId，所以 toHaveBeenNthCalledWith(2, USE_OPENAI, 'model-b', undefined) 无论清除是否存在都会通过（已用 probe 验证：注释掉 3806 处的清除，该测试仍为绿）。— 具体代价：删除清除会漏过一个没有测试能捕获的回归 —— 在一次成功的向前切换 A→B（设置 ref）之后，一次失败的向后切换 B→A 会留下过期 ref，下一次 Ctrl+F 会错误地切回 A 而不是显示 "Already on B"。
建议修复：增加一个测试，先让一次向前切换成功（RESOLVE，使 ref 被设置），再让向后切换失败（REJECT），然后断言第三次 Ctrl+F 产生一条 "Already on" 信息而不是向后调用 switchModel，例如 switchModelSpy.mockResolvedValueOnce(undefined).mockRejectedValueOnce(new Error('back failed'))。
— qwen3.8-max-preview via Qwen Code /review
|
Closing this PR. After refactoring we will open a fresh, clean PR per the CONTRIBUTING guidelines (smaller focused diff, no unrelated churn like package-lock.json / vitest.config.ts). All resolved review feedback will be incorporated. QwenCode QwenLLM |
5 participants
What this PR does
Adds a dynamic model toggle hotkey (Ctrl+F) that lets users switch between the current model and an alternate model configured via a new
model.toggleModelsetting. The toggle persists across turns and is reflected in the header, with a contextual info message (● Switched to <model>) appearing in the conversation history after each switch.A
previousModelReftracks the pre-toggle model so pressing the hotkey again restores the original. Manual model switches (via/modelcommand) invalidate the reference so the next toggle always goes to the configuredtoggleModel.Why it's needed
Users working with multiple models (e.g. a fast local model for quick iterations and a cloud model for complex reasoning) currently must run
/model <name>each time they want to switch — a multi-step flow that breaks concentration. A single blind keypress makes this instant.Reviewer Test Plan
How to verify
"model": { "toggleModel": "<alternate-model-id>" }to your confignpm run devand confirm the status bar shows the new shortcut (ctrl+f to toggle model)● Switched to <alternate-model-id>message appears● Switched to <original-model>appears/model <third-model>manually, then press the hotkey. Verify it switches totoggleModel, not the manually-set modelEvidence (Before & After)
Before: No model toggle hotkey. Users must run
/modelto switch models.After: Ctrl+F instantly toggles between current and alternate model.
Tested on
Risk & Scope
→for cursor movement.model.toggleModelis optional and defaults toundefined.Linked Issues
Fixes #6442
中文说明
这个 PR 做了什么
添加了一个动态模型切换快捷键（Ctrl+F），让用户可以在当前模型和新的
model.toggleModel设置中配置的备用模型之间切换。切换在轮次之间保持不变，并在标题栏中反映，每次切换后在对话历史中出现一条上下文信息消息（● Switched to <model>）。previousModelRef会记录切换前的模型，因此再次按下快捷键会恢复到原始模型。手动切换模型（通过/model命令）会使引用失效，因此下次切换总是会切换到已配置的toggleModel。为什么需要它
使用多个模型的用户（例如快速的本地模型用于快速迭代，云端模型用于复杂推理）目前每次想切换时都必须运行
/model <name>——这是一个会打断注意力且需要多步操作的流程。一键盲按使其瞬间完成。审阅者测试计划
如何验证
"model": { "toggleModel": "<alternate-model-id>" }添加到你的配置中npm run dev并确认状态栏显示新的快捷键（ctrl+f to toggle model）● Switched to <alternate-model-id>消息● Switched to <original-model>/model <third-model>，然后按下快捷键。验证它切换到toggleModel而不是手动设置的模型证据（前后对比）
之前： 没有模型切换快捷键。用户必须运行
/model来切换模型。之后： Ctrl+F 即时在当前模型和备用模型之间切换。
已测试
风险与范围
→进行光标移动。model.toggleModel是可选的，默认为undefined。关联问题
Fixes #6442
QwenCode QwenLLM