fix(popup): use fresh broadcast timing for connection popup

This commit is contained in:
darken
2026-04-14 11:28:41 +02:00
committed by Matthias Urhahn
parent a4e7820e6f
commit 46fcf5a4cb
2 changed files with 25 additions and 11 deletions
@@ -166,16 +166,18 @@ class PopUpReaction @Inject constructor(
return@mapNotNull null return@mapNotNull null
} }
val deviceSeenFirst = currentBroadcasted.seenFirstAt ?: return@mapNotNull null // Use the most recent BLE advertisement, not the first time this device was ever seen.
// Connection popups are meant to verify that a connection coincides with a fresh broadcast.
val broadcastSeenLast = currentBroadcasted.ble?.seenLastAt ?: return@mapNotNull null
val now = timeSource.now() val now = timeSource.now()
val deviceAge = Duration.between(deviceSeenFirst, now) val broadcastAge = Duration.between(broadcastSeenLast, now)
val connectionAge = Duration.between(currentConnected.seenFirstAt, now) val connectionAge = Duration.between(currentConnected.seenFirstAt, now)
val decision = evaluateConnectionPopUp( val decision = evaluateConnectionPopUp(
hasConnectedDevice = true, hasConnectedDevice = true,
hasPodDevice = true, hasPodDevice = true,
hasAlreadyShown = connectionCoolDowns.containsKey(currentConnected.address), hasAlreadyShown = connectionCoolDowns.containsKey(currentConnected.address),
deviceAge = deviceAge, broadcastAge = broadcastAge,
connectionAge = connectionAge, connectionAge = connectionAge,
) )
@@ -264,8 +266,9 @@ class PopUpReaction @Inject constructor(
hasConnectedDevice: Boolean, hasConnectedDevice: Boolean,
hasPodDevice: Boolean, hasPodDevice: Boolean,
hasAlreadyShown: Boolean, hasAlreadyShown: Boolean,
deviceAge: Duration, broadcastAge: Duration,
connectionAge: Duration, connectionAge: Duration,
maxConnectionAge: Duration = Duration.ofSeconds(30),
maxAgeDiff: Duration = Duration.ofSeconds(30), maxAgeDiff: Duration = Duration.ofSeconds(30),
): ConnectionPopUpDecision { ): ConnectionPopUpDecision {
if (!hasConnectedDevice) { if (!hasConnectedDevice) {
@@ -274,7 +277,10 @@ class PopUpReaction @Inject constructor(
if (!hasPodDevice) { if (!hasPodDevice) {
return ConnectionPopUpDecision(false, "No pod device found") return ConnectionPopUpDecision(false, "No pod device found")
} }
if (deviceAge.abs() > (connectionAge.abs() + maxAgeDiff)) { if (connectionAge.abs() > maxConnectionAge) {
return ConnectionPopUpDecision(false, "Connection is no longer new")
}
if (broadcastAge.abs() > (connectionAge.abs() + maxAgeDiff)) {
return ConnectionPopUpDecision(false, "Broadcast too old, likely false positive") return ConnectionPopUpDecision(false, "Broadcast too old, likely false positive")
} }
if (hasAlreadyShown) { if (hasAlreadyShown) {
@@ -117,13 +117,13 @@ class PopUpReactionLogicTest : BaseTest() {
hasConnectedDevice: Boolean = true, hasConnectedDevice: Boolean = true,
hasPodDevice: Boolean = true, hasPodDevice: Boolean = true,
hasAlreadyShown: Boolean = false, hasAlreadyShown: Boolean = false,
deviceAge: Duration = Duration.ofSeconds(5), broadcastAge: Duration = Duration.ofSeconds(5),
connectionAge: Duration = Duration.ofSeconds(5), connectionAge: Duration = Duration.ofSeconds(5),
) = popUpReaction.evaluateConnectionPopUp( ) = popUpReaction.evaluateConnectionPopUp(
hasConnectedDevice = hasConnectedDevice, hasConnectedDevice = hasConnectedDevice,
hasPodDevice = hasPodDevice, hasPodDevice = hasPodDevice,
hasAlreadyShown = hasAlreadyShown, hasAlreadyShown = hasAlreadyShown,
deviceAge = deviceAge, broadcastAge = broadcastAge,
connectionAge = connectionAge, connectionAge = connectionAge,
) )
@@ -142,6 +142,14 @@ class PopUpReactionLogicTest : BaseTest() {
evaluate(hasPodDevice = false).shouldShow shouldBe false evaluate(hasPodDevice = false).shouldShow shouldBe false
} }
@Test
fun `connected long ago - should NOT show`() {
evaluate(
broadcastAge = Duration.ofSeconds(5),
connectionAge = Duration.ofSeconds(60),
).shouldShow shouldBe false
}
@Test @Test
fun `already shown for this connection - should NOT show`() { fun `already shown for this connection - should NOT show`() {
evaluate(hasAlreadyShown = true).shouldShow shouldBe false evaluate(hasAlreadyShown = true).shouldShow shouldBe false
@@ -150,7 +158,7 @@ class PopUpReactionLogicTest : BaseTest() {
@Test @Test
fun `broadcast device age much older than connection (false positive) - should NOT show`() { fun `broadcast device age much older than connection (false positive) - should NOT show`() {
evaluate( evaluate(
deviceAge = Duration.ofSeconds(60), broadcastAge = Duration.ofSeconds(60),
connectionAge = Duration.ofSeconds(5), connectionAge = Duration.ofSeconds(5),
).shouldShow shouldBe false ).shouldShow shouldBe false
} }
@@ -158,7 +166,7 @@ class PopUpReactionLogicTest : BaseTest() {
@Test @Test
fun `broadcast device age within threshold of connection - should show`() { fun `broadcast device age within threshold of connection - should show`() {
evaluate( evaluate(
deviceAge = Duration.ofSeconds(20), broadcastAge = Duration.ofSeconds(20),
connectionAge = Duration.ofSeconds(5), connectionAge = Duration.ofSeconds(5),
).shouldShow shouldBe true ).shouldShow shouldBe true
} }
@@ -166,7 +174,7 @@ class PopUpReactionLogicTest : BaseTest() {
@Test @Test
fun `broadcast device age exactly at threshold - should NOT show`() { fun `broadcast device age exactly at threshold - should NOT show`() {
evaluate( evaluate(
deviceAge = Duration.ofSeconds(36), broadcastAge = Duration.ofSeconds(36),
connectionAge = Duration.ofSeconds(5), connectionAge = Duration.ofSeconds(5),
).shouldShow shouldBe false ).shouldShow shouldBe false
} }
@@ -176,7 +184,7 @@ class PopUpReactionLogicTest : BaseTest() {
// Duration.between(now, pastTimestamp) produces negative durations. // Duration.between(now, pastTimestamp) produces negative durations.
// The false-positive filter must work regardless of sign. // The false-positive filter must work regardless of sign.
evaluate( evaluate(
deviceAge = Duration.ofSeconds(-60), broadcastAge = Duration.ofSeconds(-60),
connectionAge = Duration.ofSeconds(-5), connectionAge = Duration.ofSeconds(-5),
).shouldShow shouldBe false ).shouldShow shouldBe false
} }