fix(terminal): don't swallow Option+arrow/backspace on keyCode 229#956
fix(terminal): don't swallow Option+arrow/backspace on keyCode 229#956ayii0111 wants to merge 4 commits into
Conversation
macOS WKWebView mistags Option-modified keydowns with keyCode 229 (the code Chromium/WebKit use for IME composition), since Option is also the accent/dead-key modifier on macOS keyboards. The existing guard in attachCustomKeyEventHandler treated any keyCode 229 event as IME input and swallowed it before terminalWordNavigationSequence / terminalDeleteSequence ever ran, silently breaking Option+Left/Right word navigation and Option+Backspace. Arrow keys and Backspace can never legitimately be part of IME composition text, so exempt them from the keyCode 229 guard. Reproduced with a keydown listener on macOS 15 (Apple Silicon): Option+ArrowLeft/ArrowRight reports isComposing:false, keyCode:229.
📝 WalkthroughWalkthroughThe terminal key handler now maps Option+Z to a readline undo sequence and forwards it on keydown, while IME blocking is narrowed to composing events only. Repo notes and docs were expanded to describe the fork workflow, the terminal key issue, and build/release steps. ChangesTerminal key handling and repository notes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/modules/terminal/lib/rendererPool.ts (1)
254-259: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMinor:
isNavigationKeychecksevent.keyonly, unlike sibling helpers.
terminalWordNavigationSequence/terminalDeleteSequenceinkeymap.tsmatch onevent.key === "ArrowLeft" || event.code === "ArrowLeft"for resilience againstkeybeing masked. If WKWebView's mistagging also affectsevent.keyfor some inputs, this exemption could miss and reintroduce the bug it's fixing. Given the PR's manual validation this may be moot, but matching theevent.codefallback would keep the two checks consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/terminal/lib/rendererPool.ts` around lines 254 - 259, The `isNavigationKey` check in `rendererPool.ts` only looks at `event.key`, unlike the sibling helpers in `keymap.ts`, so it can miss masked key events. Update the navigation-key exemption logic to use the same `event.key` plus `event.code` fallback pattern used by `terminalWordNavigationSequence` and `terminalDeleteSequence`, keeping the handling consistent for `ArrowLeft`, `ArrowRight`, `ArrowUp`, `ArrowDown`, and `Backspace`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/terminal/lib/rendererPool.ts`:
- Around line 254-262: Move the keyCode-229 / composing gate out of the
attachCustomKeyEventHandler closure in rendererPool.ts and into keymap.ts as a
reusable predicate alongside terminalWordNavigationSequence and
terminalDeleteSequence. Add a focused unit test in keymap.test.ts for the new
helper (for example isImeComposition or equivalent) covering the
ArrowLeft/ArrowRight/ArrowUp/ArrowDown/Backspace exemption and the isComposing
case so the terminal input invariant is locked without needing a Terminal or
Slot.
- Around line 250-253: The comment in rendererPool.ts uses an em dash, which
violates the project text style rule. Update the affected explanatory comment
near the WKWebView/Option+arrow handling in rendererPool to replace the em dash
with a standard hyphen or rewrite the sentence without that punctuation, keeping
the meaning unchanged.
---
Nitpick comments:
In `@src/modules/terminal/lib/rendererPool.ts`:
- Around line 254-259: The `isNavigationKey` check in `rendererPool.ts` only
looks at `event.key`, unlike the sibling helpers in `keymap.ts`, so it can miss
masked key events. Update the navigation-key exemption logic to use the same
`event.key` plus `event.code` fallback pattern used by
`terminalWordNavigationSequence` and `terminalDeleteSequence`, keeping the
handling consistent for `ArrowLeft`, `ArrowRight`, `ArrowUp`, `ArrowDown`, and
`Backspace`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f0cbf6a4-1889-4a10-847b-0fab01043e05
📒 Files selected for processing (1)
src/modules/terminal/lib/rendererPool.ts
| const isNavigationKey = | ||
| event.key === "ArrowLeft" || | ||
| event.key === "ArrowRight" || | ||
| event.key === "ArrowUp" || | ||
| event.key === "ArrowDown" || | ||
| event.key === "Backspace"; | ||
| if (event.isComposing || (event.keyCode === 229 && !isNavigationKey)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Gating logic belongs in keymap.ts, and it's untested.
This keyCode-229 exemption is a pure predicate over KeyboardEvent, same shape as terminalWordNavigationSequence/terminalDeleteSequence in keymap.ts, but it's inlined in the attachCustomKeyEventHandler closure instead, and ships without a test. TERAX.md states core-subsystem terminal input changes need a test that locks the invariant, and this is exactly the kind of regression (macOS Option+word-nav silently swallowed) the existing keymap.test.ts suite exists to prevent for its neighbors.
Extracting an isImeComposition(event) (or similar) helper into keymap.ts would make it directly unit-testable (keyCode 229 + ArrowLeft/Backspace → not blocked; keyCode 229 + isComposing → blocked; keyCode 229 + arbitrary key → blocked) without needing to construct a Terminal/Slot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/terminal/lib/rendererPool.ts` around lines 254 - 262, Move the
keyCode-229 / composing gate out of the attachCustomKeyEventHandler closure in
rendererPool.ts and into keymap.ts as a reusable predicate alongside
terminalWordNavigationSequence and terminalDeleteSequence. Add a focused unit
test in keymap.test.ts for the new helper (for example isImeComposition or
equivalent) covering the ArrowLeft/ArrowRight/ArrowUp/ArrowDown/Backspace
exemption and the isComposing case so the terminal input invariant is locked
without needing a Terminal or Slot.
Source: Coding guidelines
This repo maintains a personal customization layer on top of upstream Terax, not a one-off fork. Document why (upstream won't provide certain fixes/features), how customizations get reproduced after upstream releases a new version (branch + rebase, FIX_NOTES.md per branch for root-cause context conflicts can't resolve on their own), and the origin/fork remote split - so a future agent with zero context can pick this up without re-deriving it.
…de 229 Option+Z now maps to readline's undo (Ctrl+_, \x1f) so an unsent command line edit can be undone without touching Cmd+Z (already the code editor's undo). Hits the same macOS dead-key mistagging as Option+arrow: WKWebView reports it as key "Dead" / keyCode 229, code "KeyZ" - matched on event.code like the existing word-navigation sequences. That surfaced a second, unrelated problem in the keyCode-229 IME guard in attachCustomKeyEventHandler: it swallowed *any* keyCode 229 keydown except a manually maintained exemption list (arrows/backspace, now Option+Z too). xterm's own CompositionHelper.keydown() already handles keyCode 229 correctly (calls _handleAnyTextareaChanges(), which diffs the textarea after a setTimeout and only forwards if a real composition didn't start in the meantime) - our guard ran first and swallowed the event before that logic ever got a chance to. Every future non-composing-but-229 key would have needed another manual exemption. Dropped the keyCode check down to just isComposing and let xterm's own logic own keyCode 229; confirmed multi-keystroke CJK composition (pinyin/zhuyin) still works. Does not fix: CJK IME direct-commit full-width punctuation (e.g. Shift+,) still occasionally drops the first keystroke. Root-caused to xterm.js's own _handleAnyTextareaChanges diffing (newValue.replace(oldValue, '') is not a real diff under rapid successive composition-flagged keydowns) - a bug in the @xterm/xterm dependency itself, not this wrapper. Left unpatched per request; see FIX_NOTES.md.
This repo is a long-lived customization layer on upstream Terax, not a one-shot PR branch, and now also serves as a cloud backup. Overwrite README.md with fork-specific docs (what's customized, why, remotes, rebase workflow) instead of upstream's generic project pitch. Bring FIX_NOTES.md (root-cause analysis, repro steps, dead ends) into version control so the backup actually includes it, not just the local checkout.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
FIX_NOTES.md (1)
1-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNormalize this note to the markdown rules.
This file still uses em dashes in the title/body, and the shell snippet should be labeled
bash. Please replace the punctuation with ASCII punctuation and add the fenced language tag.As per coding guidelines,
**/*.{ts,tsx,rs,md,css,json,sh,bash,zsh,ps1}: Do not use em dashes anywhere in code, comments, docs, or related text files. Markdownlint also expects fenced code blocks to declare a language.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@FIX_NOTES.md` around lines 1 - 68, The note in FIX_NOTES.md needs to be normalized to markdown rules by replacing all em dashes in the title and body with ASCII punctuation and by adding the bash language tag to the fenced shell snippet. Update the document text while keeping the same meaning, and make sure the code block that shows the console command is explicitly marked as bash so it satisfies markdownlint.Sources: Coding guidelines, Linters/SAST tools
🧹 Nitpick comments (2)
src/modules/terminal/lib/keymap.test.ts (1)
139-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for non-US physical layouts.
Good coverage for the dead-key mistag, but none of these cases exercise
code === "KeyZ"paired with a legitimate, non-"Dead"key value (e.g. AZERTY typingwat that physical position). If thekeymap.tsfix suggested above is applied, this test would lock the invariant that only the dead-key mistag triggers the code-based fallback.+ it("does not remap other layouts' KeyZ position (e.g. AZERTY 'w')", () => { + expect( + terminalUndoSequence(evt({ altKey: true, key: "w", code: "KeyZ" })), + ).toBeNull(); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/terminal/lib/keymap.test.ts` around lines 139 - 166, Add a regression test in terminalUndoSequence to cover a non-US physical layout case where code is "KeyZ" but key is a legitimate non-"Dead" value (for example, AZERTY-style input like "w"). Update the keymap.test.ts cases around terminalUndoSequence so the code-based fallback is only asserted for the macOS dead-key mistag, and verify a non-Dead key with code "KeyZ" does not incorrectly trigger the undo mapping.src/modules/terminal/lib/rendererPool.ts (1)
249-261: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment is longer than the project's "1-2 lines, why not what" convention.
The explanation is genuinely useful context (prevents re-introducing the swallowed-Option-arrow bug), but at 13 lines it exceeds the repo's stated comment convention. Consider trimming to the core "why" (isComposing is the only signal that's ours to check; keyCode 229 duplicates xterm's own CompositionHelper) and dropping the narrative detail.
As per coding guidelines, "Default to no comments; if truly needed, write 1-2 lines explaining why, never what, with no AI-generic filler."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/terminal/lib/rendererPool.ts` around lines 249 - 261, The comment in rendererPool.ts is too long and exceeds the project’s 1-2 line “why not what” convention. Trim the block near the keyCode 229 / isComposing logic to a concise explanation that only states why isComposing is the only signal to check and why keyCode 229 should not be swallowed again, keeping the reference to CompositionHelper.keydown() but removing the extra historical narrative.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 33: Replace the em dash in the CLAUDE.md guidance with a comma or plain
hyphen so it follows the repo markdown rule; update the sentence that mentions
TERAX.md and the related docs to use punctuation without em dashes, keeping the
meaning unchanged.
In `@README.md`:
- Around line 15-17: The customization list in README.md still uses em dashes in
the bullet text, which violates the repository markdown rule. Update those
bullets to use commas or plain hyphens instead of em dashes, keeping the same
meaning and preserving the existing list structure.
---
Outside diff comments:
In `@FIX_NOTES.md`:
- Around line 1-68: The note in FIX_NOTES.md needs to be normalized to markdown
rules by replacing all em dashes in the title and body with ASCII punctuation
and by adding the bash language tag to the fenced shell snippet. Update the
document text while keeping the same meaning, and make sure the code block that
shows the console command is explicitly marked as bash so it satisfies
markdownlint.
---
Nitpick comments:
In `@src/modules/terminal/lib/keymap.test.ts`:
- Around line 139-166: Add a regression test in terminalUndoSequence to cover a
non-US physical layout case where code is "KeyZ" but key is a legitimate
non-"Dead" value (for example, AZERTY-style input like "w"). Update the
keymap.test.ts cases around terminalUndoSequence so the code-based fallback is
only asserted for the macOS dead-key mistag, and verify a non-Dead key with code
"KeyZ" does not incorrectly trigger the undo mapping.
In `@src/modules/terminal/lib/rendererPool.ts`:
- Around line 249-261: The comment in rendererPool.ts is too long and exceeds
the project’s 1-2 line “why not what” convention. Trim the block near the
keyCode 229 / isComposing logic to a concise explanation that only states why
isComposing is the only signal to check and why keyCode 229 should not be
swallowed again, keeping the reference to CompositionHelper.keydown() but
removing the extra historical narrative.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d1aa437a-a4ed-4f6b-9f89-44715a33f0ce
📒 Files selected for processing (6)
CLAUDE.mdFIX_NOTES.mdREADME.mdsrc/modules/terminal/lib/keymap.test.tssrc/modules/terminal/lib/keymap.tssrc/modules/terminal/lib/rendererPool.ts
| - Commit message 要完整說明根因（不是只寫症狀），因為改版後可能要靠 commit log 反查當初為什麼這樣改。 | ||
| - 官方發新版後的標準流程：`git fetch origin`，把客製化分支 rebase 到新的 `origin/main`，解衝突，重新走一次「Commands」段落的檢查（lint / check-types / test / clippy / nextest），確認客製化修改在新版上還是成立，再重新 `pnpm tauri build`。 | ||
|
|
||
| `TERAX.md` at the repo root is the source of truth for upstream architecture, conventions, and the quality bar — read it before making changes. `docs/architecture/*.md` and `docs/contributing/testing.md` elaborate on specific subsystems without duplicating it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the em dashes from this guidance.
These lines violate the repo markdown rule for md files. Please swap them to commas or plain hyphens.
As per coding guidelines, **/*.{ts,tsx,rs,md,css,json,sh,bash,zsh,ps1}: Do not use em dashes anywhere in code, comments, docs, or related text files.
Also applies to: 51-51
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLAUDE.md` at line 33, Replace the em dash in the CLAUDE.md guidance with a
comma or plain hyphen so it follows the repo markdown rule; update the sentence
that mentions TERAX.md and the related docs to use punctuation without em
dashes, keeping the meaning unchanged.
Source: Coding guidelines
| - **修復 Option+方向鍵/Option+Backspace 在 macOS 上完全無反應**——macOS WKWebView 把 Option 修飾鍵誤標成 `keyCode 229`(IME 組字碼),原本的 IME 守衛把這些事件整個吞掉。已送官方 PR [#956](https://github.com/crynta/terax-ai/pull/956)。 | ||
| - **新增 Option+Z:對映到 shell readline 的 undo(`Ctrl+_`)**,用來撤銷終端機輸入行還沒送出的編輯。 | ||
| - **簡化 IME keyCode-229 守衛**:拿掉 Terax 自己重複、且更粗糙的 `keyCode === 229` 判斷,交還給 xterm.js 自己內建、更精確的 `CompositionHelper` 邏輯處理。 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the em dash from the customization list.
This bullet violates the repo markdown rule. Please switch it to commas or plain hyphens.
As per coding guidelines, **/*.{ts,tsx,rs,md,css,json,sh,bash,zsh,ps1}: Do not use em dashes anywhere in code, comments, docs, or related text files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 15 - 17, The customization list in README.md still
uses em dashes in the bullet text, which violates the repository markdown rule.
Update those bullets to use commas or plain hyphens instead of em dashes,
keeping the same meaning and preserving the existing list structure.
Source: Coding guidelines
1 participant
Summary
macOS WKWebView mistags Option-modified keydown events with
keyCode 229(the code Chromium/WebKit normally reserve for IME composition "Process" events). Since Option is also the accent/dead-key modifier on macOS keyboards, this fires for plain Option+Arrow and Option+Backspace too.The guard in
attachCustomKeyEventHandler(src/modules/terminal/lib/rendererPool.ts) treats anykeyCode === 229as IME input and returnsfalsebeforeterminalWordNavigationSequence/terminalLineNavigationSequence/terminalDeleteSequenceever run — silently breaking Option+Left/Right word navigation and Option+Backspace on macOS, even though those functions are implemented correctly (see #308, #493, #258).Repro
Attached a keydown listener directly in the terminal's WKWebView (macOS 15, Apple Silicon):
Pressing Option+ArrowLeft / Option+ArrowRight:
isComposingisfalse, butkeyCodeis229, so the existing guard swallows the event before the word-navigation logic runs.Fix
Arrow keys and Backspace can never legitimately be part of IME composition text, so exempt them from the
keyCode === 229guard while still fully honoringisComposingfor real IME sessions.Test plan
pnpm tauri buildand verified Option+Left/Right now jumps by word, Option+Backspace deletes a word, in a fresh terminal tabpnpm check-typescleanSummary by CodeRabbit