Skip to content

Fix stale React props corrupting text during IME batched input - #1551

Open
heyncth wants to merge 2 commits into
CodebuffAI:mainfrom
heyncth:fix/ime-batched-input-stale-props
Open

heyncth wants to merge 2 commits into
CodebuffAI:mainfrom
heyncth:fix/ime-batched-input-stale-props

Conversation

@heyncth

@heyncth heyncth commented Oct 24, 2026 •

Copy link
Copy Markdown

What

All keyboard handlers in MultilineInput now read from synchronous refs (valueRef.current/cursorPositionRef.current) instead of React props, and update refs before calling onChange.

Why

The code already documents this race at multiline-input.tsx lines 237-246: when multiple key events fire in one React batch, refs are updated synchronously but props are stale. insertTextAtCursor was fixed to read refs, but no other handler was. Deletion, enter, navigation, mouse click, and cursor movement all still read stale value/cursorPosition props.

This affects any input where multiple key events arrive in one stdin chunk: Chinese, Japanese, Korean, Arabic IMEs, and fast ASCII typists. Vietnamese makes it unmissable because compose-and-replace sends base char + backspace + accented char in rapid succession, so the stale backspace visibly corrupts the text.

Repro

  1. Enable a Vietnamese Telex IME (Unikey/EVKey)
  2. Type xin chao then f (Telex grave)
  3. Expected: xin chào
  4. Actual (before fix): xin chaào (or similar) - backspace deleted from the pre-batch string, never removing the base a

Root cause trace

Event Handler Reads Problem
a insertTextAtCursor valueRef.current inserts base char, updates ref
\x7f (backspace) handleDeletionKeys value prop (stale) deletes wrong char
à insertTextAtCursor valueRef.current inserts after un-removed a

Changes

  • cli/src/components/multiline-input.tsx: 8 handlers switched to ref-based reads + ref updates before onChange. value/cursorPosition removed from useCallback deps.
  • cli/src/components/tests/multiline-input.test.tsx: 4 new tests covering ref-based IME sequence, batched input, stale-backspace regression guard, multi-batch diacritics.

Testing

bun test src/components/tests/multiline-input.test.tsx
76 pass, 0 fail

bun run typecheck: no errors in changed files (pre-existing errors in common/, sdk/, packages/ untouched).

Vietnamese IMEs (Unikey, EVKey) send backspace + accented char in a single
stdin chunk. OpenTUI splits this into separate key events, but React batches
state updates, so handleDeletionKeys read stale value/cursorPosition props
while insertTextAtCursor already read synchronous refs.

Result: 'chào' rendered as 'chaà' — backspace deleted from the pre-batch
string, never removing the base character.

All keyboard handlers (deletion, enter, navigation, mouse, cursor movement)
now read from valueRef.current/cursorPositionRef.current and update refs
before calling onChange, matching the pattern insertTextAtCursor already
used. Removes value/cursorPosition from useCallback dependency arrays.
@codebuff-team

Copy link
Copy Markdown
Contributor

The root-cause analysis is solid: insertTextAtCursor already reads from valueRef/cursorPositionRef per the comment at lines 237-246, and this PR correctly extends that pattern to the rest of the handlers (deletion, enter, navigation, mouse click, cursor movement) so that stale props don't get read mid-batch. The mechanical changes themselves — reading from refs, writing refs before calling onChange, and dropping value/cursorPosition from the useCallback deps — are applied consistently across handleDeletionKeys, handleNavigationKeys, moveCursor, handleMouseClick, and the enter-key handler. That's the right layer for the fix given the existing architecture.

The weak point is the test suite. The four new tests in multiline-input.test.tsx under Vietnamese IME compose-and-replace don't import or exercise any of the actual code paths you changed in multiline-input.tsx. createBatchedState is a hand-rolled reimplementation of the ref/batch pattern, and the tests assert against that reimplementation, not against MultilineInput's real handleDeletionKeys/insertTextAtCursor/onChange wiring. That means these tests would pass even if the actual fix in the component were subtly wrong (e.g. a wrong ref not updated, a handler missed), and they'd keep passing if someone reverted the real fix. The file already has an established harness for simulating real key events against the component (the 1100+ lines above your diff) — the IME regression tests should be written against that harness so they actually catch the bug they describe.

Given the size of the diff (8 handlers touched) and the IME/batching subtlety involved, a maintainer porting this needs tests that actually drive the component's key event pipeline, not a parallel model of it. Please rewrite the four tests using the existing test infrastructure for MultilineInput, feeding real key sequences (backspace + composed char) through the actual handlers and asserting on the resulting onChange calls/output.

@codebuff-team codebuff-team added bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written labels Oct 24, 2026
…ndlers

Review feedback: the four Vietnamese IME tests used a hand-rolled
reimplementation of the ref/batch pattern instead of exercising the
actual MultilineInput code paths. They would pass even if the fix
were wrong or reverted.

New multiline-input.ime.test.tsx uses createTestRenderer + mockInput
(the same harness as queue-editing.test.tsx) to mount MultilineInput
and drive real key events through handleDeletionKeys and
insertTextAtCursor. Verified the tests fail when the ref update in
the backspace handler is removed (produces 'chaà' as expected).

Removed the four reimplementation tests from multiline-input.test.tsx.
@heyncth

heyncth commented Oct 25, 2026

Copy link
Copy Markdown
Author

Addressed the test feedback in c703b80.

Replaced the four reimplementation tests with a new integration test file (multiline-input.ime.test.tsx) that uses the same harness as queue-editing.test.tsx: createTestRenderer + createRoot + mockInput. The tests now mount the real MultilineInput component and drive key events through the actual handleDeletionKeys and insertTextAtCursor handlers.

Verification: temporarily removed the ref update from the backspace handler and confirmed 3 of 5 integration tests fail with output 'chaà' (the reported bug), then restored the fix.

The old reimplementation tests were removed since they are now redundant with the integration coverage.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:triaged Classified by the community triage bot pr:needs-work Right idea, not mergeable as written

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants