fix: adapt keyboard shortcut display for macOS - #2484
Conversation
|
@copilot resolve the merge conflicts in this pull request |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.4 via Qwen Code /review
Closes QwenLM#2227 On macOS, keyboard shortcut hints now use native modifier symbols (⌃ for Ctrl, ⌘ for Cmd, ⌥ for Alt, ⇧ for Shift) instead of the generic "ctrl+", "cmd+" text format. - Add formatShortcut() utility in shortcutFormatter.ts - Apply to KeyboardShortcuts panel and retry hint messages - Non-macOS platforms are unaffected
bc3eb5e to
aa8428d
Compare
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
|
@Br1an67 friendly ping — CI is currently red on all 9 Root cause: the PR branch is based on Fix: rebase (or merge) latest Thanks for the contribution! |
|
Reviewed the change. Direction is correct (native symbols on Mac, full no-op elsewhere keeps the blast radius tiny), but a few points worth addressing: Main issues1.
|
Dismissing the prior auto-LGTM: CI is failing on all platforms (build error) and a fresh review surfaced i18n / layout-width / minor issues that need addressing.
|
Two follow-ups on the review state: Dismissed the prior auto-LGTM — the earlier CI is red, but it's not from this change. The failing test is |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI has test failures (pre-existing base branch issues in coreToolScheduler.test.ts, not caused by this PR).
[Suggestion] Help.tsx (lines 110-155) still renders shortcuts as hardcoded strings (Ctrl+C, Ctrl+L, Ctrl+O, Alt+Left/Right) without going through formatShortcut. On macOS, the bottom shortcut bar will show symbols like ⌃C, while the Help dialog shows Ctrl+C for the exact same shortcuts — a user-visible inconsistency.
Suggested fix: also route Help.tsx shortcut keys through formatShortcut().
Overall: clean implementation, no blocking issues. The i18n coupling and layout-width concerns previously raised by @wenshao remain valid and are not repeated here. — Qwen Code /review
|
|
||
| return shortcut | ||
| .split(/\s+/) | ||
| .map((combo) => { |
There was a problem hiding this comment.
[Nice to have] combo.split('+') cannot represent the + key itself (e.g., ctrl++ for zoom-in would silently mangle). No current shortcut uses + as a key, but consider documenting this limitation or switching to a regex-based parser that matches known modifiers and leaves the remainder as the key.
— Qwen Code /review
| it('converts modifier keys to Mac symbols on darwin', async () => { | ||
| Object.defineProperty(process, 'platform', { value: 'darwin' }); | ||
| const { formatShortcut } = await import('./shortcutFormatter.js'); | ||
| expect(formatShortcut('ctrl+y')).toBe('⌃Y'); |
There was a problem hiding this comment.
[Nice to have] Consider adding test cases for combined modifiers (ctrl+shift+y → ⌃⇧Y), single-character keys (!, /, @), and mixed-case input (Ctrl+Y) to catch regressions if future shortcuts use these patterns.
— Qwen Code /review
|
@Br1an67 heads up — this PR currently has merge conflicts with Conflicting files:
The rest merges cleanly. Thanks! 中文@Br1an67 提个醒 —— 这个 PR 目前和 冲突文件:
其余文件可以自动合并。谢谢! |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Clean extraction of formatShortcut utility for macOS modifier symbols (⌃⌘⌥⇧). Dynamic import via vi.resetModules() correctly handles process.platform detection in tests. KeyboardShortcuts.tsx and useGeminiStream.ts properly consume the new formatter.
Downgraded from Approve to Comment: CI failing.
— claude-opus-4-6 via Qwen Code /review
E2E Tmux Test Report — PR #2484Tested on: Linux (Ubuntu, x86_64) | Branch: Test Results
DetailsTest 1 — UI component tests All 107 test files in the
Test 2 — CLI typecheck Both errors are PR Change Summary4 files changed, +102/-5:
VerdictPASS — All 1568 tests pass. The 2 typecheck warnings are pre-existing infrastructure issues unrelated to the PR changes. The new 中文测试报告E2E Tmux 测试报告 — PR #2484测试环境： Linux (Ubuntu, x86_64) | 分支： 测试结果
详情测试 1 — UI 组件测试
测试 2 — CLI 类型检查
结论通过 — 全部 1568 个测试通过。2 个类型检查警告为预先存在的基础设施问题，与本 PR 变更无关。新增的 |
|
@qwen-code /triage |
|
Thanks for the PR, @Br1an67! Template: all essential sections present (TLDR, Dive Deeper, Reviewer Test Plan, Testing Matrix, Linked Issues) ✓ Problem: real and well-documented. Issue #2227 includes a screenshot showing Direction: aligned with the project's platform-awareness goals. CLI tools respecting macOS keyboard symbol conventions is standard practice. No CHANGELOG precedent needed — this is a straightforward platform fix. Size: 102 additions, 5 deletions across 4 files. All changes in Approach: minimal and focused. A small Moving on to code review. 🔍 中文说明感谢 @Br1an67 的 PR！ 模板：所有必要章节齐全（TLDR、Dive Deeper、Reviewer Test Plan、Testing Matrix、Linked Issues）✓ 问题：真实且有据可查。Issue #2227 包含截图，显示 macOS 上显示 方向：符合项目的平台适配目标。CLI 工具遵循 macOS 键盘符号规范是标准做法。无需 CHANGELOG 先例——这是一个直接的平台修复。 规模：4 个文件，102 行新增、5 行删除。全部改动在 方案：精简聚焦。一个带平台守卫的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal: To show macOS keyboard symbols (⌃⌘⌥⇧) instead of text modifiers, I'd create a small platform-guarded formatter function, apply it at the shortcut display points, and mock Comparison with PR: The PR matches this approach exactly. Reuse check: no existing formatter for platform-specific keyboard display in the codebase. The utility is justified and not duplicated elsewhere. Findings:
No critical bugs, security issues, or AGENTS.md violations found. TestingUnit tests: 3/3 pass ✓ Runtime test (Linux): CLI starts and responds correctly with the PR code. On Linux, TUI rendering: the Ink/React TUI renders in the alternate screen buffer, which makes Merge conflicts: 中文说明代码审查独立方案： 为显示 macOS 键盘符号（⌃⌘⌥⇧），我会创建一个带平台守卫的格式化函数，在快捷键显示点应用，并在测试中 mock 与 PR 对比： PR 完全符合这个方案。 复用检查： 代码库中没有现有的平台特定键盘显示格式化器。这个工具函数是合理的且没有重复。 发现：
未发现关键 bug、安全问题或 AGENTS.md 违规。 测试单元测试： 3/3 通过 ✓ 运行时测试（Linux）： CLI 使用 PR 代码正常启动和响应。在 Linux 上， 合并冲突： — Qwen Code · qwen3.7-max |
|
This is a clean, focused fix that solves a real problem. The author saw that macOS users get The implementation is straightforward: a small My independent proposal matched the PR's approach exactly. I don't see a simpler path. The The only merge blocker is the unresolved merge conflict with Verdict: Code is good. Approving. 🟢 中文说明这是一个干净、专注的修复，解决了真实问题。作者发现 macOS 用户在 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. ✅
Note: merge conflicts with main need resolving before merge — please rebase when you get a chance.
4 participants
TLDR
On macOS, keyboard shortcut hints now use native modifier symbols (
⌃for Ctrl,⌘for Cmd,⌥for Alt,⇧for Shift) instead of the generic text format.Closes #2227
Dive Deeper
The CLI keyboard shortcut panel and error retry hints displayed shortcuts like
ctrl+y,cmd+vregardless of platform. On macOS, the convention is to use symbolic modifiers (⌃Y,⌘V).Changes:
packages/cli/src/ui/utils/shortcutFormatter.ts— NewformatShortcut()utility that converts modifier names to macOS symbols whenprocess.platform === "darwin"packages/cli/src/ui/components/KeyboardShortcuts.tsx— ApplyformatShortcut()to shortcut key displaypackages/cli/src/ui/hooks/useGeminiStream.ts— ApplyformatShortcut()to "Press Ctrl+Y to retry" hint messagespackages/cli/src/ui/utils/shortcutFormatter.test.ts— Tests for Mac symbol conversion, multi-part shortcuts, and non-Mac passthroughNon-macOS platforms are completely unaffected — the formatter is a no-op on Linux/Windows.
Reviewer Test Plan
⌃Y,⌃C,⌘Vetc.⌃Yinstead ofCtrl+Yctrl+y,ctrl+c)Testing Matrix
Linked issues / bugs
Closes #2227