From ae48e964f0f32122a994a1ff0bb57b4da0322e59 Mon Sep 17 00:00:00 2001 From: Shreya Keshive Date: Fri, 10 Oct 2025 13:18:38 -0700 Subject: [PATCH] feat(ui): add flicker detection and metrics (#10821) --- packages/cli/src/ui/App.test.tsx | 59 ++++----- packages/cli/src/ui/AppContainer.tsx | 4 + .../src/ui/components/DebugProfiler.test.tsx | 13 ++ .../cli/src/ui/components/DebugProfiler.tsx | 39 +++++- .../cli/src/ui/contexts/UIStateContext.tsx | 2 + .../src/ui/hooks/useFlickerDetector.test.ts | 114 ++++++++++++++++++ .../cli/src/ui/hooks/useFlickerDetector.ts | 43 +++++++ .../cli/src/ui/layouts/DefaultAppLayout.tsx | 9 +- .../src/ui/layouts/ScreenReaderAppLayout.tsx | 10 +- packages/cli/src/utils/events.ts | 1 + packages/core/src/telemetry/index.ts | 1 + packages/core/src/telemetry/metrics.test.ts | 24 ++++ packages/core/src/telemetry/metrics.ts | 17 +++ 13 files changed, 297 insertions(+), 39 deletions(-) create mode 100644 packages/cli/src/ui/hooks/useFlickerDetector.test.ts create mode 100644 packages/cli/src/ui/hooks/useFlickerDetector.ts diff --git a/packages/cli/src/ui/App.test.tsx b/packages/cli/src/ui/App.test.tsx index d8883e6735..904a7f7080 100644 --- a/packages/cli/src/ui/App.test.tsx +++ b/packages/cli/src/ui/App.test.tsx @@ -10,6 +10,11 @@ import { Text, useIsScreenReaderEnabled } from 'ink'; import { App } from './App.js'; import { UIStateContext, type UIState } from './contexts/UIStateContext.js'; import { StreamingState } from './types.js'; +import { + ConfigContext, + type Config, + type Telemetry, +} from './contexts/ConfigContext.js'; vi.mock('ink', async (importOriginal) => { const original = await importOriginal(); @@ -49,6 +54,7 @@ describe('App', () => { quittingMessages: null, dialogsVisible: false, mainControlsRef: { current: null }, + rootUiRef: { current: null }, historyManager: { addItem: vi.fn(), history: [], @@ -58,13 +64,20 @@ describe('App', () => { }, }; - it('should render main content and composer when not quitting', () => { - const { lastFrame } = render( - - - , + const mockConfig = { + telemetry: {} as Telemetry, + } as Config; + + const renderWithProviders = (ui: React.ReactElement, state: UIState) => + render( + + {ui} + , ); + it('should render main content and composer when not quitting', () => { + const { lastFrame } = renderWithProviders(, mockUIState as UIState); + expect(lastFrame()).toContain('MainContent'); expect(lastFrame()).toContain('Notifications'); expect(lastFrame()).toContain('Composer'); @@ -76,11 +89,7 @@ describe('App', () => { quittingMessages: [{ id: 1, type: 'user', text: 'test' }], } as UIState; - const { lastFrame } = render( - - - , - ); + const { lastFrame } = renderWithProviders(, quittingUIState); expect(lastFrame()).toContain('Quitting...'); }); @@ -91,11 +100,7 @@ describe('App', () => { dialogsVisible: true, } as UIState; - const { lastFrame } = render( - - - , - ); + const { lastFrame } = renderWithProviders(, dialogUIState); expect(lastFrame()).toContain('MainContent'); expect(lastFrame()).toContain('Notifications'); @@ -109,11 +114,7 @@ describe('App', () => { ctrlCPressedOnce: true, } as UIState; - const { lastFrame } = render( - - - , - ); + const { lastFrame } = renderWithProviders(, ctrlCUIState); expect(lastFrame()).toContain('Press Ctrl+C again to exit.'); }); @@ -125,11 +126,7 @@ describe('App', () => { ctrlDPressedOnce: true, } as UIState; - const { lastFrame } = render( - - - , - ); + const { lastFrame } = renderWithProviders(, ctrlDUIState); expect(lastFrame()).toContain('Press Ctrl+D again to exit.'); }); @@ -137,11 +134,7 @@ describe('App', () => { it('should render ScreenReaderAppLayout when screen reader is enabled', () => { (useIsScreenReaderEnabled as vi.Mock).mockReturnValue(true); - const { lastFrame } = render( - - - , - ); + const { lastFrame } = renderWithProviders(, mockUIState as UIState); expect(lastFrame()).toContain( 'Notifications\nFooter\nMainContent\nComposer', @@ -151,11 +144,7 @@ describe('App', () => { it('should render DefaultAppLayout when screen reader is not enabled', () => { (useIsScreenReaderEnabled as vi.Mock).mockReturnValue(false); - const { lastFrame } = render( - - - , - ); + const { lastFrame } = renderWithProviders(, mockUIState as UIState); expect(lastFrame()).toContain('MainContent\nNotifications\nComposer'); }); diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index f8bdda1899..bad03fa568 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -210,6 +210,8 @@ export const AppContainer = (props: AppContainerProps) => { // Layout measurements const mainControlsRef = useRef(null); + // For performance profiling only + const rootUiRef = useRef(null); const originalTitleRef = useRef( computeWindowTitle(basename(config.getTargetDir())), ); @@ -1148,6 +1150,7 @@ Logging in with Google... Please restart Gemini CLI to continue. terminalWidth, terminalHeight, mainControlsRef, + rootUiRef, currentIDE, updateInfo, showIdeRestartPrompt, @@ -1227,6 +1230,7 @@ Logging in with Google... Please restart Gemini CLI to continue. terminalWidth, terminalHeight, mainControlsRef, + rootUiRef, currentIDE, updateInfo, showIdeRestartPrompt, diff --git a/packages/cli/src/ui/components/DebugProfiler.test.tsx b/packages/cli/src/ui/components/DebugProfiler.test.tsx index 000995e08b..c7e63f0b26 100644 --- a/packages/cli/src/ui/components/DebugProfiler.test.tsx +++ b/packages/cli/src/ui/components/DebugProfiler.test.tsx @@ -5,6 +5,7 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { appEvents, AppEvent } from '../../utils/events.js'; import { profiler, ACTION_TIMESTAMP_CAPACITY, @@ -157,6 +158,18 @@ describe('DebugProfiler', () => { expect(profiler.totalIdleFrames).toBe(3); }); + it('should report flicker frames', () => { + const reportActionSpy = vi.spyOn(profiler, 'reportAction'); + const cleanup = profiler.registerFlickerHandler(true); + + appEvents.emit(AppEvent.Flicker); + + expect(profiler.totalFlickerFrames).toBe(1); + expect(reportActionSpy).toHaveBeenCalled(); + + cleanup(); + }); + it('should not report idle frames when actions are interleaved', async () => { const startTime = Date.now(); vi.setSystemTime(startTime); diff --git a/packages/cli/src/ui/components/DebugProfiler.tsx b/packages/cli/src/ui/components/DebugProfiler.tsx index a8ae225399..5b46e33251 100644 --- a/packages/cli/src/ui/components/DebugProfiler.tsx +++ b/packages/cli/src/ui/components/DebugProfiler.tsx @@ -23,6 +23,8 @@ export const FRAME_TIMESTAMP_CAPACITY = 2048; export const profiler = { numFrames: 0, totalIdleFrames: 0, + totalFlickerFrames: 0, + hasLoggedFirstFlicker: false, lastFrameStartTime: 0, openedDebugConsole: false, lastActionTimestamp: 0, @@ -114,10 +116,35 @@ export const profiler = { ); } }, + + registerFlickerHandler(constrainHeight: boolean) { + const flickerHandler = () => { + // If we are not constraining the height, we are intentionally + // overflowing the screen. + if (!constrainHeight) { + return; + } + + this.totalFlickerFrames++; + this.reportAction(); + + if (!this.hasLoggedFirstFlicker) { + this.hasLoggedFirstFlicker = true; + appEvents.emit( + AppEvent.LogError, + 'A flicker frame was detected. This will cause UI instability. Type `/profile` for more info.', + ); + } + }; + appEvents.on(AppEvent.Flicker, flickerHandler); + return () => { + appEvents.off(AppEvent.Flicker, flickerHandler); + }; + }, }; export const DebugProfiler = () => { - const { showDebugProfiler } = useUIState(); + const { showDebugProfiler, constrainHeight } = useUIState(); const [forceRefresh, setForceRefresh] = useState(0); // Effect for listening to stdin for keypresses and stdout for resize events. @@ -170,6 +197,11 @@ export const DebugProfiler = () => { return () => clearInterval(updateInterval); }, []); + useEffect( + () => profiler.registerFlickerHandler(constrainHeight), + [constrainHeight], + ); + // Effect for updating stats useEffect(() => { if (!showDebugProfiler) { @@ -191,7 +223,10 @@ export const DebugProfiler = () => { return ( Renders: {profiler.numFrames} (total),{' '} - {profiler.totalIdleFrames} (idle) + {profiler.totalIdleFrames} (idle),{' '} + + {profiler.totalFlickerFrames} (flicker) + ); }; diff --git a/packages/cli/src/ui/contexts/UIStateContext.tsx b/packages/cli/src/ui/contexts/UIStateContext.tsx index b7ca159361..b9a8b6f739 100644 --- a/packages/cli/src/ui/contexts/UIStateContext.tsx +++ b/packages/cli/src/ui/contexts/UIStateContext.tsx @@ -111,6 +111,8 @@ export interface UIState { terminalWidth: number; terminalHeight: number; mainControlsRef: React.MutableRefObject; + // NOTE: This is for performance profiling only. + rootUiRef: React.MutableRefObject; currentIDE: IdeInfo | null; updateInfo: UpdateObject | null; showIdeRestartPrompt: boolean; diff --git a/packages/cli/src/ui/hooks/useFlickerDetector.test.ts b/packages/cli/src/ui/hooks/useFlickerDetector.test.ts new file mode 100644 index 0000000000..5ccccc111c --- /dev/null +++ b/packages/cli/src/ui/hooks/useFlickerDetector.test.ts @@ -0,0 +1,114 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { renderHook } from '@testing-library/react'; +import { vi, type Mock } from 'vitest'; +import { useFlickerDetector } from './useFlickerDetector.js'; +import { useConfig } from '../contexts/ConfigContext.js'; +import { recordFlickerFrame } from '@google/gemini-cli-core'; +import { type Config } from '@google/gemini-cli-core'; +import { type DOMElement, measureElement } from 'ink'; +import { useUIState } from '../contexts/UIStateContext.js'; +import { appEvents, AppEvent } from '../../utils/events.js'; + +// Mock dependencies +vi.mock('../contexts/ConfigContext.js'); +vi.mock('../contexts/UIStateContext.js'); +vi.mock('@google/gemini-cli-core', () => ({ + recordFlickerFrame: vi.fn(), +})); +vi.mock('ink', async (importOriginal) => { + const original = await importOriginal(); + return { + ...original, + measureElement: vi.fn(), + }; +}); +vi.mock('../../utils/events.js', () => ({ + appEvents: { + emit: vi.fn(), + }, + AppEvent: { + Flicker: 'flicker', + }, +})); + +const mockUseConfig = useConfig as Mock; +const mockUseUIState = useUIState as Mock; +const mockRecordFlickerFrame = recordFlickerFrame as Mock; +const mockMeasureElement = measureElement as Mock; +const mockAppEventsEmit = appEvents.emit as Mock; + +describe('useFlickerDetector', () => { + const mockConfig = {} as Config; + let mockRef: React.RefObject; + + beforeEach(() => { + mockUseConfig.mockReturnValue(mockConfig); + mockRef = { current: { yogaNode: {} } as DOMElement }; + // Default UI state + mockUseUIState.mockReturnValue({ constrainHeight: true }); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('should not record a flicker when height is less than terminal height', () => { + mockMeasureElement.mockReturnValue({ width: 80, height: 20 }); + renderHook(() => useFlickerDetector(mockRef, 25)); + expect(mockRecordFlickerFrame).not.toHaveBeenCalled(); + expect(mockAppEventsEmit).not.toHaveBeenCalled(); + }); + + it('should not record a flicker when height is equal to terminal height', () => { + mockMeasureElement.mockReturnValue({ width: 80, height: 25 }); + renderHook(() => useFlickerDetector(mockRef, 25)); + expect(mockRecordFlickerFrame).not.toHaveBeenCalled(); + expect(mockAppEventsEmit).not.toHaveBeenCalled(); + }); + + it('should record a flicker when height is greater than terminal height and height is constrained', () => { + mockMeasureElement.mockReturnValue({ width: 80, height: 30 }); + renderHook(() => useFlickerDetector(mockRef, 25)); + expect(mockRecordFlickerFrame).toHaveBeenCalledTimes(1); + expect(mockRecordFlickerFrame).toHaveBeenCalledWith(mockConfig); + expect(mockAppEventsEmit).toHaveBeenCalledTimes(1); + expect(mockAppEventsEmit).toHaveBeenCalledWith(AppEvent.Flicker); + }); + + it('should NOT record a flicker when height is greater than terminal height but height is NOT constrained', () => { + // Override default UI state for this test + mockUseUIState.mockReturnValue({ constrainHeight: false }); + mockMeasureElement.mockReturnValue({ width: 80, height: 30 }); + renderHook(() => useFlickerDetector(mockRef, 25)); + expect(mockRecordFlickerFrame).not.toHaveBeenCalled(); + expect(mockAppEventsEmit).not.toHaveBeenCalled(); + }); + + it('should not check for flicker if the ref is not set', () => { + mockRef.current = null; + mockMeasureElement.mockReturnValue({ width: 80, height: 30 }); + renderHook(() => useFlickerDetector(mockRef, 25)); + expect(mockMeasureElement).not.toHaveBeenCalled(); + expect(mockRecordFlickerFrame).not.toHaveBeenCalled(); + expect(mockAppEventsEmit).not.toHaveBeenCalled(); + }); + + it('should re-evaluate on re-render', () => { + // Start with a valid height + mockMeasureElement.mockReturnValue({ width: 80, height: 20 }); + const { rerender } = renderHook(() => useFlickerDetector(mockRef, 25)); + expect(mockRecordFlickerFrame).not.toHaveBeenCalled(); + + // Now, simulate a re-render where the height is too great + mockMeasureElement.mockReturnValue({ width: 80, height: 30 }); + rerender(); + + expect(mockRecordFlickerFrame).toHaveBeenCalledTimes(1); + expect(mockAppEventsEmit).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/cli/src/ui/hooks/useFlickerDetector.ts b/packages/cli/src/ui/hooks/useFlickerDetector.ts new file mode 100644 index 0000000000..49ee6ca8a9 --- /dev/null +++ b/packages/cli/src/ui/hooks/useFlickerDetector.ts @@ -0,0 +1,43 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { type DOMElement, measureElement } from 'ink'; +import { useEffect } from 'react'; +import { useConfig } from '../contexts/ConfigContext.js'; +import { recordFlickerFrame } from '@google/gemini-cli-core'; +import { appEvents, AppEvent } from '../../utils/events.js'; +import { useUIState } from '../contexts/UIStateContext.js'; + +/** + * A hook that detects when the UI flickers (renders taller than the terminal). + * This is a sign of a rendering bug that should be fixed. + * + * @param rootUiRef A ref to the root UI element. + * @param terminalHeight The height of the terminal. + */ +export function useFlickerDetector( + rootUiRef: React.RefObject, + terminalHeight: number, +) { + const config = useConfig(); + const { constrainHeight } = useUIState(); + + useEffect(() => { + if (rootUiRef.current) { + const measurement = measureElement(rootUiRef.current); + if (measurement.height > terminalHeight) { + // If we are not constraining the height, we are intentionally + // overflowing the screen. + if (!constrainHeight) { + return; + } + + recordFlickerFrame(config); + appEvents.emit(AppEvent.Flicker); + } + } + }); +} diff --git a/packages/cli/src/ui/layouts/DefaultAppLayout.tsx b/packages/cli/src/ui/layouts/DefaultAppLayout.tsx index b0835ec77b..2f92b77a23 100644 --- a/packages/cli/src/ui/layouts/DefaultAppLayout.tsx +++ b/packages/cli/src/ui/layouts/DefaultAppLayout.tsx @@ -12,12 +12,19 @@ import { DialogManager } from '../components/DialogManager.js'; import { Composer } from '../components/Composer.js'; import { ExitWarning } from '../components/ExitWarning.js'; import { useUIState } from '../contexts/UIStateContext.js'; +import { useFlickerDetector } from '../hooks/useFlickerDetector.js'; export const DefaultAppLayout: React.FC = () => { const uiState = useUIState(); + const { rootUiRef, terminalHeight } = uiState; + useFlickerDetector(rootUiRef, terminalHeight); return ( - + diff --git a/packages/cli/src/ui/layouts/ScreenReaderAppLayout.tsx b/packages/cli/src/ui/layouts/ScreenReaderAppLayout.tsx index b25646bb32..d88f3f1fb2 100644 --- a/packages/cli/src/ui/layouts/ScreenReaderAppLayout.tsx +++ b/packages/cli/src/ui/layouts/ScreenReaderAppLayout.tsx @@ -13,12 +13,20 @@ import { Composer } from '../components/Composer.js'; import { Footer } from '../components/Footer.js'; import { ExitWarning } from '../components/ExitWarning.js'; import { useUIState } from '../contexts/UIStateContext.js'; +import { useFlickerDetector } from '../hooks/useFlickerDetector.js'; export const ScreenReaderAppLayout: React.FC = () => { const uiState = useUIState(); + const { rootUiRef, terminalHeight } = uiState; + useFlickerDetector(rootUiRef, terminalHeight); return ( - +