Fix: Terminal scrolls to top after tab switch (Issue #560)

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) => {

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

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<typeof vi.fn> };
 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,26 +112,52 @@ 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(() => {
     cleanup();
     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(<Terminal wsPath="/ws/terminal/test" />);
+    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(<Terminal wsPath="/ws/terminal/test" />);
+    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(<Terminal wsPath="/ws/terminal/test" />);
+    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(<Terminal wsPath="/ws/terminal/test" />);
+    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(<Terminal wsPath="/ws/terminal/test" />);
+    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(<Terminal wsPath="/ws/terminal/test" />);
+    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(<Terminal wsPath="/ws/terminal/test" />);
+    // 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();
+  });
 });

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;

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;

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

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;

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;

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;

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