fix(cli): show ⌥T instead of alt+T on macOS for thinking expansion - #5802
Conversation
On macOS the Option key (⌥) maps to the meta keybinding, but the UI hint displayed "alt+t" which is confusing since Mac keyboards have no dedicated Alt key. Show the native ⌥ symbol on darwin so the on-screen hint matches the actual key users need to press.
| const durationSuffix = | ||
| durationMs != null ? ` ${formatDuration(durationMs)}` : ''; | ||
|
|
||
| const toggleKeyHint = process.platform === 'darwin' ? '\u2325+t' : 'alt+t'; |
There was a problem hiding this comment.
[Suggestion] The Unicode ⌥ (\u2325) symbol breaks the existing CLI key-hint convention. KeyboardShortcuts.tsx exclusively uses ASCII labels (alt+v, cmd+v, ctrl+o) — zero unicode modifier symbols anywhere in packages/cli/src/. This creates UI inconsistency: macOS users see ⌥+t in the thinking hint but alt+v in the keyboard shortcuts panel.
Additionally, the same toggleKeyHint expression is copy-pasted in 3 files (ConversationMessages.tsx, HistoryItemDisplay.test.tsx, ConversationMessages.test.tsx). If the hint text or platform logic ever changes, all 3 must be updated in lockstep.
Consider using alt+t on all platforms to match the existing convention, or extract this to a shared constant if introducing unicode symbols is intentional.
— qwen3.7-max via Qwen Code /review
| <Text dimColor italic> | ||
| {THINKING_ICON} | ||
| {label} {t('(alt+t to expand)')} | ||
| {label} {t(`(${toggleKeyHint} to expand)`)} |
There was a problem hiding this comment.
[Suggestion] The template literal inside t() produces two distinct translation keys per platform: "(⌥+t to expand)" on macOS and "(alt+t to expand)" elsewhere. When a translator later adds a locale entry for one key, the other platform silently falls back to English.
The t() function already supports {{param}} interpolation (i18n/index.ts:214-222). Use a stable key with a parameter:
| {label} {t(`(${toggleKeyHint} to expand)`)} | |
| {label} {t('({{keyHint}} to expand)', { keyHint: toggleKeyHint })} |
This way translators work with one key ("({{keyHint}} to expand)") and the platform-specific value is injected at render time. Apply the same fix to the collapse variant below.
— qwen3.7-max via Qwen Code /review
- Use ASCII 'option+t' instead of Unicode '⌥t' to match the existing
CLI key-hint convention (KeyboardShortcuts.tsx uses alt/cmd/ctrl
strings exclusively)
- Use t() parameter interpolation instead of template literals so the
translation key stays stable: t('({{keyHint}} to expand)', {keyHint})
rather than generating separate keys per platform
Signed-off-by: Alex <alex.tech.lab@outlook.com>
|
Thanks for the review, @wenshao! Both points are valid — addressed: **1. Unicode ⌥ symbol → ASCII ** You're right that uses ASCII exclusively for key hints (, , , etc.). I've changed the macOS hint from to to match the existing convention. No UI inconsistency now. 2. Dynamic translation keys → stable key with parameter interpolation Good catch. Changed from: t(`(${toggleKeyHint} to expand)`) // produces two different keys per platformto: t('({{keyHint}} to expand)', { keyHint: toggleKeyHint }) // single stable keyTranslators now work with one key and the platform-specific value is injected at render time. Applied to both expand and collapse variants. All 35 tests pass. |
| const durationSuffix = | ||
| durationMs != null ? ` ${formatDuration(durationMs)}` : ''; | ||
|
|
||
| const toggleKeyHint = process.platform === 'darwin' ? 'option+t' : 'alt+t'; |
There was a problem hiding this comment.
[Suggestion] toggleKeyHint is declared inside the ThinkMessage component body and recomputed on every render. Both test files and KeyboardShortcuts.tsx define platform-aware key helpers (getNewlineKey, getPasteKey, etc.) at module scope.
| const toggleKeyHint = process.platform === 'darwin' ? 'option+t' : 'alt+t'; |
Consider moving to module scope (e.g., right after THINKING_ICON) for consistency:
const toggleKeyHint = process.platform === 'darwin' ? 'option+t' : 'alt+t';— qwen3.7-max via Qwen Code /review
| import { render } from 'ink-testing-library'; | ||
| import { ThinkMessage, ThinkMessageContent } from './ConversationMessages.js'; | ||
|
|
||
| const toggleKeyHint = process.platform === 'darwin' ? 'option+t' : 'alt+t'; |
There was a problem hiding this comment.
[Suggestion] Tests mirror the component's process.platform conditional, so only the host platform's branch is ever exercised in CI. Other tests in the project (e.g., detect-terminal-theme.test.ts, clipboardUtils.test.ts) mock process.platform to cover both branches.
Consider adding a test that stubs process.platform to the opposite value and asserts the other key hint appears. This would catch a regression that hardcodes the wrong key on the non-host platform.
Additionally, the "should render full text when committed and expanded" test (line 37) asserts content is shown but doesn't verify the collapse hint text (to collapse). The collapse interpolation at ConversationMessages.tsx:376 is untested in this direct unit test — adding expect(output).toContain(\${toggleKeyHint} to collapse`)` would close that gap.
— qwen3.7-max via Qwen Code /review
| useMouseEvents: vi.fn(), | ||
| })); | ||
|
|
||
| const toggleKeyHint = process.platform === 'darwin' ? 'option+t' : 'alt+t'; |
There was a problem hiding this comment.
[Suggestion] Same cross-platform test gap as ConversationMessages.test.tsx — this constant mirrors the component's conditional, so only the host platform's branch is exercised. Consider mocking process.platform to verify both branches.
— qwen3.7-max via Qwen Code /review
Placed next to THINKING_ICON constant for consistency with KeyboardShortcuts.tsx patterns (getNewlineKey, getPasteKey at module scope). Avoids recomputing on every render. Signed-off-by: Alex <alex.tech.lab@outlook.com>
|
Thanks for the second round of review, @wenshao! 1. Move toggleKeyHint to module scope ✅ Done — placed right after 2. Cross-platform test coverage
Given that 3. Collapse text assertion ✅ The |
|
|
||
| export const THINKING_ICON = '∴ '; | ||
|
|
||
| const toggleKeyHint = process.platform === 'darwin' ? 'option+t' : 'alt+t'; |
There was a problem hiding this comment.
[Suggestion] toggleKeyHint is independently defined in 3 files — this source file plus both test files (ConversationMessages.test.tsx:10 and HistoryItemDisplay.test.tsx:32) each copy-paste the identical process.platform === 'darwin' ? 'option+t' : 'alt+t' expression. If the platform logic or label ever changes, it must be updated in 3 places, with silent divergence risk (a test file could pass on one platform and fail on another with a stale expectation).
Consider exporting the constant from this module (same pattern as THINKING_ICON) and importing it in the test files:
| const toggleKeyHint = process.platform === 'darwin' ? 'option+t' : 'alt+t'; | |
| export const toggleKeyHint = process.platform === 'darwin' ? 'option+t' : 'alt+t'; |
Then in both test files:
import { toggleKeyHint } from './messages/ConversationMessages.js';— qwen3.7-max via Qwen Code /review
Eliminates copy-paste duplication across 3 files. Tests now import from the source module (same pattern as THINKING_ICON), so any future change to the platform logic or label text only needs to be made once. Signed-off-by: Alex <alex.tech.lab@outlook.com>
|
Good catch, @wenshao. Exported from and both test files now import it instead of copy-pasting the ternary. Single source of truth — future changes only need one edit. All 35 tests pass. |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
✅ No issues found — clean, well-scoped display-string fix. LGTM.
Verified at HEAD b82e70c: all three concerns from the earlier review rounds are resolved — ASCII option+t/alt+t (no Unicode), the constant lives at module scope, and it's a single exported source of truth imported by both test files.
The i18n change to t('({{keyHint}} to expand)', { keyHint }) is correct:
- It uses the established
{{var}}interpolation pattern already used across the codebase (t('Found {{count}} ...', { count }), etc.). - No
check-i18nfailure — the script only flags unuseden.jskeys as a warning and never errors on a source key absent fromen.js; this hint was never anen.jskey (neither before nor after). - No translation regression — the hint was never translated in any locale, so both the old and new keys fall back to the English source string identically.
- The
{' '}on the collapse line correctly preserves the space across the JSX line-wrap (without it, JSX would collapse the newline and drop the space).
Ran the two affected test files (ConversationMessages.test.tsx, HistoryItemDisplay.test.tsx): 35/35 pass.
中文版
✅ 未发现问题 —— 一个干净、范围明确的显示文案修复。可以合并。
在 HEAD b82e70c 上核验：前几轮 review 提出的三个问题都已解决 —— 改用 ASCII option+t/alt+t（不再用 Unicode）、常量提到模块作用域、并导出为唯一来源由两个测试文件 import。
i18n 改动 t('({{keyHint}} to expand)', { keyHint }) 是正确的：
- 使用了代码库中既有的
{{var}}插值模式（如t('Found {{count}} ...', { count })）。 - 不会导致
check-i18n失败 —— 该脚本只把「en.js里未被使用的 key」报为 warning，从不对「源码中存在但en.js没有的 key」报错；而这个提示文案本来就不是en.js的 key（改动前后都不是）。 - 没有翻译回退退化 —— 该提示从未在任何语言包里被翻译过，新旧 key 都同样回退到英文源串。
- collapse 行的
{' '}正确地在 JSX 换行处保留了空格（否则 JSX 会折叠换行并丢掉空格）。
已运行两个受影响的测试文件（ConversationMessages.test.tsx、HistoryItemDisplay.test.tsx）：35/35 通过。
— claude-opus-4-8 via Claude Code /qreview
✅ Maintainer local verification — PR #5802Built and tested the PR head ( Results
Real rendered outputCaptured through
Findings1.
Both
2. ✅ i18n is correct, but the hint stays English-only (not a regression) The new keys 3. ✅ Tests are platform-adaptive and guard the leak Both test files import the shared 4. ℹ️ Pre-existing (not changed by this PR): the binding only fires on Option when the terminal maps Option→Meta
5. ✅ Non-macOS unchanged — Linux/Windows still render RecommendationFunctionally safe and every gate is green. Recommend merging once the 🔁 Reproductiongit fetch origin pull/5802/head:pr-5802
git worktree add ../wt-pr5802 pr-5802
cd ../wt-pr5802 && npm ci && npm --prefix packages/core run build
# unit tests (the two changed files)
( cd packages/cli && npx vitest run \
src/ui/components/messages/ConversationMessages.test.tsx \
src/ui/components/HistoryItemDisplay.test.tsx )
# gates
( cd packages/cli && npx tsc --noEmit )
npm run check-i18n
npx prettier --check packages/cli/src/ui/components/messages/ConversationMessages.tsx
npm run build && npm run bundle && node dist/cli.js --version🇨🇳 中文版（完整对应）✅ 维护者本地验证 — PR #5802在独立 worktree（全新 结果
真实渲染输出通过
发现1.
2. ✅ i18n 实现正确,但提示仍为纯英文(非回归) 新增的 key 3. ✅ 测试随平台自适应,并能挡住占位符泄漏 两个测试文件都导入共享的 4. ℹ️ 既有问题(非本 PR 引入):仅当终端把 Option 映射为 Meta 时,该绑定才会触发
5. ✅ 非 macOS 不变 —— Linux/Windows 仍渲染 建议功能上安全,所有关卡全绿。建议在对齐 Verified locally by the maintainer on macOS. Static gates + 35 unit tests green; the only open item is the |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
3 participants
What this PR does
Changes the thinking block expand/collapse keyboard shortcut hint from "alt+t" to "⌥t" on macOS, so the on-screen hint matches the actual key users need to press.
Why it's needed
The keybinding is defined as
{ key: 't', meta: true }, which maps to the Option (⌥) key on Mac. However, the UI always displayed "alt+t to expand/collapse" — macOS keyboards have no dedicated Alt key, so Mac users never knew which key to press. This PR detectsprocess.platform === 'darwin'and shows the native ⌥ symbol instead.Reviewer Test Plan
How to verify
npm run devEvidence (Before & After)
Before:
(alt+t to expand)— confusing on Mac, no Alt key existsAfter:
(⌥t to expand)— matches the actual Option key on Mac keyboardsTested on
Risk & Scope
Linked Issues
N/A
中文说明
这个 PR 做了什么
在 macOS 上将思考块的展开/折叠快捷键提示从 "alt+t" 改为 "⌥t"，使界面提示与实际需要按的键一致。
为什么需要
快捷键定义为
{ key: 't', meta: true }，在 Mac 上对应 Option (⌥) 键。但界面始终显示 "alt+t"，而 Mac 键盘没有独立的 Alt 键，导致 Mac 用户不知道按哪个键。此 PR 检测process.platform === 'darwin'并显示原生的 ⌥ 符号。审查者验证方案
如何验证
npm run dev证据（Before & After）
Before:
(alt+t to expand)— Mac 上令人困惑，不存在 Alt 键After:
(⌥t to expand)— 与 Mac 键盘上的 Option 键匹配风险与范围