Skip to content

fix(desktop): prevent IME submit in inline edit composer - #40557

Closed
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/desktop-inline-edit-ime-submit
Closed

fix(desktop): prevent IME submit in inline edit composer#40557
izumi0uu wants to merge 1 commit into
NousResearch:mainfrom
izumi0uu:fix/desktop-inline-edit-ime-submit

Conversation

@izumi0uu

@izumi0uu izumi0uu commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Extends the Desktop IME handling work from #40210 to the separate inline-edit composer used for editing already-sent user messages.

#40210 fixed the main chat composer path, but the inline editor in apps/desktop/src/components/assistant-ui/thread.tsx still had its own Enter-to-submit path without the same composition guards or onCompositionEnd draft flush. As a result, pressing Enter to confirm a CJK IME candidate while editing an already-sent message could immediately submit the edit.

This PR brings that inline-edit path into parity with the main composer by tracking composition state, skipping preedit draft sync, ignoring submit-on-Enter while composing, and flushing committed text on compositionend.

Related Issue

Fixes #40544

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • add IME composition tracking to the inline edit composer in apps/desktop/src/components/assistant-ui/thread.tsx
  • prevent Enter from submitting inline edits while isComposing / composition is active
  • flush committed editor text on onCompositionEnd so the inline edit path matches the #40210 main-composer behavior
  • add apps/desktop/src/components/assistant-ui/user-edit-ime-repro.test.tsx to lock the regression with a focused DOM repro

How to Test

  1. Open Hermes Desktop and send any user message.
  2. Edit that previously sent message, switch to a CJK IME, and start a composition.
  3. Press Enter to confirm the IME candidate.
  4. Confirm the edit is not submitted during composition.
  5. Press Enter again after composition ends and confirm the edit submits normally.

Automated checks run locally:

  1. cd apps/desktop && npx vitest run --environment jsdom src/components/assistant-ui/user-edit-ime-repro.test.tsx
  2. cd apps/desktop && npx vitest run --environment jsdom src/app/chat/composer/ime-composition-dom-repro.test.tsx
  3. cd apps/desktop && npm run type-check
  4. cd apps/desktop && npx eslint src/components/assistant-ui/thread.tsx src/components/assistant-ui/user-edit-ime-repro.test.tsx

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.7.7

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Regression is covered by a focused DOM test for the inline edit composer path, plus the existing main-composer IME repro test from #40210.

@alpindiay alpindiay 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: fix(desktop): prevent IME submit in inline edit composer

Summary

Prevents IME (Input Method Editor) composition events from leaking into draft state or triggering premature submission in the inline message edit composer. Mirrors the same IME composition guards already present in the main composer.

Issues Found

None. This is a clean, well-contained fix.

What Looks Good

  • composingRef tracks IME state identically to the main composer pattern
  • handleInput is gated on !composingRef.current — prevents preedit text from syncing to draft
  • submitEdit is gated on !composingRef.current — prevents Enter-during-composition from submitting
  • handleKeyDown checks both composingRef.current and event.nativeEvent.isComposing as a belt-and-suspenders guard
  • onCompositionEnd resets the ref and flushes the final composed text to draft
  • The flushEditorToDraft helper extracts the shared logic (empty BR cleanup + sync + trigger refresh), reducing duplication
  • Test reproduces the exact CJK IME scenario: compositionStart → type → Enter (isComposing=true) → no submit → compositionEnd → Enter → submit with correct text
  • No security concerns, no regression risk for non-IME users

Minor Suggestion (cosmetic)

The test file uses vi from vitest but vi.fn() only — consider import { vi } from "vitest" explicitly for clarity, though the current destructured describe, expect, it, vi pattern is consistent with the rest of the test suite.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 6, 2026

@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 isolating the inline-edit IME path. The underlying issue is still present on current main: apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx:573-576 submits Enter without checking composition state, and its editor wiring at :627-653 has no composition handlers.

Problems

  • The patch targets the former thread.tsx; commit 7ff6908a5 moved this component to thread/user-edit-composer.tsx. Port the guard and composition-end flush there instead.
  • user-edit-ime-repro.test.tsx tests a copied Harness, not the production editor. Current main's runtime-backed edit harness is thread/user-message-edit.test.tsx:80-115; extend that path so the regression remains coupled to actual UserEditComposer wiring.

Suggested changes

  • Apply the main-composer pattern from app/chat/composer/index.tsx:364-372 and :727-741 to thread/user-edit-composer.tsx.
  • Exercise composition start, IME Enter, composition end, and a subsequent submit against the real edit composer.

Automated hermes-sweeper review.

Comment thread apps/desktop/src/components/assistant-ui/user-edit-ime-repro.test.tsx Outdated
@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 14, 2026
@izumi0uu
izumi0uu force-pushed the fix/desktop-inline-edit-ime-submit branch from cc42cb7 to 68b3b19 Compare July 25, 2026 14:28
@izumi0uu

Copy link
Copy Markdown
Contributor Author

Addressed in 68b3b196. Ported the IME composition guard and composition-end draft flush to the current thread/user-edit-composer.tsx implementation, using both the composition ref and nativeEvent.isComposing to prevent candidate-confirmation Enter events from submitting an inline edit. Preedit input is excluded from draft synchronization, while committed text is flushed on compositionend.
I also removed the copied standalone Harness and moved the regression into the existing runtime-backed user-message-edit.test.tsx. The test now exercises the real Thread and UserEditComposer path: composition start, IME Enter without submission, composition end, and a subsequent Enter that submits exactly once with the committed text.

@david-bowiegxw

Copy link
Copy Markdown

Independent verification from an affected user (not the author) — this patch fixes a variant of #40544 that the issue doesn't currently describe, and I can confirm the bug is still live on today's main.

The variant

#40544 describes pressing Enter to confirm an IME candidate. There's a second, arguably more common way Chinese full-pinyin users hit this: to type literal ASCII without leaving the IME, you type the letters and press Enter instead of Space, which dismisses the candidate list and commits the raw buffer verbatim. So shili1 + Enter emits shili1.

That Enter is consumed by the IME, but the inline edit composer reads it as "save the edit" — the edit submits mid-word. For a bilingual user this fires constantly while editing an already-sent message, because mixing English tokens into Chinese text is routine.

Result

Tested against main at 6d1e08b2b, driving the real Thread / UserEditComposer path through the runtime-backed harness in user-message-edit.test.tsx:

compositionStart
  → input (preedit "…shili1")
  → keyDown Enter { isComposing: true }
  → compositionEnd { data: "shili1" }
  → input (committed)
  • On current main: fails. onEdit is called once during the composition — the edit is submitted with the just-committed text and the editor closes.
  • With this PR's user-edit-composer.tsx applied: passes. The composing Enter is swallowed, the editor stays open, and a subsequent non-composing Enter submits exactly once with the full committed text. The four existing tests in user-message-edit.test.tsx stay green (5/5).

So the composingRef + nativeEvent.isComposing guard covers this variant too — no additional change needed for it. Noting it here mainly because it's a distinct user-facing symptom worth having on record, and because it's independent confirmation that the regression is still reproducible on current main rather than only on the pre-refactor path.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for this fix! It was salvaged into #86760 (cherry-picked onto current main with your authorship preserved in the commit history) and is now merged. Closing since the work has landed.

@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

P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Desktop inline edit still submits on Enter during IME composition

5 participants