Repository navigation
Conversation
A structured send clears the draft asynchronously, on RPC acceptance. If the user opens the next IME composition first, the clear lands while the browser owns the field, the DOM sync skips it, and settlement adopts element.value — which still holds the message that was already sent. The field now records a clear dropped mid-composition and applies it at settlement, keeping only what the IME composed on top of the value the field held when the composition started. Browser ownership is unchanged for every other programmatic draft; the clear stays on the acceptance path, so a rejected send still keeps its draft. Also advances a frame before the attachment-flush focus assertions, which were vacuous because the focus they forbid is scheduled in rAF. Fixes stablyai#17359
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
Greptile SummaryThis PR defers structured-send draft clears that arrive during IME composition and replays them when composition settles, with integration coverage for accepted and rejected sends. It also makes the attachment focus assertion advance the animation frame.
Confidence Score: 2/5The PR should not merge until deferred settlement preserves repeated and supplementary-plane composition text and distinguishes launch-draft adoption from accepted-send clears. The new settlement algorithm can silently erase newly composed text, produce malformed Unicode, and discard launch-draft content under reachable asynchronous composition races. Files Needing Attention: src/renderer/src/components/native-chat/NativeChatComposerField.tsx
|
| Filename | Overview |
|---|---|
| src/renderer/src/components/native-chat/NativeChatComposerField.tsx | Adds deferred-clear replay at IME settlement, but the string diff can erase repeated-prefix input, split surrogate pairs, and misclassify launch-draft clears. |
| src/renderer/src/components/native-chat/native-chat-composer-composition.test.tsx | Adds blur-settlement and flag-reset coverage but omits ambiguous repeated-text and supplementary-plane cases. |
| src/renderer/src/components/native-chat/native-chat-structured-send-composition-clear.test.tsx | Adds realistic controlled-transport coverage for the reported race and rejected-send behavior. |
| src/renderer/src/components/native-chat/use-native-chat-composer-attachments.test.tsx | Correctly advances requestAnimationFrame so the existing no-focus-steal assertion observes scheduled behavior. |
Reviews (1): Last reviewed commit: "fix(native-chat): replay a draft clear d..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) { | ||
| suffix += 1 | ||
| } | ||
| return settled.slice(prefix, settled.length - suffix) |
There was a problem hiding this comment.
When the new composition inserts text matching the old draft's prefix, such as 안녕하세요 becoming 안안녕하세요, the prefix and suffix scans consume both the insertion and the entire base. The helper then returns an empty string, silently deleting the user's newly composed text.
Knowledge Base Used: Restore Terminal IME Composition Handling
| prefix += 1 | ||
| } | ||
| let suffix = 0 | ||
| while ( | ||
| suffix < limit - prefix && | ||
| base[base.length - 1 - suffix] === settled[settled.length - 1 - suffix] | ||
| ) { | ||
| suffix += 1 | ||
| } | ||
| return settled.slice(prefix, settled.length - suffix) |
There was a problem hiding this comment.
When an IME replaces one supplementary-plane character with another sharing a surrogate code unit, such as 😀 with 😁, the code-unit prefix scan places the slice boundary inside the surrogate pair. The settled draft consequently contains a lone surrogate and displays or sends a replacement character instead of the committed text.
Knowledge Base Used: Restore Terminal IME Composition Handling
| // Why: a clear (an async structured send confirming) would otherwise be lost outright and | ||
| // the sent text would ride along into the next message. Only clears are carved out of | ||
| // browser ownership; every other programmatic draft still loses to the live composition. | ||
| droppedDraftClearRef.current ||= draft === '' && textarea.value !== '' |
There was a problem hiding this comment.
When launch-draft adoption resolves during composition, React's stale draft can still equal the original launch text and trigger setDraft(''). This generic flag treats that update as an accepted-send clear, so settlement removes the composition-start base and discards launch-draft content the user was actively editing.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
NativeChatComposerField.tsx— records the value atonCompositionStart(compositionBaseRef) and flags adraft === ''render that is skipped while a composition is live;settleImeValuethen keeps only the composed segment (a both-ends diff) before delegating toonImeSettled, on bothcompositionendand theblurfallback.native-chat-composer-composition.test.tsx— field-level cases for the blur settle path and for not leaking a dropped clear into the next composition.native-chat-structured-send-composition-clear.test.tsx— new integration tests driving the realNativeChatComposeragainst a deferred structured transport: the reported race, composition away from the caret, rejected-send draft retention (plain and racing), and handled-but-rejected command error reporting.use-native-chat-composer-attachments.test.tsx— advances onerequestAnimationFrameso the deferred-focus-never-steals assertions actually exercise the scheduled focus.
ℹ️ The reverse ordering still loses freshly composed text
This fix covers only the "clear lands while a composition is live" ordering. In the mirror image — the user sends, starts the next composition, and commits it before the RPC resolves — draft already follows the composed text ('안녕하'), droppedDraftClearRef is never set, and the async .then in NativeChatComposer.tsx then runs setDraft('') unconditionally, wiping the newly composed text along with the sent text. This is pre-existing behavior outside the diff, and on fast local sessions the window is negligible, but it is widest on the same remote/SSH structured sessions this PR targets. Worth confirming the issue scope doesn't require the commit-before-clear ordering, or tracking it as a follow-up (a natural guard is only clearing when draft still equals the sent text).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…yai#17392) A structured send clears the draft asynchronously, on RPC acceptance. If the user opens the next IME composition first, the clear lands while the browser owns the field, the DOM sync skips it, and settlement adopts element.value — which still holds the message that was already sent. The field now records a clear dropped mid-composition and applies it at settlement, keeping only what the IME composed on top of the value the field held when the composition started. Browser ownership is unchanged for every other programmatic draft; the clear stays on the acceptance path, so a rejected send still keeps its draft. Also advances a frame before the attachment-flush focus assertions, which were vacuous because the focus they forbid is scheduled in rAF. Fixes stablyai#17359
Fixes #17359
The bug
A structured send clears the draft asynchronously, inside
dispatchNativeChatStructuredComposerText(...).then(...), and only on acceptance(
NativeChatComposer.tsx:255). If the user opens the next IME composition before the RPCresolves:
setDraft('')lands while a composition is active.NativeChatComposerField's layout effect skips the DOM write — the browser owns the valuewhile composing (fix(native-chat): preserve IME composition across async updates #17169).
onImeSettledthen adoptselement.valueas authoritative, and the already-sent text isstill in it.
Observed
dom = "안녕하"forsent = ["안녕"]: the composer keeps the message it just sent, andit rides along on the next one. The window is one RPC round trip and is widest on remote/SSH
structured sessions. The PTY path clears synchronously and is unaffected.
The fix
The issue lists two options; this takes the second.
Clearing optimistically before awaiting was rejected: the structured path guards on
if (!accepted) return, so an early clear would trade draft-resurrection for draft loss on arejected send.
Instead the field records that a programmatic clear was dropped mid-composition and applies it
at settlement:
compositionBaseRef— the value the field held when the IME started (captured in the existingonCompositionStart); that is exactly what the clear was meant to erase.droppedDraftClearRef— set in the existing DOM-sync effect when adraft === ''render isskipped because a composition is live.
compositionendand theblurfallback), the field keeps only thecomposed segment — diffed from both ends, since an IME edits at the caret, which need not be
the end.
This is the same "defer while composing, flush at settlement" shape the attachment queue already
uses. The "browser always wins at settlement" model from #17169 is preserved: only clears are
carved out. Any other programmatic draft still loses to the live composition, and the clear stays
on the acceptance path, so a rejected send keeps its draft.
Tests
New
native-chat-structured-send-composition-clear.test.tsxdrives the realNativeChatComposer(real field, real draft hook, real settle contract) against a structured transport whose
dispatchCommandresolves under test control:ends at
하, and the follow-up send dispatches하, not안녕하Plus two field-level cases in
native-chat-composer-composition.test.tsxfor the blur settle pathand for not leaking a dropped clear into the following composition.
Mutation-checked: reverting
NativeChatComposerField.tsxfails 3 of the new tests with exactlythe reported
안녕하symptom; dropping only the flag reset fails the leak test.Also fixed (issue's "Related" section)
use-native-chat-composer-attachments.test.tsxasserted the deferred flush never steals focus,but that focus call is inside
requestAnimationFrameand the test never advanced a frame — sothe assertions were vacuous. Verified by mutating the flush to
applyResolvedPaths(paths, true, ...):all 50 tests still passed before, and the mutant now fails.
Verification
pnpm tc— cleanpnpm test src/renderer/src/components/native-chat— 89 files, 908 tests passnpx oxlinton the changed files — clean;pnpm run check:code-quality:changed— 0 new findings