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/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/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 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..eda28f0 --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/data/local/AccountDataMigratorTest.kt @@ -0,0 +1,265 @@ +// 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") + // 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() + 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 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( + 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..c8131c5 --- /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/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/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/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/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..521bc81 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/AccountDataMigrator.kt @@ -0,0 +1,221 @@ +// 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) + // 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 -> + 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 { + 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 + } + + /** + * 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 + } + } +} 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/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/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() 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/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 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" }