From d516ea5aa49f6dd8b046ca6578fe015a03ff7ee6 Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Sun, 13 Jul 2025 07:04:19 +0800 Subject: [PATCH 01/20] feat: Add session persistence and fix serialization issues --- packages/cli/src/config/settings.ts | 1 + packages/cli/src/ui/App.tsx | 10 +++++++--- packages/cli/src/ui/types.ts | 1 + 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/config/settings.ts b/packages/cli/src/config/settings.ts index 3cbfe22dcb0..b78ebca3aab 100644 --- a/packages/cli/src/config/settings.ts +++ b/packages/cli/src/config/settings.ts @@ -99,6 +99,7 @@ export interface Settings { // Add other settings here. ideMode?: boolean; + 'session.persistence'?: boolean; } export interface SettingsError { diff --git a/packages/cli/src/ui/App.tsx b/packages/cli/src/ui/App.tsx index 027665f1594..b9602b9c5f3 100644 --- a/packages/cli/src/ui/App.tsx +++ b/packages/cli/src/ui/App.tsx @@ -47,7 +47,9 @@ import { DetailedMessagesDisplay } from './components/DetailedMessagesDisplay.js import { HistoryItemDisplay } from './components/HistoryItemDisplay.js'; import { ContextSummaryDisplay } from './components/ContextSummaryDisplay.js'; import { useHistory } from './hooks/useHistoryManager.js'; +import { useSessionPersistence } from './hooks/useSessionPersistence.js'; import process from 'node:process'; +import * as path from 'path'; import { getErrorMessage, type Config, @@ -112,6 +114,8 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { }, []); const { history, addItem, clearItems, loadHistory } = useHistory(); + const sessionPersistence = settings.merged['session.persistence']; + useSessionPersistence({ sessionPersistence, history, loadHistory }); const { consoleMessages, handleNewMessage, @@ -686,7 +690,7 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { if (quittingMessages) { return ( - {quittingMessages.map((item) => ( + {quittingMessages.map((item: HistoryItem) => ( { )} {!settings.merged.hideTips && } , - ...history.map((h) => ( + ...history.map((h: HistoryItem) => ( { )), ]} > - {(item) => item} + {(item: React.ReactNode) => item} diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index 223ccd472bd..bab2e877e50 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -163,6 +163,7 @@ export enum MessageType { QUIT = 'quit', GEMINI = 'gemini', COMPRESSION = 'compression', + TOOL_GROUP = 'tool_group', } // Simplified message structure for internal feedback From c13a7843c77cbec9b2259acadc0ef3e28e17fe2f Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Mon, 14 Jul 2025 05:51:33 +0800 Subject: [PATCH 02/20] feat(cli): Refactor session persistence into a dedicated hook Extracts session persistence logic from App.tsx into a new hook. Adds comprehensive unit tests for the new hook. --- packages/cli/src/ui/App.tsx | 2 +- .../ui/hooks/useSessionPersistence.test.ts | 239 ++++++++++++++++++ .../cli/src/ui/hooks/useSessionPersistence.ts | 77 ++++++ 3 files changed, 317 insertions(+), 1 deletion(-) create mode 100644 packages/cli/src/ui/hooks/useSessionPersistence.test.ts create mode 100644 packages/cli/src/ui/hooks/useSessionPersistence.ts diff --git a/packages/cli/src/ui/App.tsx b/packages/cli/src/ui/App.tsx index b9602b9c5f3..4488e5ed8f5 100644 --- a/packages/cli/src/ui/App.tsx +++ b/packages/cli/src/ui/App.tsx @@ -49,7 +49,7 @@ import { ContextSummaryDisplay } from './components/ContextSummaryDisplay.js'; import { useHistory } from './hooks/useHistoryManager.js'; import { useSessionPersistence } from './hooks/useSessionPersistence.js'; import process from 'node:process'; -import * as path from 'path'; + import { getErrorMessage, type Config, diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts new file mode 100644 index 00000000000..6ab477b51f1 --- /dev/null +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -0,0 +1,239 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach, Mock } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; +import { useSessionPersistence } from './useSessionPersistence.js'; +import * as fs from 'fs'; +import * as path from 'path'; +import process from 'node:process'; +import { MessageType, HistoryItem } from '../types.js'; + +// Mock fs module +vi.mock('fs', async () => { + const actual = await vi.importActual('fs'); + return { + ...actual, + existsSync: vi.fn(), + readFileSync: vi.fn(), + writeFileSync: vi.fn(), + mkdirSync: vi.fn(), + }; +}); + +// Mock process.on and process.off for exit event +let mockProcessOn: Mock; +let mockProcessOff: Mock; + +describe('useSessionPersistence', () => { + let mockHistory: HistoryItem[]; + let mockLoadHistory: Mock; + + beforeEach(() => { + vi.useFakeTimers(); + mockHistory = []; + mockLoadHistory = vi.fn(); + + vi.mocked(fs.existsSync).mockReturnValue(false); + vi.mocked(fs.readFileSync).mockReturnValue(''); + vi.mocked(fs.writeFileSync).mockReturnValue(undefined); + vi.mocked(fs.mkdirSync).mockReturnValue(undefined); + + mockProcessOn = vi.spyOn(process, 'on'); + mockProcessOff = vi.spyOn(process, 'off'); + mockProcessOn.mockClear(); + mockProcessOff.mockClear(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + }); + + it('should register and unregister exit handler', () => { + const { unmount } = renderHook(() => + useSessionPersistence({ + sessionPersistence: true, + history: mockHistory, + loadHistory: mockLoadHistory, + }), + ); + + expect(mockProcessOn).toHaveBeenCalledWith('exit', expect.any(Function)); + + unmount(); + + expect(mockProcessOff).toHaveBeenCalledWith('exit', expect.any(Function)); + }); + + it('should save session history on exit when persistence is enabled', async () => { + const { unmount } = renderHook(() => + useSessionPersistence({ + sessionPersistence: true, + history: mockHistory, + loadHistory: mockLoadHistory, + }), + ); + + mockHistory.push( + { type: MessageType.USER, text: 'User message 1' }, + { type: MessageType.GEMINI, text: 'Gemini response 1' }, + { type: MessageType.INFO, text: 'Info message' }, // Should be filtered + ); + + // Manually trigger the exit handler + const exitHandler = mockProcessOn.mock.calls.find( + (call) => call[0] === 'exit', + )?.[1]; + + expect(exitHandler).toBeDefined(); + + await act(async () => { + exitHandler(); + }); + + const expectedPath = path.join(process.cwd(), '.gemini', 'session.json'); + const expectedContent = JSON.stringify( + [ + { type: MessageType.USER, text: 'User message 1' }, + { type: MessageType.GEMINI, text: 'Gemini response 1' }, + ], + null, + 2, + ); + + expect(fs.mkdirSync).toHaveBeenCalledWith( + path.join(process.cwd(), '.gemini'), + { recursive: true }, + ); + expect(fs.writeFileSync).toHaveBeenCalledWith( + expectedPath, + expectedContent, + ); + + unmount(); + }); + + it('should load session history on startup when persistence is enabled and file exists', async () => { + const savedHistory = [ + { type: MessageType.USER, text: 'Loaded user message' }, + { type: MessageType.GEMINI, text: 'Loaded gemini response' }, + ]; + + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(savedHistory)); + + const { unmount } = renderHook(() => + useSessionPersistence({ + sessionPersistence: true, + history: mockHistory, + loadHistory: mockLoadHistory, + }), + ); + + await vi.runAllTimersAsync(); // Flush effects + + expect(fs.existsSync).toHaveBeenCalledWith( + path.join(process.cwd(), '.gemini', 'session.json'), + ); + expect(fs.readFileSync).toHaveBeenCalledWith( + path.join(process.cwd(), '.gemini', 'session.json'), + 'utf-8', + ); + expect(mockLoadHistory).toHaveBeenCalledWith(savedHistory); + + unmount(); + }); + + it('should not save or load session history when persistence is disabled', async () => { + const { unmount } = renderHook(() => + useSessionPersistence({ + sessionPersistence: false, + history: mockHistory, + loadHistory: mockLoadHistory, + }), + ); + + await vi.runAllTimersAsync(); // Flush effects + + expect(fs.existsSync).not.toHaveBeenCalled(); + expect(fs.readFileSync).not.toHaveBeenCalled(); + expect(fs.writeFileSync).not.toHaveBeenCalled(); + expect(fs.mkdirSync).not.toHaveBeenCalled(); + expect(mockProcessOn).not.toHaveBeenCalledWith( + 'exit', + expect.any(Function), + ); + + unmount(); + }); + + it('should log error if loading fails', async () => { + vi.mocked(fs.existsSync).mockReturnValue(true); + vi.mocked(fs.readFileSync).mockImplementation(() => { + throw new Error('Read error'); + }); + + const consoleErrorSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); + + const { unmount } = renderHook(() => + useSessionPersistence({ + sessionPersistence: true, + history: mockHistory, + loadHistory: mockLoadHistory, + }), + ); + + await vi.runAllTimersAsync(); // Flush effects + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error loading session history:', + expect.any(Error), + ); + expect(mockLoadHistory).not.toHaveBeenCalled(); + + consoleErrorSpy.mockRestore(); + unmount(); + }); + + it('should log error if saving fails', async () => { + vi.mocked(fs.existsSync).mockReturnValue(false); // Ensure mkdirSync is called + vi.mocked(fs.writeFileSync).mockImplementation(() => { + throw new Error('Write error'); + }); + + const consoleErrorSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); + + const { unmount } = renderHook(() => + useSessionPersistence({ + sessionPersistence: true, + history: mockHistory, + loadHistory: mockLoadHistory, + }), + ); + + mockHistory.push({ type: MessageType.USER, text: 'User message' }); + + const exitHandler = mockProcessOn.mock.calls.find( + (call) => call[0] === 'exit', + )?.[1]; + + expect(exitHandler).toBeDefined(); + exitHandler(); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error saving session history:', + expect.any(Error), + ); + + consoleErrorSpy.mockRestore(); + unmount(); + }); +}); diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts new file mode 100644 index 00000000000..d7bfd313c9a --- /dev/null +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -0,0 +1,77 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { useEffect } from 'react'; +import * as fs from 'fs'; +import * as path from 'path'; +import process from 'node:process'; +import { HistoryItem, MessageType } from '../types.js'; + +interface UseSessionPersistenceProps { + sessionPersistence: boolean | undefined; + history: HistoryItem[]; + loadHistory: (history: HistoryItem[]) => void; +} + +export const useSessionPersistence = ({ + sessionPersistence, + history, + loadHistory, +}: UseSessionPersistenceProps) => { + useEffect(() => { + if (sessionPersistence) { + const sessionPath = path.join(process.cwd(), '.gemini', 'session.json'); + if (fs.existsSync(sessionPath)) { + try { + const sessionData = fs.readFileSync(sessionPath, 'utf-8'); + loadHistory(JSON.parse(sessionData)); + } catch (error) { + console.error('Error loading session history:', error); + } + } + } + }, [sessionPersistence, loadHistory]); + + useEffect(() => { + if (!sessionPersistence) { + return; + } + + const saveSession = () => { + if (sessionPersistence) { + const geminiDir = path.join(process.cwd(), '.gemini'); + if (!fs.existsSync(geminiDir)) { + fs.mkdirSync(geminiDir, { recursive: true }); + } + const sessionPath = path.join(geminiDir, 'session.json'); + + // Create a serializable version of the history + const serializableHistory = history + .filter( + (item) => + item.type === MessageType.USER || + item.type === MessageType.GEMINI, + ) + .map((item) => ({ type: item.type, text: item.text })); + + try { + fs.writeFileSync( + sessionPath, + JSON.stringify(serializableHistory, null, 2), + ); + } catch (error) { + console.error('Error saving session history:', error); + } + } + }; + + process.on('exit', saveSession); + + return () => { + process.off('exit', saveSession); + }; + }, [history, sessionPersistence]); +}; From 158d669e5c517dbc2a6ff8357ea09267c3a0f6ec Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Tue, 15 Jul 2025 06:25:39 +0800 Subject: [PATCH 03/20] refactor(session): use async file loading for session persistence --- .../ui/hooks/useSessionPersistence.test.ts | 208 +++++++----------- .../cli/src/ui/hooks/useSessionPersistence.ts | 17 +- 2 files changed, 86 insertions(+), 139 deletions(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index 6ab477b51f1..13e632406c8 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -5,71 +5,62 @@ */ import { describe, it, expect, vi, beforeEach, afterEach, Mock } from 'vitest'; -import { renderHook, act } from '@testing-library/react'; +import { renderHook, waitFor } from '@testing-library/react'; import { useSessionPersistence } from './useSessionPersistence.js'; import * as fs from 'fs'; import * as path from 'path'; +import * as os from 'os'; import process from 'node:process'; import { MessageType, HistoryItem } from '../types.js'; -// Mock fs module -vi.mock('fs', async () => { - const actual = await vi.importActual('fs'); - return { - ...actual, - existsSync: vi.fn(), - readFileSync: vi.fn(), - writeFileSync: vi.fn(), - mkdirSync: vi.fn(), - }; -}); - -// Mock process.on and process.off for exit event +// Spy on process.on/off to capture the exit handler let mockProcessOn: Mock; let mockProcessOff: Mock; -describe('useSessionPersistence', () => { +describe('useSessionPersistence - Integration Test', () => { let mockHistory: HistoryItem[]; let mockLoadHistory: Mock; + let tempDir = ''; + let originalCwd = ''; beforeEach(() => { - vi.useFakeTimers(); + // Store original CWD and create a temp directory + originalCwd = process.cwd(); + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-cli-test-')); + process.chdir(tempDir); + mockHistory = []; mockLoadHistory = vi.fn(); - vi.mocked(fs.existsSync).mockReturnValue(false); - vi.mocked(fs.readFileSync).mockReturnValue(''); - vi.mocked(fs.writeFileSync).mockReturnValue(undefined); - vi.mocked(fs.mkdirSync).mockReturnValue(undefined); - + // Spy on process.on/off mockProcessOn = vi.spyOn(process, 'on'); mockProcessOff = vi.spyOn(process, 'off'); - mockProcessOn.mockClear(); - mockProcessOff.mockClear(); }); afterEach(() => { - vi.useRealTimers(); + // Restore CWD and clean up the temp directory + process.chdir(originalCwd); + fs.rmSync(tempDir, { recursive: true, force: true }); + + // Restore mocks vi.restoreAllMocks(); }); - it('should register and unregister exit handler', () => { - const { unmount } = renderHook(() => + it('should not create any files or register handlers if persistence is disabled', () => { + renderHook(() => useSessionPersistence({ - sessionPersistence: true, + sessionPersistence: false, history: mockHistory, loadHistory: mockLoadHistory, }), ); - expect(mockProcessOn).toHaveBeenCalledWith('exit', expect.any(Function)); - - unmount(); - - expect(mockProcessOff).toHaveBeenCalledWith('exit', expect.any(Function)); + const geminiDir = path.join(tempDir, '.gemini'); + expect(fs.existsSync(geminiDir)).toBe(false); + expect(mockProcessOn).not.toHaveBeenCalled(); }); - it('should save session history on exit when persistence is enabled', async () => { + it('should save session history on exit when persistence is enabled', () => { const { unmount } = renderHook(() => useSessionPersistence({ sessionPersistence: true, @@ -78,55 +69,50 @@ describe('useSessionPersistence', () => { }), ); + // Find the registered exit handler + const exitHandler = mockProcessOn.mock.calls.find( + (call) => call[0] === 'exit', + )?.[1]; + expect(exitHandler).toBeDefined(); + + // Add items to history mockHistory.push( { type: MessageType.USER, text: 'User message 1' }, { type: MessageType.GEMINI, text: 'Gemini response 1' }, { type: MessageType.INFO, text: 'Info message' }, // Should be filtered ); - // Manually trigger the exit handler - const exitHandler = mockProcessOn.mock.calls.find( - (call) => call[0] === 'exit', - )?.[1]; - - expect(exitHandler).toBeDefined(); + // Manually trigger the exit handler to save the session + exitHandler(); - await act(async () => { - exitHandler(); - }); + const sessionPath = path.join(tempDir, '.gemini', 'session.json'); + expect(fs.existsSync(sessionPath)).toBe(true); - const expectedPath = path.join(process.cwd(), '.gemini', 'session.json'); - const expectedContent = JSON.stringify( - [ - { type: MessageType.USER, text: 'User message 1' }, - { type: MessageType.GEMINI, text: 'Gemini response 1' }, - ], - null, - 2, - ); + const savedData = JSON.parse(fs.readFileSync(sessionPath, 'utf-8')); + const expectedData = [ + { type: MessageType.USER, text: 'User message 1' }, + { type: MessageType.GEMINI, text: 'Gemini response 1' }, + ]; - expect(fs.mkdirSync).toHaveBeenCalledWith( - path.join(process.cwd(), '.gemini'), - { recursive: true }, - ); - expect(fs.writeFileSync).toHaveBeenCalledWith( - expectedPath, - expectedContent, - ); + expect(savedData).toEqual(expectedData); + // Ensure the handler is unregistered on unmount unmount(); + expect(mockProcessOff).toHaveBeenCalledWith('exit', exitHandler); }); - it('should load session history on startup when persistence is enabled and file exists', async () => { + it('should load session history on startup if file exists', async () => { + const geminiDir = path.join(tempDir, '.gemini'); + fs.mkdirSync(geminiDir, { recursive: true }); + + const sessionPath = path.join(geminiDir, 'session.json'); const savedHistory = [ { type: MessageType.USER, text: 'Loaded user message' }, { type: MessageType.GEMINI, text: 'Loaded gemini response' }, ]; + fs.writeFileSync(sessionPath, JSON.stringify(savedHistory)); - vi.mocked(fs.existsSync).mockReturnValue(true); - vi.mocked(fs.readFileSync).mockReturnValue(JSON.stringify(savedHistory)); - - const { unmount } = renderHook(() => + renderHook(() => useSessionPersistence({ sessionPersistence: true, history: mockHistory, @@ -134,54 +120,23 @@ describe('useSessionPersistence', () => { }), ); - await vi.runAllTimersAsync(); // Flush effects - - expect(fs.existsSync).toHaveBeenCalledWith( - path.join(process.cwd(), '.gemini', 'session.json'), - ); - expect(fs.readFileSync).toHaveBeenCalledWith( - path.join(process.cwd(), '.gemini', 'session.json'), - 'utf-8', - ); - expect(mockLoadHistory).toHaveBeenCalledWith(savedHistory); - - unmount(); - }); - - it('should not save or load session history when persistence is disabled', async () => { - const { unmount } = renderHook(() => - useSessionPersistence({ - sessionPersistence: false, - history: mockHistory, - loadHistory: mockLoadHistory, - }), - ); - - await vi.runAllTimersAsync(); // Flush effects - - expect(fs.existsSync).not.toHaveBeenCalled(); - expect(fs.readFileSync).not.toHaveBeenCalled(); - expect(fs.writeFileSync).not.toHaveBeenCalled(); - expect(fs.mkdirSync).not.toHaveBeenCalled(); - expect(mockProcessOn).not.toHaveBeenCalledWith( - 'exit', - expect.any(Function), - ); - - unmount(); + // The hook loads asynchronously, so we wait for the loadHistory mock to be called + await waitFor(() => { + expect(mockLoadHistory).toHaveBeenCalledWith(savedHistory); + }); }); - it('should log error if loading fails', async () => { - vi.mocked(fs.existsSync).mockReturnValue(true); - vi.mocked(fs.readFileSync).mockImplementation(() => { - throw new Error('Read error'); - }); + it('should not throw or call loadHistory if session file is empty or corrupt', async () => { + const geminiDir = path.join(tempDir, '.gemini'); + fs.mkdirSync(geminiDir, { recursive: true }); + const sessionPath = path.join(geminiDir, 'session.json'); + fs.writeFileSync(sessionPath, 'corrupt data'); const consoleErrorSpy = vi .spyOn(console, 'error') .mockImplementation(() => {}); - const { unmount } = renderHook(() => + renderHook(() => useSessionPersistence({ sessionPersistence: true, history: mockHistory, @@ -189,29 +144,24 @@ describe('useSessionPersistence', () => { }), ); - await vi.runAllTimersAsync(); // Flush effects + // Wait for the async load to finish + await waitFor(() => { + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error loading session history:', + expect.any(Error), + ); + }); - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Error loading session history:', - expect.any(Error), - ); expect(mockLoadHistory).not.toHaveBeenCalled(); - consoleErrorSpy.mockRestore(); - unmount(); }); - it('should log error if saving fails', async () => { - vi.mocked(fs.existsSync).mockReturnValue(false); // Ensure mkdirSync is called - vi.mocked(fs.writeFileSync).mockImplementation(() => { - throw new Error('Write error'); - }); - + it('should not throw or call loadHistory if session file does not exist', async () => { const consoleErrorSpy = vi .spyOn(console, 'error') .mockImplementation(() => {}); - const { unmount } = renderHook(() => + renderHook(() => useSessionPersistence({ sessionPersistence: true, history: mockHistory, @@ -219,21 +169,11 @@ describe('useSessionPersistence', () => { }), ); - mockHistory.push({ type: MessageType.USER, text: 'User message' }); - - const exitHandler = mockProcessOn.mock.calls.find( - (call) => call[0] === 'exit', - )?.[1]; - - expect(exitHandler).toBeDefined(); - exitHandler(); - - expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Error saving session history:', - expect.any(Error), - ); + // Give async operations a chance to run, though none should happen + await new Promise((resolve) => setTimeout(resolve, 100)); + expect(mockLoadHistory).not.toHaveBeenCalled(); + expect(consoleErrorSpy).not.toHaveBeenCalled(); consoleErrorSpy.mockRestore(); - unmount(); }); -}); +}); \ No newline at end of file diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index d7bfd313c9a..dfb62907ea1 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -6,6 +6,7 @@ import { useEffect } from 'react'; import * as fs from 'fs'; +import { promises as fsp } from 'fs'; import * as path from 'path'; import process from 'node:process'; import { HistoryItem, MessageType } from '../types.js'; @@ -22,17 +23,23 @@ export const useSessionPersistence = ({ loadHistory, }: UseSessionPersistenceProps) => { useEffect(() => { - if (sessionPersistence) { - const sessionPath = path.join(process.cwd(), '.gemini', 'session.json'); - if (fs.existsSync(sessionPath)) { + const loadSession = async () => { + if (sessionPersistence) { + const sessionPath = path.join(process.cwd(), '.gemini', 'session.json'); try { - const sessionData = fs.readFileSync(sessionPath, 'utf-8'); + const sessionData = await fsp.readFile(sessionPath, 'utf-8'); loadHistory(JSON.parse(sessionData)); } catch (error) { + // Silently ignore if file doesn't exist. + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return; + } console.error('Error loading session history:', error); } } - } + }; + + void loadSession(); }, [sessionPersistence, loadHistory]); useEffect(() => { From 8fbf19a394c7fe8e2164f23d0328ca68253926af Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Tue, 15 Jul 2025 06:32:26 +0800 Subject: [PATCH 04/20] refactor(session): update tests to use integration testing --- packages/cli/src/ui/hooks/useSessionPersistence.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index 13e632406c8..4d179d011d8 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -176,4 +176,4 @@ describe('useSessionPersistence - Integration Test', () => { expect(consoleErrorSpy).not.toHaveBeenCalled(); consoleErrorSpy.mockRestore(); }); -}); \ No newline at end of file +}); From 706899f69e8fa7cc9398be870f7bd1f8a61f463e Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Tue, 15 Jul 2025 14:50:21 +0800 Subject: [PATCH 05/20] fix(session): Add array validation for loaded session history --- .../ui/hooks/useSessionPersistence.test.ts | 26 +++++++++++++++++++ .../cli/src/ui/hooks/useSessionPersistence.ts | 5 +++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index 4d179d011d8..89c26844304 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -156,6 +156,32 @@ describe('useSessionPersistence - Integration Test', () => { consoleErrorSpy.mockRestore(); }); + it('should not call loadHistory if session file contains valid JSON but not an array', async () => { + const geminiDir = path.join(tempDir, '.gemini'); + fs.mkdirSync(geminiDir, { recursive: true }); + const sessionPath = path.join(geminiDir, 'session.json'); + fs.writeFileSync(sessionPath, '{"key": "value"}'); // Valid JSON, but not an array + + const consoleErrorSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); + + renderHook(() => + useSessionPersistence({ + sessionPersistence: true, + history: mockHistory, + loadHistory: mockLoadHistory, + }), + ); + + // Give async operations a chance to run + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(mockLoadHistory).not.toHaveBeenCalled(); + expect(consoleErrorSpy).not.toHaveBeenCalled(); // No error should be logged for this case + consoleErrorSpy.mockRestore(); + }); + it('should not throw or call loadHistory if session file does not exist', async () => { const consoleErrorSpy = vi .spyOn(console, 'error') diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index dfb62907ea1..ef36507e4cd 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -28,7 +28,10 @@ export const useSessionPersistence = ({ const sessionPath = path.join(process.cwd(), '.gemini', 'session.json'); try { const sessionData = await fsp.readFile(sessionPath, 'utf-8'); - loadHistory(JSON.parse(sessionData)); + const parsedHistory = JSON.parse(sessionData); + if (Array.isArray(parsedHistory)) { + loadHistory(parsedHistory); + } } catch (error) { // Silently ignore if file doesn't exist. if ((error as NodeJS.ErrnoException).code === 'ENOENT') { From d3bdf41e676b37be0fca950ac13ce2d6841517f2 Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Tue, 15 Jul 2025 15:32:46 +0800 Subject: [PATCH 06/20] fix(session): Assign unique IDs to loaded history items --- .../ui/hooks/useSessionPersistence.test.ts | 20 ++++++++++++++++++- .../cli/src/ui/hooks/useSessionPersistence.ts | 11 +++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index 89c26844304..e6312b113f7 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -122,7 +122,25 @@ describe('useSessionPersistence - Integration Test', () => { // The hook loads asynchronously, so we wait for the loadHistory mock to be called await waitFor(() => { - expect(mockLoadHistory).toHaveBeenCalledWith(savedHistory); + expect(mockLoadHistory).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + type: MessageType.USER, + text: 'Loaded user message', + id: expect.any(Number), + }), + expect.objectContaining({ + type: MessageType.GEMINI, + text: 'Loaded gemini response', + id: expect.any(Number), + }), + ]), + ); + + const loadedItems = mockLoadHistory.mock.calls[0][0]; + loadedItems.forEach((item: HistoryItem) => { + expect(item.id).toBeLessThan(0); + }); }); }); diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index ef36507e4cd..593fc55ce4f 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -30,7 +30,16 @@ export const useSessionPersistence = ({ const sessionData = await fsp.readFile(sessionPath, 'utf-8'); const parsedHistory = JSON.parse(sessionData); if (Array.isArray(parsedHistory)) { - loadHistory(parsedHistory); + const historyWithIds: HistoryItem[] = parsedHistory.map( + (item: any, index: number) => ({ + type: item.type, + text: item.text, + // Use negative IDs for loaded items to avoid conflicts with new items + // generated by useHistoryManager, which will have positive IDs. + id: -(Date.now() + index), + }), + ); + loadHistory(historyWithIds); } } catch (error) { // Silently ignore if file doesn't exist. From 86100acc56323f554e403f5a1aa9b22b95a61f04 Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Tue, 15 Jul 2025 15:51:02 +0800 Subject: [PATCH 07/20] fix(session): Import HistoryItemUser and HistoryItemGemini for type correctness --- packages/cli/src/ui/hooks/useSessionPersistence.test.ts | 6 +++--- packages/cli/src/ui/hooks/useSessionPersistence.ts | 9 +++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index e6312b113f7..98ca9b88ec2 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -77,9 +77,9 @@ describe('useSessionPersistence - Integration Test', () => { // Add items to history mockHistory.push( - { type: MessageType.USER, text: 'User message 1' }, - { type: MessageType.GEMINI, text: 'Gemini response 1' }, - { type: MessageType.INFO, text: 'Info message' }, // Should be filtered + { id: 1, type: MessageType.USER, text: 'User message 1' }, + { id: 2, type: MessageType.GEMINI, text: 'Gemini response 1' }, + { id: 3, type: MessageType.INFO, text: 'Info message' }, // Should be filtered ); // Manually trigger the exit handler to save the session diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index 593fc55ce4f..15a8c8179f6 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -9,7 +9,12 @@ import * as fs from 'fs'; import { promises as fsp } from 'fs'; import * as path from 'path'; import process from 'node:process'; -import { HistoryItem, MessageType } from '../types.js'; +import { + HistoryItem, + MessageType, + HistoryItemUser, + HistoryItemGemini, +} from '../types.js'; interface UseSessionPersistenceProps { sessionPersistence: boolean | undefined; @@ -31,7 +36,7 @@ export const useSessionPersistence = ({ const parsedHistory = JSON.parse(sessionData); if (Array.isArray(parsedHistory)) { const historyWithIds: HistoryItem[] = parsedHistory.map( - (item: any, index: number) => ({ + (item: HistoryItemUser | HistoryItemGemini, index: number) => ({ type: item.type, text: item.text, // Use negative IDs for loaded items to avoid conflicts with new items From 34bb1110f12b5f2d6877ff34d1398c4494c23dbb Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Wed, 16 Jul 2025 06:13:28 +0800 Subject: [PATCH 08/20] fix(session): Add length check before calling loadHistory --- .../ui/hooks/useSessionPersistence.test.ts | 20 +++++++++++++ .../cli/src/ui/hooks/useSessionPersistence.ts | 28 ++++++++++++++----- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index 98ca9b88ec2..bd3d8e3cc73 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -200,6 +200,26 @@ describe('useSessionPersistence - Integration Test', () => { consoleErrorSpy.mockRestore(); }); + it('should not call loadHistory if session file contains malformed history items', async () => { + const geminiDir = path.join(tempDir, '.gemini'); + fs.mkdirSync(geminiDir, { recursive: true }); + const sessionPath = path.join(geminiDir, 'session.json'); + fs.writeFileSync(sessionPath, '[null, {"foo": "bar"}]'); // Malformed history items + + renderHook(() => + useSessionPersistence({ + sessionPersistence: true, + history: mockHistory, + loadHistory: mockLoadHistory, + }), + ); + + // Give async operations a chance to run + await new Promise((resolve) => setTimeout(resolve, 100)); + + expect(mockLoadHistory).not.toHaveBeenCalled(); + }); + it('should not throw or call loadHistory if session file does not exist', async () => { const consoleErrorSpy = vi .spyOn(console, 'error') diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index 15a8c8179f6..a4041116fc1 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -35,16 +35,30 @@ export const useSessionPersistence = ({ const sessionData = await fsp.readFile(sessionPath, 'utf-8'); const parsedHistory = JSON.parse(sessionData); if (Array.isArray(parsedHistory)) { - const historyWithIds: HistoryItem[] = parsedHistory.map( - (item: HistoryItemUser | HistoryItemGemini, index: number) => ({ + const historyWithIds: HistoryItem[] = parsedHistory + .filter( + (item: unknown): item is HistoryItemUser | HistoryItemGemini => + item !== null && + typeof item === 'object' && + 'type' in item && + (item.type === MessageType.USER || + item.type === MessageType.GEMINI) && + 'text' in item && + typeof (item as { text: unknown }).text === 'string', + ) + .map((item, index) => ({ type: item.type, text: item.text, - // Use negative IDs for loaded items to avoid conflicts with new items - // generated by useHistoryManager, which will have positive IDs. - id: -(Date.now() + index), - }), + id: -(index + 1), + })); + if (historyWithIds.length > 0) { + loadHistory(historyWithIds); + } + } else if (parsedHistory.length > 0) { + console.error( + 'Error loading session history: Malformed history items found.', + parsedHistory, ); - loadHistory(historyWithIds); } } catch (error) { // Silently ignore if file doesn't exist. From 36226e4db78d85229fc31ef93e954846d8372dfa Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Wed, 16 Jul 2025 06:26:37 +0800 Subject: [PATCH 09/20] fix(session): Remove misleading error log for malformed history items --- packages/cli/src/ui/hooks/useSessionPersistence.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index a4041116fc1..e3f6608a5bb 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -54,11 +54,6 @@ export const useSessionPersistence = ({ if (historyWithIds.length > 0) { loadHistory(historyWithIds); } - } else if (parsedHistory.length > 0) { - console.error( - 'Error loading session history: Malformed history items found.', - parsedHistory, - ); } } catch (error) { // Silently ignore if file doesn't exist. From 56acc2ace82bb5bac412182e6cb4625ef5e190f7 Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Wed, 16 Jul 2025 06:48:30 +0800 Subject: [PATCH 10/20] refactor(session): Optimize useEffect with useRef for history state --- .../cli/src/ui/hooks/useSessionPersistence.test.ts | 14 +++++++------- packages/cli/src/ui/hooks/useSessionPersistence.ts | 11 ++++++++--- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index bd3d8e3cc73..7aecf0ba1ff 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -61,6 +61,13 @@ describe('useSessionPersistence - Integration Test', () => { }); it('should save session history on exit when persistence is enabled', () => { + // Add items to history before rendering the hook + mockHistory.push( + { id: 1, type: MessageType.USER, text: 'User message 1' }, + { id: 2, type: MessageType.GEMINI, text: 'Gemini response 1' }, + { id: 3, type: MessageType.INFO, text: 'Info message' }, // Should be filtered + ); + const { unmount } = renderHook(() => useSessionPersistence({ sessionPersistence: true, @@ -75,13 +82,6 @@ describe('useSessionPersistence - Integration Test', () => { )?.[1]; expect(exitHandler).toBeDefined(); - // Add items to history - mockHistory.push( - { id: 1, type: MessageType.USER, text: 'User message 1' }, - { id: 2, type: MessageType.GEMINI, text: 'Gemini response 1' }, - { id: 3, type: MessageType.INFO, text: 'Info message' }, // Should be filtered - ); - // Manually trigger the exit handler to save the session exitHandler(); diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index e3f6608a5bb..f04afb7cf71 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { useEffect } from 'react'; +import { useEffect, useRef } from 'react'; import * as fs from 'fs'; import { promises as fsp } from 'fs'; import * as path from 'path'; @@ -27,6 +27,11 @@ export const useSessionPersistence = ({ history, loadHistory, }: UseSessionPersistenceProps) => { + const historyRef = useRef(history); + + useEffect(() => { + historyRef.current = history; + }, [history]); useEffect(() => { const loadSession = async () => { if (sessionPersistence) { @@ -82,7 +87,7 @@ export const useSessionPersistence = ({ const sessionPath = path.join(geminiDir, 'session.json'); // Create a serializable version of the history - const serializableHistory = history + const serializableHistory = historyRef.current .filter( (item) => item.type === MessageType.USER || @@ -106,5 +111,5 @@ export const useSessionPersistence = ({ return () => { process.off('exit', saveSession); }; - }, [history, sessionPersistence]); + }, [sessionPersistence]); }; From b6ac52fd886270222facd5d5377469ef61270650 Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Wed, 16 Jul 2025 14:53:29 +0800 Subject: [PATCH 11/20] fix(session): Improve error handling for saving session history --- .../ui/hooks/useSessionPersistence.test.ts | 39 +++++++++++++++++++ .../cli/src/ui/hooks/useSessionPersistence.ts | 17 ++++---- 2 files changed, 47 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index 7aecf0ba1ff..fd5611e4687 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -240,4 +240,43 @@ describe('useSessionPersistence - Integration Test', () => { expect(consoleErrorSpy).not.toHaveBeenCalled(); consoleErrorSpy.mockRestore(); }); + + it('should log error if saving fails due to file system permissions', () => { + const consoleErrorSpy = vi + .spyOn(console, 'error') + .mockImplementation(() => {}); + + // Create a read-only directory to simulate permission error + const geminiDir = path.join(tempDir, '.gemini'); + fs.mkdirSync(geminiDir, { recursive: true }); + fs.chmodSync(geminiDir, 0o444); // Set read-only permissions + + // Add items to history before rendering the hook + mockHistory.push( + { id: 1, type: MessageType.USER, text: 'User message' }, + ); + + const { unmount } = renderHook(() => + useSessionPersistence({ + sessionPersistence: true, + history: mockHistory, + loadHistory: mockLoadHistory, + }), + ); + + const exitHandler = mockProcessOn.mock.calls.find( + (call) => call[0] === 'exit', + )?.[1]; + expect(exitHandler).toBeDefined(); + + exitHandler(); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Error saving session history:', + expect.any(Error), + ); + + consoleErrorSpy.mockRestore(); + unmount(); + }); }); diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index f04afb7cf71..edbd64a7719 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -79,7 +79,8 @@ export const useSessionPersistence = ({ } const saveSession = () => { - if (sessionPersistence) { + // The surrounding useEffect ensures this only runs when persistence is enabled. + try { const geminiDir = path.join(process.cwd(), '.gemini'); if (!fs.existsSync(geminiDir)) { fs.mkdirSync(geminiDir, { recursive: true }); @@ -95,14 +96,12 @@ export const useSessionPersistence = ({ ) .map((item) => ({ type: item.type, text: item.text })); - try { - fs.writeFileSync( - sessionPath, - JSON.stringify(serializableHistory, null, 2), - ); - } catch (error) { - console.error('Error saving session history:', error); - } + fs.writeFileSync( + sessionPath, + JSON.stringify(serializableHistory, null, 2), + ); + } catch (error) { + console.error('Error saving session history:', error); } }; From 76556b02e2997959794a242f03016e55e44780b5 Mon Sep 17 00:00:00 2001 From: Jiecheng Z Date: Wed, 16 Jul 2025 15:02:04 +0800 Subject: [PATCH 12/20] Update packages/cli/src/ui/hooks/useSessionPersistence.ts Avoid race condition Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- packages/cli/src/ui/hooks/useSessionPersistence.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index edbd64a7719..c0b5574923b 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -57,7 +57,7 @@ export const useSessionPersistence = ({ id: -(index + 1), })); if (historyWithIds.length > 0) { - loadHistory(historyWithIds); + loadHistory([...historyWithIds, ...historyRef.current]); } } } catch (error) { From 73fefb367115d860031ef7d8562b542e792d4d9f Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Wed, 16 Jul 2025 15:06:08 +0800 Subject: [PATCH 13/20] fix(session): Merge loaded history with current history to prevent data loss --- .../cli/src/ui/hooks/useSessionPersistence.test.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index fd5611e4687..b5732fd54a4 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -112,6 +112,11 @@ describe('useSessionPersistence - Integration Test', () => { ]; fs.writeFileSync(sessionPath, JSON.stringify(savedHistory)); + const initialHistory = [ + { id: 100, type: MessageType.USER, text: 'Initial message' }, + ]; + mockHistory.push(...initialHistory); + renderHook(() => useSessionPersistence({ sessionPersistence: true, @@ -134,12 +139,19 @@ describe('useSessionPersistence - Integration Test', () => { text: 'Loaded gemini response', id: expect.any(Number), }), + expect.objectContaining({ + type: MessageType.USER, + text: 'Initial message', + id: 100, + }), ]), ); const loadedItems = mockLoadHistory.mock.calls[0][0]; loadedItems.forEach((item: HistoryItem) => { - expect(item.id).toBeLessThan(0); + if (item.id !== 100) { // Only check loaded items for negative IDs + expect(item.id).toBeLessThan(0); + } }); }); }); From 868575b5cc2d8208fa397e7e4a7df3b4e1852b54 Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Wed, 16 Jul 2025 15:16:42 +0800 Subject: [PATCH 14/20] fix(session): Handle race condition in loadSession and improve saving error test --- .../cli/src/ui/hooks/useSessionPersistence.test.ts | 7 +++---- packages/cli/src/ui/hooks/useSessionPersistence.ts | 11 +++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index b5732fd54a4..a3c7159a8d5 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -149,7 +149,8 @@ describe('useSessionPersistence - Integration Test', () => { const loadedItems = mockLoadHistory.mock.calls[0][0]; loadedItems.forEach((item: HistoryItem) => { - if (item.id !== 100) { // Only check loaded items for negative IDs + if (item.id !== 100) { + // Only check loaded items for negative IDs expect(item.id).toBeLessThan(0); } }); @@ -264,9 +265,7 @@ describe('useSessionPersistence - Integration Test', () => { fs.chmodSync(geminiDir, 0o444); // Set read-only permissions // Add items to history before rendering the hook - mockHistory.push( - { id: 1, type: MessageType.USER, text: 'User message' }, - ); + mockHistory.push({ id: 1, type: MessageType.USER, text: 'User message' }); const { unmount } = renderHook(() => useSessionPersistence({ diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index c0b5574923b..4f1e2892e6e 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -33,12 +33,17 @@ export const useSessionPersistence = ({ historyRef.current = history; }, [history]); useEffect(() => { + let isMounted = true; + const loadSession = async () => { if (sessionPersistence) { const sessionPath = path.join(process.cwd(), '.gemini', 'session.json'); try { const sessionData = await fsp.readFile(sessionPath, 'utf-8'); const parsedHistory = JSON.parse(sessionData); + + if (!isMounted) return; + if (Array.isArray(parsedHistory)) { const historyWithIds: HistoryItem[] = parsedHistory .filter( @@ -61,6 +66,8 @@ export const useSessionPersistence = ({ } } } catch (error) { + if (!isMounted) return; + // Silently ignore if file doesn't exist. if ((error as NodeJS.ErrnoException).code === 'ENOENT') { return; @@ -71,6 +78,10 @@ export const useSessionPersistence = ({ }; void loadSession(); + + return () => { + isMounted = false; + }; }, [sessionPersistence, loadHistory]); useEffect(() => { From fb1f7c67c2f9bc91cca691b5145db3b0e73cfea0 Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Fri, 18 Jul 2025 06:07:49 +0800 Subject: [PATCH 15/20] fix(cli): remove duplicate line in useSessionPersistence.ts --- packages/cli/src/ui/hooks/useSessionPersistence.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index 4f1e2892e6e..e4c7ea8d68f 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -99,12 +99,14 @@ export const useSessionPersistence = ({ const sessionPath = path.join(geminiDir, 'session.json'); // Create a serializable version of the history + const MAX_PERSISTED_HISTORY = 200; // A reasonable default, could be made configurable. const serializableHistory = historyRef.current .filter( (item) => item.type === MessageType.USER || item.type === MessageType.GEMINI, ) + .slice(-MAX_PERSISTED_HISTORY) .map((item) => ({ type: item.type, text: item.text })); fs.writeFileSync( From baf2eee5aac024adad8cae352992a09cc6ef1138 Mon Sep 17 00:00:00 2001 From: JiechengZhao Date: Tue, 22 Jul 2025 13:39:51 +0800 Subject: [PATCH 16/20] feat: Add session persistence and onLoadComplete callback --- .gitignore | 2 + packages/cli/src/ui/App.tsx | 10 ++- .../ui/hooks/useSessionPersistence.test.ts | 14 ++++ .../cli/src/ui/hooks/useSessionPersistence.ts | 82 +++++++++++-------- 4 files changed, 72 insertions(+), 36 deletions(-) diff --git a/.gitignore b/.gitignore index af3591bda23..32be8052a0b 100644 --- a/.gitignore +++ b/.gitignore @@ -39,3 +39,5 @@ packages/*/coverage/ packages/cli/src/generated/ .integration-tests/ packages/vscode-ide-companion/*.vsix + +tmp_run_preflight_without_proxy.sh \ No newline at end of file diff --git a/packages/cli/src/ui/App.tsx b/packages/cli/src/ui/App.tsx index 4488e5ed8f5..2afcd2c0c5f 100644 --- a/packages/cli/src/ui/App.tsx +++ b/packages/cli/src/ui/App.tsx @@ -115,7 +115,13 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { const { history, addItem, clearItems, loadHistory } = useHistory(); const sessionPersistence = settings.merged['session.persistence']; - useSessionPersistence({ sessionPersistence, history, loadHistory }); + const [sessionLoaded, setSessionLoaded] = useState(false); + useSessionPersistence({ + sessionPersistence, + history, + loadHistory, + onLoadComplete: () => setSessionLoaded(true), + }); const { consoleMessages, handleNewMessage, @@ -666,6 +672,7 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { if ( initialPrompt && !initialPromptSubmitted.current && + sessionLoaded && !isAuthenticating && !isAuthDialogOpen && !isThemeDialogOpen && @@ -679,6 +686,7 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { }, [ initialPrompt, submitQuery, + sessionLoaded, isAuthenticating, isAuthDialogOpen, isThemeDialogOpen, diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index a3c7159a8d5..9bad30742c0 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -20,6 +20,7 @@ let mockProcessOff: Mock; describe('useSessionPersistence - Integration Test', () => { let mockHistory: HistoryItem[]; let mockLoadHistory: Mock; + let mockOnLoadComplete: Mock; let tempDir = ''; let originalCwd = ''; @@ -31,6 +32,11 @@ describe('useSessionPersistence - Integration Test', () => { mockHistory = []; mockLoadHistory = vi.fn(); + mockOnLoadComplete = vi.fn(); + mockOnLoadComplete = vi.fn(); + mockOnLoadComplete = vi.fn(); + mockOnLoadComplete = vi.fn(); + mockOnLoadComplete = vi.fn(); // Spy on process.on/off mockProcessOn = vi.spyOn(process, 'on'); @@ -52,6 +58,7 @@ describe('useSessionPersistence - Integration Test', () => { sessionPersistence: false, history: mockHistory, loadHistory: mockLoadHistory, + onLoadComplete: mockOnLoadComplete, }), ); @@ -73,6 +80,7 @@ describe('useSessionPersistence - Integration Test', () => { sessionPersistence: true, history: mockHistory, loadHistory: mockLoadHistory, + onLoadComplete: mockOnLoadComplete, }), ); @@ -122,6 +130,7 @@ describe('useSessionPersistence - Integration Test', () => { sessionPersistence: true, history: mockHistory, loadHistory: mockLoadHistory, + onLoadComplete: mockOnLoadComplete, }), ); @@ -172,6 +181,7 @@ describe('useSessionPersistence - Integration Test', () => { sessionPersistence: true, history: mockHistory, loadHistory: mockLoadHistory, + onLoadComplete: mockOnLoadComplete, }), ); @@ -202,6 +212,7 @@ describe('useSessionPersistence - Integration Test', () => { sessionPersistence: true, history: mockHistory, loadHistory: mockLoadHistory, + onLoadComplete: mockOnLoadComplete, }), ); @@ -224,6 +235,7 @@ describe('useSessionPersistence - Integration Test', () => { sessionPersistence: true, history: mockHistory, loadHistory: mockLoadHistory, + onLoadComplete: mockOnLoadComplete, }), ); @@ -243,6 +255,7 @@ describe('useSessionPersistence - Integration Test', () => { sessionPersistence: true, history: mockHistory, loadHistory: mockLoadHistory, + onLoadComplete: mockOnLoadComplete, }), ); @@ -272,6 +285,7 @@ describe('useSessionPersistence - Integration Test', () => { sessionPersistence: true, history: mockHistory, loadHistory: mockLoadHistory, + onLoadComplete: mockOnLoadComplete, }), ); diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index e4c7ea8d68f..702cbe65d2c 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -20,12 +20,14 @@ interface UseSessionPersistenceProps { sessionPersistence: boolean | undefined; history: HistoryItem[]; loadHistory: (history: HistoryItem[]) => void; + onLoadComplete: () => void; } export const useSessionPersistence = ({ sessionPersistence, history, loadHistory, + onLoadComplete, }: UseSessionPersistenceProps) => { const historyRef = useRef(history); @@ -36,44 +38,54 @@ export const useSessionPersistence = ({ let isMounted = true; const loadSession = async () => { - if (sessionPersistence) { - const sessionPath = path.join(process.cwd(), '.gemini', 'session.json'); - try { - const sessionData = await fsp.readFile(sessionPath, 'utf-8'); - const parsedHistory = JSON.parse(sessionData); - - if (!isMounted) return; - - if (Array.isArray(parsedHistory)) { - const historyWithIds: HistoryItem[] = parsedHistory - .filter( - (item: unknown): item is HistoryItemUser | HistoryItemGemini => - item !== null && - typeof item === 'object' && - 'type' in item && - (item.type === MessageType.USER || - item.type === MessageType.GEMINI) && - 'text' in item && - typeof (item as { text: unknown }).text === 'string', - ) - .map((item, index) => ({ - type: item.type, - text: item.text, - id: -(index + 1), - })); - if (historyWithIds.length > 0) { - loadHistory([...historyWithIds, ...historyRef.current]); + try { + if (sessionPersistence) { + const sessionPath = path.join( + process.cwd(), + '.gemini', + 'session.json', + ); + try { + const sessionData = await fsp.readFile(sessionPath, 'utf-8'); + const parsedHistory = JSON.parse(sessionData); + + if (!isMounted) return; + + if (Array.isArray(parsedHistory)) { + const historyWithIds: HistoryItem[] = parsedHistory + .filter( + ( + item: unknown, + ): item is HistoryItemUser | HistoryItemGemini => + item !== null && + typeof item === 'object' && + 'type' in item && + (item.type === MessageType.USER || + item.type === MessageType.GEMINI) && + 'text' in item && + typeof (item as { text: unknown }).text === 'string', + ) + .map((item, index) => ({ + type: item.type, + text: item.text, + id: -(index + 1), + })); + if (historyWithIds.length > 0) { + loadHistory([...historyWithIds, ...historyRef.current]); + } } - } - } catch (error) { - if (!isMounted) return; + } catch (error) { + if (!isMounted) return; - // Silently ignore if file doesn't exist. - if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - return; + // Silently ignore if file doesn't exist. + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + return; + } + console.error('Error loading session history:', error); } - console.error('Error loading session history:', error); } + } finally { + onLoadComplete(); } }; @@ -82,7 +94,7 @@ export const useSessionPersistence = ({ return () => { isMounted = false; }; - }, [sessionPersistence, loadHistory]); + }, [sessionPersistence, loadHistory, onLoadComplete]); useEffect(() => { if (!sessionPersistence) { From 09527e91a8fd2a83dc0fe5331245bb0d766001ba Mon Sep 17 00:00:00 2001 From: Jiecheng Z Date: Tue, 22 Jul 2025 17:14:38 +0800 Subject: [PATCH 17/20] Update packages/cli/src/ui/hooks/useSessionPersistence.ts Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- packages/cli/src/ui/hooks/useSessionPersistence.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index 702cbe65d2c..2c860e9b281 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -104,7 +104,7 @@ export const useSessionPersistence = ({ const saveSession = () => { // The surrounding useEffect ensures this only runs when persistence is enabled. try { - const geminiDir = path.join(process.cwd(), '.gemini'); + const geminiDir = USER_SETTINGS_DIR; if (!fs.existsSync(geminiDir)) { fs.mkdirSync(geminiDir, { recursive: true }); } From 057f172ef069c1b2c5c6abc59ae878363d9e157c Mon Sep 17 00:00:00 2001 From: Jason Zhao Date: Wed, 23 Jul 2025 10:05:51 +0800 Subject: [PATCH 18/20] Refactor: Store session file in user's home directory --- packages/cli/src/config/config.test.ts | 4 ++++ packages/cli/src/ui/hooks/useSessionPersistence.ts | 7 ++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/config/config.test.ts b/packages/cli/src/config/config.test.ts index 0c0761cc124..db3c70748d2 100644 --- a/packages/cli/src/config/config.test.ts +++ b/packages/cli/src/config/config.test.ts @@ -156,6 +156,10 @@ describe('loadCliConfig', () => { vi.resetAllMocks(); vi.mocked(os.homedir).mockReturnValue('/mock/home/user'); process.env.GEMINI_API_KEY = 'test-api-key'; // Ensure API key is set for tests + delete process.env.https_proxy; + delete process.env.http_proxy; + delete process.env.HTTPS_PROXY; + delete process.env.HTTP_PROXY; }); afterEach(() => { diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.ts b/packages/cli/src/ui/hooks/useSessionPersistence.ts index 2c860e9b281..5fd18c19b9c 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.ts @@ -9,6 +9,7 @@ import * as fs from 'fs'; import { promises as fsp } from 'fs'; import * as path from 'path'; import process from 'node:process'; +import { USER_SETTINGS_DIR } from '../../config/settings.js'; import { HistoryItem, MessageType, @@ -40,11 +41,7 @@ export const useSessionPersistence = ({ const loadSession = async () => { try { if (sessionPersistence) { - const sessionPath = path.join( - process.cwd(), - '.gemini', - 'session.json', - ); + const sessionPath = path.join(USER_SETTINGS_DIR, 'session.json'); try { const sessionData = await fsp.readFile(sessionPath, 'utf-8'); const parsedHistory = JSON.parse(sessionData); From d0aa7994a5b2afe5659f4ff3698fa1e7b032d6c6 Mon Sep 17 00:00:00 2001 From: Jason Zhao Date: Wed, 23 Jul 2025 10:33:52 +0800 Subject: [PATCH 19/20] fix test --- .../ui/hooks/useSessionPersistence.test.ts | 29 ++++++++++++------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts index 9bad30742c0..4eaa7235bd2 100644 --- a/packages/cli/src/ui/hooks/useSessionPersistence.test.ts +++ b/packages/cli/src/ui/hooks/useSessionPersistence.test.ts @@ -21,14 +21,21 @@ describe('useSessionPersistence - Integration Test', () => { let mockHistory: HistoryItem[]; let mockLoadHistory: Mock; let mockOnLoadComplete: Mock; - let tempDir = ''; let originalCwd = ''; + vi.mock('os', async (importOriginal) => { + const actualOs = await importOriginal(); + return { + ...actualOs, + homedir: vi.fn(() => 'home-user'), + }; + }); + beforeEach(() => { // Store original CWD and create a temp directory originalCwd = process.cwd(); - tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gemini-cli-test-')); - process.chdir(tempDir); + fs.mkdirSync(os.homedir()); + process.chdir(os.homedir()); mockHistory = []; mockLoadHistory = vi.fn(); @@ -46,7 +53,7 @@ describe('useSessionPersistence - Integration Test', () => { afterEach(() => { // Restore CWD and clean up the temp directory process.chdir(originalCwd); - fs.rmSync(tempDir, { recursive: true, force: true }); + fs.rmSync(os.homedir(), { recursive: true, force: true }); // Restore mocks vi.restoreAllMocks(); @@ -62,7 +69,7 @@ describe('useSessionPersistence - Integration Test', () => { }), ); - const geminiDir = path.join(tempDir, '.gemini'); + const geminiDir = path.join(os.homedir(), '.gemini'); expect(fs.existsSync(geminiDir)).toBe(false); expect(mockProcessOn).not.toHaveBeenCalled(); }); @@ -93,7 +100,7 @@ describe('useSessionPersistence - Integration Test', () => { // Manually trigger the exit handler to save the session exitHandler(); - const sessionPath = path.join(tempDir, '.gemini', 'session.json'); + const sessionPath = path.join(os.homedir(), '.gemini', 'session.json'); expect(fs.existsSync(sessionPath)).toBe(true); const savedData = JSON.parse(fs.readFileSync(sessionPath, 'utf-8')); @@ -110,7 +117,7 @@ describe('useSessionPersistence - Integration Test', () => { }); it('should load session history on startup if file exists', async () => { - const geminiDir = path.join(tempDir, '.gemini'); + const geminiDir = path.join(os.homedir(), '.gemini'); fs.mkdirSync(geminiDir, { recursive: true }); const sessionPath = path.join(geminiDir, 'session.json'); @@ -167,7 +174,7 @@ describe('useSessionPersistence - Integration Test', () => { }); it('should not throw or call loadHistory if session file is empty or corrupt', async () => { - const geminiDir = path.join(tempDir, '.gemini'); + const geminiDir = path.join(os.homedir(), '.gemini'); fs.mkdirSync(geminiDir, { recursive: true }); const sessionPath = path.join(geminiDir, 'session.json'); fs.writeFileSync(sessionPath, 'corrupt data'); @@ -198,7 +205,7 @@ describe('useSessionPersistence - Integration Test', () => { }); it('should not call loadHistory if session file contains valid JSON but not an array', async () => { - const geminiDir = path.join(tempDir, '.gemini'); + const geminiDir = path.join(os.homedir(), '.gemini'); fs.mkdirSync(geminiDir, { recursive: true }); const sessionPath = path.join(geminiDir, 'session.json'); fs.writeFileSync(sessionPath, '{"key": "value"}'); // Valid JSON, but not an array @@ -225,7 +232,7 @@ describe('useSessionPersistence - Integration Test', () => { }); it('should not call loadHistory if session file contains malformed history items', async () => { - const geminiDir = path.join(tempDir, '.gemini'); + const geminiDir = path.join(os.homedir(), '.gemini'); fs.mkdirSync(geminiDir, { recursive: true }); const sessionPath = path.join(geminiDir, 'session.json'); fs.writeFileSync(sessionPath, '[null, {"foo": "bar"}]'); // Malformed history items @@ -273,7 +280,7 @@ describe('useSessionPersistence - Integration Test', () => { .mockImplementation(() => {}); // Create a read-only directory to simulate permission error - const geminiDir = path.join(tempDir, '.gemini'); + const geminiDir = path.join(os.homedir(), '.gemini'); fs.mkdirSync(geminiDir, { recursive: true }); fs.chmodSync(geminiDir, 0o444); // Set read-only permissions From 90b7c5a992165fb3dda60fde008c2cd6511ae3bf Mon Sep 17 00:00:00 2001 From: Jason Zhao Date: Wed, 23 Jul 2025 10:38:37 +0800 Subject: [PATCH 20/20] fix onloadComplete --- packages/cli/src/ui/App.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/ui/App.tsx b/packages/cli/src/ui/App.tsx index 2afcd2c0c5f..ca421a066c7 100644 --- a/packages/cli/src/ui/App.tsx +++ b/packages/cli/src/ui/App.tsx @@ -116,11 +116,12 @@ const App = ({ config, settings, startupWarnings = [], version }: AppProps) => { const { history, addItem, clearItems, loadHistory } = useHistory(); const sessionPersistence = settings.merged['session.persistence']; const [sessionLoaded, setSessionLoaded] = useState(false); + const onLoadComplete = useCallback(() => setSessionLoaded(true), []); useSessionPersistence({ sessionPersistence, history, loadHistory, - onLoadComplete: () => setSessionLoaded(true), + onLoadComplete, }); const { consoleMessages,