Skip to content

fix(desktop): guard composer Enter during IME candidate confirmation - #40015

Closed
1525164075 wants to merge 1 commit into
NousResearch:mainfrom
1525164075:fix/desktop-ime-enter-composer
Closed

fix(desktop): guard composer Enter during IME candidate confirmation#40015
1525164075 wants to merge 1 commit into
NousResearch:mainfrom
1525164075:fix/desktop-ime-enter-composer

Conversation

@1525164075

Copy link
Copy Markdown

Summary

  • Prevent the desktop chat composer from treating IME candidate-confirmation Enter key events as message submission.
  • Handle native composition, Process-key events, and keyCode/which 229 cases.
  • Add focused tests for IME Enter handling while preserving normal Enter submit behavior.

Test plan

  • npm run test:ui -- src/app/chat/composer/ime-keyboard.test.ts
  • npm run test:ui -- src/app/chat/composer
  • npm run type-check
  • npx eslint src/app/chat/composer/index.tsx src/app/chat/composer/ime-keyboard.ts src/app/chat/composer/ime-keyboard.test.ts

@daimon-nous daimon-nous Bot added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 5, 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 the focused main-composer guard.

Problems

  • The same bug class remains in the current user-edit composer: apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx:527-577 handles Enter and calls submitEdit() without a composition/229 guard; its editor at :627-652 has no composition handlers.
  • apps/desktop/src/app/chat/composer/ime-keyboard.test.ts:7-24 verifies only the pure predicate, not that a candidate-confirmation event cannot reach the production composer submit or trigger-selection paths.

Suggested changes

  • Apply the shared guard to the current user-edit composer before its trigger and submit branches, and add its composition lifecycle handling.
  • Add a DOM regression that proves 229/Process Enter neither submits nor selects a trigger item.

The linked #43405 discussion identifies #38794 as the canonical IME-fix cluster PR; that does not remove the live main-composer gap at apps/desktop/src/app/chat/composer/index.tsx:364-372.

Automated hermes-sweeper review.

})

it('lets the IME consume Enter for keyCode 229 candidate confirmation', () => {
expect(shouldLetImeHandleKeyDown({ key: 'Enter', keyCode: 229, isComposing: false })).toBe(true)

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.

This verifies only the pure helper. Please add a DOM-level regression that drives the composer keydown path and asserts a 229/Process candidate-confirmation event cannot submit or select a trigger item.

@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

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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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.

2 participants