From 5e1bdbf28d527730e6f4bc5bae8f5b689a0cebb2 Mon Sep 17 00:00:00 2001 From: Phil Condreay <0astex@gmail.com> Date: Wed, 22 Apr 2026 14:19:46 -0400 Subject: [PATCH] fix(terminal): prevent scroll-to-top race during fast output (#11102) Co-authored-by: Claude Opus 4.6 (1M context) Co-authored-by: Eugene --- tabby-terminal/src/frontends/xtermFrontend.ts | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/tabby-terminal/src/frontends/xtermFrontend.ts b/tabby-terminal/src/frontends/xtermFrontend.ts index a96d4683..15073382 100644 --- a/tabby-terminal/src/frontends/xtermFrontend.ts +++ b/tabby-terminal/src/frontends/xtermFrontend.ts @@ -82,6 +82,7 @@ export class XTermFrontend extends Frontend { private opened = false private resizeObserver?: any private flowControl: FlowControl + private pinnedToBottom = true private configService: ConfigService private hotkeysService: HotkeysService @@ -215,11 +216,31 @@ export class XTermFrontend extends Frontend { this.xtermCore._scrollToBottom = this.xtermCore.scrollToBottom.bind(this.xtermCore) this.xtermCore.scrollToBottom = () => null + // NOTE: xterm.onScroll only fires for content-driven scroll (new lines), + // NOT for user wheel/keyboard scroll (xterm.js #3864, #3201). During + // fast output, viewportY transiently equals baseY during xterm's + // internal processing, so onScroll would falsely re-pin. We do NOT + // use onScroll for pin state. Re-pinning happens only via: + // - wheel/keyboard event listeners (below) + // - explicit scrollToBottom() calls + this.resizeHandler = () => { try { if (this.xterm.element && getComputedStyle(this.xterm.element).getPropertyValue('height') !== 'auto') { + const savedPinned = this.pinnedToBottom + const savedViewportY = this.xterm.buffer.active.viewportY + this.fitAddon.fit() this.xterm.refresh(0, this.xterm.rows - 1) + + if (savedPinned) { + this.xtermCore._scrollToBottom() + } else { + // Restore the previous scroll position after fit + const maxScroll = this.xterm.buffer.active.baseY + const targetY = Math.min(savedViewportY, maxScroll) + this.xterm.scrollToLine(targetY) + } } } catch (e) { // tends to throw when element wasn't shown yet @@ -242,6 +263,15 @@ export class XTermFrontend extends Frontend { } + private isAtBottom (): boolean { + const buffer = this.xterm.buffer.active + return buffer.viewportY >= buffer.baseY - 1 + } + + private updatePinnedState (): void { + this.pinnedToBottom = this.isAtBottom() + } + async attach (host: HTMLElement, profile: BaseTerminalProfile): Promise { this.element = host @@ -291,6 +321,43 @@ export class XTermFrontend extends Frontend { // Allow an animation frame await new Promise(r => setTimeout(r, 0)) + // User-initiated scroll detection: only wheel and keyboard events + // should unpin. xterm.onScroll is content-driven only and must never + // unpin (see constructor comment). Use capture phase — xterm.js + // handles wheel/key events on its internal viewport element and may + // stop propagation, so bubbling listeners on host would never fire. + host.addEventListener('wheel', (event: WheelEvent) => { + // Immediately unpin on scroll-up so that writes arriving before + // the next animation frame don't yank the viewport back down. + if (event.deltaY < 0) { + this.pinnedToBottom = false + } + requestAnimationFrame(() => this.updatePinnedState()) + }, { capture: true, passive: true }) + + + this.hotkeysService.hotkey$ + .pipe( + takeUntil(this.destroyed$), + filter(hk => [ + 'scroll-up', + 'scroll-down', + 'scroll-page-up', + 'scroll-page-down', + 'scroll-to-top', + 'scroll-to-bottom', + ].includes(hk)), + ).subscribe(hk => { + if ([ + 'scroll-up', + 'scroll-page-up', + 'scroll-to-top', + ].includes(hk)) { + this.pinnedToBottom = false + } + requestAnimationFrame(() => this.updatePinnedState()) + }) + host.addEventListener('dragOver', (event: any) => this.dragOver.next(event)) host.addEventListener('drop', event => this.drop.next(event)) @@ -353,7 +420,24 @@ export class XTermFrontend extends Frontend { } async write (data: string): Promise { + // Capture pinned state before the write — the async write yields + // to the event loop, and RAF callbacks (e.g. from wheel events) + // could change pinnedToBottom mid-write. + const wasPinned = this.pinnedToBottom + const savedViewportY = this.xterm.buffer.active.viewportY await this.flowControl.write(data) + if (wasPinned) { + this.xtermCore._scrollToBottom() + } else { + // Restore scroll position — xterm internally disturbs viewportY + // during fast output, and the patched-out scrollToBottom no-op + // prevents xterm from correcting it. + const maxScroll = this.xterm.buffer.active.baseY + const targetY = Math.min(savedViewportY, maxScroll) + if (this.xterm.buffer.active.viewportY !== targetY) { + this.xterm.scrollToLine(targetY) + } + } } clear (): void { @@ -370,18 +454,22 @@ export class XTermFrontend extends Frontend { } scrollToTop (): void { + this.pinnedToBottom = false this.xterm.scrollToTop() } scrollPages (pages: number): void { this.xterm.scrollPages(pages) + this.updatePinnedState() } scrollLines (amount: number): void { this.xterm.scrollLines(amount) + this.updatePinnedState() } scrollToBottom (): void { + this.pinnedToBottom = true this.xtermCore._scrollToBottom() }