fix(window-tabs-panes): preserve terminal scroll position across tab switches (#9171)#10735
fix(window-tabs-panes): preserve terminal scroll position across tab switches (#9171)#10735lonexreb wants to merge 1 commit into
Conversation
…switches (warpdotdev#9171) When a user scrolled up in a tab and switched away — especially with long output from `cat`, AI CLIs (Gemini, Claude), or any streaming command — returning to the tab would yank scroll position to the top of the buffer (or otherwise away from the last-viewed position). Inactive tabs are not in the layout tree but their terminal views remain live and can have `scroll_position` mutated by streaming output and rich-content insertion while hidden, so the user's saved scroll value drifts away. The fix introduces an explicit save/restore protocol on `ScrollState`: * `ScrollState` now carries an opaque `saved_for_tab_deactivation: Option<ScrollPosition>` slot. * `save_for_tab_deactivation` snapshots `position` (idempotent so multiple deactivation signals — window blur, tab switch — capture the FIRST pre-drift value). * `restore_after_tab_activation` writes the snapshot back to `position` and clears the slot. * `TerminalView` exposes thin wrappers that the workspace calls. * `Workspace::set_active_tab_index` snapshots every terminal view in the outgoing tab's pane group before flipping the index and restores every terminal view in the incoming tab's pane group after notifying focus change. The contract is: on tab activation, you return to exactly the position recorded at the last deactivation; rendering's existing `scroll_top_in_lines` clamps handle content-shrink edge cases without extra logic here. Tests (`scroll_state_tab_switch_tests` in `block_list_viewport.rs`) cover the save/restore round-trip, no-op semantics on first activation, save idempotency within a deactivation, snapshot-clearing on restore, and the case where no drift occurred between save and restore.
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 snapshots terminal scroll positions when switching away from a tab and restores them when returning, with unit coverage for the pure ScrollState save/restore behavior.
Concerns
⚠️ Manual testing is required for changes that can be manually tested. This change directly affects user-visible terminal tab-switch behavior, but the PR marks the manual repro as unchecked and does not include screenshots or a screen recording showing the fix end to end. Please include visual evidence or justify why manual testing is not possible.
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
|
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 preserves terminal scroll position across workspace tab switches by snapshotting each terminal view's ScrollState before a tab is deactivated and restoring that snapshot when the tab becomes active again. The added unit tests cover the core save/restore semantics, including idempotent saves and clearing snapshots between cycles.
Concerns
- No blocking correctness concerns found in the reviewed diff.
- No security findings.
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
captainsafia
and removed request for
a team
|
Closing this PR out as it lacks visual verifications of the fix. |
2 participants
Closes #9171
Summary
Fixes #9171.
When a user scrolls up to read long output (
caton a large log, an AI CLI session, etc.) and switches to another tab, returning yanks the scroll to the top of the buffer — disrupting the read in progress. Inactive tabs aren't in the layout tree, but theirTerminalViewis still live andscroll_positionkeeps getting mutated by streaming output and rich-content insertions, so by the time the tab is reactivated, the user's last-viewed position has drifted away.What changed
ScrollState(inapp/src/terminal/block_list_viewport.rs) now carriessaved_for_tab_deactivation: Option<ScrollPosition>with two new methods:save_for_tab_deactivation()— snapshotspositioninto the slot; idempotent so the first deactivation signal wins (matters because window-blur + tab-switch can fire in sequence and we want the pre-drift value).restore_after_tab_activation()— writes the snapshot back topositionand clears the slot.TerminalViewexposessave_scroll_position_for_tab_deactivationandrestore_scroll_position_after_tab_activationwrappers.Workspace::set_active_tab_indexsnapshots every terminal view in the outgoing tab's pane group before flipping the index, then restores every terminal view in the incoming tab's pane group after notifying focus change.The contract is exactly what the issue asks for: on tab activation, the user returns to the scroll position recorded at the moment of last deactivation. Render-time clamping (
ViewportState::scroll_top_in_lines's existing.min(max_scroll_top)) handles the content-shrink edge case for free.The fix is deliberately surgical — we save/restore the opaque
ScrollPositionvalue without redesigning anything around block-list virtualization, scroll anchoring, or input-mode (PinnedToBottom/PinnedToTop/Waterfall) semantics.Test plan
scroll_state_tab_switch_tests(block_list_viewport.rs) — all 5 pass:save_then_restore_returns_to_recorded_position— round-trip with simulated mid-deactivation drift.restore_without_prior_save_is_a_noop— first activation / never-deactivated case.save_is_idempotent_within_one_deactivation— multiple save signals don't overwrite the pre-drift snapshot.restore_clears_snapshot_so_next_cycle_is_fresh— back-to-back deactivate/activate cycles work.restore_is_a_noop_when_position_did_not_drift— clean exit when nothing moved.cargo check -p warp --lib— clean.cargo clippy -p warp --lib --no-deps— clean.cargo fmt -p warp -- --check— clean.cargo test -p warp --lib scroll— 65 scroll-related tests pass (no regressions).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.