Addresses six of the seven below-cut LOW review nits collected in #308; the seventh is deliberately skipped with a rationale (below). Each behavioral change ships a JVM/Robolectric test; PII-free AppLog breadcrumbs were added on the new fallback / state-change paths.
Per-nit outcome
#
Nit
Outcome
1
SettingsViewModel app-lock disable runs Keystore sealWithMaster() on Main
Fixed — reseal now runs in withContext(defaultDispatcher) (default Dispatchers.Default), mirroring AppLockViewModel's threading policy. Behavior-preserving.
2
AppLockGateHost leaves app content in the semantics tree behind the lock cover (TalkBack can traverse occluded nodes)
Fixed — the covered subtree is clearAndSetSemantics {}'d while Checking/Locked (the repo's established a11y-gating idiom). Content stays composed so its state still survives a re-lock.
3
ReaderViewModel.toggleStar optimistic update never reconciled on failure
Fixed — on a failed setStarred, the optimistic flip is rolled back and a one-shot ReaderEvent.StarFailed is surfaced (snackbar).
4
OnboardingViewModel.firstAddedAccountId not in SavedStateHandle → process-kill lands on unfiltered mailbox
Fixed — stored in SavedStateHandle (nav-graph entry state survives process death), preserving #30.
5
RichTextEditor toolbar re-parses whole body + ~10 scans per keystroke
Fixed — the parse + all selection scans are derived once via remember(value) into a ToolbarState. Pure refactor (identical values).
6
AccountSetupViewModel surfaces a normal OAuth cancel as an error snackbar
Fixed — a null result (RESULT_CANCELED) is now a no-op (breadcrumb only), not an error.
7
MailboxViewModel search re-pages the local list per keystroke (documented intentional)
The finding is explicitly "documented intentional" and phrased as a "could". The current design is deliberate and good: the local Room pager narrows already-cached results instantly as the user types (responsive), while the expensive server search is already debounced at 400 ms (SEARCH_DEBOUNCE_MS). Debouncing the local pager would add typing lag to local narrowing for no clear benefit, and doing it correctly (debouncing only the query dimension, not account/folder switches) adds real complexity/risk to a hot, well-tested cachedIn path also driven by the keep-alive presenter (#219). Per the perf drilldown, the mailbox's real cost is server-side IMAP throttling, which is already debounced. Disproportionate to apply blind.
Tests
SettingsViewModelTest — new test proves the disable-path reseal is dispatched off-main; existing branch tests pin behavior (dispatcher pinned to the scheduler).
AppLockGateHostJvmTest (Robolectric Compose) — rewrote the re-lock test to prove content stays composed (DisposableEffect counter) and is gated out of the a11y tree while locked, then re-displayed on unlock.
ReaderViewModelActionsTest — new rollback + StarFailed test.
OnboardingViewModelTest — new process-death survival test (restored SavedStateHandle).
RichTextEditorTest — new toolbarStateOf one-pass derivation test.
AccountSetupViewModelTest — cancel test updated to assert the no-op.
Gate
Local fast gate green: assembleDebug, testDebugUnitTest (1452 tests), jacocoTestCoverageVerification (floor 0.84), compileDebugAndroidTestKotlin, lintDebug, ktlintCheck, detekt. E2E via CI matrix.
Closes #308.
Addresses six of the seven below-cut LOW review nits collected in #308; the seventh is deliberately skipped with a rationale (below). Each behavioral change ships a JVM/Robolectric test; PII-free `AppLog` breadcrumbs were added on the new fallback / state-change paths.
## Per-nit outcome
| # | Nit | Outcome |
|---|-----|---------|
| 1 | `SettingsViewModel` app-lock disable runs Keystore `sealWithMaster()` on Main | **Fixed** — reseal now runs in `withContext(defaultDispatcher)` (default `Dispatchers.Default`), mirroring `AppLockViewModel`'s threading policy. Behavior-preserving. |
| 2 | `AppLockGateHost` leaves app content in the semantics tree behind the lock cover (TalkBack can traverse occluded nodes) | **Fixed** — the covered subtree is `clearAndSetSemantics {}`'d while Checking/Locked (the repo's established a11y-gating idiom). Content stays composed so its state still survives a re-lock. |
| 3 | `ReaderViewModel.toggleStar` optimistic update never reconciled on failure | **Fixed** — on a failed `setStarred`, the optimistic flip is rolled back and a one-shot `ReaderEvent.StarFailed` is surfaced (snackbar). |
| 4 | `OnboardingViewModel.firstAddedAccountId` not in `SavedStateHandle` → process-kill lands on unfiltered mailbox | **Fixed** — stored in `SavedStateHandle` (nav-graph entry state survives process death), preserving #30. |
| 5 | `RichTextEditor` toolbar re-parses whole body + ~10 scans per keystroke | **Fixed** — the parse + all selection scans are derived once via `remember(value)` into a `ToolbarState`. Pure refactor (identical values). |
| 6 | `AccountSetupViewModel` surfaces a normal OAuth cancel as an error snackbar | **Fixed** — a null result (RESULT_CANCELED) is now a no-op (breadcrumb only), not an error. |
| 7 | `MailboxViewModel` search re-pages the local list per keystroke (documented intentional) | **Skipped** — see below. |
### Why #7 is skipped
The finding is explicitly *"documented intentional"* and phrased as a *"could"*. The current design is deliberate and good: the local Room pager narrows already-cached results **instantly** as the user types (responsive), while the expensive **server** search is already debounced at 400 ms (`SEARCH_DEBOUNCE_MS`). Debouncing the local pager would add typing lag to local narrowing for no clear benefit, and doing it correctly (debouncing only the query dimension, not account/folder switches) adds real complexity/risk to a hot, well-tested `cachedIn` path also driven by the keep-alive presenter (#219). Per the perf drilldown, the mailbox's real cost is server-side IMAP throttling, which is already debounced. Disproportionate to apply blind.
## Tests
- `SettingsViewModelTest` — new test proves the disable-path reseal is dispatched off-main; existing branch tests pin behavior (dispatcher pinned to the scheduler).
- `AppLockGateHostJvmTest` (Robolectric Compose) — rewrote the re-lock test to prove content stays composed (DisposableEffect counter) **and** is gated out of the a11y tree while locked, then re-displayed on unlock.
- `ReaderViewModelActionsTest` — new rollback + `StarFailed` test.
- `OnboardingViewModelTest` — new process-death survival test (restored `SavedStateHandle`).
- `RichTextEditorTest` — new `toolbarStateOf` one-pass derivation test.
- `AccountSetupViewModelTest` — cancel test updated to assert the no-op.
## Gate
Local fast gate green: `assembleDebug`, `testDebugUnitTest` (1452 tests), `jacocoTestCoverageVerification` (floor 0.84), `compileDebugAndroidTestKotlin`, `lintDebug`, `ktlintCheck`, `detekt`. E2E via CI matrix.
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.
Closes #308.
Addresses six of the seven below-cut LOW review nits collected in #308; the seventh is deliberately skipped with a rationale (below). Each behavioral change ships a JVM/Robolectric test; PII-free
AppLogbreadcrumbs were added on the new fallback / state-change paths.Per-nit outcome
SettingsViewModelapp-lock disable runs KeystoresealWithMaster()on MainwithContext(defaultDispatcher)(defaultDispatchers.Default), mirroringAppLockViewModel's threading policy. Behavior-preserving.AppLockGateHostleaves app content in the semantics tree behind the lock cover (TalkBack can traverse occluded nodes)clearAndSetSemantics {}'d while Checking/Locked (the repo's established a11y-gating idiom). Content stays composed so its state still survives a re-lock.ReaderViewModel.toggleStaroptimistic update never reconciled on failuresetStarred, the optimistic flip is rolled back and a one-shotReaderEvent.StarFailedis surfaced (snackbar).OnboardingViewModel.firstAddedAccountIdnot inSavedStateHandle→ process-kill lands on unfiltered mailboxSavedStateHandle(nav-graph entry state survives process death), preserving #30.RichTextEditortoolbar re-parses whole body + ~10 scans per keystrokeremember(value)into aToolbarState. Pure refactor (identical values).AccountSetupViewModelsurfaces a normal OAuth cancel as an error snackbarMailboxViewModelsearch re-pages the local list per keystroke (documented intentional)Why #7 is skipped
The finding is explicitly "documented intentional" and phrased as a "could". The current design is deliberate and good: the local Room pager narrows already-cached results instantly as the user types (responsive), while the expensive server search is already debounced at 400 ms (
SEARCH_DEBOUNCE_MS). Debouncing the local pager would add typing lag to local narrowing for no clear benefit, and doing it correctly (debouncing only the query dimension, not account/folder switches) adds real complexity/risk to a hot, well-testedcachedInpath also driven by the keep-alive presenter (#219). Per the perf drilldown, the mailbox's real cost is server-side IMAP throttling, which is already debounced. Disproportionate to apply blind.Tests
SettingsViewModelTest— new test proves the disable-path reseal is dispatched off-main; existing branch tests pin behavior (dispatcher pinned to the scheduler).AppLockGateHostJvmTest(Robolectric Compose) — rewrote the re-lock test to prove content stays composed (DisposableEffect counter) and is gated out of the a11y tree while locked, then re-displayed on unlock.ReaderViewModelActionsTest— new rollback +StarFailedtest.OnboardingViewModelTest— new process-death survival test (restoredSavedStateHandle).RichTextEditorTest— newtoolbarStateOfone-pass derivation test.AccountSetupViewModelTest— cancel test updated to assert the no-op.Gate
Local fast gate green:
assembleDebug,testDebugUnitTest(1452 tests),jacocoTestCoverageVerification(floor 0.84),compileDebugAndroidTestKotlin,lintDebug,ktlintCheck,detekt. E2E via CI matrix.Merge Queue Status
2026-07-08 16:12 UTC· Rule:default· triggered by merge protections2026-07-08 17:24 UTC· at91f2f7105b31016394ece5c0434a3dcbf73bcc87· mergeThis pull request spent 1 hour 12 minutes 20 seconds in the queue, including 18 minutes 59 seconds running CI.
Required conditions to merge
-conflict-draftbase = maincheck-success = CI passedgithub-review-approved[🛡 GitHub repository ruleset rulemain]label != brokencheck-success = Debug buildcheck-neutral = Debug buildcheck-skipped = Debug buildcheck-success = Unit testscheck-neutral = Unit testscheck-skipped = Unit testscheck-success = CI passedcheck-neutral = CI passedcheck-skipped = CI passedmain]:check-success = @github-actions/CI passedcheck-neutral = @github-actions/CI passedcheck-skipped = @github-actions/CI passed