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);