Repository navigation
fix(terminal): let Linux IMEs keep the candidate key for a preedit they own - #22607
Conversation
…ey own Sogou on fcitx draws its preedit and candidate list in its own window and opens no Chromium composition session, so every composition-scoped candidate guard is idle when the user presses Space or a candidate digit. The selector reached the PTY as literal text and the commit arrived after it. The marker the input framework does still deliver is the claimed letter keydown - `keyCode 229` / `key 'Process'` over the physical key. A bounded window armed from that claims the next Space or digit, and stands down for a real composition session, for the commit, and on expiry. Co-authored-by: nxiaobai <48705519+nxiaobai@users.noreply.github.com> Co-authored-by: Big-ios <18501866+Big-ios@users.noreply.github.com> Co-authored-by: cporoske <14369468+cporoske@users.noreply.github.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change adds a Linux IME-owned preedit guard that recognizes unmodified claimed letter keydowns and briefly classifies a following Space or digit as a candidate selector. The terminal keyboard policy suppresses matching selectors and prevents their native default action, except during Hangul preedit. Unit tests cover guard behavior and exclusions. An end-to-end test verifies candidate selection and the resulting PTY input. Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The selector guard appears ready for normal merge checks. Native fcitx/Sogou behavior has not yet been verified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new window is released on any keydown that is neither a selector nor a Key[A-Z] letter, so a keyCode 229 / 'Process' keydown the IME did claim for candidate navigation or preedit editing (Arrow*, Backspace, Home/End) disarms it and re-exposes the Space/digit this fix captures. Details inline.
Reviewed changes
- New claimed-keydown signal —
terminal-ime-linux-candidate-state.tsarms a 1500 ms window from akeyCode 229/'Process'letter keydown and classifies the next Space or candidate digit as IME-owned for a preedit that opens no composition session. - Policy wiring —
xterm-bypass-policy.tsaddslinuxImeOwnedPreeditGuardActiveto the suppression andpreventDefaultdecisions viaclaimsImeOwnedPreeditSelector(Space included,hangulPreeditopted out);terminal-pane-pane-input.tsthreads the classification through. - Tests — a unit spec for the state machine and an Electron e2e spec asserting the exact PTY bytes, which the PR states fails on
main.
ℹ️ Answers to the review questions
The hangulPreedit opt-out for Space looks correct: on Linux isHangulPreedit() is only ever true off a compositionupdate preedit (terminal-ime-composition-tracker.ts:50-53, 95), and the recorded ibus-hangul flow in terminal-ime-hangul-terminating-digit.test.ts:74-80 always runs a composition session, so 2-Set's syllable-terminating Space/digit are owned by the composition guards rather than by this window. The per-claimed-preedit one-shot also seems right: a fresh letter keydown re-arms, and re-arming per selector would only widen the over-claim surface for a multi-page list.
Technical details
# Answers and the release-path asymmetry
## Affected sites
- `terminal-ime-linux-candidate-state.ts:197-202` — one-shot release on a claimed selector.
- `terminal-ime-linux-candidate-state.ts:261, 264-269` — `compositionstart` and non-`insertCompositionText` `input` release paths.
## Required outcome
- None for the questions above; the inline finding is the only change requested.
## Notes
- The commit `input` from the recorded ibus trace is `insertText`, which `releaseOnCommit` (line 264-269) honors, so the composition-session path self-heals and does not need a `compositionend` reset for that shape.
- Because a claimed selector both suppresses and `preventDefault`s and then arms the pending-release guard, the selector keyup is also covered — no follow-up needed there.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…ts it The window released on any keydown that was not a letter the IME had claimed, but the IME claims the keys that edit and page a preedit too — Backspace, the arrows, `-`/`=` for the next candidate page — and Chromium keeps the original `code` on those 229/Process keydowns. Paging to a later candidate therefore disarmed the window and the digit that picked from that page reached the PTY. A claimed keydown now refreshes the deadline instead of clearing it. Arming still requires a claimed letter, so a bare navigation key with no preedit behind it cannot open the window from cold.
…ends An engine that does run a composition session still emits the claimed `keyCode 229` letter keydowns that arm this window, and both its commit and its cancel travel as `insertCompositionText` — the one input type the commit release deliberately ignores. Nothing else closed the window, so a cancelled preedit left it open and the next literal Space was swallowed. Refreshing the deadline on every claimed keydown had widened that tail. `compositionend` now releases it, as `compositionstart` already did. The engines this guard exists for emit neither event, so their path is unchanged. The selector suite drives the state object directly and cannot see a missing listener, so the release wiring gets its own DOM-level suite.
The regression the compositionend release fixes needs the whole session to show up: compositionstart fires once, so every letter after the first re-arms the window, the commit and the cancel both travel as insertCompositionText, and the committing Space arrives claimed rather than bare. Only the end of the session closes the window before the user's next literal Space. Fails with the compositionend listener removed.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The incremental delta (8bc3f58) resolves the prior review's inline finding that a claimed non-letter keydown disarmed the window.
- Predicate split — extracted
isImeClaimedKeydown(keyCode 229/key 'Process', unmodified) and rebuiltisImeOwnedLetterKeydownon top of it, so "claimed" and "claimed over a letter" are separable. - Refresh instead of disarm — a claimed non-letter keydown (
Backspace, arrows,-/=,PageUp/PageDown) now extends an already-liveimeOwnedPreeditUntil; arming still requires a claimedKey[A-Z], so a cold navigation key cannot open the window. - Regression tests — new cases cover claimed editing/paging keys keeping the window open, multi-page browsing past one window length, and the cold-navigation negative, each with exact assertions that fail without the change.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since the prior review adds one release path for composition-session IMEs and its wiring test.
- Release on
compositionend—installTerminalImeLinuxCandidateStatenow tears the claimed window down at both composition boundaries (releaseToCompositionrenamedreleaseToCompositionSession). Rationale: an engine that runs a composition session still emits the claimed keydowns that arm the window, and both its commit and its cancel travel asinsertCompositionText, whichreleaseOnCommitdeliberately ignores; without thecompositionendrelease the window outlived a cancelled preedit and swallowed the next literal Space. - Wiring test —
terminal-ime-linux-claimed-preedit-release.test.ts(happy-dom) drives the installed DOM listeners rather than the bare state object:compositionstart/compositionendrelease, preedit-text vs. non-preedit-textinput, disposal removing listeners, and the full Space-committing session shape. All six cases pass locally; thecompositionend/disposal cases fail without the listener.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
… took A claimed selector means the engine picked the candidate itself, so the round is over. It reads `key: 'Process'`, which the selector predicate cannot see, so this asks the physical code instead. Belt and braces for an engine that opens a composition session and never closes it — a documented defect in at least one shipped input framework. Without a `compositionend` the session-end release never runs, and the refresh added for candidate paging would otherwise hold the window open across the commit and swallow the user's next literal Space. Paging keys still refresh, so a long candidate browse is unaffected.
Japanese and Zhuyin commit with Enter, not Space, and Escape cancels. Both arrived claimed and fell into the refresh branch, so for an engine that opens a composition session and never closes it they held the window across the commit — the same defect the Space case was written to close, reached by a different key. Arrows, `-`/`=`, Backspace and PageUp/PageDown still refresh, because those page the candidate list rather than ending the round. Physical codes rather than `key`: `key` reads `Process` on all of them, and `Digit*` survives AZERTY, Dvorak and Colemak moving the digit row. Also corrects the window comment, which still described the orphan-keyup picking window. Since claimed keys refresh it, it is now a stuck-state valve that only expires when the IME stops reporting.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The incremental delta (00f2002) closes the residual leak for a composition-session IME that never fires compositionend: a selector the IME took itself now ends the window instead of extending it.
- Claimed selector ends the window — a
keyCode 229/key 'Process'keydown whose physicalcodeisSpace,Digit[0-9]orNumpad[0-9]now clearsimeOwnedPreeditUntilrather than refreshing it, so the next literal Space/digit is no longer swallowed when a session-running engine (one that skipscompositionend) took the pick itself. The physicalcodeis the only usable signal becausekeyreadsProcessthere. - Tests —
terminal-ime-linux-claimed-preedit-selector.test.tsgains a "composition session that never ends" block:Space/Digit2/Numpad2end the window, andEqualstill pages it. The new ending cases fail without the ternary; all 24 cases across the two specs pass locally.
I checked whether ending on a claimed Shift+Space (fcitx's width toggle) regresses the picking window: it does not. The pre-existing unclaimed-key branch at line 234 already ends the window for any non-selector keydown, and isTerminalImeCandidateSelectionKeyEvent excludes shiftKey, so the claimed and unclaimed paths now agree. Numpad[0-9] with NumLock off (code: 'Numpad2', key: 'ArrowDown') is likewise treated as an ender on both paths, so the change is internally consistent rather than a new edge.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The incremental delta (f01bdda) generalizes the claimed-key ending from selectors to any commit or cancel key, completing the release contract for a session IME that never fires compositionend.
- Claimed commit/cancel ends the window — the ender predicate is renamed
PHYSICAL_PICK_ENDING_CODEand widened toSpace/Enter/NumpadEnter/Escape/Digit[0-9]/Numpad[0-9], so a claimedEnter(commit) orEscape(cancel) now ends the pick instead of refreshing it. The prior shape still refreshes for the true navigation/paging keys (Backspace, arrows,-/=,PageUp/PageDown). Using the physicalcodeis right:keyreadsProcesson every claimed key, and the digit row moves under AZERTY/Dvorak/Colemak whileDigit*does not. - Tests — the "composition session that never ends"
it.eachnow covers all six ending codes; the newEnter/NumpadEnter/Escapecases fail without the widened predicate. All 27 cases across the two specs pass locally.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
You are typing Chinese in a terminal with Sogou. You type
ni, then press the spacebar (or a number) to pick the character you want. A space — or that number — appeared in the terminal, and then the Chinese character showed up. Now the spacebar and the number go to the input method, where you aimed them, and only the Chinese character reaches the terminal.What Changed
Before:
ni+ Space produced你.hao+3produced3好. With a phrase, nothing usable came out at all.After:
ni+ Space produces你.hao+3produces好. Ordinary typing is untouched.Mechanism. Every candidate-key guard Orca had was keyed on a Chromium composition session —
compositionstart…compositionend. Sogou on fcitx never opens one: it draws the preedit and the candidate list in its own window and only ever hands the renderer the finalinsertText. So at the moment the user presses the selector,compositionActiveandcandidateKeyGuardActiveare both false, the Space or digit is an ordinary printable key, and xterm sends it to the PTY.The one existing escape hatch, the orphan-keyup window, arms off a letter keyup with no matching keydown and only ever claims digits. When fcitx delivers the letter keydowns as
keyCode 229/key 'Process'the keydown is seen, so that window never arms.Review rounds since the first push
Three defects were found after the first push and are fixed on the branch; each has a test that fails without its fix.
Claimed keys that edit or page a preedit disarmed the window. The arming predicate only matched
Key*codes, but the IME also claims Backspace, the arrows and-/=, and Chromium keeps the originalcodeon those229/Processkeydowns. Paging to a later candidate closed the window, so the digit that picked from that page reached the PTY. A claimed keydown now refreshes the deadline; only a claimed letter arms it, so a bare navigation key cannot open it from cold.The window outlived a composition session. An engine that does open a session still emits the claimed letter keydowns that arm this window, and both its commit and its cancel travel as
insertCompositionText— the one input type the commit release ignores.compositionstartfires once per session, so every letter after the first re-armed it with nothing left to close it, and the user's next literal Space was swallowed.compositionendnow releases it too. The engines this guard exists for emit neither event, so their path is untouched.A claimed selector extended the window instead of ending it. A selector the engine took itself reads
key: 'Process', which the selector predicate cannot see, so it fell into the refresh path. It now asks the physicalcodeand ends the window: a claimedSpace,Digit0-9orNumpad0-9means the pick is over. This is what keeps a literal Space safe for an engine that opens a session and never closes it, which is a known defect in at least one shipped input framework.The net contract is four release conditions with no overlap and no gap. Engines that run a composition session are owned by the composition-scoped guards and this window stands down at both session boundaries; engines that do not are owned by this window, emit neither event, and commit through
insertText, which the commit release catches.This adds the missing signal: a letter keydown the IME has already claimed (
keyCode 229/key 'Process'over aKey*code) opens a 1500 ms window in the existing Linux candidate state. While it is open, the next Space or candidate digit is treated as IME-owned — suppressed andpreventDefault-ed, exactly as a composing selector already is. The window closes on the first selector it claims, oncompositionstart(a real session owns it instead), on a non-preeditinput(the round committed), on a key the IME did not claim, on blur, and on expiry.Why
Two arming signals rather than one, because the frameworks disagree: fcitx5 on Wayland omits the
229marker entirely, which is what the orphan-keyup window covers; fcitx on X11 emits it, which is what this covers. Neither can be inverted into a gate on its own, and the existing window was left untouched so its contract does not move.Space is claimed here where the orphan-keyup window deliberately declines it. A claimed keydown is much stronger evidence of an open preedit than a bare orphaned keyup, and Space is the selector Sogou users reach for first — it is the half of the report that the digit-only window could never have fixed.
The alternative — guarding Space and digits whenever a Linux IME has been used recently — swallows real spaces in a shell. The alternative of rendering Sogou's preedit ourselves is a much larger change that does not fix the leak.
Hangul keeps its carve-out: 2-Set commits its syllable on Space and still owes the literal space, so
hangulPreeditopts out of both selectors.Linked Issue
Fixes #22442
Same root cause, and the same invariant: #16260 (single Chinese character invisible during composition — no composition session exists to render), #19180 (Sogou/fcitx5 on Deepin).
Visual Proof
N/A — terminal input routing, not a renderer layout change. The user-visible before/after is the PTY byte stream, which the e2e spec below asserts directly.
Recorded on this branch with a read-boundary reader on the PTY (Linux renderer policy, CDP-replayed Sogou key shapes):
ni, then Space" "then"你""你"ha, then3"3"then"好""好"ls 7"l" "s" " " "7""l" "s" " " "7"" "" "" "" ""한" " ""한" " "Testing
tests/e2e/terminal-linux-claimed-preedit-selector.spec.tsdrives the real terminal under the Linux IME platform policy and asserts the exact PTY bytes. It fails onmain(" 你3好 ls7"vs"你好 ls7") and passes here.Native fcitx/Sogou evidence is still owed, per
docs/reference/ime-regression-checklist.md— CDP replays the renderer policy and the PTY bytes, not the input framework. I do not have an Ubuntu 20.04 + Sogou machine; I would like the reporters to confirm on theirs.AI Disclosure
Claude Opus 5 was used to reproduce the leak, locate the root cause, write the fix, and write the tests.
Review
Please focus on:
compositionstart, non-preeditinput, an unclaimed keydown, blur, one-shot, and 1500 ms;hangulPreeditis the right opt-out for Space as well as digits;Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Linux-only by construction (
isLinuxgate inxterm-bypass-policy); macOS and Windows keyboard paths are unreachable from the new code. No wire fields, no Git commands, no new permissions. SSH and folder workspaces are unaffected — the decision is renderer-side, ahead of the PTY, and identical for a local or remote host.Checklist