Skip to content

fix: gate scroll-sync workarounds to normal buffer so alt-screen TUIs own scroll#132

Merged
InbarR merged 1 commit into
InbarR:mainfrom
yoziv:feature/yoziv/altscreen-scroll-gating
Jun 13, 2026
Merged

fix: gate scroll-sync workarounds to normal buffer so alt-screen TUIs own scroll#132
InbarR merged 1 commit into
InbarR:mainfrom
yoziv:feature/yoziv/altscreen-scroll-gating

Conversation

@yoziv

@yoziv yoziv commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Motivation: This started from the latest GitHub Copilot CLI (v1.0.61), which added its own full-screen scrollbar. Running it full-screen inside a tmax pane surfaced the scroll-stuck behavior below — tmax was fighting the CLI''s own scrollbar instead of letting it own scroll.

Related to #130 (scrolling occasionally disabled / scrollbar disappears mid-session). This PR addresses the alt-screen/full-screen-TUI manifestation; see Follow-ups for the normal-buffer cases that may also contribute to #130.

Problem

Scrolling sometimes "gets stuck" in tmax. The file TerminalPanel.tsx carries a battery of xterm 5.5 Viewport scroll-sync workarounds (wheelPreSyncHandler, wheelRecoveryHandler, wheelClampHandler, manualSyncHandler, syncBufferToScrollbar, and the computeScrolledAway jump-to-bottom arrow). These are all written for the normal scrollback buffer, but they ran unconditionally.

When a full-screen TUI runs in a pane — vim, less, htop, and notably GitHub Copilot CLI v1.0.61, which just added its own full-screen scrollbar — the terminal switches to the alternate buffer, which has no scrollback and where the app owns its own scrolling. The workarounds had nothing useful to do there and could interfere with the app''s own scrollbar.

Fix

  • Add isNormalBuffer(term) => term.buffer.active.type === 'normal'.
  • Early-return from each scrollback workaround when in alt-screen, so a full-screen TUI''s own scrollbar owns scroll.
  • Normal-buffer behavior is unchanged.

Wheel-to-PTY forwarding (attachCustomWheelEventHandler, gated on mouseTrackingMode !== 'none' && baseY === 0) is untouched, so the app''s scrollbar still receives wheel input. The change is public-API only (buffer.active.type) — it touches no xterm internals.

Tests

tests/e2e/task-altscreen-scroll-gating.spec.ts (2 tests, both green):

  1. Wheel in alt-screen no longer force-syncs the viewport cache (the staged cache value is preserved; on the un-gated code it would be overwritten).
  2. The jump-to-bottom arrow hides in alt-screen and reappears on exit.

tsc --noEmit: no new errors.

Review

Reviewed by a 10-agent model fleet (Opus 4.8 / 4.7 / 4.7-xhigh, Sonnet 4.6, GPT-5.5, MAI). Code-review verdicts: 5/5 approve.

Follow-ups (not in this PR)

  • xterm 5.5 -> 6.0 upgrade. 6.0 rewrote the Viewport on VSCode''s SmoothScrollableElement and removed the private fields this workaround pile depends on — upgrading would let most of these handlers be deleted outright. This is the durable fix for the bug class (and likely the remaining normal-buffer part of Tmax scrolling is occasionally disabled #130).
  • Wheel delivery for default/x10-encoded mouse apps. onBinary -> PTY forwarding (TASK-184) is currently reverted on main, so this gating is sufficient for SGR apps (Copilot CLI / Ink) but not for default-encoded mouse reports after a pane split. Tracked separately.

… own scroll

xterm 5.5 viewport scroll-sync workarounds (wheelPreSync/recovery/clamp,
manualSync, syncBufferToScrollbar, jump-to-bottom arrow) target the normal
scrollback buffer but ran unconditionally. Full-screen TUIs (vim, less, htop,
Copilot CLI's full-screen scrollbar) run in the alternate buffer with no
scrollback and own their scrolling, so the workarounds had nothing useful to
do there and could fight the app's own scrollbar.

Add isNormalBuffer(term) and early-return from each workaround in alt-screen.
Wheel->PTY forwarding (attachCustomWheelEventHandler) is untouched, so the
app's scrollbar still receives wheel input.

Adds e2e coverage for alt-screen gating.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@yoziv

yoziv commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

CI note: the red E2E check is pre-existing on main, not from this PR

The Playwright (Windows) check is red, but the failures are unrelated to this change and already fail on main:

  • This PR's own tests pass in CI — both task-altscreen-scroll-gating.spec.ts tests are green in the same run (ok 279 4.8s, ok 280 7.0s).
  • 📉 Identical baseline on main: this PR's run is 34 failed / 187 passed; the latest main E2E run (27089953995) is also 34 failed / 187 passed — the last 5 consecutive main runs are all red.
  • 🧩 The 34 failures are in unrelated areas — cwd resolution (Received: ""), rich-text paste, image-path-click (openPath), rename-watcher, workspaces. This +28-line change only adds buffer.active.type === 'normal' early-returns to alt-screen scroll handlers; it cannot affect those paths.

So this PR is green on its own merits and does not regress the suite. Happy to rebase once main's E2E is healthy if that helps confirm.

@InbarR InbarR merged commit 0e9747d into InbarR:main Jun 13, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants