Skip to content

fix(desktop): respect IME enter in edit composer - #53753

Closed
2001Y wants to merge 1 commit into
NousResearch:mainfrom
2001Y:fix/desktop-edit-ime-enter
Closed

fix(desktop): respect IME enter in edit composer#53753
2001Y wants to merge 1 commit into
NousResearch:mainfrom
2001Y:fix/desktop-edit-ime-enter

Conversation

@2001Y

@2001Y 2001Y commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the desktop app's inline user-message edit composer so Enter during IME composition confirms the candidate text instead of submitting the edited message.

The main message composer already tracks compositionstart/compositionend and ignores keydown while composing. This applies the same behavior to the current edit composer path (user-edit-composer.tsx), then flushes the finalized composition on composition end.

Guards:

  • nativeEvent.isComposing
  • local composition ref
  • keyCode === 229 (legacy IME enter on some platforms)

Test plan

  • Focused Vitest IME + sanitizer: 12/12
  • Typecheck under Node 22
  • Full Desktop Vitest: 1,696 passed, 1 skipped (prior full run) + eslint/build
  • Rebased onto current main with range-diff equivalent patch

New head: 3374c099e0dab370f10ce9c421337343bfae1af1

@2001Y
2001Y force-pushed the fix/desktop-edit-ime-enter branch from 8f483d0 to 1f24a1e Compare June 27, 2026 17:45
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) labels Jun 27, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused IME regression fix. The reported behavior still exists on current main: apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx:573-576 submits unmodified Enter without checking composition state, and :475-476 synchronizes input during composition. The main composer already uses the proposed pattern at apps/desktop/src/app/chat/composer/index.tsx:303-312, :364-372, and :727-741.

Problems

  • The PR targets the removed apps/desktop/src/components/assistant-ui/thread.tsx. Refactors 7ff6908a5 and fa7bce078 moved the implementation to thread/user-edit-composer.tsx, so this needs a mechanical port rather than a direct cherry-pick.
  • Current main already has the test CSS.escape stub via 6016997a72bc34df899e2118fea958256d922cae; do not duplicate that part of the diff.

Suggested changes

  • Port only the composition guard/flush and regression test to the current component and test paths.

Automated hermes-sweeper review.

@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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@2001Y
2001Y force-pushed the fix/desktop-edit-ime-enter branch from 1f24a1e to 3374c09 Compare July 16, 2026 03:03
@2001Y

2001Y commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up update (author)

Rebased onto current main (f8b6d381e2f8e8b2cf84ac0e9e628ea062a173e3) and pushed after review/test pass.

What changed

  • IME Enter guard moved to current component path: user-edit-composer.tsx.
  • Guards on nativeEvent.isComposing, composition ref, and keyCode === 229.
  • Regression tests in user-message-edit.test.tsx (no duplicate CSS.escape stub).

Verification

  • Focused Vitest (IME + sanitizer): 12/12 passed
  • Typecheck: pass (Node 22)
  • Full Desktop Vitest previously: 1,696 passed, 1 skipped + eslint/build pass
  • Range-diff equivalent across latest main rebases

New head: 3374c099e0dab370f10ce9c421337343bfae1af1.

@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

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have 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-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.

3 participants