JMR-dev/LibreMail
1
0
Fork 0
Code Issues 40 Pull Requests 2 Actions Packages Projects Releases 2 Wiki Activity

perf(reader): render message body once, move openMessage IO off-main, drop wasted work

Browse Source 
Follow-up to #148: opening an already-cached message was still slow. Three fixes
on the cached-open critical path (issue #186).

Fix 1 - WebView renders once. The reader resolved cid: inline images AFTER the
first render, so the AndroidView update key (which included inlineImages.keys)
changed and reloaded the whole document a second time for any inline-image email.
ReaderViewModel now resolves inline images and folds them into the SAME state
update as the body, and HtmlBody drops inline images from the reload key, so the
WebView loads exactly once and a late inline-image change never reloads. The
WebView is also destroyed onRelease so it (and its Context) is not leaked.
Pool/pre-warm is left as a TODO (leak-prone; single-render is the dominant win).

Fix 2 - openMessage does no wasted work for a cached, already-read message. Added
a body-less MessageRouting projection (mirrors MessageSummary, no migration);
the routing/flag callers (openMessage's first read, downloadAttachment, setStarred,
deleteMessage, expunge, moveByRole/moveToFolder, buildReplyDraft, prefetchMessage)
route on it, and getById (SELECT *) is reserved for the single read that returns
the body. imapParamsFor (Keystore decrypt + DataStore read) is resolved lazily,
only in the fetch / SEEN-push branches; the cached+read path also skips the
account lookup. De-duped the inlineImages attachment N+1 via a shared
ensureAttachmentFile helper that takes the already-resolved account/folder.

Fix 3 - repository IO off the main thread. openMessage, inlineImages,
downloadedAttachmentParts, and downloadAttachment now run in
withContext(Dispatchers.IO), so their DB/file/crypto work no longer runs on the
Main.immediate viewModelScope during the open animation.

Reader behavior (content, read/SEEN semantics) is unchanged; does not touch the
async SEEN network push handled separately by #170.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
JMR-dev
2026-07-02 20:33:07 -05:00
co-authored by Claude Opus 4.8
parent 1d63d6c7e1
commit aafb4f8f6a
8 changed files with 414 additions and 120 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRoutingTest.kt
+118
View File
@@ -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<Context>()
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())
}
}
app/src/main/kotlin/org/libremail/data/local/dao/MessageDao.kt
+21
View File
@@ -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<String>): List<MessageRouting>
/** 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<String>
app/src/main/kotlin/org/libremail/data/local/entity/MessageRouting.kt
+25
View File
@@ -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,
)
app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt
+114 -72
View File
@@ -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<Message> = 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<Message> = 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<InlineImage> = attachmentDao.getForMessage(messageId)
.filter { it.contentId != null }
.mapNotNull { row ->
override suspend fun inlineImages(messageId: String): List<InlineImage> = 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 <img> 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<File> = 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<Int> = 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<File> =
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<Int> = 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<Unit> = 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<Unit> = 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<Unit> = 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<String>): Result<Unit> = 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<String>, destFolderFullName: String): Result<Unit> = 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<String> = 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<String>, role: FolderRole, fallbackExpunge: Boolean): Result<Unit> =
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<MessageEntity>,
block: suspend (params: ImapConnectionParams, folder: String, group: List<MessageEntity>) -> Unit,
routings: List<MessageRouting>,
block: suspend (params: ImapConnectionParams, folder: String, group: List<MessageRouting>) -> 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) }
app/src/main/kotlin/org/libremail/ui/reader/HtmlBody.kt
+17 -8
View File
@@ -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<Triple<String, Boolean, Set<String>>?>(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<Pair<String, Boolean>?>(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)
app/src/main/kotlin/org/libremail/ui/reader/ReaderViewModel.kt
+9 -6
View File
@@ -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 {
app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt
+87 -34
View File
@@ -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(
app/src/test/kotlin/org/libremail/ui/reader/ReaderViewModelTest.kt
+23
View File
@@ -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 = "<img src=\"cid:logo\">")
val image = InlineImage("logo", "image/png", byteArrayOf(1, 2, 3))
val repo = mockk<MailRepository>(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<MailRepository>(relaxed = true)