Skip to content

fix(desktop): read live DOM text for Ctrl+Enter steer to avoid stale-state race - #53668

Open
liuhao1024 wants to merge 16 commits into
NousResearch:mainfrom
liuhao1024:fix/desktop-ctrl-enter-steer-race
Open

fix(desktop): read live DOM text for Ctrl+Enter steer to avoid stale-state race#53668
liuhao1024 wants to merge 16 commits into
NousResearch:mainfrom
liuhao1024:fix/desktop-ctrl-enter-steer-race

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes a race condition where Ctrl+Enter / Cmd+Enter in the Desktop chat composer silently missed steer messages when the user typed and immediately pressed the shortcut. The handler gated on canSteer, which derives from React state (trimmedDraft) that lags the contentEditable DOM by one render.

Related Issue

Fixes #53659

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • apps/desktop/src/app/chat/composer/index.tsx: Read live DOM text via composerPlainText(editorRef.current) in the Ctrl+Enter handler instead of relying on canSteer (React-state-derived). Sync draftRef.current before calling steerDraft() so the function reads the just-typed text. Remove the canSteer gate from steerDraft() — the Ctrl+Enter handler already validates conditions using live DOM, and the Steer button visibility is separately gated on canSteer via ComposerControls props.
  • apps/desktop/src/app/chat/composer/ctrl-enter-steer-dom-race.test.tsx: Regression test covering the stale-state race (DOM has text, React state is empty), empty-editor no-op, not-busy no-op, and whitespace-only no-op.

How to Test

  1. Open Hermes Desktop, start a chat turn that is still busy/running.
  2. Focus the composer, type a short steering message, and immediately press Ctrl+Enter (Windows/Linux) or Cmd+Enter (macOS).
  3. The steer message should be sent reliably every time, regardless of typing speed.
  4. Run npx vitest run --environment jsdom apps/desktop/src/app/chat/composer/ctrl-enter-steer-dom-race.test.tsx — all 5 tests should pass.
  5. Run npx vitest run --environment jsdom apps/desktop/src/app/chat/composer/enter-submit-dom-race.test.tsx — existing plain-Enter race tests should still pass (5/5).

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 npx vitest run --environment jsdom on the affected test files 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

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

Code Intelligence

  • Analyzed: apps/desktop/src/app/chat/composer/index.tsxhandleEditorKeyDown Ctrl+Enter branch + steerDraft callback
  • Blast radius: LOW — only affects the Ctrl+Enter steer path in the Desktop composer; the Steer button's visibility is separately gated on canSteer via ComposerControls props
  • Related patterns: Desktop] Enter key doesn't send message — must add trailing space #39630 (plain Enter stale-state race fix) — same live-DOM-read pattern applied to Ctrl+Enter

…state race

The Ctrl+Enter steer handler gated on `canSteer`, which derives from React
state (`trimmedDraft`).  React state lags the contentEditable DOM by one
render, so typing quickly and pressing Ctrl+Enter immediately could miss
the steer because the state hadn't flushed yet.

Apply the same live-DOM-read pattern that the plain Enter handler already
uses (NousResearch#39630): read `composerPlainText(editorRef.current)` in the
Ctrl+Enter branch and check steer conditions against the live text.
Also sync `draftRef.current` before calling `steerDraft()` so the
function reads the just-typed text (same re-sync pattern as submitDraft).

Remove the `canSteer` gate from `steerDraft()` itself — the Ctrl+Enter
handler already validates conditions using live DOM, and the Steer button
visibility is separately gated on `canSteer` via ComposerControls props.

Fixes NousResearch#53659
@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have labels Jun 27, 2026
TypeScript error TS2774: 'This condition will always return true since
this function is always defined.' on line 63 where onSteer was typed as
required (text: string) => void but checked with if (onSteer && ...).

Make onSteer optional to match the actual component type in types.ts
(onSteer?: (text: string) => Promise<boolean> | boolean) and add a
test case for when onSteer is undefined.

@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 stale DOM/state race; the current main path still has the underlying defect.

Problems

  • This branch changes the old inline steerDraft implementation, but current main extracted it in 773a3703bfc1f8ff2f3aef40d7a565e7f4fe1404. The live shortcut still gates at apps/desktop/src/app/chat/composer/index.tsx:551, and the extracted implementation independently returns on stale canSteer at apps/desktop/src/app/chat/composer/hooks/use-composer-submit.ts:176-179. GitHub currently marks the PR DIRTY, so the fix needs a targeted port to those current paths.
  • ctrl-enter-steer-dom-race.test.tsx:16 tests a copied Harness rather than production ChatBar/useComposerSubmit; it cannot prevent a regression in the live implementation. It also lacks coverage for attachment and slash-command exclusions.

Suggested changes

  • Port the live-DOM validation and draft-ref synchronization to the current keydown path, and update the extracted steer guard without weakening the text-only/non-slash contract.
  • Add coverage through the current production component or submit hook for stale DOM text plus slash and attachment no-op cases.

Automated hermes-sweeper review.

// Regression repro for #53659: Ctrl+Enter right after typing (fast typing)
// did nothing. The `canSteer` guard derives from React state (`trimmedDraft`)
// which lags the contentEditable DOM by a render, so the keydown handler saw
// empty state and swallowed the steer. The fix reads the live editor text in

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 Harness duplicates the proposed implementation instead of exercising ChatBar or the extracted useComposerSubmit production path. On current main, the second stale canSteer gate lives in hooks/use-composer-submit.ts:176-179, so this test can pass while the shipped shortcut still fails. Please replace or supplement it with a test through the current component/hook boundary.

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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop composer Ctrl+Enter can miss freshly typed steer text

3 participants