Skip to content

fix(desktop): Esc interrupts running turn even when trigger popover is open (#74374) - #74428

Open
webtecnica wants to merge 2 commits into
NousResearch:mainfrom
webtecnica:fix/74374-esc-interrupt
Open

fix(desktop): Esc interrupts running turn even when trigger popover is open (#74374)#74428
webtecnica wants to merge 2 commits into
NousResearch:mainfrom
webtecnica:fix/74374-esc-interrupt

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Description

Fixes #74374 — pressing Esc during an active assistant turn now correctly interrupts generation, matching Stop-button behavior.

Root Cause

In handleEditorKeyDown, the trigger popover's Escape handler (line ~624-630) catches the Escape key when any @ or / completion popover is open. It closes the popover via closeTrigger() and returns — but never reaches the general Esc handler at line ~788-805 which calls haltRun() to stop the running turn.

This means: when a trigger popover was active during generation, pressing Esc would dismiss the popover but the turn kept running, requiring a second Esc press to stop. When focus was in the composer input without a trigger popover, Esc worked correctly (line 788 caught it).

Fix

After closing the trigger popover, the handler now also checks busy && !awaitingInput and calls haltRun() to stop the running turn. One Esc press now both dismisses the popover AND interrupts generation.

Changed file

  • apps/desktop/src/app/chat/composer/index.tsx: Add haltRun() call after closeTrigger() when turn is busy.

…Research#74387)

The per-profile route-scoping introduced in c4212b9 correctly writes
profile-scoped localStorage keys for both the remembered route and
session id, but the cold-start restore effect in use-desktop-integrations
reads .get() inside the effect body without
subscribing to it.

On mount,  is 'default' (its initial value). The
real profile is resolved asynchronously by adoptPrimaryProfile() via an
IPC round-trip to the Electron main process — the restore effect fires
before that IPC completes, so it reads profile-scoped keys from the
'default' scope while the true profile's keys hold the correct values.
By the time adoptPrimaryProfile() finishes, restoredRef already blocks a
second run, so the wrong session is restored on every cold start.

Fix:
- Add  atom (false until the gateway profile resolves)
- Set it in adoptPrimaryProfile() and adoptBoot() (HMR path)
- Mirror it into React state via useSubscription in use-desktop-integrations
- Gate the restore effect on profileReady so it waits for the real profile
  before reading localStorage keys

This is the remaining half of the route-scoping fix: the write side was
already correct (scoped per profile), but the read side needed a timing
gate to pair the scoped write with a scoped read.
…er is open

The trigger popover's Escape handler (line 624-630 of handleEditorKeyDown)
caught the key, closed the popover, and returned — but never reached the
general Esc handler at line 788 that calls haltRun() to stop a running
turn.  When a trigger (@ or / completion popover) was active during
generation, a single Esc press would dismiss the popover but the turn
kept running, requiring a second press to stop.

Fix: after closeTrigger(), check busy && !awaitingInput and call
haltRun() to stop the running turn.  One Esc press now both dismisses
the popover AND interrupts generation.

Fixes NousResearch#74374
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) needs-decision Awaiting maintainer decision before any implementation labels Jul 29, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #74406 for #74374, but this uses a different Esc mechanism. The branch also carries the unrelated cold-start restore patch from #74426/#74388; please separate or consolidate that carried work before selecting the Esc repair.

@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 trigger-popover path. The reported bug is still present on current main: apps/desktop/src/app/chat/composer/index.tsx:648-653 closes the populated trigger popover and returns before the busy-turn Escape halt at :821-828. Calling the existing haltRun() is consistent with the current Stop/Esc queue-parking semantics at :272-284.

Problems

  • This branch also carries the separate cold-start restore change across use-desktop-integrations.ts, use-gateway-boot.ts, and store/profile.ts (commit 6c81772c3828). Please separate or explicitly consolidate that work, as the existing maintainer comment requests.
  • The Esc behavior has no regression test. Add coverage for a populated trigger popover + busy && !awaitingInput, asserting one Escape both closes the popover and invokes cancellation; retain coverage that awaitingInput does not halt.

Suggested changes

  • Salvage the focused Esc hunk from 62f5e622080d after resolving its current-main context, then add the behavior test.

This is an automated hermes-sweeper review.

// button and the general Esc handler below. The trigger popover is
// closed first so a single Esc both dismisses the popover and stops
// the generation, instead of needing a second Esc press.
if (busy && !awaitingInput) {

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 regression test for this branch: with a populated trigger popover and busy && !awaitingInput, one Escape should close the popover and invoke cancellation. Cover the paired awaitingInput case to preserve the existing non-interrupt contract.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Two PRs reference #74374, but they address different Escape-routing paths: #74406 adds stale active-composer fallback machinery, while #74428 fixes the trigger-popover branch that returns before the existing busy-turn cancellation handler.

Related pull requests

Suggested consolidation

Keep #74428 open with the reviewed salvage path: retain only the trigger-popover Escape fix, split out the cold-start restore work identified by the contributor reviews, and add the requested regression coverage. Retain #74406's closure as already implemented on main; the PRs are not duplicates because #74406 concerns stale composer targeting while #74428 covers an independent early return in the trigger-popover handler.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I74374(["issue #74374 (closed)"])
    P74428["PR #74428 (open)"]
    P74428 -->|best fix| I74374
    class I74374 closed
    class P74428 open
    class P74428 best
    class P74428 target
    click I74374 "https://github.com/NousResearch/hermes-agent/issues/74374"
    click P74428 "https://github.com/NousResearch/hermes-agent/pull/74428"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 17 kB of PR diffs, 7 kB of issue/PR text, 2 kB of discussion (3 comments), 4 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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/*) needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists 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-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Desktop — Esc does not interrupt a running turn (Stop button works)

4 participants