From 566d2c12794ba8ec06825bbef8bfececcba44cc6 Mon Sep 17 00:00:00 2001 From: ChiGao Date: Thu, 30 Jul 2026 00:04:42 +0800 Subject: [PATCH] fix(cli): stop resize repaint from causing scroll storm (#8009) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cli): stop resize repaint from causing scroll storm (#8004) Remove the useResizeSettleRepaint -> refreshStatic wiring that wrote clearTerminal (destroying scrollback) and remounted on every settled resize, re-emitting all conversation history in 50-item chunks. Ghostty's panel-toggle animation exceeds the 200ms debounce, triggering multiple settle-repaint cycles per toggle -- visible as continuous scrolling/flickering. Ink's dynamic region already re-renders on width changes via useTerminalSize; modern terminals handle scrollback reflow natively. The full remount is no longer necessary. The now-unused hook and its test are removed (no remaining callers). * test(cli): guard resize no-repaint contract against settle-time regression (#8004) * test(cli): make resize settle regression test non-vacuous (#8004) The previous test used rerender() which remounts the tree via ink's ErrorBoundary (measureElement returns undefined → layout effect throws → tree unmounted), so the settle debounce never fired and the test passed regardless. Rewrite to keep the tree alive (measureElement mock returns a real value) and deliver width changes to the same mounted instance via a listener pattern. Mutation-verified: fails when the removed useResizeSettleRepaint hook is restored. * chore(cli): remove dead useResizeSettleRepaint from eslint legacy filenames (#8004) --------- Co-authored-by: 秦奇 Co-authored-by: qwen-code-ci-bot --- eslint.legacy-filenames.mjs | 1 - packages/cli/src/ui/AppContainer.test.tsx | 71 ++++++++++-- packages/cli/src/ui/AppContainer.tsx | 9 +- .../ui/hooks/useResizeSettleRepaint.test.ts | 102 ------------------ .../src/ui/hooks/useResizeSettleRepaint.ts | 50 --------- 5 files changed, 69 insertions(+), 164 deletions(-) delete mode 100644 packages/cli/src/ui/hooks/useResizeSettleRepaint.test.ts delete mode 100644 packages/cli/src/ui/hooks/useResizeSettleRepaint.ts diff --git a/eslint.legacy-filenames.mjs b/eslint.legacy-filenames.mjs index c971af1fc9..992f22518c 100644 --- a/eslint.legacy-filenames.mjs +++ b/eslint.legacy-filenames.mjs @@ -563,7 +563,6 @@ export const legacyFilenames = [ 'toolResultDisplayCompaction', 'usageHistoryService', 'useMcpApproval', - 'useResizeSettleRepaint', 'useStatsDialog', 'useTeamInProcess', 'userMemory', diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index fe6154110c..71843d9dfb 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -32,7 +32,7 @@ import { type Mock, } from 'vitest'; import { render, cleanup } from 'ink-testing-library'; -import { useContext, useState, act } from 'react'; +import { useContext, useState, useReducer, useEffect, act } from 'react'; import { AppContainer, dedupeNewestFirst, @@ -1092,13 +1092,10 @@ describe('AppContainer State Management', () => { expect(capturedUIState.useTerminalBuffer).toBe(true); }); - // #4891 changed the resize contract: width changes now trigger ONE full - // clearTerminal after RESIZE_REPAINT_SETTLE_MS (trailing-edge debounce), - // instead of never (#3967) or per-event (pre-#3967). This test pins the - // synchronous half: no immediate clear during the burst. The settle-time - // half is not observable here — ink-testing-library's rerender does not - // flush update-time passive effects — and is covered by - // useResizeSettleRepaint.test.ts. + // Resize no longer triggers a clearTerminal or history remount (#8004). + // The old settle → refreshStatic path caused a scroll storm; the dynamic + // region now re-renders via useTerminalSize alone. This test pins that + // no synchronous clear fires during a width change. it('does not clear the terminal synchronously on width change', () => { vi.spyOn(mockConfig, 'initialize').mockResolvedValue(undefined); mockedUseTerminalSize.mockReturnValue({ columns: 80, rows: 24 }); @@ -1127,6 +1124,64 @@ describe('AppContainer State Management', () => { ); }); + it('does not repaint static history after a resize settles (#8004)', () => { + vi.useFakeTimers(); + vi.spyOn(mockConfig, 'initialize').mockResolvedValue(undefined); + // measureElement must return a real measurement; a bare vi.fn() returns + // undefined, the controlsHeight layout effect throws on .height, and + // ink's ErrorBoundary silently unmounts the tree — making every + // post-mount assertion vacuous. + (measureElement as Mock).mockReturnValue({ width: 80, height: 2 }); + + // Deliver width changes to the SAME mounted instance. rerender() from + // ink-testing-library remounts the tree (ErrorBoundary issue above), + // re-seeding useRef(terminalWidth) so a settle debounce never fires. + let columns = 80; + const resizeListeners = new Set<() => void>(); + mockedUseTerminalSize.mockImplementation(() => { + const [, force] = useReducer((x: number) => x + 1, 0); + useEffect(() => { + resizeListeners.add(force); + return () => { + resizeListeners.delete(force); + }; + }, []); + return { columns, rows: 24 }; + }); + + render( + , + ); + + // Liveness control: fails if the ErrorBoundary unmounted the tree. + expect(resizeListeners.size).toBeGreaterThan(0); + const remountKeyBefore = capturedUIState.historyRemountKey; + mockStdout.write.mockClear(); + + act(() => { + columns = 100; + for (const notify of resizeListeners) notify(); + }); + + // Advance well past the old RESIZE_REPAINT_SETTLE_MS (200ms) debounce. + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(mockStdout.write).not.toHaveBeenCalledWith( + ansiEscapes.clearTerminal, + ); + expect(capturedUIState.historyRemountKey).toBe(remountKeyBefore); + + vi.useRealTimers(); + (measureElement as Mock).mockReturnValue(undefined); + }); + it('handleClearScreen avoids a second clearTerminal write', () => { const clearSpy = vi.spyOn(console, 'clear').mockImplementation(() => {}); diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index f911d5c2eb..0eaf5195b4 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -119,7 +119,6 @@ const MCP_BATCH_FLUSH_MS = 16; const STARTUP_PROFILE_FINALIZE_CAP_MS = 35_000; import { useHistory } from './hooks/useHistoryManager.js'; import { useMemoryMonitor } from './hooks/useMemoryMonitor.js'; -import { useResizeSettleRepaint } from './hooks/useResizeSettleRepaint.js'; import { useWakeRepaint } from './hooks/use-wake-repaint.js'; import { useThemeCommand } from './hooks/useThemeCommand.js'; import { useFeedbackDialog } from './hooks/useFeedbackDialog.js'; @@ -3245,8 +3244,12 @@ export const AppContainer = (props: AppContainerProps) => { } }, [terminalWidth, availableTerminalHeight, activePtyId]); - // Repaint static history on the trailing edge of a resize burst (#4891). - useResizeSettleRepaint(terminalWidth, refreshStatic); + // Resize no longer repaints static history (#8004). The old settle → + // refreshStatic path wrote clearTerminal (destroying scrollback) and remounted + // , re-emitting all history in 50-item chunks — a scroll storm when + // the terminal's resize animation exceeded the debounce window (e.g. Ghostty + // panel toggle). Ink's dynamic region already re-renders on width changes via + // useTerminalSize; modern terminals reflow scrollback natively. // Repaint after the process resumes from OS sleep / suspend (lid close, // display sleep, Ctrl+Z → fg). The terminal's screen buffer is stale but diff --git a/packages/cli/src/ui/hooks/useResizeSettleRepaint.test.ts b/packages/cli/src/ui/hooks/useResizeSettleRepaint.test.ts deleted file mode 100644 index fc6a67ac5e..0000000000 --- a/packages/cli/src/ui/hooks/useResizeSettleRepaint.test.ts +++ /dev/null @@ -1,102 +0,0 @@ -/** - * @license - * Copyright 2025 Qwen - * SPDX-License-Identifier: Apache-2.0 - */ - -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { renderHook, act } from '@testing-library/react'; -import { - RESIZE_REPAINT_SETTLE_MS, - useResizeSettleRepaint, -} from './useResizeSettleRepaint.js'; - -describe('useResizeSettleRepaint (#4891)', () => { - beforeEach(() => { - vi.useFakeTimers(); - }); - - afterEach(() => { - vi.useRealTimers(); - }); - - const setup = (initialWidth: number) => { - const refreshStatic = vi.fn(); - const view = renderHook( - ({ width }: { width: number }) => - useResizeSettleRepaint(width, refreshStatic), - { initialProps: { width: initialWidth } }, - ); - return { refreshStatic, view }; - }; - - it('does not repaint on first mount', () => { - const { refreshStatic } = setup(80); - - act(() => { - vi.advanceTimersByTime(RESIZE_REPAINT_SETTLE_MS + 50); - }); - - expect(refreshStatic).not.toHaveBeenCalled(); - }); - - it('coalesces a burst of width changes into one repaint after settle', () => { - const { refreshStatic, view } = setup(80); - - // Three rapid width changes, each well within the settle window. - act(() => view.rerender({ width: 90 })); - act(() => vi.advanceTimersByTime(100)); - act(() => view.rerender({ width: 100 })); - act(() => vi.advanceTimersByTime(100)); - act(() => view.rerender({ width: 110 })); - - // Burst not yet settled (measured from the last change): nothing fired. - act(() => vi.advanceTimersByTime(RESIZE_REPAINT_SETTLE_MS - 1)); - expect(refreshStatic).not.toHaveBeenCalled(); - - // Settle window elapses → exactly one repaint for the whole burst. - act(() => vi.advanceTimersByTime(1)); - expect(refreshStatic).toHaveBeenCalledTimes(1); - }); - - it('does not repaint when a drag returns to the original width', () => { - const { refreshStatic, view } = setup(80); - - act(() => view.rerender({ width: 100 })); - act(() => vi.advanceTimersByTime(RESIZE_REPAINT_SETTLE_MS - 1)); // pending - act(() => view.rerender({ width: 80 })); // back to start before it fires - - act(() => vi.advanceTimersByTime(RESIZE_REPAINT_SETTLE_MS + 50)); - expect(refreshStatic).not.toHaveBeenCalled(); - }); - - it('repaints exactly once after a single settled width change', () => { - const { refreshStatic, view } = setup(80); - - act(() => view.rerender({ width: 100 })); - act(() => vi.advanceTimersByTime(RESIZE_REPAINT_SETTLE_MS)); - - expect(refreshStatic).toHaveBeenCalledTimes(1); - }); - - it('schedules nothing when a re-render leaves the width unchanged', () => { - // A height-only resize re-renders with the same width: no repaint. - const { refreshStatic, view } = setup(80); - - act(() => view.rerender({ width: 80 })); - act(() => vi.advanceTimersByTime(RESIZE_REPAINT_SETTLE_MS + 50)); - - expect(refreshStatic).not.toHaveBeenCalled(); - }); - - it('cancels a pending repaint when unmounted mid-settle', () => { - const { refreshStatic, view } = setup(80); - - act(() => view.rerender({ width: 100 })); - act(() => vi.advanceTimersByTime(RESIZE_REPAINT_SETTLE_MS - 1)); // pending - view.unmount(); - - act(() => vi.advanceTimersByTime(RESIZE_REPAINT_SETTLE_MS + 50)); - expect(refreshStatic).not.toHaveBeenCalled(); - }); -}); diff --git a/packages/cli/src/ui/hooks/useResizeSettleRepaint.ts b/packages/cli/src/ui/hooks/useResizeSettleRepaint.ts deleted file mode 100644 index c5b8138ee5..0000000000 --- a/packages/cli/src/ui/hooks/useResizeSettleRepaint.ts +++ /dev/null @@ -1,50 +0,0 @@ -/** - * @license - * Copyright 2025 Qwen - * SPDX-License-Identifier: Apache-2.0 - */ - -import { useEffect, useRef } from 'react'; - -// Trailing-edge debounce for resize-triggered static repaints (#4891). -export const RESIZE_REPAINT_SETTLE_MS = 200; - -/** - * Repaint the static history once the terminal width settles (#4891). - * - * A window drag fires dozens of `resize` events. Per-event repainting restarted - * the progressive replay (#3899) mid-flight, and the viewport-only - * cursorTo+eraseDown erase introduced by #3967 (4bab7a1a) cannot reach output - * that has already scrolled into the scrollback, so each event stranded a - * fragment at that instant's width. Debouncing to the trailing edge and issuing - * one full `refreshStatic` (clearTerminal incl. ESC[3J + remount) wipes the - * fragments and re-emits the history once at the final width. The cleanup - * cancels the pending timer on the next width change or unmount, so a drag - * returning to the start width is a no-op. - * - * Trade-off: the full-screen flash #3967 removed returns, but at most once per - * resize gesture; the settle-time ESC[3J clears pre-session scrollback — the - * same as the pre-#3967 per-event behavior and today's /clear. - * - * `refreshStatic` must be referentially stable (e.g. `useCallback`) so an - * unrelated re-render does not cancel an in-flight settle. - */ -export function useResizeSettleRepaint( - terminalWidth: number, - refreshStatic: () => void, -): void { - // Width at the last settled repaint; starts at mount width (first mount is a - // no-op) and only advances when a repaint actually fires. - const settledTerminalWidthRef = useRef(terminalWidth); - - useEffect(() => { - if (settledTerminalWidthRef.current === terminalWidth) { - return; - } - const timer = setTimeout(() => { - settledTerminalWidthRef.current = terminalWidth; - refreshStatic(); - }, RESIZE_REPAINT_SETTLE_MS); - return () => clearTimeout(timer); - }, [terminalWidth, refreshStatic]); -}