fix(desktop): Esc cancels a running turn when the focus bus is stale - #74406
Closed
Ahmett101 wants to merge 1 commit into
Closed
fix(desktop): Esc cancels a running turn when the focus bus is stale#74406Ahmett101 wants to merge 1 commit into
Ahmett101 wants to merge 1 commit into
Conversation
The global Esc-to-cancel handler bailed whenever getActiveComposer() !== target, which silently no-op'd whenever the active-composer tracker was out of sync with the mounted composer — after a tab switch, a tile mount, a dialog restore, or simply because focus never landed on the composer input (the transcript click repro in NousResearch#74374). The Stop button still worked because it bypasses the bus, so the halt path itself was fine; only the keybinding routing was broken. Track which composers are actually mounted (registerMountedComposer/ unregisterMountedComposer in focus.ts) and let the Esc handler fall back to the lone mounted busy composer when the focus bus disagrees with reality. The multiple-busy-composers ambiguity stays a no-op rather than halting the wrong one, and mounts/unmounts reconcile the set so a stale target can never resolve to a phantom composer. The pure resolveEscTarget helper is unit-tested. Closes NousResearch#74374
19 tasks
Contributor
|
Thanks for the focused investigation. This is an automated hermes-sweeper review: current
Closing as implemented on main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On the desktop app, pressing Esc during an active assistant turn did nothing (#74374), even though the Stop button halted the run immediately. The
haltRunpath was fine — the break was in the keybinding routing: the globaluseComposerEscCancelhandler bailed whenevergetActiveComposer() !== target, which silently no-op'd every time the active-composer tracker was out of sync with the mounted composer. That desync happens routinely — a tab switch, a tile mount, a dialog restore, or (the actual repro) the user clicking into the transcript so focus never lands on the composer input and the tracker never updates. The composer-local Esc handler (composer/index.tsx:766-783) only fires when the input itself has focus, so it never covered this surface.Changes
apps/desktop/src/app/chat/composer/focus.ts: add a module-levelmountedComposersset plusregisterMountedComposer/unregisterMountedComposer/getMountedComposers. This is the source of truth for which composers are actually live, independent of the input-focus-drivenactiveTarget.apps/desktop/src/app/chat/composer/index.tsx: register the composer's target on mount and unregister on unmount (in auseEffectkeyed onscope.target), so the mount set reconciles against reality and a stale target can never resolve to a phantom composer.apps/desktop/src/app/chat/composer/hooks/use-composer-esc-cancel.ts: extract a pureresolveEscTarget(target, activeTarget, busyByTarget, mounted)helper. The authoritative path (activeTarget === target && mounted.has(target)) is unchanged; when the focus bus is stale, fall back to the lone mounted busy composer. The multiple-busy ambiguity stays a no-op (never halt the wrong tile), and an unmounted target returns null (no stale-closure cancel).apps/desktop/src/app/chat/composer/hooks/use-composer-esc-cancel.test.ts: 6 unit tests forresolveEscTarget— authoritative match, stale-tracker lone-busy fallback, multiple-busy ambiguity, unmounted-target rejection, focus-bus preference agreement, and unmount reconciliation. No DOM needed; the helper is pure.How to Test
cd apps/desktop && npx vitest run src/app/chat/composer/hooks/use-composer-esc-cancel.test.ts
6 passed
Manual: with a streaming turn running in the desktop app, click into the transcript and press Esc — the run now halts, matching the Stop button.
Checklist
Risk & Impact
Low. The authoritative path is unchanged (same behavior when the focus bus agrees); the new code only fires when the tracker is stale, and only when exactly one mounted composer is busy. Existing Stop-button behavior is untouched. The mount-tracking set is a soft signal used only by this fallback.
Type: Bug fix
Closes: #74374