Write cell states to files #2668
Conversation
Fixes #2427 and fixes #2463 Adds a column into the cell state database to allow saving patterns as files, referenced by a file name. This also adds support for synchronized pattern collections to populate the cell state repository, with the list of extract patterns. Autosaved cell states are saved into a specific autosaved cell state folder, which is kept separate from the synchronized pattern collection folders. This allows more easily creating, updating, and removing both pattern collections and the associated cell states. Future work includes: - Allowing a working space for explicitly saved and imported patterns - Allow browsing/searching/deleting patterns - Allow configuring pruning frequency
There was a problem hiding this comment.
Summary of Changes
Hello @alexvanyo, 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 fundamentally changes how cell states are stored and managed. Instead of embedding the serialized cell state directly in the database, they are now saved as files on the file system. The database is updated to store metadata and pointers to these files. This change facilitates the integration of synchronized pattern collections by allowing patterns within archives to be extracted and managed as individual cell states linked to their source collection. A background cleanup mechanism is also introduced to manage autosaved states stored as files.
Highlights
- File-based Cell State Storage: Cell states, including autosaved ones and those from synchronized pattern collections, are now stored in files on disk rather than directly in the database. The database now stores metadata and a reference to the file path.
- Pattern Collection Synchronization: Synchronizing a pattern collection (from a zip file) now extracts the contained patterns to a dedicated folder on disk and creates corresponding
CellStateentries in the database, linked to the pattern collection and referencing the extracted files. - Cell State Cleanup: Introduced a daily background job (using Android WorkManager) to prune autosaved cell state files that are no longer referenced in the database, helping manage storage space.
- Database Schema Update: The
cellStatetable in the database has been updated to includeserializedCellStateFile(nullable) andpatternCollectionId(nullable, with a foreign key constraint) columns. A migration is included to handle existing data.
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 configureGemini 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 introduces significant changes to how cell states are stored and managed, moving from direct database storage to file-based storage for autosaved states and patterns from collections. It also adds support for synchronizing pattern collections and includes a cleanup mechanism for unused autosaved files. The changes involve database schema updates, new repository methods, WorkManager integration for cleanup, and corresponding test updates. Overall, the approach aligns with the PR's goals, but there are a few critical and high-severity issues related to file path handling, pruning logic, and potential null data issues in the database schema that should be addressed.
| // Delete the cell state before deleting the underlying file to avoid a race where the | ||
| // file no longer exists | ||
| cellStateQueries.deleteCellState(databaseCellState.id) | ||
| fileSystem.delete(serializedCellStateFile) |
There was a problem hiding this comment.
The file deletion here is using the relative path stored in the database (serializedCellStateFile) directly with fileSystem.delete. This will attempt to delete a file relative to the root of the fileSystem, not relative to the persistedDataPath. The path should be constructed by combining persistedDataPath and serializedCellStateFile.
| fileSystem.delete(serializedCellStateFile) | |
| fileSystem.delete(persistedDataPath / serializedCellStateFile) |
| lines = checkNotNull(cellState.serializedCellState) { | ||
| "No serialized cell state or saved file!" | ||
| }.lineSequence(), |
There was a problem hiding this comment.
The code assumes that if serializedCellStateFile is null, the serializedCellState must contain the data. However, the database schema allows both to be null. If both are null, this checkNotNull will throw an exception, but it only checks cellState.serializedCellState. Consider restructuring this logic to explicitly handle the case where both serializedCellState and serializedCellStateFile are null, perhaps returning DeserializationResult.Unsuccessful or logging an error.
| maxAge: Duration = Duration.ZERO, | ||
| ) = withContext(dispatchers.IO) { | ||
| // Capture the timestamp to compare duration against | ||
| val pruningTimestamp = clock.now() | ||
| // Get the list of all current autosaved cell states and find their corresponding files | ||
| val currentAutosavedCellStates = cellStateQueries.getAutosavedCellStates().executeAsList() | ||
| .mapNotNull { it.serializedCellStateFile } | ||
| .toSet() | ||
| fileSystem.listOrNull(persistedDataPath / autosavedCellStatesFolder) | ||
| .orEmpty() | ||
| .all { file -> | ||
| try { | ||
| if (file.relativeTo(persistedDataPath).toString() !in currentAutosavedCellStates) { | ||
| val lastModifiedTime = fileSystem | ||
| .metadata(file) | ||
| .lastModifiedAtMillis | ||
| ?.let(Instant::fromEpochMilliseconds) ?: pruningTimestamp | ||
| if (pruningTimestamp - lastModifiedTime > maxAge) { | ||
| fileSystem.delete(file) | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation prunes unused autosaved files immediately due to the default maxAge = Duration.ZERO and the comparison logic. While this fulfills a basic cleanup need, it doesn't allow for retaining recent unused files (e.g., for a few days) as might be desired for a configurable pruning frequency feature. Consider adjusting the default maxAge to a more typical retention period (e.g., 7 days) or making the parameter mandatory if immediate deletion is not the final intended behavior. The comparison pruningTimestamp - lastModifiedTime > maxAge correctly identifies files older than maxAge relative to the pruning time.
| serializedCellState TEXT, | ||
| serializedCellStateFile TEXT, |
There was a problem hiding this comment.
The schema allows both serializedCellState and serializedCellStateFile to be null. This would result in a database entry with no associated cell state data, which would likely cause issues when trying to load it. Consider adding a database constraint or handling this case explicitly in the repository to ensure at least one of these fields is non-null when a cell state is saved.
Fixes #2427 and fixes #2463
Adds a column into the cell state database to allow saving patterns as files, referenced by a file name.
This also adds support for synchronized pattern collections to populate the cell state repository, with the list of extract patterns.
Autosaved cell states are saved into a specific autosaved cell state folder, which is kept separate from the synchronized pattern collection folders. This allows more easily creating, updating, and removing both pattern collections and the associated cell states.
Future work includes: