feat: Implement session persistence for CLI chat history#4070
feat: Implement session persistence for CLI chat history#4070JiechengZhao wants to merge 22 commits intogoogle-gemini:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Summary of Changes
Hello @JiechengZhao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant new feature: the ability to persist CLI chat history. The core change involves refactoring the persistence mechanism into a dedicated, testable React hook, useSessionPersistence, which handles the loading of previous sessions on startup and saving the current session upon exit. This enhances the user experience by maintaining chat context and improves the codebase's modularity and testability.
Highlights
- Feature Implementation: Implemented session persistence for the CLI chat history, enabling user and Gemini messages to be saved to and loaded from a
.gemini/session.jsonfile across application sessions. - Code Refactoring: Extracted the session persistence logic from the main
App.tsxcomponent into a new, dedicateduseSessionPersistenceReact hook. This significantly improves code organization, reusability, and separation of concerns. - Test Coverage: Added comprehensive unit tests for the new
useSessionPersistencehook. These tests thoroughly validate the hook's behavior, including history loading, saving, and proper handling of process exit events, by mocking file system operations. - Configuration: Introduced a new
session.persistencesetting within the application's configuration, allowing users to easily enable or disable the chat history persistence feature. - Type System Enhancement: Added a new
MessageType.TOOL_GROUPenum member to theMessageTypeenum intypes.ts, expanding the types of messages that can be represented.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors session persistence logic into a dedicated useSessionPersistence hook, improving modularity and testability. A performance issue was identified where synchronous file I/O is used for loading session history, which could block the UI on startup. A suggestion to switch to asynchronous I/O has been provided.
4cffcb0 to
678de9c
Compare
678de9c to
e12bcb2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the session persistence logic into a new, well-tested useSessionPersistence hook, which improves code organization and maintainability. A high-severity issue was identified regarding data validation when loading the session file. Adding a check to ensure the parsed data is an array will prevent potential runtime crashes from a corrupt session.json file.
5342f01 to
1f64dc4
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully refactors session persistence logic into a dedicated useSessionPersistence hook, improving modularity. The accompanying tests are thorough.
My review identifies a critical issue where history items loaded from the session are missing required id properties, which would cause rendering problems. I've also found a typing issue in the test data. I've provided suggestions to fix both issues.
1f64dc4 to
9582dc1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces session persistence for the CLI chat history by refactoring the session persistence logic into a new useSessionPersistence hook and adding comprehensive unit tests. The review focuses on enhancing the robustness of the new hook by hardening the session loading logic against malformed data and preventing a potential race condition in ID generation.
9582dc1 to
465bea0
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors session persistence logic into a dedicated hook, improving code organization and testability. Comprehensive tests cover many edge cases. A high-severity issue was identified in the hook's handling of non-array session data, potentially causing inconsistent behavior and confusing error messages. The provided feedback addresses this issue.
465bea0 to
88bb400
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors session persistence logic into a dedicated useSessionPersistence hook, improving modularity and separation of concerns. A potential performance issue was identified in the new hook where the exit event handler is re-registered on every history change. A code suggestion was provided to refactor this using a useRef to hold the history, which is a more standard and performant pattern for this use case.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request effectively refactors the session persistence logic into a dedicated useSessionPersistence hook, which is a great improvement for code organization and testability. The new hook is well-implemented and comes with a comprehensive set of unit tests that cover various success and failure scenarios.
I have one high-severity suggestion for the useSessionPersistence.ts file to improve its robustness by ensuring all file system operations that can throw errors within the exit handler are properly wrapped in a try...catch block. This will prevent unhandled exceptions from crashing the process during shutdown.
Overall, this is a solid contribution that improves the maintainability of the codebase.
bc5a352 to
e76acfa
Compare
|
/gemini review |
Avoid race condition Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
8d972b2 to
baf2eee
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request does a great job of refactoring the session persistence logic into a dedicated and well-tested useSessionPersistence hook. The separation of concerns is a significant improvement for maintainability.
My main feedback is a critical security and usability issue regarding the storage location of the session file. It's currently being saved in the current working directory, which could lead to data exposure and a fragmented user experience. I've provided a detailed comment with a suggested fix to store the session file in the user's home directory instead.
Once that is addressed, this will be a solid contribution.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request refactors session persistence logic into a dedicated hook, improving code organization. A critical issue exists in the test suite where the file path validation is incorrect, leading to unreliable tests. The test suite needs to be updated to ensure the persistence logic is properly covered.
| const sessionPath = path.join(tempDir, '.gemini', 'session.json'); | ||
| expect(fs.existsSync(sessionPath)).toBe(true); |
There was a problem hiding this comment.
The test logic uses tempDir to construct the session file path, but the useSessionPersistence hook uses USER_SETTINGS_DIR (based on os.homedir()). This discrepancy means the test isn't validating the correct file location. Update the test to use path.join(os.homedir(), '.gemini', 'session.json') to match the hook's behavior.
| const sessionPath = path.join(tempDir, '.gemini', 'session.json'); | |
| expect(fs.existsSync(sessionPath)).toBe(true); | |
| const sessionPath = path.join(os.homedir(), '.gemini', 'session.json'); | |
| expect(fs.existsSync(sessionPath)).toBe(true); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the session persistence logic into a dedicated useSessionPersistence hook, improving code organization and testability. However, the inline onLoadComplete callback in App.tsx causes repeated session history loading, leading to data duplication. Memoizing the callback with useCallback is recommended to prevent this issue.
| useSessionPersistence({ | ||
| sessionPersistence, | ||
| history, | ||
| loadHistory, | ||
| onLoadComplete: () => setSessionLoaded(true), | ||
| }); |
There was a problem hiding this comment.
The onLoadComplete callback passed to useSessionPersistence is created inline, which means a new function instance is generated on every render of the App component.
The useSessionPersistence hook includes this callback in a useEffect dependency array. Consequently, the effect that loads the session history from disk re-runs on every render of App. This leads to the session history being repeatedly loaded and prepended to the state, causing severe data duplication in the chat history.
To fix this, the onLoadComplete callback must be memoized with useCallback to ensure it has a stable reference across re-renders.
const [sessionLoaded, setSessionLoaded] = useState(false);
const onLoadComplete = useCallback(() => setSessionLoaded(true), []);
useSessionPersistence({
sessionPersistence,
history,
loadHistory,
onLoadComplete,
});|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors session persistence logic into a dedicated useSessionPersistence hook, improving modularity and separation of concerns. The new hook is well-implemented with robust error handling and includes a comprehensive set of integration tests. The changes in App.tsx to consume this hook are clean and correct.
|
Thanks for the contribution. I'm sorry we did not review it in a timely manner. Closing as duplicate work related has started to land in the small pr's linked off of #4401 has landed. Thanks, |
TLDR
This pull request refactors the session persistence logic out of the main
App.tsxcomponent and into a new, dedicateduseSessionPersistencehook. This improves code organization and separation of concerns. Comprehensive unit tests for the new hook have also been added and enabled.Dive Deeper
The session persistence logic, which handles saving chat history on exit and reloading it on startup, was previously implemented directly within the
App.tsxcomponent using a pair ofuseEffecthooks. This cluttered the main component with side effects and file system logic that were not directly related to its rendering responsibilities.By extracting this functionality into
useSessionPersistence.ts, we create a clean, reusable, and independently testable piece of logic, adhering to React best practices. TheApp.tsxcomponent is now simpler, only needing to invoke the new hook to enable the feature.The accompanying test file,
useSessionPersistence.test.ts, has been corrected and activated. It thoroughly tests the hook's behavior by mocking thefsandprocessmodules, ensuring that history is correctly loaded, saved, and filtered without performing actual file I/O during test execution.Additionally, a new
MessageType.TOOL_GROUPhas been added totypes.ts.Reviewer Test Plan
feat/session-persistencebranch.npm installto ensure all dependencies are installed.npm run preflight. All tests, linting, and build steps should pass.npx gemini.Ctrl+Ctwice)..gemini/session.jsonfile was created in your current directory and that it contains the history of your user prompts and Gemini's responses.npx gemini.session.persistencein your Gemini config file, then repeat the steps above. Confirm that history is no longer saved or loaded.Testing Matrix
Linked issues / bugs
Closes #4205