From 016a94ffaf0a120e982e177fc4dfaf51bf9f2ede Mon Sep 17 00:00:00 2001 From: Adib234 <30782825+Adib234@users.noreply.github.com> Date: Thu, 22 Jan 2026 15:46:18 -0500 Subject: [PATCH] Disable tips after 10 runs (#17101) --- .../cli/src/test-utils/persistentStateFake.ts | 45 ++++++ packages/cli/src/test-utils/render.tsx | 21 +++ .../AlternateBufferQuittingDisplay.test.tsx | 15 +- .../cli/src/ui/components/AppHeader.test.tsx | 145 +++++++++++++++--- packages/cli/src/ui/components/AppHeader.tsx | 7 +- .../src/ui/components/Notifications.test.tsx | 66 ++++---- .../cli/src/ui/components/Notifications.tsx | 42 ++--- .../__snapshots__/Notifications.test.tsx.snap | 2 +- packages/cli/src/ui/hooks/useTips.test.ts | 47 ++++++ packages/cli/src/ui/hooks/useTips.ts | 26 ++++ packages/cli/src/utils/persistentState.ts | 2 + 11 files changed, 327 insertions(+), 91 deletions(-) create mode 100644 packages/cli/src/test-utils/persistentStateFake.ts create mode 100644 packages/cli/src/ui/hooks/useTips.test.ts create mode 100644 packages/cli/src/ui/hooks/useTips.ts diff --git a/packages/cli/src/test-utils/persistentStateFake.ts b/packages/cli/src/test-utils/persistentStateFake.ts new file mode 100644 index 0000000000..512b25b95b --- /dev/null +++ b/packages/cli/src/test-utils/persistentStateFake.ts @@ -0,0 +1,45 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { vi } from 'vitest'; + +/** + * A fake implementation of PersistentState for testing. + * It keeps state in memory and provides spies for get and set. + */ +export class FakePersistentState { + private data: Record = {}; + + get = vi.fn().mockImplementation((key: string) => this.data[key]); + + set = vi.fn().mockImplementation((key: string, value: unknown) => { + this.data[key] = value; + }); + + /** + * Helper to reset the fake state between tests. + */ + reset() { + this.data = {}; + this.get.mockClear(); + this.set.mockClear(); + } + + /** + * Helper to clear mock call history without wiping data. + */ + mockClear() { + this.get.mockClear(); + this.set.mockClear(); + } + + /** + * Helper to set initial data for the fake. + */ + setData(data: Record) { + this.data = { ...data }; + } +} diff --git a/packages/cli/src/test-utils/render.tsx b/packages/cli/src/test-utils/render.tsx index 7472d89c3c..54e528deaf 100644 --- a/packages/cli/src/test-utils/render.tsx +++ b/packages/cli/src/test-utils/render.tsx @@ -28,6 +28,13 @@ import { type HistoryItemToolGroup, StreamingState } from '../ui/types.js'; import { ToolActionsProvider } from '../ui/contexts/ToolActionsContext.js'; import { type Config } from '@google/gemini-cli-core'; +import { FakePersistentState } from './persistentStateFake.js'; + +export const persistentStateMock = new FakePersistentState(); + +vi.mock('../utils/persistentState.js', () => ({ + persistentState: persistentStateMock, +})); // Wrapper around ink-testing-library's render that ensures act() is called export const render = ( @@ -191,6 +198,7 @@ export const renderWithProviders = ( config = configProxy as unknown as Config, useAlternateBuffer = true, uiActions, + persistentState, }: { shellFocus?: boolean; settings?: LoadedSettings; @@ -200,6 +208,10 @@ export const renderWithProviders = ( config?: Config; useAlternateBuffer?: boolean; uiActions?: Partial; + persistentState?: { + get?: typeof persistentStateMock.get; + set?: typeof persistentStateMock.set; + }; } = {}, ): ReturnType & { simulateClick: typeof simulateClick } => { const baseState: UIState = new Proxy( @@ -220,6 +232,15 @@ export const renderWithProviders = ( }, ) as UIState; + if (persistentState?.get) { + persistentStateMock.get.mockImplementation(persistentState.get); + } + if (persistentState?.set) { + persistentStateMock.set.mockImplementation(persistentState.set); + } + + persistentStateMock.mockClear(); + const terminalWidth = width ?? baseState.terminalWidth; let finalSettings = settings; if (useAlternateBuffer !== undefined) { diff --git a/packages/cli/src/ui/components/AlternateBufferQuittingDisplay.test.tsx b/packages/cli/src/ui/components/AlternateBufferQuittingDisplay.test.tsx index 0863e50286..521c088ea2 100644 --- a/packages/cli/src/ui/components/AlternateBufferQuittingDisplay.test.tsx +++ b/packages/cli/src/ui/components/AlternateBufferQuittingDisplay.test.tsx @@ -4,12 +4,15 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect, vi } from 'vitest'; +import { + renderWithProviders, + persistentStateMock, +} from '../../test-utils/render.js'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { AlternateBufferQuittingDisplay } from './AlternateBufferQuittingDisplay.js'; import { ToolCallStatus } from '../types.js'; import type { HistoryItem, HistoryItemWithoutId } from '../types.js'; import { Text } from 'ink'; -import { renderWithProviders } from '../../test-utils/render.js'; import type { Config } from '@google/gemini-cli-core'; vi.mock('../utils/terminalSetup.js', () => ({ @@ -98,6 +101,9 @@ const mockConfig = { } as unknown as Config; describe('AlternateBufferQuittingDisplay', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); const baseUIState = { terminalWidth: 80, mainAreaWidth: 80, @@ -112,6 +118,7 @@ describe('AlternateBufferQuittingDisplay', () => { }; it('renders with active and pending tool messages', () => { + persistentStateMock.setData({ tipsShown: 0 }); const { lastFrame } = renderWithProviders( , { @@ -127,6 +134,7 @@ describe('AlternateBufferQuittingDisplay', () => { }); it('renders with empty history and no pending items', () => { + persistentStateMock.setData({ tipsShown: 0 }); const { lastFrame } = renderWithProviders( , { @@ -142,6 +150,7 @@ describe('AlternateBufferQuittingDisplay', () => { }); it('renders with history but no pending items', () => { + persistentStateMock.setData({ tipsShown: 0 }); const { lastFrame } = renderWithProviders( , { @@ -157,6 +166,7 @@ describe('AlternateBufferQuittingDisplay', () => { }); it('renders with pending items but no history', () => { + persistentStateMock.setData({ tipsShown: 0 }); const { lastFrame } = renderWithProviders( , { @@ -172,6 +182,7 @@ describe('AlternateBufferQuittingDisplay', () => { }); it('renders with user and gemini messages', () => { + persistentStateMock.setData({ tipsShown: 0 }); const history: HistoryItem[] = [ { id: 1, type: 'user', text: 'Hello Gemini' }, { id: 2, type: 'gemini', text: 'Hello User!' }, diff --git a/packages/cli/src/ui/components/AppHeader.test.tsx b/packages/cli/src/ui/components/AppHeader.test.tsx index 74388c816a..ba276533ca 100644 --- a/packages/cli/src/ui/components/AppHeader.test.tsx +++ b/packages/cli/src/ui/components/AppHeader.test.tsx @@ -4,31 +4,20 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { renderWithProviders } from '../../test-utils/render.js'; +import { + renderWithProviders, + persistentStateMock, +} from '../../test-utils/render.js'; import { AppHeader } from './AppHeader.js'; -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; import { makeFakeConfig } from '@google/gemini-cli-core'; import crypto from 'node:crypto'; -const persistentStateMock = vi.hoisted(() => ({ - get: vi.fn(), - set: vi.fn(), -})); - -vi.mock('../../utils/persistentState.js', () => ({ - persistentState: persistentStateMock, -})); - vi.mock('../utils/terminalSetup.js', () => ({ getTerminalProgram: () => null, })); describe('', () => { - beforeEach(() => { - vi.clearAllMocks(); - persistentStateMock.get.mockReturnValue({}); - }); - it('should render the banner with default text', () => { const mockConfig = makeFakeConfig(); const uiState = { @@ -42,7 +31,10 @@ describe('', () => { const { lastFrame, unmount } = renderWithProviders( , - { config: mockConfig, uiState }, + { + config: mockConfig, + uiState, + }, ); expect(lastFrame()).toContain('This is the default banner'); @@ -63,7 +55,10 @@ describe('', () => { const { lastFrame, unmount } = renderWithProviders( , - { config: mockConfig, uiState }, + { + config: mockConfig, + uiState, + }, ); expect(lastFrame()).toContain('There are capacity issues'); @@ -83,7 +78,10 @@ describe('', () => { const { lastFrame, unmount } = renderWithProviders( , - { config: mockConfig, uiState }, + { + config: mockConfig, + uiState, + }, ); expect(lastFrame()).not.toContain('Banner'); @@ -104,7 +102,10 @@ describe('', () => { const { lastFrame, unmount } = renderWithProviders( , - { config: mockConfig, uiState }, + { + config: mockConfig, + uiState, + }, ); expect(lastFrame()).toContain('This is the default banner'); @@ -124,7 +125,10 @@ describe('', () => { const { lastFrame, unmount } = renderWithProviders( , - { config: mockConfig, uiState }, + { + config: mockConfig, + uiState, + }, ); expect(lastFrame()).not.toContain('This is the default banner'); @@ -133,7 +137,6 @@ describe('', () => { }); it('should not render the default banner if shown count is 5 or more', () => { - persistentStateMock.get.mockReturnValue(5); const mockConfig = makeFakeConfig(); const uiState = { history: [], @@ -143,9 +146,21 @@ describe('', () => { }, }; + persistentStateMock.setData({ + defaultBannerShownCount: { + [crypto + .createHash('sha256') + .update(uiState.bannerData.defaultText) + .digest('hex')]: 5, + }, + }); + const { lastFrame, unmount } = renderWithProviders( , - { config: mockConfig, uiState }, + { + config: mockConfig, + uiState, + }, ); expect(lastFrame()).not.toContain('This is the default banner'); @@ -154,7 +169,6 @@ describe('', () => { }); it('should increment the version count when default banner is displayed', () => { - persistentStateMock.get.mockReturnValue({}); const mockConfig = makeFakeConfig(); const uiState = { history: [], @@ -164,6 +178,10 @@ describe('', () => { }, }; + // Set tipsShown to 10 or more to prevent Tips from incrementing its count + // and interfering with the expected persistentState.set call. + persistentStateMock.setData({ tipsShown: 10 }); + const { unmount } = renderWithProviders(, { config: mockConfig, uiState, @@ -194,10 +212,87 @@ describe('', () => { const { lastFrame, unmount } = renderWithProviders( , - { config: mockConfig, uiState }, + { + config: mockConfig, + uiState, + }, ); expect(lastFrame()).not.toContain('First line\\nSecond line'); unmount(); }); + + it('should render Tips when tipsShown is less than 10', () => { + const mockConfig = makeFakeConfig(); + const uiState = { + history: [], + bannerData: { + defaultText: 'First line\\nSecond line', + warningText: '', + }, + bannerVisible: true, + }; + + persistentStateMock.setData({ tipsShown: 5 }); + + const { lastFrame, unmount } = renderWithProviders( + , + { + config: mockConfig, + uiState, + }, + ); + + expect(lastFrame()).toContain('Tips'); + expect(persistentStateMock.set).toHaveBeenCalledWith('tipsShown', 6); + unmount(); + }); + + it('should NOT render Tips when tipsShown is 10 or more', () => { + const mockConfig = makeFakeConfig(); + + persistentStateMock.setData({ tipsShown: 10 }); + + const { lastFrame, unmount } = renderWithProviders( + , + { + config: mockConfig, + }, + ); + + expect(lastFrame()).not.toContain('Tips'); + unmount(); + }); + + it('should show tips until they have been shown 10 times (persistence flow)', () => { + persistentStateMock.setData({ tipsShown: 9 }); + + const mockConfig = makeFakeConfig(); + const uiState = { + history: [], + bannerData: { + defaultText: 'First line\\nSecond line', + warningText: '', + }, + bannerVisible: true, + }; + + // First session + const session1 = renderWithProviders(, { + config: mockConfig, + uiState, + }); + + expect(session1.lastFrame()).toContain('Tips'); + expect(persistentStateMock.get('tipsShown')).toBe(10); + session1.unmount(); + + // Second session - state is persisted in the fake + const session2 = renderWithProviders(, { + config: mockConfig, + }); + + expect(session2.lastFrame()).not.toContain('Tips'); + session2.unmount(); + }); }); diff --git a/packages/cli/src/ui/components/AppHeader.tsx b/packages/cli/src/ui/components/AppHeader.tsx index a70a7b20d8..5efe1ed81f 100644 --- a/packages/cli/src/ui/components/AppHeader.tsx +++ b/packages/cli/src/ui/components/AppHeader.tsx @@ -12,6 +12,7 @@ import { useConfig } from '../contexts/ConfigContext.js'; import { useUIState } from '../contexts/UIStateContext.js'; import { Banner } from './Banner.js'; import { useBanner } from '../hooks/useBanner.js'; +import { useTips } from '../hooks/useTips.js'; interface AppHeaderProps { version: string; @@ -23,6 +24,7 @@ export const AppHeader = ({ version }: AppHeaderProps) => { const { nightly, mainAreaWidth, bannerData, bannerVisible } = useUIState(); const { bannerText } = useBanner(bannerData, config); + const { showTips } = useTips(); return ( @@ -38,9 +40,8 @@ export const AppHeader = ({ version }: AppHeaderProps) => { )} )} - {!(settings.merged.ui.hideTips || config.getScreenReader()) && ( - - )} + {!(settings.merged.ui.hideTips || config.getScreenReader()) && + showTips && } ); }; diff --git a/packages/cli/src/ui/components/Notifications.test.tsx b/packages/cli/src/ui/components/Notifications.test.tsx index 0e04799cba..df35ac02ab 100644 --- a/packages/cli/src/ui/components/Notifications.test.tsx +++ b/packages/cli/src/ui/components/Notifications.test.tsx @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { render } from '../../test-utils/render.js'; +import { render, persistentStateMock } from '../../test-utils/render.js'; import { Notifications } from './Notifications.js'; import { describe, it, expect, vi, beforeEach } from 'vitest'; import { useAppContext, type AppState } from '../contexts/AppContext.js'; @@ -30,6 +30,7 @@ vi.mock('node:fs/promises', async () => { access: vi.fn(), writeFile: vi.fn(), mkdir: vi.fn().mockResolvedValue(undefined), + unlink: vi.fn().mockResolvedValue(undefined), }; }); vi.mock('node:os', () => ({ @@ -68,10 +69,11 @@ describe('Notifications', () => { const mockUseUIState = vi.mocked(useUIState); const mockUseIsScreenReaderEnabled = vi.mocked(useIsScreenReaderEnabled); const mockFsAccess = vi.mocked(fs.access); - const mockFsWriteFile = vi.mocked(fs.writeFile); + const mockFsUnlink = vi.mocked(fs.unlink); beforeEach(() => { vi.clearAllMocks(); + persistentStateMock.reset(); mockUseAppContext.mockReturnValue({ startupWarnings: [], version: '1.0.0', @@ -134,51 +136,47 @@ describe('Notifications', () => { expect(lastFrame()).toMatchSnapshot(); }); - it('renders screen reader nudge when enabled and not seen', async () => { + it('renders screen reader nudge when enabled and not seen (no legacy file)', async () => { mockUseIsScreenReaderEnabled.mockReturnValue(true); - - let rejectAccess: (err: Error) => void; - mockFsAccess.mockImplementation( - () => - new Promise((_, reject) => { - rejectAccess = reject; - }), - ); + persistentStateMock.setData({ hasSeenScreenReaderNudge: false }); + mockFsAccess.mockRejectedValue(new Error('No legacy file')); const { lastFrame } = render(); - // Trigger rejection inside act - await act(async () => { - rejectAccess(new Error('File not found')); - }); - - // Wait for effect to propagate - await vi.waitFor(() => { - expect(mockFsWriteFile).toHaveBeenCalled(); - }); + expect(lastFrame()).toContain('screen reader-friendly view'); + expect(persistentStateMock.set).toHaveBeenCalledWith( + 'hasSeenScreenReaderNudge', + true, + ); expect(lastFrame()).toMatchSnapshot(); }); - it('does not render screen reader nudge when already seen', async () => { + it('migrates legacy screen reader nudge file', async () => { mockUseIsScreenReaderEnabled.mockReturnValue(true); + persistentStateMock.setData({ hasSeenScreenReaderNudge: undefined }); + mockFsAccess.mockResolvedValue(undefined); - let resolveAccess: (val: undefined) => void; - mockFsAccess.mockImplementation( - () => - new Promise((resolve) => { - resolveAccess = resolve; - }), - ); + render(); + + await act(async () => { + await vi.waitFor(() => { + expect(persistentStateMock.set).toHaveBeenCalledWith( + 'hasSeenScreenReaderNudge', + true, + ); + expect(mockFsUnlink).toHaveBeenCalled(); + }); + }); + }); + + it('does not render screen reader nudge when already seen in persistent state', async () => { + mockUseIsScreenReaderEnabled.mockReturnValue(true); + persistentStateMock.setData({ hasSeenScreenReaderNudge: true }); const { lastFrame } = render(); - // Trigger resolution inside act - await act(async () => { - resolveAccess(undefined); - }); - expect(lastFrame()).toBe(''); - expect(mockFsWriteFile).not.toHaveBeenCalled(); + expect(persistentStateMock.set).not.toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/ui/components/Notifications.tsx b/packages/cli/src/ui/components/Notifications.tsx index 460d03f88b..c252dd12de 100644 --- a/packages/cli/src/ui/components/Notifications.tsx +++ b/packages/cli/src/ui/components/Notifications.tsx @@ -11,13 +11,9 @@ import { useUIState } from '../contexts/UIStateContext.js'; import { theme } from '../semantic-colors.js'; import { StreamingState } from '../types.js'; import { UpdateNotification } from './UpdateNotification.js'; +import { persistentState } from '../../utils/persistentState.js'; -import { - GEMINI_DIR, - Storage, - debugLogger, - homedir, -} from '@google/gemini-cli-core'; +import { GEMINI_DIR, Storage, homedir } from '@google/gemini-cli-core'; import * as fs from 'node:fs/promises'; import path from 'node:path'; @@ -38,15 +34,20 @@ export const Notifications = () => { const showInitError = initError && streamingState !== StreamingState.Responding; - const [hasSeenScreenReaderNudge, setHasSeenScreenReaderNudge] = useState< - boolean | undefined - >(undefined); + const [hasSeenScreenReaderNudge, setHasSeenScreenReaderNudge] = useState(() => + persistentState.get('hasSeenScreenReaderNudge'), + ); useEffect(() => { - const checkScreenReader = async () => { + const checkLegacyScreenReaderNudge = async () => { + if (hasSeenScreenReaderNudge !== undefined) return; + try { await fs.access(screenReaderNudgeFilePath); + persistentState.set('hasSeenScreenReaderNudge', true); setHasSeenScreenReaderNudge(true); + // Best effort cleanup of legacy file + await fs.unlink(screenReaderNudgeFilePath).catch(() => {}); } catch { setHasSeenScreenReaderNudge(false); } @@ -54,28 +55,17 @@ export const Notifications = () => { if (isScreenReaderEnabled) { // eslint-disable-next-line @typescript-eslint/no-floating-promises - checkScreenReader(); + checkLegacyScreenReaderNudge(); } - }, [isScreenReaderEnabled]); + }, [isScreenReaderEnabled, hasSeenScreenReaderNudge]); const showScreenReaderNudge = isScreenReaderEnabled && hasSeenScreenReaderNudge === false; useEffect(() => { - const writeScreenReaderNudgeFile = async () => { - if (showScreenReaderNudge) { - try { - await fs.mkdir(path.dirname(screenReaderNudgeFilePath), { - recursive: true, - }); - await fs.writeFile(screenReaderNudgeFilePath, 'true'); - } catch (error) { - debugLogger.error('Error storing screen reader nudge', error); - } - } - }; - // eslint-disable-next-line @typescript-eslint/no-floating-promises - writeScreenReaderNudgeFile(); + if (showScreenReaderNudge) { + persistentState.set('hasSeenScreenReaderNudge', true); + } }, [showScreenReaderNudge]); if ( diff --git a/packages/cli/src/ui/components/__snapshots__/Notifications.test.tsx.snap b/packages/cli/src/ui/components/__snapshots__/Notifications.test.tsx.snap index 32704a9313..46e269fea4 100644 --- a/packages/cli/src/ui/components/__snapshots__/Notifications.test.tsx.snap +++ b/packages/cli/src/ui/components/__snapshots__/Notifications.test.tsx.snap @@ -7,7 +7,7 @@ exports[`Notifications > renders init error 1`] = ` " `; -exports[`Notifications > renders screen reader nudge when enabled and not seen 1`] = ` +exports[`Notifications > renders screen reader nudge when enabled and not seen (no legacy file) 1`] = ` "You are currently in screen reader-friendly view. To switch out, open /mock/home/.gemini/settings.json and remove the entry for "screenReader". This will disappear on next run." diff --git a/packages/cli/src/ui/hooks/useTips.test.ts b/packages/cli/src/ui/hooks/useTips.test.ts new file mode 100644 index 0000000000..bb15204e0c --- /dev/null +++ b/packages/cli/src/ui/hooks/useTips.test.ts @@ -0,0 +1,47 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + renderHookWithProviders, + persistentStateMock, +} from '../../test-utils/render.js'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { useTips } from './useTips.js'; + +describe('useTips()', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should return false and call set(1) if state is undefined', () => { + const { result } = renderHookWithProviders(() => useTips()); + + expect(result.current.showTips).toBe(true); + + expect(persistentStateMock.set).toHaveBeenCalledWith('tipsShown', 1); + expect(persistentStateMock.get('tipsShown')).toBe(1); + }); + + it('should return false and call set(6) if state is 5', () => { + persistentStateMock.setData({ tipsShown: 5 }); + + const { result } = renderHookWithProviders(() => useTips()); + + expect(result.current.showTips).toBe(true); + + expect(persistentStateMock.get('tipsShown')).toBe(6); + }); + + it('should return true if state is 10', () => { + persistentStateMock.setData({ tipsShown: 10 }); + + const { result } = renderHookWithProviders(() => useTips()); + + expect(result.current.showTips).toBe(false); + expect(persistentStateMock.set).not.toHaveBeenCalled(); + expect(persistentStateMock.get('tipsShown')).toBe(10); + }); +}); diff --git a/packages/cli/src/ui/hooks/useTips.ts b/packages/cli/src/ui/hooks/useTips.ts new file mode 100644 index 0000000000..75fe8bb096 --- /dev/null +++ b/packages/cli/src/ui/hooks/useTips.ts @@ -0,0 +1,26 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useEffect, useState } from 'react'; +import { persistentState } from '../../utils/persistentState.js'; + +interface UseTipsResult { + showTips: boolean; +} + +export function useTips(): UseTipsResult { + const [tipsCount] = useState(() => persistentState.get('tipsShown') ?? 0); + + const showTips = tipsCount < 10; + + useEffect(() => { + if (showTips) { + persistentState.set('tipsShown', tipsCount + 1); + } + }, [tipsCount, showTips]); + + return { showTips }; +} diff --git a/packages/cli/src/utils/persistentState.ts b/packages/cli/src/utils/persistentState.ts index f5fc4d4b29..b849703f53 100644 --- a/packages/cli/src/utils/persistentState.ts +++ b/packages/cli/src/utils/persistentState.ts @@ -12,6 +12,8 @@ const STATE_FILENAME = 'state.json'; interface PersistentStateData { defaultBannerShownCount?: Record; + tipsShown?: number; + hasSeenScreenReaderNudge?: boolean; // Add other persistent state keys here as needed }