fix(terminal-input): keep pinned input anchored during inline interactive prompts (#10144)#10729
Conversation
…tive prompts (warpdotdev#10144) When an inline interactive command like `aws configure` runs, the inferior's cursor walks down (and possibly back up) through the output grid. The grid's `max_cursor_point` watermark only grows — never shrinks — so `BlockGrid::len()` reports the deepest row ever reached even after the cursor returns to the actual prompt line. In `PinnedToBottom` layout this inflated height causes the active block to "reserve" a tall area in the blocklist viewport. The visible cursor (at the real prompt row) is rendered far above the bottom of the pane, leaving a large blank gap and effectively detaching the active input from the pinned bottom command line. Pressing Ctrl-C restores the expected layout because `Block::finish` truncates to cursor and resets the watermark. Warp already has the right mechanism for this: `trim_trailing_blank_rows` on the output grid caps `len_displayed()` at the bottommost row with actual content via a cached backward scan refreshed on each PTY-read batch. Today it is only turned on at CLI-agent session start (`view.rs::handle_cli_agent_sessions_event`). Enable trimming for every block at `preexec` time, gated on the existing `TrimTrailingBlankLines` feature flag so the rollout matches the CLI-agent path. The eager initial scan in `set_track_content_length` keeps the very first render correct; subsequent renders pay an O(trailing_blank_rows × cols) cost per PTY-read batch, which we already accept for CLI agents and is bounded by visible rows. Alt-screen programs (vim, top, less) don't render the output grid in the blocklist, so the toggle is a no-op for them. CLI-agent sessions continue to manage trim explicitly; the redundant call from this path is idempotent. Adds two unit tests in `blockgrid_tests.rs`: - `test_trim_trailing_blank_rows_handles_inline_interactive_prompt_pattern` reproduces the `aws configure` cursor-walk pattern and asserts that `len_displayed()` stays bounded by real content when trim is on. - `test_without_trim_inline_interactive_prompt_pattern_balloons_height` pins the current-without-fix behavior to guard the trim contract.
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 enables the existing trailing-blank-row trimming path during block preexec and adds BlockGrid unit coverage for the cursor high-watermark pattern behind #10144. The code change is limited to terminal block/grid behavior, and the security pass did not identify security-specific issues.
Concerns
- Manual testing evidence is missing for a user-visible terminal layout change. The PR includes manual repro steps but asks reviewers to verify on macOS and does not attach screenshots or a screen recording showing the pinned input before/after behavior end to end. Warp repo guidance requires visual/manual evidence for behavior that can be manually tested.
Verdict
Found: 0 critical, 1 important, 1 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
| // `view.rs::handle_cli_agent_sessions_event`; this call is idempotent | ||
| // and the second `set_track_content_length(true)` is a no-op (the | ||
| // eager backward scan recomputes the already-correct value). | ||
| if FeatureFlag::TrimTrailingBlankLines.is_enabled() { |
There was a problem hiding this comment.
💡 [SUGGESTION] Add a preexec-level test for this wiring; the new tests only cover BlockGrid with trimming enabled manually, so removing this feature-flagged call would not fail them.
|
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
their stale review
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR enables the existing trailing-blank-row trimming path for executing blocks behind the existing feature flag and adds focused BlockGrid coverage for the high-water-mark behavior that caused pinned-bottom prompts to detach.
Concerns
- No blocking correctness or security issues found.
- One non-blocking test coverage gap remains: the new tests exercise the trimming mechanism directly, but not the new preexec wiring that turns it on for active blocks.
Verdict
Found: 0 critical, 0 important, 1 suggestions
Approve with nits
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // and the second `set_track_content_length(true)` is a no-op (the | ||
| // eager backward scan recomputes the already-correct value). | ||
| if FeatureFlag::TrimTrailingBlankLines.is_enabled() { | ||
| self.output_grid.set_trim_trailing_blank_rows(true); |
There was a problem hiding this comment.
💡 [SUGGESTION] The added tests manually enable BlockGrid trimming, so they would still pass if this preexec wiring were removed. Add a handler-level test that exercises preexec with TrimTrailingBlankLines enabled and asserts the output grid trims displayed height.
oz-for-oss
Bot
requested review from
a team and
vorporeal
and removed request for
a team
1 participant
Closes #10144
Summary
Fixes #10144. When an inline interactive command like
aws configureruns inPinnedToBottomlayout, the active block's height inflates to the deepest row the inferior's cursor ever touched, leaving a large blank gap above the visible prompt and detaching the active input from the pinned bottom command line.Ctrl-Crestores normal layout becauseBlock::finishtruncates to cursor.The fix turns on the existing
trim_trailing_blank_rowsmechanism for every block atpreexec, gated on the sameTrimTrailingBlankLinesfeature flag used for CLI-agent sessions, so the rollout matches the existing path.Root cause
app/src/terminal/model/blockgrid.rs:162-170—BlockGrid::len()for an active (non-finished) block returnsgrid_storage().max_cursor_point.row + history_size + 1.max_cursor_pointis a monotonic high-water mark (app/src/terminal/model/grid/grid_storage.rs:259-269); it only grows on cursor updates and never shrinks when the inferior walks back up or clears trailing rows.Block::output_grid_displayed_height(app/src/terminal/model/block.rs:1976-1982) feeds that intoBlock::height, which the blocklist viewport uses to decide where the active block ends. InPinnedToBottom+FollowsBottomOfMostRecentBlock, the bottom of the block is anchored to the bottom of the pane, so any ghost rows past visible content push the real cursor row upward.Readline-style prompts (Python
input(),aws configure,node/pythonREPL) routinely produce this pattern via\r\x1b[Kredraws and cursor probes that move past the current line. Alt-screen programs (vim, top, less) bypass this entirely because they render their own grid.The trim mechanism that fixes it already exists —
GridHandler::set_track_content_lengthandBlockGrid::set_trim_trailing_blank_rowscaplen_displayed()at the bottommost row with visible content via a cached backward scan refreshed inon_finish_byte_processing. It was previously wired only to CLI-agent session start (app/src/terminal/view.rs:12253-12256).Fix
app/src/terminal/model/block.rs::preexec(~line 3315): afterstate = BlockState::Executing, callself.output_grid.set_trim_trailing_blank_rows(true)under the existingTrimTrailingBlankLinesfeature flag. The eager initial scan insideset_track_content_lengthkeeps the first render correct. Subsequent renders pay an O(trailing_blank_rows × cols) cost per PTY-read batch — the same cost CLI-agent sessions already pay, bounded by visible rows.The CLI-agent session handler still calls
set_trim_trailing_blank_rowsexplicitly; the redundant call frompreexecis idempotent.Test plan
Unit tests (in
app/src/terminal/model/blockgrid_tests.rs):test_trim_trailing_blank_rows_handles_inline_interactive_prompt_pattern— reproduces theaws configurecursor-walk pattern (write two prompt rows, jump cursor to row 20, return cursor to row 1) and assertslen()reflects the high-water mark (21) whilelen_displayed()stays bounded by real content (≤3 rows).test_without_trim_inline_interactive_prompt_pattern_balloons_height— pins the without-fix behavior so the trim contract can't silently regress: with trim off,len_displayed() == len() == 21even though only one row has visible content.Build status:
cargo check -p app— could not run locally: this environment is missing the macOSmetalshader toolchain thatwarpui/build.rsrequires. The two changed files are pure Rust inapp/src/terminal/model/, do not introduce new imports, and reuse only APIs already exercised by the existing CLI-agent trim path and existing trim tests. Please run CI to confirm.set_trim_trailing_blank_rows,on_finish_byte_processing,goto,input,linefeed,carriage_return).Manual repro (please verify on macOS):
PinnedToBottominput mode.aws configure.top,vim,less(alt-screen) — behavior unchanged. Run a CLI-agent session — behavior unchanged.Files
app/src/terminal/model/block.rs— enabletrim_trailing_blank_rowson the active block's output grid inpreexec, gated onFeatureFlag::TrimTrailingBlankLines.app/src/terminal/model/blockgrid_tests.rs— two new tests covering the regression and the without-fix baseline.Manual testing note
End-to-end manual verification on macOS requires the Xcode
metalshader toolchain, which is not available in my contribution environment (Command Line Tools only —crates/warpui/build.rspanics on missingmetal). I verified the change via:cargo check/cargo teston the affected crate(s) — all green (see PR body for specific counts).Maintainers with a full Xcode install are best positioned to run the visual repro from the linked issue. Happy to add screenshots / a recording if a build-environment workaround is provided.