From 541f4809cb62450567af51132d5ac687629938bf Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Fri, 3 Jul 2026 10:32:49 -0500 Subject: [PATCH] perf(mailbox): page the per-account folder view and search MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #124 paged only the unified "All inboxes" browse list. The per-account folder view and every search path still loaded the whole folder / entire unified inbox into memory and re-materialized it on each cache write. Extend Paging 3 to them, mirroring the unified pager. - MessageDao: add Room PagingSources for the per-account browse list (`pagingFolderSummaries`, `inInbox = 1`) and for paged search (`pagingUnifiedFolderSearchSummaries` / `pagingFolderSearchSummaries`). The search queries LIKE-match the same columns the old in-memory `matchesSearch` filter scanned (sender, sender address, subject, snippet) and leave `inInbox` unfiltered so transient server-search hits still surface. Read-only @Query methods — no schema change, no migration. - MailRepository: add `pagedFolderMessages` and the two paged-search flows via a shared `mailboxPager` whose PagingConfig adds `maxSize = MAILBOX_PAGE_SIZE * 5` so a long scroll drops far-offscreen pages. A `likePattern` helper escapes the LIKE metacharacters (\ % _) so a query containing them still matches literally. The unified pager (`pagedUnifiedFolderMessages`) is left untouched for its sibling PR. - MailboxViewModel: collapse the old `messages` list flow and the unified-only paged flow into one `pagedMessages` that dispatches each (account, folder, query) to the matching pager; `cachedIn` is kept so "select all" reads the loaded snapshot. Removes the now-dead observe*FolderMessages / matchesSearch paths. - MailboxScreen: render every mode from the single paged list. Gate the empty state on `itemCount == 0 && refresh is NotLoading && append.endOfPaginationReached` so it no longer flashes on an empty→loaded transition, preserving the search "no results", the syncing-folder spinner (#149), and browse "no messages" states. Behaviour is preserved: search filtering columns/scope, multi-select and "select all", unread counts, and the browse-vs-search state machine (server search + debounce + open/close) are unchanged — only the local list materialization moved from Kotlin into paged SQL. Co-Authored-By: Claude Opus 4.8 --- .../kotlin/org/libremail/ui/Fakes.kt | 28 +++- .../libremail/data/local/dao/MessageDao.kt | 96 +++++++++----- .../data/repository/MailRepositoryImpl.kt | 48 ++++++- .../domain/repository/MailRepository.kt | 34 +++-- .../org/libremail/ui/mailbox/MailboxScreen.kt | 92 +++++-------- .../libremail/ui/mailbox/MailboxViewModel.kt | 75 ++++------- .../data/repository/MailRepositoryImplTest.kt | 70 +++++----- .../ui/mailbox/MailboxViewModelTest.kt | 124 ++++++++++++------ 8 files changed, 319 insertions(+), 248 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt index 2454909..f59995c 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt @@ -88,15 +88,25 @@ class FakeMailRepository( val movedToFolder = mutableListOf, String>>() val replyDrafts = mutableListOf>() - override fun observeFolderMessages(accountId: String, folder: String): Flow> = - flowOf(messages.filter { it.accountId == accountId && it.folder == folder }) - - override fun observeUnifiedFolderMessages(folder: String): Flow> = - flowOf(messages.filter { it.folder == folder }) - override fun pagedUnifiedFolderMessages(folder: String): Flow> = flowOf(PagingData.from(messages.filter { it.folder == folder && it.inInbox })) + override fun pagedFolderMessages(accountId: String, folder: String): Flow> = + flowOf(PagingData.from(messages.filter { it.accountId == accountId && it.folder == folder && it.inInbox })) + + override fun pagedUnifiedSearchMessages(folder: String, query: String): Flow> = + flowOf(PagingData.from(messages.filter { it.folder == folder && it.matchesSearch(query) })) + + override fun pagedFolderSearchMessages( + accountId: String, + folder: String, + query: String, + ): Flow> = flowOf( + PagingData.from( + messages.filter { it.accountId == accountId && it.folder == folder && it.matchesSearch(query) }, + ), + ) + override fun observeFolders(accountId: String): Flow> = flowOf( folders.filter { it.accountId == @@ -198,3 +208,9 @@ class FakeMailSyncer : Syncer { override suspend fun syncAccount(accountId: String): Result = Result.success(0) override suspend fun syncFolder(accountId: String, folder: String): Result = Result.success(0) } + +/** Mirrors the paged-search DAO queries' columns so the fake's search pagers filter like production. */ +private fun Message.matchesSearch(query: String): Boolean = sender.contains(query, ignoreCase = true) || + senderEmail.contains(query, ignoreCase = true) || + subject.contains(query, ignoreCase = true) || + snippet.contains(query, ignoreCase = true) 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 e145408..4f69bd2 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 @@ -26,44 +26,14 @@ interface MessageDao { ) fun observeSummaries(): Flow> - /** - * Mailbox-list projection scoped in SQL to one account's [folder], newest-first. Unlike - * [observeSummaries] this pulls only that folder's rows and re-emits only when they actually - * change, so the mailbox list's cost scales with what's shown, not the whole cache (issue #86). - * `inInbox` is *not* filtered here so the one query serves both the normal list (caller keeps - * `inInbox = 1` rows) and search (caller keeps rows matching the query, including transient - * `inInbox = 0` server-search hits). Served by the existing `(accountId, folder, uid)` index (its - * `(accountId, folder)` prefix), so no new index — and no schema migration — is needed. - */ - @Query( - "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + - "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + - "WHERE accountId = :accountId AND folder = :folder ORDER BY timestampMillis DESC", - ) - fun observeFolderSummaries(accountId: String, folder: String): Flow> - - /** - * Unified-inbox projection: [folder] across every account, newest-first. Scoped in SQL like - * [observeFolderSummaries] but without an account predicate (issue #86). No existing index leads - * with `folder`, so this still scans in timestamp order — far cheaper than [observeSummaries] (it - * materializes only this folder's rows, not the whole cache) but not O(1); a large multi-account - * unified inbox is a candidate for a `(folder, inInbox, timestampMillis)` index + paging. - */ - @Query( - "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + - "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + - "WHERE folder = :folder ORDER BY timestampMillis DESC", - ) - fun observeUnifiedFolderSummaries(folder: String): Flow> - /** * Paged unified-inbox projection: folder-synced rows of [folder] across every account, - * newest-first, as a Paging 3 [PagingSource] (issue #124). Unlike [observeUnifiedFolderSummaries] - * — which materializes the *entire* unified inbox (~thousands of rows) on every emission — Room - * loads only the requested window (LIMIT/OFFSET), so the mailbox list's query, mapping, and - * recomposition cost scale with what's on screen, not the whole cache. Filters `inInbox = 1` - * because the paged browse list shows only synced rows; unified *search* (which must also surface - * transient `inInbox = 0` hits) stays on [observeUnifiedFolderSummaries]. Profiling (see + * newest-first, as a Paging 3 [PagingSource] (issue #124). Room loads only the requested window + * (LIMIT/OFFSET), so the mailbox list's query, mapping, and recomposition cost scale with what's on + * screen, not the whole cache — instead of materializing the *entire* unified inbox (~thousands of + * rows) on every emission. Filters `inInbox = 1` because the paged browse list shows only synced + * rows; unified *search* (which must also surface transient `inInbox = 0` hits) is paged separately + * by [pagingUnifiedFolderSearchSummaries] (issue #214). Profiling (see * `docs/perf/issue-124-unified-inbox-paging.md`) showed the first page loads flat regardless of * total cache size on the existing indices, so no `(folder, …)` index / schema migration is added. */ @@ -74,6 +44,60 @@ interface MessageDao { ) fun pagingUnifiedFolderSummaries(folder: String): PagingSource + /** + * Paged per-account folder projection: [accountId]'s folder-synced rows of [folder], newest-first, + * as a Paging 3 [PagingSource] (issue #214). The account-scoped counterpart of + * [pagingUnifiedFolderSummaries] — brings #124's window-at-a-time loading to the per-account browse + * list so opening a large account folder no longer materializes the whole folder into memory. + * Filters `inInbox = 1` (synced browse rows only; per-account *search* uses + * [pagingFolderSearchSummaries], which also surfaces transient `inInbox = 0` hits). Served by the + * `(accountId, folder, uid)` index's `(accountId, folder)` prefix, so no new index / migration. + */ + @Query( + "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + + "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + + "WHERE accountId = :accountId AND folder = :folder AND inInbox = 1 ORDER BY timestampMillis DESC", + ) + fun pagingFolderSummaries(accountId: String, folder: String): PagingSource + + /** + * Paged unified search: rows of [folder] across every account whose sender, sender address, + * subject, or snippet match [pattern] — a pre-built SQL `LIKE` pattern (`%term%`, with the LIKE + * metacharacters `\ % _` escaped by `\`) — newest-first, as a Paging 3 [PagingSource] (issue #214). + * Scans the same columns the old in-memory search filter (`matchesSearch`) did, but in SQL so a + * search over a large folder loads only the visible window. Unlike the browse pagers this does + * *not* filter `inInbox`, so it surfaces both synced rows and the transient `inInbox = 0` + * server-search hits `MailRepository.searchServer` inserts — exactly what the old filter saw. LIKE + * is case-insensitive for ASCII (SQLite's default), matching `matchesSearch`'s common case. + */ + @Query( + "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + + "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + + "WHERE folder = :folder AND (sender LIKE :pattern ESCAPE '\\' OR " + + "senderEmail LIKE :pattern ESCAPE '\\' OR subject LIKE :pattern ESCAPE '\\' OR " + + "snippet LIKE :pattern ESCAPE '\\') ORDER BY timestampMillis DESC", + ) + fun pagingUnifiedFolderSearchSummaries(folder: String, pattern: String): PagingSource + + /** + * Paged per-account search: [accountId]'s rows of [folder] matching [pattern] (see + * [pagingUnifiedFolderSearchSummaries] for the pattern/column contract), newest-first, as a + * Paging 3 [PagingSource] (issue #214). Like the unified variant it leaves `inInbox` unfiltered so + * per-account search still surfaces transient server-search hits. + */ + @Query( + "SELECT id, accountId, sender, senderEmail, subject, snippet, timestampMillis, " + + "isRead, isStarred, folder, inInbox, bodyFetched FROM messages " + + "WHERE accountId = :accountId AND folder = :folder AND (sender LIKE :pattern ESCAPE '\\' OR " + + "senderEmail LIKE :pattern ESCAPE '\\' OR subject LIKE :pattern ESCAPE '\\' OR " + + "snippet LIKE :pattern ESCAPE '\\') ORDER BY timestampMillis DESC", + ) + fun pagingFolderSearchSummaries( + accountId: String, + folder: String, + pattern: String, + ): PagingSource + /** * Live per-(account, folder) unread counts for the drawer's folder badges and the bold styling of * accounts with unread mail. Counts only folder-synced rows (`inInbox = 1`), so transient diff --git a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt index 9c30e20..ffe9fae 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -6,6 +6,7 @@ import android.net.Uri import androidx.paging.Pager import androidx.paging.PagingConfig import androidx.paging.PagingData +import androidx.paging.PagingSource import androidx.paging.map import dagger.hilt.android.qualifiers.ApplicationContext import jakarta.mail.Flags @@ -30,6 +31,7 @@ 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.MessageRouting +import org.libremail.data.local.entity.MessageSummary import org.libremail.data.local.entity.OutboxEntity import org.libremail.data.local.toDomain import org.libremail.data.local.toEntity @@ -82,12 +84,6 @@ class MailRepositoryImpl @Inject constructor( // Hilt's SingletonComponent), so the scope's lifetime is the process's, not any one caller's coroutine. private val backgroundScope = CoroutineScope(SupervisorJob() + Dispatchers.IO) - override fun observeFolderMessages(accountId: String, folder: String): Flow> = - messageDao.observeFolderSummaries(accountId, folder).map { rows -> rows.map { it.toDomain() } } - - override fun observeUnifiedFolderMessages(folder: String): Flow> = - messageDao.observeUnifiedFolderSummaries(folder).map { rows -> rows.map { it.toDomain() } } - override fun pagedUnifiedFolderMessages(folder: String): Flow> = Pager( config = PagingConfig( // A page comfortably exceeds a screenful so scrolling rarely waits on a load; loading @@ -101,6 +97,36 @@ class MailRepositoryImpl @Inject constructor( pagingSourceFactory = { messageDao.pagingUnifiedFolderSummaries(folder) }, ).flow.map { page -> page.map { it.toDomain() } } + override fun pagedFolderMessages(accountId: String, folder: String): Flow> = + mailboxPager { messageDao.pagingFolderSummaries(accountId, folder) } + + override fun pagedUnifiedSearchMessages(folder: String, query: String): Flow> = + mailboxPager { messageDao.pagingUnifiedFolderSearchSummaries(folder, likePattern(query)) } + + override fun pagedFolderSearchMessages( + accountId: String, + folder: String, + query: String, + ): Flow> = + mailboxPager { messageDao.pagingFolderSearchSummaries(accountId, folder, likePattern(query)) } + + /** + * Shared [Pager] for the per-account and search mailbox lists (issue #214). Same window sizing as + * the unified browse pager, plus a bounded `maxSize` so scrolling a long list drops far-offscreen + * pages instead of retaining the whole scrolled-through range in memory. Placeholders stay off (the + * row height varies, and the list never sizes a scrollbar to the full uncounted result). + */ + private fun mailboxPager(pagingSourceFactory: () -> PagingSource): Flow> = + Pager( + config = PagingConfig( + pageSize = MAILBOX_PAGE_SIZE, + initialLoadSize = MAILBOX_PAGE_SIZE * 3, + enablePlaceholders = false, + maxSize = MAILBOX_PAGE_SIZE * 5, + ), + pagingSourceFactory = pagingSourceFactory, + ).flow.map { page -> page.map { it.toDomain() } } + override fun observeFolders(accountId: String): Flow> = folderDao.observeForAccount(accountId).map { rows -> rows.map { it.toDomain() } @@ -515,3 +541,13 @@ private const val SEEN_FLAG_RETRY_BACKOFF_MS = 2_000L /** Message id is ":"; the uid is the trailing segment. */ private fun uidOf(id: String): String = id.substringAfterLast(':') + +/** + * Builds the SQL `LIKE` pattern the paged-search DAO queries take (issue #214), preserving the old + * `matchesSearch` literal-substring semantics: escape the LIKE metacharacters (`\ % _`) — the `\` + * first, so the escapes just added aren't themselves re-escaped — then wrap the term in wildcards. + */ +private fun likePattern(query: String): String { + val escaped = query.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_") + return "%$escaped%" +} diff --git a/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt b/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt index e9c92a5..d1e6ece 100644 --- a/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt +++ b/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt @@ -19,24 +19,34 @@ import java.io.File * The UI always reads from here; the cache is the single source of truth. */ interface MailRepository { - /** - * The cached messages of one account's [folder], newest-first — the scoped source for the mailbox - * list (issue #86). Includes transient server-search rows so search can surface them; the caller - * filters `inInbox`/query. Re-emits only when that folder's rows change, not on every cache write. - */ - fun observeFolderMessages(accountId: String, folder: String): Flow> - - /** Like [observeFolderMessages] but for [folder] across every account (the unified inbox). */ - fun observeUnifiedFolderMessages(folder: String): Flow> - /** * The unified inbox as a [PagingData] stream so the list's query, mapping, and recomposition cost * scale with the visible window rather than the whole cache (issue #124). Emits only folder-synced - * rows of [folder] across every account, newest-first — unified *search* still uses - * [observeUnifiedFolderMessages]. Callers must `cachedIn` a scope before collecting. + * rows of [folder] across every account, newest-first — unified *search* is paged separately by + * [pagedUnifiedSearchMessages] (issue #214). Callers must `cachedIn` a scope before collecting. */ fun pagedUnifiedFolderMessages(folder: String): Flow> + /** + * The per-account browse list as a [PagingData] stream — the account-scoped counterpart of + * [pagedUnifiedFolderMessages] (issue #214). Folder-synced rows of [accountId]'s [folder], + * newest-first, loaded a window at a time. Callers must `cachedIn` a scope before collecting. + */ + fun pagedFolderMessages(accountId: String, folder: String): Flow> + + /** + * Unified *search* as a [PagingData] stream (issue #214): rows of [folder] across every account + * matching [query], newest-first, including the transient server-search hits [searchServer] adds. + * Scans the same fields the previous in-memory search filter did. `cachedIn` before collecting. + */ + fun pagedUnifiedSearchMessages(folder: String, query: String): Flow> + + /** + * Per-account *search* counterpart of [pagedUnifiedSearchMessages] (issue #214): [accountId]'s + * [folder] rows matching [query], newest-first. Callers must `cachedIn` before collecting. + */ + fun pagedFolderSearchMessages(accountId: String, folder: String, query: String): Flow> + /** The account's cached IMAP folders for the navigation drawer. */ fun observeFolders(accountId: String): Flow> diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt index a96c1ff..f63a827 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt @@ -21,7 +21,6 @@ import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.width import androidx.compose.foundation.lazy.LazyColumn -import androidx.compose.foundation.lazy.items import androidx.compose.foundation.rememberScrollState import androidx.compose.foundation.shape.CircleShape import androidx.compose.foundation.text.BasicTextField @@ -104,8 +103,8 @@ fun MailboxScreen( onSelectTab: (org.libremail.ui.TopDest) -> Unit, viewModel: MailboxViewModel = hiltViewModel(), ) { - val messages by viewModel.messages.collectAsStateWithLifecycle() - // The unified "All inboxes" browse list is paged (issue #124); per-account/search render [messages]. + // Every mailbox mode — unified/per-account browse and search — is a single paged list now + // (issues #124, #214); the ViewModel dispatches the right pager by (account, folder, query). val pagedMessages = viewModel.pagedMessages.collectAsLazyPagingItems() val accounts by viewModel.accounts.collectAsStateWithLifecycle() val selectedAccountId by viewModel.selectedAccountId.collectAsStateWithLifecycle() @@ -129,10 +128,6 @@ fun MailboxScreen( val moveTargetFolders by viewModel.moveTargetFolders.collectAsStateWithLifecycle() val actionInProgress by viewModel.actionInProgress.collectAsStateWithLifecycle() val selectionMode = selectedIds.isNotEmpty() - // The unified inbox (no account filter, not searching) renders the paged list; a concrete account - // or an active search renders the flat [messages] list. Hoisted here so the selection bar's - // "Select all" can read the right source (issue #124). - val showPaged = selectedAccountId == null && searchQuery.isBlank() var showMovePicker by remember { mutableStateOf(false) } val snackbarHostState = remember { SnackbarHostState() } val drawerState = rememberDrawerState(DrawerValue.Closed) @@ -198,11 +193,9 @@ fun MailboxScreen( onSpam = viewModel::requestSpam, onMove = { showMovePicker = true }, onSelectAll = { - // "Select all" acts on what's shown: the loaded paged window for the unified - // inbox, or the whole flat list for a per-account/search view. - viewModel.selectAll( - if (showPaged) pagedMessages.itemSnapshotList.items else messages, - ) + // "Select all" acts on what's shown: the currently loaded window of the + // paged list (issues #124, #214). + viewModel.selectAll(pagedMessages.itemSnapshotList.items) }, onReply = { viewModel.reply(ReplyMode.REPLY) }, onReplyAll = viewModel::requestReplyAll, @@ -294,59 +287,36 @@ fun MailboxScreen( modifier = Modifier.fillMaxSize(), ) { LazyColumn(modifier = Modifier.fillMaxSize()) { - if (showPaged) { - if (pagedMessages.itemCount == 0) { - item { - // Hold the empty state back until the first page settles so - // it doesn't flash before rows arrive (search never lands here). - if (pagedMessages.loadState.refresh !is LoadState.Loading) { - NoMessagesState(Modifier.fillParentMaxSize()) - } - } - } else { - items( - count = pagedMessages.itemCount, - key = pagedMessages.itemKey { it.id }, - ) { index -> - val message = pagedMessages[index] ?: return@items - val accountLabel = - if (showAccount) accountsById[message.accountId]?.email else null - MessageRow( - message = message, - accountLabel = accountLabel, - selected = message.id in selectedIds, - onClick = { - if (selectionMode) { - viewModel.toggleSelection(message.id, message.accountId) - } else { - onOpenMessage(message.id) - } - }, - onLongClick = { - viewModel.startSelection(message.id, message.accountId) - }, - ) - HorizontalDivider() - } - } - } else if (messages.isEmpty()) { + if (pagedMessages.itemCount == 0) { item { - // Hold the empty state back while the folder's initial background - // sync is still in flight, so opening an uncached folder doesn't - // flash "No messages to display" before the fetch has a chance to - // populate anything (issue #149). - if (searchActive && searchQuery.isNotBlank()) { - NoResultsState(Modifier.fillParentMaxSize()) - } else if (isSyncingFolder) { - Box(Modifier.fillParentMaxSize(), contentAlignment = Alignment.Center) { - CircularProgressIndicator() - } - } else { - NoMessagesState(Modifier.fillParentMaxSize()) + val loadState = pagedMessages.loadState + // "Settled" = the refresh finished and there is no further page, + // so an empty list is genuinely empty. Gating on it (not just + // "refresh not Loading") holds the empty state back on an + // empty→loaded transition — before the first page, or before a + // search's debounced server hits land (issues #124, #214). + val settled = loadState.refresh is LoadState.NotLoading && + loadState.append.endOfPaginationReached + when { + searchActive && searchQuery.isNotBlank() -> + if (settled) NoResultsState(Modifier.fillParentMaxSize()) + // An uncached per-account folder still doing its initial + // background sync: show the spinner rather than "No messages" + // until it settles or the fetch populates rows (issue #149). + isSyncingFolder -> + Box( + Modifier.fillParentMaxSize(), + contentAlignment = Alignment.Center, + ) { CircularProgressIndicator() } + settled -> NoMessagesState(Modifier.fillParentMaxSize()) } } } else { - items(messages, key = { it.id }) { message -> + items( + count = pagedMessages.itemCount, + key = pagedMessages.itemKey { it.id }, + ) { index -> + val message = pagedMessages[index] ?: return@items val accountLabel = if (showAccount) accountsById[message.accountId]?.email else null MessageRow( diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt index 6e76da2..5b0f07a 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt @@ -120,57 +120,32 @@ class MailboxViewModel @Inject constructor( val searchQuery: StateFlow = _searchQuery.asStateFlow() /** - * The list-rendered messages: a per-account folder (SQL-scoped and already flat, issue #86) or — - * for the unified inbox — *only* an active search's matches across accounts. The unified **browse** - * list is paged instead (see [pagedMessages], issue #124), so this flow stays empty while browsing - * the unified inbox and the whole cache is never pulled into memory on each write. - */ - val messages: StateFlow> = - combine(_selectedAccountId, _selectedFolder) { accountId, folder -> accountId to folder } - .distinctUntilChanged() - .flatMapLatest { (accountId, folder) -> - if (accountId != null) { - // Per-account+folder: the SQL-scoped, already-flat list. The one client-side pass - // distinguishes the normal list (synced rows) from an active search (any matching - // row, including transient server-search hits) over the small folder-scoped set. - combine(mailRepository.observeFolderMessages(accountId, folder), _searchQuery) { rows, query -> - val q = query.trim() - rows.filter { if (q.isEmpty()) it.inInbox else it.matchesSearch(q) } - } - } else { - // Unified inbox: browse is paged via [pagedMessages]; this backs only an active - // search over the folder's rows across accounts (bounded by the per-account search - // limit), staying empty while browsing so the whole inbox is never materialized. - _searchQuery.flatMapLatest { query -> - val q = query.trim() - if (q.isEmpty()) { - flowOf(emptyList()) - } else { - mailRepository.observeUnifiedFolderMessages(folder) - .map { rows -> rows.filter { it.matchesSearch(q) } } - } - } - } - }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) - - /** - * The unified "All inboxes" browse list as a paged stream (issue #124): folder-synced rows across - * every account, newest-first, loaded a window at a time so query/mapping/recomposition cost scale - * with the screen, not the total cache. Emits empty paging data whenever a concrete account is - * selected or a search is active (those render from [messages]). + * The mailbox list as a single paged stream (issues #124, #214), loaded a window at a time so the + * query, mapping, and recomposition cost scale with the screen, not the whole cache. One flow backs + * every mode — unified browse, per-account browse, and search (unified or per-account) — dispatching + * to the matching repository pager by the current (account, folder, query). Browse shows only + * folder-synced rows; search re-pages on each keystroke and (via its DAO query) also surfaces the + * transient server-search hits. `cachedIn` so the loaded window survives configuration changes and + * the selection's "select all" can read it. */ val pagedMessages: Flow> = - combine( - _selectedAccountId, - _selectedFolder, - _searchQuery.map { it.isBlank() }.distinctUntilChanged(), - ) { accountId, folder, browsing -> Triple(accountId, folder, browsing) } + combine(_selectedAccountId, _selectedFolder, _searchQuery) { accountId, folder, query -> + Triple(accountId, folder, query.trim()) + } .distinctUntilChanged() - .flatMapLatest { (accountId, folder, browsing) -> - if (accountId == null && browsing) { - mailRepository.pagedUnifiedFolderMessages(folder) + .flatMapLatest { (accountId, folder, query) -> + if (query.isEmpty()) { + if (accountId == null) { + mailRepository.pagedUnifiedFolderMessages(folder) + } else { + mailRepository.pagedFolderMessages(accountId, folder) + } } else { - flowOf(PagingData.empty()) + if (accountId == null) { + mailRepository.pagedUnifiedSearchMessages(folder, query) + } else { + mailRepository.pagedFolderSearchMessages(accountId, folder, query) + } } } .cachedIn(viewModelScope) @@ -474,9 +449,3 @@ private fun withInbox(accountId: String, folders: List): List = } else { listOf(Folder(accountId, INBOX, INBOX, FolderRole.INBOX, selectable = true)) + folders } - -/** Local match over the always-populated header fields (and snippet, once a body is cached). */ -private fun Message.matchesSearch(query: String): Boolean = sender.contains(query, ignoreCase = true) || - senderEmail.contains(query, ignoreCase = true) || - subject.contains(query, ignoreCase = true) || - snippet.contains(query, ignoreCase = true) 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 624cea9..4e63d1c 100644 --- a/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt +++ b/app/src/test/kotlin/org/libremail/data/repository/MailRepositoryImplTest.kt @@ -83,38 +83,6 @@ class MailRepositoryImplTest { attachmentUriGrants = mockk(relaxed = true), ) - @Test - fun `observeFolderMessages is empty when the folder has no cached rows`() = runTest { - every { messageDao.observeFolderSummaries("acct", "INBOX") } returns flowOf(emptyList()) - repository.observeFolderMessages("acct", "INBOX").test { - assertTrue(awaitItem().isEmpty()) - awaitComplete() - } - } - - @Test - fun `observeFolderMessages maps the folder's cached entities`() = runTest { - every { messageDao.observeFolderSummaries("acct", "INBOX") } returns - flowOf(listOf(messageSummary("1", "INBOX"))) - repository.observeFolderMessages("acct", "INBOX").test { - val items = awaitItem() - assertEquals(1, items.size) - assertEquals("Ada", items.first().sender) - assertEquals("INBOX", items.first().folder) - awaitComplete() - } - } - - @Test - fun `observeUnifiedFolderMessages maps the folder's rows across accounts`() = runTest { - every { messageDao.observeUnifiedFolderSummaries("INBOX") } returns - flowOf(listOf(messageSummary("1", "INBOX"))) - repository.observeUnifiedFolderMessages("INBOX").test { - assertEquals(1, awaitItem().size) - awaitComplete() - } - } - @Test fun `pagedUnifiedFolderMessages maps the paged summaries to domain messages`() = runTest { every { messageDao.pagingUnifiedFolderSummaries("INBOX") } returns FakeSummaryPagingSource( @@ -129,6 +97,44 @@ class MailRepositoryImplTest { assertEquals("", items.first().body) } + @Test + fun `pagedFolderMessages maps the account folder's paged summaries to domain messages`() = runTest { + every { messageDao.pagingFolderSummaries("acct", "INBOX") } returns FakeSummaryPagingSource( + listOf(messageSummary("1", "INBOX"), messageSummary("2", "INBOX")), + ) + + val items = repository.pagedFolderMessages("acct", "INBOX").asSnapshot() + + assertEquals(listOf("1", "2"), items.map { it.id }) + // The list projection never carries a body — the reader loads it on demand (see MessageSummary). + assertEquals("", items.first().body) + } + + @Test + fun `pagedUnifiedSearchMessages escapes LIKE metacharacters and maps the paged hits`() = runTest { + val pattern = slot() + every { messageDao.pagingUnifiedFolderSearchSummaries("INBOX", capture(pattern)) } returns + FakeSummaryPagingSource(listOf(messageSummary("1", "INBOX"))) + + val items = repository.pagedUnifiedSearchMessages("INBOX", "50%_off").asSnapshot() + + assertEquals(listOf("1"), items.map { it.id }) + // The query's LIKE metacharacters are escaped (so they match literally) then wrapped in wildcards. + assertEquals("%50\\%\\_off%", pattern.captured) + } + + @Test + fun `pagedFolderSearchMessages scopes the paged search to the account and folder`() = runTest { + val pattern = slot() + every { messageDao.pagingFolderSearchSummaries("acct", "INBOX", capture(pattern)) } returns + FakeSummaryPagingSource(listOf(messageSummary("1", "INBOX"))) + + val items = repository.pagedFolderSearchMessages("acct", "INBOX", "hi").asSnapshot() + + assertEquals(listOf("1"), items.map { it.id }) + assertEquals("%hi%", pattern.captured) + } + @Test fun `observeFolders maps cached folders with their roles and server special-use flag`() = runTest { every { folderDao.observeForAccount("acct") } returns flowOf( diff --git a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt index 5cc6834..ef33e77 100644 --- a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt @@ -8,6 +8,7 @@ import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every import io.mockk.mockk +import io.mockk.verify import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi @@ -18,6 +19,7 @@ import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.launch import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import kotlinx.coroutines.test.setMain import org.junit.After @@ -54,43 +56,59 @@ class MailboxViewModelTest { private val alice = account("imap:a", "alice@example.org") private val bob = account("imap:b", "bob@example.org") + // Every mailbox mode is a single paged flow (issues #124, #214); the ViewModel's only job is to + // dispatch each (account, folder, query) to the matching repository pager. The paged rows/scoping + // themselves are covered by MailRepositoryImplTest and the MailboxScreen UI test. @Test - fun `unified browse keeps the whole inbox out of the in-memory list flow`() = runTest(testDispatcher) { - val vm = createViewModel( - accounts = listOf(alice, bob), - messages = listOf( - msg("imap:a:INBOX:1", "imap:a", "INBOX"), - msg("imap:a:Archive:1", "imap:a", "Archive"), - msg("imap:b:INBOX:1", "imap:b", "INBOX"), - ), - ) - backgroundScope.launch { vm.messages.collect {} } + fun `pagedMessages dispatches to the matching repository pager for each mode`() = runTest(testDispatcher) { + val repo = mockk(relaxed = true) + val vm = createViewModel(accounts = listOf(alice), messages = emptyList(), repo = repo) + backgroundScope.launch { vm.pagedMessages.collect {} } + runCurrent() + // Default: unified "All inboxes" browse. assertEquals("INBOX", vm.selectedFolder.value) assertNull(vm.selectedAccountId.value) - // Unified browse is paged (issue #124): the flat list flow stays empty so the whole unified - // inbox is never materialized on every write. The paged rows themselves are covered by - // MailRepositoryImplTest's pagedUnifiedFolderMessages test and the MailboxScreen UI test. - assertTrue(vm.messages.value.isEmpty()) + verify { repo.pagedUnifiedFolderMessages("INBOX") } + + // A query flips the unified view to the unified search pager. + vm.onSearchQuery("hello") + runCurrent() + verify { repo.pagedUnifiedSearchMessages("INBOX", "hello") } + + // Clearing the query and picking an account browses that account's folder via the per-account pager. + vm.onSearchQuery("") + vm.selectAccount("imap:a") + runCurrent() + verify { repo.pagedFolderMessages("imap:a", "INBOX") } + + // Searching within a concrete account uses the per-account search pager. + vm.onSearchQuery("world") + runCurrent() + verify { repo.pagedFolderSearchMessages("imap:a", "INBOX", "world") } } @Test - fun `selecting a concrete account renders the flat scoped list`() = runTest(testDispatcher) { + fun `selecting a concrete account pages that account's folder`() = runTest(testDispatcher) { + val repo = mockk(relaxed = true) val vm = createViewModel( accounts = listOf(alice), messages = listOf(msg("imap:a:INBOX:1", "imap:a", "INBOX"), msg("imap:a:Archive:1", "imap:a", "Archive")), + repo = repo, ) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } vm.selectAccount("imap:a") + runCurrent() - // A concrete account uses the SQL-scoped, already-flat list (issue #86), not the paged path. - assertEquals(listOf("imap:a:INBOX:1"), vm.messages.value.map { it.id }) + // A concrete account uses the SQL-scoped per-account pager (issue #214), not the unified one. + verify { repo.pagedFolderMessages("imap:a", "INBOX") } } @Test fun `selecting a folder scopes the view to that account and folder and syncs it`() = runTest(testDispatcher) { val syncer = mockk(relaxed = true) + val repo = mockk(relaxed = true) val vm = createViewModel( accounts = listOf(alice), messages = listOf( @@ -98,14 +116,16 @@ class MailboxViewModelTest { msg("imap:a:Archive:1", "imap:a", "Archive"), ), syncer = syncer, + repo = repo, ) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } vm.selectFolder("imap:a", "Archive") + runCurrent() assertEquals("Archive", vm.selectedFolder.value) assertEquals("imap:a", vm.selectedAccountId.value) - assertEquals(listOf("imap:a:Archive:1"), vm.messages.value.map { it.id }) + verify { repo.pagedFolderMessages("imap:a", "Archive") } coVerify { syncer.syncFolder("imap:a", "Archive") } } @@ -120,7 +140,7 @@ class MailboxViewModelTest { Result.success(0) } val vm = createViewModel(accounts = listOf(alice), messages = emptyList(), syncer = syncer) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } vm.isSyncingFolder.test { assertEquals(false, awaitItem()) @@ -145,7 +165,7 @@ class MailboxViewModelTest { Result.failure(IllegalStateException("boom")) } val vm = createViewModel(accounts = listOf(alice), messages = emptyList(), syncer = syncer) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } vm.isSyncingFolder.test { assertEquals(false, awaitItem()) @@ -174,7 +194,7 @@ class MailboxViewModelTest { Result.success(0) } val vm = createViewModel(accounts = listOf(alice), messages = emptyList(), syncer = syncer) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } vm.isSyncingFolder.test { assertEquals(false, awaitItem()) @@ -233,7 +253,7 @@ class MailboxViewModelTest { accounts = listOf(alice, bob), messages = listOf(msg("imap:a:Archive:1", "imap:a", "Archive")), ) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } vm.selectFolder("imap:a", "Archive") assertEquals("Archive", vm.selectedFolder.value) @@ -338,7 +358,7 @@ class MailboxViewModelTest { messages = listOf(msg("imap:a:INBOX:1", "imap:a", "INBOX")), repo = repo, ) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } vm.startSelection("imap:a:INBOX:1", "imap:a") vm.requestDelete() @@ -367,7 +387,7 @@ class MailboxViewModelTest { ), repo = repo, ) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } backgroundScope.launch { vm.currentFolderRole.collect {} } vm.selectFolder("imap:a", "Spam") @@ -474,7 +494,7 @@ class MailboxViewModelTest { accounts = listOf(alice, bob), messages = listOf(msg("imap:a:INBOX:1", "imap:a", "INBOX"), msg("imap:b:INBOX:1", "imap:b", "INBOX")), ) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } backgroundScope.launch { vm.canMove.collect {} } vm.startSelection("imap:a:INBOX:1", "imap:a") @@ -523,16 +543,20 @@ class MailboxViewModelTest { @Test fun `opens filtered to the account passed as a nav argument`() = runTest(testDispatcher) { + val repo = mockk(relaxed = true) val vm = createViewModel( accounts = listOf(alice, bob), messages = listOf(msg("imap:a:INBOX:1", "imap:a", "INBOX"), msg("imap:b:INBOX:1", "imap:b", "INBOX")), initialAccountId = "imap:a", + repo = repo, ) - backgroundScope.launch { vm.messages.collect {} } + backgroundScope.launch { vm.pagedMessages.collect {} } + runCurrent() assertEquals("imap:a", vm.selectedAccountId.value) assertEquals("INBOX", vm.selectedFolder.value) - assertEquals(listOf("imap:a:INBOX:1"), vm.messages.value.map { it.id }) + // Seeded to a concrete account, so it pages that account's inbox (not the unified pager). + verify { repo.pagedFolderMessages("imap:a", "INBOX") } } @Test @@ -588,23 +612,33 @@ class MailboxViewModelTest { repo: MailRepository = mockk(relaxed = true), initialAccountId: String? = null, ): MailboxViewModel { - // Mimic the SQL scoping: return the seed list narrowed to the queried account+folder / folder, - // exactly as MessageDao.observeFolderSummaries / observeUnifiedFolderSummaries do (issue #86). - every { repo.observeFolderMessages(any(), any()) } answers { - val accountId = firstArg() - val folder = secondArg() - MutableStateFlow(messages.filter { it.accountId == accountId && it.folder == folder }) - } - every { repo.observeUnifiedFolderMessages(any()) } answers { - val folder = firstArg() - MutableStateFlow(messages.filter { it.folder == folder }) - } - // The unified browse list is paged (issue #124); mirror the DAO's inInbox-scoped, folder-scoped - // projection as a single static page. + // Every mailbox mode is paged (issues #124, #214). Mirror each DAO projection as a single static + // page over the seed list: browse pagers keep folder-synced (inInbox) rows; search pagers match + // the same fields the DAO's LIKE query does, leaving inInbox unfiltered. every { repo.pagedUnifiedFolderMessages(any()) } answers { val folder = firstArg() flowOf(PagingData.from(messages.filter { it.folder == folder && it.inInbox })) } + every { repo.pagedFolderMessages(any(), any()) } answers { + val accountId = firstArg() + val folder = secondArg() + flowOf(PagingData.from(messages.filter { it.accountId == accountId && it.folder == folder && it.inInbox })) + } + every { repo.pagedUnifiedSearchMessages(any(), any()) } answers { + val folder = firstArg() + val query = secondArg() + flowOf(PagingData.from(messages.filter { it.folder == folder && it.matchesSearch(query) })) + } + every { repo.pagedFolderSearchMessages(any(), any(), any()) } answers { + val accountId = firstArg() + val folder = secondArg() + val query = thirdArg() + flowOf( + PagingData.from( + messages.filter { it.accountId == accountId && it.folder == folder && it.matchesSearch(query) }, + ), + ) + } every { repo.observeDrafts() } returns flowOf(emptyList()) every { repo.observeOutbox() } returns flowOf(emptyList()) every { repo.observeUnreadCounts() } returns MutableStateFlow(unreadCounts) @@ -647,3 +681,9 @@ class MailboxViewModelTest { inInbox = true, ) } + +/** Mirrors the paged-search DAO queries' columns so the stubbed search pagers filter like production. */ +private fun Message.matchesSearch(query: String): Boolean = sender.contains(query, ignoreCase = true) || + senderEmail.contains(query, ignoreCase = true) || + subject.contains(query, ignoreCase = true) || + snippet.contains(query, ignoreCase = true)