From d28bdecd7d0d7d663dbc00e8b6c3a43d5c110af8 Mon Sep 17 00:00:00 2001 From: yoziv <59967231+yoziv@users.noreply.github.com> Date: Wed, 10 Jun 2026 09:48:06 +0300 Subject: [PATCH] fix: gate scroll-sync workarounds to normal buffer so alt-screen TUIs 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> --- src/renderer/components/TerminalPanel.tsx | 31 ++++ .../e2e/task-altscreen-scroll-gating.spec.ts | 134 ++++++++++++++++++ 2 files changed, 165 insertions(+) create mode 100644 tests/e2e/task-altscreen-scroll-gating.spec.ts diff --git a/src/renderer/components/TerminalPanel.tsx b/src/renderer/components/TerminalPanel.tsx index a734954..7037fce 100644 --- a/src/renderer/components/TerminalPanel.tsx +++ b/src/renderer/components/TerminalPanel.tsx @@ -145,6 +145,20 @@ function syncViewportScrollArea(term: Terminal): void { } catch { /* viewport may not be ready */ } } +/** + * True when the terminal is showing its normal (scrollback) buffer. + * + * Alt-screen TUIs (vim, less, htop, and Copilot CLI's full-screen UI with + * its own scrollbar) render into the alternate buffer, which has no + * scrollback and is fixed to the viewport size. Those apps own their own + * scrolling, so tmax's viewport scroll-sync workarounds below must stay out + * of their way — otherwise they fight the app's scrollbar (e.g. a stray + * scrollToBottom) instead of helping. + */ +function isNormalBuffer(term: Terminal): boolean { + return term.buffer.active.type === 'normal'; +} + const WSL_PROMPT_DEBOUNCE_MS = 200; const WSL_PROMPT_FALLBACK_MS = 5000; @@ -1258,6 +1272,9 @@ const TerminalPanel: React.FC = ({ terminalId, floatTitleBar const computeScrolledAway = () => { try { const buf = term.buffer.active; + // Alt-screen has no scrollback — you can't be "scrolled away" from a + // live prompt that doesn't exist, so hide the jump-to-bottom arrow. + if (buf.type !== 'normal') return false; if (buf.viewportY < buf.baseY) return true; const vp = containerRef.current?.querySelector('.xterm-viewport') as HTMLElement | null; if (vp && vp.scrollHeight - vp.clientHeight - vp.scrollTop > 2) return true; @@ -1289,6 +1306,9 @@ const TerminalPanel: React.FC = ({ terminalId, floatTitleBar try { const vp = viewportScrollEl; if (!vp) return; + // Alt-screen apps own the viewport and have no scrollback; mapping + // DOM scrollTop back to a buffer line would fight the app's own UI. + if (!isNormalBuffer(term)) return; const cellHeight = (term as unknown as { _core?: { _renderService?: { dimensions?: { css?: { cell?: { height?: number } } } } } }) ._core?._renderService?.dimensions?.css?.cell?.height || 0; @@ -1433,6 +1453,9 @@ const TerminalPanel: React.FC = ({ terminalId, floatTitleBar // means max-scroll always lands at the live prompt. const wheelPreSyncHandler = (e: WheelEvent) => { if (e.deltaY === 0 || e.shiftKey) return; + // Alt-screen apps own their scroll; resyncing the (nonexistent) + // scrollback area just fights the app's own scrollbar. + if (!isNormalBuffer(term)) return; try { const v: any = (term as any)?._core?.viewport; if (!v) return; @@ -1467,6 +1490,9 @@ const TerminalPanel: React.FC = ({ terminalId, floatTitleBar // but didn't — otherwise we'd thrash sync calls at scroll boundaries // and on shift/horizontal wheels. if (e.deltaY === 0 || e.shiftKey) return; + // In alt-screen there's no scrollback to recover into; a wheel that + // "does nothing" is the expected behavior, so don't resync. + if (!isNormalBuffer(term)) return; const viewport = containerRef.current?.querySelector('.xterm-viewport') as HTMLElement | null; if (!viewport) return; const before = viewport.scrollTop; @@ -1484,6 +1510,8 @@ const TerminalPanel: React.FC = ({ terminalId, floatTitleBar // would be) forces a sync. Useful when the auto-recovery hasn't yet // kicked in - the user can manually refresh the scroll area. const manualSyncHandler = (e: MouseEvent) => { + // No scrollback area to refresh while an alt-screen app is up. + if (!isNormalBuffer(term)) return; const rect = containerRef.current?.getBoundingClientRect(); if (!rect) return; // Only fire if the dblclick was within ~16px of the right edge. @@ -1500,6 +1528,9 @@ const TerminalPanel: React.FC = ({ terminalId, floatTitleBar // so it doesn't interfere with mid-scrolling. const wheelClampHandler = (e: WheelEvent) => { if (e.deltaY <= 0 || e.shiftKey) return; + // Never snap an alt-screen app to "bottom" — it has no scrollback and + // scrollToBottom() would yank the full-screen TUI's own view. + if (!isNormalBuffer(term)) return; const viewport = containerRef.current?.querySelector('.xterm-viewport') as HTMLElement | null; if (!viewport) return; requestAnimationFrame(() => { diff --git a/tests/e2e/task-altscreen-scroll-gating.spec.ts b/tests/e2e/task-altscreen-scroll-gating.spec.ts new file mode 100644 index 0000000..883733b --- /dev/null +++ b/tests/e2e/task-altscreen-scroll-gating.spec.ts @@ -0,0 +1,134 @@ +// Alt-screen scroll-gating: tmax's xterm 5.5 viewport scroll-sync +// workarounds (wheelPreSyncHandler, wheelRecoveryHandler, wheelClampHandler, +// manualSyncHandler, syncBufferToScrollbar, computeScrolledAway) are written +// for the NORMAL scrollback buffer. Full-screen TUIs (vim, less, htop, and +// Copilot CLI's full-screen scrollbar) run in the ALTERNATE buffer, which has +// no scrollback and where the app owns its own scrolling. The workarounds are +// now gated on `term.buffer.active.type === 'normal'` so they stay out of the +// app's way instead of fighting it. +// +// Test 1 proves wheelPreSyncHandler does NOT mutate the viewport cache while +// in alt-screen (on the un-gated code it would force-resync and overwrite the +// staged cache value). Test 2 proves the floating jump-to-bottom arrow hides +// in alt-screen and reappears after the app exits alt-screen. +import { test, expect, Page } from '@playwright/test'; +import { launchTmax, getStoreState } from './fixtures/launch'; + +async function writeTerm(window: Page, id: string, data: string): Promise { + await window.evaluate((args: { id: string; data: string }) => { + const entry = (window as any).__getTerminalEntry(args.id); + return new Promise((resolve) => entry.terminal.write(args.data, () => resolve())); + }, { id, data }); +} + +test('wheel in alt-screen does NOT force-sync the viewport cache', async () => { + const { window, close } = await launchTmax(); + try { + await window.waitForSelector('.terminal-panel', { timeout: 15_000 }); + await window.waitForTimeout(2500); + + const state = await getStoreState(window); + const terminalId = state.terminalIds[0]; + + // Seed normal-buffer scrollback so the viewport has real geometry, then + // scroll up off the bottom. + const filler: string[] = []; + for (let i = 0; i < 200; i++) filler.push(`base-line-${i.toString().padStart(4, '0')}`); + await writeTerm(window, terminalId, '\r\n' + filler.join('\r\n') + '\r\n'); + await window.waitForTimeout(300); + await window.evaluate((id) => { + (window as any).__getTerminalEntry(id).terminal.scrollLines(-50); + }, terminalId); + await window.waitForTimeout(150); + + // Enter alt-screen + enable mouse tracking (what a full-screen TUI does). + await writeTerm(window, terminalId, '\x1b[?1049h\x1b[?1000h\x1b[?1006h'); + await window.waitForTimeout(150); + + const result = await window.evaluate((id) => { + const entry = (window as any).__getTerminalEntry(id); + const term = entry.terminal; + const v: any = (term as any)._core.viewport; + const vp = (entry.container || document).querySelector('.xterm-viewport') as HTMLElement; + + // Sanity: we are in the alternate buffer. + const bufType = term.buffer.active.type; + + // Stage a cache lag the un-gated wheelPreSyncHandler would "fix": + // bufLen > _lastRecordedBufferLength triggers a force-resync that + // overwrites this sentinel. The gate must make it a no-op. + const altLen = term.buffer.active.length; + const sentinel = Math.max(0, altLen - 20); + v._lastRecordedBufferLength = sentinel; + + const beforeViewportY = term.buffer.active.viewportY; + + const ev = new WheelEvent('wheel', { + deltaY: 100000, + deltaMode: WheelEvent.DOM_DELTA_PIXEL, + bubbles: true, + cancelable: true, + }); + vp.dispatchEvent(ev); + + return { bufType, sentinel, afterRecorded: v._lastRecordedBufferLength, beforeViewportY }; + }, terminalId); + + // Let any rAF the un-gated path would schedule settle. + await window.waitForTimeout(200); + + const after = await window.evaluate((id) => { + const term = (window as any).__getTerminalEntry(id).terminal; + const v: any = (term as any)._core.viewport; + return { recorded: v._lastRecordedBufferLength, viewportY: term.buffer.active.viewportY }; + }, terminalId); + + // We must actually be in alt-screen for the test to be meaningful. + expect(result.bufType).toBe('alternate'); + // The gated wheelPreSyncHandler bailed: the staged cache value is intact. + // On the un-gated code this would be -1 or the recomputed buffer length. + expect(result.afterRecorded).toBe(result.sentinel); + expect(after.recorded).toBe(result.sentinel); + // wheelClampHandler did not yank the alt-screen view. + expect(after.viewportY).toBe(result.beforeViewportY); + } finally { + await close(); + } +}); + +test('jump-to-bottom arrow hides in alt-screen and reappears on exit', async () => { + const { window, close } = await launchTmax(); + try { + await window.waitForSelector('.terminal-panel', { timeout: 15_000 }); + await window.waitForTimeout(2500); + + const state = await getStoreState(window); + const terminalId = state.terminalIds[0]; + + const filler: string[] = []; + for (let i = 0; i < 200; i++) filler.push(`away-line-${i.toString().padStart(4, '0')}`); + await writeTerm(window, terminalId, '\r\n' + filler.join('\r\n') + '\r\n'); + await window.waitForTimeout(300); + + // Scroll up so the normal buffer is "scrolled away" → arrow appears. + await window.evaluate((id) => { + (window as any).__getTerminalEntry(id).terminal.scrollLines(-50); + }, terminalId); + // > 750ms scroll-away poll so React state settles regardless of events. + await window.waitForTimeout(900); + expect(await window.locator('.terminal-jump-to-bottom').count()).toBe(1); + + // Enter alt-screen: the arrow must hide (no scrollback to be away from). + await writeTerm(window, terminalId, '\x1b[?1049h'); + await window.waitForTimeout(900); + expect(await window.locator('.terminal-jump-to-bottom').count()).toBe(0); + + // Exit alt-screen: normal-buffer scroll position is restored, so the + // arrow reappears within one poll cycle. + await writeTerm(window, terminalId, '\x1b[?1049l'); + await window.waitForTimeout(900); + expect(await window.locator('.terminal-jump-to-bottom').count()).toBe(1); + } finally { + await close(); + } +});