address feedback from Jacob

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

packages/cli/src/ui/components/AppHeader.test.tsx

@@ -133,16 +133,7 @@ describe('<AppHeader />', () => {
   });
 
   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('<AppHeader />', () => {
     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('<AppHeader />', () => {
     );
 
     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('<AppHeader />', () => {
     unmount();
   });
 
-  it('should show tips on the first run and hide them on the second run (persistence flow)', () => {
-    const fakeStore: Record<string, boolean> = {};
+  it('should show tips until they have been shown 10 times (persistence flow)', () => {
+    const fakeStore: Record<string, number> = {
+      tipsShown: 9,
+    };
 
     persistentStateMock.get.mockImplementation((key) => fakeStore[key]);
     persistentStateMock.set.mockImplementation((key, val) => {
@@ -276,9 +268,7 @@ describe('<AppHeader />', () => {
     });
 
     expect(session1.lastFrame()).toContain('Tips');
-
-    expect(fakeStore['tipsShown']).toBe(true);
-
+    expect(fakeStore['tipsShown']).toBe(10);
     session1.unmount();
 
     const session2 = renderWithProviders(<AppHeader version="1.0.0" />, {
@@ -286,7 +276,6 @@ describe('<AppHeader />', () => {
     });
 
     expect(session2.lastFrame()).not.toContain('Tips');
-
     session2.unmount();
   });
 });

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(<Notifications />);
 
-    // 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(<Notifications />);
+
+    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(<Notifications />);
 
-    // Trigger resolution inside act
-    await act(async () => {
-      resolveAccess(undefined);
-    });
-
     expect(lastFrame()).toBe('');
-    expect(mockFsWriteFile).not.toHaveBeenCalled();
+    expect(mockPersistentStateSet).not.toHaveBeenCalled();
   });
 });

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 (

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`] = `
 "
 ╭──────────────────────────────────────────────────────────────────────────────────────────────────╮

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

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;
 }

packages/cli/src/utils/persistentState.ts

@@ -12,7 +12,8 @@ const STATE_FILENAME = 'state.json';
 
 interface PersistentStateData {
   defaultBannerShownCount?: Record<string, number>;
-  tipsShown?: boolean;
+  tipsShown?: number;
+  hasSeenScreenReaderNudge?: boolean;
   // Add other persistent state keys here as needed
 }