fix(workspace): suppress stale "still running" toast when input box is hidden for non-command reasons#10742
fix(workspace): suppress stale "still running" toast when input box is hidden for non-command reasons#10742lonexreb wants to merge 3 commits into
Conversation
…s hidden for non-command reasons The toast "A command in this session is still running." was previously gated solely on `!is_input_box_visible`. The input box is hidden for many reasons unrelated to a running command — pending shared session, alt-screen application, SSH choice block, AI confirmation block, freshly-created pane in startup — and the toast fired falsely in those cases. `focus_terminal_input` also created a new pane via `unwrap_or_else` even in the `RequireExisting` path, then turned around and either returned None or fired the toast, leaving a surprise pane behind. This restructures `focus_terminal_input` so a new pane is only created under the explicit "open new" branch, and adds an `is_active_block_running` predicate on `TerminalView` that gates the busy toast on whether the active block is actually executing. Closes warpdotdev#9100.
github-actions
Bot
added
the
external-contributor
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR narrows the stale "still running" toast by separating active-session lookup from pane creation and by adding a running-block predicate for the toast gate.
Concerns
- The new running-block predicate still returns true for agent-controlled/tagged-in executing commands before the exclusion can apply.
- The fallthrough path for unavailable existing sessions still returns the current active session without creating the new pane promised by
OpenIfNeededand the new comments. - No security findings were identified in the changed lines.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| active_block.is_executing() | ||
| || (active_block.is_active_and_long_running() | ||
| && !active_block.is_agent_in_control() | ||
| && !active_block.is_agent_tagged_in()) |
There was a problem hiding this comment.
is_executing() makes this return true before the agent-control exclusion is evaluated, so blocked/tagged agent flows with an executing active block still show the stale busy toast. Gate the executing case with the same agent-flow exclusion.
| active_block.is_executing() | |
| || (active_block.is_active_and_long_running() | |
| && !active_block.is_agent_in_control() | |
| && !active_block.is_agent_tagged_in()) | |
| if active_block.is_agent_in_control() || active_block.is_agent_tagged_in() { | |
| return false; | |
| } | |
| active_block.is_executing() || active_block.is_active_and_long_running() |
| if existing_terminal_view_handle.is_none() { | ||
| active_pane_group.update(ctx, |pane_group, ctx| { | ||
| pane_group.add_terminal_pane(Direction::Right, None /*chosen_shell*/, ctx); | ||
| }); | ||
| } |
There was a problem hiding this comment.
OpenIfNeeded callers still get the hidden/busy active session back from active_session_view(ctx) instead of a new usable pane. After this fallthrough, add the replacement pane unconditionally.
| if existing_terminal_view_handle.is_none() { | |
| active_pane_group.update(ctx, |pane_group, ctx| { | |
| pane_group.add_terminal_pane(Direction::Right, None /*chosen_shell*/, ctx); | |
| }); | |
| } | |
| active_pane_group.update(ctx, |pane_group, ctx| { | |
| pane_group.add_terminal_pane(Direction::Right, None /*chosen_shell*/, ctx); | |
| }); |
…allthrough (warpdotdev#9100) Two issues raised by the oz-for-oss bot: 1. `is_active_block_running` previously checked the in-band-command and `is_executing()` short-circuits BEFORE the agent-controlled / tagged-in exclusion. For agent-driven long-running commands, `is_executing()` returns true and would fire the "still running" toast even though the user is intentionally interacting with the agent (and the input box is hidden by design). Reorder: the agent-control / tagged-in gate now runs FIRST and short-circuits to `false`, so executing-state checks can only apply to genuinely user-driven commands. 2. `focus_terminal_input` fallthrough for `OpenIfNeeded` only created a new pane when there was no existing session at all. When an existing session was present but unusable (input not visible, `should_show_busy_toast` suppressed by `OpenIfNeeded`), it silently returned the busy session instead of honoring the `OpenIfNeeded` contract. Update the condition so `OpenIfNeeded` always creates a new pane on fallthrough. Tests added: - `is_active_block_running_returns_false_for_never_started_block` - `is_active_block_running_returns_false_for_finished_block` - `is_active_block_running_returns_true_for_user_long_running_block` - `is_active_block_running_returns_false_when_user_tagged_in_agent` - `is_active_block_running_returns_false_when_agent_is_in_control` - `focus_terminal_input_open_if_needed_creates_new_pane_when_existing_session_busy`
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR narrows the stale "still running" toast to sessions with actually running blocks and avoids eager pane creation in focus_terminal_input, with regression coverage for the new predicates and OpenIfNeeded behavior.
Concerns
focus_terminal_inputcan still return an existing hidden/unavailable session forOpenIfNonewhen the input is hidden for non-running reasons, letting callers operate on an input that is not focusable instead of no-oping or opening a usable pane.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // session as if it were focusable. Returning the busy session would | ||
| // reintroduce #9100 by letting callers act on a session they can't | ||
| // actually type into. | ||
| let needs_new_pane = existing_terminal_view_handle.is_none() |
There was a problem hiding this comment.
OpenIfNone still reaches this fallthrough when an existing session's input is hidden for non-running reasons; because a handle exists and the fallback is not OpenIfNeeded, needs_new_pane is false and the function returns the hidden/unusable session to callers. Track that unavailable-existing-session case separately and return None or open a usable pane according to the intended contract.
When focus_terminal_input is called with OpenIfNone and an existing session is found whose input box is hidden for non-running reasons (alt-screen application, SSH choice block, AI confirmation block, init-project step, cloud-agent pre-first-exchange), the prior fallthrough returned the existing unfocusable session. Callers would then proceed as if they had a focusable input and the user would see no effect. Return None in that case instead. OpenIfNone semantics: create a new pane only when there is no existing session at all; return None when the existing session is not usable (OpenIfNeeded handles the create path for unusable sessions). Add unit test focus_terminal_input_open_if_none_returns_none_when_existing_input_hidden covering the alt-screen hidden-input case.
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
oz-for-oss
Bot
dismissed stale reviews from themself
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR updates focus_terminal_input so it no longer creates surprise panes before evaluating fallback behavior and only shows the stale-command toast when the active terminal block is actually running. It also adds predicate and workspace regression coverage for the busy-toast and pane-creation paths.
Concerns
- No blocking correctness, security, or error-handling concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
oz-for-oss
Bot
requested review from
a team and
zachbai
and removed request for
a team
1 participant
Closes #9100.
Summary
The toast "A command in this session is still running." fires falsely when the user clicks something in Warp Drive (or other call sites of
focus_terminal_input) and the active terminal's input box happens to be hidden for reasons unrelated to a running command. This restructuresfocus_terminal_inputto (a) only create a new pane in the explicit "open new" branch and (b) only fire the busy toast when the active block is actually executing.Root cause
app/src/workspace/view.rs:focus_terminal_input(lines ~15165–15237 before this change):active_session_view(...).unwrap_or_else(|| { add_terminal_pane(...); ...unwrap() })to obtain a terminal view handle. Even for theRequireExistingandOpenIfNonepaths, this always created a new pane when none existed.self.is_input_box_visible(ctx). If false, and the fallback behavior was anything other thanOpenIfNeeded, it fired the stale-command toast and returnedNone— leaving the user with a surprise new pane and a misleading "still running" error.TerminalView::is_input_box_visiblereturnsfalsefor many reasons that have nothing to do with a running command:shared_session_status().is_view_pending()(app/src/terminal/view.rs:7227) — true for freshly-created panes still establishing the session.is_alt_screen_active()without agent control/tag-in (view.rs:7220).view.rs:7261–7278).view.rs:7280–7287).None of those are "a command is running." Mapping all of them to the
still runningtoast is the bug reported in #9100.Fix
app/src/workspace/view.rsfocus_terminal_inputnow resolves the existing active session without creating a new pane up front. The new pane is only added in the explicit "open new" branch at the bottom, eliminating the surprise-pane behavior forRequireExisting.should_show_busy_toasthelper that returnstrueonly when the active block is actually executing.app/src/terminal/view.rsTerminalView::is_active_block_runningpredicate that composes the existingis_writing_or_executing_in_band_command,Block::is_executing, andBlock::is_active_and_long_runningpredicates. We intentionally exclude blocks under agent control / tagged-in agents so the toast doesn't surface for agent flows that legitimately hide the input.Test plan
app/src/terminal/model/block_tests.rs— new regression testtest_idle_and_finished_blocks_are_not_executingthat asserts the underlying predicates we now use to gate the toast (Block::is_executing,Block::is_active_and_long_running) returnfalsefor:These are exactly the states that previously surfaced the false toast in #9100.
Manual testing note
End-to-end repro requires building the app, which depends on
crates/warpuicompiling Metal shaders via Xcode'smetaltoolchain (xcrun metal). The Xcode toolchain is not available in this environment — only the Command Line Tools are — socargo check -p warpandcargo test -p warpboth fail at thecrates/warpui/build.rs:92shader-compile step:For local verification I therefore relied on:
terminal/view.rs::is_input_box_visible,pane_group/mod.rs::active_session_view, andterminal/model/block.rsto confirm the predicates compose correctly.cargo check -p commandto confirm the shared workspace deps still build.A reviewer with the full Xcode toolchain can finish the end-to-end repro by:
vim, switch to alt-screen, or open Warp Drive on a fresh tab while shared-session is still resolving).focus_terminal_inputcall site withRequireExistingorOpenIfNone.RequireExisting) / opens a new pane (OpenIfNeeded) as documented onTerminalSessionFallbackBehavior.