spec: horizontal scrolling per terminal pane (#9828)#10231
spec: horizontal scrolling per terminal pane (#9828)#10231lonexreb wants to merge 6 commits intowarpdotdev:masterfrom
Conversation
github-actions
Bot
added
the
external-contributor
![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 spec defines per-pane horizontal scrolling for terminal panes when line-wrap is disabled, with trackpad, Shift-wheel, and scrollbar controls.
Concerns
- The behavior contract does not define how horizontal offsets are clamped or reset when the visible long-line set changes, the pane resizes, line-wrap is toggled, or vertical scroll moves to shorter content.
- The snap-on-new-output behavior is ambiguous for terminal output patterns like partial line updates, prompt redraws, tailing logs, and alternate-screen/TUI updates.
- The new snap setting needs a small implementation/discovery note so implementers know where it is exposed and persisted.
Found: 0 critical, 2 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
| - B1. Horizontal scrolling is gated on: | ||
| - The pane's `terminal.line_wrap = false` setting (existing). | ||
| - At least one visible row exceeding pane width. | ||
| When either condition is false, the pane behaves as today (no | ||
| horizontal scroll, no scrollbar). |
There was a problem hiding this comment.
h_offset is clamped or reset when the visible long-line set changes, the pane resizes, line-wrap toggles, or vertical scrolling moves to shorter content; otherwise implementations can show blank shifted columns or stale scrollbars.
| - B6. Auto-scroll on new output: by default the pane snaps back | ||
| to horizontal offset 0 when new output arrives at the bottom | ||
| (new prompt, new line). User-controlled snap behavior can be | ||
| disabled via `terminal.h_scroll_snap_on_new_output` (default | ||
| `true`). |
There was a problem hiding this comment.
new output arrives at the bottom is too broad to implement consistently.
| - B6. Auto-scroll on new output: by default the pane snaps back | ||
| to horizontal offset 0 when new output arrives at the bottom | ||
| (new prompt, new line). User-controlled snap behavior can be | ||
| disabled via `terminal.h_scroll_snap_on_new_output` (default |
There was a problem hiding this comment.
💡 [SUGGESTION] Add where terminal.h_scroll_snap_on_new_output is exposed and persisted, plus a test or acceptance criterion for disabling it.
|
@lonexreb thanks a ton for your contributions! I work on the Talent team at Warp (https://www.linkedin.com/in/mac-bennett-7936a6a6/) and I'm wondering if you'd be interested in exploring a role with us?!? If so, I'd love to jump on a call with you. Shot you a Linkedin connection request with my availability link :-). |
Co-Authored-By: Warp <agent@warp.dev>
|
/oz-review |
|
I'm re-reviewing this spec-only 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 spec defines per-pane horizontal scrolling for terminal output when line-wrap is disabled, including input methods, offset clamping, snap-on-new-output behavior, settings persistence, acceptance criteria, and tests.
Concerns
- The offset model does not define how terminal grid columns map across wide glyphs, combining marks, tabs, and other multi-cell content, which can make rendering, hit-testing, and selection inconsistent.
- Trackpad support is specified only for native macOS/Windows, leaving Linux behavior ambiguous even though the rest of the feature is platform-agnostic and Warp has Linux windowing support.
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
| - At least one visible row exceeding pane width. | ||
| When either condition is false, the pane behaves as today (no | ||
| horizontal scroll, no scrollbar). | ||
| - B1a. `h_offset` is a logical column offset clamped to |
There was a problem hiding this comment.
h_offset is measured in terminal grid cells and how it behaves for wide glyphs, combining marks, tabs, and other multi-cell content; otherwise render, hit-testing, and selection paths can map the same visible column differently.
| shrinks but remains positive, clamp `h_offset` down to the new | ||
| maximum so blank shifted columns are never rendered. | ||
| - B2. Horizontal scroll inputs: | ||
| - Trackpad two-finger horizontal swipe (native macOS/Windows). |
There was a problem hiding this comment.
Round-1 fixes for oz-for-oss bot review: - Add Offset Clamping & Reset Rules covering pane resize, vertical scroll, line-wrap toggle, and long-line set changes - Define Snap-on-New-Output precisely: excludes alt-screen, prompt redraws, partial-line tailing; only true line-append-below triggers - Rename setting to terminal.horizontal_scroll.snap_on_new_output and add Settings UI surface plus per-pane override - Define offset model in terminal grid columns with explicit handling of wide glyphs, combining marks, and tab stops - Add Linux trackpad support via libinput two-finger horizontal swipe with Shift+wheel and scrollbar drag fallback
|
Round-1 fixes for oz-for-oss review: (1) added Offset Clamping & Reset Rules covering pane resize, vertical scroll, line-wrap toggle, and long-line set changes; (2) precise Snap-on-New-Output definition excluding alt-screen, prompt redraws, and partial-line tailing — only true line-append-below triggers snap; (3) added implementation/discovery note: setting |
|
/oz-review |
|
I'm re-reviewing this spec-only 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 spec defines per-pane horizontal scrolling for terminal panes when line wrap is disabled, including input methods, snap behavior, settings, column semantics, acceptance criteria, and tests.
Concerns
- The line-wrap toggle behavior is internally contradictory: one section resets
h_offsetto 0 when line wrap is enabled, while a later section preserves it and restores it when line wrap is disabled again.
Verdict
Found: 1 critical, 0 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
| `max(visible_longest_line_columns - visible_pane_columns, 0)`. | ||
| Recompute that maximum whenever the visible buffer range changes, | ||
| the pane resizes, line-wrap toggles, or rendered line contents | ||
| change. If the maximum becomes 0 or line-wrap is enabled, reset |
There was a problem hiding this comment.
🚨 [CRITICAL] This conflicts with the later line-wrap rule that preserves h_offset internally and restores it when line-wrap is disabled again. Choose one behavior so implementation and tests know whether toggling wrap resets or preserves the horizontal position.
|
Round-2 fix: CRITICAL resolved — chose PRESERVE-AND-RESTORE for line-wrap toggle. |
|
/oz-review |
|
I'm re-reviewing this spec-only 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
Reviewed the spec for per-pane horizontal scrolling when line-wrap is disabled. The behavior contract is detailed, and the security pass did not identify design-level security issues.
Concerns
- The spec is added as
specs/GH9828/SPEC.md, but the repository's GitHub-issue spec convention usesspecs/GH*/product.mdandspecs/GH*/tech.md; this can make downstream spec discovery and implementation workflows miss the approved spec. - The reset/clamping rules are internally inconsistent: one section resets/clamps
h_offsetto 0 when there is no valid horizontal offset, while the later "reset to 0 ONLY" invariant omits that case. - One wrap-toggle test assertion uses
>=where equality is needed to verify exact restore-or-clamp behavior.
Verdict
Found: 0 critical, 2 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
| @@ -0,0 +1,286 @@ | |||
| # Spec: Horizontal scrolling per terminal pane (GH-9828) | |||
There was a problem hiding this comment.
specs/GH<issue>/product.md and specs/GH<issue>/tech.md; adding only SPEC.md can make spec-context and implementation workflows miss this spec. Split or rename the file to the expected product/tech spec paths, or document why this PR is an accepted exception.
| preserved offset's required range, the offset clamps down (it | ||
| is never silently snapped to 0 unless the clamp resolves to | ||
| 0 because nothing exceeds the viewport). | ||
| - **Reset to 0 ONLY on explicit triggers.** `h_offset` is reset |
There was a problem hiding this comment.
reset to 0 ONLY invariant conflicts with B1a and resize/visible-range clamping, which reset or clamp h_offset to 0 when no visible row exceeds the viewport. Add that no-valid-offset case here, or define it as clamping rather than a reset, so implementers do not preserve stale offsets indefinitely.
| Assertion: post-toggle `h_offset >= min(120, max_valid)` and | ||
| is NEVER 0 unless `max_valid == 0`. |
There was a problem hiding this comment.
💡 [SUGGESTION] This assertion should require the exact restored/clamped offset; >= would allow values beyond max_valid and miss clamping bugs.
| Assertion: post-toggle `h_offset >= min(120, max_valid)` and | |
| is NEVER 0 unless `max_valid == 0`. | |
| Assertion: post-toggle `h_offset == min(120, max_valid)` and | |
| is NEVER 0 unless `max_valid == 0`. |
|
Round-3 review fixes (commit c0ef2da):
|
|
/oz-review |
|
Round-4 review addressed: (1) split SPEC.md into product.md (Problem/Goal/AC/Out-of-scope) and tech.md (Behavior contract/Implementation/Tests) per repo convention (matches GH1066, GH849, GH703, GH478). (2) Tightened B1a so the max-cleared boundary is explicitly the GENERAL clamp at its boundary (not a separate rule); 'Reset to 0 ONLY' list is canonical and exhaustive — neither wrap-toggle nor max-cleared appear there. (3) T_line_wrap_toggle_round_trip and T_line_wrap_toggle_clamps_on_shrink now assert with == in every case (no >=). |
|
/oz-review |
2 participants
Spec for #9828. Per-pane horizontal scroll when line-wrap is off and content exceeds pane width. Trackpad swipe, Shift-wheel, and a bottom scrollbar all drive it. Snaps to offset 0 on new output by default. Selection copies logical text.
Closes (spec-only) #9828.