fix(ui): distinguish auto approval mode indicators#4600
Conversation
| ) : approvalModePromptStyle ? ( | ||
| approvalModePromptStyle.prefix | ||
| ) : ( | ||
| '>' |
There was a problem hiding this comment.
[Suggestion] The : '>' fallback here is dead code. getApprovalModePromptStyle() always returns at least { prefix: '>' }, so approvalModePromptStyle is always truthy when this branch is reached (shell mode is handled above). The fallback can never execute.
| '>' | |
| ) : ( | |
| approvalModePromptStyle.prefix | |
| )}{' '} |
— qwen3.7-max via Qwen Code /review
| } from './approvalModeVisuals.js'; | ||
|
|
||
| describe('approval mode visuals', () => { | ||
| it('uses distinct colors for auto-edit and classifier auto mode', () => { |
There was a problem hiding this comment.
[Suggestion] Several ApprovalMode branches are untested:
getApprovalModeIndicatorColor: missing assertions forPLAN(theme.status.success),YOLO(theme.status.error), andDEFAULT(undefined)getApprovalModePromptStyle: thePLAN/DEFAULT/defaultfallthrough (which returns{ prefix: '>' }with no color/label) is completely untested
Adding these would guard against accidental styling regressions, especially for the PLAN/DEFAULT path which is the exact code path that changes behavior in AgentComposer.
— qwen3.7-max via Qwen Code /review
| } else if (approvalModePromptStyle.label === 'edits') { | ||
| statusText = t('Accepting edits'); | ||
| } else if (approvalModePromptStyle.label === 'auto') { | ||
| statusText = t('Auto mode'); |
There was a problem hiding this comment.
[Suggestion] The 'Auto mode' key is missing from all locale files under packages/cli/src/i18n/locales/ (zh, fr, ca, ja, etc.) and from mustTranslateKeys.ts. All peer status-text keys ('Shell mode', 'YOLO mode', 'Accepting edits') are properly translated. Non-English users will see the raw English string "Auto mode" in the prompt status area.
— qwen3.7-max via Qwen Code /review
ReviewSolid change, right direction. It pulls the approval-mode color logic that was duplicated across 3 components into a shared 1. [Should fix] The new
|
| key | en.js | zh.js |
|---|---|---|
'Shell mode' |
✅ | Shell 模式 |
'YOLO mode' |
✅ | YOLO 模式 |
'Accepting edits' |
✅ | 接受编辑 |
'Auto mode' |
❌ missing | ❌ missing |
So non-English users will see the literal English "Auto mode" in the input prompt while every other mode is localized. Suggest adding the key to at least en.js + zh.js, ideally aligning the other 8 locales too.
2. [Minor / author's call] AUTO's prompt color is full-intensity while its siblings are dimmed
In getApprovalModePromptStyle: AUTO_EDIT → warningDim, YOLO → errorDim, but AUTO → theme.text.link (full AccentBlue, not a dim variant). There is no AccentBlueDim/linkDim token in the codebase.
The result is that the AUTO input-prompt border/prefix renders noticeably brighter than the other two. The footer indicator uses full colors for all four modes (consistent there), but the input-prompt border convention is the dim variant.
I would not add a whole dim-blue token just for this (it'd touch ~15 theme files — overkill). But it's worth a conscious decision: accept the brightness difference, or use a slightly darker blue for the prompt. A note in the PR would help.
3. [Confirm intent] AgentComposer's PLAN-mode border behavior changed (not mentioned in the description)
The old AgentComposer logic was isAutoAccept = mode !== DEFAULT, so PLAN mode also got a warningDim (yellow) border. The new code gives PLAN no color, so it falls back to theme.border.focused.
Since APPROVAL_MODES = Object.values(ApprovalMode) includes PLAN and the agent view can cycle into it, the PLAN border genuinely changes from "dim yellow" to "default focused". This actually aligns AgentComposer with the main InputPrompt (which also gives PLAN no special color), so it's arguably a fix rather than a regression — but since the description doesn't mention it, please confirm it's intentional.
Fine as-is (no change needed)
The label: 'edits' | 'auto' | 'yolo' indirection (helper returns a label, the component maps it back to t()) looks a little roundabout, but since the i18n extractor needs literal t('...') strings, keeping the t() calls in the component and using label as a discriminator is reasonable. No refactor needed.
Verdict: Mergeable — core logic and tests are correct. The only thing that really should be addressed is the #1 i18n key; #2 and #3 just need a confirmation of the trade-off / intent.
中文
Review
方向正确,质量不错。把原本分散在 3 个组件里的审批模式配色逻辑抽到了共享模块 approvalModeVisuals.ts,AUTO 模式现在用蓝色(text.link)和 AUTO_EDIT 的黄色区分开,正好对上 #4575 的诉求,还补了测试覆盖。下面按优先级列问题。
1.【应修复】新增的 t('Auto mode') 没有任何 i18n 翻译
InputPrompt.tsx 引入了 t('Auto mode'),但这个 key 在所有 locale 文件里都不存在(包括 en.js)。而它的同类状态标签都是有翻译的:
| key | en.js | zh.js |
|---|---|---|
'Shell mode' |
✅ | Shell 模式 |
'YOLO mode' |
✅ | YOLO 模式 |
'Accepting edits' |
✅ | 接受编辑 |
'Auto mode' |
❌ 缺失 | ❌ 缺失 |
后果:中文/日文等用户在输入框看到的会是英文 "Auto mode",而其它模式都是本地化的,不一致。建议至少补 en.js + zh.js,最好对齐其余 8 个 locale。
2.【小问题,作者拍板】AUTO 的输入框配色用了全亮度,与同类 dim 不一致
getApprovalModePromptStyle 里:AUTO_EDIT → warningDim、YOLO → errorDim,唯独 AUTO → theme.text.link(全亮 AccentBlue,不是 dim 变体)。代码库里并没有 AccentBlueDim/linkDim 这个 token。
结果是 AUTO 模式的输入框边框/前缀会比另外两个明显更亮。footer 指示器那边四个模式都用全色是一致的,但输入框边框的既有惯例是用 dim 色。
我不建议为此新增一整套 dim 蓝 token(要改 ~15 个主题文件,过度了)。但作者应该有意识地接受这个亮度差异,或者在 prompt 里用个稍暗的蓝。在 PR 里说明一下取舍会更好。
3.【需确认意图】AgentComposer 对 PLAN 模式的边框行为变了(描述未提)
旧 AgentComposer 逻辑是 isAutoAccept = mode !== DEFAULT,所以 PLAN 模式也会拿到 warningDim 黄边;新代码里 PLAN 无 color,回退到 theme.border.focused。
由于 APPROVAL_MODES = Object.values(ApprovalMode) 包含 PLAN,agent view 能循环切到 PLAN,所以这个边框确实从"黄-dim"变成了"默认 focused 边"。这其实让 AgentComposer 和主 InputPrompt 对齐了(主 InputPrompt 对 PLAN 同样不特殊上色),算修正而非回归——但描述没提,建议确认是有意为之。
可以不动的(避免过度设计)
label: 'edits' | 'auto' | 'yolo' 这层间接(helper 返回 label、组件再映射回 t())初看有点绕,但因为 i18n 提取工具需要 t('字面量'),把 t() 调用留在组件、用 label 当判别器是合理做法,不必重构。
结论: 可以合 —— 核心逻辑和测试都正确。唯一真正该补的是 #1 的 i18n key;#2、#3 让作者确认取舍/意图即可。
Follow-up (KISS): the
|
Maintainer Verification ReportReviewer: @penwenshao (local build & test) 1. TypeScript Type Check
2. Lint
3. Unit Tests
Total: 153 tests passed, 0 failed 4. Code ReviewDesign: ✅ Clean extraction
Visual mapping correctness:
Test coverage: ✅
Return type: The Verdict: ✅ Ready to mergeFocused, well-scoped fix with proper test coverage. Resolves #4575 by giving classifier |
|
Updated the branch for the review feedback: added the missing Auto mode translation key across the existing locale files, removed the label field from the shared approval-mode visual helper, and kept the status-text mapping local to InputPrompt. I left the PLAN border behavior as-is because this PR is intentionally aligning AgentComposer with the existing InputPrompt behavior. Validation run locally:
|
| : isAutoAccept | ||
| ? theme.status.warningDim | ||
| : undefined; | ||
| const approvalModePromptStyle = getApprovalModePromptStyle(agentApprovalMode); |
There was a problem hiding this comment.
[Suggestion] PLAN mode silently loses visual styling in sub-agent views — behavioral regression.
The old code used agentApprovalMode !== ApprovalMode.DEFAULT as the guard, which gave PLAN mode theme.status.warningDim for both border and prefix. The new getApprovalModePromptStyle(PLAN) returns { prefix: '>' } with no color, making PLAN visually identical to DEFAULT in AgentComposer. Meanwhile, AutoAcceptIndicator still shows PLAN as green (theme.status.success), creating conflicting visual signals.
If this is intentional (PLAN shouldn't have warning-style coloring), consider adding a test that asserts getApprovalModePromptStyle(ApprovalMode.PLAN) has no color to lock in the intent. Otherwise, add PLAN to the shared function:
| const approvalModePromptStyle = getApprovalModePromptStyle(agentApprovalMode); | |
| const approvalModePromptStyle = getApprovalModePromptStyle(agentApprovalMode); | |
| const statusColor = approvalModePromptStyle.color; | |
| // Note: PLAN mode now renders identically to DEFAULT — confirm this is intended. |
— qwen3.7-max via Qwen Code /review
| case ApprovalMode.AUTO_EDIT: | ||
| return { color: theme.status.warningDim, prefix: '>' }; | ||
| case ApprovalMode.AUTO: | ||
| return { color: theme.text.link, prefix: '>' }; |
There was a problem hiding this comment.
[Suggestion] AUTO uses full-brightness theme.text.link (AccentBlue) for the prompt, breaking the dim-for-prompt / bright-for-indicator pattern and colliding with border.focused.
Every other mode uses dim tokens for prompts: AUTO_EDIT → status.warningDim (AccentYellowDim), YOLO → status.errorDim (AccentRedDim). AUTO uses text.link which resolves to the same AccentBlue as border.focused across all three themes. This makes the AUTO-mode input border visually indistinguishable from the DEFAULT-mode focused border.
Consider adding an AccentBlueDim token to the theme palette and using it here for consistency:
case ApprovalMode.AUTO:
return { color: theme.status.infoDim /* new AccentBlueDim token */, prefix: '>' };— qwen3.7-max via Qwen Code /review
| @@ -0,0 +1,44 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2025 Google LLC | |||
There was a problem hiding this comment.
Copyright header says "Google LLC" — should be "Qwen Team" to match the rest of the project (e.g. authMethods.ts).
| @@ -0,0 +1,42 @@ | |||
| /** | |||
| * @license | |||
| * Copyright 2025 Google LLC | |||
There was a problem hiding this comment.
Same here — "Google LLC" → "Qwen Team".
|
This PR changes user-visible TUI components (InputPrompt, AutoAcceptIndicator, AgentComposer) and i18n strings — could you add a before/after screenshot or short terminal recording showing the old vs new indicator styling? Per our PR template, UI changes need visual evidence so reviewers can verify without pulling locally. The test coverage for the mapping logic looks fine, but a quick screenshot of the actual rendered difference would help a lot here. |
|
Added a before/after terminal-style visual for the approval-mode indicators:
This compares the previous upstream/main mapping with the current PR branch mapping under the default dark theme. The visible difference is that AUTO no longer shares the warning / warningDim path with AUTO_EDIT; the footer indicator and prompt styling now use ext.link blue for AUTO, while AUTO_EDIT remains yellow and YOLO remains red. |
|
Fixed the two copyright headers pointed out in review: Validation:
|
wenshao
left a comment
There was a problem hiding this comment.
No new issues found in this round. LGTM! ✅ — qwen3.7-max via Qwen Code /review
Verification Report — PR #4600Commit: Test Results
CLI Typecheck NoteThe Test File Breakdown
Execution Environment
VerdictAll checks pass for the PR's own scope. 153 tests cover InputPrompt and approvalModeVisuals with no regressions. The CLI typecheck error is a cross-branch dependency on PR #4572, not a defect in this PR. Recommend merging after or together with PR #4572. |
tanzhenxin
left a comment
There was a problem hiding this comment.
Approving — clean, parity-preserving refactor that distinguishes the auto-approval mode indicators as intended, and full CI is green. Thanks @he-yufeng!
QwenLM#4600 unified the approval-mode prompt styling into approvalModePromptStyle and removed the showAutoAcceptStyling / showYoloStyling constants, but left a dangling showYoloStyling reference in the prefixWidth calculation. This broke the tsc build (TS2304: Cannot find name showYoloStyling). npm run bundle (esbuild) skips type-checking, so the break only surfaced under npm run build / tsc --build. Replace it with approvalMode === ApprovalMode.YOLO, matching how the rest of the file branches on approval mode after QwenLM#4600. Both ternary arms stay 2 because every approval-mode prefix is a single char (getApprovalModePromptStyle returns prefix > or *), so the rendered prefix width and cursor positioning are unchanged.
Summary
auto-accept editson warning/yellow, but render classifierauto modewith the link/blue colorFixes #4575
To verify