fix(overview): Keep the review prompt from crashing or stacking

Corrupt review settings terminated the shared state flow on AppScope,
so the exception crashed the process instead of reaching the ViewModel's
catch. Absorb it upstream of both replayingShare calls.

The review card also no longer stacks on top of the enable-Bluetooth
prompt.

Fixes review findings F1, F2
This commit is contained in:
darken
2026-08-05 16:27:13 +02:00
committed by Matthias Urhahn
parent 7716221053
commit 1e1f0b13c6
4 changed files with 55 additions and 3 deletions
@@ -20,6 +20,7 @@ import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.catch
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.flatMapLatest
@@ -65,6 +66,13 @@ class GplayReviewTool @Inject constructor(
hasPaidForPro && !isSnoozed && !hasReviewed
}
.distinctUntilChanged()
// Upstream of the shares below: an exception there would kill the sharing coroutine on
// AppScope (crashing the process) instead of reaching any downstream `catch`.
.catch { e ->
if (e is CancellationException) throw e
log(TAG, ERROR) { "Eligibility failed: ${e.asLog()}" }
emit(false)
}
// Only probed once the user is eligible: Play counts requests against the app's quota, and an
// `isNoOp` answer is Play's deliberate verdict, i.e. an answer and not a failure to retry.
@@ -107,6 +115,11 @@ class GplayReviewTool @Inject constructor(
}
.throttleLatest(500)
.onStart { emit(ReviewTool.State()) }
.catch { e ->
if (e is CancellationException) throw e
log(TAG, ERROR) { "State failed: ${e.asLog()}" }
emit(ReviewTool.State())
}
.replayingShare(appScope)
// Single-flight: a second tap must not queue up behind the first, or Play's flow would be
@@ -332,14 +332,15 @@ class OverviewViewModel @Inject constructor(
/**
* Whether the overview is currently showing a card that outranks the review prompt: a
* missing permission, the troubleshooter hint, the background-monitoring-off notice or the
* no-profiles setup card. All of those ask the user to do something, so the review prompt
* stays hidden while any of them is on screen.
* missing permission, the troubleshooter hint, the background-monitoring-off notice, the
* no-profiles setup card or the enable-Bluetooth prompt. All of those ask the user to do
* something, so the review prompt stays hidden while any of them is on screen.
*/
val hasHigherPriorityCard: Boolean
get() = permissions.isNotEmpty() ||
showTroubleshootSuggestion ||
monitoringStatus == MonitoringStatus.BACKGROUND_OFF ||
!isBluetoothEnabled ||
(profiles.isEmpty() && !isScanBlocked && isBluetoothEnabled)
val soleProfileId: ProfileId? get() = profiles.singleOrNull()?.id
@@ -927,6 +927,16 @@ class OverviewViewModelTest : BaseTest() {
vm.state.first().showReviewCard shouldBe false
}
@Test
fun `suppressed by the enable-Bluetooth prompt`() = runTest(testDispatcher) {
quietOverview()
isBluetoothEnabledFlow.value = false
val vm = createViewModel()
vm.state.first().showReviewCard shouldBe false
}
@Test
fun `suppressed by the troubleshooter suggestion`() = runTest(testDispatcher) {
quietOverview()
@@ -19,11 +19,13 @@ import io.mockk.verify
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.flow.drop
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.launch
import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.advanceUntilIdle
import kotlinx.coroutines.withTimeoutOrNull
import kotlinx.serialization.SerializationException
import org.junit.jupiter.api.Test
import testhelpers.BaseTest
import testhelpers.coroutine.runTest2
@@ -234,6 +236,32 @@ class GplayReviewToolTest : BaseTest() {
verify(exactly = 3) { manager.requestReviewFlow() }
}
@Test fun `corrupt review settings fall back to the default state`() = runTest2 {
every { manager.requestReviewFlow() } returns Tasks.forResult(reviewInfo())
lastDismissedMock = rwSetting(null)
// A malformed stored timestamp throws when the DataStore flow is read.
reviewedAtMock = mockk<DataStoreValue<Instant?>>(relaxed = true).apply {
every { flow } returns flow { throw SerializationException("corrupt") }
}
every { settings.lastDismissed } returns lastDismissedMock
every { settings.reviewedAt } returns reviewedAtMock
val upgradeInfo = mockk<UpgradeRepo.Info>()
every { upgradeInfo.upgradedAt } returns Instant.now().minus(Duration.ofDays(30))
every { upgradeRepo.upgradeInfo } returns flowOf(upgradeInfo)
val tool = GplayReviewTool(
appScope = backgroundScope,
settings = settings,
manager = manager,
upgradeRepo = upgradeRepo,
)
// The failure has to be absorbed upstream of the share: on the shared coroutine it would go
// to AppScope (no handler, i.e. a process crash) and never reach a downstream collector.
tool.computedState() shouldBe ReviewTool.State()
}
@Test fun `cancellation during reviewNow is not swallowed`() = runTest2 {
every { manager.requestReviewFlow() } throws CancellationException("scope died")
val tool = tool()