From 9d70bc29320b65498af38f2a992bd26641fea74a Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 03:00:45 -0500 Subject: [PATCH 1/7] fix(security): move accounts/credentials to a non-auth-bound database Accounts, credentials, per-account settings and signatures lived in the same libremail.db that SQLCipher encrypts under the auth-bound passphrase when app-lock + encrypted-cache are on. A genuine key invalidation (biometric re-enrollment or lock removal/re-add) made that file undecryptable, and the "clear + re-sync" recovery wiped the accounts and stored credentials along with the mail cache, dropping the user into onboarding (issue #111). Move those four tables into a new plaintext AccountDatabase (libremail-accounts.db) that is never bound to the auth key. Credentials stay AES-GCM sealed at the column level by the surviving non-auth KeystoreCrypto master key, so the only secret never touches disk in the clear. A cache-key invalidation now wipes only libremail.db; the user stays signed in. - AccountDatabase (v1) + AccountDatabaseModule; the cache DB drops to v15 via MIGRATION_14_15. DAOs are unchanged and re-provided from the new DB, so no injection site changes. - AccountDataMigrator performs the one-time cross-DB copy at startup, before Room opens either database. It attaches the cache (with its resolved passphrase, so an encrypted source is handled) and copies with INSERT OR IGNORE. It is crash-safe and idempotent: the source is dropped only by MIGRATION_14_15 after the copy, a re-run never duplicates or overwrites, and it runs after the clear-pending wipe so an unrecoverable cache degrades to "nothing to move" instead of blocking. - Exported schemas for both databases; MigrationTest asserts the account rows/backfills survive to v14 then are dropped at v15, plus a dedicated 14->15 test. AccountDataMigratorTest covers the plaintext + encrypted copy, idempotency, a DDL-vs-Room drift guard, and end-to-end survival of a simulated cache wipe. Closes #111 Co-Authored-By: Claude Fable 5 --- .../1.json | 234 ++++++++++++++++++ .../data/local/AccountDataMigratorTest.kt | 229 +++++++++++++++++ .../data/local/AccountDatabaseTest.kt | 85 +++++++ .../data/local/LibreMailDatabaseTest.kt | 31 +-- .../org/libremail/data/local/MigrationTest.kt | 85 +++++-- .../libremail/ui/compose/ComposeScreenTest.kt | 6 +- .../ui/settings/AccountSettingsScreenTest.kt | 4 +- .../data/local/AccountDataMigrator.kt | 193 +++++++++++++++ .../libremail/data/local/AccountDatabase.kt | 51 ++++ .../data/local/DatabaseEncryption.kt | 10 +- .../org/libremail/data/local/DatabaseFiles.kt | 7 + .../libremail/data/local/LibreMailDatabase.kt | 28 +-- .../org/libremail/data/local/Migrations.kt | 22 ++ .../org/libremail/di/AccountDatabaseModule.kt | 53 ++++ .../kotlin/org/libremail/di/DatabaseModule.kt | 32 ++- 15 files changed, 979 insertions(+), 91 deletions(-) create mode 100644 app/schemas/org.libremail.data.local.AccountDatabase/1.json create mode 100644 app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt create mode 100644 app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt create mode 100644 app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt create mode 100644 app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt create mode 100644 app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt diff --git a/app/schemas/org.libremail.data.local.AccountDatabase/1.json b/app/schemas/org.libremail.data.local.AccountDatabase/1.json new file mode 100644 index 0000000..dd0f0fa --- /dev/null +++ b/app/schemas/org.libremail.data.local.AccountDatabase/1.json @@ -0,0 +1,234 @@ +{ + "formatVersion": 1, + "database": { + "version": 1, + "identityHash": "f2bbe80e572de72f50869b14aca4c4bb", + "entities": [ + { + "tableName": "accounts", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `email` TEXT NOT NULL, `displayName` TEXT NOT NULL, `authType` TEXT NOT NULL, `imap_host` TEXT NOT NULL, `imap_port` INTEGER NOT NULL, `imap_security` TEXT NOT NULL, `smtp_host` TEXT NOT NULL, `smtp_port` INTEGER NOT NULL, `smtp_security` TEXT NOT NULL, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "email", + "columnName": "email", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "displayName", + "columnName": "displayName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "authType", + "columnName": "authType", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "imap.host", + "columnName": "imap_host", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "imap.port", + "columnName": "imap_port", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "imap.security", + "columnName": "imap_security", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "smtp.host", + "columnName": "smtp_host", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "smtp.port", + "columnName": "smtp_port", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "smtp.security", + "columnName": "smtp_security", + "affinity": "TEXT", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + } + }, + { + "tableName": "credentials", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `encryptedSecret` TEXT NOT NULL, PRIMARY KEY(`accountId`))", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "encryptedSecret", + "columnName": "encryptedSecret", + "affinity": "TEXT", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId" + ] + } + }, + { + "tableName": "account_settings", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `signature` TEXT NOT NULL, `signatureEnabled` INTEGER NOT NULL, `notificationsEnabled` INTEGER NOT NULL, `retentionCount` INTEGER, `retentionMonths` INTEGER, PRIMARY KEY(`accountId`), FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "signature", + "columnName": "signature", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "signatureEnabled", + "columnName": "signatureEnabled", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "notificationsEnabled", + "columnName": "notificationsEnabled", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "retentionCount", + "columnName": "retentionCount", + "affinity": "INTEGER" + }, + { + "fieldPath": "retentionMonths", + "columnName": "retentionMonths", + "affinity": "INTEGER" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId" + ] + }, + "foreignKeys": [ + { + "table": "accounts", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "accountId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "signatures", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, `name` TEXT NOT NULL, `contentHtml` TEXT NOT NULL, `isDefault` INTEGER NOT NULL, PRIMARY KEY(`id`), FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "name", + "columnName": "name", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "contentHtml", + "columnName": "contentHtml", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "isDefault", + "columnName": "isDefault", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_signatures_accountId", + "unique": false, + "columnNames": [ + "accountId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_signatures_accountId` ON `${TABLE_NAME}` (`accountId`)" + } + ], + "foreignKeys": [ + { + "table": "accounts", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "accountId" + ], + "referencedColumns": [ + "id" + ] + } + ] + } + ], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, 'f2bbe80e572de72f50869b14aca4c4bb')" + ] + } +} \ No newline at end of file diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt new file mode 100644 index 0000000..d610200 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import android.content.Context +import androidx.room.Room +import androidx.room.testing.MigrationTestHelper +import androidx.sqlite.db.framework.FrameworkSQLiteOpenHelperFactory +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import org.json.JSONObject +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.data.local.entity.CredentialEntity + +/** + * The one-time move performed by [AccountDataMigrator] (issue #111): copying accounts / credentials / + * per-account settings / signatures out of the cache database into the plaintext [AccountDatabase]. + * + * Exercises the [AccountDataMigrator.copyAccountTables] core directly (the full [AccountDataMigrator] + * also resolves the passphrase and flips the done-flag, which need the real DataStore/Keystore). A v14 + * cache is built with [MigrationTestHelper] from the exported schema, so the copy runs against exactly + * the on-disk shape an upgrading user has. + */ +@RunWith(AndroidJUnit4::class) +class AccountDataMigratorTest { + + @get:Rule + val helper = MigrationTestHelper( + InstrumentationRegistry.getInstrumentation(), + LibreMailDatabase::class.java, + emptyList(), + FrameworkSQLiteOpenHelperFactory(), + ) + + private val context = ApplicationProvider.getApplicationContext() + private val cacheName = "acct-migrator-cache-test.db" + private val accountsName = "acct-migrator-accounts-test.db" + private val cacheFile get() = context.getDatabasePath(cacheName) + private val accountsFile get() = context.getDatabasePath(accountsName) + + // 64 hex chars == a 32-byte SQLCipher passphrase. + private val passphrase = "0123456789abcdef".repeat(4) + + @Before + @After + fun clean() { + listOf(cacheName, accountsName).forEach { name -> + context.deleteDatabase(name) + context.getDatabasePath(name).parentFile + ?.listFiles { f -> f.name.startsWith(name) } + ?.forEach { it.delete() } + } + } + + /** Builds a v14 cache holding one fully-populated account plus a mail row. */ + private fun seedVersion14Cache() { + helper.createDatabase(cacheName, 14).apply { + execSQL( + "INSERT INTO accounts (id, email, displayName, authType, imap_host, imap_port, imap_security, " + + "smtp_host, smtp_port, smtp_security) VALUES ('acct', 'ada@example.org', 'Ada', " + + "'PASSWORD_IMAP', 'imap.example.org', 993, 'SSL_TLS', 'smtp.example.org', 465, 'SSL_TLS')", + ) + execSQL("INSERT INTO credentials (accountId, encryptedSecret) VALUES ('acct', 'sealed-secret')") + execSQL( + "INSERT INTO account_settings (accountId, signature, signatureEnabled, notificationsEnabled, " + + "retentionCount, retentionMonths) VALUES ('acct', 'Cheers', 1, 0, NULL, 6)", + ) + execSQL( + "INSERT INTO signatures (id, accountId, name, contentHtml, isDefault) " + + "VALUES ('sig-1', 'acct', 'Work', '

Regards

', 1)", + ) + execSQL( + "INSERT INTO messages (id, accountId, sender, senderEmail, subject, snippet, body, isHtml, " + + "timestampMillis, isRead, isStarred, folder, inInbox, bodyFetched, uid) VALUES " + + "('acct:INBOX:1', 'acct', 'Ada', 'a@x', 'Hi', '', '', 0, 1, 0, 0, 'INBOX', 1, 0, 1)", + ) + close() + } + } + + private fun openAccountsDb(): AccountDatabase = + Room.databaseBuilder(context, AccountDatabase::class.java, accountsName).build() + + @Test + fun movesEveryAccountTableOutOfAPlaintextCache() = runBlocking { + seedVersion14Cache() + + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + // First open lets Room stamp its identity onto the migrator-created file; reopen so a real + // session's reads run against a fully Room-owned database. + openAccountsDb().apply { + assertEquals("Ada", accountDao().getById("acct")?.displayName) + close() + } + openAccountsDb().apply { + val account = accountDao().getById("acct") + assertEquals("ada@example.org", account?.email) + assertEquals(993, account?.imap?.port) + assertEquals("smtp.example.org", account?.smtp?.host) + assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) + val settings = accountSettingsDao().get("acct") + assertEquals(false, settings?.signatureEnabled) + assertEquals(6, settings?.retentionMonths) + assertNull(settings?.retentionCount) + val signatures = signatureDao().observeForAccount("acct").first() + assertEquals(listOf("Work"), signatures.map { it.name }) + assertTrue("the default flag must round-trip", signatures.single().isDefault) + close() + } + } + + @Test + fun movesAccountsOutOfAnEncryptedCache() = runBlocking { + seedVersion14Cache() + // Turn the cache into the SQLCipher form an app-lock + encrypted-cache user has on disk. + DatabaseEncryption.ensureEncrypted(cacheFile, passphrase) + assertTrue("precondition: the source cache is encrypted", DatabaseEncryption.isEncrypted(cacheFile)) + + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = passphrase, accountsFile = accountsFile) + + openAccountsDb().apply { + assertNotNull(accountDao().getById("acct")) + close() + } + openAccountsDb().apply { + assertEquals("ada@example.org", accountDao().getById("acct")?.email) + assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) + close() + } + } + + @Test + fun reRunningTheCopyIsIdempotentAndKeepsLaterEdits() = runBlocking { + seedVersion14Cache() + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + // Simulate the user editing an account AFTER the migration. + openAccountsDb().apply { + val edited = accountDao().getById("acct")!!.copy(displayName = "Ada Lovelace") + accountDao().upsert(edited) + close() + } + + // A re-run (e.g. after a mid-startup crash before the done-flag was set) must not clobber it. + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + openAccountsDb().apply { + assertEquals(1, accountDao().getAll().size) + assertEquals( + "INSERT OR IGNORE must not overwrite the post-migration edit", + "Ada Lovelace", + accountDao().getById("acct")?.displayName, + ) + close() + } + } + + @Test + fun accountsAndCredentialsSurviveACacheWipe() = runBlocking { + seedVersion14Cache() + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + // The "clear + re-sync" recovery wipes only the cache file; AccountDatabase is a separate file. + context.deleteDatabase(cacheName) + assertTrue("precondition: the cache file is gone", !cacheFile.exists()) + + openAccountsDb().apply { + assertNotNull("the account must outlive a cache wipe (issue #111)", accountDao().getById("acct")) + assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) + // And it is still usable: a fresh credential can be written with no cache present. + credentialDao().upsert(CredentialEntity("acct", "rotated")) + assertEquals("rotated", credentialDao().getById("acct")?.encryptedSecret) + close() + } + } + + @Test + fun migratorDdlMatchesExportedAccountDatabaseSchema() { + val schema = JSONObject( + InstrumentationRegistry.getInstrumentation().context.assets + .open("org.libremail.data.local.AccountDatabase/1.json") + .bufferedReader().use { it.readText() }, + ).getJSONObject("database") + val entities = schema.getJSONArray("entities") + + var checkedIndex = false + for (i in 0 until entities.length()) { + val entity = entities.getJSONObject(i) + val table = entity.getString("tableName") + val expectedCreate = entity.getString("createSql").replace("\${TABLE_NAME}", "`$table`") + assertEquals( + "AccountDataMigrator DDL for `$table` must match the exported AccountDatabase schema", + expectedCreate, + AccountDataMigrator.CREATE_TABLE_SQL[table], + ) + if (entity.has("indices")) { + val indices = entity.getJSONArray("indices") + for (j in 0 until indices.length()) { + val index = indices.getJSONObject(j) + if (index.getString("name") == "index_signatures_accountId") { + assertEquals( + "AccountDataMigrator signatures index must match the exported schema", + index.getString("createSql").replace("\${TABLE_NAME}", "`$table`"), + AccountDataMigrator.SIGNATURES_INDEX_SQL, + ) + checkedIndex = true + } + } + } + } + assertEquals( + "every migrator table DDL must correspond to an exported entity", + AccountDataMigrator.CREATE_TABLE_SQL.keys, + (0 until entities.length()).map { entities.getJSONObject(it).getString("tableName") }.toSet(), + ) + assertTrue("the signatures index must be present in the exported schema", checkedIndex) + } +} diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt new file mode 100644 index 0000000..8ddae89 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import android.content.Context +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import androidx.test.ext.junit.runners.AndroidJUnit4 +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.data.local.entity.AccountEntity +import org.libremail.data.local.entity.AccountSettingsEntity +import org.libremail.data.local.entity.CredentialEntity +import org.libremail.data.local.entity.ServerConfigEmbedded +import org.libremail.data.local.entity.SignatureEntity + +/** + * Behavior of the non-auth [AccountDatabase] that holds accounts, credentials, per-account settings + * and signatures after they were moved out of the cache database (issue #111). Confirms the tables' + * foreign keys still cascade from the account, now that they live together in this database. + */ +@RunWith(AndroidJUnit4::class) +class AccountDatabaseTest { + + private lateinit var db: AccountDatabase + + @Before + fun setUp() { + val context = ApplicationProvider.getApplicationContext() + db = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build() + } + + @After + fun tearDown() = db.close() + + private fun account(id: String = "acct") = AccountEntity( + id = id, + email = "a@example.org", + displayName = "A", + authType = "PASSWORD_IMAP", + imap = ServerConfigEmbedded("imap.example.org", 993, "SSL_TLS"), + smtp = ServerConfigEmbedded("smtp.example.org", 465, "SSL_TLS"), + ) + + @Test + fun credentialRoundTripsAndIsIndependentOfTheAccountRow() = runBlocking { + db.accountDao().upsert(account()) + db.credentialDao().upsert(CredentialEntity("acct", "sealed-secret")) + + assertEquals("sealed-secret", db.credentialDao().getById("acct")?.encryptedSecret) + } + + @Test + fun accountSettingsRoundTripAndCascadeWithTheirAccount() = runBlocking { + db.accountDao().upsert(account()) + db.accountSettingsDao().upsert( + AccountSettingsEntity("acct", signature = "Hi", signatureEnabled = false, notificationsEnabled = false), + ) + assertEquals("Hi", db.accountSettingsDao().get("acct")?.signature) + + db.accountDao().deleteById("acct") + + assertNull("account_settings must cascade-delete with its account", db.accountSettingsDao().get("acct")) + } + + @Test + fun signaturesCascadeWithTheirAccount() = runBlocking { + db.accountDao().upsert(account()) + db.signatureDao().upsert(SignatureEntity("sig-1", "acct", "Work", "

Regards

", isDefault = true)) + assertEquals(1, db.signatureDao().observeForAccount("acct").first().size) + + db.accountDao().deleteById("acct") + + assertTrue( + "signatures must cascade-delete with their account", + db.signatureDao().observeForAccount("acct").first().isEmpty(), + ) + } +} diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt index b4ed6cb..5879ba1 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt @@ -9,22 +9,21 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import org.junit.After import org.junit.Assert.assertEquals -import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test import org.junit.runner.RunWith -import org.libremail.data.local.entity.AccountEntity -import org.libremail.data.local.entity.AccountSettingsEntity import org.libremail.data.local.entity.AttachmentEntity import org.libremail.data.local.entity.FolderEntity import org.libremail.data.local.entity.MessageEntity -import org.libremail.data.local.entity.ServerConfigEmbedded /** * Schema-behavior tests on a fresh in-memory database at the current version. The migration DDL * itself is exercised by [MigrationTest], which replays the schema chain exported to app/schemas. * (Migrations from before v7 predate schema export, so they can't be replayed there.) + * + * Account/credential/settings/signature behavior moved to [AccountDatabaseTest] with those tables + * (issue #111). */ @RunWith(AndroidJUnit4::class) class LibreMailDatabaseTest { @@ -97,30 +96,6 @@ class LibreMailDatabaseTest { ) } - @Test - fun accountSettingsRoundTripAndCascadeWithTheirAccount() = runBlocking { - val accountDao = db.accountDao() - val settingsDao = db.accountSettingsDao() - accountDao.upsert( - AccountEntity( - id = "acct", - email = "a@example.org", - displayName = "A", - authType = "PASSWORD_IMAP", - imap = ServerConfigEmbedded("imap.example.org", 993, "SSL_TLS"), - smtp = ServerConfigEmbedded("smtp.example.org", 465, "SSL_TLS"), - ), - ) - settingsDao.upsert( - AccountSettingsEntity("acct", signature = "Hi", signatureEnabled = false, notificationsEnabled = false), - ) - assertEquals("Hi", settingsDao.get("acct")?.signature) - - accountDao.deleteById("acct") - - assertNull("account_settings must cascade-delete with its account", settingsDao.get("acct")) - } - @Test fun searchRowsAreNotInboxAndAreCleared() = runBlocking { val messageDao = db.messageDao() diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt index 6716829..1d669db 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MigrationTest.kt @@ -133,6 +133,9 @@ class MigrationTest { open?.close() val stepDb = helper.runMigrationsAndValidate(TEST_DB, migration.endVersion, true, migration) stepDb.writeMidChainData() + // v16 moves the account tables out to AccountDatabase and drops them, so assert their rows + // and backfills reached v15 intact — just before the move (issue #111). + if (stepDb.version == 15) stepDb.assertAccountDataPresentAtV15() open = stepDb } val db = checkNotNull(open) { "no migration starts at v$OLDEST_EXPORTED_SCHEMA" } @@ -140,6 +143,29 @@ class MigrationTest { assertEquals("the chain must end at the newest exported schema", latestExportedSchemaVersion(), db.version) db.assertVersion7CacheSurvived() db.assertMigrationBackfillsApplied() + db.assertAccountTablesDroppedAtV16() + db.close() + } + + /** v15 -> v16 (issue #111): the moved account tables are dropped and the mail cache is untouched. */ + @Test + fun migrate15To16_dropsMovedAccountTablesAndKeepsCache() { + helper.createDatabase(TEST_DB, 15).apply { + insertAccount() + execSQL("INSERT INTO credentials (accountId, encryptedSecret) VALUES ('acct', 'sealed')") + execSQL( + "INSERT INTO messages (id, accountId, sender, senderEmail, subject, snippet, body, isHtml, " + + "timestampMillis, isRead, isStarred, folder, inInbox, bodyFetched, uid) VALUES " + + "('acct:INBOX:1', 'acct', 'Ada', 'ada@example.org', 'Hi', '', '', 0, 1000, 0, 0, " + + "'INBOX', 1, 0, 1)", + ) + close() + } + + val db = helper.runMigrationsAndValidate(TEST_DB, 16, true, MIGRATION_15_16) + + db.assertAccountTablesDroppedAtV16() + assertEquals("the mail cache must be untouched by 15->16", 1, db.count("messages")) db.close() } @@ -203,9 +229,8 @@ class MigrationTest { } } - /** Every row cached at v7 must still be present and correct at the end of the chain. */ + /** Every mail-cache row cached at v7 must survive to v16 (account tables are checked separately). */ private fun SupportSQLiteDatabase.assertVersion7CacheSurvived() { - assertEquals(1, count("accounts")) assertEquals(2, count("messages")) assertEquals(1, count("outbox")) assertEquals(1, count("drafts")) @@ -215,24 +240,14 @@ class MigrationTest { assertEquals("Analytical engines", c.getString(1)) assertEquals(1, c.getInt(2)) } - query("SELECT encryptedSecret FROM credentials WHERE accountId = 'acct'").use { c -> - assertTrue("stored credentials must never be dropped by a migration", c.moveToFirst()) - assertEquals("sealed-secret", c.getString(0)) - } query("SELECT filename FROM attachments WHERE messageId = 'acct:1'").use { c -> assertTrue("attachment rows must survive the 6->7 style table rebuilds", c.moveToFirst()) assertEquals("notes.pdf", c.getString(0)) } } - /** Columns and rows created by the migrations themselves must hold their documented defaults. */ + /** Cache-table columns/rows the migrations backfill must hold their documented defaults at v16. */ private fun SupportSQLiteDatabase.assertMigrationBackfillsApplied() { - // 8->9 backfills one default settings row per existing account. - query("SELECT signatureEnabled, notificationsEnabled FROM account_settings").use { c -> - assertTrue("8->9 must backfill a settings row for the v7 account", c.moveToFirst()) - assertEquals(1, c.getInt(0)) - assertEquals(1, c.getInt(1)) - } // 9->10 adds bcc columns defaulting to ''; 10->11 adds nullable bodyHtml. query("SELECT bccAddresses, bodyHtml FROM outbox WHERE id = 'out-1'").use { c -> assertTrue(c.moveToFirst()) @@ -244,14 +259,6 @@ class MigrationTest { assertEquals("", c.getString(0)) assertTrue(c.isNull(1)) } - // 10->11 turns the signature written at v9 into that account's default rich-text signature. - query("SELECT name, contentHtml, isDefault FROM signatures WHERE accountId = 'acct'").use { c -> - assertTrue("10->11 must backfill the legacy per-account signature", c.moveToFirst()) - assertEquals("Signature", c.getString(0)) - assertEquals("Cheers,
Ada", c.getString(1)) - assertEquals(1, c.getInt(2)) - assertFalse("exactly one signature row must be backfilled", c.moveToNext()) - } // 11->12 stamps the folder cached at v8 as not special-use. query("SELECT specialUse FROM folders WHERE fullName = 'INBOX'").use { c -> assertTrue("folder cached at v8 must survive to the newest version", c.moveToFirst()) @@ -265,6 +272,42 @@ class MigrationTest { } } + /** + * The account tables' rows + migration backfills must be intact at v15, just before 15->16 moves + * them to [AccountDatabase] and drops them (issue #111). AccountDataMigrator's own copy is + * exercised in `AccountDataMigratorTest`; here we only assert the source rows reach the move point. + */ + private fun SupportSQLiteDatabase.assertAccountDataPresentAtV15() { + assertEquals(1, count("accounts")) + query("SELECT encryptedSecret FROM credentials WHERE accountId = 'acct'").use { c -> + assertTrue("stored credentials must reach v15 before the move", c.moveToFirst()) + assertEquals("sealed-secret", c.getString(0)) + } + // 8->9 backfills one default settings row per existing account. + query("SELECT signatureEnabled, notificationsEnabled FROM account_settings").use { c -> + assertTrue("8->9 must backfill a settings row for the v7 account", c.moveToFirst()) + assertEquals(1, c.getInt(0)) + assertEquals(1, c.getInt(1)) + } + // 10->11 turns the signature written at v9 into that account's default rich-text signature. + query("SELECT name, contentHtml, isDefault FROM signatures WHERE accountId = 'acct'").use { c -> + assertTrue("10->11 must backfill the legacy per-account signature", c.moveToFirst()) + assertEquals("Signature", c.getString(0)) + assertEquals("Cheers,
Ada", c.getString(1)) + assertEquals(1, c.getInt(2)) + assertFalse("exactly one signature row must be backfilled", c.moveToNext()) + } + } + + /** 15->16 drops the account tables from the cache (AccountDataMigrator copies them out first). */ + private fun SupportSQLiteDatabase.assertAccountTablesDroppedAtV16() { + listOf("accounts", "credentials", "account_settings", "signatures").forEach { table -> + query("SELECT name FROM sqlite_master WHERE type = 'table' AND name = '$table'").use { c -> + assertFalse("15->16 must drop `$table` from the cache database", c.moveToFirst()) + } + } + } + private fun SupportSQLiteDatabase.count(table: String): Int = query("SELECT COUNT(*) FROM $table").use { c -> c.moveToFirst() c.getInt(0) diff --git a/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt index 25355e6..bbd2bbf 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/compose/ComposeScreenTest.kt @@ -26,7 +26,7 @@ import org.junit.Test import org.junit.runner.RunWith import org.libremail.R import org.libremail.contacts.ContactsRepository -import org.libremail.data.local.LibreMailDatabase +import org.libremail.data.local.AccountDatabase import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.SignatureRepository import org.libremail.domain.model.Account @@ -56,7 +56,7 @@ class ComposeScreenTest { smtp = ServerConfig("smtp.example.com", 465, MailSecurity.SSL_TLS), ) - private var db: LibreMailDatabase? = null + private var db: AccountDatabase? = null private fun string(resId: Int) = composeTestRule.activity.getString(resId) @@ -79,7 +79,7 @@ class ComposeScreenTest { // Build the view model once and capture it, so recomposition doesn't recreate it. private fun setContent(mailRepository: FakeMailRepository = FakeMailRepository(), onBack: () -> Unit = {}) { val context = InstrumentationRegistry.getInstrumentation().targetContext.applicationContext - val database = Room.inMemoryDatabaseBuilder(context, LibreMailDatabase::class.java).build().also { db = it } + val database = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build().also { db = it } val viewModel = ComposeViewModel( savedStateHandle = SavedStateHandle(), mailRepository = mailRepository, diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt index 20e1173..85f90c4 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt @@ -16,7 +16,7 @@ import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith import org.libremail.R -import org.libremail.data.local.LibreMailDatabase +import org.libremail.data.local.AccountDatabase import org.libremail.data.local.toEntity import org.libremail.data.settings.AccountSettingsRepository import org.libremail.data.settings.SignatureRepository @@ -59,7 +59,7 @@ class AccountSettingsScreenTest { // stateIn/WhileSubscribed) keeps querying after the test body, so closing the in-memory DB out // from under it races and crashes ("connection pool has been closed"). The DB is reclaimed with // the test process. - val db = Room.inMemoryDatabaseBuilder(context, LibreMailDatabase::class.java).build() + val db = Room.inMemoryDatabaseBuilder(context, AccountDatabase::class.java).build() val repository = AccountSettingsRepository(db.accountSettingsDao()) runBlocking { db.accountDao().upsert(account.toEntity()) // FK parent for the account_settings row diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt new file mode 100644 index 0000000..8e5a50e --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt @@ -0,0 +1,193 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import android.content.Context +import android.util.Log +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.booleanPreferencesKey +import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.preferencesDataStore +import dagger.hilt.android.qualifiers.ApplicationContext +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withContext +import net.zetetic.database.sqlcipher.SQLiteDatabase +import org.libremail.data.security.DatabaseKeyStore +import org.libremail.data.settings.SettingsRepository +import java.io.File +import javax.inject.Inject +import javax.inject.Singleton + +private val Context.accountMigrationDataStore: DataStore by + preferencesDataStore(name = "libremail_account_migration") + +/** + * One-time, crash-safe move of the account tables (`accounts`, `credentials`, `account_settings`, + * `signatures`) out of the auth-bound cache database [LibreMailDatabase] into the non-auth + * [AccountDatabase] (issue #111). Runs at startup, driven by `DatabaseModule.provideDatabase`, BEFORE + * Room opens the cache and its [MIGRATION_15_16] drops the moved tables. + * + * ### Why not a Room migration + * The copy is cross-database, so it needs `ATTACH DATABASE`, which SQLite forbids inside the + * transaction Room wraps every migration in. It therefore runs here on a dedicated SQLCipher + * connection before Room opens either database. + * + * ### Handling the encrypted source + * When the opt-in encrypted cache is on, the source `libremail.db` is SQLCipher-encrypted. The + * caller resolves and hands us its passphrase (the same one Room uses to open it); we attach the + * cache with that passphrase and copy into a plaintext `libremail-accounts.db`. When the cache is + * plaintext the passphrase is empty. Reading the source's schema validates the passphrase, so a + * genuinely wrong key fails loudly here (the same open would fail in Room) rather than losing data. + * + * The unrecoverable-key case does not reach us: `provideDatabase` wipes an undecryptable cache (and + * resets its seals) BEFORE calling us, so we then see a fresh/empty cache with nothing to move — the + * accounts trapped in that already-invalidated cache are lost regardless (the pre-existing bug), but + * no future invalidation can strand them again once they live in [AccountDatabase]. + * + * ### Crash-safety & idempotency + * - We never drop the source here; [MIGRATION_15_16] does that after we return, so if we crash the + * source rows are still intact for the next attempt. + * - The copy uses `INSERT OR IGNORE`, so a re-run after a mid-copy crash converges (existing rows + * are skipped, never duplicated, and never overwrite anything the user changed post-migration). + * - The "done" flag is only set after a successful copy; until then every start retries. Once set we + * return immediately and never touch the cache passphrase again — so after migration the account + * database opens with no Keystore dependency at all. + */ +@Singleton +class AccountDataMigrator @Inject constructor( + @ApplicationContext private val context: Context, + private val keyStore: DatabaseKeyStore, + private val settingsRepository: SettingsRepository, +) { + + /** + * Copy the account tables into [AccountDatabase] if it has not been done yet. Idempotent and + * safe to call from every `provideDatabase` construction. Throws (rather than silently skipping) + * on an unexpected copy failure so the caller does not proceed to drop the source tables — a + * crash-loop that preserves data is strictly safer than a wipe that loses it. + */ + suspend fun migrateIfNeeded() { + if (isDone()) return + val cacheFile = context.getDatabasePath(DatabaseFiles.NAME) + if (cacheFile.exists() && cacheFile.length() > 0L) { + // Read the cache in its CURRENT on-disk form. `provideDatabase` runs us before it converts + // between plaintext and encrypted, so the key is empty unless the file is encrypted now. + val cacheKey = if (DatabaseEncryption.isEncrypted(cacheFile)) { + keyStore.resolvePassphrase(settingsRepository.settings.first().appLock) + } else { + "" + } + val accountsFile = context.getDatabasePath(DatabaseFiles.ACCOUNTS_NAME) + withContext(Dispatchers.IO) { copyAccountTables(cacheFile, cacheKey, accountsFile) } + } + markDone() + } + + private suspend fun isDone(): Boolean = context.accountMigrationDataStore.data.first()[DONE] == true + + private suspend fun markDone() { + context.accountMigrationDataStore.edit { it[DONE] = true } + } + + companion object { + private const val TAG = "LibreMailAcctMigrate" + private val DONE = booleanPreferencesKey("accounts_moved_out_of_cache") + + /** The account tables, parent before children so foreign keys never block an insert. */ + private val TABLES = listOf("accounts", "credentials", "account_settings", "signatures") + + /** + * DDL for the account tables in [AccountDatabase] v1, copied verbatim from the exported Room + * schema (`schemas/org.libremail.data.local.AccountDatabase/1.json`). It MUST stay byte-for-byte + * identical to what Room generates for those entities, or Room silently accepts a subtly wrong + * schema (its identity check only compares the hash it writes, not the pre-existing tables). + * `AccountDataMigratorTest.migratorDdlMatchesExportedAccountDatabaseSchema` guards it against the + * exported schema; `internal` only so that test can read it. + */ + internal val CREATE_TABLE_SQL = mapOf( + "accounts" to + "CREATE TABLE IF NOT EXISTS `accounts` (`id` TEXT NOT NULL, `email` TEXT NOT NULL, " + + "`displayName` TEXT NOT NULL, `authType` TEXT NOT NULL, `imap_host` TEXT NOT NULL, " + + "`imap_port` INTEGER NOT NULL, `imap_security` TEXT NOT NULL, `smtp_host` TEXT NOT NULL, " + + "`smtp_port` INTEGER NOT NULL, `smtp_security` TEXT NOT NULL, PRIMARY KEY(`id`))", + "credentials" to + "CREATE TABLE IF NOT EXISTS `credentials` (`accountId` TEXT NOT NULL, " + + "`encryptedSecret` TEXT NOT NULL, PRIMARY KEY(`accountId`))", + "account_settings" to + "CREATE TABLE IF NOT EXISTS `account_settings` (`accountId` TEXT NOT NULL, " + + "`signature` TEXT NOT NULL, `signatureEnabled` INTEGER NOT NULL, " + + "`notificationsEnabled` INTEGER NOT NULL, `retentionCount` INTEGER, " + + "`retentionMonths` INTEGER, PRIMARY KEY(`accountId`), " + + "FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) " + + "ON UPDATE NO ACTION ON DELETE CASCADE )", + "signatures" to + "CREATE TABLE IF NOT EXISTS `signatures` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, " + + "`name` TEXT NOT NULL, `contentHtml` TEXT NOT NULL, `isDefault` INTEGER NOT NULL, " + + "PRIMARY KEY(`id`), FOREIGN KEY(`accountId`) REFERENCES `accounts`(`id`) " + + "ON UPDATE NO ACTION ON DELETE CASCADE )", + ) + + internal const val SIGNATURES_INDEX_SQL = + "CREATE INDEX IF NOT EXISTS `index_signatures_accountId` ON `signatures` (`accountId`)" + + /** + * Copies the account tables from [cacheFile] (opened with [cachePassphrase]; empty = plaintext) + * into a plaintext [accountsFile], creating the destination schema first. Opens the destination + * as `main` and attaches the (possibly encrypted) cache as `cache`, so a plaintext connection + * can still read the encrypted source via SQLCipher's per-attach key. Visible for the migrator + * test; call [migrateIfNeeded] in production. + */ + internal fun copyAccountTables(cacheFile: File, cachePassphrase: String, accountsFile: File) { + DatabaseEncryption.ensureNativeLibraryLoaded() + val db = SQLiteDatabase.openOrCreateDatabase( + accountsFile.absolutePath, + "".toByteArray(Charsets.US_ASCII), // destination is plaintext + null, + null, + ) + try { + // No WAL: keep the destination in rollback-journal mode (as DatabaseEncryption does) + // so that after close there is no -wal/-shm holding uncommitted rows for Room to miss. + db.rawExecSQL("PRAGMA journal_mode = DELETE;") + val keyLiteral = cachePassphrase.replace("'", "''") + val cachePath = cacheFile.absolutePath.replace("'", "''") + db.rawExecSQL("ATTACH DATABASE '$cachePath' AS cache KEY '$keyLiteral';") + try { + val present = presentTables(db) + if (present.isEmpty()) return // fresh cache or already dropped: nothing to move + TABLES.forEach { db.rawExecSQL(CREATE_TABLE_SQL.getValue(it)) } + db.rawExecSQL(SIGNATURES_INDEX_SQL) + // Parent first so an enforced foreign key (Room enables them; this raw connection + // does not) would still be satisfied. INSERT OR IGNORE makes each copy idempotent. + TABLES.filter { it in present }.forEach { table -> + db.rawExecSQL("INSERT OR IGNORE INTO `$table` SELECT * FROM cache.`$table`") + } + Log.d(TAG, "moved account tables into the account database: $present") + } finally { + db.rawExecSQL("DETACH DATABASE cache;") + } + } finally { + db.close() + } + // Room opens the destination next; drop any sidecars the copy left so a stale WAL/SHM can't + // confuse its first open. + val dir = accountsFile.parentFile + if (dir != null) { + listOf("-wal", "-shm", "-journal").forEach { File(dir, accountsFile.name + it).delete() } + } + } + + private fun presentTables(db: SQLiteDatabase): Set { + val names = TABLES.joinToString(",") { "'$it'" } + val present = mutableSetOf() + db.rawQuery( + "SELECT name FROM cache.sqlite_master WHERE type = 'table' AND name IN ($names)", + null, + ).use { cursor -> + while (cursor.moveToNext()) present += cursor.getString(0) + } + return present + } + } +} diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt new file mode 100644 index 0000000..cdf4e47 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDatabase.kt @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local + +import androidx.room.Database +import androidx.room.RoomDatabase +import org.libremail.data.local.dao.AccountDao +import org.libremail.data.local.dao.AccountSettingsDao +import org.libremail.data.local.dao.CredentialDao +import org.libremail.data.local.dao.SignatureDao +import org.libremail.data.local.entity.AccountEntity +import org.libremail.data.local.entity.AccountSettingsEntity +import org.libremail.data.local.entity.CredentialEntity +import org.libremail.data.local.entity.SignatureEntity + +/** + * Durable store for the pieces of an account that must survive a mail-cache wipe (issue #111): the + * account itself, its sealed credential, per-account settings, and saved signatures. + * + * This lives in its OWN database file ([DatabaseFiles.ACCOUNTS_NAME]) that is deliberately NEVER + * bound to the auth-bound SQLCipher key. When app-lock + encrypted-cache are on and that key is + * invalidated (a genuine biometric re-enrollment or lock removal/re-add), only the mail cache + * ([LibreMailDatabase]) becomes undecryptable and is wiped; this database is untouched, so the user + * stays signed in instead of being dropped back into onboarding. + * + * It is plaintext on disk. The only secret it holds is [CredentialEntity.encryptedSecret], which is + * already AES-GCM ciphertext sealed at the column level by the non-auth + * [org.libremail.data.security.KeystoreCrypto] master key (and that key survives an auth-key + * invalidation), so the secret never touches disk in the clear regardless of this file's own + * encryption. Account metadata (email address, server hosts) is not a secret. Keeping the file + * plaintext is what makes it maximally resilient — it can always be opened without any Keystore key, + * so no key invalidation can ever strand it. + * + * Existing installs are migrated into this database once, at startup, by [AccountDataMigrator] + * before [MIGRATION_15_16] drops the moved tables from the cache database. + */ +@Database( + entities = [ + AccountEntity::class, + CredentialEntity::class, + AccountSettingsEntity::class, + SignatureEntity::class, + ], + version = 1, + exportSchema = true, +) +abstract class AccountDatabase : RoomDatabase() { + abstract fun accountDao(): AccountDao + abstract fun credentialDao(): CredentialDao + abstract fun accountSettingsDao(): AccountSettingsDao + abstract fun signatureDao(): SignatureDao +} diff --git a/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt b/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt index 515944c..6694a2c 100644 --- a/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt +++ b/app/src/main/kotlin/org/libremail/data/local/DatabaseEncryption.kt @@ -40,7 +40,7 @@ object DatabaseEncryption { * tables but not that pragma, and a reset version would make Room attempt a bogus migration. */ private fun migrate(dbFile: File, sourcePassphrase: String, targetPassphrase: String) { - ensureLibraryLoaded() + ensureNativeLibraryLoaded() val dir = dbFile.parentFile ?: error("database file has no parent directory") val tmp = File(dir, dbFile.name + ".migrate").apply { delete() } @@ -98,7 +98,13 @@ object DatabaseEncryption { } @Volatile private var libraryLoaded = false - private fun ensureLibraryLoaded() { + + /** + * Load SQLCipher's native library once. Public so other startup helpers that open a database via + * [net.zetetic.database.sqlcipher.SQLiteDatabase] before Room does (e.g. [AccountDataMigrator]) + * can guarantee it is loaded first. + */ + fun ensureNativeLibraryLoaded() { if (libraryLoaded) return synchronized(this) { if (!libraryLoaded) { diff --git a/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt b/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt index 0b18f42..4e52837 100644 --- a/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt +++ b/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt @@ -9,6 +9,13 @@ object DatabaseFiles { const val NAME = "libremail.db" + /** + * The [org.libremail.data.local.AccountDatabase] file — accounts, credentials, per-account + * settings and signatures. Deliberately a DIFFERENT file from [NAME] and NEVER wiped by [clear], + * so a cache-key invalidation keeps the user signed in (issue #111). + */ + const val ACCOUNTS_NAME = "libremail-accounts.db" + /** * Delete the database and any WAL/SHM/journal sidecars. Call only when no connection is open — * used by the "clear + re-sync" path when the encryption key is invalidated and the encrypted diff --git a/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt b/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt index 24bf856..567ad08 100644 --- a/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt +++ b/app/src/main/kotlin/org/libremail/data/local/LibreMailDatabase.kt @@ -3,52 +3,46 @@ package org.libremail.data.local import androidx.room.Database import androidx.room.RoomDatabase -import org.libremail.data.local.dao.AccountDao -import org.libremail.data.local.dao.AccountSettingsDao import org.libremail.data.local.dao.AttachmentDao import org.libremail.data.local.dao.BackfillProgressDao -import org.libremail.data.local.dao.CredentialDao import org.libremail.data.local.dao.DraftDao import org.libremail.data.local.dao.FolderDao import org.libremail.data.local.dao.MessageDao import org.libremail.data.local.dao.OutboxDao -import org.libremail.data.local.dao.SignatureDao -import org.libremail.data.local.entity.AccountEntity -import org.libremail.data.local.entity.AccountSettingsEntity import org.libremail.data.local.entity.AttachmentEntity import org.libremail.data.local.entity.BackfillProgressEntity -import org.libremail.data.local.entity.CredentialEntity import org.libremail.data.local.entity.DraftEntity import org.libremail.data.local.entity.FolderEntity import org.libremail.data.local.entity.MessageEntity import org.libremail.data.local.entity.OutboxEntity -import org.libremail.data.local.entity.SignatureEntity +/** + * The offline mail cache. Everything here is re-derivable from the server on a fresh sync, so it is + * the database that opt-in SQLCipher encryption is applied to and — when the auth-bound key is + * invalidated — the one that "clear + re-sync" wipes. + * + * Account identity and user configuration (accounts, credentials, per-account settings, signatures) + * are deliberately NOT here: they live in [AccountDatabase], a separate non-auth-bound file, so a + * cache-key invalidation can never sign the user out (issue #111). [MIGRATION_15_16] dropped those + * tables from this database; [AccountDataMigrator] copies existing rows into [AccountDatabase] first. + */ @Database( entities = [ - AccountEntity::class, - AccountSettingsEntity::class, MessageEntity::class, - CredentialEntity::class, AttachmentEntity::class, OutboxEntity::class, DraftEntity::class, FolderEntity::class, - SignatureEntity::class, BackfillProgressEntity::class, ], - version = 15, + version = 16, exportSchema = true, ) abstract class LibreMailDatabase : RoomDatabase() { abstract fun messageDao(): MessageDao - abstract fun accountDao(): AccountDao - abstract fun accountSettingsDao(): AccountSettingsDao - abstract fun credentialDao(): CredentialDao abstract fun attachmentDao(): AttachmentDao abstract fun outboxDao(): OutboxDao abstract fun draftDao(): DraftDao abstract fun folderDao(): FolderDao - abstract fun signatureDao(): SignatureDao abstract fun backfillProgressDao(): BackfillProgressDao } diff --git a/app/src/main/kotlin/org/libremail/data/local/Migrations.kt b/app/src/main/kotlin/org/libremail/data/local/Migrations.kt index 1b3a5f5..feadf8a 100644 --- a/app/src/main/kotlin/org/libremail/data/local/Migrations.kt +++ b/app/src/main/kotlin/org/libremail/data/local/Migrations.kt @@ -308,3 +308,25 @@ val MIGRATION_14_15 = object : Migration(14, 15) { db.execSQL("ALTER TABLE `folders` ADD COLUMN `hierarchyDelimiter` TEXT") } } + +/** + * v15 -> v16: move account identity + configuration OUT of the cache database (issue #111). The + * `accounts`, `credentials`, `account_settings` and `signatures` tables now live in [AccountDatabase] + * — a separate file that is never sealed by the auth-bound SQLCipher key — so a cache-key invalidation + * (biometric re-enrollment / lock removal) wipes only mail and can no longer sign the user out. + * + * The rows are copied into [AccountDatabase] by [AccountDataMigrator] at startup BEFORE Room opens the + * cache and runs this migration. The copy CANNOT happen here: Room wraps each migration in a + * transaction and SQLite forbids `ATTACH DATABASE` inside one, so a cross-database copy has to run on + * a separate connection before the cache is opened. This migration therefore only drops the tables + * that were moved. `DROP TABLE IF EXISTS` keeps it idempotent, and children (foreign-keyed to + * `accounts`) are dropped before the parent so the drop never trips a foreign-key check. + */ +val MIGRATION_15_16 = object : Migration(15, 16) { + override fun migrate(db: SupportSQLiteDatabase) { + db.execSQL("DROP TABLE IF EXISTS `signatures`") + db.execSQL("DROP TABLE IF EXISTS `account_settings`") + db.execSQL("DROP TABLE IF EXISTS `credentials`") + db.execSQL("DROP TABLE IF EXISTS `accounts`") + } +} diff --git a/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt new file mode 100644 index 0000000..435f929 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/di/AccountDatabaseModule.kt @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.di + +import android.content.Context +import androidx.room.Room +import dagger.Module +import dagger.Provides +import dagger.hilt.InstallIn +import dagger.hilt.android.qualifiers.ApplicationContext +import dagger.hilt.components.SingletonComponent +import org.libremail.data.local.AccountDatabase +import org.libremail.data.local.DatabaseFiles.ACCOUNTS_NAME +import org.libremail.data.local.LibreMailDatabase +import org.libremail.data.local.dao.AccountDao +import org.libremail.data.local.dao.AccountSettingsDao +import org.libremail.data.local.dao.CredentialDao +import org.libremail.data.local.dao.SignatureDao +import javax.inject.Singleton + +/** + * Hilt wiring for [AccountDatabase] — the non-auth-bound store for accounts, credentials, per-account + * settings and signatures (issue #111). Kept separate from [DatabaseModule] so each database's + * provides stay cohesive (and neither module grows past detekt's per-object function limit). + */ +@Module +@InstallIn(SingletonComponent::class) +object AccountDatabaseModule { + + /** + * The plaintext account store. Depends on [LibreMailDatabase] purely for construction ordering: + * building the cache runs the one-time [org.libremail.data.local.AccountDataMigrator] (which + * populates this file on a dedicated connection) and then drops the moved tables, so by the time + * Room opens this file the data is already present and no other connection is touching it. + */ + @Provides + @Singleton + fun provideAccountDatabase( + @ApplicationContext context: Context, + @Suppress("UNUSED_PARAMETER") cacheDatabase: LibreMailDatabase, + ): AccountDatabase = Room.databaseBuilder(context, AccountDatabase::class.java, ACCOUNTS_NAME).build() + + @Provides + fun provideAccountDao(database: AccountDatabase): AccountDao = database.accountDao() + + @Provides + fun provideCredentialDao(database: AccountDatabase): CredentialDao = database.credentialDao() + + @Provides + fun provideAccountSettingsDao(database: AccountDatabase): AccountSettingsDao = database.accountSettingsDao() + + @Provides + fun provideSignatureDao(database: AccountDatabase): SignatureDao = database.signatureDao() +} diff --git a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt index 792e3ef..d247ad1 100644 --- a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt +++ b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt @@ -11,6 +11,7 @@ import dagger.hilt.components.SingletonComponent import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking import net.zetetic.database.sqlcipher.SupportOpenHelperFactory +import org.libremail.data.local.AccountDataMigrator import org.libremail.data.local.DatabaseEncryption import org.libremail.data.local.DatabaseFiles import org.libremail.data.local.LibreMailDatabase @@ -19,6 +20,7 @@ import org.libremail.data.local.MIGRATION_11_12 import org.libremail.data.local.MIGRATION_12_13 import org.libremail.data.local.MIGRATION_13_14 import org.libremail.data.local.MIGRATION_14_15 +import org.libremail.data.local.MIGRATION_15_16 import org.libremail.data.local.MIGRATION_1_2 import org.libremail.data.local.MIGRATION_2_3 import org.libremail.data.local.MIGRATION_3_4 @@ -28,16 +30,12 @@ import org.libremail.data.local.MIGRATION_6_7 import org.libremail.data.local.MIGRATION_7_8 import org.libremail.data.local.MIGRATION_8_9 import org.libremail.data.local.MIGRATION_9_10 -import org.libremail.data.local.dao.AccountDao -import org.libremail.data.local.dao.AccountSettingsDao import org.libremail.data.local.dao.AttachmentDao import org.libremail.data.local.dao.BackfillProgressDao -import org.libremail.data.local.dao.CredentialDao import org.libremail.data.local.dao.DraftDao import org.libremail.data.local.dao.FolderDao import org.libremail.data.local.dao.MessageDao import org.libremail.data.local.dao.OutboxDao -import org.libremail.data.local.dao.SignatureDao import org.libremail.data.security.DatabaseKeyStore import org.libremail.data.settings.SettingsRepository import javax.inject.Singleton @@ -52,6 +50,7 @@ object DatabaseModule { @ApplicationContext context: Context, keyStore: DatabaseKeyStore, settingsRepository: SettingsRepository, + accountDataMigrator: AccountDataMigrator, ): LibreMailDatabase { val builder = Room.databaseBuilder(context, LibreMailDatabase::class.java, DB_NAME) .addMigrations( @@ -69,10 +68,11 @@ object DatabaseModule { MIGRATION_12_13, MIGRATION_13_14, MIGRATION_14_15, + MIGRATION_15_16, ) // No destructive fallback: the migration chain is complete, and silently dropping the - // accounts/credentials/mail tables would lose stored secrets. A missing migration should - // fail loudly in testing instead. + // mail/message tables would lose cached data. A missing migration should fail loudly in + // testing instead. // Opt-in at-rest encryption of the local cache (off by default). The conversion runs here — // before the database is opened — so it never races an open connection; toggling the setting @@ -92,6 +92,8 @@ object DatabaseModule { // restarts the app; we wipe the cache HERE — at cold start, before Room opens — so the file is // never deleted from under an open connection. Crash-safe order: wipe + reset the seals, and // only THEN clear the flag, so a kill mid-wipe just repeats the idempotent wipe next start. + // Only libremail.db is wiped: accounts/credentials live in AccountDatabase (a separate file), + // so the user stays signed in across the wipe (issue #111). if (runBlocking { keyStore.isClearPending() }) { DatabaseFiles.clear(context) runBlocking { @@ -100,6 +102,12 @@ object DatabaseModule { } } + // One-time move of accounts/credentials/settings/signatures into the non-auth AccountDatabase + // (issue #111). MUST run before builder.build() below: opening the cache applies MIGRATION_15_16, + // which drops the moved tables. It runs AFTER the wipe above so an unrecoverable-key cache is + // gone first (nothing left to move) and we never block waiting on a passphrase we can't get. + runBlocking { accountDataMigrator.migrateIfNeeded() } + val settings = runBlocking { settingsRepository.settings.first() } val appLock = settings.appLock if (settings.encryptCache) { @@ -119,15 +127,6 @@ object DatabaseModule { @Provides fun provideMessageDao(database: LibreMailDatabase): MessageDao = database.messageDao() - @Provides - fun provideAccountDao(database: LibreMailDatabase): AccountDao = database.accountDao() - - @Provides - fun provideAccountSettingsDao(database: LibreMailDatabase): AccountSettingsDao = database.accountSettingsDao() - - @Provides - fun provideCredentialDao(database: LibreMailDatabase): CredentialDao = database.credentialDao() - @Provides fun provideAttachmentDao(database: LibreMailDatabase): AttachmentDao = database.attachmentDao() @@ -140,9 +139,6 @@ object DatabaseModule { @Provides fun provideFolderDao(database: LibreMailDatabase): FolderDao = database.folderDao() - @Provides - fun provideSignatureDao(database: LibreMailDatabase): SignatureDao = database.signatureDao() - @Provides fun provideBackfillProgressDao(database: LibreMailDatabase): BackfillProgressDao = database.backfillProgressDao() From d9d50f190356f77fc1dfa4a32480ad3e3f93900d Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 06:37:07 -0500 Subject: [PATCH 2/7] fix(test): make instrumented migrator tests return Unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@Test fun x() = runBlocking { ... }` whose block ends in `.apply { }` returns the DB (non-Unit), so JUnit4 rejects the whole class at runtime with InvalidTestClassError ("method should be void") — which compiles fine locally but fails every E2E job on the emulator. Use `runBlocking` (the existing DatabaseEncryptionTest idiom) so the methods are void while keeping the expression body ktlint expects. Co-Authored-By: Claude Fable 5 --- .../org/libremail/data/local/AccountDataMigratorTest.kt | 8 ++++---- .../org/libremail/data/local/AccountDatabaseTest.kt | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt index d610200..4eaf071 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -92,7 +92,7 @@ class AccountDataMigratorTest { Room.databaseBuilder(context, AccountDatabase::class.java, accountsName).build() @Test - fun movesEveryAccountTableOutOfAPlaintextCache() = runBlocking { + fun movesEveryAccountTableOutOfAPlaintextCache() = runBlocking { seedVersion14Cache() AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) @@ -121,7 +121,7 @@ class AccountDataMigratorTest { } @Test - fun movesAccountsOutOfAnEncryptedCache() = runBlocking { + fun movesAccountsOutOfAnEncryptedCache() = runBlocking { seedVersion14Cache() // Turn the cache into the SQLCipher form an app-lock + encrypted-cache user has on disk. DatabaseEncryption.ensureEncrypted(cacheFile, passphrase) @@ -141,7 +141,7 @@ class AccountDataMigratorTest { } @Test - fun reRunningTheCopyIsIdempotentAndKeepsLaterEdits() = runBlocking { + fun reRunningTheCopyIsIdempotentAndKeepsLaterEdits() = runBlocking { seedVersion14Cache() AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) @@ -167,7 +167,7 @@ class AccountDataMigratorTest { } @Test - fun accountsAndCredentialsSurviveACacheWipe() = runBlocking { + fun accountsAndCredentialsSurviveACacheWipe() = runBlocking { seedVersion14Cache() AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt index 8ddae89..c8131c5 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDatabaseTest.kt @@ -49,7 +49,7 @@ class AccountDatabaseTest { ) @Test - fun credentialRoundTripsAndIsIndependentOfTheAccountRow() = runBlocking { + fun credentialRoundTripsAndIsIndependentOfTheAccountRow() = runBlocking { db.accountDao().upsert(account()) db.credentialDao().upsert(CredentialEntity("acct", "sealed-secret")) @@ -57,7 +57,7 @@ class AccountDatabaseTest { } @Test - fun accountSettingsRoundTripAndCascadeWithTheirAccount() = runBlocking { + fun accountSettingsRoundTripAndCascadeWithTheirAccount() = runBlocking { db.accountDao().upsert(account()) db.accountSettingsDao().upsert( AccountSettingsEntity("acct", signature = "Hi", signatureEnabled = false, notificationsEnabled = false), @@ -70,7 +70,7 @@ class AccountDatabaseTest { } @Test - fun signaturesCascadeWithTheirAccount() = runBlocking { + fun signaturesCascadeWithTheirAccount() = runBlocking { db.accountDao().upsert(account()) db.signatureDao().upsert(SignatureEntity("sig-1", "acct", "Work", "

Regards

", isDefault = true)) assertEquals(1, db.signatureDao().observeForAccount("acct").first().size) From 7529a8fa7d2e0079a44a7534d8b7c3c994ac8ac2 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 07:06:23 -0500 Subject: [PATCH 3/7] fix(test): correct AccountDataMigratorTest assertions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two on-device assertion failures (green on JVM compile, red on the emulator): - migratorDdlMatchesExportedAccountDatabaseSchema built its expected DDL by substituting the schema's `${TABLE_NAME}` placeholder with a backtick-wrapped name, but the exported createSql already wraps the placeholder in backticks — producing a double-backticked identifier that never matched the (correct, single-backticked) migrator DDL. Substitute the bare name so the guard compares like-for-like. - movesEveryAccountTableOutOfAPlaintextCache asserted signatureEnabled was false, but the seed row sets it to 1 (true). Assert the seeded values for both booleans so a true and a false each round-trip. The production migrator DDL and drop logic were already correct; only the tests were wrong. Co-Authored-By: Claude Fable 5 --- .../org/libremail/data/local/AccountDataMigratorTest.kt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt index 4eaf071..66e37d1 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -110,7 +110,9 @@ class AccountDataMigratorTest { assertEquals("smtp.example.org", account?.smtp?.host) assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) val settings = accountSettingsDao().get("acct") - assertEquals(false, settings?.signatureEnabled) + // Seeded signatureEnabled = 1, notificationsEnabled = 0: both booleans must round-trip. + assertEquals(true, settings?.signatureEnabled) + assertEquals(false, settings?.notificationsEnabled) assertEquals(6, settings?.retentionMonths) assertNull(settings?.retentionCount) val signatures = signatureDao().observeForAccount("acct").first() @@ -198,7 +200,7 @@ class AccountDataMigratorTest { for (i in 0 until entities.length()) { val entity = entities.getJSONObject(i) val table = entity.getString("tableName") - val expectedCreate = entity.getString("createSql").replace("\${TABLE_NAME}", "`$table`") + val expectedCreate = entity.getString("createSql").replace("\${TABLE_NAME}", table) assertEquals( "AccountDataMigrator DDL for `$table` must match the exported AccountDatabase schema", expectedCreate, @@ -211,7 +213,7 @@ class AccountDataMigratorTest { if (index.getString("name") == "index_signatures_accountId") { assertEquals( "AccountDataMigrator signatures index must match the exported schema", - index.getString("createSql").replace("\${TABLE_NAME}", "`$table`"), + index.getString("createSql").replace("\${TABLE_NAME}", table), AccountDataMigrator.SIGNATURES_INDEX_SQL, ) checkedIndex = true From ec5e3088c0adc029aea59837d8a7c4f79f635d9c Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 08:27:53 -0500 Subject: [PATCH 4/7] chore(schema): export v16 cache schema after rebase on main Main advanced to @Database v15 (the #66 folder hierarchyDelimiter migration). Renumbered the account-tables-drop migration 14->15 to 15->16 and bumped the cache DB to v16; this exports the v16 schema (main's v15 delimiter schema minus the moved account tables). Main's own 15.json is kept unchanged. Co-Authored-By: Claude Fable 5 --- .../16.json | 455 ++++++++++++++++++ 1 file changed, 455 insertions(+) create mode 100644 app/schemas/org.libremail.data.local.LibreMailDatabase/16.json diff --git a/app/schemas/org.libremail.data.local.LibreMailDatabase/16.json b/app/schemas/org.libremail.data.local.LibreMailDatabase/16.json new file mode 100644 index 0000000..c88d55d --- /dev/null +++ b/app/schemas/org.libremail.data.local.LibreMailDatabase/16.json @@ -0,0 +1,455 @@ +{ + "formatVersion": 1, + "database": { + "version": 16, + "identityHash": "b5c1a38d197cf1335d3092e413d55d0d", + "entities": [ + { + "tableName": "messages", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, `sender` TEXT NOT NULL, `senderEmail` TEXT NOT NULL, `subject` TEXT NOT NULL, `snippet` TEXT NOT NULL, `body` TEXT NOT NULL, `isHtml` INTEGER NOT NULL, `timestampMillis` INTEGER NOT NULL, `isRead` INTEGER NOT NULL, `isStarred` INTEGER NOT NULL, `folder` TEXT NOT NULL DEFAULT 'INBOX', `inInbox` INTEGER NOT NULL, `bodyFetched` INTEGER NOT NULL, `uid` INTEGER NOT NULL DEFAULT 0, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sender", + "columnName": "sender", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "senderEmail", + "columnName": "senderEmail", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "subject", + "columnName": "subject", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "snippet", + "columnName": "snippet", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "body", + "columnName": "body", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "isHtml", + "columnName": "isHtml", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "timestampMillis", + "columnName": "timestampMillis", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "isRead", + "columnName": "isRead", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "isStarred", + "columnName": "isStarred", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "folder", + "columnName": "folder", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "'INBOX'" + }, + { + "fieldPath": "inInbox", + "columnName": "inInbox", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "bodyFetched", + "columnName": "bodyFetched", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "uid", + "columnName": "uid", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + }, + "indices": [ + { + "name": "index_messages_accountId", + "unique": false, + "columnNames": [ + "accountId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_accountId` ON `${TABLE_NAME}` (`accountId`)" + }, + { + "name": "index_messages_timestampMillis", + "unique": false, + "columnNames": [ + "timestampMillis" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_timestampMillis` ON `${TABLE_NAME}` (`timestampMillis`)" + }, + { + "name": "index_messages_accountId_folder_uid", + "unique": false, + "columnNames": [ + "accountId", + "folder", + "uid" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_messages_accountId_folder_uid` ON `${TABLE_NAME}` (`accountId`, `folder`, `uid`)" + } + ] + }, + { + "tableName": "attachments", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`messageId` TEXT NOT NULL, `partIndex` INTEGER NOT NULL, `filename` TEXT NOT NULL, `mimeType` TEXT NOT NULL, `sizeBytes` INTEGER NOT NULL, PRIMARY KEY(`messageId`, `partIndex`), FOREIGN KEY(`messageId`) REFERENCES `messages`(`id`) ON UPDATE NO ACTION ON DELETE CASCADE )", + "fields": [ + { + "fieldPath": "messageId", + "columnName": "messageId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "partIndex", + "columnName": "partIndex", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "filename", + "columnName": "filename", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "mimeType", + "columnName": "mimeType", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "sizeBytes", + "columnName": "sizeBytes", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "messageId", + "partIndex" + ] + }, + "indices": [ + { + "name": "index_attachments_messageId", + "unique": false, + "columnNames": [ + "messageId" + ], + "orders": [], + "createSql": "CREATE INDEX IF NOT EXISTS `index_attachments_messageId` ON `${TABLE_NAME}` (`messageId`)" + } + ], + "foreignKeys": [ + { + "table": "messages", + "onDelete": "CASCADE", + "onUpdate": "NO ACTION", + "columns": [ + "messageId" + ], + "referencedColumns": [ + "id" + ] + } + ] + }, + { + "tableName": "outbox", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT NOT NULL, `toAddresses` TEXT NOT NULL, `ccAddresses` TEXT NOT NULL, `bccAddresses` TEXT NOT NULL DEFAULT '', `subject` TEXT NOT NULL, `body` TEXT NOT NULL, `createdAt` INTEGER NOT NULL, `lastError` TEXT, `bodyHtml` TEXT, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "toAddresses", + "columnName": "toAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "ccAddresses", + "columnName": "ccAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bccAddresses", + "columnName": "bccAddresses", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "''" + }, + { + "fieldPath": "subject", + "columnName": "subject", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "body", + "columnName": "body", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "createdAt", + "columnName": "createdAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "lastError", + "columnName": "lastError", + "affinity": "TEXT" + }, + { + "fieldPath": "bodyHtml", + "columnName": "bodyHtml", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + } + }, + { + "tableName": "drafts", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`id` TEXT NOT NULL, `accountId` TEXT, `toAddresses` TEXT NOT NULL, `ccAddresses` TEXT NOT NULL, `bccAddresses` TEXT NOT NULL DEFAULT '', `subject` TEXT NOT NULL, `body` TEXT NOT NULL, `updatedAt` INTEGER NOT NULL, `attachments` TEXT NOT NULL, `bodyHtml` TEXT, PRIMARY KEY(`id`))", + "fields": [ + { + "fieldPath": "id", + "columnName": "id", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT" + }, + { + "fieldPath": "toAddresses", + "columnName": "toAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "ccAddresses", + "columnName": "ccAddresses", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bccAddresses", + "columnName": "bccAddresses", + "affinity": "TEXT", + "notNull": true, + "defaultValue": "''" + }, + { + "fieldPath": "subject", + "columnName": "subject", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "body", + "columnName": "body", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "updatedAt", + "columnName": "updatedAt", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "attachments", + "columnName": "attachments", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "bodyHtml", + "columnName": "bodyHtml", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "id" + ] + } + }, + { + "tableName": "folders", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `fullName` TEXT NOT NULL, `displayName` TEXT NOT NULL, `role` TEXT NOT NULL, `selectable` INTEGER NOT NULL, `sortOrder` INTEGER NOT NULL, `specialUse` INTEGER NOT NULL DEFAULT 0, `hierarchyDelimiter` TEXT, PRIMARY KEY(`accountId`, `fullName`))", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "fullName", + "columnName": "fullName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "displayName", + "columnName": "displayName", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "role", + "columnName": "role", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "selectable", + "columnName": "selectable", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "sortOrder", + "columnName": "sortOrder", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "specialUse", + "columnName": "specialUse", + "affinity": "INTEGER", + "notNull": true, + "defaultValue": "0" + }, + { + "fieldPath": "hierarchyDelimiter", + "columnName": "hierarchyDelimiter", + "affinity": "TEXT" + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId", + "fullName" + ] + } + }, + { + "tableName": "backfill_progress", + "createSql": "CREATE TABLE IF NOT EXISTS `${TABLE_NAME}` (`accountId` TEXT NOT NULL, `folder` TEXT NOT NULL, `nextBeforeUid` INTEGER NOT NULL, `complete` INTEGER NOT NULL, PRIMARY KEY(`accountId`, `folder`))", + "fields": [ + { + "fieldPath": "accountId", + "columnName": "accountId", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "folder", + "columnName": "folder", + "affinity": "TEXT", + "notNull": true + }, + { + "fieldPath": "nextBeforeUid", + "columnName": "nextBeforeUid", + "affinity": "INTEGER", + "notNull": true + }, + { + "fieldPath": "complete", + "columnName": "complete", + "affinity": "INTEGER", + "notNull": true + } + ], + "primaryKey": { + "autoGenerate": false, + "columnNames": [ + "accountId", + "folder" + ] + } + } + ], + "setupQueries": [ + "CREATE TABLE IF NOT EXISTS room_master_table (id INTEGER PRIMARY KEY,identity_hash TEXT)", + "INSERT OR REPLACE INTO room_master_table (id,identity_hash) VALUES(42, 'b5c1a38d197cf1335d3092e413d55d0d')" + ] + } +} \ No newline at end of file From e53a553398aed9826caad554e1d1f48dbc4b788d Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 08:46:14 -0500 Subject: [PATCH 5/7] fix(security): copy account tables by shared columns, not SELECT * MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Device upgrade testing surfaced a crash: on a cache last written before v13, account_settings has 4 columns (accountId, signature, signatureEnabled, notificationsEnabled) but the destination table has 6 (retentionCount/retentionMonths were added at v13). The migrator ran `INSERT OR IGNORE INTO account_settings SELECT * FROM cache...`, which supplied 4 values for 6 columns and threw SQLiteException — and because the done-flag is only set after a successful copy, every launch re-ran and re-crashed (crash loop). AccountDataMigrator now copies each table by the column names present in BOTH the freshly-created destination and the (possibly older) source, so columns the source lacks take the destination's defaults instead of overflowing the value list. Verified on-device: the upgrade migrates a pre-v13 install cleanly and the account stays signed in (sync/backfill workers run). Regression test seeds a v12 cache and asserts the copy. Co-Authored-By: Claude Fable 5 --- .../data/local/AccountDataMigratorTest.kt | 34 +++++++++++++++++++ .../data/local/AccountDataMigrator.kt | 34 +++++++++++++++++-- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt index 66e37d1..eda28f0 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -187,6 +187,40 @@ class AccountDataMigratorTest { } } + @Test + fun copiesFromACacheOlderThanTheCurrentSchema() = runBlocking { + // A cache last written at v12 — before account_settings gained retentionCount/retentionMonths + // (v13). The copy must not choke on the columns the destination has but the source lacks + // (a device upgrade from an old install crashed the migrator here). + helper.createDatabase(cacheName, 12).apply { + execSQL( + "INSERT INTO accounts (id, email, displayName, authType, imap_host, imap_port, imap_security, " + + "smtp_host, smtp_port, smtp_security) VALUES ('acct', 'ada@example.org', 'Ada', " + + "'PASSWORD_IMAP', 'imap.example.org', 993, 'SSL_TLS', 'smtp.example.org', 465, 'SSL_TLS')", + ) + execSQL("INSERT INTO credentials (accountId, encryptedSecret) VALUES ('acct', 'sealed-secret')") + execSQL( + "INSERT INTO account_settings (accountId, signature, signatureEnabled, notificationsEnabled) " + + "VALUES ('acct', 'Sig', 0, 1)", + ) + close() + } + + AccountDataMigrator.copyAccountTables(cacheFile, cachePassphrase = "", accountsFile = accountsFile) + + openAccountsDb().apply { + assertEquals("ada@example.org", accountDao().getById("acct")?.email) + assertEquals("sealed-secret", credentialDao().getById("acct")?.encryptedSecret) + val settings = accountSettingsDao().get("acct") + assertEquals(false, settings?.signatureEnabled) + assertEquals(true, settings?.notificationsEnabled) + // Columns the v12 source lacked come across as the destination's defaults (null). + assertNull("retentionCount absent from a v12 cache must default to null", settings?.retentionCount) + assertNull(settings?.retentionMonths) + close() + } + } + @Test fun migratorDdlMatchesExportedAccountDatabaseSchema() { val schema = JSONObject( diff --git a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt index 8e5a50e..521bc81 100644 --- a/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt @@ -158,10 +158,15 @@ class AccountDataMigrator @Inject constructor( if (present.isEmpty()) return // fresh cache or already dropped: nothing to move TABLES.forEach { db.rawExecSQL(CREATE_TABLE_SQL.getValue(it)) } db.rawExecSQL(SIGNATURES_INDEX_SQL) - // Parent first so an enforced foreign key (Room enables them; this raw connection - // does not) would still be satisfied. INSERT OR IGNORE makes each copy idempotent. + // Copy by explicit shared column names, never SELECT *: the on-disk cache may predate + // columns the current schema added (e.g. account_settings gained retentionCount / + // retentionMonths at v13), and a bare SELECT * would then supply fewer values than the + // destination has columns and fail the whole migration. Listing the columns the source + // actually has lets the destination's newer columns take their defaults (NULL). Parent + // first so an enforced foreign key would still be satisfied; INSERT OR IGNORE is idempotent. TABLES.filter { it in present }.forEach { table -> - db.rawExecSQL("INSERT OR IGNORE INTO `$table` SELECT * FROM cache.`$table`") + val cols = sharedColumns(db, table) + db.rawExecSQL("INSERT OR IGNORE INTO `$table` ($cols) SELECT $cols FROM cache.`$table`") } Log.d(TAG, "moved account tables into the account database: $present") } finally { @@ -189,5 +194,28 @@ class AccountDataMigrator @Inject constructor( } return present } + + /** + * Column names present in BOTH the freshly-created destination `$table` (always the current + * schema) and the source `cache.$table` (possibly an older on-disk schema), backtick-quoted and + * comma-joined for an INSERT/SELECT column list. Destination-only columns are omitted so they + * take their defaults instead of overflowing the value list. + */ + private fun sharedColumns(db: SQLiteDatabase, table: String): String { + val source = tableColumns(db, "cache", table) + return tableColumns(db, "main", table) + .filter { it in source } + .joinToString(", ") { "`$it`" } + } + + /** The column names of `$schema.$table`, in declared order, via `PRAGMA table_info`. */ + private fun tableColumns(db: SQLiteDatabase, schema: String, table: String): List { + val columns = mutableListOf() + db.rawQuery("PRAGMA $schema.table_info(`$table`)", null).use { cursor -> + val nameIndex = cursor.getColumnIndexOrThrow("name") + while (cursor.moveToNext()) columns += cursor.getString(nameIndex) + } + return columns + } } } From f8d03a434330d0dcc352133390bfbc51cd33718f Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 08:49:52 -0500 Subject: [PATCH 6/7] perf(mailbox): page the unified "All inboxes" list (#124) The unified inbox query (WHERE folder = ?, no accountId) has no folder-leading index, so it scans in timestamp order and materializes the whole unified inbox (~4k rows at a 20k cache) into memory on every emission. Apply Paging 3 to the unified browse path so query, mapping, and recomposition cost scale with the visible window, not the total cache. - MessageDao.pagingUnifiedFolderSummaries: a PagingSource over the folder's synced rows (inInbox = 1); unified search keeps the whole-folder query so it can still surface transient server-search hits. - MailRepository.pagedUnifiedFolderMessages: a Pager (pageSize 40, initialLoad 120, no placeholders) mapping summaries to domain. - MailboxViewModel.pagedMessages: paged while browsing the unified inbox, else empty; the messages list flow stays empty in that state so the whole cache is never materialized. Selection captures each row's accountId at tap time, so "Move" still resolves the selection's account without an in-memory list. - MailboxScreen renders the unified browse list via collectAsLazyPagingItems; per-account and search views render the flat list unchanged (issue #86 stays flat). Profiling (docs/perf/issue-124-unified-inbox-paging.md) on an api29 emulator: current whole-inbox first-emit ~24.6 ms at a 20k cache vs. the paged first page ~6.8 ms and flat regardless of cache size (~3.6x). EXPLAIN QUERY PLAN shows the paged query still stops early on the existing timestamp index, so no (folder, ...) index and no schema migration are added. Co-Authored-By: Claude Fable 5 --- app/build.gradle.kts | 7 ++ .../kotlin/org/libremail/ui/Fakes.kt | 4 + .../libremail/data/local/dao/MessageDao.kt | 19 ++++ .../data/repository/MailRepositoryImpl.kt | 20 ++++ .../domain/repository/MailRepository.kt | 9 ++ .../org/libremail/ui/mailbox/MailboxScreen.kt | 58 +++++++++- .../libremail/ui/mailbox/MailboxViewModel.kt | 94 +++++++++++---- .../data/repository/MailRepositoryImplTest.kt | 25 ++++ .../ui/mailbox/MailboxViewModelTest.kt | 66 +++++++---- docs/perf/issue-124-unified-inbox-paging.md | 107 ++++++++++++++++++ docs/perf/issue-86-profiling.md | 7 +- gradle/libs.versions.toml | 10 ++ 12 files changed, 376 insertions(+), 50 deletions(-) create mode 100644 docs/perf/issue-124-unified-inbox-paging.md diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 08aad4f..4f62af4 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -200,9 +200,14 @@ dependencies { implementation(libs.androidx.room.runtime) implementation(libs.androidx.room.ktx) + implementation(libs.androidx.room.paging) ksp(libs.androidx.room.compiler) implementation(libs.sqlcipher.android) + // Paging 3 — the unified inbox list is paged so its cost scales with the screen (issue #124). + implementation(libs.androidx.paging.runtime) + implementation(libs.androidx.paging.compose) + // Raise kotlinx-serialization to the version Room's schema-bundle serializers were compiled // against (see libs.versions.toml). AGP 9 consistent resolution shares it with the androidTest // classpath so MigrationTestHelper can parse the exported schema JSON. @@ -214,6 +219,8 @@ dependencies { testImplementation(libs.turbine) testImplementation(libs.mockk) testImplementation(libs.greenmail) + // asSnapshot() drives a PagingData flow to a concrete list in JVM unit tests (issue #124). + testImplementation(libs.androidx.paging.testing) // The real org.json for unit tests (android.jar ships a stubbed, no-op version). testImplementation("org.json:json:20231013") diff --git a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt index 4bd4fe8..d9a4261 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.ui +import androidx.paging.PagingData import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.flowOf @@ -92,6 +93,9 @@ class FakeMailRepository( override fun observeUnifiedFolderMessages(folder: String): Flow> = flowOf(messages.filter { it.folder == folder }) + override fun pagedUnifiedFolderMessages(folder: String): Flow> = + flowOf(PagingData.from(messages.filter { it.folder == folder && it.inInbox })) + override fun observeFolders(accountId: String): Flow> = flowOf( folders.filter { it.accountId == diff --git a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt index ef86725..efe1096 100644 --- a/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt +++ b/app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.local.dao +import androidx.paging.PagingSource import androidx.room.Dao import androidx.room.Insert import androidx.room.OnConflictStrategy @@ -54,6 +55,24 @@ interface MessageDao { ) fun observeUnifiedFolderSummaries(folder: String): Flow> + /** + * Paged unified-inbox projection: folder-synced rows of [folder] across every account, + * newest-first, as a Paging 3 [PagingSource] (issue #124). Unlike [observeUnifiedFolderSummaries] + * — which materializes the *entire* unified inbox (~thousands of rows) on every emission — Room + * loads only the requested window (LIMIT/OFFSET), so the mailbox list's query, mapping, and + * recomposition cost scale with what's on screen, not the whole cache. Filters `inInbox = 1` + * because the paged browse list shows only synced rows; unified *search* (which must also surface + * transient `inInbox = 0` hits) stays on [observeUnifiedFolderSummaries]. Profiling (see + * `docs/perf/issue-124-unified-inbox-paging.md`) showed the first page loads flat regardless of + * total cache size on the existing indices, so no `(folder, …)` index / schema migration is added. + */ + @Query( + "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + + "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + + "WHERE folder = :folder AND inInbox = 1 ORDER BY timestampMillis DESC", + ) + fun pagingUnifiedFolderSummaries(folder: String): PagingSource + /** * Live per-(account, folder) unread counts for the drawer's folder badges and the bold styling of * accounts with unread mail. Counts only folder-synced rows (`inInbox = 1`), so transient diff --git a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt index 68bc9ed..bcfa1d8 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -3,6 +3,10 @@ package org.libremail.data.repository import android.content.Context import android.net.Uri +import androidx.paging.Pager +import androidx.paging.PagingConfig +import androidx.paging.PagingData +import androidx.paging.map import dagger.hilt.android.qualifiers.ApplicationContext import jakarta.mail.Flags import kotlinx.coroutines.flow.Flow @@ -66,6 +70,19 @@ class MailRepositoryImpl @Inject constructor( override fun observeUnifiedFolderMessages(folder: String): Flow> = messageDao.observeUnifiedFolderSummaries(folder).map { rows -> rows.map { it.toDomain() } } + override fun pagedUnifiedFolderMessages(folder: String): Flow> = Pager( + config = PagingConfig( + // A page comfortably exceeds a screenful so scrolling rarely waits on a load; loading + // three pages up front fills the first viewport without a visible gap. Placeholders + // are off: the row height varies (snippet/account label), so a fixed-height placeholder + // would jump, and the list never needs a scrollbar sized to the full (uncounted) inbox. + pageSize = MAILBOX_PAGE_SIZE, + initialLoadSize = MAILBOX_PAGE_SIZE * 3, + enablePlaceholders = false, + ), + pagingSourceFactory = { messageDao.pagingUnifiedFolderSummaries(folder) }, + ).flow.map { page -> page.map { it.toDomain() } } + override fun observeFolders(accountId: String): Flow> = folderDao.observeForAccount(accountId).map { rows -> rows.map { it.toDomain() } @@ -380,5 +397,8 @@ class MailRepositoryImpl @Inject constructor( private const val SEARCH_LIMIT = 50 +/** Rows per page for the unified inbox (issue #124) — a page is a few screenfuls of message rows. */ +private const val MAILBOX_PAGE_SIZE = 40 + /** Message id is ":"; the uid is the trailing segment. */ private fun uidOf(id: String): String = id.substringAfterLast(':') diff --git a/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt b/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt index 8a4adc0..8b79e00 100644 --- a/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt +++ b/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.domain.repository +import androidx.paging.PagingData import kotlinx.coroutines.flow.Flow import org.libremail.domain.model.Attachment import org.libremail.domain.model.Draft @@ -27,6 +28,14 @@ interface MailRepository { /** Like [observeFolderMessages] but for [folder] across every account (the unified inbox). */ fun observeUnifiedFolderMessages(folder: String): Flow> + /** + * The unified inbox as a [PagingData] stream so the list's query, mapping, and recomposition cost + * scale with the visible window rather than the whole cache (issue #124). Emits only folder-synced + * rows of [folder] across every account, newest-first — unified *search* still uses + * [observeUnifiedFolderMessages]. Callers must `cachedIn` a scope before collecting. + */ + fun pagedUnifiedFolderMessages(folder: String): Flow> + /** The account's cached IMAP folders for the navigation drawer. */ fun observeFolders(accountId: String): Flow> diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt index 663aeaa..3a5704c 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt @@ -80,6 +80,9 @@ import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import androidx.hilt.navigation.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle +import androidx.paging.LoadState +import androidx.paging.compose.collectAsLazyPagingItems +import androidx.paging.compose.itemKey import kotlinx.coroutines.launch import org.libremail.R import org.libremail.domain.model.Account @@ -102,6 +105,8 @@ fun MailboxScreen( viewModel: MailboxViewModel = hiltViewModel(), ) { val messages by viewModel.messages.collectAsStateWithLifecycle() + // The unified "All inboxes" browse list is paged (issue #124); per-account/search render [messages]. + val pagedMessages = viewModel.pagedMessages.collectAsLazyPagingItems() val accounts by viewModel.accounts.collectAsStateWithLifecycle() val selectedAccountId by viewModel.selectedAccountId.collectAsStateWithLifecycle() val selectedFolder by viewModel.selectedFolder.collectAsStateWithLifecycle() @@ -123,6 +128,10 @@ fun MailboxScreen( val moveTargetFolders by viewModel.moveTargetFolders.collectAsStateWithLifecycle() val actionInProgress by viewModel.actionInProgress.collectAsStateWithLifecycle() val selectionMode = selectedIds.isNotEmpty() + // The unified inbox (no account filter, not searching) renders the paged list; a concrete account + // or an active search renders the flat [messages] list. Hoisted here so the selection bar's + // "Select all" can read the right source (issue #124). + val showPaged = selectedAccountId == null && searchQuery.isBlank() var showMovePicker by remember { mutableStateOf(false) } val snackbarHostState = remember { SnackbarHostState() } val drawerState = rememberDrawerState(DrawerValue.Closed) @@ -187,7 +196,13 @@ fun MailboxScreen( onDelete = viewModel::requestDelete, onSpam = viewModel::requestSpam, onMove = { showMovePicker = true }, - onSelectAll = viewModel::selectAll, + onSelectAll = { + // "Select all" acts on what's shown: the loaded paged window for the unified + // inbox, or the whole flat list for a per-account/search view. + viewModel.selectAll( + if (showPaged) pagedMessages.itemSnapshotList.items else messages, + ) + }, onReply = { viewModel.reply(ReplyMode.REPLY) }, onReplyAll = viewModel::requestReplyAll, onForward = { viewModel.reply(ReplyMode.FORWARD) }, @@ -278,7 +293,42 @@ fun MailboxScreen( modifier = Modifier.fillMaxSize(), ) { LazyColumn(modifier = Modifier.fillMaxSize()) { - if (messages.isEmpty()) { + if (showPaged) { + if (pagedMessages.itemCount == 0) { + item { + // Hold the empty state back until the first page settles so + // it doesn't flash before rows arrive (search never lands here). + if (pagedMessages.loadState.refresh !is LoadState.Loading) { + NoMessagesState(Modifier.fillParentMaxSize()) + } + } + } else { + items( + count = pagedMessages.itemCount, + key = pagedMessages.itemKey { it.id }, + ) { index -> + val message = pagedMessages[index] ?: return@items + val accountLabel = + if (showAccount) accountsById[message.accountId]?.email else null + MessageRow( + message = message, + accountLabel = accountLabel, + selected = message.id in selectedIds, + onClick = { + if (selectionMode) { + viewModel.toggleSelection(message.id, message.accountId) + } else { + onOpenMessage(message.id) + } + }, + onLongClick = { + viewModel.startSelection(message.id, message.accountId) + }, + ) + HorizontalDivider() + } + } + } else if (messages.isEmpty()) { item { if (searchActive && searchQuery.isNotBlank()) { NoResultsState(Modifier.fillParentMaxSize()) @@ -296,12 +346,12 @@ fun MailboxScreen( selected = message.id in selectedIds, onClick = { if (selectionMode) { - viewModel.toggleSelection(message.id) + viewModel.toggleSelection(message.id, message.accountId) } else { onOpenMessage(message.id) } }, - onLongClick = { viewModel.startSelection(message.id) }, + onLongClick = { viewModel.startSelection(message.id, message.accountId) }, ) HorizontalDivider() } diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt index fe319ed..0a25be6 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt @@ -4,10 +4,13 @@ package org.libremail.ui.mailbox import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import androidx.paging.PagingData +import androidx.paging.cachedIn import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.FlowPreview import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow @@ -116,27 +119,62 @@ class MailboxViewModel @Inject constructor( private val _searchQuery = MutableStateFlow("") val searchQuery: StateFlow = _searchQuery.asStateFlow() + /** + * The list-rendered messages: a per-account folder (SQL-scoped and already flat, issue #86) or — + * for the unified inbox — *only* an active search's matches across accounts. The unified **browse** + * list is paged instead (see [pagedMessages], issue #124), so this flow stays empty while browsing + * the unified inbox and the whole cache is never pulled into memory on each write. + */ val messages: StateFlow> = combine(_selectedAccountId, _selectedFolder) { accountId, folder -> accountId to folder } .distinctUntilChanged() .flatMapLatest { (accountId, folder) -> - // Scope the query in SQL to the viewed account+folder (or [folder] across accounts for - // the unified inbox) so Room only re-queries/re-emits when those rows change, and the - // cost scales with the folder, not the whole cache (issue #86). - val scoped = if (accountId == null) { - mailRepository.observeUnifiedFolderMessages(folder) + if (accountId != null) { + // Per-account+folder: the SQL-scoped, already-flat list. The one client-side pass + // distinguishes the normal list (synced rows) from an active search (any matching + // row, including transient server-search hits) over the small folder-scoped set. + combine(mailRepository.observeFolderMessages(accountId, folder), _searchQuery) { rows, query -> + val q = query.trim() + rows.filter { if (q.isEmpty()) it.inInbox else it.matchesSearch(q) } + } } else { - mailRepository.observeFolderMessages(accountId, folder) - } - // The only remaining client-side pass distinguishes the normal list (synced rows) from - // an active search (any matching row, including transient server-search hits) — a match - // over the small folder-scoped set, never the whole cache. - combine(scoped, _searchQuery) { rows, query -> - val q = query.trim() - rows.filter { if (q.isEmpty()) it.inInbox else it.matchesSearch(q) } + // Unified inbox: browse is paged via [pagedMessages]; this backs only an active + // search over the folder's rows across accounts (bounded by the per-account search + // limit), staying empty while browsing so the whole inbox is never materialized. + _searchQuery.flatMapLatest { query -> + val q = query.trim() + if (q.isEmpty()) { + flowOf(emptyList()) + } else { + mailRepository.observeUnifiedFolderMessages(folder) + .map { rows -> rows.filter { it.matchesSearch(q) } } + } + } } }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) + /** + * The unified "All inboxes" browse list as a paged stream (issue #124): folder-synced rows across + * every account, newest-first, loaded a window at a time so query/mapping/recomposition cost scale + * with the screen, not the total cache. Emits empty paging data whenever a concrete account is + * selected or a search is active (those render from [messages]). + */ + val pagedMessages: Flow> = + combine( + _selectedAccountId, + _selectedFolder, + _searchQuery.map { it.isBlank() }.distinctUntilChanged(), + ) { accountId, folder, browsing -> Triple(accountId, folder, browsing) } + .distinctUntilChanged() + .flatMapLatest { (accountId, folder, browsing) -> + if (accountId == null && browsing) { + mailRepository.pagedUnifiedFolderMessages(folder) + } else { + flowOf(PagingData.empty()) + } + } + .cachedIn(viewModelScope) + val draftCount: StateFlow = mailRepository.observeDrafts() .map { it.size } .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), 0) @@ -156,6 +194,11 @@ class MailboxViewModel @Inject constructor( private val _selectedIds = MutableStateFlow>(emptySet()) val selectedIds: StateFlow> = _selectedIds.asStateFlow() + // Account id per selected message id, captured at selection time. The unified inbox is paged + // (issue #124), so the full list isn't held in memory; this lets [selectionAccountId] decide + // whether a selection sits within a single account (→ Move offered) without materializing it. + private val selectionAccounts = MutableStateFlow>(emptyMap()) + private val _pendingConfirm = MutableStateFlow(null) val pendingConfirm: StateFlow = _pendingConfirm.asStateFlow() @@ -173,9 +216,8 @@ class MailboxViewModel @Inject constructor( /** The single account every selected message belongs to, or null if the selection spans accounts. */ private val selectionAccountId: StateFlow = - combine(_selectedIds, messages) { ids, msgs -> - msgs.filter { it.id in ids }.map { it.accountId }.distinct().singleOrNull() - }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), null) + selectionAccounts.map { it.values.distinct().singleOrNull() } + .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), null) /** Whether "Move" is offered: only when the whole selection sits in a single account's folder tree. */ val canMove: StateFlow = selectionAccountId @@ -187,20 +229,30 @@ class MailboxViewModel @Inject constructor( .flatMapLatest { acct -> if (acct == null) flowOf(emptyList()) else mailRepository.observeFolders(acct) } .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) - fun startSelection(id: String) { + fun startSelection(id: String, accountId: String) { _selectedIds.value = setOf(id) + selectionAccounts.value = mapOf(id to accountId) } - fun toggleSelection(id: String) { - _selectedIds.value = _selectedIds.value.let { if (id in it) it - id else it + id } + fun toggleSelection(id: String, accountId: String) { + if (id in _selectedIds.value) { + _selectedIds.value = _selectedIds.value - id + selectionAccounts.value = selectionAccounts.value - id + } else { + _selectedIds.value = _selectedIds.value + id + selectionAccounts.value = selectionAccounts.value + (id to accountId) + } } fun clearSelection() { _selectedIds.value = emptySet() + selectionAccounts.value = emptyMap() } - fun selectAll() { - _selectedIds.value = messages.value.map { it.id }.toSet() + /** Selects everything currently shown; [items] is the visible list (paged snapshot or list). */ + fun selectAll(items: List) { + _selectedIds.value = items.map { it.id }.toSet() + selectionAccounts.value = items.associate { it.id to it.accountId } } fun archiveSelected() = runOnSelection { mailRepository.archive(it) } diff --git a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt index 2e43f53..ad92865 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt @@ -2,6 +2,9 @@ package org.libremail.data.repository import android.content.Context +import androidx.paging.PagingSource +import androidx.paging.PagingState +import androidx.paging.testing.asSnapshot import app.cash.turbine.test import io.mockk.Runs import io.mockk.coEvery @@ -105,6 +108,20 @@ class MailRepositoryImplTest { } } + @Test + fun `pagedUnifiedFolderMessages maps the paged summaries to domain messages`() = runTest { + every { messageDao.pagingUnifiedFolderSummaries("INBOX") } returns FakeSummaryPagingSource( + listOf(messageSummary("1", "INBOX"), messageSummary("2", "INBOX", accountId = "acct2")), + ) + + val items = repository.pagedUnifiedFolderMessages("INBOX").asSnapshot() + + assertEquals(listOf("1", "2"), items.map { it.id }) + assertEquals("Ada", items.first().sender) + // The list projection never carries a body — the reader loads it on demand (see MessageSummary). + assertEquals("", items.first().body) + } + @Test fun `observeFolders maps cached folders with their roles and server special-use flag`() = runTest { every { folderDao.observeForAccount("acct") } returns flowOf( @@ -604,4 +621,12 @@ class MailRepositoryImplTest { secret = "secret", useXoauth2 = false, ) + + /** Serves a fixed set of summaries as a single page, standing in for Room's generated source. */ + private class FakeSummaryPagingSource(private val rows: List) : + PagingSource() { + override fun getRefreshKey(state: PagingState): Int? = null + override suspend fun load(params: LoadParams): LoadResult = + LoadResult.Page(data = rows, prevKey = null, nextKey = null) + } } diff --git a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt index e9d3da2..c90ea90 100644 --- a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt @@ -2,6 +2,7 @@ package org.libremail.ui.mailbox import androidx.lifecycle.SavedStateHandle +import androidx.paging.PagingData import app.cash.turbine.test import io.mockk.coEvery import io.mockk.coVerify @@ -53,7 +54,7 @@ class MailboxViewModelTest { private val bob = account("imap:b", "bob@example.org") @Test - fun `default view shows only inbox messages across all accounts`() = runTest(testDispatcher) { + fun `unified browse keeps the whole inbox out of the in-memory list flow`() = runTest(testDispatcher) { val vm = createViewModel( accounts = listOf(alice, bob), messages = listOf( @@ -66,7 +67,24 @@ class MailboxViewModelTest { assertEquals("INBOX", vm.selectedFolder.value) assertNull(vm.selectedAccountId.value) - assertEquals(setOf("imap:a:INBOX:1", "imap:b:INBOX:1"), vm.messages.value.map { it.id }.toSet()) + // Unified browse is paged (issue #124): the flat list flow stays empty so the whole unified + // inbox is never materialized on every write. The paged rows themselves are covered by + // MailRepositoryImplTest's pagedUnifiedFolderMessages test and the MailboxScreen UI test. + assertTrue(vm.messages.value.isEmpty()) + } + + @Test + fun `selecting a concrete account renders the flat scoped list`() = runTest(testDispatcher) { + val vm = createViewModel( + accounts = listOf(alice), + messages = listOf(msg("imap:a:INBOX:1", "imap:a", "INBOX"), msg("imap:a:Archive:1", "imap:a", "Archive")), + ) + backgroundScope.launch { vm.messages.collect {} } + + vm.selectAccount("imap:a") + + // A concrete account uses the SQL-scoped, already-flat list (issue #86), not the paged path. + assertEquals(listOf("imap:a:INBOX:1"), vm.messages.value.map { it.id }) } @Test @@ -200,11 +218,11 @@ class MailboxViewModelTest { fun `toggle adds then removes a message from the selection`() = runTest(testDispatcher) { val vm = createViewModel(accounts = listOf(alice), messages = emptyList()) - vm.startSelection("a") + vm.startSelection("a", "acct") assertEquals(setOf("a"), vm.selectedIds.value) - vm.toggleSelection("b") + vm.toggleSelection("b", "acct") assertEquals(setOf("a", "b"), vm.selectedIds.value) - vm.toggleSelection("a") + vm.toggleSelection("a", "acct") assertEquals(setOf("b"), vm.selectedIds.value) vm.clearSelection() assertTrue(vm.selectedIds.value.isEmpty()) @@ -212,13 +230,11 @@ class MailboxViewModelTest { @Test fun `selectAll selects every visible message`() = runTest(testDispatcher) { - val vm = createViewModel( - accounts = listOf(alice), - messages = listOf(msg("imap:a:INBOX:1", "imap:a", "INBOX"), msg("imap:a:INBOX:2", "imap:a", "INBOX")), - ) - backgroundScope.launch { vm.messages.collect {} } + val shown = listOf(msg("imap:a:INBOX:1", "imap:a", "INBOX"), msg("imap:a:INBOX:2", "imap:a", "INBOX")) + val vm = createViewModel(accounts = listOf(alice), messages = shown) - vm.selectAll() + // The screen passes what's shown (paged snapshot or flat list); the VM records their ids. + vm.selectAll(shown) assertEquals(setOf("imap:a:INBOX:1", "imap:a:INBOX:2"), vm.selectedIds.value) } @@ -234,7 +250,7 @@ class MailboxViewModelTest { ) backgroundScope.launch { vm.messages.collect {} } - vm.startSelection("imap:a:INBOX:1") + vm.startSelection("imap:a:INBOX:1", "imap:a") vm.requestDelete() val pending = vm.pendingConfirm.value assertTrue(pending is PendingAction.Delete && !pending.permanent) @@ -265,7 +281,7 @@ class MailboxViewModelTest { backgroundScope.launch { vm.currentFolderRole.collect {} } vm.selectFolder("imap:a", "Spam") - vm.startSelection("imap:a:Spam:1") + vm.startSelection("imap:a:Spam:1", "imap:a") vm.requestDelete() val pending = vm.pendingConfirm.value assertTrue(pending is PendingAction.Delete && pending.permanent) @@ -285,7 +301,7 @@ class MailboxViewModelTest { repo = repo, ) - vm.startSelection("imap:a:INBOX:1") + vm.startSelection("imap:a:INBOX:1", "imap:a") vm.requestSpam() assertTrue(vm.pendingConfirm.value is PendingAction.Spam) coVerify(exactly = 0) { repo.reportSpam(any()) } @@ -304,7 +320,7 @@ class MailboxViewModelTest { repo = repo, ) - vm.startSelection("imap:a:INBOX:1") + vm.startSelection("imap:a:INBOX:1", "imap:a") vm.reply(ReplyMode.FORWARD) assertEquals(MailboxEvent.OpenCompose("draft1"), vm.events.first()) @@ -321,7 +337,7 @@ class MailboxViewModelTest { repo = repo, ) - vm.startSelection("imap:a:INBOX:1") + vm.startSelection("imap:a:INBOX:1", "imap:a") vm.archiveSelected() coVerify { repo.archive(listOf("imap:a:INBOX:1")) } @@ -338,7 +354,7 @@ class MailboxViewModelTest { repo = repo, ) - vm.startSelection("imap:a:INBOX:1") + vm.startSelection("imap:a:INBOX:1", "imap:a") vm.reply(ReplyMode.REPLY) assertEquals(MailboxEvent.OpenCompose("d2"), vm.events.first()) @@ -352,7 +368,7 @@ class MailboxViewModelTest { messages = listOf(msg("imap:a:INBOX:1", "imap:a", "INBOX")), repo = repo, ) - vm.startSelection("imap:a:INBOX:1") + vm.startSelection("imap:a:INBOX:1", "imap:a") vm.requestSpam() vm.dismissConfirm() @@ -371,10 +387,10 @@ class MailboxViewModelTest { backgroundScope.launch { vm.messages.collect {} } backgroundScope.launch { vm.canMove.collect {} } - vm.startSelection("imap:a:INBOX:1") + vm.startSelection("imap:a:INBOX:1", "imap:a") assertEquals(true, vm.canMove.value) - vm.toggleSelection("imap:b:INBOX:1") + vm.toggleSelection("imap:b:INBOX:1", "imap:b") assertEquals(false, vm.canMove.value) } @@ -388,7 +404,7 @@ class MailboxViewModelTest { repo = repo, ) - vm.startSelection("imap:a:INBOX:1") + vm.startSelection("imap:a:INBOX:1", "imap:a") vm.moveSelected("Receipts") coVerify { repo.moveToFolder(listOf("imap:a:INBOX:1"), "Receipts") } @@ -404,7 +420,7 @@ class MailboxViewModelTest { messages = listOf(msg("imap:a:INBOX:1", "imap:a", "INBOX")), repo = repo, ) - vm.startSelection("imap:a:INBOX:1") + vm.startSelection("imap:a:INBOX:1", "imap:a") vm.requestReplyAll() assertTrue(vm.pendingConfirm.value is PendingAction.ReplyAll) @@ -493,6 +509,12 @@ class MailboxViewModelTest { val folder = firstArg() MutableStateFlow(messages.filter { it.folder == folder }) } + // The unified browse list is paged (issue #124); mirror the DAO's inInbox-scoped, folder-scoped + // projection as a single static page. + every { repo.pagedUnifiedFolderMessages(any()) } answers { + val folder = firstArg() + flowOf(PagingData.from(messages.filter { it.folder == folder && it.inInbox })) + } every { repo.observeDrafts() } returns flowOf(emptyList()) every { repo.observeOutbox() } returns flowOf(emptyList()) every { repo.observeUnreadCounts() } returns MutableStateFlow(unreadCounts) diff --git a/docs/perf/issue-124-unified-inbox-paging.md b/docs/perf/issue-124-unified-inbox-paging.md new file mode 100644 index 0000000..239cd48 --- /dev/null +++ b/docs/perf/issue-124-unified-inbox-paging.md @@ -0,0 +1,107 @@ + +# Issue #124 — paging the unified "All inboxes" view + +Follow-up to #86. PR #123 made the per-account+folder list flat by scoping the query in SQL, but the +unified inbox (`WHERE folder = ?`, no `accountId`) has no `folder`-leading index, so it still **scans** +in timestamp order and — worse — materializes the **entire** unified inbox (~thousands of rows) into +memory on every emission. This measures that path **before** the fix and **with** Paging 3, on a large +seeded cache, to decide whether an index (schema migration) is actually needed once paged. + +## TL;DR / verdict + +- **Paging fixes it; no index, no schema migration.** The current whole-unified-inbox query grows + linearly with the number of INBOX rows (**~6.8 ms at 1k INBOX → ~24.6 ms at 4k INBOX**), because it + reads and maps every INBOX row across all accounts. The paged first page (the production initial + load of 120 rows) is **flat at ~5–7 ms regardless of total cache size** — it stops as soon as it has + a screenful — a **~3.6× first-emit speedup at a 20k cache**, and, more importantly, it stays flat as + the cache keeps growing (full-history backfill, #12/#13) while the current path keeps rising. +- **`EXPLAIN QUERY PLAN` still shows a `SCAN … index_messages_timestampMillis`** for the paged query on + the existing indices — i.e. no `folder`-leading index is used — yet the paged first page is already + flat, because the `LIMIT` lets the planner stop early. A `(folder, timestampMillis)` / + `(folder, inInbox, timestampMillis)` index would only materially help a **deep scroll** (large + `OFFSET`), which is rare and already bounded by scroll depth (not total cache), and even the worst + deep page measured (40 rows at offset ~3.9k) is **faster than the current whole-inbox load**. So the + ticket's strong preference holds: **Paging 3 alone captures the win — no `(folder, …)` index, no + v15→v16 migration, no #118 version coordination.** +- **Per-account views are untouched** (already flat from #86); only the unified browse list is paged. + +## Setup & method + +- Device: Gradle-managed AVD `libremail_api29` (API 29, x86, google_apis), the same device #86 used, so + the figures are comparable. Cross-checked on a physical Pixel (API 37) — same shape (see below). +- Harness: a throwaway instrumented probe (`UnifiedInboxPagingProbe`, removed after measuring, like + #86's) built the real `LibreMailDatabase` in-memory (real entities, real indices, real generated + `MessageDao`), seeded it, and timed each variant with **5 warmup + 15 measured iterations**, reporting + **median and min** wall-clock ms with a GC between phases. `androidx.benchmark` was deliberately not + used (as in #86: on an emulator only the **relative** A/B result is meaningful, and it would force the + module's instrumentation runner, changing what CI's E2E jobs run under). +- Dataset: rows spread across **3 accounts × {INBOX, Sent, Archive, Spam, Work}** (INBOX ≈ 20%, so at + 20k the unified inbox is ~4k rows), timestamps interleaved across folders (a realistic worst case — + INBOX rows are **not** clustered at the top of the timestamp index), each row carrying a ~1 KB body so + rows are realistically sized (the summary projection never selects the body). +- Variants: (a) **CURRENT** = `observeUnifiedFolderSummaries("INBOX")` first Flow emission (materializes + the whole unified inbox); (b) **PAGED first page** = `pagingUnifiedFolderSummaries("INBOX")` loaded via + a `Refresh(key = null, loadSize = 120)` — the production `initialLoadSize` (3 × pageSize); (c) **PAGED + deep page** = a `Refresh(key ≈ inboxCount − 60, loadSize = 40)` window near the end of the inbox + (models scrolling to the bottom). Each query's `EXPLAIN QUERY PLAN` was captured with existing indices + only. + +## Results (median / min ms, lower is better) + +| total rows | INBOX rows | CURRENT whole-inbox first-emit | PAGED first page (120) | PAGED deep page (40 @ ~end) | +|-----------:|-----------:|-------------------------------:|-----------------------:|----------------------------:| +| 5 000 | 1 000 | **6.80** / 4.93 | 5.44 / 4.64 | 3.67 / 2.83 | +| 20 000 | 4 000 | **24.56** / 18.89 | 6.82 / 5.67 | 12.86 / 10.52 | + +- **CURRENT scales with the inbox size:** 6.8 → 24.6 ms as INBOX rows go 1k → 4k (≈ linear); this cost + is paid on **every** re-emission (any write to `messages` — IDLE delivery, a read/star toggle, a + backfill page, a sync of any folder), and it also builds a full `List` of every INBOX row. +- **PAGED first page is flat:** ~5.4 → ~6.8 ms (within noise) — independent of the total cache, because + the scan stops once it has 120 INBOX rows. This is the common case (opening / reading the top of the + inbox), and it's what the acceptance criterion asks for. +- **PAGED deep page** (40 rows at offset ~3.9k) costs 12.9 ms at 20k — more than the first page (the + `OFFSET` walks ~3.9k INBOX matches) but still **below the current whole-inbox load**, returns only 40 + rows (vs. the current path's 4 000), and scales with **scroll depth**, not total cache. + +Cross-check on a physical **Pixel (API 37)** at 20k: CURRENT 27.2 / 24.8, PAGED first page 8.6 / 7.2, +PAGED deep page 15.3 / 12.4 ms — same shape (~3.2× first-page speedup). + +## Query plans (`EXPLAIN QUERY PLAN`, existing indices only) + +``` +CURRENT whole-inbox : SCAN TABLE messages USING INDEX index_messages_timestampMillis +PAGED first page : SCAN TABLE messages USING INDEX index_messages_timestampMillis (LIMIT 120) +PAGED deep page : SCAN TABLE messages USING INDEX index_messages_timestampMillis (LIMIT 40 OFFSET ~3.9k) +``` + +- All three walk `index_messages_timestampMillis` newest-first (no `folder`-leading index), filtering + `folder`/`inInbox` per row. The paged queries differ only by the `LIMIT`/`OFFSET` the planner applies, + which is exactly what makes the first page cheap: it stops after a screenful. +- A `(folder, inInbox, timestampMillis)` index would turn the scan into a seek and remove the deep-page + `OFFSET` walk. It is **not added**: the first page — the case the ticket targets — is already flat on + the existing indices, deep scroll is rare and bounded by depth, and avoiding the index avoids a + schema migration (v15→v16) and the #118 version-coordination it would require. Revisit only if deep + scrolling the unified inbox becomes a measured problem. + +## What was implemented + +- `MessageDao.pagingUnifiedFolderSummaries(folder)` — a Paging 3 `PagingSource` + over `WHERE folder = ? AND inInbox = 1 ORDER BY timestampMillis DESC` (synced rows only; unified + **search** keeps using `observeUnifiedFolderSummaries`, which also surfaces transient `inInbox = 0` + hits). +- `MailRepository.pagedUnifiedFolderMessages(folder)` — wraps it in a `Pager` + (`pageSize = 40`, `initialLoadSize = 120`, no placeholders) and maps summaries to domain. +- `MailboxViewModel.pagedMessages` — `flatMapLatest` to the paged flow while browsing the unified inbox + (no account selected, no active search), else empty paging data; `cachedIn(viewModelScope)`. The old + `messages` list flow now stays **empty** while browsing the unified inbox, so the whole inbox is never + pulled into memory; it still serves per-account views and unified **search**. Selection carries each + row's `accountId` (captured at tap time) so "Move" still works without a full in-memory list. +- `MailboxScreen` renders the unified browse list via `collectAsLazyPagingItems()`; per-account/search + render the flat list as before. + +## Reproduce + +Seed a large multi-account `messages` cache, then compare `observeUnifiedFolderSummaries("INBOX")` +(first emit) against `pagingUnifiedFolderSummaries("INBOX")` loaded with a `Refresh(null, 120)` on an +emulator, and inspect `EXPLAIN QUERY PLAN`. The paged first page should be ~flat (~5–7 ms) regardless +of total cache; the whole-inbox query should grow with the INBOX row count. diff --git a/docs/perf/issue-86-profiling.md b/docs/perf/issue-86-profiling.md index e51dbf7..e9edc15 100644 --- a/docs/perf/issue-86-profiling.md +++ b/docs/perf/issue-86-profiling.md @@ -114,9 +114,10 @@ UNIFIED folder-only : SCAN TABLE messages USING INDEX index_messages_timestampMi ## Deferred follow-ups -- **Unified inbox**: add Room `PagingSource`/Paging3 so the unified list's cost scales with the screen, - not the total inbox count; optionally a `(folder, timestampMillis)` index to turn the scan into a - seek. (The per-account views are already flat, so Paging3 there is lower priority.) +- **Unified inbox**: ~~add Room `PagingSource`/Paging3 so the unified list's cost scales with the + screen, not the total inbox count~~ — done in #124 (`docs/perf/issue-124-unified-inbox-paging.md`). + Paging alone captured the win (first page ~flat at ~5–7 ms vs. the current ~25 ms at a 20k cache); + the `(folder, timestampMillis)` index proved unnecessary, so no schema migration was added. - **IMAP latency on folder open** is out of scope for this cached-render fix. ## Reproduce diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index b9540e7..ac5b242 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -12,6 +12,7 @@ webkit = "1.12.1" biometric = "1.1.0" composeBom = "2026.06.00" room = "2.8.4" +paging = "3.3.6" sqlcipher = "4.16.0" datastore = "1.2.1" work = "2.11.2" @@ -72,9 +73,18 @@ androidx-room-ktx = { group = "androidx.room", name = "room-ktx", version.ref = androidx-room-compiler = { group = "androidx.room", name = "room-compiler", version.ref = "room" } # Room MigrationTestHelper (instrumented migration tests). androidx-room-testing = { group = "androidx.room", name = "room-testing", version.ref = "room" } +# Room PagingSource support — lets @Query methods return androidx.paging.PagingSource. +androidx-room-paging = { group = "androidx.room", name = "room-paging", version.ref = "room" } # SQLCipher — opt-in at-rest encryption of the Room cache. sqlcipher-android = { group = "net.zetetic", name = "sqlcipher-android", version.ref = "sqlcipher" } +# Paging 3 — pages the unified "All inboxes" list so its query/recomposition cost scales with the +# visible window, not the whole cache (issue #124). runtime = Pager/PagingData; compose = +# collectAsLazyPagingItems; testing = asSnapshot for JVM unit tests. +androidx-paging-runtime = { group = "androidx.paging", name = "paging-runtime", version.ref = "paging" } +androidx-paging-compose = { group = "androidx.paging", name = "paging-compose", version.ref = "paging" } +androidx-paging-testing = { group = "androidx.paging", name = "paging-testing", version.ref = "paging" } + # DataStore (settings) / WorkManager (sync) — wired in later increments androidx-datastore-preferences = { group = "androidx.datastore", name = "datastore-preferences", version.ref = "datastore" } androidx-work-runtime-ktx = { group = "androidx.work", name = "work-runtime-ktx", version.ref = "work" } From ae37823aec4e05377f52f664b667416432ae3be0 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 09:56:56 -0500 Subject: [PATCH 7/7] feat(reader): collapse extra attachments into an accordion A message with several attachments used to render every AttachmentRow stacked vertically, pushing the message body arbitrarily far down. Now only the first attachment shows by default; when there is more than one, the extras collapse behind a "See x more attachments" control that expands and collapses with an animated, rotating chevron. A single attachment renders exactly as before (no accordion). The count uses a plurals resource (quantity one/other) so it reads "See 1 more attachment" / "See 2 more attachments" correctly. The toggle is one clickable Role.Button whose label and chevron contentDescription expose the expanded state to screen readers. Download/open behavior of each row is unchanged. Refs #134 Co-Authored-By: Claude Fable 5 --- .../libremail/ui/reader/ReaderScreenTest.kt | 66 ++++++++++++-- .../org/libremail/ui/reader/ReaderScreen.kt | 88 +++++++++++++++++-- app/src/main/res/values/strings.xml | 8 ++ 3 files changed, 147 insertions(+), 15 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/ui/reader/ReaderScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/reader/ReaderScreenTest.kt index 1ccae3e..e7928b1 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/reader/ReaderScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/reader/ReaderScreenTest.kt @@ -6,6 +6,8 @@ import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.junit4.createAndroidComposeRule import androidx.compose.ui.test.onAllNodesWithText import androidx.compose.ui.test.onNodeWithContentDescription +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick import androidx.lifecycle.SavedStateHandle import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry @@ -20,7 +22,7 @@ import org.libremail.ui.FakeMailRepository import org.libremail.ui.navigation.Routes import org.libremail.ui.theme.LibreMailTheme -/** End-to-end test that the reader marks an already-cached attachment as downloaded. */ +/** Compose UI tests for the reader's attachment list: the downloaded indicator and the accordion. */ @RunWith(AndroidJUnit4::class) class ReaderScreenTest { @@ -29,6 +31,9 @@ class ReaderScreenTest { private fun string(resId: Int) = composeTestRule.activity.getString(resId) + private fun seeMore(extraCount: Int) = + composeTestRule.activity.resources.getQuantityString(R.plurals.attachments_see_more, extraCount, extraCount) + private val messageId = "imap:a:INBOX:1" private val message = Message( id = messageId, accountId = "imap:a", sender = "Sender", senderEmail = "s@example.org", @@ -36,29 +41,74 @@ class ReaderScreenTest { isRead = true, isStarred = false, ) - @Test - fun reader_showsDownloadedIndicator_forCachedAttachment() { + private fun attachment(partIndex: Int, filename: String) = + Attachment(messageId, partIndex, filename, "application/pdf", 1_000L) + + /** Renders [ReaderScreen] for the fixed [message] with the given [attachments] and awaits load. */ + private fun renderReader(attachments: List, downloadedParts: Set = emptySet()) { val context = InstrumentationRegistry.getInstrumentation().targetContext.applicationContext val repo = FakeMailRepository( messages = listOf(message), - attachments = listOf(Attachment(messageId, 0, "report.pdf", "application/pdf", 1234L)), - downloadedParts = setOf(0), + attachments = attachments, + downloadedParts = downloadedParts, ) val viewModel = ReaderViewModel( SavedStateHandle(mapOf(Routes.READER_ARG_ID to messageId)), repo, SettingsRepository(context), ) - composeTestRule.setContent { LibreMailTheme(darkTheme = false, dynamicColor = false) { ReaderScreen(onBack = {}, onReply = { _, _, _ -> }, viewModel = viewModel) } } - composeTestRule.waitUntil(5_000) { - composeTestRule.onAllNodesWithText("report.pdf").fetchSemanticsNodes().isNotEmpty() + composeTestRule.onAllNodesWithText(attachments.first().filename).fetchSemanticsNodes().isNotEmpty() } + } + + @Test + fun reader_showsDownloadedIndicator_forCachedAttachment() { + renderReader(listOf(attachment(0, "report.pdf")), downloadedParts = setOf(0)) + + composeTestRule.onNodeWithText("report.pdf").assertIsDisplayed() composeTestRule.onNodeWithContentDescription(string(R.string.attachment_downloaded)).assertIsDisplayed() } + + @Test + fun reader_singleAttachment_showsNoAccordion() { + renderReader(listOf(attachment(0, "solo.pdf"))) + + composeTestRule.onNodeWithText("solo.pdf").assertIsDisplayed() + // A lone attachment has no "See more" control. + composeTestRule.onNodeWithContentDescription(string(R.string.attachments_expand)).assertDoesNotExist() + } + + @Test + fun reader_multipleAttachments_collapseExtrasUntilExpanded() { + renderReader(listOf(attachment(0, "one.pdf"), attachment(1, "two.pdf"), attachment(2, "three.pdf"))) + + // First row shown; the two extras are hidden behind the collapsed accordion. + composeTestRule.onNodeWithText("one.pdf").assertIsDisplayed() + composeTestRule.onNodeWithText(seeMore(2)).assertIsDisplayed() + composeTestRule.onNodeWithText("two.pdf").assertDoesNotExist() + composeTestRule.onNodeWithText("three.pdf").assertDoesNotExist() + + // Tapping the control reveals the remaining rows. + composeTestRule.onNodeWithText(seeMore(2)).performClick() + composeTestRule.waitUntil(5_000) { + composeTestRule.onAllNodesWithText("two.pdf").fetchSemanticsNodes().isNotEmpty() + } + composeTestRule.onNodeWithText("two.pdf").assertIsDisplayed() + composeTestRule.onNodeWithText("three.pdf").assertIsDisplayed() + } + + @Test + fun reader_twoAttachments_useSingularPlural() { + renderReader(listOf(attachment(0, "a.pdf"), attachment(1, "b.pdf"))) + + // Exactly one extra: the singular plural form, e.g. "See 1 more attachment". + composeTestRule.onNodeWithText(seeMore(1)).assertIsDisplayed() + composeTestRule.onNodeWithText("b.pdf").assertDoesNotExist() + } } diff --git a/app/src/main/kotlin/org/libremail/ui/reader/ReaderScreen.kt b/app/src/main/kotlin/org/libremail/ui/reader/ReaderScreen.kt index 7ad51ac..4b3895b 100644 --- a/app/src/main/kotlin/org/libremail/ui/reader/ReaderScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/reader/ReaderScreen.kt @@ -4,6 +4,8 @@ package org.libremail.ui.reader import android.content.ActivityNotFoundException import android.content.Context import android.content.Intent +import androidx.compose.animation.AnimatedVisibility +import androidx.compose.animation.core.animateFloatAsState import androidx.compose.foundation.background import androidx.compose.foundation.clickable import androidx.compose.foundation.layout.Box @@ -25,6 +27,7 @@ import androidx.compose.material.icons.Icons import androidx.compose.material.icons.automirrored.filled.ArrowBack import androidx.compose.material.icons.filled.Check import androidx.compose.material.icons.filled.Delete +import androidx.compose.material.icons.filled.KeyboardArrowDown import androidx.compose.material.icons.filled.Star import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.ExperimentalMaterial3Api @@ -42,12 +45,18 @@ import androidx.compose.material3.TopAppBar import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.saveable.rememberSaveable +import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip +import androidx.compose.ui.draw.rotate import androidx.compose.ui.platform.LocalContext +import androidx.compose.ui.res.pluralStringResource import androidx.compose.ui.res.stringResource +import androidx.compose.ui.semantics.Role import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import androidx.core.content.FileProvider @@ -213,18 +222,83 @@ private fun Attachments( color = MaterialTheme.colorScheme.onSurfaceVariant, ) Spacer(Modifier.height(8.dp)) - attachments.forEach { attachment -> - AttachmentRow( - attachment = attachment, - downloading = attachment.partIndex in downloading, - downloaded = attachment.partIndex in downloaded, - onClick = { onDownload(attachment) }, + // The first attachment always shows. Any extras collapse behind an accordion so a message + // with many attachments can't push its body off-screen (#134). + val first = attachments.first() + AttachmentRow( + attachment = first, + downloading = first.partIndex in downloading, + downloaded = first.partIndex in downloaded, + onClick = { onDownload(first) }, + ) + Spacer(Modifier.height(8.dp)) + val extras = attachments.drop(1) + if (extras.isNotEmpty()) { + var expanded by rememberSaveable { mutableStateOf(false) } + AttachmentsToggle( + extraCount = extras.size, + expanded = expanded, + onToggle = { expanded = !expanded }, ) - Spacer(Modifier.height(8.dp)) + AnimatedVisibility(visible = expanded) { + Column { + extras.forEach { attachment -> + Spacer(Modifier.height(8.dp)) + AttachmentRow( + attachment = attachment, + downloading = attachment.partIndex in downloading, + downloaded = attachment.partIndex in downloaded, + onClick = { onDownload(attachment) }, + ) + } + } + } } } } +/** + * Collapsed-by-default control that reveals the 2nd..Nth attachments. It is a single clickable + * [Role.Button] whose label ("See x more attachments" / "See fewer attachments") and rotating + * chevron expose the expanded state to screen readers. + */ +@Composable +private fun AttachmentsToggle(extraCount: Int, expanded: Boolean, onToggle: () -> Unit) { + val label = if (expanded) { + stringResource(R.string.attachments_see_fewer) + } else { + pluralStringResource(R.plurals.attachments_see_more, extraCount, extraCount) + } + val chevronDescription = stringResource( + if (expanded) R.string.attachments_collapse else R.string.attachments_expand, + ) + val rotation by animateFloatAsState( + targetValue = if (expanded) 180f else 0f, + label = "attachmentsChevronRotation", + ) + Row( + modifier = Modifier + .fillMaxWidth() + .clip(MaterialTheme.shapes.small) + .clickable(role = Role.Button, onClick = onToggle) + .padding(vertical = 12.dp), + verticalAlignment = Alignment.CenterVertically, + ) { + Icon( + imageVector = Icons.Filled.KeyboardArrowDown, + contentDescription = chevronDescription, + tint = MaterialTheme.colorScheme.primary, + modifier = Modifier.rotate(rotation), + ) + Spacer(Modifier.width(8.dp)) + Text( + text = label, + style = MaterialTheme.typography.labelLarge, + color = MaterialTheme.colorScheme.primary, + ) + } +} + @Composable private fun AttachmentRow(attachment: Attachment, downloading: Boolean, downloaded: Boolean, onClick: () -> Unit) { Surface( diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 8521a5d..687db18 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -131,6 +131,14 @@ Available offline Couldn\'t download %1$s No app can open this file + + + See %1$d more attachment + See %1$d more attachments + + See fewer attachments + Expand attachments + Collapse attachments Welcome to LibreMail