From a547c01ff7dac6ced0f055fe33e20c7b4fd509f0 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 1 Jul 2026 22:51:18 -0500 Subject: [PATCH 1/4] feat(onboarding): link Google 2FA help from Gmail app-password step Gmail's app-passwords page rejects accounts that don't have 2-Step Verification enabled, and the setup screen's intro text names that prerequisite without giving the user any way to act on it. Add a nullable MailProvider.twoFactorHelpUrl (set only for Gmail, to Google's "Turn on 2-Step Verification" article) and surface it as a second outlined button under the existing app-password link, reusing the same UriHandler + snackbar failure plumbing. Yahoo and iCloud screens are unchanged. Closes #98 Co-Authored-By: Claude Fable 5 --- .../ui/onboarding/OnboardingFlowTest.kt | 20 +++++++++++++++- .../libremail/domain/model/MailProvider.kt | 9 +++++++ .../ui/accountsetup/AppPasswordSetupScreen.kt | 24 +++++++++++++++---- app/src/main/res/values/strings.xml | 1 + .../domain/model/MailProviderTest.kt | 15 ++++++++++++ 5 files changed, 63 insertions(+), 6 deletions(-) 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 b35a7bf..580543f 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/onboarding/OnboardingFlowTest.kt @@ -53,7 +53,7 @@ class OnboardingFlowTest { @get:Rule val composeTestRule = createAndroidComposeRule() - private fun string(resId: Int) = composeTestRule.activity.getString(resId) + private fun string(resId: Int, vararg args: Any) = composeTestRule.activity.getString(resId, *args) // Generous cap for the slow, animation-disabled CI matrix emulators; waitUntil returns as soon // as the text appears, so the happy path is unaffected. @@ -206,6 +206,10 @@ class OnboardingFlowTest { // add" button sit below the fold of this scrolling screen, and a positional click on an // off-screen button is a silent no-op (which is why this passed only on API 37's taller AVD). waitForText(string(R.string.app_password_email)) + // Gmail requires 2-Step Verification before app passwords, so its screen (and only its + // screen — see yahooSetup_hasNoTwoFactorHelpLink) links Google's setup article (issue #98). + composeTestRule.onNodeWithText(string(R.string.app_password_2fa_help)) + .performScrollTo().assertIsDisplayed() composeTestRule.onNodeWithText(string(R.string.app_password_email)) .performScrollTo().performTextInput("e2e@gmail.com") composeTestRule.onNodeWithText(string(R.string.app_password_field)) @@ -221,4 +225,18 @@ class OnboardingFlowTest { waitForText("E2E first message") composeTestRule.onNodeWithText("E2E first message").assertIsDisplayed() } + + @Test + fun yahooSetup_hasNoTwoFactorHelpLink() { + setOnboardingContent(FakeAccountRepository(), FakeMailRepository()) + + composeTestRule.onNodeWithText(string(R.string.onboarding_add_account)).performClick() + waitForText("Yahoo Mail") + composeTestRule.onNodeWithText("Yahoo Mail").performClick() + + // Yahoo's setup screen keeps its app-password link… + waitForText(string(R.string.app_password_open_page, "Yahoo Mail")) + // …but gains no 2-Step Verification link: that prerequisite is Gmail-specific (issue #98). + composeTestRule.onNodeWithText(string(R.string.app_password_2fa_help)).assertDoesNotExist() + } } 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 927b32e..20f25a8 100644 --- a/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt +++ b/app/src/main/kotlin/org/libremail/domain/model/MailProvider.kt @@ -29,6 +29,12 @@ enum class MailProvider( val displayName: String, /** The page where the user creates an app password for this provider. */ val appPasswordHelpUrl: String, + /** + * Setup instructions for the provider's two-factor prerequisite, or null when there isn't one. + * Only Gmail refuses to create app passwords until 2-Step Verification is on, so only Gmail + * links its setup article; Yahoo and iCloud gate nothing on it. + */ + val twoFactorHelpUrl: String? = null, private val imapHost: String, private val smtpHost: String, private val smtpPort: Int, @@ -38,6 +44,9 @@ enum class MailProvider( key = "gmail", displayName = "Gmail", appPasswordHelpUrl = "https://myaccount.google.com/apppasswords", + // Google's "Turn on 2-Step Verification" article — the app-passwords page above bounces + // accounts that haven't enabled it yet, so the setup screen offers this as a way out. + twoFactorHelpUrl = "https://support.google.com/accounts/answer/185839", imapHost = "imap.gmail.com", smtpHost = "smtp.gmail.com", // Google documents smtp.gmail.com:587 with STARTTLS as the standard submission endpoint. diff --git a/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreen.kt b/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreen.kt index ff910e9..1a640ba 100644 --- a/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/accountsetup/AppPasswordSetupScreen.kt @@ -78,6 +78,12 @@ fun AppPasswordSetupScreen( val scope = rememberCoroutineScope() // Resolved up front so the failure handler (a non-composable lambda) can use it. val openFailedMessage = stringResource(R.string.app_password_open_failed) + // Shared by every outbound link on this screen: openUri throws if no browser/handler is + // installed, so surface that as an inline snackbar instead of crashing. + val openUrl: (String) -> Unit = { url -> + runCatching { uriHandler.openUri(url) } + .onFailure { scope.launch { snackbarHostState.showSnackbar(openFailedMessage) } } + } LaunchedEffect(form.status, form.addedAccountId) { if (form.status == SetupStatus.DONE) { @@ -146,15 +152,23 @@ fun AppPasswordSetupScreen( Spacer(Modifier.height(12.dp)) OutlinedButton( - onClick = { - // openUri throws if no browser/handler is installed; surface it instead of crashing. - runCatching { uriHandler.openUri(provider.appPasswordHelpUrl) } - .onFailure { scope.launch { snackbarHostState.showSnackbar(openFailedMessage) } } - }, + onClick = { openUrl(provider.appPasswordHelpUrl) }, modifier = Modifier.fillMaxWidth(), ) { Text(stringResource(R.string.app_password_open_page, provider.displayName)) } + // Only Gmail has a two-factor prerequisite (see MailProvider.twoFactorHelpUrl): its + // app-passwords page rejects accounts without 2-Step Verification, so give those users + // a way to set it up instead of a dead end. + provider.twoFactorHelpUrl?.let { twoFactorHelpUrl -> + Spacer(Modifier.height(8.dp)) + OutlinedButton( + onClick = { openUrl(twoFactorHelpUrl) }, + modifier = Modifier.fillMaxWidth(), + ) { + Text(stringResource(R.string.app_password_2fa_help)) + } + } Spacer(Modifier.height(20.dp)) OutlinedTextField( diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 9b713a0..6b604ab 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -152,6 +152,7 @@ An app password is a one-off password that lets an app sign in to your account without your main password or a two-factor code. Store this app password carefully — it grants full access to your email. LibreMail keeps it only on this device. Create an app password for %1$s + How to turn on 2-Step Verification Couldn\'t open your browser Email address App password 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 52a8519..e8f868e 100644 --- a/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt +++ b/app/src/test/kotlin/org/libremail/domain/model/MailProviderTest.kt @@ -80,6 +80,21 @@ class MailProviderTest { } } + @Test + fun `only gmail links two-factor setup help, over https`() { + // Gmail's app-passwords page rejects accounts without 2-Step Verification, so its setup + // screen must offer the setup article as a way out (issue #98). + val gmailUrl = assertNotNull( + MailProvider.GMAIL.twoFactorHelpUrl, + "gmail must expose a 2-Step Verification help URL", + ) + assertTrue(gmailUrl.startsWith("https://"), "gmail 2FA help URL must be https") + + // Yahoo and iCloud gate nothing on two-factor, so they must not grow the extra link. + assertNull(MailProvider.YAHOO.twoFactorHelpUrl) + assertNull(MailProvider.ICLOUD.twoFactorHelpUrl) + } + @Test fun `createAccount trims the email and derives a stable id and display name`() { val account = MailProvider.GMAIL.createAccount(" User@Gmail.com ") From bce82452d088a5eb8fa0bf8a46ff88e9672f7c61 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 02:19:05 -0500 Subject: [PATCH 2/4] fix(sync): make backfill age floor robust to out-of-order dates and guard lowestSyncedUid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two code-review-derived backfill-correctness bugs (from the PR #46 review). Both govern where MailBackfiller stops and resumes paging a folder, so they are fixed together. (MIN(timestampMillis)), but paging descends by UID. One high-UID message with an old Date header (moved/imported mail) dragged the cached minimum below the cutoff and marked the folder complete while lower-UID within-retention messages were still unfetched — a silent, permanent gap (completion is sticky). The age floor is now decided from each page actually fetched: only a page ENTIRELY older than the cutoff (or folder exhaustion) ends paging, and such a prune-fodder page is not persisted. The count floor keeps its cheap cache check — it orders by UID like paging, so inversions can't bite it. oldestSyncedTimestamp had no remaining caller and is removed. migrated before the uid column existed, or a UIDFolder.getUID -1 fetch); fetchOlderThan treats beforeUid <= 1 as "nothing older", so the folder was falsely marked fully backfilled. lowestSyncedUid now ignores uid <= 0 rows (matching MailSyncer's minWindowUid guard), a stale persisted boundary <= 0 is discarded on resume, and the per-page descent takes min over positive UIDs only. A page of entirely unresolved UIDs stalls the folder — it stays incomplete (a future scheduled run retries) but reports no immediate more-work, so BackfillWorker's slice-chaining loop can't busy-spin on it. Together: #95 guarantees paging always descends with a real positive UID boundary, and #94 makes the stop decision independent of cached aggregates, so a placeholder or old-Dated row can no longer end backfill early through either path. Completion stays sticky and is declared only on positive evidence, preserving the #12/#13 backfill/pruner non-interference. Tests (JVM, GreenMail + the existing in-memory DAO-fake harness; all four fail against the pre-fix code): a high-UID/old-Date message must not gap within-retention history (#94); an entirely-old page ends paging without persisting prune-fodder (#94); a uid=0 row must not poison the boundary (#95); a page of unresolvable UIDs stalls instead of falsely completing (#95). MessageDaoRetentionTest pins the uid > 0 SQL guard against real SQLite and drops the removed oldestSyncedTimestamp probe. Closes #94 Closes #95 Co-Authored-By: Claude Fable 5 --- .../data/local/MessageDaoRetentionTest.kt | 25 ++-- .../libremail/data/local/dao/MessageDao.kt | 22 +-- .../org/libremail/data/sync/MailBackfiller.kt | 102 +++++++++----- .../libremail/data/sync/MailBackfillerTest.kt | 133 +++++++++++++++++- 4 files changed, 223 insertions(+), 59 deletions(-) diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRetentionTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRetentionTest.kt index bb78a66..a4d750e 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRetentionTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/MessageDaoRetentionTest.kt @@ -21,12 +21,12 @@ import org.libremail.data.local.entity.MessageEntity * * These queries *define* the device-only retention floor — the pruner deletes below it * ([MessageDao.syncedIdsBeyondCountInFolder] / [MessageDao.syncedIdsOlderThan]) and the backfiller - * stops above it ([MessageDao.lowestSyncedUid] / [MessageDao.countSynced] / - * [MessageDao.oldestSyncedTimestamp]) — so the whole "backfill and prune never fight over the same - * rows" guarantee rests on their SQL. The [org.libremail.data.sync.MailPruner] / - * [org.libremail.data.sync.MailBackfiller] unit tests mock the DAO, so the `ORDER BY … DESC LIMIT` - * newest-N selection, the strict age cutoff, and the windowed reconcile that spares backfilled history - * are exercised here against a real database instead. + * pages and stops around it ([MessageDao.lowestSyncedUid] / [MessageDao.countSynced]) — so the whole + * "backfill and prune never fight over the same rows" guarantee rests on their SQL. The + * [org.libremail.data.sync.MailPruner] / [org.libremail.data.sync.MailBackfiller] unit tests mock + * the DAO, so the `ORDER BY … DESC LIMIT` newest-N selection, the strict age cutoff, and the + * windowed reconcile that spares backfilled history are exercised here against a real database + * instead. */ @RunWith(AndroidJUnit4::class) class MessageDaoRetentionTest { @@ -153,6 +153,9 @@ class MessageDaoRetentionTest { /** * The backfiller's floor probes reflect only an account's synced rows in the given folder, and are * null/zero for a folder with nothing cached (so the backfiller then starts from `Long.MAX_VALUE`). + * The paging boundary additionally skips `uid <= 0` placeholder rows (#95): a row migrated before + * the `uid` column existed (backfilled to 0) or one whose UID the server failed to resolve (-1) + * must not collapse MIN(uid) to a bound the backfiller treats as "folder fully paged". */ @Test fun floorProbesReflectOnlySyncedRowsInTheFolder() = runBlocking { @@ -160,18 +163,22 @@ class MessageDaoRetentionTest { listOf( message("a", uid = 30, timestampMillis = 300), message("d", uid = 10, timestampMillis = 100), + message("legacy", uid = 0, timestampMillis = 40), // pre-uid-column migration row (#95) + message("unresolved", uid = -1, timestampMillis = 30), // UIDFolder.getUID failure (#95) message("search", uid = 1, timestampMillis = 1, inInbox = false), // excluded message("archive", uid = 5, timestampMillis = 50, folder = "Archive"), // different folder ), ) assertEquals(10L, dao.lowestSyncedUid("acct", "INBOX")) - assertEquals(2, dao.countSynced("acct", "INBOX")) - assertEquals(100L, dao.oldestSyncedTimestamp("acct", "INBOX")) + assertEquals(4, dao.countSynced("acct", "INBOX")) assertNull(dao.lowestSyncedUid("acct", "Nonexistent")) assertEquals(0, dao.countSynced("acct", "Nonexistent")) - assertNull(dao.oldestSyncedTimestamp("acct", "Nonexistent")) + + // A folder holding ONLY placeholder rows has no usable boundary: null (start from the top). + dao.insertNew(listOf(message("only-legacy", uid = 0, folder = "Imported"))) + assertNull(dao.lowestSyncedUid("acct", "Imported")) } /** Backfill / prune enumerate their targets via [syncedFolders]: distinct synced folders, per account. */ 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 51e3704..dfc556c 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 @@ -104,21 +104,25 @@ interface MessageDao { ) suspend fun deleteSyncedInWindowNotIn(accountId: String, folder: String, minWindowUid: Long, keepIds: List) - /** Lowest cached UID among an account's synced rows in [folder] — the backfill boundary. Null if none. */ - @Query("SELECT MIN(uid) FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1") + /** + * Lowest cached *resolved* UID among an account's synced rows in [folder] — the backfill + * boundary. Placeholder rows with `uid <= 0` (a row migrated before the `uid` column existed, or + * a fetch where the server failed to resolve the UID) are excluded: letting one collapse + * MIN(uid) to `<= 0` would make the backfiller page below a bound `fetchOlderThan` treats as + * "nothing older", falsely marking the folder fully backfilled (#95, matching the + * `minWindowUid` guard in MailSyncer). Null when no resolved-UID row exists, in which case + * backfill starts over from the newest message. + */ + @Query( + "SELECT MIN(uid) FROM messages WHERE accountId = :accountId AND folder = :folder " + + "AND inInbox = 1 AND uid > 0", + ) suspend fun lowestSyncedUid(accountId: String, folder: String): Long? /** Number of an account's synced rows in [folder] (count-based retention floor / prune sizing). */ @Query("SELECT COUNT(*) FROM messages WHERE accountId = :accountId AND folder = :folder AND inInbox = 1") suspend fun countSynced(accountId: String, folder: String): Int - /** Oldest cached timestamp among an account's synced rows in [folder] (age-based retention floor). Null if none. */ - @Query( - "SELECT MIN(timestampMillis) FROM messages " + - "WHERE accountId = :accountId AND folder = :folder AND inInbox = 1", - ) - suspend fun oldestSyncedTimestamp(accountId: String, folder: String): Long? - /** Distinct folders that have at least one synced row for [accountId] (backfill/prune targets). */ @Query("SELECT DISTINCT folder FROM messages WHERE accountId = :accountId AND inInbox = 1") suspend fun syncedFolders(accountId: String): List diff --git a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt index 8a2e822..7d8fa92 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/MailBackfiller.kt @@ -54,12 +54,15 @@ class MailBackfiller @Inject constructor( private val mailRepository: MailRepository, private val maintenanceGate: MailMaintenanceGate, ) { - private data class FolderResult(val batches: Int, val complete: Boolean) + /** One folder's slice outcome: pages fetched, and whether an immediate follow-up slice has work to do. */ + private data class FolderResult(val batches: Int, val moreWork: Boolean) /** * Runs one bounded slice of backfill across all accounts and their synced folders. Does at most - * [maxBatches] server pages total, persisting progress after each, then returns whether any - * folder still has history left to fetch (so the caller may schedule another run sooner). + * [maxBatches] server pages total, persisting progress after each, then returns whether an + * immediate follow-up slice has more work to do (so the caller may chain another run). A folder + * that stalled (see the unresolved-UID guard in [backfillFolder]) stays incomplete but does not + * count as more work — it is retried on a future scheduled run instead of spun on back-to-back. */ suspend fun runBackfill(maxBatches: Int = DEFAULT_MAX_BATCHES): Boolean = maintenanceGate.mutex.withLock { var remaining = maxBatches @@ -71,9 +74,9 @@ class MailBackfiller @Inject constructor( if (remaining <= 0) return@withLock true // Per-folder failures (e.g. a transient server error) must not abort the whole slice. val result = runCatching { backfillFolder(account, params, folder, policy, remaining) } - .getOrElse { FolderResult(batches = 0, complete = false) } + .getOrElse { FolderResult(batches = 0, moreWork = true) } remaining -= result.batches - if (!result.complete) moreWork = true + if (result.moreWork) moreWork = true } } moreWork @@ -88,57 +91,88 @@ class MailBackfiller @Inject constructor( ): FolderResult { val progress = backfillProgressDao.get(account.id, folder) if (progress?.complete == true) { - return FolderResult(batches = 0, complete = true) + return FolderResult(batches = 0, moreWork = false) } // Resume from the persisted low-water mark so paging is monotonic: it never re-descends into a // region an earlier run already reached, even after the pruner deletes rows above it. Falls back // to the lowest currently-cached UID on the very first run, before any progress is persisted. - var beforeUid = progress?.nextBeforeUid + // Both sources are guarded against unresolved-UID placeholders (#95): lowestSyncedUid excludes + // `uid <= 0` rows at the SQL level, and a stale persisted boundary `<= 0` is discarded rather + // than trusted — fetchOlderThan treats such a bound as "nothing older", which would falsely + // mark the folder fully backfilled. + var beforeUid = progress?.nextBeforeUid?.takeIf { it > 0L } ?: messageDao.lowestSyncedUid(account.id, folder) ?: Long.MAX_VALUE var batches = 0 + var complete = false + var stalled = false while (batches < maxBatches) { currentCoroutineContext().ensureActive() - // Retention floor (#13 precedence): once the folder holds everything retention keeps, mark it - // complete and stop. Marking complete (rather than pausing) is what keeps backfill and the - // pruner from fighting: otherwise the pruner deleting aged-out rows would raise the oldest - // cached timestamp back above the age cutoff and re-open paging on the next run, forever. A - // retention change resets progress (AccountRepository.resetBackfillProgress) so loosening - // still resumes paging. - if (reachedRetentionFloor(account.id, folder, policy)) { - markComplete(account.id, folder, beforeUid) - return FolderResult(batches, complete = true) + // Count floor (#13 precedence): once the folder holds as many messages as retention keeps, + // stop before fetching another page. Unlike the age floor below, this can be decided from + // the cache alone: the count pruner keeps the newest-N by UID — exactly the order paging + // descends in — so a Date/UID inversion cannot make it stop early. + if (reachedCountFloor(account.id, folder, policy)) { + complete = true + break } val fetched = imapClient.fetchOlderThan(params, folder, beforeUid, BACKFILL_BATCH_SIZE) batches++ - if (fetched.isEmpty()) { - // Genuine end of the folder — mark complete so it is skipped on future runs. - markComplete(account.id, folder, beforeUid) - return FolderResult(batches, complete = true) - } val entities = fetched.map { it.toEntity(account.id, folder) } + // Stop at the genuine end of the folder, or at the age floor (#13): a page ENTIRELY older + // than the Date cutoff. The age decision is made from the page actually fetched, NOT from + // the oldest cached timestamp — paging is by UID (arrival order) while the floor cuts by + // the Date header, so a single high-UID message with an old Date (moved/imported mail) + // would drag the cached minimum below the cutoff and end paging while within-retention + // history is still unfetched (#94). An entirely-old page is pure prune-fodder, so it is + // not persisted either. Both cases mark the folder complete (rather than pausing): the + // pruner deleting aged-out rows must never re-open paging on the next run, forever + // re-downloading what was just pruned. A retention change resets progress + // (AccountRepository.resetBackfillProgress) so loosening still resumes paging. + if (fetched.isEmpty() || entirelyBeyondAgeFloor(entities, policy)) { + complete = true + break + } persistBatch(entities) - beforeUid = entities.minOf { it.uid } + // Derive the next boundary only from resolved UIDs (#95): a row whose UID the server + // failed to resolve (UIDFolder.getUID returns -1) must not collapse the boundary to <= 0, + // where fetchOlderThan reads "nothing older" and the folder would be FALSELY marked fully + // backfilled. If a whole page came back unresolved, stall the folder: not complete (so a + // later run retries once the server behaves) but claiming no more work either — an + // immediate follow-up slice would just spin on the same page. + val nextBeforeUid = entities.mapNotNull { entity -> entity.uid.takeIf { it > 0L } }.minOrNull() + if (nextBeforeUid == null) { + stalled = true + break + } + beforeUid = nextBeforeUid backfillProgressDao.upsert(BackfillProgressEntity(account.id, folder, beforeUid, complete = false)) prefetchIfEnabled(entities.map { it.id }) // Breathe between pages so a large mailbox doesn't hammer the server. delay(BACKFILL_BATCH_DELAY_MS) } - return FolderResult(batches, complete = false) + if (complete) markComplete(account.id, folder, beforeUid) + return FolderResult(batches, moreWork = !complete && !stalled) } - /** True once the folder already holds as much as the retention policy would keep (or more). */ - private suspend fun reachedRetentionFloor(accountId: String, folder: String, policy: RetentionPolicy): Boolean { - if (policy.isUnlimited) return false - policy.countLimit?.let { limit -> - if (messageDao.countSynced(accountId, folder) >= limit) return true - } - policy.ageCutoffMillis(System.currentTimeMillis())?.let { cutoff -> - val oldest = messageDao.oldestSyncedTimestamp(accountId, folder) - if (oldest != null && oldest < cutoff) return true - } - return false + /** True once the folder already holds as many messages as the count retention keeps (or more). */ + private suspend fun reachedCountFloor(accountId: String, folder: String, policy: RetentionPolicy): Boolean { + val limit = policy.countLimit ?: return false + return messageDao.countSynced(accountId, folder) >= limit + } + + /** + * True when a fetched page sits entirely below the age retention floor — every message on it is + * older than the policy's Date cutoff. Deciding per page (rather than from the single oldest + * cached timestamp) makes the floor robust to Date/UID inversions (#94): one old-Dated high-UID + * message ends paging only if a whole page around it is old too. The trade is deliberate — a + * pathologically interleaved mailbox may over-fetch (the pruner reclaims the excess), but + * backfill never silently gaps within-retention history. + */ + private fun entirelyBeyondAgeFloor(page: List, policy: RetentionPolicy): Boolean { + val cutoff = policy.ageCutoffMillis(System.currentTimeMillis()) ?: return false + return page.isNotEmpty() && page.all { it.timestampMillis < cutoff } } /** Inserts backfilled headers; never deletes. Uncancellable so a persisted boundary always has its rows. */ diff --git a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt index c900ba4..7cbd260 100644 --- a/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt +++ b/app/src/test/kotlin/org/libremail/data/sync/MailBackfillerTest.kt @@ -34,6 +34,7 @@ import org.libremail.domain.model.AccountSettings import org.libremail.domain.model.ImapConnectionParams import org.libremail.domain.model.MailSecurity import org.libremail.domain.repository.MailRepository +import org.libremail.mail.FetchedMessage import org.libremail.mail.ImapClient import org.libremail.power.BatteryStatus import org.libremail.power.BatteryStatusProvider @@ -41,6 +42,7 @@ import java.util.Date import java.util.Properties import kotlin.test.assertEquals import kotlin.test.assertFalse +import kotlin.test.assertNotEquals import kotlin.test.assertNotNull import kotlin.test.assertTrue @@ -221,11 +223,131 @@ class MailBackfillerTest { coVerify(atLeast = 1) { requireNotNull(lastMailRepository).prefetchMessage(any()) } } - /** Builds a backfiller wired to GreenMail with the in-memory fakes and the given account settings. */ + /** + * Regression for #94: backfill pages by UID (arrival order) while the age floor cuts by the Date + * header. A single message with a HIGH UID but an OLD Date (mail moved/imported into the folder) + * used to drag the oldest cached timestamp below the cutoff and stop paging after zero pages, + * silently gapping every older-UID message whose Date is still within retention. The floor must + * instead be decided from the pages actually fetched. + */ + @Test + fun `a high-UID old-Date message must not stop the age floor while history is unfetched`() = runTest { + val now = System.currentTimeMillis() + // UIDs 1..20: genuinely old (~8 months). UIDs 21..99: within retention, Dates ascending with + // UID. UID 100: a recent arrival whose Date header is 8 months old — the Date/UID inversion — + // which lands inside the seeded foreground window. + val old = (1..20).map { now - 8 * MONTH_MILLIS - it * DAY_MILLIS } + val recent = (1..79).map { now - (80 - it) * DAY_MILLIS } + val inverted = listOf(now - 8 * MONTH_MILLIS) + appendMessages(old + recent + inverted) + seedForegroundWindow() + + val backfiller = backfiller(AccountSettings("acct", retentionMonths = 6)) + var guard = 0 + while (backfiller.runBackfill() && guard++ < 10) { /* drive to completion */ } + + val cutoff = now - 6 * MONTH_MILLIS + assertEquals( + recent.size, + cached.count { it.timestampMillis >= cutoff }, + "every within-retention message must be cached — no silent history gap", + ) + assertEquals(true, progress["acct" to "INBOX"]?.complete, "paging still terminates at the floor") + assertNoDeletes() + } + + /** + * The flip side of the page-based age floor (#94): a page ENTIRELY older than the cutoff ends + * paging — the folder is marked complete without caching that page, which is pure prune-fodder + * (persisting it would just make the pruner delete it again, the #12/#13 churn the sticky floor + * exists to prevent). + */ + @Test + fun `an entirely-old page ends age paging without caching beyond the floor`() = runTest { + val now = System.currentTimeMillis() + // UIDs 1..50 are ~8 months old; UIDs 51..100 are within retention and fill the whole window. + val old = (1..50).map { now - 8 * MONTH_MILLIS - (51 - it) * DAY_MILLIS } + val recent = (1..50).map { now - (51 - it) * DAY_MILLIS } + appendMessages(old + recent) + seedForegroundWindow() + + backfiller(AccountSettings("acct", retentionMonths = 6)).runBackfill() + + assertEquals(true, progress["acct" to "INBOX"]?.complete, "one entirely-old page ends the folder") + assertEquals(0, totalOffered, "the beyond-the-floor page must not be cached") + val cutoff = now - 6 * MONTH_MILLIS + assertTrue(cached.none { it.timestampMillis < cutoff }, "nothing older than the cutoff is cached") + assertNoDeletes() + } + + /** + * Regression for #95: a cached row with `uid = 0` — a row migrated before the `uid` column + * existed (MIGRATION_12_13 backfills a non-numeric id tail to 0) — used to collapse MIN(uid) to + * 0, and `fetchOlderThan` treats a bound `<= 1` as "nothing older": the folder was marked fully + * backfilled after caching NOTHING. The boundary must come from resolved (positive) UIDs only. + */ + @Test + fun `a uid 0 placeholder row must not poison the backfill boundary`() = runTest { + appendMessages(TOTAL) + seedForegroundWindow() + // A pre-uid-column row, exactly as MIGRATION_12_13 leaves one whose id tail isn't numeric. + cached += cached.first().copy(id = "acct:INBOX:legacy", uid = 0L) + + val backfiller = backfiller(AccountSettings("acct")) + var guard = 0 + while (backfiller.runBackfill() && guard++ < 10) { /* drive to completion */ } + + assertEquals( + TOTAL, + cached.mapTo(HashSet()) { it.uid }.count { it > 0L }, + "the placeholder row must not stop backfill from paging the full history", + ) + assertEquals(TOTAL - WINDOW, totalOffered, "each backfilled message fetched exactly once") + assertEquals(true, progress["acct" to "INBOX"]?.complete) + assertNoDeletes() + } + + /** + * Regression for #95 (server variant): when every UID on a fetched page is unresolvable + * (UIDFolder.getUID returned -1), the boundary must not descend to -1 — the next fetch would come + * back empty and the folder would be FALSELY marked fully backfilled. The folder must instead + * stall: stay incomplete (retryable on a future run) without claiming immediate more-work (which + * would make the worker's slice-chaining loop spin on the same page). + */ + @Test + fun `a page of unresolvable UIDs stalls the folder instead of falsely completing it`() = runTest { + // One cached window row makes INBOX a backfill target with boundary UID 60. + cached += fetchedMessage(uid = "60").toEntity("acct", "INBOX") + + val imapClient = mockk() + coEvery { imapClient.fetchOlderThan(any(), any(), any(), any()) } answers { + // The real client's contract: a collapsed boundary (<= 1) means "nothing older". + if (thirdArg() <= 1L) emptyList() else listOf(fetchedMessage(uid = "-1")) + } + + val moreWork = backfiller(AccountSettings("acct"), imapClient = imapClient).runBackfill() + + assertNotEquals(true, progress["acct" to "INBOX"]?.complete, "the folder must stay retryable") + assertFalse(moreWork, "a stalled folder must not spin the worker's slice-chaining loop") + coVerify(exactly = 0) { imapClient.fetchOlderThan(any(), any(), match { it <= 1L }, any()) } + } + + private fun fetchedMessage(uid: String) = FetchedMessage( + uid = uid, + sender = "Sender", + senderEmail = "sender@example.org", + subject = "Message $uid", + timestampMillis = System.currentTimeMillis(), + isRead = false, + isFlagged = false, + ) + + /** Builds a backfiller wired to [imapClient] (GreenMail-backed by default) with the in-memory fakes. */ private fun backfiller( accountSettings: AccountSettings, fetchPolicy: FetchPolicy = FetchPolicy.ON_DEMAND, battery: BatteryStatus = BatteryStatus(percent = 100, isCharging = false), + imapClient: ImapClient = client, ): MailBackfiller { val accountDao = mockk() coEvery { accountDao.getAll() } returns listOf(accountEntity) @@ -241,16 +363,13 @@ class MailBackfillerTest { } coEvery { messageDao.lowestSyncedUid("acct", any()) } answers { val folder = secondArg() - cached.filter { it.inInbox && it.folder == folder }.minOfOrNull { it.uid } + // Mirrors the real query's `uid > 0` guard (#95): placeholder rows never drive the boundary. + cached.filter { it.inInbox && it.folder == folder && it.uid > 0L }.minOfOrNull { it.uid } } coEvery { messageDao.countSynced("acct", any()) } answers { val folder = secondArg() cached.count { it.inInbox && it.folder == folder } } - coEvery { messageDao.oldestSyncedTimestamp("acct", any()) } answers { - val folder = secondArg() - cached.filter { it.inInbox && it.folder == folder }.minOfOrNull { it.timestampMillis } - } val backfillProgressDao = mockk(relaxed = true) coEvery { backfillProgressDao.get("acct", any()) } answers { progress["acct" to secondArg()] } @@ -277,7 +396,7 @@ class MailBackfillerTest { accountDao = accountDao, messageDao = messageDao, backfillProgressDao = backfillProgressDao, - imapClient = client, + imapClient = imapClient, connectionFactory = connectionFactory, settingsRepository = settingsRepository, accountSettingsRepository = accountSettingsRepository, From e8c4fc1ea14ab50474a3bb9de27539ff3e9031ad Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 02:27:33 -0500 Subject: [PATCH 3/4] fix(sync): re-enqueue periodic work with UPDATE so upgrades re-apply the schedule Periodic sync, backfill, and prune were enqueued with ExistingPeriodicWorkPolicy.KEEP, so a newer app version's interval or constraint change never reached already-installed devices: KEEP pins the job to the spec from whichever version first scheduled it. Switch the three periodic schedulers to UPDATE (WorkManager 2.8+; 2.11.2 in use), which re-applies the current spec on each app-start re-enqueue while preserving the running period's progress. An unchanged spec is effectively a no-op, so this never resets the schedule on launch the way REPLACE (cancel + re-enqueue) would. The one-shot kicks (syncNow/backfillNow/pruneNow) keep their existing policies -- they are a separate concern from #96. SyncScheduler now injects Provider (via a new WorkManagerModule) instead of calling the WorkManager.getInstance() static directly, so the policy is unit-testable with MockK; the Provider keeps resolution lazy to preserve the previous initialization timing. Co-Authored-By: Claude Fable 5 --- .../ui/settings/AccountSettingsScreenTest.kt | 4 +- .../ui/settings/SettingsScreenTest.kt | 4 +- .../org/libremail/data/sync/SyncScheduler.kt | 28 +++-- .../org/libremail/di/WorkManagerModule.kt | 24 ++++ .../kotlin/org/libremail/push/IdleService.kt | 5 +- .../libremail/data/sync/SyncSchedulerTest.kt | 107 ++++++++++++++++++ 6 files changed, 160 insertions(+), 12 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/di/WorkManagerModule.kt create mode 100644 app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt index 36b3ed2..20e1173 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/AccountSettingsScreenTest.kt @@ -10,6 +10,7 @@ import androidx.lifecycle.SavedStateHandle import androidx.room.Room import androidx.test.core.app.ApplicationProvider import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.work.WorkManager import kotlinx.coroutines.runBlocking import org.junit.Rule import org.junit.Test @@ -27,6 +28,7 @@ import org.libremail.domain.model.ServerConfig import org.libremail.ui.FakeAccountRepository import org.libremail.ui.navigation.Routes import org.libremail.ui.theme.LibreMailTheme +import javax.inject.Provider /** * End-to-end test for the per-account settings screen: editing the signature and toggling the @@ -68,7 +70,7 @@ class AccountSettingsScreenTest { accountRepository = FakeAccountRepository(accounts = listOf(account)), accountSettingsRepository = repository, signatureRepository = SignatureRepository(db.signatureDao()), - syncScheduler = SyncScheduler(context), + syncScheduler = SyncScheduler(Provider { WorkManager.getInstance(context) }), ) composeTestRule.setContent { LibreMailTheme(darkTheme = false, dynamicColor = false) { diff --git a/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt index 84d2832..4db2142 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/settings/SettingsScreenTest.kt @@ -8,6 +8,7 @@ import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performScrollTo import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry +import androidx.work.WorkManager import kotlinx.coroutines.runBlocking import org.junit.Rule import org.junit.Test @@ -25,6 +26,7 @@ import org.libremail.data.sync.SyncScheduler import org.libremail.push.BatteryOptimizationManager import org.libremail.ui.FakeAccountRepository import org.libremail.ui.theme.LibreMailTheme +import javax.inject.Provider /** * End-to-end test for the top-level message-downloading setting: tapping a policy must round-trip @@ -54,7 +56,7 @@ class SettingsScreenTest { appLockManager, keyStore, BatteryOptimizationManager(context), - SyncScheduler(context), + SyncScheduler(Provider { WorkManager.getInstance(context) }), ) composeTestRule.setContent { diff --git a/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt b/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt index 5cd91b4..d23806f 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/SyncScheduler.kt @@ -1,7 +1,6 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.data.sync -import android.content.Context import androidx.work.Constraints import androidx.work.ExistingPeriodicWorkPolicy import androidx.work.ExistingWorkPolicy @@ -10,15 +9,20 @@ import androidx.work.OneTimeWorkRequestBuilder import androidx.work.OutOfQuotaPolicy import androidx.work.PeriodicWorkRequestBuilder import androidx.work.WorkManager -import dagger.hilt.android.qualifiers.ApplicationContext import java.util.concurrent.TimeUnit import javax.inject.Inject +import javax.inject.Provider import javax.inject.Singleton /** Schedules background mail sync, full-history backfill, and retention pruning via WorkManager. */ @Singleton -class SyncScheduler @Inject constructor(@ApplicationContext private val context: Context) { - private val workManager get() = WorkManager.getInstance(context) +class SyncScheduler @Inject constructor( + // A Provider (not the WorkManager itself) so WorkManager.getInstance() is resolved lazily at + // schedule time — never during Hilt's Application field injection, which can run before the + // HiltWorkerFactory that on-demand WorkManager initialization needs is set. + private val workManagerProvider: Provider, +) { + private val workManager get() = workManagerProvider.get() private val networkConstraint = Constraints.Builder() .setRequiredNetworkType(NetworkType.CONNECTED) @@ -41,7 +45,7 @@ class SyncScheduler @Inject constructor(@ApplicationContext private val context: val request = PeriodicWorkRequestBuilder(15, TimeUnit.MINUTES) .setConstraints(networkConstraint) .build() - workManager.enqueueUniquePeriodicWork(PERIODIC_WORK, ExistingPeriodicWorkPolicy.KEEP, request) + workManager.enqueueUniquePeriodicWork(PERIODIC_WORK, PERIODIC_POLICY, request) } /** One-shot sync, e.g. right after an account is added. */ @@ -55,13 +59,13 @@ class SyncScheduler @Inject constructor(@ApplicationContext private val context: /** * Periodic full-history backfill (issue #12). Each run pages a bounded slice and persists its - * boundary, so history fills in over successive runs; KEEP preserves an already-scheduled cadence. + * boundary, so history fills in over successive runs. */ fun schedulePeriodicBackfill() { val request = PeriodicWorkRequestBuilder(30, TimeUnit.MINUTES) .setConstraints(backfillConstraint) .build() - workManager.enqueueUniquePeriodicWork(PERIODIC_BACKFILL, ExistingPeriodicWorkPolicy.KEEP, request) + workManager.enqueueUniquePeriodicWork(PERIODIC_BACKFILL, PERIODIC_POLICY, request) } /** Kicks an immediate backfill slice (e.g. just after an account is added) without waiting for the cadence. */ @@ -79,7 +83,7 @@ class SyncScheduler @Inject constructor(@ApplicationContext private val context: val request = PeriodicWorkRequestBuilder(12, TimeUnit.HOURS) .setConstraints(pruneConstraint) .build() - workManager.enqueueUniquePeriodicWork(PERIODIC_PRUNE, ExistingPeriodicWorkPolicy.KEEP, request) + workManager.enqueueUniquePeriodicWork(PERIODIC_PRUNE, PERIODIC_POLICY, request) } /** Runs pruning promptly, e.g. right after the user tightens a retention limit. */ @@ -95,5 +99,13 @@ class SyncScheduler @Inject constructor(@ApplicationContext private val context: const val ONESHOT_BACKFILL = "libremail_oneshot_backfill" const val PERIODIC_PRUNE = "libremail_periodic_prune" const val ONESHOT_PRUNE = "libremail_oneshot_prune" + + // UPDATE, not KEEP (issue #96). These periodic jobs are re-enqueued at every app start, so KEEP + // pinned an already-installed device to the interval/constraints from the version that first + // scheduled it — later tuning never reached upgraders. UPDATE re-applies the current spec while + // preserving the running period's progress: an unchanged spec is effectively a no-op, so this + // does NOT reset the schedule on launch the way REPLACE (cancel + re-enqueue) would. UPDATE is + // available since WorkManager 2.8. + val PERIODIC_POLICY = ExistingPeriodicWorkPolicy.UPDATE } } diff --git a/app/src/main/kotlin/org/libremail/di/WorkManagerModule.kt b/app/src/main/kotlin/org/libremail/di/WorkManagerModule.kt new file mode 100644 index 0000000..5df1c41 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/di/WorkManagerModule.kt @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.di + +import android.content.Context +import androidx.work.WorkManager +import dagger.Module +import dagger.Provides +import dagger.hilt.InstallIn +import dagger.hilt.android.qualifiers.ApplicationContext +import dagger.hilt.components.SingletonComponent +import javax.inject.Singleton + +/** + * Provides the process-wide [WorkManager] so schedulers can inject it (and unit tests can substitute + * a fake) instead of reaching for the [WorkManager.getInstance] static directly. + */ +@Module +@InstallIn(SingletonComponent::class) +object WorkManagerModule { + + @Provides + @Singleton + fun provideWorkManager(@ApplicationContext context: Context): WorkManager = WorkManager.getInstance(context) +} diff --git a/app/src/main/kotlin/org/libremail/push/IdleService.kt b/app/src/main/kotlin/org/libremail/push/IdleService.kt index a65307c..336dee2 100644 --- a/app/src/main/kotlin/org/libremail/push/IdleService.kt +++ b/app/src/main/kotlin/org/libremail/push/IdleService.kt @@ -134,8 +134,9 @@ class IdleService : Service() { startAsForeground(mode) if (mode == PushMode.POLLING) { AppLog.i(TAG, "Battery low: pausing IMAP IDLE; mail arrives via 15-minute periodic sync") - // The periodic fallback is scheduled at every app start with KEEP, so this is normally a - // no-op — re-asserted here so the fallback provably exists whenever push is paused. + // The periodic fallback is already scheduled at every app start (UPDATE), so re-asserting it + // here is effectively a no-op — done anyway so the fallback provably exists whenever push is + // paused, without disturbing the running period. syncScheduler.schedulePeriodicSync() } else { AppLog.i(TAG, "Battery recovered: resuming IMAP IDLE push") diff --git a/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt b/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt new file mode 100644 index 0000000..24092ed --- /dev/null +++ b/app/src/test/kotlin/org/libremail/data/sync/SyncSchedulerTest.kt @@ -0,0 +1,107 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.sync + +import androidx.work.ExistingPeriodicWorkPolicy +import androidx.work.ExistingWorkPolicy +import androidx.work.OneTimeWorkRequest +import androidx.work.PeriodicWorkRequest +import androidx.work.WorkManager +import io.mockk.mockk +import io.mockk.verify +import org.junit.Test +import javax.inject.Provider + +/** + * The enqueue methods are thin wrappers over WorkManager, so these tests pin the one thing that carries + * behaviour: the existing-work policy each unique job is enqueued with. + */ +class SyncSchedulerTest { + + private val workManager = mockk(relaxed = true) + private val scheduler = SyncScheduler(Provider { workManager }) + + // Issue #96: periodic jobs must re-enqueue with UPDATE (not KEEP). At every app start KEEP would pin + // an already-installed device to the interval/constraints of whichever version first scheduled the + // job, so cadence/constraint tuning never reached upgraders; UPDATE re-applies the current spec. + + @Test + fun `periodic sync is enqueued with UPDATE so upgrades re-apply its schedule`() { + scheduler.schedulePeriodicSync() + + verify { + workManager.enqueueUniquePeriodicWork( + "libremail_periodic_sync", + ExistingPeriodicWorkPolicy.UPDATE, + any(), + ) + } + } + + @Test + fun `periodic backfill is enqueued with UPDATE so upgrades re-apply its schedule`() { + scheduler.schedulePeriodicBackfill() + + verify { + workManager.enqueueUniquePeriodicWork( + "libremail_periodic_backfill", + ExistingPeriodicWorkPolicy.UPDATE, + any(), + ) + } + } + + @Test + fun `periodic prune is enqueued with UPDATE so upgrades re-apply its schedule`() { + scheduler.schedulePeriodicPrune() + + verify { + workManager.enqueueUniquePeriodicWork( + "libremail_periodic_prune", + ExistingPeriodicWorkPolicy.UPDATE, + any(), + ) + } + } + + // The one-shot kicks are a separate concern from #96 and keep their existing policies: syncNow and + // pruneNow REPLACE for a clean immediate attempt; backfillNow KEEPs an in-flight all-account run. + + @Test + fun `syncNow replaces any pending one-shot sync`() { + scheduler.syncNow() + + verify { + workManager.enqueueUniqueWork( + "libremail_oneshot_sync", + ExistingWorkPolicy.REPLACE, + any(), + ) + } + } + + @Test + fun `backfillNow keeps an already-running backfill`() { + scheduler.backfillNow() + + verify { + workManager.enqueueUniqueWork( + "libremail_oneshot_backfill", + ExistingWorkPolicy.KEEP, + any(), + ) + } + } + + @Test + fun `pruneNow replaces any pending one-shot prune`() { + scheduler.pruneNow() + + verify { + workManager.enqueueUniqueWork( + "libremail_oneshot_prune", + ExistingWorkPolicy.REPLACE, + any(), + ) + } + } +} From 9b970af2d170c24ecf48f7f97c773c4a4d933f7f Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 02:37:17 -0500 Subject: [PATCH 4/4] feat(drawer): show per-folder unread counts and bold accounts with unread mail Adds a live unread-count signal shared by two navigation-drawer indicators: - #83: each folder row shows a trailing unread-count badge (capped at "99+"), hidden when zero. Screen readers announce the exact count via a plurals content description. - #84: accounts with unread mail render their email in bold in the drawer account switcher and the mailbox account-filter chips. Both derive from one efficient Room aggregate, MessageDao.observeUnreadCounts(): a COUNT(*) ... GROUP BY accountId, folder over folder-synced rows (inInbox = 1 AND isRead = 0) that pulls no message rows into memory. Its GROUP BY is served by the existing (accountId, folder, uid) index, so no schema change or migration is needed. MailboxViewModel derives folderUnreadCounts (drawer account, per folder) and accountsWithUnread (any folder) from the one shared flow. Unread scope is folder-synced mail in any folder, kept consistent across both features: an account reads as bold exactly when one of its folders shows a badge. Co-Authored-By: Claude Fable 5 --- .../data/local/LibreMailDatabaseTest.kt | 28 +++++++++ .../kotlin/org/libremail/ui/Fakes.kt | 4 ++ .../libremail/ui/mailbox/FolderDrawerTest.kt | 27 ++++++++ .../org/libremail/data/local/Mappers.kt | 8 +++ .../libremail/data/local/dao/MessageDao.kt | 14 +++++ .../data/local/entity/FolderUnreadCount.kt | 9 +++ .../data/repository/MailRepositoryImpl.kt | 5 ++ .../org/libremail/domain/model/UnreadCount.kt | 9 +++ .../domain/repository/MailRepository.kt | 8 +++ .../org/libremail/ui/mailbox/FolderDrawer.kt | 62 +++++++++++++++++-- .../org/libremail/ui/mailbox/MailboxScreen.kt | 20 +++++- .../libremail/ui/mailbox/MailboxViewModel.kt | 23 +++++++ app/src/main/res/values/strings.xml | 7 +++ .../ui/mailbox/MailboxViewModelTest.kt | 47 ++++++++++++++ 14 files changed, 264 insertions(+), 7 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/data/local/entity/FolderUnreadCount.kt create mode 100644 app/src/main/kotlin/org/libremail/domain/model/UnreadCount.kt diff --git a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt index 16cbccf..b4ed6cb 100644 --- a/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/data/local/LibreMailDatabaseTest.kt @@ -53,6 +53,34 @@ class LibreMailDatabaseTest { isStarred = false, ) + @Test + fun observeUnreadCountsAggregatesUnreadSyncedRowsPerAccountAndFolder() = runBlocking { + val messageDao = db.messageDao() + messageDao.insertNew( + listOf( + // acct / INBOX: two unread + one read -> counts 2. + message("acct:INBOX:1").copy(folder = "INBOX", isRead = false), + message("acct:INBOX:2").copy(folder = "INBOX", isRead = false), + message("acct:INBOX:3").copy(folder = "INBOX", isRead = true), + // acct / Archive: one unread -> counts 1. + message("acct:Archive:1").copy(folder = "Archive", isRead = false), + // An unread server-search hit (inInbox = false) must never inflate a badge. + message("acct:INBOX:search").copy(folder = "INBOX", isRead = false, inInbox = false), + // A second account's unread inbox row is counted under its own accountId. + message("acct2:INBOX:1").copy(accountId = "acct2", folder = "INBOX", isRead = false), + ), + ) + + val counts = messageDao.observeUnreadCounts().first() + .associate { (it.accountId to it.folder) to it.unreadCount } + + assertEquals(2, counts[("acct" to "INBOX")]) + assertEquals(1, counts[("acct" to "Archive")]) + assertEquals(1, counts[("acct2" to "INBOX")]) + // Fully-read folders and search-only rows produce no group at all. + assertEquals(3, counts.size) + } + @Test fun deletingMessageCascadesToItsAttachments() = runBlocking { val messageDao = db.messageDao() diff --git a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt index c4dade3..b3adafa 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/Fakes.kt @@ -14,6 +14,7 @@ import org.libremail.domain.model.Message import org.libremail.domain.model.OutboxMessage import org.libremail.domain.model.OutgoingMessage import org.libremail.domain.model.ReplyMode +import org.libremail.domain.model.UnreadCount import org.libremail.domain.repository.AccountRepository import org.libremail.domain.repository.MailRepository import java.io.File @@ -72,6 +73,7 @@ class FakeMailRepository( private val folders: List = emptyList(), private val attachments: List = emptyList(), private val downloadedParts: Set = emptySet(), + private val unreadCounts: List = emptyList(), ) : MailRepository { val sentMessages = mutableListOf() @@ -93,6 +95,8 @@ class FakeMailRepository( }, ) + override fun observeUnreadCounts(): Flow> = flowOf(unreadCounts) + override suspend fun refreshFolders(accountId: String): Result = Result.success(Unit) override suspend fun getMessage(id: String): Message? = messages.firstOrNull { it.id == id } diff --git a/app/src/androidTest/kotlin/org/libremail/ui/mailbox/FolderDrawerTest.kt b/app/src/androidTest/kotlin/org/libremail/ui/mailbox/FolderDrawerTest.kt index 711d87c..2abb830 100644 --- a/app/src/androidTest/kotlin/org/libremail/ui/mailbox/FolderDrawerTest.kt +++ b/app/src/androidTest/kotlin/org/libremail/ui/mailbox/FolderDrawerTest.kt @@ -5,6 +5,7 @@ import androidx.activity.ComponentActivity import androidx.compose.material3.ModalDrawerSheet import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.compose.ui.test.onNodeWithContentDescription import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.test.ext.junit.runners.AndroidJUnit4 @@ -145,10 +146,34 @@ class FolderDrawerTest { assertTrue(unifiedTapped) } + @Test + fun folderWithUnreadMail_showsCountBadge_andReadFolderShowsNone() { + setContent( + accounts = listOf(alice), + drawerAccount = alice, + folders = listOf( + folder("imap:a", "INBOX", "INBOX", FolderRole.INBOX), + folder("imap:a", "Archive", "Archive", FolderRole.ARCHIVE), + ), + folderUnreadCounts = mapOf("INBOX" to 3), + ) + + // The inbox badge announces its exact count for screen readers. + val threeUnread = composeTestRule.activity.resources + .getQuantityString(R.plurals.folder_unread_count_description, 3, 3) + composeTestRule.onNodeWithContentDescription(threeUnread).assertIsDisplayed() + // Archive has no unread mail, so no badge is rendered for it. + val oneUnread = composeTestRule.activity.resources + .getQuantityString(R.plurals.folder_unread_count_description, 1, 1) + composeTestRule.onNodeWithContentDescription(oneUnread).assertDoesNotExist() + } + private fun setContent( accounts: List, drawerAccount: Account?, folders: List, + folderUnreadCounts: Map = emptyMap(), + accountsWithUnread: Set = emptySet(), selectedAccountId: String? = null, selectedFolder: String = "INBOX", onSelectUnifiedInbox: () -> Unit = {}, @@ -162,6 +187,8 @@ class FolderDrawerTest { accounts = accounts, drawerAccount = drawerAccount, folders = folders, + folderUnreadCounts = folderUnreadCounts, + accountsWithUnread = accountsWithUnread, selectedAccountId = selectedAccountId, selectedFolder = selectedFolder, onSelectUnifiedInbox = onSelectUnifiedInbox, diff --git a/app/src/main/kotlin/org/libremail/data/local/Mappers.kt b/app/src/main/kotlin/org/libremail/data/local/Mappers.kt index 3247fbe..c05bbb9 100644 --- a/app/src/main/kotlin/org/libremail/data/local/Mappers.kt +++ b/app/src/main/kotlin/org/libremail/data/local/Mappers.kt @@ -8,6 +8,7 @@ import org.libremail.data.local.entity.AccountSettingsEntity 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.FolderUnreadCount import org.libremail.data.local.entity.MessageEntity import org.libremail.data.local.entity.MessageSummary import org.libremail.data.local.entity.OutboxEntity @@ -26,6 +27,7 @@ import org.libremail.domain.model.OutboxMessage import org.libremail.domain.model.OutgoingAttachment import org.libremail.domain.model.ServerConfig import org.libremail.domain.model.SmtpParams +import org.libremail.domain.model.UnreadCount import org.libremail.mail.AttachmentPart import org.libremail.mail.FetchedFolder import org.libremail.mail.FetchedMessage @@ -168,6 +170,12 @@ internal fun FetchedFolder.toEntity(accountId: String, sortOrder: Int): FolderEn specialUse = FolderRole.isServerSpecial(attributes), ) +internal fun FolderUnreadCount.toDomain(): UnreadCount = UnreadCount( + accountId = accountId, + folder = folder, + count = unreadCount, +) + internal fun AttachmentEntity.toDomain(): Attachment = Attachment( messageId = messageId, partIndex = partIndex, 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 51e3704..88579f3 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 @@ -6,6 +6,7 @@ import androidx.room.Insert import androidx.room.OnConflictStrategy 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.MessageSummary @@ -23,6 +24,19 @@ interface MessageDao { ) fun observeSummaries(): Flow> + /** + * 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 + * server-search hits never inflate a badge; read rows and folders with no unread mail are simply + * absent from the result. A pure `COUNT(*)` aggregate — no message rows are pulled into memory — + * whose `GROUP BY accountId, folder` is served by the existing `(accountId, folder, uid)` index. + */ + @Query( + "SELECT accountId, folder, COUNT(*) AS unreadCount FROM messages " + + "WHERE inInbox = 1 AND isRead = 0 GROUP BY accountId, folder", + ) + fun observeUnreadCounts(): Flow> + @Query("SELECT * FROM messages WHERE id = :id LIMIT 1") suspend fun getById(id: String): MessageEntity? diff --git a/app/src/main/kotlin/org/libremail/data/local/entity/FolderUnreadCount.kt b/app/src/main/kotlin/org/libremail/data/local/entity/FolderUnreadCount.kt new file mode 100644 index 0000000..9f53a00 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/local/entity/FolderUnreadCount.kt @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.local.entity + +/** + * Aggregate projection of [MessageEntity] counting unread, folder-synced rows per (account, folder). + * Produced by `MessageDao.observeUnreadCounts`' `GROUP BY` query — never a stored table — so the + * counts come straight from SQLite without pulling any message rows into memory. + */ +data class FolderUnreadCount(val accountId: String, val folder: String, val unreadCount: Int) 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 ba3f629..61ff531 100644 --- a/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt +++ b/app/src/main/kotlin/org/libremail/data/repository/MailRepositoryImpl.kt @@ -36,6 +36,7 @@ import org.libremail.domain.model.OutboxMessage import org.libremail.domain.model.OutgoingAttachment import org.libremail.domain.model.OutgoingMessage import org.libremail.domain.model.ReplyMode +import org.libremail.domain.model.UnreadCount import org.libremail.domain.repository.MailRepository import org.libremail.mail.ImapClient import java.io.File @@ -68,6 +69,10 @@ class MailRepositoryImpl @Inject constructor( rows.map { it.toDomain() } } + override fun observeUnreadCounts(): Flow> = messageDao.observeUnreadCounts().map { rows -> + rows.map { it.toDomain() } + } + override suspend fun refreshFolders(accountId: String): Result = runCatching { val account = accountDao.getById(accountId)?.toDomain() ?: error("Account not found") val params = connectionFactory.imapParamsFor(account) diff --git a/app/src/main/kotlin/org/libremail/domain/model/UnreadCount.kt b/app/src/main/kotlin/org/libremail/domain/model/UnreadCount.kt new file mode 100644 index 0000000..3e0db33 --- /dev/null +++ b/app/src/main/kotlin/org/libremail/domain/model/UnreadCount.kt @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.domain.model + +/** + * The number of unread, folder-synced messages in one account's folder. Emitted per (account, folder) + * pair that currently holds unread mail; pairs with no unread mail are simply absent. Feeds the + * drawer's per-folder unread badges (#83) and the bold styling of accounts that have unread mail (#84). + */ +data class UnreadCount(val accountId: String, val folder: String, val count: Int) 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 1a761b5..4126fc6 100644 --- a/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt +++ b/app/src/main/kotlin/org/libremail/domain/repository/MailRepository.kt @@ -9,6 +9,7 @@ import org.libremail.domain.model.Message import org.libremail.domain.model.OutboxMessage import org.libremail.domain.model.OutgoingMessage import org.libremail.domain.model.ReplyMode +import org.libremail.domain.model.UnreadCount import java.io.File /** @@ -21,6 +22,13 @@ interface MailRepository { /** The account's cached IMAP folders for the navigation drawer. */ fun observeFolders(accountId: String): Flow> + /** + * Live per-(account, folder) unread counts across every account, for the drawer's folder badges + * and the bold styling of accounts with unread mail. Only (account, folder) pairs that currently + * hold unread, folder-synced mail are emitted. + */ + fun observeUnreadCounts(): Flow> + /** Refreshes the account's folder list from the server into the cache. */ suspend fun refreshFolders(accountId: String): Result diff --git a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt index de9a918..8695334 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/FolderDrawer.kt @@ -27,7 +27,11 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier import androidx.compose.ui.graphics.vector.ImageVector +import androidx.compose.ui.res.pluralStringResource import androidx.compose.ui.res.stringResource +import androidx.compose.ui.semantics.clearAndSetSemantics +import androidx.compose.ui.semantics.contentDescription +import androidx.compose.ui.text.font.FontWeight import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import org.libremail.R @@ -37,13 +41,17 @@ import org.libremail.domain.model.FolderRole /** * The navigation drawer's contents: an optional account switcher and "All Inboxes" entry (only with - * 2+ accounts), then the drawer account's folders — standard folders (Inbox, Sent, …) first. + * 2+ accounts), then the drawer account's folders — standard folders (Inbox, Sent, …) first. Each + * folder shows its unread count as a trailing badge ([folderUnreadCounts]), and accounts with unread + * mail ([accountsWithUnread]) render their email in bold. */ @Composable fun FolderDrawer( accounts: List, drawerAccount: Account?, folders: List, + folderUnreadCounts: Map, + accountsWithUnread: Set, selectedAccountId: String?, selectedFolder: String, onSelectUnifiedInbox: () -> Unit, @@ -63,7 +71,7 @@ fun FolderDrawer( ) if (accounts.size >= 2 && drawerAccount != null) { - AccountSwitcher(accounts, drawerAccount, onSelectDrawerAccount) + AccountSwitcher(accounts, drawerAccount, accountsWithUnread, onSelectDrawerAccount) NavigationDrawerItem( label = { Text(stringResource(R.string.folder_all_inboxes)) }, icon = { Icon(Icons.Filled.Email, contentDescription = null) }, @@ -87,9 +95,15 @@ fun FolderDrawer( val iconContent: (@Composable () -> Unit)? = folderIcon(folder.role)?.let { vector -> { Icon(vector, contentDescription = null) } } + val unread = folderUnreadCounts[folder.fullName] ?: 0 NavigationDrawerItem( label = { Text(resolvedLabels[folder.fullName] ?: folderDisplayLabel(folder)) }, icon = iconContent, + badge = if (unread > 0) { + { UnreadBadgeLabel(unread) } + } else { + null + }, selected = isSelected, onClick = { if (folder.selectable && drawerAccount != null) { @@ -103,19 +117,34 @@ fun FolderDrawer( } @Composable -private fun AccountSwitcher(accounts: List, current: Account, onSelect: (String) -> Unit) { +private fun AccountSwitcher( + accounts: List, + current: Account, + accountsWithUnread: Set, + onSelect: (String) -> Unit, +) { var expanded by remember { mutableStateOf(false) } TextButton( onClick = { expanded = true }, modifier = Modifier.padding(horizontal = 16.dp), ) { - Text(current.email, maxLines = 1, overflow = TextOverflow.Ellipsis) + Text( + current.email, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + fontWeight = if (current.id in accountsWithUnread) FontWeight.Bold else FontWeight.Normal, + ) Icon(Icons.Filled.ArrowDropDown, contentDescription = stringResource(R.string.drawer_switch_account)) } DropdownMenu(expanded = expanded, onDismissRequest = { expanded = false }) { accounts.forEach { account -> DropdownMenuItem( - text = { Text(account.email) }, + text = { + Text( + account.email, + fontWeight = if (account.id in accountsWithUnread) FontWeight.Bold else FontWeight.Normal, + ) + }, onClick = { onSelect(account.id) expanded = false @@ -125,6 +154,29 @@ private fun AccountSwitcher(accounts: List, current: Account, onSelect: } } +/** Cap for a folder's visible unread badge; higher counts render as "99+" to keep the row compact. */ +private const val UNREAD_BADGE_CAP = 99 + +/** + * A folder row's trailing unread-count badge. The visible label is capped at [UNREAD_BADGE_CAP] as + * "99+" so a large count can't blow out the row, while the semantics announce the exact count as + * "N unread messages" (overriding the terse glyph) for screen readers. + */ +@Composable +private fun UnreadBadgeLabel(count: Int) { + val display = if (count > UNREAD_BADGE_CAP) { + stringResource(R.string.folder_unread_overflow) + } else { + count.toString() + } + val description = pluralStringResource(R.plurals.folder_unread_count_description, count, count) + Text( + text = display, + style = MaterialTheme.typography.labelMedium, + modifier = Modifier.clearAndSetSemantics { contentDescription = description }, + ) +} + /** * De-duplicated display labels for [folders], keyed by [Folder.fullName] — the one resolution used * by the drawer, the move-to picker, and the app-bar title, so a folder reads the same everywhere. 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 fc4f1aa..663aeaa 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxScreen.kt @@ -106,6 +106,8 @@ fun MailboxScreen( val selectedAccountId by viewModel.selectedAccountId.collectAsStateWithLifecycle() val selectedFolder by viewModel.selectedFolder.collectAsStateWithLifecycle() val folders by viewModel.folders.collectAsStateWithLifecycle() + val folderUnreadCounts by viewModel.folderUnreadCounts.collectAsStateWithLifecycle() + val accountsWithUnread by viewModel.accountsWithUnread.collectAsStateWithLifecycle() val drawerAccount by viewModel.drawerAccount.collectAsStateWithLifecycle() val hasAccounts by viewModel.hasAccounts.collectAsStateWithLifecycle() val draftCount by viewModel.draftCount.collectAsStateWithLifecycle() @@ -156,6 +158,8 @@ fun MailboxScreen( accounts = accounts, drawerAccount = drawerAccount, folders = folders, + folderUnreadCounts = folderUnreadCounts, + accountsWithUnread = accountsWithUnread, selectedAccountId = selectedAccountId, selectedFolder = selectedFolder, onSelectUnifiedInbox = { @@ -256,6 +260,7 @@ fun MailboxScreen( AccountFilterRow( accounts = accounts, selectedId = selectedAccountId, + accountsWithUnread = accountsWithUnread, onSelect = viewModel::selectAccount, ) } @@ -411,7 +416,12 @@ private fun OutboxEntry(count: Int, onClick: () -> Unit) { } @Composable -private fun AccountFilterRow(accounts: List, selectedId: String?, onSelect: (String?) -> Unit) { +private fun AccountFilterRow( + accounts: List, + selectedId: String?, + accountsWithUnread: Set, + onSelect: (String?) -> Unit, +) { Row( modifier = Modifier .fillMaxWidth() @@ -428,7 +438,13 @@ private fun AccountFilterRow(accounts: List, selectedId: String?, onSel FilterChip( selected = selectedId == account.id, onClick = { onSelect(account.id) }, - label = { Text(account.email, maxLines = 1) }, + label = { + Text( + account.email, + maxLines = 1, + fontWeight = if (account.id in accountsWithUnread) FontWeight.Bold else FontWeight.Normal, + ) + }, ) } } 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 180b9a4..d2d60fd 100644 --- a/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/mailbox/MailboxViewModel.kt @@ -28,6 +28,7 @@ import org.libremail.domain.model.Folder import org.libremail.domain.model.FolderRole import org.libremail.domain.model.Message import org.libremail.domain.model.ReplyMode +import org.libremail.domain.model.UnreadCount import org.libremail.domain.repository.AccountRepository import org.libremail.domain.repository.MailRepository import org.libremail.ui.navigation.Routes @@ -87,6 +88,28 @@ class MailboxViewModel @Inject constructor( } .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) + /** + * Live unread counts across every account, shared by the two drawer indicators below so the + * underlying `COUNT` query is collected once no matter how many observers derive from it. + */ + private val unreadCounts: StateFlow> = mailRepository.observeUnreadCounts() + .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) + + /** #83: unread-message count keyed by folder full name for the drawer account (absent = none). */ + val folderUnreadCounts: StateFlow> = + combine(drawerAccount, unreadCounts) { account, counts -> + if (account == null) { + emptyMap() + } else { + counts.filter { it.accountId == account.id }.associate { it.folder to it.count } + } + }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyMap()) + + /** #84: ids of accounts that currently have unread mail in any folder. */ + val accountsWithUnread: StateFlow> = unreadCounts + .map { counts -> counts.filter { it.count > 0 }.map { it.accountId }.toSet() } + .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptySet()) + private val _searchActive = MutableStateFlow(false) val searchActive: StateFlow = _searchActive.asStateFlow() diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 9b713a0..75f6c41 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -68,6 +68,13 @@ Archive Spam Trash + + 99+ + + + %1$d unread message + %1$d unread messages + From 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 b73e4dd..02d2699 100644 --- a/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/mailbox/MailboxViewModelTest.kt @@ -30,6 +30,7 @@ import org.libremail.domain.model.MailSecurity import org.libremail.domain.model.Message import org.libremail.domain.model.ReplyMode import org.libremail.domain.model.ServerConfig +import org.libremail.domain.model.UnreadCount import org.libremail.domain.repository.AccountRepository import org.libremail.domain.repository.MailRepository import org.libremail.ui.navigation.Routes @@ -429,10 +430,55 @@ class MailboxViewModelTest { assertEquals(listOf("imap:a:INBOX:1"), vm.messages.value.map { it.id }) } + @Test + fun `folderUnreadCounts maps the drawer account's folders to their unread counts`() = runTest(testDispatcher) { + val vm = createViewModel( + accounts = listOf(alice, bob), + messages = emptyList(), + unreadCounts = listOf( + UnreadCount("imap:a", "INBOX", 3), + UnreadCount("imap:a", "[Gmail]/Spam", 1), + UnreadCount("imap:b", "INBOX", 9), + ), + ) + backgroundScope.launch { vm.folderUnreadCounts.collect {} } + + // The drawer defaults to the first account (alice); bob's counts are excluded. + assertEquals(mapOf("INBOX" to 3, "[Gmail]/Spam" to 1), vm.folderUnreadCounts.value) + } + + @Test + fun `folderUnreadCounts follows the drawer-account switch`() = runTest(testDispatcher) { + val vm = createViewModel( + accounts = listOf(alice, bob), + messages = emptyList(), + unreadCounts = listOf(UnreadCount("imap:a", "INBOX", 3), UnreadCount("imap:b", "INBOX", 9)), + ) + backgroundScope.launch { vm.folderUnreadCounts.collect {} } + + vm.setDrawerAccount("imap:b") + + assertEquals(mapOf("INBOX" to 9), vm.folderUnreadCounts.value) + } + + @Test + fun `accountsWithUnread lists every account that has unread mail in any folder`() = runTest(testDispatcher) { + val vm = createViewModel( + accounts = listOf(alice, bob), + messages = emptyList(), + // alice has unread mail (in a non-inbox folder too); bob has none. + unreadCounts = listOf(UnreadCount("imap:a", "INBOX", 3), UnreadCount("imap:a", "Archive", 2)), + ) + backgroundScope.launch { vm.accountsWithUnread.collect {} } + + assertEquals(setOf("imap:a"), vm.accountsWithUnread.value) + } + private fun createViewModel( accounts: List, messages: List, folders: Map> = emptyMap(), + unreadCounts: List = emptyList(), syncer: MailSyncer = mockk(relaxed = true), repo: MailRepository = mockk(relaxed = true), initialAccountId: String? = null, @@ -440,6 +486,7 @@ class MailboxViewModelTest { every { repo.observeMessages() } returns MutableStateFlow(messages) every { repo.observeDrafts() } returns flowOf(emptyList()) every { repo.observeOutbox() } returns flowOf(emptyList()) + every { repo.observeUnreadCounts() } returns MutableStateFlow(unreadCounts) accounts.forEach { account -> every { repo.observeFolders(account.id) } returns MutableStateFlow(folders[account.id] ?: emptyList()) }