[Codex] Add SQLite dictionary storage with experimental legacy migration action#2335
[Codex] Add SQLite dictionary storage with experimental legacy migration action#2335bee-san wants to merge 3 commits intoyomidevs:masterfrom
Conversation
bee-san
changed the title
Merging this PR will degrade performance by 48.47%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | Translator.prototype.findKanji - (n=3) |
3.9 ms | 7.6 ms | -48.47% |
Comparing bee-san:yomitan-sqlite-migration-pr (9d42893) with master (78eee11)
Footnotes
-
1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 197ab49637
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (this._db !== null) { | ||
| this._clearCachedStatements(); | ||
| this._db.close(); | ||
| this._db = null; | ||
| this._usesFallbackStorage = false; |
There was a problem hiding this comment.
This method closes the active database before attempting importOpfsDatabase, but there is no recovery path if the import payload is invalid/corrupt and that call throws. In that failure case, _db stays null, so subsequent dictionary operations fail with "Database not open" until the extension is restarted, which turns a single bad backup file selection into a persistent runtime failure.
Useful? React with 👍 / 👎.
| async importDatabase(content) { | ||
| await this._offscreen.sendMessagePromise({ | ||
| action: 'importDictionaryDatabaseOffscreen', | ||
| params: {content: arrayBufferToBase64(content)}, |
There was a problem hiding this comment.
The new offscreen bridge converts the entire SQLite backup payload to a base64 string before messaging, which inflates size and forces additional full-buffer copies. For large dictionary collections, this can cause import/export failures due to memory pressure or message transport limits even though the underlying database export/import logic is valid; using transferable ArrayBuffers or chunked streaming avoids this regression.
Useful? React with 👍 / 👎.
bee-san
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d428938d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| void this._settingsController.application.api.triggerDatabaseUpdated('dictionary', 'import'); | ||
| async _importDatabase(file) { | ||
| const content = await this._readFileArrayBuffer(file); | ||
| await this._settingsController.application.api.importDictionaryDatabase(content); |
There was a problem hiding this comment.
This import path now unconditionally treats the selected file as a SQLite payload and sends it to importDictionaryDatabase, but previous releases exported dictionary backups as Dexie JSON; those existing backups will fail to restore after upgrade because DictionaryDatabase.importDatabase only imports SQLite images via OPFS. Combined with the new file picker filter in ext/settings.html, users who only have pre-SQLite .json backups lose their recovery path.
Useful? React with 👍 / 👎.
1 participant
Human note
AI speaking now
Summary
Testing
npx eslint ext/js/dictionary/dictionary-database.js ext/js/background/backend.js ext/js/comm/api.js ext/js/background/offscreen-proxy.js ext/js/background/offscreen.js ext/js/pages/settings/backup-controller.js types/ext/api.d.ts types/ext/backend.d.ts types/ext/offscreen.d.ts test/database.test.jsnpm run test:unit -- test/database.test.js -t 'Migrates dictionaries from legacy IndexedDB into SQLite only when requested|Skips legacy IndexedDB migration when SQLite already has dictionaries'