diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRoutingTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRoutingTest.kt new file mode 100644 index 0000000..e1bb15f --- /dev/null +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRoutingTest.kt @@ -0,0 +1,118 @@ +// 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.runBlocking +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.libremail.data.local.dao.MessageDao +import org.libremail.data.local.entity.MessageEntity + +/** + * Real-SQLite behavior of the body-less routing projection [MessageDao.getRouting] / + * [MessageDao.getRoutingByIds] (issue #186). The open path and the flag/move callers route on these + * instead of pulling the whole `body` blob through [MessageDao.getById]. These tests pin that the + * projection maps every routing/flag column correctly (and can do so for a row whose body is large, + * which is exactly the over-fetch the projection avoids). + */ +@RunWith(AndroidJUnit4::class) +class MessageDaoRoutingTest { + + private lateinit var db: LibreMailDatabase + private lateinit var dao: MessageDao + + @Before + fun setUp() { + val context = ApplicationProvider.getApplicationContext() + db = Room.inMemoryDatabaseBuilder(context, LibreMailDatabase::class.java).build() + dao = db.messageDao() + } + + @After + fun tearDown() = db.close() + + @Suppress("LongParameterList") + private fun message( + id: String, + accountId: String = "acct", + folder: String = "INBOX", + uid: Long = 0L, + isRead: Boolean = false, + isStarred: Boolean = false, + bodyFetched: Boolean = false, + isHtml: Boolean = false, + body: String = "", + ) = MessageEntity( + id = id, + accountId = accountId, + sender = "Ada", + senderEmail = "ada@example.org", + subject = "Hi", + snippet = "", + body = body, + isHtml = isHtml, + timestampMillis = 1_000L, + isRead = isRead, + isStarred = isStarred, + folder = folder, + bodyFetched = bodyFetched, + uid = uid, + ) + + @Test + fun getRoutingProjectsEveryRoutingAndFlagColumn() = runBlocking { + dao.insertNew( + listOf( + message( + id = "acct:Archive:9", + folder = "Archive", + uid = 9, + isRead = true, + isStarred = true, + bodyFetched = true, + isHtml = true, + body = "x".repeat(4_000), // a large body the projection must not need to read + ), + ), + ) + + val routing = requireNotNull(dao.getRouting("acct:Archive:9")) + + assertEquals("acct:Archive:9", routing.id) + assertEquals("acct", routing.accountId) + assertEquals("Archive", routing.folder) + assertEquals(9L, routing.uid) + assertEquals(true, routing.isRead) + assertEquals(true, routing.isStarred) + assertEquals(true, routing.bodyFetched) + assertEquals(true, routing.isHtml) + } + + @Test + fun getRoutingIsNullForAnUnknownId() = runBlocking { + assertNull(dao.getRouting("acct:INBOX:404")) + } + + @Test + fun getRoutingByIdsReturnsOnlyTheRequestedRows() = runBlocking { + dao.insertNew( + listOf( + message(id = "acct:INBOX:1", folder = "INBOX", uid = 1), + message(id = "acct:INBOX:2", folder = "INBOX", uid = 2), + message(id = "acct2:Sent:3", folder = "Sent", accountId = "acct2", uid = 3), + ), + ) + + val routings = dao.getRoutingByIds(listOf("acct:INBOX:1", "acct2:Sent:3")) + + assertEquals(setOf("acct:INBOX:1", "acct2:Sent:3"), routings.map { it.id }.toSet()) + assertEquals(setOf("INBOX", "Sent"), routings.map { it.folder }.toSet()) + } +} diff --git a/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt index 121a218..ff116c8 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt @@ -254,10 +254,11 @@ class OnboardingFlowTest { waitForText("Yahoo Mail") composeTestRule.onNodeWithText("Yahoo Mail").performClick() - // Yahoo's setup screen keeps its app-password link… + // Yahoo's setup screen keeps its app-password link (now pointing at Yahoo's step-by-step + // instructions article instead of the generic account-security page, issue #155)… waitForText(string(R.string.app_password_open_page, "Yahoo Mail")) // …but gains no two-factor help link: unlike Gmail and iCloud, Yahoo gates nothing on it - // (issue #98, #153). + // (issue #98, #153, #155). composeTestRule.onNodeWithText(string(R.string.app_password_2fa_help)).assertDoesNotExist() composeTestRule.onNodeWithText(string(R.string.app_password_2fa_help_icloud)).assertDoesNotExist() } 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 efe1096..e145408 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 @@ -9,6 +9,7 @@ import androidx.room.Query import kotlinx.coroutines.flow.Flow import org.libremail.data.local.entity.FolderUnreadCount import org.libremail.data.local.entity.MessageEntity +import org.libremail.data.local.entity.MessageRouting import org.libremail.data.local.entity.MessageSummary @Dao @@ -89,6 +90,26 @@ interface MessageDao { @Query("SELECT * FROM messages WHERE id = :id LIMIT 1") suspend fun getById(id: String): MessageEntity? + /** + * Body-less routing/flags projection for a single message (issue #186). The open path and the + * flag/move callers only need routing and flag columns, so pulling the whole `body` through + * SQLite's shared CursorWindow on every such read is pure over-fetch — [getById] (`SELECT *`) is + * reserved for the one read that actually returns the body to the reader. Served by the primary-key + * lookup, so no new index / migration (mirrors the [MessageSummary] projection). + */ + @Query( + "SELECT id, accountId, folder, uid, isRead, isStarred, bodyFetched, isHtml FROM messages " + + "WHERE id = :id LIMIT 1", + ) + suspend fun getRouting(id: String): MessageRouting? + + /** Body-less routing/flags projection for a set of messages — batch move/delete/expunge callers. */ + @Query( + "SELECT id, accountId, folder, uid, isRead, isStarred, bodyFetched, isHtml FROM messages " + + "WHERE id IN (:ids)", + ) + suspend fun getRoutingByIds(ids: List): List + /** Ids of an account's synced rows in [folder] (excludes transient server-search hits). */ @Query("SELECT id FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1") suspend fun getSyncedIds(accountId: String, folder: String): List diff --git a/app/src/main/kotlin/org/libremail/data/local/entity/MessageRouting.kt b/app/src/main/kotlin/org/libremail/data/local/entity/MessageRouting.kt new file mode 100644 index 0000000..ae61f55 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/entity/MessageRouting.kt @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local.entity + +/** + * Body-less projection of [MessageEntity] for the repository's routing and flag decisions: every + * field needed to decide whether to fetch a body, push a SEEN/FLAGGED change, or resolve the + * message's folder/account — but *not* the potentially large `body` blob. + * + * The open path and the flag/move callers (`openMessage`, `downloadAttachment`, `setStarred`, + * `deleteMessage`, `expunge`, `moveByRole`, `moveToFolder`, `buildReplyDraft`, `prefetchMessage`) + * never render the body, so pulling it through SQLite's shared CursorWindow on every such read is + * pure over-fetch (issue #186). `MessageDao.getById` (`SELECT *`) is reserved for the one read that + * actually returns the body to the reader. Mirrors the existing [MessageSummary] projection — served + * by the primary-key lookup, so no new index and no schema migration. + */ +data class MessageRouting( + val id: String, + val accountId: String, + val folder: String, + val uid: Long, + val isRead: Boolean, + val isStarred: Boolean, + val bodyFetched: Boolean, + val isHtml: Boolean, +) 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 35f5813..669f59f 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -16,6 +16,7 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext import org.libremail.data.ReplyBuilder import org.libremail.data.SignatureBlock import org.libremail.data.Snippet @@ -27,7 +28,7 @@ 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.entity.FolderEntity -import org.libremail.data.local.entity.MessageEntity +import org.libremail.data.local.entity.MessageRouting import org.libremail.data.local.entity.OutboxEntity import org.libremail.data.local.toDomain import org.libremail.data.local.toEntity @@ -114,26 +115,34 @@ class MailRepositoryImpl @Inject constructor( override suspend fun getMessage(id: String): Message? = messageDao.getById(id)?.toDomain() - override suspend fun openMessage(id: String): Result = runCatching { - val entity = messageDao.getById(id) ?: error("Message not found") - val account = accountDao.getById(entity.accountId)?.toDomain() - if (account != null) { - val params = connectionFactory.imapParamsFor(account) - if (!entity.bodyFetched) { - val content = imapClient.fetchBodyMarkingSeen(params, entity.folder, uidOf(id)) - messageDao.updateBody(id, content.body, content.isHtml, Snippet.of(content.body, content.isHtml)) - attachmentDao.replaceForMessage(id, content.attachments.map { it.toEntity(id) }) - messageDao.setRead(id, true) - } else if (!entity.isRead) { - // Optimistic, local-only: the reader can render as soon as this returns. The SEEN flag - // still needs to reach the server, but that IMAP round trip (connection + STORE) must not - // sit on this path (#148) — the body/attachments are already fully local, so nothing about - // rendering the screen needs it. Pushed on backgroundScope, which outlives this call. - messageDao.setRead(id, true) - pushSeenFlagInBackground(params, entity.folder, id) + override suspend fun openMessage(id: String): Result = withContext(Dispatchers.IO) { + runCatching { + // Route on the body-less projection: a cached, already-read message needs no account, no + // credentials, and no network, so it skips the Keystore decrypt + DataStore read that + // resolving connection params costs (issue #186). Only the fetch / SEEN-push branches below + // pull the account and resolve params, and each does so lazily right where it is needed. + val routing = messageDao.getRouting(id) ?: error("Message not found") + if (!routing.bodyFetched || !routing.isRead) { + val account = accountDao.getById(routing.accountId)?.toDomain() + if (account != null && !routing.bodyFetched) { + val params = connectionFactory.imapParamsFor(account) + val content = imapClient.fetchBodyMarkingSeen(params, routing.folder, uidOf(id)) + messageDao.updateBody(id, content.body, content.isHtml, Snippet.of(content.body, content.isHtml)) + attachmentDao.replaceForMessage(id, content.attachments.map { it.toEntity(id) }) + messageDao.setRead(id, true) + } else if (account != null) { + // Optimistic, local-only: the reader can render as soon as this returns. The SEEN flag + // still needs to reach the server, but that IMAP round trip (connection + STORE) must not + // sit on this path (#148/#186) — the body/attachments are already fully local. Pushed on + // backgroundScope, which outlives this call. + val params = connectionFactory.imapParamsFor(account) + messageDao.setRead(id, true) + pushSeenFlagInBackground(params, routing.folder, id) + } } + // The single full-body read, reserved for the value the reader actually renders (issue #186). + messageDao.getById(id)?.toDomain() ?: error("Message not found") } - messageDao.getById(id)?.toDomain() ?: error("Message not found") } /** @@ -164,44 +173,77 @@ class MailRepositoryImpl @Inject constructor( rows.map { it.toDomain() } } - override suspend fun inlineImages(messageId: String): List = attachmentDao.getForMessage(messageId) - .filter { it.contentId != null } - .mapNotNull { row -> + override suspend fun inlineImages(messageId: String): List = withContext(Dispatchers.IO) { + // Resolve the message's account/folder once (body-less), then reuse the on-disk cache per cid: + // image — no per-image message re-read or attachment re-query (the old downloadAttachment N+1, #186). + val routing = messageDao.getRouting(messageId) ?: return@withContext emptyList() + val parts = attachmentDao.getForMessage(messageId) + parts.filter { it.contentId != null }.mapNotNull { row -> // Reuse the on-disk attachment cache (download once, then instant + offline). A failed // fetch just omits that image, leaving a broken rather than failing the open. - val file = downloadAttachment(messageId, row.partIndex).getOrNull() ?: return@mapNotNull null + val file = runCatching { + ensureAttachmentFile(messageId, routing.accountId, routing.folder, row.partIndex, row.filename) + }.getOrNull() ?: return@mapNotNull null InlineImage(contentId = row.contentId!!, mimeType = row.mimeType, bytes = file.readBytes()) } - - override suspend fun downloadAttachment(messageId: String, partIndex: Int): Result = runCatching { - val entity = messageDao.getById(messageId) ?: error("Message not found") - val meta = attachmentDao.getForMessage(messageId).firstOrNull { it.partIndex == partIndex } - val target = attachmentFile(messageId, partIndex, meta?.filename ?: "attachment") - // Reuse a previously downloaded (or pre-fetched) file so it opens instantly and offline. - if (target.exists() && target.length() > 0L) return@runCatching target - val account = accountDao.getById(entity.accountId)?.toDomain() ?: error("Account not found") - val params = connectionFactory.imapParamsFor(account) - val downloaded = imapClient.fetchAttachment(params, entity.folder, uidOf(messageId), partIndex) - target.parentFile?.mkdirs() - target.outputStream().use { it.write(downloaded.bytes) } - target } - override suspend fun downloadedAttachmentParts(messageId: String): Set = attachmentDao.getForMessage(messageId) - .filter { - val file = attachmentFile(messageId, it.partIndex, it.filename) - file.exists() && file.length() > 0L + override suspend fun downloadAttachment(messageId: String, partIndex: Int): Result = + withContext(Dispatchers.IO) { + runCatching { + val routing = messageDao.getRouting(messageId) ?: error("Message not found") + val meta = attachmentDao.getForMessage(messageId).firstOrNull { it.partIndex == partIndex } + ensureAttachmentFile( + messageId, + routing.accountId, + routing.folder, + partIndex, + meta?.filename ?: "attachment", + ) + } } - .map { it.partIndex } - .toSet() + + /** + * Returns the on-disk file for one attachment part, downloading and caching it on first use so it + * then opens instantly and offline. Takes the message's already-resolved account/folder so a batch + * loop (e.g. [inlineImages]) resolves them once instead of re-reading the message row per part (#186). + * Connection params are resolved lazily — only when the file is missing and must be fetched. + */ + private suspend fun ensureAttachmentFile( + messageId: String, + accountId: String, + folder: String, + partIndex: Int, + filename: String, + ): File { + val target = attachmentFile(messageId, partIndex, filename) + // Reuse a previously downloaded (or pre-fetched) file so it opens instantly and offline. + if (target.exists() && target.length() > 0L) return target + val account = accountDao.getById(accountId)?.toDomain() ?: error("Account not found") + val params = connectionFactory.imapParamsFor(account) + val downloaded = imapClient.fetchAttachment(params, folder, uidOf(messageId), partIndex) + target.parentFile?.mkdirs() + target.outputStream().use { it.write(downloaded.bytes) } + return target + } + + override suspend fun downloadedAttachmentParts(messageId: String): Set = withContext(Dispatchers.IO) { + attachmentDao.getForMessage(messageId) + .filter { + val file = attachmentFile(messageId, it.partIndex, it.filename) + file.exists() && file.length() > 0L + } + .map { it.partIndex } + .toSet() + } override suspend fun prefetchMessage(messageId: String): Result = runCatching { - val entity = messageDao.getById(messageId) ?: return@runCatching - val account = accountDao.getById(entity.accountId)?.toDomain() ?: return@runCatching - val params = connectionFactory.imapParamsFor(account) + val routing = messageDao.getRouting(messageId) ?: return@runCatching + val account = accountDao.getById(routing.accountId)?.toDomain() ?: return@runCatching // Cache the body (peek, so prefetching never marks the message read) and its attachment metadata. - if (!entity.bodyFetched) { - val content = imapClient.fetchBodyPeek(params, entity.folder, uidOf(messageId)) + if (!routing.bodyFetched) { + val params = connectionFactory.imapParamsFor(account) + val content = imapClient.fetchBodyPeek(params, routing.folder, uidOf(messageId)) messageDao.updateBody(messageId, content.body, content.isHtml, Snippet.of(content.body, content.isHtml)) attachmentDao.replaceForMessage(messageId, content.attachments.map { it.toEntity(messageId) }) } @@ -213,12 +255,12 @@ class MailRepositoryImpl @Inject constructor( override suspend fun setStarred(id: String, starred: Boolean): Result = runCatching { messageDao.setStarred(id, starred) // optimistic; next sync reconciles on failure - val entity = messageDao.getById(id) - val account = entity?.let { accountDao.getById(it.accountId)?.toDomain() } - if (entity != null && account != null) { + val routing = messageDao.getRouting(id) + val account = routing?.let { accountDao.getById(it.accountId)?.toDomain() } + if (routing != null && account != null) { imapClient.setFlag( connectionFactory.imapParamsFor(account), - entity.folder, + routing.folder, uidOf(id), Flags.Flag.FLAGGED, starred, @@ -227,11 +269,11 @@ class MailRepositoryImpl @Inject constructor( } override suspend fun deleteMessage(id: String): Result = runCatching { - val entity = messageDao.getById(id) - val account = entity?.let { accountDao.getById(it.accountId)?.toDomain() } + val routing = messageDao.getRouting(id) + val account = routing?.let { accountDao.getById(it.accountId)?.toDomain() } messageDao.deleteById(id) // optimistic; reappears on next sync if the server delete failed - if (entity != null && account != null) { - imapClient.deleteMessage(connectionFactory.imapParamsFor(account), entity.folder, uidOf(id)) + if (routing != null && account != null) { + imapClient.deleteMessage(connectionFactory.imapParamsFor(account), routing.folder, uidOf(id)) } } @@ -245,17 +287,17 @@ class MailRepositoryImpl @Inject constructor( moveByRole(ids, FolderRole.TRASH, fallbackExpunge = true) override suspend fun expunge(ids: List): Result = runCatching { - val entities = ids.mapNotNull { messageDao.getById(it) } + val routings = messageDao.getRoutingByIds(ids) messageDao.deleteByIds(ids) // optimistic - forEachAccountFolder(entities) { params, folder, group -> + forEachAccountFolder(routings) { params, folder, group -> group.forEach { imapClient.deleteMessage(params, folder, uidOf(it.id)) } } } override suspend fun moveToFolder(ids: List, destFolderFullName: String): Result = runCatching { - val entities = ids.mapNotNull { messageDao.getById(it) } + val routings = messageDao.getRoutingByIds(ids) messageDao.deleteByIds(ids) // optimistic - forEachAccountFolder(entities) { params, folder, group -> + forEachAccountFolder(routings) { params, folder, group -> if (folder != destFolderFullName) { imapClient.moveMessages(params, folder, group.map { uidOf(it.id) }, destFolderFullName) } @@ -263,17 +305,17 @@ class MailRepositoryImpl @Inject constructor( } override suspend fun buildReplyDraft(messageId: String, mode: ReplyMode): Result = runCatching { - val entity = messageDao.getById(messageId) ?: error("Message not found") - val account = accountDao.getById(entity.accountId)?.toDomain() ?: error("Account not found") + val routing = messageDao.getRouting(messageId) ?: error("Message not found") + val account = accountDao.getById(routing.accountId)?.toDomain() ?: error("Account not found") val params = connectionFactory.imapParamsFor(account) - val context = imapClient.fetchForReply(params, entity.folder, uidOf(messageId)) + val context = imapClient.fetchForReply(params, routing.folder, uidOf(messageId)) val content = ReplyBuilder.build(context, mode, account.email) // Bake the sending account's default signature into the reply/forward body — above the quoted // original — so it round-trips as part of the draft (compose won't re-append for drafts). Both // the plaintext and HTML forms are stored so the reply can go out as multipart/alternative. - val settings = accountSettingsRepository.get(entity.accountId) + val settings = accountSettingsRepository.get(routing.accountId) val sig = if (settings.signatureEnabled) { - SignatureBlock.of(signatureRepository.getDefault(entity.accountId)) + SignatureBlock.of(signatureRepository.getDefault(routing.accountId)) } else { SignatureBlock.EMPTY } @@ -281,7 +323,7 @@ class MailRepositoryImpl @Inject constructor( saveDraft( Draft( id = draftId, - accountId = entity.accountId, + accountId = routing.accountId, to = content.to, cc = content.cc, subject = content.subject, @@ -301,11 +343,11 @@ class MailRepositoryImpl @Inject constructor( */ private suspend fun moveByRole(ids: List, role: FolderRole, fallbackExpunge: Boolean): Result = runCatching { - val entities = ids.mapNotNull { messageDao.getById(it) } + val routings = messageDao.getRoutingByIds(ids) messageDao.deleteByIds(ids) // optimistic - val destByAccount = entities.map { it.accountId }.distinct() + val destByAccount = routings.map { it.accountId }.distinct() .associateWith { resolveRoleFolder(it, role) } - forEachAccountFolder(entities) { params, folder, group -> + forEachAccountFolder(routings) { params, folder, group -> when (val dest = destByAccount[group.first().accountId]) { null -> if (fallbackExpunge) { @@ -318,12 +360,12 @@ class MailRepositoryImpl @Inject constructor( } } - /** Groups [entities] by account then source folder and runs [block] once per (account, folder) group. */ + /** Groups [routings] by account then source folder and runs [block] once per (account, folder) group. */ private suspend fun forEachAccountFolder( - entities: List, - block: suspend (params: ImapConnectionParams, folder: String, group: List) -> Unit, + routings: List, + block: suspend (params: ImapConnectionParams, folder: String, group: List) -> Unit, ) { - entities.groupBy { it.accountId }.forEach { (accountId, accountMessages) -> + routings.groupBy { it.accountId }.forEach { (accountId, accountMessages) -> val account = accountDao.getById(accountId)?.toDomain() ?: return@forEach val params = connectionFactory.imapParamsFor(account) accountMessages.groupBy { it.folder }.forEach { (folder, group) -> block(params, folder, group) } diff --git a/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt b/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt index 5f93cc2..1bfa541 100644 --- a/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt +++ b/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt @@ -64,7 +64,12 @@ enum class MailProvider( YAHOO( key = "yahoo", displayName = "Yahoo Mail", - appPasswordHelpUrl = "https://login.yahoo.com/account/security", + // Yahoo's own step-by-step "Generate and manage 3rd-party app passwords" article, not + // just the generic account-security sign-in page — that page assumes the user already + // knows to look for "Create app password" once there (issue #155). Unlike Gmail/iCloud, + // this article never lists two-step verification as a prerequisite for generating an app + // password, so there is no twoFactorHelpUrl below. + appPasswordHelpUrl = "https://my.help.yahoo.com/kb/mail/generate-app-specific-password-sln15241.html", imapHost = "imap.mail.yahoo.com", smtpHost = "smtp.mail.yahoo.com", // Yahoo documents smtp.mail.yahoo.com:465 with implicit SSL/TLS as its outgoing server. diff --git a/app/src/main/kotlin/org/libremail/ui/reader/HtmlBody.kt b/app/src/main/kotlin/org/libremail/ui/reader/HtmlBody.kt index e61ab7c..ae46a2b 100644 --- a/app/src/main/kotlin/org/libremail/ui/reader/HtmlBody.kt +++ b/app/src/main/kotlin/org/libremail/ui/reader/HtmlBody.kt @@ -62,17 +62,23 @@ fun HtmlBody( dark = isDark, ) } - // A mutable holder the WebViewClient reads on the (background) interception thread, kept current - // by the update block so inline images that arrive after the first composition are resolvable. + // A mutable, thread-visible holder the WebViewClient reads on the (background) interception thread. + // The reader now resolves inline images before the first render (issue #186), so the holder is + // already populated when the page first loads; keeping it current across recompositions just means + // any later change stays resolvable without forcing a reload. val imageHolder = remember { InlineImageHolder() } imageHolder.images = inlineImages - // Tracks the content actually loaded so recompositions (star/attachment state changes) don't - // reload the page and throw away the user's scroll position. Keyed on the fully wrapped - // document so a theme (light/dark) change still re-renders with the new colors, and on the set - // of available cid: keys so the page reloads once when inline images finish resolving. - val lastLoaded = remember { mutableStateOf>?>(null) } + // Tracks the content actually loaded so recompositions (star/attachment/inline-image state changes) + // don't reload the page and throw away the user's scroll position. Keyed on the fully wrapped + // document (so a theme light/dark change re-renders with the new colors) and the remote-images + // toggle. Inline images are deliberately NOT part of the key: they are resolved before the first + // render and served from [imageHolder], so a late inline-image update must not reload the page. + val lastLoaded = remember { mutableStateOf?>(null) } AndroidView( modifier = modifier, + // TODO(#186): pool/pre-warm a WebView across reader opens instead of constructing one per open. + // Deferred as leak-prone — a pooled WebView must never retain an Activity Context or outlive its + // use. The single-render fix below already removes the dominant double-render cost. factory = { ctx -> WebView(ctx).apply { with(settings) { @@ -118,13 +124,16 @@ fun HtmlBody( } } }, + // A WebView retains its Context and is not collected promptly; destroy it when the reader leaves + // composition so neither the page nor its Context leaks (issue #186 lifecycle guard). + onRelease = { it.destroy() }, update = { webView -> // Re-apply theme-dependent state so toggling light/dark while the reader is open updates // the chrome behind the (padding of the) page as well as the content. webView.setBackgroundColor(surfaceArgb) webView.applyAlgorithmicDarkening(isDark) webView.settings.blockNetworkLoads = !loadRemoteImages - val key = Triple(document, loadRemoteImages, inlineImages.keys.toSet()) + val key = document to loadRemoteImages if (lastLoaded.value != key) { lastLoaded.value = key webView.loadDataWithBaseURL(null, document, "text/html", "UTF-8", null) diff --git a/app/src/main/kotlin/org/libremail/ui/reader/ReaderViewModel.kt b/app/src/main/kotlin/org/libremail/ui/reader/ReaderViewModel.kt index f31ff7a..5c79508 100644 --- a/app/src/main/kotlin/org/libremail/ui/reader/ReaderViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/reader/ReaderViewModel.kt @@ -67,13 +67,16 @@ class ReaderViewModel @Inject constructor( viewModelScope.launch { repository.openMessage(messageId).fold( onSuccess = { message -> - _state.update { it.copy(loading = false, message = message) } - // Resolve inline cid: images so the WebView can embed them. Runs after openMessage - // has cached the parts; skipped for plain-text mail and messages with none. - if (message.isHtml) { - val images = repository.inlineImages(messageId).associateBy { it.contentId } - if (images.isNotEmpty()) _state.update { it.copy(inlineImages = images) } + // Resolve inline cid: images BEFORE the first render and publish them in the SAME + // state update as the body, so the reader's WebView loads exactly once instead of + // rendering with an empty image map and reloading when they arrive (issue #186). + // openMessage has already cached the parts; plain-text mail has none to resolve. + val images = if (message.isHtml) { + repository.inlineImages(messageId).associateBy { it.contentId } + } else { + emptyMap() } + _state.update { it.copy(loading = false, message = message, inlineImages = images) } }, onFailure = { e -> _state.update { 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 cbb561b..874e88c 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt @@ -30,6 +30,7 @@ import org.libremail.data.local.entity.AttachmentEntity 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.MessageRouting import org.libremail.data.local.entity.MessageSummary import org.libremail.data.local.entity.ServerConfigEmbedded import org.libremail.data.settings.AccountSettingsRepository @@ -176,6 +177,7 @@ class MailRepositoryImplTest { @Test fun `openMessage fetches the body from the message's own folder, not the inbox`() = runTest { val id = "acct:Archive:5" + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "Archive") coEvery { messageDao.getById(id) } returns messageEntity(id, "Archive") coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -194,6 +196,7 @@ class MailRepositoryImplTest { @Test fun `openMessage derives a readable plain-text snippet from an HTML body`() = runTest { val id = "acct:INBOX:20" + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX") coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -224,6 +227,7 @@ class MailRepositoryImplTest { // (CompletableDeferred.complete), and JUnit4 requires @Test methods to return void/Unit. runBlocking { val id = "acct:INBOX:40" + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX", bodyFetched = true) coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX", bodyFetched = true) // isRead = false coEvery { accountDao.getById("acct") } returns accountEntity() @@ -252,6 +256,7 @@ class MailRepositoryImplTest { @Test fun `a failed SEEN-flag push is retried in the background`() = runBlocking { val id = "acct:INBOX:41" + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX", bodyFetched = true) coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX", bodyFetched = true) coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -267,12 +272,35 @@ class MailRepositoryImplTest { } } + @Test + fun `openMessage does no credential, network, or second full read for a cached read message`() = runTest { + val id = "acct:INBOX:42" + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX", bodyFetched = true, isRead = true) + coEvery { messageDao.getById(id) } returns + messageEntity(id, "INBOX", bodyFetched = true, isRead = true, body = "Cached body") + + val result = repository.openMessage(id) + + // The reader still gets the stored body back... + assertEquals("Cached body", result.getOrThrow().body) + // ...but a cached, already-read open resolves no account/credentials, touches no network, and + // never re-marks the row read (issue #186 — the Keystore decrypt + DataStore read are skipped). + coVerify(exactly = 0) { accountDao.getById(any()) } + coVerify(exactly = 0) { connectionFactory.imapParamsFor(any()) } + coVerify(exactly = 0) { imapClient.fetchBodyMarkingSeen(any(), any(), any()) } + coVerify(exactly = 0) { imapClient.setFlag(any(), any(), any(), any(), any()) } + coVerify(exactly = 0) { messageDao.setRead(any(), any()) } + // Routing used the body-less projection; the only full-body read is the single returned value. + coVerify(exactly = 1) { messageDao.getRouting(id) } + coVerify(exactly = 1) { messageDao.getById(id) } + } + @Test fun `prefetchMessage leaves a plain-text body's literal angle brackets in the snippet`() = runTest { val cache = Files.createTempDirectory("attach").toFile() every { context.cacheDir } returns cache val id = "acct:INBOX:21" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX") coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() coEvery { imapClient.fetchBodyPeek(any(), "INBOX", "21") } returns @@ -290,7 +318,7 @@ class MailRepositoryImplTest { @Test fun `archive moves messages to the account's archive folder and drops the local rows`() = runTest { val id = "acct:INBOX:5" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRoutingByIds(listOf(id)) } returns listOf(messageRouting(id, "INBOX")) coEvery { messageDao.deleteByIds(listOf(id)) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -306,7 +334,7 @@ class MailRepositoryImplTest { @Test fun `trash falls back to a permanent delete when the account has no trash folder`() = runTest { val id = "acct:INBOX:7" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRoutingByIds(listOf(id)) } returns listOf(messageRouting(id, "INBOX")) coEvery { messageDao.deleteByIds(any()) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -324,7 +352,7 @@ class MailRepositoryImplTest { @Test fun `archive fails when the account has no archive folder`() = runTest { val id = "acct:INBOX:9" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRoutingByIds(listOf(id)) } returns listOf(messageRouting(id, "INBOX")) coEvery { messageDao.deleteByIds(any()) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -338,7 +366,7 @@ class MailRepositoryImplTest { @Test fun `expunge permanently deletes each message from its own folder`() = runTest { val id = "acct:Trash:3" - coEvery { messageDao.getById(id) } returns messageEntity(id, "Trash") + coEvery { messageDao.getRoutingByIds(listOf(id)) } returns listOf(messageRouting(id, "Trash")) coEvery { messageDao.deleteByIds(listOf(id)) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -351,7 +379,7 @@ class MailRepositoryImplTest { @Test fun `moveToFolder moves messages to the chosen destination`() = runTest { val id = "acct:INBOX:11" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRoutingByIds(listOf(id)) } returns listOf(messageRouting(id, "INBOX")) coEvery { messageDao.deleteByIds(listOf(id)) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -364,7 +392,7 @@ class MailRepositoryImplTest { @Test fun `buildReplyDraft fetches the original and saves a prefilled draft`() = runTest { val id = "acct:INBOX:2" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX") coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { accountSettingsRepository.get(any()) } returns AccountSettings("acct") coEvery { signatureRepository.getDefault(any()) } returns null @@ -393,7 +421,7 @@ class MailRepositoryImplTest { @Test fun `buildReplyDraft bakes the account default signature above the quote`() = runTest { val id = "acct:INBOX:3" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX") coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { accountSettingsRepository.get(any()) } returns AccountSettings("acct") coEvery { signatureRepository.getDefault("acct") } returns org.libremail.domain.model.Signature( @@ -429,7 +457,7 @@ class MailRepositoryImplTest { val cache = Files.createTempDirectory("attach").toFile() every { context.cacheDir } returns cache val id = "acct:INBOX:4" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX") coEvery { attachmentDao.getForMessage(id) } returns listOf(attachmentEntity(id, 0, "report.pdf")) val cached = File(cache, "attachments/acct_INBOX_4/0/report.pdf").apply { parentFile?.mkdirs() @@ -447,7 +475,7 @@ class MailRepositoryImplTest { val cache = Files.createTempDirectory("attach").toFile() every { context.cacheDir } returns cache val id = "acct:INBOX:6" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX") coEvery { attachmentDao.getForMessage(id) } returns listOf(attachmentEntity(id, 0, "a.txt")) coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -482,7 +510,7 @@ class MailRepositoryImplTest { val cache = Files.createTempDirectory("attach").toFile() every { context.cacheDir } returns cache val id = "acct:INBOX:30" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX") coEvery { attachmentDao.getForMessage(id) } returns listOf( AttachmentEntity(id, 0, "logo.png", "image/png", 4, contentId = "logo1"), AttachmentEntity(id, 1, "invoice.pdf", "application/pdf", 10, contentId = null), @@ -508,7 +536,7 @@ class MailRepositoryImplTest { val cache = Files.createTempDirectory("attach").toFile() every { context.cacheDir } returns cache val id = "acct:INBOX:10" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") // bodyFetched = false + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX") // bodyFetched = false coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() coEvery { imapClient.fetchBodyPeek(any(), "INBOX", "10") } returns @@ -533,8 +561,10 @@ class MailRepositoryImplTest { fun `archive resolves a separate destination folder for each account`() = runTest { val id1 = "acct:INBOX:1" val id2 = "acct2:INBOX:1" - coEvery { messageDao.getById(id1) } returns messageEntity(id1, "INBOX", accountId = "acct") - coEvery { messageDao.getById(id2) } returns messageEntity(id2, "INBOX", accountId = "acct2") + coEvery { messageDao.getRoutingByIds(listOf(id1, id2)) } returns listOf( + messageRouting(id1, "INBOX", accountId = "acct"), + messageRouting(id2, "INBOX", accountId = "acct2"), + ) coEvery { messageDao.deleteByIds(any()) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity(id = "acct") coEvery { accountDao.getById("acct2") } returns accountEntity(id = "acct2", email = "bob@example.org") @@ -554,7 +584,7 @@ class MailRepositoryImplTest { val cache = Files.createTempDirectory("attach").toFile() every { context.cacheDir } returns cache val id = "acct:INBOX:12" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX", bodyFetched = true) + coEvery { messageDao.getRouting(id) } returns messageRouting(id, "INBOX", bodyFetched = true) coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() coEvery { attachmentDao.getForMessage(id) } returns listOf(attachmentEntity(id, 0, "f.bin")) @@ -570,7 +600,7 @@ class MailRepositoryImplTest { @Test fun `archive refreshes folders once and retries when the archive folder is initially missing`() = runTest { val id = "acct:INBOX:14" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRoutingByIds(listOf(id)) } returns listOf(messageRouting(id, "INBOX")) coEvery { messageDao.deleteByIds(any()) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -590,7 +620,7 @@ class MailRepositoryImplTest { @Test fun `reportSpam prefers the special-use spam folder when the user folder is listed first`() = runTest { val id = "acct:INBOX:16" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRoutingByIds(listOf(id)) } returns listOf(messageRouting(id, "INBOX")) coEvery { messageDao.deleteByIds(any()) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -610,7 +640,7 @@ class MailRepositoryImplTest { @Test fun `reportSpam prefers the special-use spam folder when it is listed first`() = runTest { val id = "acct:INBOX:18" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRoutingByIds(listOf(id)) } returns listOf(messageRouting(id, "INBOX")) coEvery { messageDao.deleteByIds(any()) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -629,7 +659,7 @@ class MailRepositoryImplTest { @Test fun `reportSpam keeps the first listed folder when no special-use folder holds the role`() = runTest { val id = "acct:INBOX:20" - coEvery { messageDao.getById(id) } returns messageEntity(id, "INBOX") + coEvery { messageDao.getRoutingByIds(listOf(id)) } returns listOf(messageRouting(id, "INBOX")) coEvery { messageDao.deleteByIds(any()) } just Runs coEvery { accountDao.getById("acct") } returns accountEntity() coEvery { connectionFactory.imapParamsFor(any()) } returns imapParams() @@ -658,21 +688,44 @@ class MailRepositoryImplTest { private fun attachmentEntity(messageId: String, partIndex: Int, filename: String) = AttachmentEntity(messageId, partIndex, filename, "application/octet-stream", 10L) - private fun messageEntity(id: String, folder: String, accountId: String = "acct", bodyFetched: Boolean = false) = - MessageEntity( - id = id, - accountId = accountId, - sender = "Ada", - senderEmail = "ada@example.org", - subject = "Hi", - snippet = "snippet", - body = "", - timestampMillis = 1_000L, - isRead = false, - isStarred = false, - folder = folder, - bodyFetched = bodyFetched, - ) + private fun messageEntity( + id: String, + folder: String, + accountId: String = "acct", + bodyFetched: Boolean = false, + isRead: Boolean = false, + body: String = "", + ) = MessageEntity( + id = id, + accountId = accountId, + sender = "Ada", + senderEmail = "ada@example.org", + subject = "Hi", + snippet = "snippet", + body = body, + timestampMillis = 1_000L, + isRead = isRead, + isStarred = false, + folder = folder, + bodyFetched = bodyFetched, + ) + + private fun messageRouting( + id: String, + folder: String, + accountId: String = "acct", + bodyFetched: Boolean = false, + isRead: Boolean = false, + ) = MessageRouting( + id = id, + accountId = accountId, + folder = folder, + uid = id.substringAfterLast(':').toLongOrNull() ?: 0L, + isRead = isRead, + isStarred = false, + bodyFetched = bodyFetched, + isHtml = false, + ) private fun messageSummary(id: String, folder: String, accountId: String = "acct", bodyFetched: Boolean = false) = MessageSummary( diff --git a/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt b/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt index 69d16de..49f0ea1 100644 --- a/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt +++ b/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt @@ -96,7 +96,9 @@ class MailProviderTest { // generic Apple ID sign-in page (issue #153). assertEquals("https://support.apple.com/en-us/102660", MailProvider.ICLOUD.twoFactorHelpUrl) - // Yahoo gates nothing on two-factor, so it must not grow the extra link. + // Yahoo gates nothing on two-factor — reconfirmed directly against Yahoo's own help docs, + // which never list two-step verification as a prerequisite for generating an app password + // (issue #155) — so it must not grow the extra link. assertNull(MailProvider.YAHOO.twoFactorHelpUrl) } @@ -105,6 +107,14 @@ class MailProviderTest { assertEquals("https://support.apple.com/en-us/102654", MailProvider.ICLOUD.appPasswordHelpUrl) } + @Test + fun `yahoo app-password help points at Yahoo's step-by-step instructions, not the generic sign-in page`() { + assertEquals( + "https://my.help.yahoo.com/kb/mail/generate-app-specific-password-sln15241.html", + MailProvider.YAHOO.appPasswordHelpUrl, + ) + } + @Test fun `createAccount trims the email and derives a stable id and display name`() { val account = MailProvider.GMAIL.createAccount(" User@Gmail.com ") diff --git a/app/src/test/kotlin/org/libremail/ui/reader/ReaderViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/reader/ReaderViewModelTest.kt index cf30ab0..6822e83 100644 --- a/app/src/test/kotlin/org/libremail/ui/reader/ReaderViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/reader/ReaderViewModelTest.kt @@ -19,6 +19,7 @@ import org.junit.Test import org.libremail.data.settings.AppSettings import org.libremail.data.settings.SettingsRepository import org.libremail.domain.model.Attachment +import org.libremail.domain.model.InlineImage import org.libremail.domain.model.Message import org.libremail.domain.repository.MailRepository import org.libremail.ui.navigation.Routes @@ -66,6 +67,28 @@ class ReaderViewModelTest { assertEquals(setOf(0), vm.state.value.downloaded) } + @Test + fun `inline images resolve into the same state update as the body so the reader renders once`() = + runTest(dispatcher) { + val htmlMessage = message.copy(isHtml = true, body = "") + val image = InlineImage("logo", "image/png", byteArrayOf(1, 2, 3)) + val repo = mockk(relaxed = true) + coEvery { repo.openMessage(messageId) } returns Result.success(htmlMessage) + coEvery { repo.inlineImages(messageId) } returns listOf(image) + every { repo.observeAttachments(messageId) } returns flowOf(emptyList()) + coEvery { repo.downloadedAttachmentParts(messageId) } returns emptySet() + + val vm = viewModel(repo) + advanceUntilIdle() + + val state = vm.state.value + // The body and its inline cid: image land together (loading already false), so HtmlBody + // first composes with the image in place and loads the WebView exactly once (issue #186). + assertEquals(false, state.loading) + assertEquals(htmlMessage, state.message) + assertEquals(setOf("logo"), state.inlineImages.keys) + } + @Test fun `downloading an attachment adds its part to the downloaded set`() = runTest(dispatcher) { val repo = mockk(relaxed = true)