diff --git a/packages/cli/src/ui/components/AlternateBufferQuittingDisplay.test.tsx b/packages/cli/src/ui/components/AlternateBufferQuittingDisplay.test.tsx index 223a797b0b..aaf89c59e8 100644 --- a/packages/cli/src/ui/components/AlternateBufferQuittingDisplay.test.tsx +++ b/packages/cli/src/ui/components/AlternateBufferQuittingDisplay.test.tsx @@ -15,7 +15,7 @@ import type { Config } from '@google/gemini-cli-core'; vi.mock('../../utils/persistentState.js', () => ({ persistentState: { get: vi.fn().mockImplementation((key) => { - if (key === 'tipsShown') return false; + if (key === 'tipsShown') return 0; if (key === 'defaultBannerShownCount') return {}; return undefined; }), diff --git a/packages/cli/src/ui/components/AppHeader.test.tsx b/packages/cli/src/ui/components/AppHeader.test.tsx index 2648ac57ca..2cf0c91cf4 100644 --- a/packages/cli/src/ui/components/AppHeader.test.tsx +++ b/packages/cli/src/ui/components/AppHeader.test.tsx @@ -133,16 +133,7 @@ describe('', () => { }); it('should not render the default banner if shown count is 5 or more', () => { - persistentStateMock.get.mockImplementation((key) => { - if (key === 'defaultBannerShownCount') { - const hash = crypto - .createHash('sha256') - .update('This is the default banner') - .digest('hex'); - return { [hash]: 5 }; - } - return undefined; - }); + persistentStateMock.get.mockReturnValue(5); const mockConfig = makeFakeConfig(); const uiState = { @@ -211,10 +202,10 @@ describe('', () => { unmount(); }); - it('should render Tips when tipsShown is false', () => { + it('should render Tips when tipsShown is less than 10', () => { persistentStateMock.get.mockImplementation((key) => { - if (key === 'tipsShown') return false; - return {}; + if (key === 'tipsShown') return 5; + return undefined; }); const mockConfig = makeFakeConfig(); @@ -232,15 +223,14 @@ describe('', () => { ); expect(lastFrame()).toContain('Tips'); - - expect(persistentStateMock.set).toHaveBeenCalledWith('tipsShown', true); + expect(persistentStateMock.set).toHaveBeenCalledWith('tipsShown', 6); unmount(); }); - it('should NOT render Tips when tipsShown is true', () => { + it('should NOT render Tips when tipsShown is 10 or more', () => { persistentStateMock.get.mockImplementation((key) => { - if (key === 'tipsShown') return true; - return {}; + if (key === 'tipsShown') return 10; + return undefined; }); const mockConfig = makeFakeConfig(); @@ -253,8 +243,10 @@ describe('', () => { unmount(); }); - it('should show tips on the first run and hide them on the second run (persistence flow)', () => { - const fakeStore: Record = {}; + it('should show tips until they have been shown 10 times (persistence flow)', () => { + const fakeStore: Record = { + tipsShown: 9, + }; persistentStateMock.get.mockImplementation((key) => fakeStore[key]); persistentStateMock.set.mockImplementation((key, val) => { @@ -276,9 +268,7 @@ describe('', () => { }); expect(session1.lastFrame()).toContain('Tips'); - - expect(fakeStore['tipsShown']).toBe(true); - + expect(fakeStore['tipsShown']).toBe(10); session1.unmount(); const session2 = renderWithProviders(, { @@ -286,7 +276,6 @@ describe('', () => { }); expect(session2.lastFrame()).not.toContain('Tips'); - session2.unmount(); }); }); diff --git a/packages/cli/src/ui/components/Notifications.test.tsx b/packages/cli/src/ui/components/Notifications.test.tsx index 0e04799cba..c28a2602cc 100644 --- a/packages/cli/src/ui/components/Notifications.test.tsx +++ b/packages/cli/src/ui/components/Notifications.test.tsx @@ -12,10 +12,17 @@ import { useUIState, type UIState } from '../contexts/UIStateContext.js'; import { useIsScreenReaderEnabled } from 'ink'; import * as fs from 'node:fs/promises'; import { act } from 'react'; +import { persistentState } from '../../utils/persistentState.js'; // Mock dependencies vi.mock('../contexts/AppContext.js'); vi.mock('../contexts/UIStateContext.js'); +vi.mock('../../utils/persistentState.js', () => ({ + persistentState: { + get: vi.fn(), + set: vi.fn(), + }, +})); vi.mock('ink', async () => { const actual = await vi.importActual('ink'); return { @@ -30,6 +37,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,7 +76,9 @@ 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); + const mockPersistentStateGet = vi.mocked(persistentState.get); + const mockPersistentStateSet = vi.mocked(persistentState.set); beforeEach(() => { vi.clearAllMocks(); @@ -82,6 +92,7 @@ describe('Notifications', () => { updateInfo: null, } as unknown as UIState); mockUseIsScreenReaderEnabled.mockReturnValue(false); + mockPersistentStateGet.mockReturnValue(undefined); }); it('renders nothing when no notifications', () => { @@ -134,51 +145,45 @@ 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; - }), - ); + mockPersistentStateGet.mockReturnValue(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()).toMatchSnapshot(); + expect(lastFrame()).toContain('screen reader-friendly view'); + expect(mockPersistentStateSet).toHaveBeenCalledWith( + 'hasSeenScreenReaderNudge', + true, + ); }); - it('does not render screen reader nudge when already seen', async () => { + it('migrates legacy screen reader nudge file', async () => { mockUseIsScreenReaderEnabled.mockReturnValue(true); + mockPersistentStateGet.mockReturnValue(undefined); + mockFsAccess.mockResolvedValue(undefined); - let resolveAccess: (val: undefined) => void; - mockFsAccess.mockImplementation( - () => - new Promise((resolve) => { - resolveAccess = resolve; - }), - ); + render(); + + await act(async () => { + await vi.waitFor(() => { + expect(mockPersistentStateSet).toHaveBeenCalledWith( + 'hasSeenScreenReaderNudge', + true, + ); + expect(mockFsUnlink).toHaveBeenCalled(); + }); + }); + }); + + it('does not render screen reader nudge when already seen in persistent state', async () => { + mockUseIsScreenReaderEnabled.mockReturnValue(true); + mockPersistentStateGet.mockReturnValue(true); const { lastFrame } = render(); - // Trigger resolution inside act - await act(async () => { - resolveAccess(undefined); - }); - expect(lastFrame()).toBe(''); - expect(mockFsWriteFile).not.toHaveBeenCalled(); + expect(mockPersistentStateSet).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..c83d739429 100644 --- a/packages/cli/src/ui/components/__snapshots__/Notifications.test.tsx.snap +++ b/packages/cli/src/ui/components/__snapshots__/Notifications.test.tsx.snap @@ -7,12 +7,6 @@ exports[`Notifications > renders init error 1`] = ` " `; -exports[`Notifications > renders screen reader nudge when enabled and not seen 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." -`; - exports[`Notifications > renders update notification 1`] = ` " ╭──────────────────────────────────────────────────────────────────────────────────────────────────╮ diff --git a/packages/cli/src/ui/hooks/useTips.test.ts b/packages/cli/src/ui/hooks/useTips.test.ts index 0270f73864..c15512435b 100644 --- a/packages/cli/src/ui/hooks/useTips.test.ts +++ b/packages/cli/src/ui/hooks/useTips.test.ts @@ -21,18 +21,28 @@ describe('useTips()', () => { vi.clearAllMocks(); }); - it('should return false and call set(true) if state is undefined', () => { + it('should return false and call set(1) if state is undefined', () => { vi.mocked(persistentState.get).mockReturnValue(undefined); const { result } = renderHookWithProviders(() => useTips()); expect(result.current).toBe(false); - expect(persistentState.set).toHaveBeenCalledWith('tipsShown', true); + expect(persistentState.set).toHaveBeenCalledWith('tipsShown', 1); }); - it('should return true if state is already true', () => { - vi.mocked(persistentState.get).mockReturnValue(true); + it('should return false and call set(6) if state is 5', () => { + vi.mocked(persistentState.get).mockReturnValue(5); + + const { result } = renderHookWithProviders(() => useTips()); + + expect(result.current).toBe(false); + + expect(persistentState.set).toHaveBeenCalledWith('tipsShown', 6); + }); + + it('should return true if state is 10', () => { + vi.mocked(persistentState.get).mockReturnValue(10); const { result } = renderHookWithProviders(() => useTips()); diff --git a/packages/cli/src/ui/hooks/useTips.ts b/packages/cli/src/ui/hooks/useTips.ts index d28df9ddb8..8f43766b7f 100644 --- a/packages/cli/src/ui/hooks/useTips.ts +++ b/packages/cli/src/ui/hooks/useTips.ts @@ -8,13 +8,15 @@ import { useEffect, useState } from 'react'; import { persistentState } from '../../utils/persistentState.js'; export function useTips() { - const [tipsShown] = useState(() => !!persistentState.get('tipsShown')); + const [tipsCount] = useState(() => persistentState.get('tipsShown') ?? 0); + + const tipsHidden = tipsCount >= 10; useEffect(() => { - if (!tipsShown) { - persistentState.set('tipsShown', true); + if (!tipsHidden) { + persistentState.set('tipsShown', tipsCount + 1); } - }, [tipsShown]); + }, [tipsCount, tipsHidden]); - return tipsShown; + return tipsHidden; } diff --git a/packages/cli/src/utils/persistentState.ts b/packages/cli/src/utils/persistentState.ts index f565157f69..b849703f53 100644 --- a/packages/cli/src/utils/persistentState.ts +++ b/packages/cli/src/utils/persistentState.ts @@ -12,7 +12,8 @@ const STATE_FILENAME = 'state.json'; interface PersistentStateData { defaultBannerShownCount?: Record; - tipsShown?: boolean; + tipsShown?: number; + hasSeenScreenReaderNudge?: boolean; // Add other persistent state keys here as needed }