-
Notifications
You must be signed in to change notification settings - Fork 492
fix: cleanup sessions from SQLite using separate store instance #3056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: cleanup sessions from SQLite using separate store instance #3056
Conversation
…ister Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote canceled.
|
✅ Deploy Preview for hyprnote-storybook canceled.
|
✅ Deploy Preview for howto-fix-macos-audio-selection canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Devin Review found 1 potential issue
- Remove incorrect approach that deleted from store before SessionPersister could save - Add cleanupSqliteSessions() that runs after all persisters initialize - Delete directly from SQLite using SQL commands, not from the store - This ensures sessions are migrated to file system before being deleted from SQLite Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Devin Review found 1 potential issue
| const SESSION_TABLES = [ | ||
| "sessions", | ||
| "mapping_session_participant", | ||
| "tags", | ||
| "mapping_tag_session", | ||
| "transcripts", | ||
| "enhanced_notes", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Tags table deletion may delete non-session-related tags
The SESSION_TABLES array at initialize.ts:26-32 includes the tags table, and the cleanup deletes all entries matching tags/% from SQLite.
However, tags might be a shared resource used by entities other than sessions. Deleting all tags when cleaning up session data could cause data loss for other features that rely on tags.
The PR description explicitly calls this out: "Verify tags table deletion is correct - The tags table is included in SESSION_TABLES, but tags might be shared across other entities. Confirm that deleting all tags is the intended behavior."
Recommendation: Review whether tags are session-specific or shared. If shared, remove 'tags' from SESSION_TABLES or only delete tags that are linked to sessions via the mapping_tag_session table.
Was this helpful? React with 👍 or 👎 to provide feedback.
…of SQL The json-mode persister stores the entire store as a single JSON blob, so we can't delete individual rows using SQL. Instead, we need to: 1. Load the store from SQLite 2. Delete session-related rows using store.delRow() 3. Save the modified store back to SQLite This follows the same pattern as migrateWordsAndHintsToTranscripts. Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
1 similar comment
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
Creates a temporary TinyBase store instance to perform the cleanup, avoiding modifications to the main store that would trigger other persisters (like SessionPersister) to react to the changes. The cleanup now: 1. Creates a temporary store with the same schema 2. Loads SQLite data into the temporary store 3. Deletes session data from the temporary store 4. Saves back to SQLite and destroys the persister 5. This happens BEFORE the main store loads, so it never sees the session data Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
Summary
Cleans up session-related data from SQLite (db.sqlite) before the main LocalPersister loads, to prevent stale
folder_idvalues from conflicting with the SessionPersister.The following tables are cleared when sessions exist:
sessionsmapping_session_participanttagsmapping_tag_sessiontranscriptsenhanced_notesImplementation approach: The json-mode persister stores the entire store as a single JSON blob in SQLite, so we cannot delete individual rows using SQL. Additionally, modifying the main store directly would trigger other persisters (like SessionPersister) to react to the deletions. Instead, we use a separate temporary store instance:
createMergeableStore()store.delRow()within a transactionUpdates since last revision
Review & Testing Checklist for Human
tagstable deletion is correct - Thetagstable is included in cleanup, but tags might be shared across other entities. Confirm that deleting all tags is the intended behavior.SCHEMA.tableandSCHEMA.valuefrom@hypr/store. Confirm these match the main store's schema.Test Plan
Notes