The key-invalidation recovery restart was unreliable in two ways, both in
AppLockViewModel:
1. Same-process self-restart race: restartProcess() did
context.startActivity(...) immediately followed by Runtime.exit(0) in the
same process, so ActivityManager could schedule the relaunch into the
process being killed and drop it — the app just closed, recovering only on
the next manual launch. Fixed with a ProcessPhoenix-style separate-process
trampoline (RestartActivity in a distinct ":restart" process, driven by
ProcessRestarter): it kills the original process by PID and only then
relaunches, so the relaunch is issued from a process that survives the kill.
No new dependency; LibreMailApplication early-returns in the ":restart"
process so it runs no normal startup work.
2. Lost syncNow() enqueue: clearCacheAndRestart() enqueued the post-wipe
re-sync fire-and-forget, but WorkManager persists the WorkSpec
asynchronously on its serial task executor, so exiting raced that insert and
could drop the re-sync (now user-visible after #118: an empty mailbox until
the next periodic sync). syncNow() now returns its enqueue Operation, and
clearCacheAndRestart awaits it (bounded by a 5s timeout) before restarting,
so the WorkSpec is durably persisted first.
CLEAR_PENDING recovery-flag semantics are preserved; the cache wipe still
happens at cold start in DatabaseModule (unchanged).
Tests: JVM unit tests assert the enqueue Operation is awaited before the
restart is triggered (order) and that a timed-out enqueue still restarts;
SyncSchedulerTest pins syncNow() returning the enqueue Operation. The
separate-process kill/relaunch is device-only and noted for on-device
wipe+resync verification.
Closes#99
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DatabaseModule.provideDatabase ran the whole startup sequence with
runBlocking while Hilt constructed the singleton database — a DataStore
read, a Keystore op, a possible SQLCipher re-key conversion, and (since
#111) the cross-database AccountDataMigrator — synchronously on whichever
thread first injected it, which can be the main thread (jank / ANR).
Move that work behind DatabaseProvisioner.prepareCache(): a memoized,
mutex-guarded suspend that runs the same sequence, in the same order, on
the IO dispatcher. Both databases' Room builders now open through a
DeferredOpenHelperFactory whose delegate — and therefore the gate — is
materialised only when Room first OPENS the database, on its background
query executor, never at inject time. AccountDatabase's open gates on the
same prepareCache(), preserving the #111 migrate-before-open ordering that
the old construction-time dependency on LibreMailDatabase enforced.
Behaviour, ordering, and crash-safety are unchanged — only where and when
the work runs moved off the (possibly main) inject thread.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A message with several attachments used to render every AttachmentRow
stacked vertically, pushing the message body arbitrarily far down. Now
only the first attachment shows by default; when there is more than one,
the extras collapse behind a "See x more attachments" control that
expands and collapses with an animated, rotating chevron. A single
attachment renders exactly as before (no accordion).
The count uses a plurals resource (quantity one/other) so it reads
"See 1 more attachment" / "See 2 more attachments" correctly. The toggle
is one clickable Role.Button whose label and chevron contentDescription
expose the expanded state to screen readers. Download/open behavior of
each row is unchanged.
Refs #134
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Make BackupPolicy.EXCLUDED_DATABASE_PATHS the true single source of truth
by deriving it from DatabaseFiles.NAME and DatabaseFiles.ACCOUNTS_NAME plus
their SQLite sidecars via a new DatabaseFiles.fileNames() helper, instead of
a hand-maintained list. This adds libremail-accounts.db (accounts + encrypted
credentials, split into their own DB by #118/#111) to the never-back-up set,
matching the field's stated intent, so a newly added database can never
silently fall out of the exclusions again.
Also fix DatabaseFiles.clear to wipe the cache DB via
context.deleteDatabase(NAME), which additionally removes the -mj*
master-journal temp files the hand-rolled suffix list missed. It still wipes
ONLY the cache DB (NAME) and never the accounts DB (ACCOUNTS_NAME), preserving
the sign-in-survives-cache-wipe separation from #111.
Update the backup XML comments (data_extraction_rules.xml, backup_rules.xml)
to note libremail-accounts.db is also kept off-device by the strict include-
allowlist, and extend the tests to assert the accounts DB is covered by the
exclusion SoT and that the derivation stays in lockstep with the XML resources.
There is no active backup leak today: the XML is a strict include-allowlist,
so the accounts DB was already excluded by omission. This closes the SoT drift
#118 introduced and the -mj* gap, so the security posture no longer depends on
the allowlist staying strict by luck.
Closes#103
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Recipient autocomplete's READ_CONTACTS permission was requested lazily on
every compose-screen open (a LaunchedEffect(Unit)), re-prompting users who
had declined. Move the request to a dedicated, skippable onboarding step and
add a Settings entry to turn it on later, each with an in-context rationale.
- #127: new skippable ONBOARDING_CONTACTS step (mirrors the battery step),
requested once. ComposeScreen no longer prompts; it only reads the current
grant on resume, so a grant made later (e.g. from Settings) still takes
effect the next time compose opens.
- #128: the onboarding step and the Settings request show a short rationale
(contacts are used only for on-device autocomplete, never uploaded) and
handle shouldShowRequestPermissionRationale so a re-request explains itself.
docs/play-permissions.md updated to match.
- #129: Settings -> Contacts -> Recipient autocomplete reflects on / off /
blocked-in-settings; requests in-app when grantable, deep-links to the app's
system settings when permanently denied.
Graceful degradation is preserved: ContactsRepository.search still runCatch-es,
ComposeViewModel.searchContacts() still guards on contactsAllowed, and the
suggestion list still renders only when non-empty.
Adds a pure ContactPermissionDecision (JVM unit-tested), extends the onboarding
view-model tests, and adds Compose UI tests for the onboarding step
(skip / grant / deny / rationale) and the Settings row states.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The unified inbox query (WHERE folder = ?, no accountId) has no folder-leading
index, so it scans in timestamp order and materializes the whole unified inbox
(~4k rows at a 20k cache) into memory on every emission. Apply Paging 3 to the
unified browse path so query, mapping, and recomposition cost scale with the
visible window, not the total cache.
- MessageDao.pagingUnifiedFolderSummaries: a PagingSource over the folder's
synced rows (inInbox = 1); unified search keeps the whole-folder query so it
can still surface transient server-search hits.
- MailRepository.pagedUnifiedFolderMessages: a Pager (pageSize 40, initialLoad
120, no placeholders) mapping summaries to domain.
- MailboxViewModel.pagedMessages: paged while browsing the unified inbox, else
empty; the messages list flow stays empty in that state so the whole cache is
never materialized. Selection captures each row's accountId at tap time, so
"Move" still resolves the selection's account without an in-memory list.
- MailboxScreen renders the unified browse list via collectAsLazyPagingItems;
per-account and search views render the flat list unchanged (issue #86 stays
flat).
Profiling (docs/perf/issue-124-unified-inbox-paging.md) on an api29 emulator:
current whole-inbox first-emit ~24.6 ms at a 20k cache vs. the paged first page
~6.8 ms and flat regardless of cache size (~3.6x). EXPLAIN QUERY PLAN shows the
paged query still stops early on the existing timestamp index, so no
(folder, ...) index and no schema migration are added.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Device upgrade testing surfaced a crash: on a cache last written before
v13, account_settings has 4 columns (accountId, signature,
signatureEnabled, notificationsEnabled) but the destination table has 6
(retentionCount/retentionMonths were added at v13). The migrator ran
`INSERT OR IGNORE INTO account_settings SELECT * FROM cache...`, which
supplied 4 values for 6 columns and threw SQLiteException — and because
the done-flag is only set after a successful copy, every launch re-ran
and re-crashed (crash loop).
AccountDataMigrator now copies each table by the column names present in
BOTH the freshly-created destination and the (possibly older) source, so
columns the source lacks take the destination's defaults instead of
overflowing the value list. Verified on-device: the upgrade migrates a
pre-v13 install cleanly and the account stays signed in (sync/backfill
workers run). Regression test seeds a v12 cache and asserts the copy.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Main advanced to @Database v15 (the #66 folder hierarchyDelimiter
migration). Renumbered the account-tables-drop migration 14->15 to
15->16 and bumped the cache DB to v16; this exports the v16 schema
(main's v15 delimiter schema minus the moved account tables). Main's
own 15.json is kept unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two on-device assertion failures (green on JVM compile, red on the
emulator):
- migratorDdlMatchesExportedAccountDatabaseSchema built its expected DDL
by substituting the schema's `${TABLE_NAME}` placeholder with a
backtick-wrapped name, but the exported createSql already wraps the
placeholder in backticks — producing a double-backticked identifier
that never matched the (correct, single-backticked) migrator DDL.
Substitute the bare name so the guard compares like-for-like.
- movesEveryAccountTableOutOfAPlaintextCache asserted signatureEnabled
was false, but the seed row sets it to 1 (true). Assert the seeded
values for both booleans so a true and a false each round-trip.
The production migrator DDL and drop logic were already correct; only
the tests were wrong.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`@Test fun x() = runBlocking { ... }` whose block ends in `.apply { }`
returns the DB (non-Unit), so JUnit4 rejects the whole class at runtime
with InvalidTestClassError ("method should be void") — which compiles
fine locally but fails every E2E job on the emulator. Use
`runBlocking<Unit>` (the existing DatabaseEncryptionTest idiom) so the
methods are void while keeping the expression body ktlint expects.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Accounts, credentials, per-account settings and signatures lived in the
same libremail.db that SQLCipher encrypts under the auth-bound passphrase
when app-lock + encrypted-cache are on. A genuine key invalidation
(biometric re-enrollment or lock removal/re-add) made that file
undecryptable, and the "clear + re-sync" recovery wiped the accounts and
stored credentials along with the mail cache, dropping the user into
onboarding (issue #111).
Move those four tables into a new plaintext AccountDatabase
(libremail-accounts.db) that is never bound to the auth key. Credentials
stay AES-GCM sealed at the column level by the surviving non-auth
KeystoreCrypto master key, so the only secret never touches disk in the
clear. A cache-key invalidation now wipes only libremail.db; the user
stays signed in.
- AccountDatabase (v1) + AccountDatabaseModule; the cache DB drops to v15
via MIGRATION_14_15. DAOs are unchanged and re-provided from the new DB,
so no injection site changes.
- AccountDataMigrator performs the one-time cross-DB copy at startup,
before Room opens either database. It attaches the cache (with its
resolved passphrase, so an encrypted source is handled) and copies with
INSERT OR IGNORE. It is crash-safe and idempotent: the source is dropped
only by MIGRATION_14_15 after the copy, a re-run never duplicates or
overwrites, and it runs after the clear-pending wipe so an unrecoverable
cache degrades to "nothing to move" instead of blocking.
- Exported schemas for both databases; MigrationTest asserts the account
rows/backfills survive to v14 then are dropped at v15, plus a dedicated
14->15 test. AccountDataMigratorTest covers the plaintext + encrypted
copy, idempotency, a DDL-vs-Room drift guard, and end-to-end survival of
a simulated cache wipe.
Closes#111
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Investigate IMAP folder-open latency (follow-up to #86). Localhost GreenMail
has ~0 RTT, so real wall-clock latency can't be measured here; instead this
pins the folder-open round-trip STRUCTURE deterministically.
Finding: ImapClient.withStore wraps every operation in its own short-lived
Store, so each folder-open pays a full CONNECT + TLS + LOGIN + EXAMINE +
FETCH + LOGOUT. Only EXAMINE + FETCH is intrinsic to opening a folder; the
whole connection-setup group is avoidable on the 2nd+ operation if a
connection were reused. Optimistic render-from-cache already exists
(selectFolder renders cached rows; the network sync is a background refresh).
Adds:
- CountingImapProxy: a localhost TCP proxy that forwards a cleartext IMAP
session to GreenMail while counting TCP connections and parsing IMAP
command words.
- ImapFolderOpenLatencyTest: asserts the current no-reuse behaviour (N opens
=> N connections and N LOGINs; list+read => 2 connections) against a real
in-process IMAP server. Doubles as the harness to validate a future
connection-reuse fix (flip the counts to assert reuse).
- docs/perf/issue-125-imap-folder-open.md: the per-open round-trip sequence,
avoidable vs. necessary round-trips, and the recommended per-account
connection-reuse/keep-alive mitigation with its IDLE / thread-safety /
battery / stale-connection constraints.
Analysis + harness only; the connection-reuse fix is deferred pending
real-network + real-device measurement (see the doc's measurement plan), so
this references #125 without closing it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The mailbox list observed the entire `messages` table (observeSummaries, no
WHERE/LIMIT), mapped every cached row to a domain Message, and filtered down to
the visible account+folder in MailboxViewModel — so its cost scaled with the
whole cache and re-ran on every write to `messages` (IDLE delivery, a flag
toggle, a backfill page, any folder sync). On a 20k-row cache that is ~125 ms of
work per unrelated write.
Push the account/folder filter into SQL (observeFolderSummaries /
observeUnifiedFolderSummaries, exposed via observeFolderMessages /
observeUnifiedFolderMessages) and flatMapLatest the ViewModel over the selected
account+folder. The only remaining client-side pass separates the normal list
from an active search over the small folder-scoped set.
Validated on an emulator against 1k/5k/20k-row caches (docs/perf/issue-86-
profiling.md): the account-scoped query is ~1.5 ms flat (~80x faster at 20k) and
is already served by the existing (accountId, folder, uid) index — so NO
composite index and NO schema migration are added. The ticket's proposed
(accountId, folder, inInbox, timestampMillis) index changes timing only within
noise and isn't even preferred by SQLite's planner. The unified "All inboxes"
view stays an O(N) folder scan (still 5.6x better) and is a follow-up for paging;
IMAP latency on folder open is a separate, unmeasured concern.
Closes#86
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
parentOf() re-inferred the IMAP hierarchy separator from each folder's name,
relying on an unenforced invariant (displayName == fullName.substringAfterLast(
separator)) established three layers from where ImapClient reads the
authoritative JavaMail folder.separator and then discards it.
Carry that separator through FetchedFolder -> FolderEntity -> Folder and split a
folder's parent on it. Fall back to the old name inference only for legacy rows
whose delimiter is null, until the next folder refresh (delete-then-insert)
backfills the real value.
Adds a nullable folders.hierarchyDelimiter column via a Room v14 -> v15 migration
with the exported v15 schema, and registers MIGRATION_14_15 in DatabaseModule so
existing v14 installs actually upgrade (provideDatabase configures no destructive
fallback, so an unregistered migration would crash every upgrading user).
Tests: JVM unit tests for parentOf() (persisted vs. null delimiter, incl. the
case where inference cannot locate the parent) and the FetchedFolder->entity
round-trip; an instrumented v14->v15 migration test plus the chain-replay
assertion.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Behavior-preserving cleanup of the app-lock UI plumbing:
- SettingsScreen: replace the app's only Toast with the canonical
SnackbarHostState + Scaffold(snackbarHost) + consume pattern for the
app-lock rejection message (matches MailboxScreen); the ViewModel keeps
the @StringRes id, resolved via LocalResources at the display boundary.
- AppLockGateHost: replace the hand-rolled DisposableEffect +
LifecycleEventObserver with LifecycleEventEffect, and the ContextWrapper
findFragmentActivity() walk with LocalActivity; remember the derived
activity and the authenticate lambda.
- AppLockManager: delete the dead availability() API and the four-value
AppLockAvailability enum (no production caller; the
BIOMETRIC_STRONG or DEVICE_CREDENTIAL canAuthenticate combo is
unsupported on minSdk 29). Keep isDeviceSecure() and AUTHENTICATORS.
- AppLockViewModel: derive the gated uiState from the injected gate via a
single publish() helper instead of hand-mirroring gate.state at each
auth site; the transient Checking cover and app-lock-off unlocked states
stay explicit (settings/lifecycle-driven, not session-gate-driven).
Extend SettingsScreenTest with a Compose test for the rejection snackbar
(now visible to Compose semantics) and drop availability() from its fake.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three code-quality cleanups from the PR #54 review, all in the folder-label
plumbing so they ship as one change (adapted to the post-#108/#117 code):
#69 providerLabel: consolidate provider-brand host matching. Host->brand
knowledge now lives solely in MailProvider: forImapHost matches an entry's
imapHost plus new hostAliases (Gmail gains legacy imap.googlemail.com), and a
new companion brandFor(account) is the single seam that also recognizes
Outlook (by OAuth auth type or a precise office365.com / outlook.office.com
host, not any substring). MailProvider stays the app-password preset registry
(Outlook is not an entry). providerLabel() drops its ad-hoc host substrings.
#68 i18n: move folder-label disambiguation patterns into strings.xml. The
"base - provider", "base (parent)", and "base [path]" grammars become
folder_label_with_provider/parent/path resources, threaded into the pure
resolver as a LabelPatterns bundle whose defaults match the old literals; the
composable resolves the localized strings and passes them down.
#67 FolderDrawer: memoize label resolution, resolver early-return, fail-fast
lookups. resolvedFolderLabels hoists the role->string and pattern lookups out
of a remember() so the resolved map is rebuilt only when folders/accounts/
strings change (not every recomposition of the idle drawer). resolveDrawerLabels
returns baseLabels unchanged when nothing collides, and both map lookups use
getValue so a key miss fails loudly instead of silently un-deduplicating.
Behavior is unchanged: existing FolderLabelsTest and FolderDrawerTest
assertions (from #60/#61/#64/#108) stay green. Adds unit tests for host->brand
matching and its over-match guard, pattern-driven formatting, the early-return
identity, and fail-fast on a missing base label.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Jason Ross
JMR-devandClaude Fable 5