Skip to content

fix(desktop): respect IME composition in composers - #38800

Closed
t3ta wants to merge 2 commits into
NousResearch:mainfrom
t3ta:fix-desktop-ime-composition-edit-composer
Closed

fix(desktop): respect IME composition in composers#38800
t3ta wants to merge 2 commits into
NousResearch:mainfrom
t3ta:fix-desktop-ime-composition-edit-composer

Conversation

@t3ta

@t3ta t3ta commented Jun 4, 2026

Copy link
Copy Markdown

Summary

This improves Desktop composer behavior for CJK and other IME-based input methods.

When typing Japanese, Chinese, Korean, or other text through an IME, Enter is commonly used to confirm the current preedit/composition text. The composer should treat that Enter as part of text input, not as a request to send or submit the message.

This PR:

  • keeps the main chat composer in sync after IME composition ends
  • adds the same composition guard to the user edit composer
  • avoids draft/trigger updates while the edit composer still contains active preedit text
  • prevents Enter during active composition from submitting an edited message

Why

The main composer already had a composition-aware Enter guard, but composition end did not explicitly flush the finalized text through the same input path. The edit composer also did not track composition state, so confirming CJK preedit text with Enter could be interpreted as submitting the edit.

For CJK users, this makes normal text entry feel unsafe: a user can accidentally send or submit a half-composed message while simply trying to confirm conversion candidates.

Testing

  • npm --prefix apps/desktop run type-check
  • npm --prefix apps/desktop run test:ui -- src/app/chat/composer/slash-nav-dom-repro.test.tsx src/app/chat/composer/rich-editor.test.ts
  • npx eslint src/app/chat/composer/index.tsx src/components/assistant-ui/thread.tsx (from apps/desktop)
  • built and launched a local macOS Desktop app from this branch

Note: full npm --prefix apps/desktop run lint currently reports unrelated existing lint errors outside this change.

@t3ta t3ta closed this Jun 4, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Looks Good — minor cosmetic note

Small, well-scoped fix for IME composition handling in the desktop composer. The handleCompositionEnd callback correctly defers the input processing until composition ends, preventing premature submission while the user is composing CJK text.

The blank lines added inside the onSubmit handler (around the composingRef.current guard) are a minor readability nit — consider a comment like // skip submit while IME composition is active to make the intent explicit, but this is not blocking.

✅ Looks Good

  • Clean separation of composition lifecycle events
  • Reuses existing composingRef pattern consistently
  • Low-risk change

Reviewed by Hermes Agent

@mizugeek

mizugeek commented Jun 4, 2026

Copy link
Copy Markdown

Thanks for the PR! This successfully fixes the text synchronization issue by updating the state right on compositionend.

However, this change alone does not fully prevent the premature message submission on macOS/Chromium.

The Problem on macOS

On macOS (Chromium/Electron), the event sequence when pressing Enter to commit the IME conversion is:

  1. compositionend fires -> composingRef.current is set to false.
  2. keydown (Enter) fires -> both composingRef.current and event.nativeEvent.isComposing are already false.

Because composingRef.current becomes false before keydown runs, handleEditorKeyDown bypasses the composition guard and immediately triggers submitDraft(). The message is still sent prematurely on macOS.

Suggested Solution

We can introduce a transient flag (e.g., compositionJustEndedRef) to temporarily block the synchronous keydown (Enter) event immediately following the composition end, while keeping composingRef updated instantly so that the final onInput text sync is not blocked.

Here is the suggested adjustment:

1. Define the ref:

const compositionJustEndedRef = useRef(false);

2. Update onCompositionEnd to set the transient flag:

onCompositionEnd={() => {
  composingRef.current = false;
  compositionJustEndedRef.current = true;
  setTimeout(() => {
    compositionJustEndedRef.current = false;
  }, 100);

  // Sync the committed text immediately
  if (editorRef.current) {
    const nextDraft = composerPlainText(editorRef.current);
    if (nextDraft !== draftRef.current) {
      draftRef.current = nextDraft;
      aui.composer().setText(nextDraft);
    }
  }
}}

3. Update the guard in handleEditorKeyDown:

const handleEditorKeyDown = (event: KeyboardEvent<HTMLDivElement>) => {
  if (composingRef.current || compositionJustEndedRef.current) {
    return;
  }
  // ...
}

This ensures that the Enter key confirming the IME conversion is blocked on macOS, while subsequent normal Enter presses successfully submit the synchronized draft.

@t3ta

t3ta commented Jun 5, 2026

Copy link
Copy Markdown
Author

Thanks for the detailed macOS/Chromium event-order note. You were right: flipping composingRef on compositionend is not enough because the confirming Enter keydown can arrive immediately after compositionend with isComposing already false.

I pushed d276837 to add a short post-composition Enter guard in both the main composer and the edit composer. compositionend still syncs the committed text immediately, but the Enter that confirmed the IME preedit text is ignored for submission.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for addressing the macOS/Chromium event-order detail raised in the discussion. The underlying bug remains on current main: apps/desktop/src/app/chat/composer/index.tsx:727-740 clears composition state before the plain-Enter path at :558-589, and apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx:527-576 still submits edits on Enter with no IME guard.

Problems

  • The PR has no regression test for the key sequence it fixes: compositionend immediately followed by Enter with isComposing === false. Current apps/desktop/src/app/chat/composer/ime-composition-dom-repro.test.tsx:61-107 covers only composition-end text synchronization, and there is no edit-composer IME test.
  • The head is conflicting. thread.tsx was extracted into thread/user-edit-composer.tsx by 7ff6908a59536d2d788c8bc9aac64791829dbdeb; current main also already uses flushEditorToDraft() at composer/index.tsx:262-278 for the earlier composition-end synchronization fix (8e629b9f386d12b726bccb32e9d7b48402ea73ea).

Suggested changes

  • Salvage the post-composition Enter guard into both current components and retain the current main-composer flush path.
  • Add DOM regressions that verify the confirming Enter does not submit while a subsequent normal Enter does.

Automated hermes-sweeper review.

@@ -173,6 +174,7 @@ export function ChatBar({
const [focusRequestId, setFocusRequestId] = useState(0)
const dragDepthRef = useRef(0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a DOM regression for the macOS sequence this ref protects: composition end, then an immediate Enter with isComposing === false, asserting no submit; also assert a later ordinary Enter submits. The current IME regression test covers finalized-text synchronization only.

@teknium1

Copy link
Copy Markdown
Contributor

This fix landed on main via #86760, which consolidated the duplicate PRs for this bug (earliest submission by @satotakumi in #37487; all contributors credited in that PR's body). Closing as the fix is now merged. Thanks for catching it!

@teknium1 teknium1 closed this Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants