feat: encrypt sqlite database#1659
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (6)
📝 WalkthroughWalkthroughAdds optional SQLCipher encryption for the app DB: docs/spec/tasks, DatabaseSecurityPresenter with guarded migration and safeStorage wrapping, startup unlock via splash UI and IPC, sensitive-config migration into ChangesSQLite Database Encryption
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/main/presenter/SyncConfigImportService.test.ts (1)
75-84:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrack migration flags per id in the mock instead of one global boolean.
hasConfigMigration(id)currently ignores id semantics (same boolean for all ids), so tests can miss id-specific migration bugs.Proposed fix
type MockState = { @@ - hasMigration: boolean + migrationIds: Set<string> } @@ - hasMigration: false + migrationIds: new Set() }) @@ hasConfigMigration(id?: string): boolean { - if (id === 'sensitive-config-sqlite-v1') { - return this.state.hasMigration - } - return this.state.hasMigration + if (!id) { + return this.state.migrationIds.size > 0 + } + return this.state.migrationIds.has(id) } - markConfigMigrationApplied(): void { - this.state.hasMigration = true + markConfigMigrationApplied(id = 'legacy-config-sqlite-v1'): void { + this.state.migrationIds.add(id) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/presenter/SyncConfigImportService.test.ts` around lines 75 - 84, The mock currently uses a single boolean (state.hasMigration) for all ids; change it to track migrations per id by replacing state.hasMigration with a Set or Map (e.g., migratedIds: Set<string>), update hasConfigMigration(id?: string) to return migratedIds.has(id) (or false when id is missing), and change markConfigMigrationApplied() to accept an id parameter (markConfigMigrationApplied(id: string)) that adds the id to migratedIds; update any tests calling these methods to pass the id so migration flags are id-specific.src/main/routes/index.ts (1)
285-295:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRequire a real
sqlitePresenterbefore exposing the encryption routes.
createMainKernelRouteRuntime()still allowssqlitePresenterto be omitted and replaces it with a minimal settings-activity stub, but the new encryption routes immediately cast that field toSQLitePresenterand rely on the full migration API. Any runtime or test that builds without a realsqlitePresenterwill now pass type-checking and then fail at runtime on enable/change/disable.🐛 Suggested direction
export function createMainKernelRouteRuntime(deps: { configPresenter: IConfigPresenter llmProviderPresenter: ILlmProviderPresenter agentSessionPresenter: IAgentSessionPresenter skillPresenter: ISkillPresenter mcpPresenter: IMCPPresenter syncPresenter: ISyncPresenter upgradePresenter: IUpgradePresenter dialogPresenter: IDialogPresenter toolPresenter: IToolPresenter - sqlitePresenter?: ISQLitePresenter + sqlitePresenter: SQLitePresenter windowPresenter: IWindowPresenter devicePresenter: IDevicePresenter projectPresenter: IProjectPresenter filePresenter: IFilePresenter workspacePresenter: IWorkspacePresenter @@ return { @@ - sqlitePresenter: - deps.sqlitePresenter ?? - ({ - recordSettingsActivity: async (input: SettingsActivityInput) => ({ - id: 'noop', - category: input.category, - action: input.action, - targetType: input.targetType, - targetId: input.targetId ?? null, - targetLabel: input.targetLabel ?? '', - routeName: input.routeName ?? null, - routeParams: input.routeParams ?? {}, - summaryKey: input.summaryKey, - summaryParams: input.summaryParams ?? {}, - createdAt: Date.now() - }), - listSettingsActivity: async () => [] - } as unknown as ISQLitePresenter), + sqlitePresenter: deps.sqlitePresenter, @@ - sqlitePresenter: runtime.sqlitePresenter as unknown as SQLitePresenter, + sqlitePresenter: runtime.sqlitePresenter, @@ - sqlitePresenter: runtime.sqlitePresenter as unknown as SQLitePresenter, + sqlitePresenter: runtime.sqlitePresenter, @@ - sqlitePresenter: runtime.sqlitePresenter as unknown as SQLitePresenter, + sqlitePresenter: runtime.sqlitePresenter,Also applies to: 317-334, 1444-1508
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/routes/index.ts` around lines 285 - 295, The new encryption routes assume a full SQLitePresenter but createMainKernelRouteRuntime still allows sqlitePresenter to be omitted and substituted with a stub; update createMainKernelRouteRuntime (and any route registration code that casts sqlitePresenter to SQLitePresenter) to first verify that sqlitePresenter is a real SQLitePresenter (not the minimal settings-activity stub) before registering the enable/change/disable encryption routes—if it's missing or is the stub, do not register those routes (or throw/return an explicit error), and log/propagate a clear message so tests/runtimes that construct the kernel without a real sqlitePresenter don’t hit runtime cast failures. Ensure references to sqlitePresenter and SQLitePresenter and the encryption route registration code are updated accordingly.
🧹 Nitpick comments (2)
src/main/presenter/syncPresenter/index.ts (1)
548-576: 💤 Low valueError messages don't distinguish between encryption mismatch scenarios.
The
resolveBackupDatabasePassword(line 556) andassertOverwriteEncryptionCompatible(line 574) both throw generic'sync.error.importFailed'without indicating the specific cause:
- Encrypted backup but no current password set
- Encryption state mismatch between backup and current database
While the code correctly prevents incompatible imports and restores from temp backup on failure, users won't know why the import failed or how to fix it. Consider using distinct error keys for better UX.
Suggested improvement for clearer error messages
private resolveBackupDatabasePassword( manifest: SyncBackupManifest | null, activeDatabasePassword: string | undefined ): string | undefined { if (!manifest?.databaseEncrypted) { return undefined } if (!activeDatabasePassword) { - throw new Error('sync.error.importFailed') + throw new Error('sync.error.encryptedBackupNoPassword') } return activeDatabasePassword } private assertOverwriteEncryptionCompatible( backupDbType: BackupDbSource['type'], importMode: ImportMode, manifest: SyncBackupManifest | null, activeDatabasePassword: string | undefined ): void { if (backupDbType !== 'agent' || importMode !== ImportMode.OVERWRITE) { return } const backupDatabaseEncrypted = manifest?.databaseEncrypted === true const activeDatabaseEncrypted = Boolean(activeDatabasePassword) if (backupDatabaseEncrypted !== activeDatabaseEncrypted) { - throw new Error('sync.error.importFailed') + throw new Error('sync.error.encryptionStateMismatch') } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/syncPresenter/index.ts` around lines 548 - 576, Change the generic error throws to distinct, descriptive error keys so callers can surface precise UX messages: in resolveBackupDatabasePassword replace the throw in the missing-password case with a specific error (e.g. 'sync.error.missingDatabasePassword' or similar) and in assertOverwriteEncryptionCompatible replace the throw for encryption-state mismatch with a different key (e.g. 'sync.error.encryptionMismatch'); update any code that catches 'sync.error.importFailed' to handle these new error keys (or let them bubble) so the UI can show the exact cause. Ensure the modifications are applied in the methods resolveBackupDatabasePassword and assertOverwriteEncryptionCompatible and keep the existing guard logic intact.docs/architecture/sqlite-database-encryption/plan.md (1)
1-1: ⚡ Quick winMove this feature SDD doc to
docs/features/sqlite-database-encryption/.This change is feature work, but the document lives under
docs/architecture/..., which is reserved for refactors in the repository convention.As per coding guidelines
docs/**/*.md: Create specification-driven development documentation in kebab-case folders:docs/features/<goal>/for new features,docs/issues/<goal>/for bug fixes,docs/architecture/<goal>/for refactors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/sqlite-database-encryption/plan.md` at line 1, The document titled "SQLite Database Encryption Plan" is in the architecture folder but is a feature SDD; move this file into a kebab-case features folder named for the goal (e.g., a new docs/features/sqlite-database-encryption/ directory) and update any internal links or TOC references accordingly; ensure the file name and folder use kebab-case and that any front-matter or metadata reflects it is a feature SDD rather than an architecture/refactor doc.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/presenter/databaseSecurityPresenter/index.ts`:
- Around line 240-245: The instance-only flag this.migrationInProgress is
insufficient; switch to a process-wide migration lock keyed to the database file
(use input.sqlitePresenter.getDatabasePath()). Before setting migration state,
acquire a process/global lock (e.g., a lock file next to the DB like
"<dbPath>.migration.lock" using OS file locking or a cross-process mutex
library), fail fast if lock cannot be acquired, and only then proceed with
migration; ensure the lock is always released in a finally/cleanup block and
remove or keep the instance flag only as a local convenience after acquiring the
global lock. Use the same lock acquisition/release around the code paths in
DatabaseSecurityPresenter that currently reference this.migrationInProgress so
concurrent backup/import/reset flows cannot mutate the DB during migration.
- Around line 261-265: The swap can leave agent.db missing if
fs.renameSync(tempPath, dbPath) throws after the active DB was already moved to
rollback; wrap the temp->db rename in its own try/catch and on error immediately
restore the rollback by renaming rollbackPath back to dbPath (and rethrow or
handle the original error), then proceed to cleanup; apply the same pattern to
the other swap block (lines around 286-295) and ensure removeSidecars(dbPath)
runs only after a successful final rename.
- Around line 399-401: qualifyCreateTableSql currently only matches the prefix
"CREATE TABLE " and thus rewrites "CREATE TABLE IF NOT EXISTS ..." into "CREATE
TABLE migration_target.IF NOT EXISTS ..."; update qualifyCreateTableSql to
recognize the optional "IF NOT EXISTS" clause and insert
MIGRATION_TARGET_SCHEMA. before the actual table name (i.e., match
/^CREATE\s+TABLE\s+(IF\s+NOT\s+EXISTS\s+)?/i and replace with `CREATE TABLE
$1${MIGRATION_TARGET_SCHEMA}.`) so the schema appears immediately before the
table identifier while preserving the optional clause.
In `@src/renderer/settings/components/DataSettings.vue`:
- Around line 237-264: The template shows encryption action buttons when
databaseSecurityStatus is null (unknown) — add a load guard: introduce a boolean
like databaseSecurityStatusLoaded (set true only after getStatus() resolves
successfully, false on error) and update the template conditionals (where
databaseSecurityStatus is checked around the Button blocks) to require
databaseSecurityStatusLoaded before rendering actions; also ensure
isDatabaseSecurityActionDisabled returns true while not loaded or when
getStatus() failed so openDatabaseEncryptionDialog cannot be called until the
real status is known. Use the existing symbols databaseSecurityStatus,
getStatus(), isDatabaseSecurityActionDisabled, and openDatabaseEncryptionDialog
to locate and change the logic.
In `@src/renderer/splash/loading.vue`:
- Around line 189-210: The component keeps the same requestId after
cancelUnlock(), allowing a cancelled request to still be submitted; update
cancelUnlock() (and related state) to retire the unlock request locally by
clearing requestId.value (and any unlock form state like password.value and
unlockSubmitting.value) immediately after sending the
DATABASE_UNLOCK_CANCEL_CHANNEL IPC so the form cannot submit the cancelled
request (ensure submitUnlock() checks requestId.value as it already does).
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Around line 282-319: The Danish locale file contains an untranslated
"databaseEncryption" object (keys like title, description, enabled, disabled,
cipher, systemUnlock, startupUnlock, lastMigration, loading, never, notRequired,
systemUnlockAvailable, systemUnlockUnavailable, systemUnlockMode, manualUnlock,
currentPassword, newPassword, confirmPassword, passwordMismatch,
systemCredentialStore, safeStorageUnavailable, enableButton, changeButton,
disableButton, enabledTitle, changedTitle, disabledTitle, failedTitle,
failedDescription, setPasswordButton, enableDialogTitle, changeDialogTitle,
disableDialogTitle, enableDialogDescription, changeDialogDescription,
disableDialogDescription, cancelButton) that needs Danish translations; update
the values for each key in the databaseEncryption object in
src/renderer/src/i18n/da-DK/settings.json with proper Danish strings replacing
the English text so the encryption flow is fully localized.
In `@src/renderer/src/i18n/fa-IR/settings.json`:
- Around line 349-387: The databaseEncryption block contains English strings in
the fa-IR locale; translate every value in the "databaseEncryption" object
(e.g., title, description, enabled, disabled, cipher, systemUnlock,
startupUnlock, lastMigration, loading, never, notRequired,
systemUnlockAvailable, systemUnlockUnavailable, systemUnlockMode, manualUnlock,
currentPassword, newPassword, confirmPassword, passwordMismatch,
systemCredentialStore, safeStorageUnavailable, enableButton, changeButton,
disableButton, enabledTitle, changedTitle, disabledTitle, failedTitle,
failedDescription, setPasswordButton, enableDialogTitle, changeDialogTitle,
disableDialogTitle, enableDialogDescription, changeDialogDescription,
disableDialogDescription, cancelButton) into Persian (fa-IR) leaving the keys
unchanged; ensure translations are accurate, context-appropriate for a
security/settings flow and preserve RTL conventions, then run the i18n/JSON
linter to confirm formatting.
In `@src/renderer/src/i18n/fr-FR/settings.json`:
- Around line 349-387: The databaseEncryption i18n block is in English;
translate every string value under the "databaseEncryption" key (e.g., "title",
"description", "enabled", "disabled", "cipher", "systemUnlock", "startupUnlock",
"lastMigration", "loading", "never", "notRequired", "systemUnlockAvailable",
"systemUnlockUnavailable", "systemUnlockMode", "manualUnlock",
"currentPassword", "newPassword", "confirmPassword", "passwordMismatch",
"systemCredentialStore", "safeStorageUnavailable", "enableButton",
"changeButton", "disableButton", "enabledTitle", "changedTitle",
"disabledTitle", "failedTitle", "failedDescription", "setPasswordButton",
"enableDialogTitle", "changeDialogTitle", "disableDialogTitle",
"enableDialogDescription", "changeDialogDescription",
"disableDialogDescription", "cancelButton") into correct French, preserving
meaning, punctuation and any technical terms (e.g., SQLCipher, SQLite) and
keeping string keys unchanged so the UI uses the localized values. Ensure
translations are fluent French and retain capitalization/phrasing appropriate
for UI labels and dialog texts.
In `@src/renderer/src/i18n/he-IL/settings.json`:
- Around line 349-387: The he-IL locale's databaseEncryption block (the data key
"databaseEncryption" and its subkeys like "title", "description", "enabled",
"disabled", "cipher", "systemUnlock", "startupUnlock", "lastMigration",
"loading", "never", "notRequired", "systemUnlockAvailable",
"systemUnlockUnavailable", "systemUnlockMode", "manualUnlock",
"currentPassword", "newPassword", "confirmPassword", "passwordMismatch",
"systemCredentialStore", "safeStorageUnavailable", "enableButton",
"changeButton", "disableButton", "enabledTitle", "changedTitle",
"disabledTitle", "failedTitle", "failedDescription", "setPasswordButton",
"enableDialogTitle", "changeDialogTitle", "disableDialogTitle",
"enableDialogDescription", "changeDialogDescription",
"disableDialogDescription", "cancelButton") is still in English; translate each
value into Hebrew preserving the same keys and intent (including UI tone and
punctuation) so RTL rendering and user dialogs are localized for he-IL users.
In `@src/renderer/src/i18n/ja-JP/settings.json`:
- Around line 349-387: The data.databaseEncryption block (the
"databaseEncryption" object and its nested keys like "title", "description",
"enabled", "disabled", "cipher", "systemUnlock", "startupUnlock", etc.) is still
in English; replace each English string value with the correct Japanese
translations so the UI is fully localized for ja-JP (translate every property:
title, description, enabled/disabled labels, cipher, systemUnlock,
startupUnlock, lastMigration, loading, never, notRequired,
systemUnlockAvailable/Unavailable/Mode, manualUnlock, currentPassword,
newPassword, confirmPassword, passwordMismatch, systemCredentialStore,
safeStorageUnavailable, enableButton/changeButton/disableButton,
enabledTitle/changedTitle/disabledTitle/failedTitle, failedDescription,
setPasswordButton, enableDialogTitle/changeDialogTitle/disableDialogTitle,
enableDialogDescription/changeDialogDescription/disableDialogDescription,
cancelButton) ensuring grammar and tone consistent with existing ja-JP
translations.
In `@src/renderer/src/i18n/ko-KR/settings.json`:
- Around line 349-387: The ko-KR locale contains an untranslated block under the
"databaseEncryption" key; translate each English string value (title,
description, enabled, disabled, cipher, systemUnlock, startupUnlock,
lastMigration, loading, never, notRequired, systemUnlockAvailable,
systemUnlockUnavailable, systemUnlockMode, manualUnlock, currentPassword,
newPassword, confirmPassword, passwordMismatch, systemCredentialStore,
safeStorageUnavailable, enableButton, changeButton, disableButton, enabledTitle,
changedTitle, disabledTitle, failedTitle, failedDescription, setPasswordButton,
enableDialogTitle, changeDialogTitle, disableDialogTitle,
enableDialogDescription, changeDialogDescription, disableDialogDescription,
cancelButton) into fluent Korean, preserving the same keys and
punctuation/placeholder structure so the app can load the translations without
changing code.
In `@src/renderer/src/i18n/pt-BR/settings.json`:
- Around line 349-387: The pt-BR locale contains untranslated English values
under the "databaseEncryption" object; translate every value for keys like
title, description, enabled/disabled, cipher, systemUnlock, startupUnlock,
lastMigration, loading, never, notRequired,
systemUnlockAvailable/Unavailable/Mode, manualUnlock, currentPassword,
newPassword, confirmPassword, passwordMismatch, systemCredentialStore,
safeStorageUnavailable, enableButton/changeButton/disableButton,
enabledTitle/changedTitle/disabledTitle/failedTitle/failedDescription,
setPasswordButton, enableDialogTitle/changeDialogTitle/disableDialogTitle,
enableDialogDescription/changeDialogDescription/disableDialogDescription, and
cancelButton into natural pt-BR (preserve punctuation, capitalization and any
technical terms like "SQLite" and "SQLCipher"); ensure translations keep meaning
for dialogs and button labels and do not alter key names.
In `@src/renderer/src/i18n/ru-RU/settings.json`:
- Around line 349-387: The databaseEncryption block in ru-RU/settings.json
contains English strings; update all keys under "databaseEncryption" (e.g.,
title, description, enabled, disabled, cipher, systemUnlock, startupUnlock,
lastMigration, loading, never, notRequired, systemUnlockAvailable,
systemUnlockUnavailable, systemUnlockMode, manualUnlock, currentPassword,
newPassword, confirmPassword, passwordMismatch, systemCredentialStore,
safeStorageUnavailable, enableButton, changeButton, disableButton, enabledTitle,
changedTitle, disabledTitle, failedTitle, failedDescription, setPasswordButton,
enableDialogTitle, changeDialogTitle, disableDialogTitle,
enableDialogDescription, changeDialogDescription, disableDialogDescription,
cancelButton) to proper Russian translations so the Russian locale fully mirrors
the new data.databaseEncryption UI text. Ensure translations preserve meaning
and any placeholders or punctuation.
In `@src/renderer/src/i18n/zh-HK/settings.json`:
- Around line 349-387: The new databaseEncryption block is written mostly in
Simplified Chinese; convert all strings to Traditional Chinese to match the
zh-HK locale (update values for keys under databaseEncryption such as title,
description, enabled/disabled, cipher, systemUnlock, startupUnlock,
lastMigration, loading, never, notRequired, systemUnlockAvailable,
systemUnlockUnavailable, systemUnlockMode, manualUnlock, currentPassword,
newPassword, confirmPassword, passwordMismatch, systemCredentialStore,
safeStorageUnavailable, enableButton, changeButton, disableButton, enabledTitle,
changedTitle, disabledTitle, failedTitle, failedDescription, setPasswordButton,
enableDialogTitle, changeDialogTitle, disableDialogTitle,
enableDialogDescription, changeDialogDescription, disableDialogDescription,
cancelButton). Ensure Traditional Chinese wording and locale-appropriate terms
(e.g., 使用「系統」、「儲存」、「取消」等) and preserve punctuation and meaning.
In `@src/renderer/src/i18n/zh-TW/settings.json`:
- Around line 349-387: The databaseEncryption block contains Simplified Chinese;
convert every value under "databaseEncryption" to Traditional Chinese for the
zh-TW locale (e.g., change 数据/系统/加载/密码/启用/关闭/etc. to 資料/系統/載入/密碼/啟用/關閉),
updating keys such as title, description, enabled, disabled, loading, never,
systemUnlockAvailable, systemUnlockUnavailable, manualUnlock, currentPassword,
newPassword, confirmPassword, passwordMismatch, systemCredentialStore,
safeStorageUnavailable, enableButton, changeButton, disableButton, enabledTitle,
changedTitle, disabledTitle, failedTitle, failedDescription, setPasswordButton,
enableDialogTitle, changeDialogTitle, disableDialogTitle,
enableDialogDescription, changeDialogDescription, disableDialogDescription and
cancelButton to use consistent Traditional wording and vocabulary (e.g., 資料庫,
加密, 密碼, 載入中, 從未, 不需要, 可用, 不可用, 手動輸入密碼, 當前密碼, 新的 SQLite 密碼, 確認密碼, 兩次輸入的密碼不一致,
系統憑證庫說明, 啟用加密, 修改密碼, 關閉加密, 取消).
In `@test/renderer/components/DataSettings.test.ts`:
- Around line 351-357: The test currently uses optional chaining when setting
password inputs which can silently skip if inputs are missing; update the
section where you call wrapper.findAll('input[type="password"]') and instead
assert that inputs.length >= 2 (or that newPasswordInput and
confirmPasswordInput are truthy) before proceeding, then call setValue directly
on newPasswordInput and confirmPasswordInput (no ?. operator) and continue to
call findDatabaseEncryptionButton as before; reference the existing variables
wrapper, inputs, newPasswordInput, confirmPasswordInput, and
findDatabaseEncryptionButton to locate and modify the code.
---
Outside diff comments:
In `@src/main/routes/index.ts`:
- Around line 285-295: The new encryption routes assume a full SQLitePresenter
but createMainKernelRouteRuntime still allows sqlitePresenter to be omitted and
substituted with a stub; update createMainKernelRouteRuntime (and any route
registration code that casts sqlitePresenter to SQLitePresenter) to first verify
that sqlitePresenter is a real SQLitePresenter (not the minimal
settings-activity stub) before registering the enable/change/disable encryption
routes—if it's missing or is the stub, do not register those routes (or
throw/return an explicit error), and log/propagate a clear message so
tests/runtimes that construct the kernel without a real sqlitePresenter don’t
hit runtime cast failures. Ensure references to sqlitePresenter and
SQLitePresenter and the encryption route registration code are updated
accordingly.
In `@test/main/presenter/SyncConfigImportService.test.ts`:
- Around line 75-84: The mock currently uses a single boolean
(state.hasMigration) for all ids; change it to track migrations per id by
replacing state.hasMigration with a Set or Map (e.g., migratedIds: Set<string>),
update hasConfigMigration(id?: string) to return migratedIds.has(id) (or false
when id is missing), and change markConfigMigrationApplied() to accept an id
parameter (markConfigMigrationApplied(id: string)) that adds the id to
migratedIds; update any tests calling these methods to pass the id so migration
flags are id-specific.
---
Nitpick comments:
In `@docs/architecture/sqlite-database-encryption/plan.md`:
- Line 1: The document titled "SQLite Database Encryption Plan" is in the
architecture folder but is a feature SDD; move this file into a kebab-case
features folder named for the goal (e.g., a new
docs/features/sqlite-database-encryption/ directory) and update any internal
links or TOC references accordingly; ensure the file name and folder use
kebab-case and that any front-matter or metadata reflects it is a feature SDD
rather than an architecture/refactor doc.
In `@src/main/presenter/syncPresenter/index.ts`:
- Around line 548-576: Change the generic error throws to distinct, descriptive
error keys so callers can surface precise UX messages: in
resolveBackupDatabasePassword replace the throw in the missing-password case
with a specific error (e.g. 'sync.error.missingDatabasePassword' or similar) and
in assertOverwriteEncryptionCompatible replace the throw for encryption-state
mismatch with a different key (e.g. 'sync.error.encryptionMismatch'); update any
code that catches 'sync.error.importFailed' to handle these new error keys (or
let them bubble) so the UI can show the exact cause. Ensure the modifications
are applied in the methods resolveBackupDatabasePassword and
assertOverwriteEncryptionCompatible and keep the existing guard logic intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7352d0d3-b18f-40f4-af50-027d417adfbc
📒 Files selected for processing (44)
docs/architecture/sqlite-database-encryption/plan.mddocs/architecture/sqlite-database-encryption/spec.mddocs/architecture/sqlite-database-encryption/tasks.mdelectron.vite.config.tssrc/main/presenter/configPresenter/configDbStores.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/databaseSecurityPresenter/index.tssrc/main/presenter/index.tssrc/main/presenter/lifecyclePresenter/SplashWindowManager.tssrc/main/presenter/lifecyclePresenter/hooks/init/databaseInitHook.tssrc/main/presenter/lifecyclePresenter/index.tssrc/main/presenter/sqlitePresenter/connectionConfig.tssrc/main/presenter/sqlitePresenter/importData.tssrc/main/presenter/sqlitePresenter/index.tssrc/main/presenter/sqlitePresenter/tables/configTables.tssrc/main/presenter/syncPresenter/configImportService.tssrc/main/presenter/syncPresenter/index.tssrc/main/routes/index.tssrc/preload/splash-preload.tssrc/renderer/api/DatabaseSecurityClient.tssrc/renderer/api/index.tssrc/renderer/settings/components/DataSettings.vuesrc/renderer/splash/loading.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/contracts/databaseSecurity.tssrc/shared/contracts/routes.tssrc/shared/contracts/routes/database-security.routes.tssrc/shared/types/presenters/legacy.presenters.d.tstest/main/presenter/SyncConfigImportService.test.tstest/main/presenter/databaseSecurityPresenter.test.tstest/main/presenter/lifecyclePresenter/SplashWindowManager.display.test.tstest/main/presenter/sqlitePresenter.connectionConfig.test.tstest/renderer/components/DataSettings.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/syncPresenter/index.ts (1)
441-442:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEncrypted backups stop being restorable after a password rotation.
The manifest only records that a backup is encrypted, and
resolveBackupDatabasePassword()always reuses the current active password. Any backup created beforechangePassword()ordisableEncryption()will still be encrypted with the old key, so both overwrite and incremental import can fail even though the backup itself is valid. This needs per-backup password recovery/prompting instead of assuming the live database password can always open the archived copy.Also applies to: 550-560
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/presenter/syncPresenter/index.ts` around lines 441 - 442, The manifest currently only records databaseEncrypted/databaseCipher and resolveBackupDatabasePassword() always uses getActiveDatabasePassword(), which breaks restore after changePassword()/disableEncryption(); update the backup creation flow (where databaseEncrypted/databaseCipher are set) to persist a per-backup password identifier (e.g., passwordVersion, keySalt or an encrypted key blob) and/or passwordHint and store the exact cipher metadata, then change resolveBackupDatabasePassword() to first attempt recovery using the per-backup metadata (lookup the matching stored key/version or prompt the user for that backup's password) and only fall back to the active password if no per-backup credential exists; update any import/overwrite code paths (the ones around lines ~550-560) to pass the per-backup credential into the database open routine rather than relying on getActiveDatabasePassword().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/settings/components/DataSettings.vue`:
- Around line 987-997: In refreshDatabaseSecurityStatus, do not overwrite
databaseSecurityStatus with null on errors; instead preserve the last known
value or set a clear sentinel like 'unknown' so the UI doesn't render a false
"disabled" state. Modify the catch block for databaseSecurityClient.getStatus()
to leave databaseSecurityStatus.value untouched (or assign 'unknown'), set
hasDatabaseSecurityStatusError.value = true and
isDatabaseSecurityStatusLoaded.value = false, and ensure any downstream UI uses
the 'unknown' sentinel to show an error/unknown state rather than treating null
as "disabled".
---
Outside diff comments:
In `@src/main/presenter/syncPresenter/index.ts`:
- Around line 441-442: The manifest currently only records
databaseEncrypted/databaseCipher and resolveBackupDatabasePassword() always uses
getActiveDatabasePassword(), which breaks restore after
changePassword()/disableEncryption(); update the backup creation flow (where
databaseEncrypted/databaseCipher are set) to persist a per-backup password
identifier (e.g., passwordVersion, keySalt or an encrypted key blob) and/or
passwordHint and store the exact cipher metadata, then change
resolveBackupDatabasePassword() to first attempt recovery using the per-backup
metadata (lookup the matching stored key/version or prompt the user for that
backup's password) and only fall back to the active password if no per-backup
credential exists; update any import/overwrite code paths (the ones around lines
~550-560) to pass the per-backup credential into the database open routine
rather than relying on getActiveDatabasePassword().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3521f103-b89d-4b87-bd74-37aa8b55c8d2
📒 Files selected for processing (31)
src/main/presenter/databaseSecurityPresenter/index.tssrc/main/presenter/syncPresenter/index.tssrc/main/routes/index.tssrc/renderer/settings/components/DataSettings.vuesrc/renderer/splash/loading.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/da-DK/sync.jsonsrc/renderer/src/i18n/en-US/sync.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fa-IR/sync.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/fr-FR/sync.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/he-IL/sync.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ja-JP/sync.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ko-KR/sync.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/pt-BR/sync.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/ru-RU/sync.jsonsrc/renderer/src/i18n/zh-CN/sync.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-HK/sync.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/i18n/zh-TW/sync.jsontest/main/presenter/SyncConfigImportService.test.tstest/main/presenter/SyncPresenter.test.tstest/main/presenter/databaseSecurityPresenter.test.tstest/renderer/components/DataSettings.test.ts
✅ Files skipped from review due to trivial changes (16)
- src/renderer/src/i18n/fr-FR/sync.json
- src/renderer/src/i18n/he-IL/sync.json
- src/renderer/src/i18n/pt-BR/sync.json
- src/renderer/src/i18n/zh-HK/sync.json
- src/renderer/src/i18n/ru-RU/sync.json
- src/renderer/src/i18n/ko-KR/sync.json
- src/renderer/src/i18n/en-US/sync.json
- src/renderer/src/i18n/da-DK/sync.json
- src/renderer/src/i18n/zh-TW/sync.json
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/i18n/fr-FR/settings.json
- src/renderer/src/i18n/zh-CN/sync.json
- src/renderer/src/i18n/zh-TW/settings.json
- src/renderer/src/i18n/da-DK/settings.json
- src/renderer/src/i18n/zh-HK/settings.json
- src/renderer/src/i18n/he-IL/settings.json
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/main/presenter/lifecyclePresenter/SplashWindowManager.display.test.ts (1)
14-72: 💤 Low valueOptional: Consider adding missing mock properties for future test coverage.
The
MockBrowserWindowmock is missing:
webContents.idproperty (used byisSplashSender()in the implementation)once()method (used byforceShowSplash()whensplashReadyToShowis false)Current tests don't exercise these paths, but adding them would make the mock more complete for future IPC-related tests.
🔧 Suggested additions
class MockBrowserWindow { public visible = false public destroyed = false + private static nextWebContentsId = 1 public readonly show = vi.fn(() => { this.visible = true }) public readonly focus = vi.fn() public readonly close = vi.fn(() => { this.destroyed = true this.emit('closed') }) public readonly loadURL = vi.fn((url: string) => { return splashLoadMocks.loadURL?.(url) ?? Promise.resolve() }) public readonly loadFile = vi.fn((filePath: string) => { return splashLoadMocks.loadFile?.(filePath) ?? Promise.resolve() }) public readonly webContents = { + id: MockBrowserWindow.nextWebContentsId++, on: vi.fn((event: string, handler: (...args: unknown[]) => void) => { const handlers = this.webContentsHandlers.get(event) ?? [] handlers.push(handler) this.webContentsHandlers.set(event, handlers) }), send: vi.fn() } private readonly handlers = new Map<string, Array<(...args: unknown[]) => void>>() private readonly webContentsHandlers = new Map<string, Array<(...args: unknown[]) => void>>() // ... existing code ... + once(event: string, handler: (...args: unknown[]) => void) { + const wrappedHandler = (...args: unknown[]) => { + const handlers = this.handlers.get(event) ?? [] + const index = handlers.indexOf(wrappedHandler) + if (index !== -1) handlers.splice(index, 1) + handler(...args) + } + this.on(event, wrappedHandler) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/main/presenter/lifecyclePresenter/SplashWindowManager.display.test.ts` around lines 14 - 72, The MockBrowserWindow lacks webContents.id and a once() handler required by code paths like isSplashSender() and forceShowSplash(); update the MockBrowserWindow class to add a numeric webContents.id property and implement once(event, handler) on both the MockBrowserWindow instance and its webContents (store handlers in the existing handlers/webContentsHandlers maps and invoke them exactly once when emit/emitWebContents is called) so tests can exercise IPC sender checks and the splashReadyToShow false branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/main/presenter/lifecyclePresenter/SplashWindowManager.display.test.ts`:
- Around line 14-72: The MockBrowserWindow lacks webContents.id and a once()
handler required by code paths like isSplashSender() and forceShowSplash();
update the MockBrowserWindow class to add a numeric webContents.id property and
implement once(event, handler) on both the MockBrowserWindow instance and its
webContents (store handlers in the existing handlers/webContentsHandlers maps
and invoke them exactly once when emit/emitWebContents is called) so tests can
exercise IPC sender checks and the splashReadyToShow false branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afd38e17-d249-413c-bd36-96760cd8405e
📒 Files selected for processing (2)
src/main/presenter/lifecyclePresenter/SplashWindowManager.tstest/main/presenter/lifecyclePresenter/SplashWindowManager.display.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UI
Before:
After:
Verification
Summary by CodeRabbit