fix(composer): skip the input write-back during an IME composition - #5791
fix(composer): skip the input write-back during an IME composition#5791ntdatt812 wants to merge 4 commits into
Conversation
The composer read the raw DOM textContent on every input event and pushed it into the store. During an IME composition those events carry Lexical's pre-edit text, so the store ended up disagreeing with SyncPlugin's lastSyncedTextRef; the plugin's runtime->Lexical path then rebuilt the editor mid-composition and destroyed the composition node. The IME cancelled and committed each interrupted stage literally, so typing `nihao` left `n ni nihao 你好` in the box. Nothing is lost by removing it. SyncPlugin already owns editor-state -> composer-text and is composition-safe: it guards editor.isComposing() in both directions and moves lastSyncedTextRef in lockstep with its own setText, so its writes never re-enter the apply path. useComposerTextBridge already owns the programmatic writes -- dictation, clear-after-send, draft restore. A textContent read was also wrong for directive chips, which render as labels while the runtime serializes something else. Closes tinyhumansai#5763
How this change flows0 changed behaviours across 6 relationships. 6 surrounding behaviours are shown (60 graph nodes walked). 50 further behaviours left out to keep the diagram readable. flowchart LR
n0["store"]:::impacted
n1["ThreadComponentsContext"]:::impacted
n2["Composer"]:::impacted
n3["ThreadRoot"]:::impacted
n4["renderStreamingConversation"]:::impacted
n5["Thread"]:::impacted
n2 -->|uses| n1
n3 -->|uses| n1
n3 -->|uses| n2
n4 -->|uses| n0
n5 -->|uses| n1
n5 -->|uses| n3
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Green: changed behaviour. Grey: surrounding behaviour. Arrows name the call, use, implementation, or test relationship. Orange: has findings. Red: has a finding that blocks the merge. |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe composer now ignores active IME composition events and synchronizes committed Lexical text with the composer store. Regression tests cover pre-edit and commit events, including sending ChangesComposer synchronization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change removes the redundant input write-back that could interrupt IME composition without changing the existing synchronization owners; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant IME
participant LexicalComposerInput
participant ComposerStore
IME->>LexicalComposerInput: Send pre-edit input
LexicalComposerInput-->>ComposerStore: Ignore while isComposing is true
IME->>LexicalComposerInput: Send committed input
LexicalComposerInput->>ComposerStore: Synchronize committed text
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The implementation gates DOM-to-store synchronization on nativeEvent.isComposing, which prevents pre-edit text from rebuilding the editor while preserving composition-commit events. Regression tests cover ignored intermediate events and accepted committed text for issue
Warning Your free Security trial is over. An organization admin can activate billing to continue. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b6ef4fe19
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * opens, so a host that wants a plain box still gets one. | ||
| */} | ||
| {/* | ||
| * No `onInputCapture` write-back here. Reading the raw DOM |
There was a problem hiding this comment.
When the desktop WDIO suites call typeIntoComposer, the contenteditable branch directly assigns textContent and dispatches a synthetic input event instead of producing native editor operations (app/test/e2e/helpers/chat-harness.ts:114-133). This capture callback was the bridge that copied that DOM-only value into aui.composer; after removing it, the helper's DOM equality check can pass while the composer store remains empty, leaving the Send action without a prompt and breaking the chat E2E specs that use this helper. Keep a write-back that skips active composition, or update the helper to write through Lexical/the composer API in the same change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and already addressed — but your finding is stronger than the fix it landed on, so I want to record it properly.
That comment is anchored to 2b6ef4fe1, the commit where I deleted the handler outright. CI rejected that (19 red tests) and I replaced it with exactly what you prescribe: a write-back that skips active composition.
const native = event.nativeEvent as InputEvent;
if (native.isComposing) {
return;
}I verified your specific case rather than assuming it: chat-harness.ts never sets isComposing, and new InputEvent('input', { bubbles: true, data: nextValue }) defaults it to false — so the WDIO bridge passes the guard and still writes through to aui.composer.
You found a second harness I had not seen. I traced the vitest helper (Conversations.render.test.tsx's setComposerText) after CI failed; I did not know the WDIO suites had the same DOM-only pattern. Two independent harnesses depending on this callback is a much better argument for keeping it than the one I had, and it also answers the open question I left on this PR — I had asked whether the handler was redundant and the helper should be fixed instead. With WDIO in the picture, changing the harnesses is clearly the wrong trade.
One nuance worth flagging since you are looking at this path: keying the skip off inputType === 'insertCompositionText' instead of isComposing would break the WDIO bridge in the other direction. I had that clause in the pushed version and removed it — browsers can emit the composition commit with that same inputType and isComposing === false, so it swallowed the finished text. isComposing alone is the boundary.
Both branches now have a test, each verified red against the corresponding wrong implementation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Removing the handler outright broke 19 page-level tests. Their helper sets `textarea.textContent` directly and fires an `input` event, which never reaches Lexical's editor state -- so SyncPlugin's update listener does not fire and the removed write-back was the only thing turning typed text into store text under test. The deletion was right about production and wrong about the harness, and 19 red tests are better evidence than my reading of SyncPlugin. Guarding is also the smaller claim. It fixes the reported bug without betting on the handler being redundant: skip the write-back while `isComposing` is true or the event is an `insertCompositionText`, so nothing lands in the store mid-composition and SyncPlugin's runtime->Lexical path never rebuilds the editor under an active composition node. The commit event that ends a composition has `isComposing === false` and carries the finished text, so committed input still reaches the store exactly as before. Conversations.render.test.tsx: 54 passed. Closes tinyhumansai#5763
|
Correcting myself: CI was right and my original claim was wrong. I said "nothing is lost by dropping it". Removing the handler broke 19 page-level tests in function setComposerText(textarea: HTMLElement, text: string) {
textarea.textContent = text;
fireEvent.input(textarea, { data: text, inputType: 'insertText' });
}Setting Changed approach in const native = event.nativeEvent as InputEvent;
if (native.isComposing || native.inputType === 'insertCompositionText') {
return;
}This is also the smaller claim. It fixes the reported bug — nothing lands in the store mid-composition, so
Two things I would still value your view on, since they are the parts I cannot settle from outside:
|
ntdatt812
changed the title
…oth paths
The diff-coverage gate rejected the previous commit for leaving the guard's
`return` untested. Writing that test found a real defect in the guard itself.
The guard also blocked on `inputType === 'insertCompositionText'`. That
inputType is not exclusive to pre-edit events: the event that *ends* a
composition can carry it too, with `isComposing === false`. So the guard
swallowed the one event carrying the committed text.
`isComposing` alone is the correct discriminator — it is the spec's own "a
composition is in progress" flag, true for exactly the pre-edit events and
false on the commit.
Two tests, one per branch, each verified red against the wrong version:
- a half-composed stage must not reach the store
(red with no guard at all: 'nihao' reaches the composer)
- the committed text must be written back
(red with the inputType clause: '你好' never arrives)
56 passed in this file; 135 passed across the five files that render Thread.
|
The coverage gate was right to reject this, and chasing it found a defect in my own guard. What the gate caught. Diff-cover flagged line 387 — the guard's What writing it exposed. The guard read: if (native.isComposing || native.inputType === 'insertCompositionText') return;
That is not theoretical — I ran it:
Tests. Two, one per branch, each verified red against the corresponding wrong version (the table above is that run). The commit-path test deliberately keeps Verification. 56 passed in Worth noting for reviewers: in production |
`tsc --noEmit` rejected the new assertion: `chatSend` is imported with its real signature, so `.mock` is not on its type — only the `vi.fn()` behind the module mock has it. Switched to `toHaveBeenCalledWith(expect.objectContaining(...))`, which is type-safe and matches how every other test in this file asserts a send. My mistake was not running the repo's own type check before pushing; the test run alone was green. `tsc --noEmit` and `prettier --check` are both clean now, and the file is 56 passed.
|
Fixed the type error. That one was avoidable and mine.
The I had run the test file but not |
1 participant
Closes #5763.
The defect
thread.tsxattached anonInputCapturehandler toLexicalComposerInputthat read the raw DOMtextContenton everyinputevent and pushed it into the store:During an IME composition those events carry Lexical's pre-edit text. I traced the consequence through the shipped package rather than taking the report on trust —
@assistant-ui/react-lexical/dist/plugins/SyncPlugin.js:The handler's
setTextmoves the store without movinglastSyncedTextRef, so that guard does not hold andapplyRuntimeTextrebuilds the editor mid-composition. The composition node dies, the IME cancels and commits the current pre-edit literally, and the next keystroke repeats it —nihaoarrives asn ni nihao 你好.English never triggers it because there is no composition to interrupt: the round-trip is idempotent for committed text.
Why removing it loses nothing
Both owners of that sync already exist, and both are composition-safe in a way the handler was not:
SyncPluginowns editor-state → composer-text. It guardseditor.isComposing()in both directions (:335,:356) and moveslastSyncedTextRefin lockstep with its owncomposer.setText(:326-329), so its writes never re-enter the apply path above.useComposerTextBridge(app/src/components/chat/ChatComposer.tsx:215) owns the programmatic writes — dictation, clear-after-send, draft restore.Both shipped in the same commit as the handler, which is what made it redundant from the start.
A
textContentread was also wrong for a second reason: directive chips render as their labels while the runtime serializes something else, so any host supplying slash-command chips had a latent text-corruption path through the same line.The now-unused
const aui = useAui()binding inComposergoes with it. TheComposerActioncomponent keeps its own.Verification
npx tsc --noEmit→ exit 0, no diagnostic inthread.tsx.pnpm lint→ 0 errors (84 pre-existingreact-hooks/set-state-in-effectwarnings across the app, none in this file).I could not reproduce the IME behaviour end-to-end here — it needs a CJK IME against a running desktop build, and this is a headless Windows box. What I did verify is the mechanism, in the shipped
SyncPluginsource quoted above: the guard that would have prevented the rebuild is keyed on a ref the removed handler never updated. If you would like a regression test rather than a deletion, the natural shape is acomposerRuntime.subscribespy asserting noapplyRuntimeTextwhileeditor.isComposing()— say the word and I will add it.Upstream precedent the report cites (assistant-ui#4506, #4510, #4513, #5416) all fix in this same direction: remove the mid-composition write-back.
Summary by CodeRabbit
Bug Fixes
Tests