fix(terminal): commit Sogou Shift latin in the Electron TUI - #22026
JasonJarvan wants to merge 9 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 ignored due to path filters (1)
📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe patch updates xterm IME handling with deferred composition transactions, delayed Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: 🟡 Moderate · up to A Windows Sogou Shift keyup can still produce unintended terminal input, and composition lifecycle consumers miss the pending-session closure event. Resolve these IME behaviors before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 10 files. (2 skipped: 2 unsupported.) 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Clear Git repository-location variables before Git commands. · regenerate-xterm-patches.mjs:169-176
config/scripts/regenerate-xterm-patches.mjs:169-176
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear Git repository-location variables before Git commands.
runinheritsGIT_DIR,GIT_WORK_TREE,GIT_INDEX_FILE, and related variables. The newgit rm --cached .at Line 239 can then target the caller repository instead ofroot. The following hard reset can fail or modify the unintended checkout. Build a sanitized environment for everygitinvocation.Based on learnings: strip Git repository-location environment variables from subprocesses that target a specific
cwd.Source: Learnings
🟠 Major · Clear the IME commit debt when cancellation… · `@xterm__xterm`@6.1.0-beta.303.src.patch:800-802
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch:800-802
🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear the IME commit debt when cancellation settles the transaction.
The held-Shift path leaves
_imeKeydownAwaitingCommitset after_cancelComposition(). If Escape cancels the pending composition and the delayedinsertTextthen arrives,input()calls_claimImeKeydownCommit()and forwards the canceled text to the PTY.Clear the flag during cancellation.
Proposed fix
this._pendingComposition = undefined; this._isAwaitingCompositionEnd = false; this._isComposing = false; + this._imeKeydownAwaitingCommit = false;
🟡 Minor · Handle the delayed Sogou commit in screen… · `@xterm__xterm`@6.1.0-beta.303.src.patch:1051-1058
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch:1051-1058
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle the delayed Sogou commit in screen reader mode. Screen reader mode bypasses
CompositionHelper.input, so the Process/229 + Shift path can leave_pendingCompositionwaiting after the emptycompositionend. The delayedinsertTextcan then be dropped by xterm’s held-key guard, and later input can use stale composition state. RouteinsertTextto the helper when a pending composition finalization exists, including screen reader mode. Clear_imeKeydownAwaitingCommitwhen that pending branch consumes the text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: cc27b466-4ce6-4013-ba90-9bf5d0238088
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
config/patches/@xterm__xterm@6.1.0-beta.303.patchconfig/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patchconfig/scripts/regenerate-xterm-patches.mjsconfig/scripts/regenerate-xterm-patches.test.mjsconfig/scripts/xterm-patch-text.mjsdocs/reference/ime-regression-checklist.mdsrc/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-cancel.test.tssrc/renderer/src/components/terminal-pane/xterm-bypass-policy-non-mac.test.tssrc/renderer/src/components/terminal-pane/xterm-bypass-policy.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
|
Addressed the actionable review notes on
Not changing in this PR:
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two concrete risks worth resolving before merge. Both are conditional, but each has real fallout: a kitty release CSI-u leaked to the PTY, and a possible duplicated commit. Details inline.
Reviewed changes
- IME bypass policy —
isImeModeToggleShiftKeylets anycodestarting withShiftskipshouldSuppressTerminalImeKeyboardEvent, so Sogou'sProcess/229 Shift keydown reaches xterm'sCompositionHelper. Platform-agnostic, not gated on event type,key,keyCode, orisComposing. - xterm source patch — sets
_imeKeydownAwaitingCommitfor a Shift keydown while composing, adds_isImeModeToggleShiftand_shouldWaitForHeldImeCommit, makes the composition finalizer wait (leaving_pendingCompositionopen) while that held keydown still owes a commit, and guards_deferPreeditResyncwith!this._imeKeydownAwaitingCommit. - Patch regeneration script — Windows-only fixes:
* -textin the upstream checkout's.git/info/attributesbefore a re-reset (CRLF smudging),toPosixfor the version-stamp comparison,git diff --no-indexrun from a computedcommonParentcwd viaposixRelative, and-c core.autocrlf=falsein the test's patch replay. - Tests — two integration tests (Shift/
Process/229 keydown, emptycompositionend, delayedinsertText→ emitssonce) plus a bypass-policy unit test asserting Shift keydown/keyup are not suppressed. - Docs — new
#22021row in the IME regression checklist.
ℹ️ Nitpicks
- The
keyCode === 16 || 17 || 18_isImeModeToggleShiftblock (config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch:444-446) is unreachable through Orca's wiring:shouldSuppressTerminalModifierKeyboardEventreturnsfalseforkey:'Shift'atterminal-pane-pane-input.ts:161beforeCompositionHelper.keydownruns. The new integration tests only exercise it by dispatching directly on the textarea, so that coverage overstates real reachability.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| + } | ||
| + if (this._pendingComposition === pending) { | ||
| + if (this._shouldWaitForHeldImeCommit(pending)) { | ||
| + return; |
There was a problem hiding this comment.
_shouldWaitForHeldImeCommit's early return leaves _imeKeydownAwaitingCommit set, and nothing on the flush path clears it. When the textarea is non-empty at compositionend, _updatePostCompositionInputExpectation sets expectsPostCompositionInput false, the finalizer flushes the preedit, and a later insertText is then claimed by _claimImeKeydownCommit and sent again.
Technical details
# Sticky `_imeKeydownAwaitingCommit` can duplicate a commit or swallow a cancel
## Affected sites
- `config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch:635-636` — early return leaves `_pendingComposition` open with no timer.
- `...:920-921` — `_deferPreeditResync` refuses to cancel while the flag is set, so a key that empties the preedit later in the same composition no longer clears it.
- `_sendPendingComposition` / `_settlePendingComposition` (pre-existing) never reset the flag; `_claimImeKeydownCommit` (`:520-531`) then claims a later `insertText`.
## Sequence (double commit)
1. `compositionstart`; `compositionupdate('s')`; textarea `'s'`.
2. Shift `Process`/`ShiftLeft`/229 keydown → `_imeKeydownAwaitingCommit = true`.
3. Empty `compositionend` while the textarea still holds `'s'` → `expectsPostCompositionInput = false`, finalizer flushes `'s'`, `_pendingComposition` cleared, flag still true.
4. IME's delayed `insertText 's'` → `input()` finds no pending → `_claimImeKeydownCommit` sees the flag and sends `'s'` a second time.
## Required outcome
- Once a pending composition has been sent or settled, the held-key debt is discharged and a later `insertText` must not be re-sent.
## Suggested approach
- Clear `_imeKeydownAwaitingCommit` in `_sendPendingComposition` / `_settlePendingComposition` (one source of truth), or set it only on the empty-`compositionend` path that actually waits.
## Open questions for the human
- Does the captured Sogou trace ever leave the textarea non-empty at `compositionend`? If not, this is latent rather than live.
|
Run failed. View the logs →
|
There was a problem hiding this comment.
Important
The release-event half of the prior kitty finding is only partly closed. The new modifier guard keys off event.key, so a Sogou keyup with key:'Process' and a Shift* code still reaches xterm's kitty encoder — the same leak the guard was added to stop. Details inline.
Reviewed changes
Shiftmodifier policy — extractedshouldSuppressTerminalModifierKeyboardEventinto an options-taking function that lets a composingShiftkeydown reachCompositionHelperwhile keeping idleShiftand non-idleShiftreleases suppressed.- IME-commit-debt lifecycle —
_imeKeydownAwaitingCommitis now cleared in_cancelCompositionand whenever the deferred textarea diff observes a change, with an integration test proving Escape cancels a held-Process composition without emitting latin. - Patch-regeneration hardening —
withoutGitRepositoryLocationstripsGIT_DIR/GIT_WORK_TREE/GIT_INDEX_FILE/etc. from everygitspawn,commonParenttakes apathimplementation and returns the Windows drive root, andpnpmDiffEnvironmentreuses the sanitizer; both are unit-tested. - Bundle/lockfile/docs — regenerated the bundle patch and
pnpm-lock.yamlpatch hash, and refreshed the#22021checklist row to cover thekey=Shiftshape.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No critical issues — a couple of minor observations inline.
Reviewed changes
- Post-compositionend Shift window — added
isImeShiftCommitGuardActive()to the IME composition tracker, armed for 250 ms after everycompositionend(and covering a live composition), with unit tests for arming, survival across the delayedinsertText, and expiry. - Modifier policy — threaded
imeShiftCommitGuardActiveintoshouldSuppressTerminalModifierKeyboardEvent, so akey:'Shift'keydown inside that window reachesCompositionHelpereven whenisComposingis already false. - xterm patch — any Shift-coded keydown (
_isImeModeToggleShift) now arms_imeKeydownAwaitingCommit, covering the post-end Shift shape. - Tests/docs — added an integration test for ordinary Shift arriving after an empty
compositionendand refreshed the#22021checklist row and regenerated bundle patch/pnpm-lock.yamlhash.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| if ( | ||
| shouldSuppressTerminalModifierKeyboardEvent(ev, { | ||
| compositionActive: windowsComposing.compositionActive, | ||
| imeShiftCommitGuardActive: true |
There was a problem hiding this comment.
The pane-handler stub hardcodes imeShiftCommitGuardActive: true, so these integration tests pass even if terminal-pane-pane-input.ts stopped passing the tracker's isImeShiftCommitGuardActive() — the one piece of wiring this commit adds. The policy unit tests cover the guard logic, but nothing exercises the tracker-to-pane call; deriving the flag from a parameter (or driving it through the real tracker in one test) would close that gap.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · End the prior pending session when a new… · `@xterm__xterm`@6.1.0-beta.303.src.patch:595-596
config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch:595-596
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winEnd the prior pending session when a new composition starts.
compositionstart()only recordsnextCompositionStartfor an existing pending transaction. It does not call_endPendingCompositionSession(). The pending-reconciliation event is intended for this force-end path, not for initial pending creation. Calling it at lines 595-596 would end the session before reconciliation and cause current consumers to discard the pending event.Suggested fix
if (this._pendingComposition) { this._pendingComposition.nextCompositionStart = this._compositionPosition.start; + this._endPendingCompositionSession(this._pendingComposition); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 7d21efaf-dc42-4e61-8704-6699d94c55d3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
config/patches/@xterm__xterm@6.1.0-beta.303.patchconfig/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patchdocs/reference/ime-regression-checklist.mdsrc/renderer/src/components/terminal-pane/terminal-ime-composition-tracker.test.tssrc/renderer/src/components/terminal-pane/terminal-ime-composition-tracker.tssrc/renderer/src/components/terminal-pane/terminal-ime-xterm-composition-cancel.test.tssrc/renderer/src/components/terminal-pane/terminal-pane-pane-input.tssrc/renderer/src/components/terminal-pane/xterm-bypass-policy-interrupt.test.tssrc/renderer/src/components/terminal-pane/xterm-bypass-policy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/reference/ime-regression-checklist.md
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| + // Nothing is forwarded for a keydown the IME consumed, so it is left owing its commit; any | ||
| + // other keydown either forwards its own text or produces none, and clears the debt. | ||
| + this._imeKeydownAwaitingCommit = ev.keyCode === 229; | ||
| + this._imeKeydownAwaitingCommit = ev.keyCode === 229 || this._isImeModeToggleShift(ev); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '420,535p' config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch
sed -n '950,990p' config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch
sed -n '1010,1070p' config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patchRepository: stablyai/orca
Length of output: 8574
🏁 Script executed:
#!/bin/bash
set -eu
file='config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch'
printf '%s\n' '--- CompositionHelper keydown/input/reconciliation ---'
sed -n '400,570p' "$file"
sed -n '760,880p' "$file"
printf '%s\n' '--- CoreBrowserTerminal event wiring ---'
rg -n -C 12 'compositionHelper\.(keydown|input|keypress|textarea|blur)|_inputEvent|_keyDown|insertText' 'config/patches/@xterm__xterm@6.1.0-beta.303.patch' "$file"Repository: stablyai/orca
Length of output: 45546
🏁 Script executed:
#!/bin/bash
set -eu
file='config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch'
core='config/patches/@xterm__xterm@6.1.0-beta.303.patch'
printf '%s\n' '--- CompositionHelper state and reconciliation ---'
rg -n --no-heading --fixed-strings '_imeKeydownAwaitingCommit' "$file"
sed -n '780,875p' "$file"
printf '%s\n' '--- CoreBrowserTerminal keydown and input wiring ---'
sed -n '95,140p' "$core"
sed -n '1010,1065p' "$core"Repository: stablyai/orca
Length of output: 8234
Prevent idle Shift from creating IME commit debt.
CoreBrowserTerminal._keyDown calls CompositionHelper.keydown for every textarea keydown. An idle IME-toggle Shift can set _imeKeydownAwaitingCommit. Textarea reconciliation clears this flag only when the textarea value changes, so a later unrelated insertText can be claimed as the pending IME commit.
Gate the Shift case on active composition or pending finalization.
Suggested fix
- this._imeKeydownAwaitingCommit = ev.keyCode === 229 || this._isImeModeToggleShift(ev);
+ this._imeKeydownAwaitingCommit =
+ ev.keyCode === 229 ||
+ ((this._isComposing || this.hasPendingCompositionFinalization) &&
+ this._isImeModeToggleShift(ev));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| + this._imeKeydownAwaitingCommit = ev.keyCode === 229 || this._isImeModeToggleShift(ev); | |
| this._imeKeydownAwaitingCommit = | |
| ev.keyCode === 229 || | |
| ((this._isComposing || this.hasPendingCompositionFinalization) && | |
| this._isImeModeToggleShift(ev)); |
|
Native Sogou on Windows confirmed this follow-up: IME/composition flags are not trustworthy after empty compositionend, so gating Shift keydown on isComposing or a post-end window still drops the held-key insertText. The custom handler now always delivers Shift keydown; CompositionHelper consumes it so kitty cannot encode a bare modifier, and Shift keyup stays suppressed. |
There was a problem hiding this comment.
Important
The new unconditional Shift pass-through arms the IME-commit claim on an idle Shift keydown, and a following bypassed Shift+<non-ASCII> keydown leaves it armed, so the character is emitted twice on Windows/Linux non-Latin layouts. Details inline.
Reviewed changes
- Made
shouldSuppressTerminalModifierKeyboardEventpass everykey:'Shift'keydown unconditionally, so the kitty guard now lives in xterm:CompositionHelper.keydownconsumes any Shift-coded keydown before_keyboardService.evaluateKeyDownruns. - Armed
_imeKeydownAwaitingCommitfor any Shift-coded keydown, not just composing ones, and kept the finalizer wait for the held Sogou commit. - Dropped the composition/guard gating from the modifier policy (its options are now ignored) and refreshed the
#22021checklist row. - Regenerated the source patch, the compiled bundle patch, and the
pnpm-lock.yamlpatch hash in lockstep (bundle now carries_isImeModeToggleShift).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| + // Nothing is forwarded for a keydown the IME consumed, so it is left owing its commit; any | ||
| + // other keydown either forwards its own text or produces none, and clears the debt. | ||
| + this._imeKeydownAwaitingCommit = ev.keyCode === 229; | ||
| + this._imeKeydownAwaitingCommit = ev.keyCode === 229 || this._isImeModeToggleShift(ev); |
There was a problem hiding this comment.
Arming the claim for an idle (non-composing) Shift keydown leaves it armed when the next keydown is bypassed by shouldBypassXtermKeyboardEvent (Shift + a single non-ASCII printable). That keydown never reaches CompositionHelper.keydown to clear the flag, so the input event's _claimImeKeydownCommit re-sends the glyph _keyPress already emitted — every Shift+<non-ASCII> keystroke (uppercase Cyrillic/Greek, etc.) types twice on Windows/Linux.
Technical details
# Idle-Shift claim survives a bypassed keydown and double-emits
## Affected sites
- `config/patches/xterm-src/@xterm__xterm@6.1.0-beta.303.src.patch:456` — `_imeKeydownAwaitingCommit = ev.keyCode === 229 || this._isImeModeToggleShift(ev)` arms the claim for an idle, non-composing Shift keydown.
- `src/renderer/src/components/terminal-pane/xterm-bypass-policy.ts:315` — every `key:'Shift'` keydown now passes the modifier policy.
- `src/renderer/src/components/terminal-pane/xterm-bypass-policy.ts:346-357` — a Shift + single non-ASCII printable keydown is bypassed before `CompositionHelper.keydown`, so nothing clears the claim.
- `node_modules/@xterm/xterm/src/browser/CoreBrowserTerminal.ts:869-871` — the bypass makes `_keyDown` return at the custom-handler check, never calling `CompositionHelper.keydown`.
- `CoreBrowserTerminal.ts:1031-1035` (`_keyPress` emits and sets `_keyPressHandled`) and the patched `_inputEvent` first branch (calls `_compositionHelper.input(ev.data)` before the `_keyPressHandled` dedup).
## Sequence
1. Idle `Shift` keydown → not suppressed by the IME policy (`isImeModeToggleShiftKey`) or the modifier policy → `CompositionHelper.keydown` sets `_imeKeydownAwaitingCommit = true`.
2. `Shift+Ф` keydown → `shouldBypassXtermKeyboardEvent` returns true → `_keyDown` returns before `CompositionHelper.keydown`; claim stays true.
3. `keypress` → `_keyPress` emits `Ф`, sets `_keyPressHandled = true`.
4. `input` `insertText:'Ф'` → `_inputEvent` first branch → `_claimImeKeydownCommit` sees the stale claim and emits `Ф` again.
## Required outcome
- A Shift keydown that does not begin an IME commit must not leave a claim a later `input` event consumes, so `Shift+<non-ASCII>` emits once. macOS is already shielded by the native-text forwarder; Windows/Linux are exposed.
## Suggested approach
- Arm `_imeKeydownAwaitingCommit` only for a Shift keydown while composing/finalizing (as the previous revision did), or clear the claim whenever a keydown is bypassed. Add an integration test that types `Shift` + a non-ASCII letter and asserts one emission (`terminal-ime-xterm-consumed-key-commit.test.ts:233` only covers the non-bypassed next keydown).
nwparker
left a comment
nwparker
left a comment
There was a problem hiding this comment.
Thanks for staying on this — the diagnosis in #22589 and your native-Sogou follow-up are exactly the evidence this area needs, and I want this to land. Two things block it for me, and one of them is the note Pullfrog left last that is still open.
1. The imeShiftCommitGuard is dead code
shouldSuppressTerminalModifierKeyboardEvent(event, _options) never reads _options. Since the guard became unconditional in 6630665, nothing consumes it:
XtermModifierKeyboardOptionsis declared and never read;terminal-pane-pane-input.tscomputesimeShiftCommitGuardActive: imeCompositionTracker.isImeShiftCommitGuardActive()and throws it away;isImeShiftCommitGuardActive,imeShiftCommitGuardUntil, its three reset sites and its tracker tests have no consumer at all.
Either delete the whole guard, or restore the gating it was written for. As it stands the tests that cover it pass whatever the policy does, which is worse than no coverage. This is the same thing Pullfrog flagged on xterm-bypass-policy.ts:304.
2. The Shift carve-out is not platform-gated, and it is wider than Shift-to-English
The comment says "Windows keeps letter 229 suppression … but ShiftLeft/ShiftRight must pass", but isImeModeToggleShiftKey early-returns before isMac / isLinux are consulted, and before the isComposing and keyCode === 229 branches. I ran the matrix on your branch:
| event | main |
this branch |
|---|---|---|
Shift keydown, isComposing: true — mac / linux / win |
suppressed | passes |
Process/229 ShiftLeft keydown, composing — mac / linux / win |
suppressed | passes |
Process/229 ShiftRight keyup, composing — mac / linux / win |
suppressed | passes |
So this changes macOS and Linux, not only Windows, and it drops the 229 keyup suppression as well as the keydown. The keyup half is what Pullfrog raised against xterm-bypass-policy-non-mac.test.ts:256; the new test added at :207 asserts the keyup now passes rather than resolving whether it should. With REPORT_EVENT_TYPES negotiated, that release reaches KittyKeyboard and encodes a modifier release CSI-u for a press the TUI never saw.
If the real finding is "IME/composition flags are not trustworthy after an empty compositionend" — and I believe you, that matches what #22589 measured independently — then the narrow version is: gate on isWindows, keep the carve-out to keydown, and leave the keyup suppressed. If the keyup genuinely must pass too, that needs its own stated reason and a test for what the TUI receives, not just that the policy returns false.
Not blocking, but worth pinning
shouldSuppressTerminalModifierKeyboardEvent now returns false for an idle Shift keydown on every platform. You say CompositionHelper consumes it before the kitty encoder runs, so no bare modifier press is emitted — that is the right mechanism, but it now lives entirely in the vendored xterm patch. A policy-level test that pins "idle Shift keydown under REPORT_ALL_KEYS_AS_ESCAPE_CODES produces no CSI-u" would stop a later patch regeneration from quietly undoing it.
Context
I have a related fix up at #22607 for the Linux half of the same family — Sogou on fcitx opening no composition session at all, so candidate Space and digits leak to the PTY. It touches xterm-bypass-policy.ts and terminal-pane-pane-input.ts too, but only inside the isLinux branch and the Linux candidate state, so I do not expect a conflict beyond imports. Happy to rebase behind you if yours lands first — you have the native Windows Sogou hardware and that is the scarcer resource here.
|
Both review blockers are addressed on 1. Dead
|
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the commits close the two blocking items from the human review and the two related prior threads.
Reviewed changes
- Windows-only Shift carve-out —
shouldSuppressTerminalImeKeyboardEventnow lets aShift*-coded keydown past IME suppression only whenisWindowsand only forkeydown, so macOS and Linux return to the previous policy and the Shift-coded keyup falls through to the existing 229 suppression. - Kitty release leak closed — a Sogou
Process/229ShiftRightkeyup is suppressed again, with a pane-level test asserting the PTY receives no release CSI-u. - Dead IME-Shift guard removed —
imeShiftCommitGuardstate,isImeShiftCommitGuardActive,XtermModifierKeyboardOptions, and the ignored_optionsparameter are deleted, revertingterminal-pane-pane-input.tsandterminal-ime-composition-tracker.tsto their base shape. - Idle-Shift invariant pinned — an integration test asserts an idle
Shiftkeydown emits no kitty CSI-u underREPORT_ALL_KEYS_AS_ESCAPE_CODES, and the#22021checklist row now documents the Windows-scoped behavior.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Windows Sogou reports Shift-to-English as Process/229 on ShiftLeft during composition, then an empty compositionend and delayed insertText. Let that Shift reach CompositionHelper and wait for the held-key commit instead of dropping it. Fixes stablyai#22021 Co-authored-by: Cursor <cursoragent@cursor.com>
Clear _imeKeydownAwaitingCommit in _cancelComposition so Escape cannot later claim a delayed insertText. Isolate cwd-targeted git from GIT_DIR, and return the filesystem root from commonParent when that is the only shared path. Co-authored-by: Cursor <cursoragent@cursor.com>
…e modifiers Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Native Sogou reports Shift-to-English after compositionend with flags we cannot trust, so IME-state gates drop the held-key insertText. Always let Shift keydown reach CompositionHelper, which consumes it so kitty cannot encode a bare modifier. Fixes stablyai#22021 Co-authored-by: Cursor <cursoragent@cursor.com>
The post-composition Shift guard was unread after Shift keydown became unconditional, and the IME early return also let a Shift-coded keyup through on macOS and Linux. Delete the guard instead of restoring it. Native Sogou reports composition flags too late, so gating the keydown drops the latin commit. Keep delivering every Shift keydown; CompositionHelper still consumes it. Suppress Shift-coded keyup, including Process/229, and leave macOS and Linux on the previous IME policy.
The IME policy already gated its Shift carve-out to Windows keydown, but the modifier policy did not: every platform stopped swallowing a Shift keydown, so macOS and Linux began relying on the vendored CompositionHelper patch consuming it to avoid a kitty press CSI-u. Only Sogou needs the keydown delivered, so gate that half on Windows too and leave the other platforms on the pane-level swallow they had before. Regenerate the xterm bundle patch and lockfile hash: the committed bundle was not what the pinned upstream build produces from the committed source patch. The patched source tree is byte-identical either way — the source patch only re-anchors its hunks. Co-authored-by: JasonJarvan <JasonJarvan@users.noreply.github.com>
30c05ec to
cc28cca
Compare
|
Picked this up to get it over the line, and you had already done most of what I asked — so first, credit where it is due, and an apology for briefing my review as if you had gone quiet. Blocker 1 (the dead guard): you resolved it in Blocker 2: you fixed half, and the half you fixed is right. The half I have added: Blocker 3 you covered better than I asked. I suggested a policy-level assertion; you drove a real Also rebased onto current One thing still to settle: the branch's Speaking of which — your CI had never run. The workflow runs sat unapproved since you opened this, so only the automated reviewer ever executed. I have approved them. Native Windows Sogou verification is still owed and I am not asking you for it — one of our Windows machines has Sogou installed and I will drive that myself. Thank you for the original diagnosis; the runtime instrumentation in the linked report is what made this tractable. |
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The two blocking items from the human review are addressed, and the stale xterm bundle/lockfile pair is back in lockstep. I'm not approving only because two earlier comments on this PR remain open and are untouched by these commits.
This run re-reviewed 5d5c029 and cc28cca, the two commits pushed after the previous Pullfrog review (30c05ec).
Reviewed changes
- Gated the IME Shift carve-out to Windows keydown —
shouldSuppressTerminalImeKeyboardEventpasses aShift*-coded keydown only whenisWindowsand only forkeydown, so macOS/Linux return to the previous policy and a Shift-codedProcess/229keyup falls through to the 229 suppression again. - Scoped the modifier policy to Windows —
shouldSuppressTerminalModifierKeyboardEventnow takes{isMac, isLinux}and passes a Windowskey:'Shift'keydown while swallowing it on macOS/Linux; the deadimeShiftCommitGuardstate, option type, and tracker tests are deleted. - Re-regenerated the xterm bundle patch and
pnpm-lock.yamlpatch hash — the committed bundle now matches the pinned build;sha256(bundle patch)equals the lockfile hash and every dependent resolution key, andpnpm install --frozen-lockfilesucceeds. - Added platform-matrix and idle-Shift tests — Windows Shift keydown delivered, Shift-coded 229 keyup suppressed, macOS/Linux unchanged, and an idle Shift keydown under
REPORT_ALL_KEYS_AS_ESCAPE_CODESemits no CSI-u. All 71 tests in the three affected suites pass once the current patched bundle is installed.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Native Windows Sogou smoke on the dev client at |
stablyai#22301 added src/shared/agent-session-context-usage.ts and src/shared/agent-session-context-usage-schema.ts to the route. It touched nothing under mobile/, so mains pin stayed at 4219 while the closure measures 4221.
|
The mobile web app bundle failure is fixed in 087a738.
The other red job on that run, |
There was a problem hiding this comment.
Important
The new commit re-pins a module-count constant that main has already deleted, so it now conflicts with main on that file. Rebase onto main and keep main's version rather than carrying the repin.
This run re-reviewed the single commit pushed since the previous Pullfrog review (cc28cca).
Reviewed changes
- Re-pinned the session-route module census — bumped
SESSION_ROUTE_MODULESfrom4219to4221for #22301's twosrc/sharedcontext-usage modules, adding a docstring paragraph that records the+2. - No product code in the delta — the commit is test-only and leaves the terminal IME path reviewed in earlier runs untouched.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
… modules" This reverts commit 087a738.
There was a problem hiding this comment.
ℹ️ No new issues. The delta is a clean revert of the previous test-only commit and clears the modify/delete conflict this review flagged; the branch still conflicts with
mainonpnpm-lock.yaml(noted in the prior run), so a rebase is still required before merge.
Reviewed changes
- Reverted the mobile session-closure repin —
5207f82reverts087a738, so theSESSION_ROUTE_MODULES4219 → 4221 bump and its#22301docstring paragraph are gone andconfig/scripts/mobile-web-app-session-terminal-closure.test.mjsleaves the PR diff, resolving the modify/delete conflict againstmain. - No product code in the delta — the terminal IME path is byte-for-byte the code reviewed in runs 5 and 6.
The two earlier open findings (idle-Shift claim surviving a bypassed keydown, and the sticky _imeKeydownAwaitingCommit on the non-empty-textarea flush path) are untouched by this revert and remain open.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
2 participants
ELI5
On Windows, you type pinyin in an Electron terminal, then tap Shift to send those letters as English. In Sogou that text disappeared. It now commits as Latin, the same way Web UI and ordinary inputs already did.
What Changed
Before: Sogou Shift-to-English (
key=Processorkey=Shift,code=ShiftLeft) was swallowed before xterm's CompositionHelper. xterm then treated the emptycompositionendas a cancel, so the laterinsertTextnever reached the PTY.After: physical Shift reaches
CompositionHelperwhile composing. Idle Shift stays suppressed so kitty does not encode a bare modifier. While that IME keydown still owes a commit, an emptycompositionendwaits for the delayed latin input instead of flushing an empty textarea.Why
#11946 stopped Microsoft Pinyin Shift from turning into Enter. Sogou still lost the commit entirely: Windows 229 suppression plus swallowing composing
key=Shiftas a standalone modifier, then an empty-end flush while the key is held.Letting only composing Shift through is smaller than unblocking every Shift keydown (which would let kitty report idle modifiers) or turning off all Windows 229 handling. Waiting only when a held IME keydown still owes a commit keeps ordinary cancel (Backspace over the whole preedit) intact.
Linked Issue
Fixes #22021
Related: #11946 (Microsoft Pinyin Shift became a newline; different IME, different failure).
Visual Proof
N/A — this is terminal input routing, not a renderer layout change. The Electron TUI vs Web UI difference is covered by synthetic composition replay: Process/229 and ordinary Shift while composing, empty
compositionend, then delayedinsertTextwhile the key is held.Testing
Verified on Windows with native Sogou in the Electron TUI: pinyin then a single Shift commits latin and switches IME. Search/plain inputs were already fine.
CI should cover Linux/macOS.
AI Disclosure
Cursor Grok 4.6 was used to diagnose the Electron TUI event sequence, implement the bypass-policy and CompositionHelper changes, regenerate the xterm patches, and write tests.
Review
Please focus on:
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.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)