From a31fa8f9843f3c5609eeab4bccb53f6c10a8397e Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Wed, 25 Feb 2026 04:30:11 -0800 Subject: [PATCH] [Bugfix #560] fix: Track scroll state externally to survive display:none toggling xterm's buffer.active.viewportY (backed by ydisp) becomes stale when the terminal container is hidden via display:none during tab switches or panel collapse. The browser resets the viewport scrollTop to 0, and xterm syncs ydisp to 0. When safeFit() later reads viewportY=0, it 'preserves' this incorrect position at the top of the buffer. Fix: Track scroll state (viewportY, baseY, wasAtBottom) externally in JS variables updated via term.onScroll(). These variables are immune to DOM state changes from display toggling. safeFit() now reads from the tracked state instead of xterm's buffer. Also rewrites Terminal.fit-scroll.test.tsx with 7 tests including 3 new ones specifically for the Issue #560 scenario, and adds the onScroll mock to all 9 other terminal test files. --- .../__tests__/Terminal.clipboard.test.tsx | 1 + .../__tests__/Terminal.controls.test.tsx | 1 + .../__tests__/Terminal.fit-scroll.test.tsx | 166 +++++++++++++++--- .../__tests__/Terminal.ime-dedup.test.tsx | 1 + .../__tests__/Terminal.input-filter.test.tsx | 1 + .../__tests__/Terminal.reconnect.test.tsx | 1 + .../__tests__/Terminal.replay-scroll.test.tsx | 1 + .../__tests__/Terminal.scroll.test.tsx | 1 + .../__tests__/VirtualKeyboard.test.tsx | 1 + .../__tests__/architect-toolbar.test.tsx | 1 + .../dashboard/src/components/Terminal.tsx | 43 ++++- 11 files changed, 188 insertions(+), 30 deletions(-) diff --git a/packages/codev/dashboard/__tests__/Terminal.clipboard.test.tsx b/packages/codev/dashboard/__tests__/Terminal.clipboard.test.tsx index 82236379..23ebc49a 100644 --- a/packages/codev/dashboard/__tests__/Terminal.clipboard.test.tsx +++ b/packages/codev/dashboard/__tests__/Terminal.clipboard.test.tsx @@ -25,6 +25,7 @@ vi.mock('@xterm/xterm', () => { dispose = vi.fn(); onData = vi.fn(); onResize = vi.fn(); + onScroll = vi.fn(() => ({ dispose: vi.fn() })); cols = 80; rows = 24; attachCustomKeyEventHandler = vi.fn((handler: (event: KeyboardEvent) => boolean) => { diff --git a/packages/codev/dashboard/__tests__/Terminal.controls.test.tsx b/packages/codev/dashboard/__tests__/Terminal.controls.test.tsx index 0edc64b6..61cc8fe7 100644 --- a/packages/codev/dashboard/__tests__/Terminal.controls.test.tsx +++ b/packages/codev/dashboard/__tests__/Terminal.controls.test.tsx @@ -30,6 +30,7 @@ vi.mock('@xterm/xterm', () => { dispose = vi.fn(); onData = vi.fn(); onResize = vi.fn(); + onScroll = vi.fn(() => ({ dispose: vi.fn() })); registerLinkProvider = vi.fn(() => ({ dispose: vi.fn() })); attachCustomKeyEventHandler = vi.fn(); scrollToBottom = vi.fn(); diff --git a/packages/codev/dashboard/__tests__/Terminal.fit-scroll.test.tsx b/packages/codev/dashboard/__tests__/Terminal.fit-scroll.test.tsx index 30e4d6b1..30f2e305 100644 --- a/packages/codev/dashboard/__tests__/Terminal.fit-scroll.test.tsx +++ b/packages/codev/dashboard/__tests__/Terminal.fit-scroll.test.tsx @@ -1,10 +1,12 @@ /** - * Regression test for GitHub Issue #423: Terminal scroll position lost during fit(). + * Regression tests for terminal scroll position preservation during fit(). * - * When fitAddon.fit() is called (via ResizeObserver, visibility change, or manual - * refresh), the terminal viewport could jump to the top of the scrollback buffer. - * The fix wraps fit() with scroll position preservation: save viewportY before, - * restore after (scrollToBottom if was at bottom, scrollToLine if scrolled up). + * Issue #423: fitAddon.fit() → terminal.resize() → buffer reflow resets the + * viewport to the top of the scrollback buffer. + * + * Issue #560: After display:none toggling (tab switches, panel collapse), + * xterm's buffer.active.viewportY can become stale (reset to 0). The fix + * tracks scroll state externally in JS variables immune to DOM state changes. */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { render, cleanup } from '@testing-library/react'; @@ -25,6 +27,7 @@ let mockTermInstance: { }; let mockFitInstance: { fit: ReturnType }; let mockResizeObserverCallback: (() => void) | null = null; +let mockOnScrollCallback: (() => void) | null = null; // Mock @xterm/xterm vi.mock('@xterm/xterm', () => { @@ -40,6 +43,10 @@ vi.mock('@xterm/xterm', () => { dispose = vi.fn(); onData = vi.fn(); onResize = vi.fn(); + onScroll = vi.fn((cb: () => void) => { + mockOnScrollCallback = cb; + return { dispose: vi.fn() }; + }); registerLinkProvider = vi.fn(() => ({ dispose: vi.fn() })); attachCustomKeyEventHandler = vi.fn(); cols = 80; @@ -105,10 +112,35 @@ vi.stubGlobal('ResizeObserver', class { // Import after mocks import { Terminal } from '../src/components/Terminal.js'; -describe('Terminal fit() scroll position preservation (Issue #423)', () => { +/** + * Helper: simulate a scroll event on the mock terminal. + * This updates the externally-tracked scroll state that safeFit() uses. + */ +function simulateScroll(baseY: number, viewportY: number) { + mockTermInstance.buffer.active.baseY = baseY; + mockTermInstance.buffer.active.viewportY = viewportY; + mockOnScrollCallback?.(); +} + +/** + * Helper: mock getBoundingClientRect on the terminal container element. + * jsdom returns 0x0 by default, which causes safeFit() to skip. + */ +function mockContainerRect(width = 800, height = 600) { + const el = document.querySelector('.terminal-container'); + if (el) { + vi.spyOn(el, 'getBoundingClientRect').mockReturnValue({ + width, height, top: 0, left: 0, right: width, bottom: height, + x: 0, y: 0, toJSON: () => ({}), + }); + } +} + +describe('Terminal fit() scroll position preservation (Issue #423, #560)', () => { beforeEach(() => { vi.useFakeTimers(); mockResizeObserverCallback = null; + mockOnScrollCallback = null; }); afterEach(() => { @@ -116,15 +148,16 @@ describe('Terminal fit() scroll position preservation (Issue #423)', () => { vi.useRealTimers(); }); - it.skip('calls scrollToBottom after fit() when viewport is at the bottom', () => { // FLAKY: skipped pending investigation — fails on main (jsdom getBoundingClientRect returns 0x0) + it('calls scrollToBottom after fit() when viewport is at the bottom', () => { render(); + mockContainerRect(); - // Simulate: terminal has scrollback, user is at the bottom - mockTermInstance.buffer.active.baseY = 500; - mockTermInstance.buffer.active.viewportY = 500; // at bottom: viewportY >= baseY + // Simulate scroll state: terminal has scrollback, user is at the bottom + simulateScroll(500, 500); - // Clear any initial scrollToBottom calls + // Clear any initial calls mockTermInstance.scrollToBottom.mockClear(); + mockFitInstance.fit.mockClear(); // Trigger ResizeObserver → debouncedFit → safeFit mockResizeObserverCallback?.(); @@ -135,15 +168,16 @@ describe('Terminal fit() scroll position preservation (Issue #423)', () => { expect(mockTermInstance.scrollToLine).not.toHaveBeenCalled(); }); - it.skip('calls scrollToLine to restore position when user has scrolled up', () => { // FLAKY: skipped pending investigation — fails on main + it('calls scrollToLine to restore position when user has scrolled up', () => { render(); + mockContainerRect(); // Simulate: terminal has scrollback, user scrolled up to line 200 - mockTermInstance.buffer.active.baseY = 500; - mockTermInstance.buffer.active.viewportY = 200; // scrolled up: viewportY < baseY + simulateScroll(500, 200); mockTermInstance.scrollToBottom.mockClear(); mockTermInstance.scrollToLine.mockClear(); + mockFitInstance.fit.mockClear(); // Trigger ResizeObserver → debouncedFit → safeFit mockResizeObserverCallback?.(); @@ -154,24 +188,31 @@ describe('Terminal fit() scroll position preservation (Issue #423)', () => { expect(mockTermInstance.scrollToBottom).not.toHaveBeenCalled(); }); - it.skip('skips scroll preservation on initial safeFit when buffer is empty', () => { // FLAKY: skipped pending investigation — fails on main - // The initial safeFit() runs synchronously during render. - // With an empty buffer (baseY=0), scroll preservation is skipped — - // there's no scrollback content to lose. + it('calls fit without scroll preservation when buffer is empty', () => { render(); + mockContainerRect(); + + // Buffer is empty (default: baseY=0, viewportY=0) + // Clear any calls from render (initial safeFit skips due to jsdom 0x0) + mockFitInstance.fit.mockClear(); + mockTermInstance.scrollToBottom.mockClear(); + mockTermInstance.scrollToLine.mockClear(); + + // Trigger ResizeObserver with empty buffer + mockResizeObserverCallback?.(); + vi.advanceTimersByTime(150); + // With empty buffer, safeFit should just call fit() without scroll preservation expect(mockFitInstance.fit).toHaveBeenCalled(); - // No scrollToBottom or scrollToLine calls (no scrollback to preserve) - expect(mockTermInstance.scrollToBottom).not.toHaveBeenCalled(); expect(mockTermInstance.scrollToLine).not.toHaveBeenCalled(); }); - it.skip('preserves position across multiple rapid ResizeObserver triggers', () => { // FLAKY: skipped pending investigation — fails on main + it('preserves position across multiple rapid ResizeObserver triggers', () => { render(); + mockContainerRect(); // User is scrolled up - mockTermInstance.buffer.active.baseY = 1000; - mockTermInstance.buffer.active.viewportY = 300; + simulateScroll(1000, 300); mockTermInstance.scrollToBottom.mockClear(); mockTermInstance.scrollToLine.mockClear(); @@ -190,4 +231,83 @@ describe('Terminal fit() scroll position preservation (Issue #423)', () => { // Scroll position should be restored expect(mockTermInstance.scrollToLine).toHaveBeenCalledWith(300); }); + + it('preserves scroll position even when xterm viewportY becomes stale (Issue #560)', () => { + // This tests the scenario where display:none toggling resets xterm's + // internal viewportY to 0, but our externally-tracked state still has + // the correct position. + render(); + mockContainerRect(); + + // Step 1: Simulate user scrolled to line 200 (not at bottom) + simulateScroll(500, 200); + + // Step 2: Simulate display:none toggling resetting xterm's viewportY + // (this is what happens when a tab becomes hidden/visible) + mockTermInstance.buffer.active.viewportY = 0; // DOM scroll reset + // NOTE: we do NOT call simulateScroll — the onScroll event doesn't + // fire during display:none, so our tracked state retains the old value. + + mockTermInstance.scrollToBottom.mockClear(); + mockTermInstance.scrollToLine.mockClear(); + mockFitInstance.fit.mockClear(); + + // Step 3: Tab becomes visible, ResizeObserver fires + mockResizeObserverCallback?.(); + vi.advanceTimersByTime(150); + + // safeFit should use the externally-tracked state (viewportY=200), + // NOT xterm's stale state (viewportY=0) + expect(mockFitInstance.fit).toHaveBeenCalled(); + expect(mockTermInstance.scrollToLine).toHaveBeenCalledWith(200); + expect(mockTermInstance.scrollToBottom).not.toHaveBeenCalled(); + }); + + it('preserves at-bottom state even when xterm viewportY resets to 0 (Issue #560)', () => { + // Same as above but when user WAS at the bottom. + // Without the fix, viewportY=0 with baseY=500 would be interpreted + // as "scrolled up to the top" → scrollToLine(0) → stuck at top. + render(); + mockContainerRect(); + + // User is at the bottom + simulateScroll(500, 500); + + // display:none resets xterm's viewportY to 0 + mockTermInstance.buffer.active.viewportY = 0; + + mockTermInstance.scrollToBottom.mockClear(); + mockTermInstance.scrollToLine.mockClear(); + mockFitInstance.fit.mockClear(); + + // Tab becomes visible + mockResizeObserverCallback?.(); + vi.advanceTimersByTime(150); + + // Should scroll to bottom (tracked wasAtBottom=true), NOT scrollToLine(0) + expect(mockFitInstance.fit).toHaveBeenCalled(); + expect(mockTermInstance.scrollToBottom).toHaveBeenCalled(); + expect(mockTermInstance.scrollToLine).not.toHaveBeenCalled(); + }); + + it('skips fit when container has zero dimensions (display: none)', () => { + render(); + // Default jsdom returns 0x0 for getBoundingClientRect, simulating display:none + // Do NOT call mockContainerRect — leave dimensions at 0x0 + + simulateScroll(500, 200); + + mockFitInstance.fit.mockClear(); + mockTermInstance.scrollToBottom.mockClear(); + mockTermInstance.scrollToLine.mockClear(); + + // ResizeObserver fires with 0x0 (tab hidden) + mockResizeObserverCallback?.(); + vi.advanceTimersByTime(150); + + // safeFit should skip entirely — no fit, no scroll + expect(mockFitInstance.fit).not.toHaveBeenCalled(); + expect(mockTermInstance.scrollToBottom).not.toHaveBeenCalled(); + expect(mockTermInstance.scrollToLine).not.toHaveBeenCalled(); + }); }); diff --git a/packages/codev/dashboard/__tests__/Terminal.ime-dedup.test.tsx b/packages/codev/dashboard/__tests__/Terminal.ime-dedup.test.tsx index a15cef03..2f6d5368 100644 --- a/packages/codev/dashboard/__tests__/Terminal.ime-dedup.test.tsx +++ b/packages/codev/dashboard/__tests__/Terminal.ime-dedup.test.tsx @@ -36,6 +36,7 @@ vi.mock('@xterm/xterm', () => { capturedOnData = cb; }); onResize = vi.fn(); + onScroll = vi.fn(() => ({ dispose: vi.fn() })); registerLinkProvider = vi.fn(() => ({ dispose: vi.fn() })); attachCustomKeyEventHandler = vi.fn(); cols = 80; diff --git a/packages/codev/dashboard/__tests__/Terminal.input-filter.test.tsx b/packages/codev/dashboard/__tests__/Terminal.input-filter.test.tsx index a178ace2..e5939fac 100644 --- a/packages/codev/dashboard/__tests__/Terminal.input-filter.test.tsx +++ b/packages/codev/dashboard/__tests__/Terminal.input-filter.test.tsx @@ -34,6 +34,7 @@ vi.mock('@xterm/xterm', () => { capturedOnData = cb; }); onResize = vi.fn(); + onScroll = vi.fn(() => ({ dispose: vi.fn() })); registerLinkProvider = vi.fn(() => ({ dispose: vi.fn() })); attachCustomKeyEventHandler = vi.fn(); cols = 80; diff --git a/packages/codev/dashboard/__tests__/Terminal.reconnect.test.tsx b/packages/codev/dashboard/__tests__/Terminal.reconnect.test.tsx index 68f97b6e..0efac5d0 100644 --- a/packages/codev/dashboard/__tests__/Terminal.reconnect.test.tsx +++ b/packages/codev/dashboard/__tests__/Terminal.reconnect.test.tsx @@ -62,6 +62,7 @@ vi.mock('@xterm/xterm', () => { dispose = vi.fn(); onData = vi.fn(); onResize = vi.fn(); + onScroll = vi.fn(() => ({ dispose: vi.fn() })); registerLinkProvider = vi.fn(() => ({ dispose: vi.fn() })); attachCustomKeyEventHandler = vi.fn(); scrollToBottom = vi.fn(); diff --git a/packages/codev/dashboard/__tests__/Terminal.replay-scroll.test.tsx b/packages/codev/dashboard/__tests__/Terminal.replay-scroll.test.tsx index 33ced65d..24f490ea 100644 --- a/packages/codev/dashboard/__tests__/Terminal.replay-scroll.test.tsx +++ b/packages/codev/dashboard/__tests__/Terminal.replay-scroll.test.tsx @@ -38,6 +38,7 @@ vi.mock('@xterm/xterm', () => { dispose = vi.fn(); onData = vi.fn(); onResize = vi.fn(); + onScroll = vi.fn(() => ({ dispose: vi.fn() })); registerLinkProvider = vi.fn(() => ({ dispose: vi.fn() })); attachCustomKeyEventHandler = vi.fn(); cols = 80; diff --git a/packages/codev/dashboard/__tests__/Terminal.scroll.test.tsx b/packages/codev/dashboard/__tests__/Terminal.scroll.test.tsx index 20cfba92..c1989760 100644 --- a/packages/codev/dashboard/__tests__/Terminal.scroll.test.tsx +++ b/packages/codev/dashboard/__tests__/Terminal.scroll.test.tsx @@ -26,6 +26,7 @@ vi.mock('@xterm/xterm', () => { dispose = vi.fn(); onData = vi.fn(); onResize = vi.fn(); + onScroll = vi.fn(() => ({ dispose: vi.fn() })); registerLinkProvider = vi.fn(() => ({ dispose: vi.fn() })); attachCustomKeyEventHandler = vi.fn(); cols = 80; diff --git a/packages/codev/dashboard/__tests__/VirtualKeyboard.test.tsx b/packages/codev/dashboard/__tests__/VirtualKeyboard.test.tsx index 06f87a63..7854524c 100644 --- a/packages/codev/dashboard/__tests__/VirtualKeyboard.test.tsx +++ b/packages/codev/dashboard/__tests__/VirtualKeyboard.test.tsx @@ -30,6 +30,7 @@ vi.mock('@xterm/xterm', () => { capturedOnData = cb; }); onResize = vi.fn(); + onScroll = vi.fn(() => ({ dispose: vi.fn() })); registerLinkProvider = vi.fn(() => ({ dispose: vi.fn() })); attachCustomKeyEventHandler = vi.fn(); cols = 80; diff --git a/packages/codev/dashboard/__tests__/architect-toolbar.test.tsx b/packages/codev/dashboard/__tests__/architect-toolbar.test.tsx index 7b76a66c..1c2ac48c 100644 --- a/packages/codev/dashboard/__tests__/architect-toolbar.test.tsx +++ b/packages/codev/dashboard/__tests__/architect-toolbar.test.tsx @@ -21,6 +21,7 @@ vi.mock('@xterm/xterm', () => { dispose = vi.fn(); onData = vi.fn(); onResize = vi.fn(); + onScroll = vi.fn(() => ({ dispose: vi.fn() })); registerLinkProvider = vi.fn(() => ({ dispose: vi.fn() })); attachCustomKeyEventHandler = vi.fn(); scrollToBottom = vi.fn(); diff --git a/packages/codev/dashboard/src/components/Terminal.tsx b/packages/codev/dashboard/src/components/Terminal.tsx index d321ed8d..608ea745 100644 --- a/packages/codev/dashboard/src/components/Terminal.tsx +++ b/packages/codev/dashboard/src/components/Terminal.tsx @@ -354,11 +354,30 @@ export function Terminal({ wsPath, onFileOpen, persistent, toolbarExtra }: Termi const onNativePaste = (e: Event) => handleNativePaste(e as ClipboardEvent, term); containerRef.current.addEventListener('paste', onNativePaste); + // Scroll state tracked externally in JS variables (Bugfix #560). + // xterm's buffer.active.viewportY (backed by ydisp) can become stale + // when the terminal container is hidden via display:none (tab switches, + // panel collapse). The browser resets the viewport element's scrollTop + // to 0, and xterm may sync ydisp to 0. Reading viewportY after the + // container becomes visible again returns 0 instead of the user's + // actual position. By tracking scroll state here, we're immune to + // DOM state changes from display toggling. + const scrollState = { viewportY: 0, baseY: 0, wasAtBottom: true }; + + // Update tracked scroll state on every scroll event. + const scrollDisposable = term.onScroll(() => { + const baseY = term.buffer?.active?.baseY ?? 0; + const viewportY = term.buffer?.active?.viewportY ?? 0; + scrollState.baseY = baseY; + scrollState.viewportY = viewportY; + scrollState.wasAtBottom = !baseY || viewportY >= baseY; + }); + // Scroll-aware fit: preserves the viewport scroll position across // fit() calls. Without this, fit() → resize() → buffer reflow can // reset the viewport to the top of the scrollback (Bugfix #423). - // Only activates when there IS scrollback (baseY > 0); when the buffer - // is empty or all content fits in the viewport, there's nothing to lose. + // Uses externally-tracked scroll state instead of reading from xterm's + // buffer to avoid stale values after display:none toggling (Bugfix #560). const safeFit = () => { // Skip fit when container is hidden (display: none) or has zero dimensions. // ResizeObserver fires with 0x0 when tabs switch — fitting at that size @@ -367,17 +386,20 @@ export function Terminal({ wsPath, onFileOpen, persistent, toolbarExtra }: Termi if (!rect || rect.width === 0 || rect.height === 0) return; const baseY = term.buffer?.active?.baseY; - if (!baseY) { + if (!baseY && !scrollState.baseY) { fitAddon.fit(); return; } - const viewportY = term.buffer.active.viewportY; - const wasAtBottom = viewportY == null || viewportY >= baseY; + + // Use externally-tracked state — immune to display:none scroll reset + const wasAtBottom = scrollState.wasAtBottom; + const restoreY = scrollState.viewportY; + fitAddon.fit(); if (wasAtBottom) { term.scrollToBottom(); } else { - term.scrollToLine(viewportY); + term.scrollToLine(restoreY); } }; @@ -438,7 +460,11 @@ export function Terminal({ wsPath, onFileOpen, persistent, toolbarExtra }: Termi if (rc.initialBuffer) { const filtered = filterDA(rc.initialBuffer); if (filtered) { - term.write(filtered, () => { term.scrollToBottom(); }); + term.write(filtered, () => { + term.scrollToBottom(); + // Sync tracked scroll state after replay (Bugfix #560) + scrollState.wasAtBottom = true; + }); } rc.initialBuffer = ''; } @@ -448,6 +474,8 @@ export function Terminal({ wsPath, onFileOpen, persistent, toolbarExtra }: Termi sendControl(wsRef.current, 'resize', { cols: term.cols, rows: term.rows }); } term.scrollToBottom(); + // Sync tracked scroll state after replay (Bugfix #560) + scrollState.wasAtBottom = true; }, 350); }; @@ -642,6 +670,7 @@ export function Terminal({ wsPath, onFileOpen, persistent, toolbarExtra }: Termi textarea.removeEventListener('compositionstart', onCompositionStart); textarea.removeEventListener('compositionend', onCompositionEnd); } + scrollDisposable.dispose(); decorationManager?.dispose(); linkProviderDisposable?.dispose(); containerRef.current?.removeEventListener('paste', onNativePaste);