Repository navigation
Restrict desktop Escape interruption to confirmed foreground work - #1852
nortonandreev wants to merge 2 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueNo actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: limiting desktop Escape interruption to confirmed foreground work. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 4 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
PR Summary by QodoRestrict desktop Escape to confirmed foreground turns
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record |
nortonandreev
left a comment
nortonandreev
left a comment
There was a problem hiding this comment.
The main issue is the first comment on ChatTabViewModel.cs: AwaitingInput == false is also what an idle PTY prompt reports, so Escape can still reach one. The rest are refresh fragility, a latent Escape bubbling change, and two tests that don't pin the new gate. Moving this to draft until the first comment is resolved.
| public bool ShowsInterrupt => _showsInterrupt.Value; | ||
|
|
||
| // Running alone, or background work while the parent waits, does not make Escape safe at its prompt. | ||
| public bool CanInterruptWithEscape => ShowsInterrupt && _awaitingInput == false; |
There was a problem hiding this comment.
AwaitingInput == false isn't proof of a foreground turn: the daemon also reports false for a PTY agent idle at its prompt.
- Launched without a prompt:
AgentActivityClock._awaitingInputstartsfalseand only a relayed Stop (or the ACP turn gate's falling edge) sets ittrue. - A vendor whose hooks never relay Stop reports
falsefor its whole life. - Every submit clears it (
IsSubmit→ClearAwaitingInputSince), so a submit that starts no turn (a local/help) leaves itfalsewith no Stop to follow.
In each case the status is Working, the button reads "Interrupt (Esc)", and Escape sends 0x1b to an idle prompt, which is the case this PR sets out to prevent.
There was a problem hiding this comment.
Fixed in c690b4f. The daemon now sends TurnAttested, which is true only while an ACP turn gate is held, or after a relayed UserPromptSubmit/PreToolUse hook with no Stop or quiet mark since. A delivered input or a keyed submit never sets it, so a launch without a prompt, a vendor that never relays Stop, and a local /help all leave it false. Escape requires it; the button still works without it.
| readonly ObservableAsPropertyHelper<bool> _showsInterrupt; | ||
| public bool ShowsInterrupt => _showsInterrupt.Value; | ||
|
|
||
| // Running alone, or background work while the parent waits, does not make Escape safe at its prompt. |
There was a problem hiding this comment.
This rule restates one disjunct of SessionStatusDots.IsWorking (awaitingInput == false || liveSubagents > 0 || liveCommands > 0) from a raw field. Exposing "confirmed foreground turn" as one fact on AgentStatusPresentation, next to IsWorking/IsRunningUnknownTurn, would keep the rail, notifications and the interrupt chip agreeing, and would let this become a reactive property folded into the _showsInterrupt pipeline instead of a non-notifying getter.
There was a problem hiding this comment.
Done. InForegroundTurn is now on AgentStatusPresentation, set by SessionStatusDots.Present (Working and attested). CanInterruptWithEscape is a reactive property built from ShowsInterrupt and AgentStatus.
| && (AgentStatus.Kind == AgentStatusKind.Working || AgentStatus.IsRunningUnknownTurn)) | ||
| .DisposeWith(_disposables); | ||
|
|
||
| _interruptLabel = this.WhenAnyValue(x => x.ShowsInterrupt, x => x.AgentStatus, |
There was a problem hiding this comment.
The selector ignores its inputs and reads CanInterruptWithEscape, which depends on the plain field _awaitingInput. The label refreshes only because AgentStatus is a fresh, unequal record on every refresh (its Facts list compares by reference). If the record becomes value-equal, an AwaitingInput flip with LiveSubagents = 1 keeps the same Kind and facts, so nothing fires and the label disagrees with what Escape does.
There was a problem hiding this comment.
Fixed. InterruptLabel now derives from the reactive CanInterruptWithEscape, and InForegroundTurn is part of the presentation record's value, so a flip in attestation alone changes the record.
| @@ -608,11 +608,6 @@ bool ListTextBoxOwnsFocus() { | |||
| /// TextBox's own newline. ↑/↓ recall sent prompts only from the first/last line, so inside a | |||
| /// multi-line recall they stay the TextBox's own caret moves until the caret reaches an edge. | |||
| void OnComposerKeyDown(object? sender, KeyEventArgs e) { | |||
There was a problem hiding this comment.
With the tunnel handler gone, a bare Escape in the composer is no longer consumed when there's nothing to interrupt: TryInterrupt returns false and the key bubbles past ChatTabView to every ancestor. Nothing handles it there today, so this is latent, but a later window, dock or tab-host Escape binding would fire while the user is just typing.
There was a problem hiding this comment.
Fixed. ChatTabView.OnKeyDown now consumes a bare Escape that comes from the composer even when there is nothing to interrupt. The idle-Escape smoke test asserts the window never sees it.
|
|
||
| [Test] | ||
| [Arguments(null, 0)] | ||
| [Arguments(true, 0)] |
There was a problem hiding this comment.
(true, 0) passed before this PR: the status is Idle, ShowsInterrupt is already false, and InterruptCommand.CanExecute already blocked Escape. Reverting the CanInterruptWithEscape check leaves this row green. Rows for the AwaitingInput == false-at-an-idle-prompt cases would pin the gap above.
There was a problem hiding this comment.
Replaced. The rows are now (false, null, 0), (false, false, 0) and (true, false, 1): Working with no attestation, which passes only because of the new gate. Each row also asserts ShowsInterrupt is true and CanInterruptWithEscape false before the key press.
| } | ||
|
|
||
| [Test] | ||
| [NotInParallel("AvaloniaSession")] |
There was a problem hiding this comment.
Nothing asserts the precondition that ShowsInterrupt and CanInterruptWithEscape are true when Escape is pressed, and there's no settle after AddOrUpdate. If the status hasn't reached the view model yet, SentInput stays empty whether or not the child handler ran, and the test passes without proving the handler ordering.
There was a problem hiding this comment.
Fixed. The child-handler and context-menu tests now settle after AddOrUpdate and assert CanInterruptWithEscape is true before pressing Escape.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
An idle PTY prompt also reports AwaitingInput false, so the daemon now sends TurnAttested: true only after an ACP turn gate or a relayed submit or tool-call hook, until the next turn end or quiet mark.
c690b4f to
ac0d7a7
Compare
No GitHub or Linear issue was provided.
What & why
Escape in desktop chat interrupts only a confirmed foreground turn. Idle and unknown-state prompts, including parents waiting on background work, ignore the shortcut; child controls get the first chance to dismiss UI.
Where to look
Remote sessions with uncertain turn status retain the explicit Interrupt action. The button advertises Esc only when the shortcut is available.
Verification
LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 dotnet run --project test/Capacitor.App.Tests.Unit --no-restore --disable-build-servers -- --maximum-parallel-tests 4: 4,217 passed, 10 skipped on the final commit.UiScaleTests.A_change_is_published_and_kept_for_the_next_start; it passes with English locale.dotnet testfailed test-process handshakes before executing tests.Summary by CodeRabbit