feat(ui): add flicker detection and metrics (#10821)

This commit is contained in:
Shreya Keshive
2025-10-10 13:18:38 -07:00
committed by GitHub
parent ab3804d823
commit ae48e964f0
13 changed files with 297 additions and 39 deletions

View File

@@ -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<typeof import('ink')>();
@@ -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(
<UIStateContext.Provider value={mockUIState as UIState}>
<App />
</UIStateContext.Provider>,
const mockConfig = {
telemetry: {} as Telemetry,
} as Config;
const renderWithProviders = (ui: React.ReactElement, state: UIState) =>
render(
<ConfigContext.Provider value={mockConfig}>
<UIStateContext.Provider value={state}>{ui}</UIStateContext.Provider>
</ConfigContext.Provider>,
);
it('should render main content and composer when not quitting', () => {
const { lastFrame } = renderWithProviders(<App />, 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(
<UIStateContext.Provider value={quittingUIState}>
<App />
</UIStateContext.Provider>,
);
const { lastFrame } = renderWithProviders(<App />, quittingUIState);
expect(lastFrame()).toContain('Quitting...');
});
@@ -91,11 +100,7 @@ describe('App', () => {
dialogsVisible: true,
} as UIState;
const { lastFrame } = render(
<UIStateContext.Provider value={dialogUIState}>
<App />
</UIStateContext.Provider>,
);
const { lastFrame } = renderWithProviders(<App />, dialogUIState);
expect(lastFrame()).toContain('MainContent');
expect(lastFrame()).toContain('Notifications');
@@ -109,11 +114,7 @@ describe('App', () => {
ctrlCPressedOnce: true,
} as UIState;
const { lastFrame } = render(
<UIStateContext.Provider value={ctrlCUIState}>
<App />
</UIStateContext.Provider>,
);
const { lastFrame } = renderWithProviders(<App />, ctrlCUIState);
expect(lastFrame()).toContain('Press Ctrl+C again to exit.');
});
@@ -125,11 +126,7 @@ describe('App', () => {
ctrlDPressedOnce: true,
} as UIState;
const { lastFrame } = render(
<UIStateContext.Provider value={ctrlDUIState}>
<App />
</UIStateContext.Provider>,
);
const { lastFrame } = renderWithProviders(<App />, 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(
<UIStateContext.Provider value={mockUIState as UIState}>
<App />
</UIStateContext.Provider>,
);
const { lastFrame } = renderWithProviders(<App />, 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(
<UIStateContext.Provider value={mockUIState as UIState}>
<App />
</UIStateContext.Provider>,
);
const { lastFrame } = renderWithProviders(<App />, mockUIState as UIState);
expect(lastFrame()).toContain('MainContent\nNotifications\nComposer');
});

View File

@@ -210,6 +210,8 @@ export const AppContainer = (props: AppContainerProps) => {
// Layout measurements
const mainControlsRef = useRef<DOMElement>(null);
// For performance profiling only
const rootUiRef = useRef<DOMElement>(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,

View File

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

View File

@@ -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 (
<Text color={theme.status.warning} key={forceRefresh}>
Renders: {profiler.numFrames} (total),{' '}
<Text color={theme.status.error}>{profiler.totalIdleFrames} (idle) </Text>
<Text color={theme.status.error}>{profiler.totalIdleFrames} (idle)</Text>,{' '}
<Text color={theme.status.error}>
{profiler.totalFlickerFrames} (flicker)
</Text>
</Text>
);
};

View File

@@ -111,6 +111,8 @@ export interface UIState {
terminalWidth: number;
terminalHeight: number;
mainControlsRef: React.MutableRefObject<DOMElement | null>;
// NOTE: This is for performance profiling only.
rootUiRef: React.MutableRefObject<DOMElement | null>;
currentIDE: IdeInfo | null;
updateInfo: UpdateObject | null;
showIdeRestartPrompt: boolean;

View File

@@ -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<typeof import('ink')>();
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<DOMElement | null>;
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);
});
});

View File

@@ -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<DOMElement | null>,
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);
}
}
});
}

View File

@@ -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 (
<Box flexDirection="column" width={uiState.mainAreaWidth}>
<Box
flexDirection="column"
width={uiState.mainAreaWidth}
ref={uiState.rootUiRef}
>
<MainContent />
<Box flexDirection="column" ref={uiState.mainControlsRef}>

View File

@@ -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 (
<Box flexDirection="column" width="90%" height="100%">
<Box
flexDirection="column"
width="90%"
height="100%"
ref={uiState.rootUiRef}
>
<Notifications />
<Footer />
<Box flexGrow={1} overflow="hidden">

View File

@@ -10,6 +10,7 @@ export enum AppEvent {
OpenDebugConsole = 'open-debug-console',
LogError = 'log-error',
OauthDisplayMessage = 'oauth-display-message',
Flicker = 'flicker',
}
export const appEvents = new EventEmitter();

View File

@@ -115,6 +115,7 @@ export {
recordPerformanceRegression,
recordBaselineComparison,
isPerformanceMonitoringActive,
recordFlickerFrame,
// Performance monitoring types
PerformanceMetricType,
MemoryMetricType,

View File

@@ -91,6 +91,7 @@ describe('Telemetry Metrics', () => {
let recordBaselineComparisonModule: typeof import('./metrics.js').recordBaselineComparison;
let recordGenAiClientTokenUsageModule: typeof import('./metrics.js').recordGenAiClientTokenUsage;
let recordGenAiClientOperationDurationModule: typeof import('./metrics.js').recordGenAiClientOperationDuration;
let recordFlickerFrameModule: typeof import('./metrics.js').recordFlickerFrame;
let recordAgentRunMetricsModule: typeof import('./metrics.js').recordAgentRunMetrics;
beforeEach(async () => {
@@ -131,6 +132,7 @@ describe('Telemetry Metrics', () => {
metricsJsModule.recordGenAiClientTokenUsage;
recordGenAiClientOperationDurationModule =
metricsJsModule.recordGenAiClientOperationDuration;
recordFlickerFrameModule = metricsJsModule.recordFlickerFrame;
recordAgentRunMetricsModule = metricsJsModule.recordAgentRunMetrics;
const otelApiModule = await import('@opentelemetry/api');
@@ -146,6 +148,28 @@ describe('Telemetry Metrics', () => {
mockCreateHistogramFn.mockReturnValue(mockHistogramInstance);
});
describe('recordFlickerFrame', () => {
it('does not record metrics if not initialized', () => {
const config = makeFakeConfig({});
recordFlickerFrameModule(config);
expect(mockCounterAddFn).not.toHaveBeenCalled();
});
it('records a flicker frame event when initialized', () => {
const config = makeFakeConfig({});
initializeMetricsModule(config);
recordFlickerFrameModule(config);
// Called for session, then for flicker
expect(mockCounterAddFn).toHaveBeenCalledTimes(2);
expect(mockCounterAddFn).toHaveBeenNthCalledWith(2, 1, {
'session.id': 'test-session-id',
'installation.id': 'test-installation-id',
'user.email': 'test@example.com',
});
});
});
describe('initializeMetrics', () => {
const mockConfig = {
getSessionId: () => 'test-session-id',

View File

@@ -55,6 +55,7 @@ const REGRESSION_DETECTION = 'gemini_cli.performance.regression';
const REGRESSION_PERCENTAGE_CHANGE =
'gemini_cli.performance.regression.percentage_change';
const BASELINE_COMPARISON = 'gemini_cli.performance.baseline.comparison';
const FLICKER_FRAME_COUNT = 'gemini_cli.ui.flicker.count';
const baseMetricDefinition = {
getCommonAttributes,
@@ -167,6 +168,13 @@ const COUNTER_DEFINITIONS = {
terminate_reason: string;
},
},
[FLICKER_FRAME_COUNT]: {
description:
'Counts UI frames that flicker (render taller than the terminal).',
valueType: ValueType.INT,
assign: (c: Counter) => (flickerFrameCounter = c),
attributes: {} as Record<string, never>,
},
} as const;
const HISTOGRAM_DEFINITIONS = {
@@ -449,6 +457,7 @@ let modelSlashCommandCallCounter: Counter | undefined;
let agentRunCounter: Counter | undefined;
let agentDurationHistogram: Histogram | undefined;
let agentTurnsHistogram: Histogram | undefined;
let flickerFrameCounter: Counter | undefined;
// OpenTelemetry GenAI Semantic Convention Metrics
let genAiClientTokenUsageHistogram: Histogram | undefined;
@@ -606,6 +615,14 @@ export function recordFileOperationMetric(
// --- New Metric Recording Functions ---
/**
* Records a metric for when a UI frame flickers.
*/
export function recordFlickerFrame(config: Config): void {
if (!flickerFrameCounter || !isMetricsInitialized) return;
flickerFrameCounter.add(1, baseMetricDefinition.getCommonAttributes(config));
}
/**
* Records a metric for when an invalid chunk is received from a stream.
*/