MailRepositoryImpl.openMessage() ran a live imapClient.setFlag(SEEN) IMAP round trip
(connection + STORE) before returning whenever a message's body was already cached but
unread — purely to mark it read on the server. That network call sat directly on the reader's
critical path even though nothing needed to render the screen (body/attachments) required it. ReaderViewModel.init awaits openMessage() before flipping loading to false, so the whole
screen waited on that avoidable round trip. This is very likely the dominant cost behind
"opening messages is still slow, even with all messages downloaded."
Fix
messageDao.setRead(id, true) now happens immediately (optimistic, local-only), and openMessage() returns without awaiting imapClient.setFlag.
The SEEN push runs fire-and-forget on a new backgroundScope field on MailRepositoryImpl — CoroutineScope(SupervisorJob() + Dispatchers.IO) — so it outlives the caller's coroutine
instead of being cancelled when openMessage() returns. This mirrors the exact pattern already
used by LibreMailApplication.appScope and IdleService.scope: there was no existing
DI-injected/qualified CoroutineScope anywhere in the codebase, so rather than invent new Hilt
qualifier plumbing for a single call site, this follows the established convention (a private
field on a long-lived, here @Singleton, class).
ReaderViewModel needed no changes — awaiting openMessage() is correct (the reader genuinely
needs the returned message to render); the fix makes openMessage() itself fast, which is all
that was needed.
The body-not-yet-cached path (fetchBodyMarkingSeen, which marks SEEN as part of the same
fetch) is untouched.
Divergence handling (design point 2)
Does the existing sync self-heal a dropped/failed SEEN push? No. MessageDao.updateHeaderContent
and insertNew (OnConflictStrategy.IGNORE) are deliberately written so that syncFolder/ syncAccount/syncAllnever overwrite a message's local isRead/isStarred flags with
whatever the server reports — that's intentional, to protect an optimistic local change (e.g. a
star or a read-flag) from being clobbered by stale server state before the corresponding push has
landed. The flip side is that this same protection means sync also never re-drives a push that
never reached the server: if the SEEN flag never makes it to the server, nothing about the normal
sync path will retry it later. So this is one-directional protection, not two-directional
reconciliation, and left alone it would produce a permanent local="read"/server="unread"
divergence on a failed push.
What this PR adds:pushSeenFlagInBackground retries the setFlag call up to 3 times with a
short backoff (2s, then 4s) before giving up silently. This is in-memory only — it does not
persist across process death mid-backoff, and there is no durable/WorkManager-backed retry queue.
Given the "best-effort" framing in the issue's acceptance criteria, this seemed like the right
scope for this PR; a durable retry (or teaching sync to push local read-state upward) would be a
reasonable follow-up if a stronger guarantee is wanted, but is a larger change than "keep the diff
focused on these two files" calls for here.
Testing
Added to MailRepositoryImplTest.kt:
openMessage returns without awaiting the background SEEN-flag push — parks the mocked setFlag call on an un-completed CompletableDeferred and asserts openMessage() still
completes successfully (in ~24ms), then confirms the push is still dispatched independently.
Uses runBlocking + CompletableDeferred (the same idiom as the existing MailMaintenanceGateTest) rather than runTest, since the property under test is genuine
concurrency between the caller and the repository's own background scope.
a failed SEEN-flag push is retried in the background — asserts a second attempt is observed
after the first failure, proving this is a retry loop and not a single silently-swallowed
attempt.
Local gate (JDK 21): assembleDebug + testDebugUnitTest + lintDebug + ktlintCheck + detekt all green.
## Summary
`MailRepositoryImpl.openMessage()` ran a live `imapClient.setFlag(SEEN)` IMAP round trip
(connection + `STORE`) **before returning** whenever a message's body was already cached but
unread — purely to mark it read on the server. That network call sat directly on the reader's
critical path even though nothing needed to render the screen (body/attachments) required it.
`ReaderViewModel.init` awaits `openMessage()` before flipping `loading` to `false`, so the whole
screen waited on that avoidable round trip. This is very likely the dominant cost behind
"opening messages is still slow, even with all messages downloaded."
## Fix
- `messageDao.setRead(id, true)` now happens immediately (optimistic, local-only), and
`openMessage()` returns without awaiting `imapClient.setFlag`.
- The SEEN push runs fire-and-forget on a new `backgroundScope` field on `MailRepositoryImpl` —
`CoroutineScope(SupervisorJob() + Dispatchers.IO)` — so it outlives the caller's coroutine
instead of being cancelled when `openMessage()` returns. This mirrors the exact pattern already
used by `LibreMailApplication.appScope` and `IdleService.scope`: there was no existing
DI-injected/qualified `CoroutineScope` anywhere in the codebase, so rather than invent new Hilt
qualifier plumbing for a single call site, this follows the established convention (a private
field on a long-lived, here `@Singleton`, class).
- `ReaderViewModel` needed no changes — awaiting `openMessage()` is correct (the reader genuinely
needs the returned message to render); the fix makes `openMessage()` itself fast, which is all
that was needed.
- The body-not-yet-cached path (`fetchBodyMarkingSeen`, which marks SEEN as part of the same
fetch) is untouched.
## Divergence handling (design point 2)
**Does the existing sync self-heal a dropped/failed SEEN push?** No. `MessageDao.updateHeaderContent`
and `insertNew` (`OnConflictStrategy.IGNORE`) are deliberately written so that `syncFolder`/
`syncAccount`/`syncAll` **never overwrite a message's local `isRead`/`isStarred` flags** with
whatever the server reports — that's intentional, to protect an optimistic local change (e.g. a
star or a read-flag) from being clobbered by stale server state before the corresponding push has
landed. The flip side is that this same protection means sync also never *re-drives* a push that
never reached the server: if the SEEN flag never makes it to the server, nothing about the normal
sync path will retry it later. So this is one-directional protection, not two-directional
reconciliation, and left alone it would produce a permanent local="read"/server="unread"
divergence on a failed push.
**What this PR adds:** `pushSeenFlagInBackground` retries the `setFlag` call up to 3 times with a
short backoff (2s, then 4s) before giving up silently. This is in-memory only — it does not
persist across process death mid-backoff, and there is no durable/WorkManager-backed retry queue.
Given the "best-effort" framing in the issue's acceptance criteria, this seemed like the right
scope for this PR; a durable retry (or teaching sync to push local read-state upward) would be a
reasonable follow-up if a stronger guarantee is wanted, but is a larger change than "keep the diff
focused on these two files" calls for here.
## Testing
Added to `MailRepositoryImplTest.kt`:
- `openMessage returns without awaiting the background SEEN-flag push` — parks the mocked
`setFlag` call on an un-completed `CompletableDeferred` and asserts `openMessage()` still
completes successfully (in ~24ms), then confirms the push is still dispatched independently.
Uses `runBlocking` + `CompletableDeferred` (the same idiom as the existing
`MailMaintenanceGateTest`) rather than `runTest`, since the property under test is genuine
concurrency between the caller and the repository's own background scope.
- `a failed SEEN-flag push is retried in the background` — asserts a second attempt is observed
after the first failure, proving this is a retry loop and not a single silently-swallowed
attempt.
Local gate (JDK 21): `assembleDebug` + `testDebugUnitTest` + `lintDebug` + `ktlintCheck` +
`detekt` all green.
Closes #148
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Summary
MailRepositoryImpl.openMessage()ran a liveimapClient.setFlag(SEEN)IMAP round trip(connection +
STORE) before returning whenever a message's body was already cached butunread — purely to mark it read on the server. That network call sat directly on the reader's
critical path even though nothing needed to render the screen (body/attachments) required it.
ReaderViewModel.initawaitsopenMessage()before flippingloadingtofalse, so the wholescreen waited on that avoidable round trip. This is very likely the dominant cost behind
"opening messages is still slow, even with all messages downloaded."
Fix
messageDao.setRead(id, true)now happens immediately (optimistic, local-only), andopenMessage()returns without awaitingimapClient.setFlag.backgroundScopefield onMailRepositoryImpl—CoroutineScope(SupervisorJob() + Dispatchers.IO)— so it outlives the caller's coroutineinstead of being cancelled when
openMessage()returns. This mirrors the exact pattern alreadyused by
LibreMailApplication.appScopeandIdleService.scope: there was no existingDI-injected/qualified
CoroutineScopeanywhere in the codebase, so rather than invent new Hiltqualifier plumbing for a single call site, this follows the established convention (a private
field on a long-lived, here
@Singleton, class).ReaderViewModelneeded no changes — awaitingopenMessage()is correct (the reader genuinelyneeds the returned message to render); the fix makes
openMessage()itself fast, which is allthat was needed.
fetchBodyMarkingSeen, which marks SEEN as part of the samefetch) is untouched.
Divergence handling (design point 2)
Does the existing sync self-heal a dropped/failed SEEN push? No.
MessageDao.updateHeaderContentand
insertNew(OnConflictStrategy.IGNORE) are deliberately written so thatsyncFolder/syncAccount/syncAllnever overwrite a message's localisRead/isStarredflags withwhatever the server reports — that's intentional, to protect an optimistic local change (e.g. a
star or a read-flag) from being clobbered by stale server state before the corresponding push has
landed. The flip side is that this same protection means sync also never re-drives a push that
never reached the server: if the SEEN flag never makes it to the server, nothing about the normal
sync path will retry it later. So this is one-directional protection, not two-directional
reconciliation, and left alone it would produce a permanent local="read"/server="unread"
divergence on a failed push.
What this PR adds:
pushSeenFlagInBackgroundretries thesetFlagcall up to 3 times with ashort backoff (2s, then 4s) before giving up silently. This is in-memory only — it does not
persist across process death mid-backoff, and there is no durable/WorkManager-backed retry queue.
Given the "best-effort" framing in the issue's acceptance criteria, this seemed like the right
scope for this PR; a durable retry (or teaching sync to push local read-state upward) would be a
reasonable follow-up if a stronger guarantee is wanted, but is a larger change than "keep the diff
focused on these two files" calls for here.
Testing
Added to
MailRepositoryImplTest.kt:openMessage returns without awaiting the background SEEN-flag push— parks the mockedsetFlagcall on an un-completedCompletableDeferredand assertsopenMessage()stillcompletes successfully (in ~24ms), then confirms the push is still dispatched independently.
Uses
runBlocking+CompletableDeferred(the same idiom as the existingMailMaintenanceGateTest) rather thanrunTest, since the property under test is genuineconcurrency between the caller and the repository's own background scope.
a failed SEEN-flag push is retried in the background— asserts a second attempt is observedafter the first failure, proving this is a retry loop and not a single silently-swallowed
attempt.
Local gate (JDK 21):
assembleDebug+testDebugUnitTest+lintDebug+ktlintCheck+detektall green.Closes #148
🤖 Generated with Claude Code