Enhance alt-screen scrollback handling and rendering logic#9377
Enhance alt-screen scrollback handling and rendering logic#9377JivinSardine wants to merge 8 commits intowarpdotdev:masterfrom
Conversation
Account for alt-screen scrollback when mapping coords and rendering. coord_to_point now computes a history_offset (total_rows - visible_rows) and anchors visible rows to the bottom of the buffer, and painting uses history_offset when computing start_row/end_row so the visible content aligns with preserved history. Also set AltScreen's default max_scroll_limit from 0 to 1000 to retain alt-screen scrollback and avoid content loss during heavy streaming (e.g. apps relying on cursor-up overshoot / full-buffer redraws like Claude Code). Includes comments and bounds checks to clamp row calculations to visible/total row limits.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @JivinSardine on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
I'm starting a first review of this pull request. I requested changes on this pull request and posted feedback. Comment I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I requested changes on this pull request and posted feedback. 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 attempts to preserve alt-screen history and render from the bottom of the buffer so cursor-up redraw patterns do not lose content.
Concerns
- The new nonzero alt-screen scroll limit stores rows in
GridStorage, but theGridHandlerAPIs used by rendering and hit-testing still derive history fromflat_storage, which remains empty for alt-screen grids. This means the newhistory_offsetremains zero androw(0..visible)can read hidden grid-storage history rather than the current visible screen after scrolling. - If
history_offsetis made nonzero, the paint origin must also be offset by that history amount becauserender_gridpositions cells with absoluteoffset_rowvalues.
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
| let alt_screen = AltScreen::new( | ||
| sizes.size, | ||
| 0, /* max_scroll_limit */ | ||
| 1000, /* max_scroll_limit — preserve alt-screen scrollback to prevent content loss |
There was a problem hiding this comment.
GridStorage history, but GridHandler still reports history via flat_storage (which alt-screen keeps empty), so history_offset stays 0 and row(0..visible) reads hidden GridStorage history instead of the visible rows once scrolling occurs. Expose GridStorage history through GridHandler for alt-screen, or render/index via GridStorage absolute rows before enabling this.
| .as_f64()) | ||
| .min(grid.visible_rows() as f64); | ||
| .min(grid.total_rows() as f64); | ||
| let adjusted_grid_origin = origin - self.vertical_scroll_pixels(); |
There was a problem hiding this comment.
history_offset becomes non-zero, this origin only subtracts self.scroll_top while render_grid places cells at offset_row = start_row + offset, so the first visible row renders history_offset rows below the element. Subtract the full start_row from the origin.
| let adjusted_grid_origin = origin - self.vertical_scroll_pixels(); | |
| let adjusted_grid_origin = | |
| origin - vec2f(0., start_row as f32 * self.line_height().as_f32()); |
|
@cla-bot check |
There was a problem hiding this comment.
Pull request overview
Updates Warp’s alt-screen handling to retain a bounded scrollback history and adjusts rendering / hit-testing so the visible alt-screen content is anchored to the bottom of the (history + visible) buffer, preventing mis-rendering during heavy streaming output (e.g., cursor-up overshoot redraw patterns).
Changes:
- Set alt-screen
max_scroll_limitto 1000 (was 0) to preserve a bounded amount of alt-screen scrollback. - Render alt-screen starting from
history_offset(history size) and adjustend_rowbounds to usetotal_rows. - Update pixel→grid coordinate mapping to account for the history offset.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/src/terminal/model/terminal_model.rs | Enables bounded alt-screen scrollback by initializing AltScreen with a non-zero scroll limit. |
| app/src/terminal/alt_screen/alt_screen_element.rs | Anchors rendering to history offset and updates coordinate mapping for correct row targeting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let row = history_offset | ||
| + (coord.y() / size.cell_height_px().as_f32()) | ||
| .max(0.) | ||
| .min(visible_height.saturating_sub(1) as f32) as usize; |
There was a problem hiding this comment.
coord_to_point clamps the computed row using visible_height, but coord has already been adjusted by vertical_scroll_pixels (see to_local), meaning coord.y() includes scroll_top. When scroll_top > 0 (e.g. shared-session viewer vertical scrolling), this clamp will cap rows to the first visible_height lines and map mouse hit-testing/selection to the wrong grid rows. Clamp against the grid’s total row range (or against scroll_top + visible_height) so the computed row stays consistent with the start_row = history_offset + scroll_top rendering logic.
| let row = history_offset | |
| + (coord.y() / size.cell_height_px().as_f32()) | |
| .max(0.) | |
| .min(visible_height.saturating_sub(1) as f32) as usize; | |
| let max_visible_row = self | |
| .scroll_top | |
| .saturating_add(visible_height.saturating_sub(1)); | |
| let row = history_offset | |
| + (coord.y() / size.cell_height_px().as_f32()) | |
| .max(0.) | |
| .min(max_visible_row as f32) as usize; |
| // Render grid cells. The alt screen preserves scrollback for apps that | ||
| // rely on cursor-up overshoot and full-buffer redraws (e.g. Claude Code). | ||
| // Start rendering from the history offset so visible content anchors to | ||
| // the bottom of the buffer rather than the top. | ||
| record_trace_event!("alt_screen_element:paint:preparing_to_render_grid"); | ||
| let start_row = self.scroll_top.as_f64(); | ||
| let history_offset = (grid.total_rows().saturating_sub(grid.visible_rows())) as f64; | ||
| let start_row = history_offset + self.scroll_top.as_f64(); | ||
| let end_row = (start_row | ||
| + self | ||
| .visible_lines | ||
| .expect("should be set after layout") | ||
| .as_f64()) | ||
| .min(grid.visible_rows() as f64); | ||
| .min(grid.total_rows() as f64); |
There was a problem hiding this comment.
The new history_offset rendering path is a regression-prone behavior change (rendering from history_offset and changing end_row bounds). There are existing alt-screen interaction tests in app/src/terminal/view_test.rs, but none appear to cover an alt-screen with non-zero history + verifying paint/mouse mapping stays correct. Adding a regression test that scrolls the alt-screen enough to create history, then asserts selection/mouse coordinates target the expected rows, would help prevent future breakage.
|
The cla-bot has been summoned, and re-checked this pull request! |
GridHandler::history_size() derives from flat_storage (always empty for alt-screen), so history_offset was 0 despite GridStorage preserving scrollback rows internally via max_scroll_limit. This caused visible rows to render at wrong positions and mouse coordinates to map incorrectly. - Use GridStorage::total_rows/visible_rows directly for history_offset - Offset paint origin by history_offset so cells land at correct screen y - Fix coord_to_point clamp to account for scroll_top baked into coord.y - Bump grid_storage() visibility from pub(super) to pub(crate)
|
@oz-agent check code again |
|
I'm working on changes requested in this PR (responding to a PR conversation comment). I analyzed the request but did not produce any changes. You can view the conversation on Warp. Powered by Oz |
|
@oz-agent check the changes |
There was a problem hiding this comment.
Overview
This PR enables bounded alt-screen scrollback and updates rendering/mouse coordinate conversion to account for GridStorage history.
Concerns
- Rendering is not wired through a row source that includes
GridStoragehistory;render_gridstill asksGridHandler::rowforhistory_offset-adjusted rows, which are out of bounds according toGridHandler::total_rows(). - The coordinate change feeds history-adjusted rows into terminal mouse reporting, so mouse-enabled TUIs receive invalid row numbers after scrollback accumulates.
- Cursor rendering still uses
GridHandlercursor points without the storage-history offset, so the cursor/cursor cell state is no longer aligned with the rendered cells. - Security pass: no security-specific issues found.
Verdict
Found: 1 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
| // the bottom of the buffer rather than the top. | ||
| record_trace_event!("alt_screen_element:paint:preparing_to_render_grid"); | ||
| let start_row = self.scroll_top.as_f64(); | ||
| let start_row = history_offset as f64 + self.scroll_top.as_f64(); |
There was a problem hiding this comment.
🚨 [CRITICAL] render_grid still reads rows through GridHandler::row, whose total_rows() ignores GridStorage history for alt-screen, so adding history_offset here makes the renderer request out-of-bounds rows once scrollback exists; the bottom row is skipped immediately and the alt-screen renders blank once history_offset >= visible_rows.
| // coord.y already includes scroll_top via to_local(), so the clamp | ||
| // upper bound must account for it. | ||
| let max_row = (self.scroll_top.as_f64() as usize) + visible_height.saturating_sub(1); | ||
| let row = history_offset |
There was a problem hiding this comment.
AltMouseAction, and MouseState::to_escape_sequence serializes point.row + 1 to the TUI, so adding history_offset sends mouse rows larger than the alt-screen height after scrollback accumulates.
| // Offset the paint origin upward by the scrollback height so that | ||
| // render_grid's absolute offset_row positioning maps cells to the | ||
| // correct screen positions (the first visible row lands at origin.y). | ||
| let adjusted_grid_origin = |
There was a problem hiding this comment.
adjusted_grid_origin is used by render_cursor below, but grid.cursor_render_point() still returns a point based on GridHandler::history_size() (flat storage, 0 for alt-screen), so the cursor and cursor-cell styling are shifted above the viewport when history_offset > 0.
Co-Authored-By: Jivin Sardine M <89859763+JivinSardine@users.noreply.github.com>
captainsafia
added
the
external-contributor
Warp Dev Github Integration
|
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 |
There was a problem hiding this comment.
Overview
This PR preserves bounded alt-screen scrollback in GridStorage and offsets alt-screen rendering and mouse coordinate conversion so streaming TUIs that redraw with cursor-up overshoot do not lose rows.
Concerns
num_lines_truncated()now addsGridStoragetruncation for every grid. Normal grids intentionally keepGridStorageat a zero scroll limit while moving scrolled rows intoflat_storage, so this makes ordinary scrollback look truncated even when it was preserved.- This is a user-visible terminal rendering change, but the PR description does not include screenshots or video. For faster review, please upload screenshots or a video of the rendering behavior working end to end.
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
| /// grid's maximum scrollback limit. | ||
| pub fn num_lines_truncated(&self) -> u64 { | ||
| self.flat_storage.num_truncated_rows() | ||
| self.flat_storage.num_truncated_rows() + self.grid.num_lines_truncated |
There was a problem hiding this comment.
GridStorage's scroll limit to 0 while preserving scrolled rows in flat_storage, so self.grid.num_lines_truncated increments for every normal scroll; adding it here makes untruncated block rows look truncated. Only include grid truncation for alt-screen grids or storage modes where grid history owns scrollback.
…sible rows Now that GridHandler::history_size() includes GridStorage history, switch alt-screen rendering and hit-testing from GridStorage back to the canonical GridHandler API. Also convert absolute grid rows to visible screen coordinates for AltMouseAction escape sequences, preventing mouse-enabled TUIs from receiving history-offset row numbers that exceed the visible alt-screen height.
|
/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 preserves bounded GridStorage history for the alt screen and adjusts rendering/mouse-coordinate handling so the visible viewport is anchored below that preserved history.
Concerns
- The internal alt-screen history is now part of the shared GridHandler dimensions, so existing alt-screen find/output APIs can include hidden rows that paint deliberately excludes.
- This is a user-visible terminal rendering change, but the PR description does not include screenshots or a video. For faster review, please upload screenshots or a video of the feature working end to end.
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
| #[inline] | ||
| fn history_size(&self) -> usize { | ||
| self.flat_storage.total_rows() | ||
| self.flat_storage.total_rows() + self.grid.history_size() |
There was a problem hiding this comment.
|
/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 attempts to preserve bounded alt-screen scrollback in GridStorage and adjust rendering plus mouse coordinate mapping around the hidden history rows.
Concerns
- The new offset plumbing reads
GridHandler::history_size(), which still represents only flat-storage history in the displayed diff. Because alt-screen flat storage is now capped at 0, the offset remains 0, so rendering and hit detection do not account for the preservedGridStoragehistory, and the addedgrid_handler.history_size()test assertion should fail.
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
|
|
||
| /// Returns the grid's scrollback size (history rows above the visible viewport). | ||
| fn grid_history_offset(&self) -> usize { | ||
| self.model.lock().alt_screen().grid_handler().history_size() |
There was a problem hiding this comment.
GridHandler::history_size() still reports flat-storage rows, but alt-screen scrollback is now stored in GridStorage, so this returns 0 and the render/hit-test offsets never account for preserved history. Use grid_storage().history_size() here or update the GridHandler dimension implementation for alt-screen grids.
The previous commit inadvertently reverted the Oz bot's fix that added self.grid.history_size() to the GridHandler history calculation. Without this, GridHandler::total_rows() excludes GridStorage's internal scrollback, causing storage_row() bounds checks to reject valid history-offset rows and making history_offset always 0 for alt-screen rendering.
|
/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 |
|
/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 moves alt-screen scrollback into GridStorage and adjusts alt-screen rendering and mouse coordinate mapping around the preserved history offset.
Concerns
num_lines_truncated()now double-counts regular grid scrolls because non-alt grids intentionally keepGridStoragescrollback disabled while preserving scrolled rows inflat_storage.
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
Co-authored-by: oz-for-oss[bot] <277970191+oz-for-oss[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Overview
This PR preserves bounded alt-screen history in grid storage and updates rendering/mouse-coordinate paths to anchor the visible viewport below that history.
Concerns
- Mouse hit-testing now truncates fractional shared-session scroll offsets when computing the visible row clamp, which can map clicks on partially visible bottom rows to the wrong grid row.
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
| .as_f64() as usize; | ||
| // coord.y already includes scroll_top via to_local(), so the clamp | ||
| // upper bound must account for it. | ||
| let max_row = (self.scroll_top.as_f64() as usize) + visible_height.saturating_sub(1); |
There was a problem hiding this comment.
scroll_top can be fractional for precise shared-session scrolling, but casting it to usize truncates the visible upper bound; the bottom partially visible row can map to the row above. Compute the clamp using the fractional scroll offset, such as ceil(scroll_top + visible_height) - 1, before converting to an integer.
|
/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 preserves bounded alt-screen history in GridStorage and adjusts rendering/mouse coordinates so visible rows are anchored after the new history offset.
Concerns
- This is a user-visible terminal rendering change, but the PR description does not include screenshots or video evidence. For faster review, please upload screenshots or a video of the bug before/after or the new behavior working end to end.
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
Summary
Fixes a rendering bug where Claude Code's TUI prompt box (> line + status bar) appears mid-scrollback during heavy streaming output, causing visual content to be overlapped or clipped.
Root Cause
The alt-screen grid was initialized with max_scroll_limit = 0, meaning any content that scrolled off the visible alt-screen was permanently discarded. When streaming TUI apps like Claude Code use cursor-up overshoot sequences (Ink framework's full-buffer redraw pattern), the cursor references positions in content that Warp's alt-screen had already dropped. This caused the prompt to render at an incorrect position in the scrollback.
Changes
Verification
Affected issues
Fixes Flickering scroll when using Ink-based CLI (Claude Code, Gemini) #7200