fix(tui): stop Vietnamese Telex IME from dropping characters - #55415
fix(tui): stop Vietnamese Telex IME from dropping characters#55415hanhvs wants to merge 12 commits into
Conversation
Third-party Vietnamese IMEs (OpenKey/Unikey/EVKey in Telex mode) recompose a syllable by emitting an erase burst followed by the finished characters. Two layers of the TUI input pipeline mishandled this, dropping letters and leaving a stray space mid-syllable (e.g. "hạnh" rendered as "hạ ", and "vương sỹ hạnh" as "vương sỹ hạ "). Root causes, both confirmed from real captured byte streams: 1. parse-keypress: an IME often fuses a control byte (\x7f/\b, or even the U+202F marker OpenKey injects) with the recomposed text in a single stdin read. parseKeypress only recognizes a control key when the whole string is exactly that byte, so a mixed chunk fell through every branch, returned name:"" with a non-printable sequence, and the composer's printable gate discarded the entire chunk — taking the surrounding letters with it. Split text tokens on every control byte so the printable runs survive. CR/LF are deliberately not split, preserving paste/return semantics. 2. textInput: multi-character (IME/paste) inserts were committed through the 16ms deferred key-burst path, which raced an interleaved re-render and snapped the buffer back to a stale value, dropping the recomposed tail. Commit them synchronously. Additionally, the fast-echo "\b \b" backspace shortcut desynced the screen when it ran right after an Ink repaint (forced by the U+202F marker), stranding the marker glyph; suppress fast-echo for the recompose burst that follows an Ink repaint and resume it on the next real keystroke. Tested with real OpenKey and EVKey captures of "vương sỹ hạnh" across read timings, plus parser unit coverage and an EVKey no-regression guard.
|
Hi @0disoft — thanks a lot for #53993, it pointed me straight at the right area. I've opened this PR (#55415) which I believe takes the fix further than the original. Would you mind giving it a review to confirm it's more thorough than your version? Building on your work, I traced the bug down to three layers using real captured byte streams from OpenKey and EVKey (Telex mode), not synthetic input:
Tested with real OpenKey + EVKey captures of Full transparency: layer 3 (fast-echo) can't be exercised by the unit harness because fast-echo only runs under real terminal focus, so that part is verified by live terminal capture rather than CI. Happy to pair on reproducing it if useful. Credit to you for the original diagnosis 🙏 |
0disoft
left a comment
There was a problem hiding this comment.
Thanks for the detailed follow-up, and for the credit on #53993. I agree this is broader than my original parser-only fix: the real OpenKey/EVKey captures and the TextInput boundary coverage make it clear this bug class crosses the parser/TextInput/render boundary.
I would not treat #53993 and this PR as independent fixes; if this lands, it should supersede #53993. That said, I think a few points should be tightened before this is safe to merge:
-
The U+202F wording is currently misleading. U+202F is not a C0 control byte, and it is not handled by the parser control-byte split. The parser splits C0 controls plus DEL, excluding CR/LF. U+202F looks like a printable IME marker that forces the TextInput/Ink repaint + fast-echo path, so I would describe it separately from the parser split.
-
The sync commit change currently applies to every multi-character insert. That is correct for IME recomposition tails, but it could also catch coalesced plain ASCII reads or paste-like chunks. Could you either narrow the predicate, or add no-regression coverage for a pure ASCII multi-character read so normal burst/paste behavior does not accidentally lose the existing batching benefits?
-
The 60ms fast-echo suppression window makes sense as a practical guard, but it is currently an untested timing constant around the least reproducible layer of the fix. At minimum, I would suggest naming the constant and adding fake-timer coverage for suppression/resume behavior. A deterministic “baseline is clean again after repaint/real key” state transition would be even better if it fits the current TextInput structure.
-
The “commits a multi-character recompose synchronously” test still goes through
drive(), which waits 60ms at the end. That makes it harder to prove the sync commit itself is the thing being asserted, since the old deferred path was 16ms. Could we add an immediate/microtask-level assertion after the recomposed read so the old deferred path would fail for the intended reason?
The parser split and CR/LF handling look right to me, and the captured-byte replay is exactly the kind of regression coverage this bug needed. I just think the TextInput timing/fast-echo parts need a little more precision before I would be comfortable approving this as the superseding fix.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review: LGTM
Fixes Vietnamese Telex IME character dropping in the TUI. Two root causes:
- parse-keypress: Fused control-byte+text chunks (e.g. backspace+recomposed character in one stdin read) fell through every branch and the whole chunk was discarded. New parseTextKeypresses splits on control bytes so printable runs survive.
- textInput: Multi-character IME inserts were committed through a 16ms deferred key-burst path that raced re-renders. Now committed synchronously for atomic delivery.
Extensive test coverage: parser-level tests (probe + no-regress + control-byte splitting), and end-to-end IME regression tests with real OpenKey/EVKey byte captures for Vietnamese Telex.
Verdict: LGTM -- thorough fix with excellent test coverage using real IME captures.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the captured-input investigation. The parser/composer premise remains present on live main: ui-tui/packages/hermes-ink/src/ink/parse-keypress.ts:281 still sends a whole text token to parseKeypress, and ui-tui/src/components/textInput.tsx:1245 still defers multi-character commits.
Problems
ui-tui/src/__tests__/imeVietnameseTelex.test.tsx:95makes everydrive()call wait 60 ms, including the test at lines 112-118 that claims to prove a synchronous commit. That delay allows live main's deferredscheduleKeyBurstCommitpath (ui-tui/src/components/textInput.tsx:1245) to flush, so the test does not distinguish the proposed behavior.
Suggested changes
- Assert immediately after the final recomposition read, before the frame-delay path can run; fake timers would make that deterministic.
- Add deterministic coverage for the new 60 ms fast-echo-suppression reset introduced in
textInput.tsx.
Automated hermes-sweeper review.
| } | ||
| } | ||
|
|
||
| await wait(60) |
There was a problem hiding this comment.
This unconditional 60 ms wait means the later “commits ... synchronously” assertion can also pass through the old deferred burst path. Please expose a no-wait driver step or use fake timers and assert immediately after the final recomposition read.
- Remove unconditional 60ms wait that let deferred path pass sync-commit test - Use fake timers (setTimeout/setInterval/Date only, NOT setImmediate) - Assert immediately after final read — no trailing wait/advance - Add deterministic coverage for 60ms fast-echo suppression reset: * suppresses backspace after Ink repaint (IME recompose) * does NOT suppress on normal ASCII typing - Verified: revert sync commit -> deferred path makes 4/6 tests fail
|
@teknium1 Fixed per your review:
Verified: reverting |
|
Have you fixed this issue yet? At the moment, i have updated but the issue still occurs |
Force a full Ink redraw after coalesced terminal resize events and on terminal focus regain, and reassert bracketed paste when focus returns. This hardens the Alacritty/terminal-stale-frame class without rewriting input handling. Refs upstream TUI redraw/input issues: - NousResearch#54284 - NousResearch#25337 - NousResearch#35530 - NousResearch#48701 - NousResearch#30092 - NousResearch#55415 (cherry picked from commit 61d9540565c62a7434e35b0ec774988f18967deb) (cherry picked from commit a8385de07f84e0e4baa2f3959973328f1b609651)
|
Seeing the P3 label, I'm guessing this will be a while before it gets merged, if ever. @lthoa1807 |
Force a full Ink redraw after coalesced terminal resize events and on terminal focus regain, and reassert bracketed paste when focus returns. This hardens the Alacritty/terminal-stale-frame class without rewriting input handling. Refs upstream TUI redraw/input issues: - NousResearch#54284 - NousResearch#25337 - NousResearch#35530 - NousResearch#48701 - NousResearch#30092 - NousResearch#55415 (cherry picked from commit 61d9540565c62a7434e35b0ec774988f18967deb)
- Remove unconditional 60ms wait that let deferred path pass sync-commit test - Use fake timers (setTimeout/setInterval/Date only, NOT setImmediate) - Assert immediately after final read — no trailing wait/advance - Add deterministic coverage for 60ms fast-echo suppression reset: * suppresses backspace after Ink repaint (IME recompose) * does NOT suppress on normal ASCII typing - Verified: revert sync commit -> deferred path makes 4/6 tests fail
|
Thanks for this fix! It was salvaged into #86761 (cherry-picked onto current main with your authorship preserved in the commit history) and is now merged. Closing since the work has landed. |
Summary
Fixes #53982 — Vietnamese Telex input in the TUI dropped characters and left a stray space mid-syllable (e.g.
hạnhrendered ashạ, andvương sỹ hạnhasvương sỹ hạ).This builds on the lead in #53993 (which split fused IME backspace+text keypresses) and completes the fix. As the issue's own root-cause note predicted, the parser split alone did not fully resolve typing — there were two more layers, both confirmed from real captured byte streams from OpenKey and EVKey.
Root causes & fixes
parse-keypress.ts— generalize control-byte splitting.An IME fuses a control byte with the recomposed text in a single stdin read. The original code only recognized a control key when the whole string was exactly that byte, so a mixed chunk fell through every branch, returned
name: ""with a non-printable sequence, and the composer's printable gate discarded the entire chunk — taking the surrounding letters with it. Now text tokens are split on every control byte (not just\x7f/\b), so the printable runs survive. CR/LF are deliberately not split, preserving paste/return semantics.textInput.tsx— commit multi-character inserts synchronously.IME recompositions / pastes were committed through the 16ms deferred key-burst path, which raced an interleaved re-render and snapped the buffer back to a stale value, dropping the recomposed tail.
textInput.tsx— suppress fast-echo backspace after an Ink repaint.OpenKey injects a
U+202F(narrow no-break space) marker, which forces an Ink full-line repaint. The fast-echo"\b \b"backspace shortcut then ran against a baseline Ink had just invalidated, stranding the marker glyph. Fast-echo is now suppressed for the recompose burst that follows an Ink repaint and re-enabled on the next real keystroke.Verification
Diagnosed with a
HERMES_IME_DEBUGprobe capturing the real parser/composer I/O in a live session, then confirmed fixed by the reporter on OpenKey (the bug source); EVKey, which never exhibited the bug, is unaffected.Tests (
ui-tui):src/__tests__/imeVietnameseTelex.test.tsx— end-to-end replay of real OpenKey and EVKey captures ofvương sỹ hạnhacross read timings.packages/hermes-ink/.../parse-keypress.test.ts— control-byte split unit cases (incl. CR/LF preserved).parse-keypress-drop-probe.test.ts— exhaustive guard that no printable codepoint is silently dropped.parse-keypress-noregress.test.ts— proves clean (EVKey-style) input is untouched.tsc --noEmitclean; fullui-tuisuite green apart from 4 pre-existing, unrelated layout failures present onmain.Closes #53982