Conversation
|
Following from my comment #6459 (comment): I think this instance should be adjusted to avoid hitting 30s in the first place. WorkManager is for deferrable tasks - events like these should fire immediately. |
The issue is that even if we reduce the timeout, if I'm not wrong frireEvent might try the first Url fail and try the second one if there are any and even maybe a third one? Internal/cloud/external? |
TimoPtr
force-pushed
the
feature/notification_delete_worker
branch
from
eecb59e to
840b363
Compare
| val request = OneTimeWorkRequestBuilder<NotificationDeleteWorker>() | ||
| .setInputData(data) | ||
| // We want the event to be sent right away if it is possible | ||
| .setExpedited(OutOfQuotaPolicy.RUN_AS_NON_EXPEDITED_WORK_REQUEST) |
There was a problem hiding this comment.
Marks the WorkRequest as important to the user. In this case, WorkManager provides an additional signal to the OS that this work is important.
Note that although the execution time of this work won't be counted against your app's quota while your app is in the foreground, if the expedited work continues in the background, you are susceptible to quota. However, power management restrictions, such as Battery Saver and Doze, are less likely to affect expedited work. Because of this, expedited work is best suited for short tasks which need to start immediately and are important to the user or user-initiated.
There was a problem hiding this comment.
Pull request overview
This pull request migrates notification deletion handling from a long-lived BroadcastReceiver pattern to WorkManager by introducing NotificationDeleteWorker. The change addresses Android's recommendation that BroadcastReceivers should have short lifetimes (less than 30 seconds), moving the network call to fire the notification cleared event to a CoroutineWorker which is better suited for potentially long-running background tasks.
Changes:
- Introduced
NotificationDeleteWorkerto handle notification cleared events asynchronously via WorkManager - Refactored
NotificationDeleteReceiverto delegate work to the worker after handling immediate UI operations (canceling empty notification groups) - Changed data serialization from HashMap to separate key/value arrays to work with WorkManager's Data API
- Simplified
handleDeleteIntentinNotificationFunctions.ktto use the newcreateDeletePendingIntentmethod
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/NotificationDeleteWorker.kt |
New CoroutineWorker that fires notification cleared events to Home Assistant server using Hilt EntryPoint pattern for dependency injection |
common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/NotificationDeleteReceiver.kt |
Refactored to extract PendingIntent creation to a companion function and delegate event firing to NotificationDeleteWorker |
common/src/main/kotlin/io/homeassistant/companion/android/common/notifications/NotificationFunctions.kt |
Simplified to use the new createDeletePendingIntent helper method, removing direct PendingIntent creation |
common/src/test/kotlin/io/homeassistant/companion/android/common/notifications/NotificationDeleteWorkerTest.kt |
Comprehensive unit tests covering success cases, failure cases, null handling, and error scenarios |
| val notificationDao = entryPoints.notificationDao() | ||
|
|
||
| return try { | ||
| val eventData = keys.zip(values).toMap() |
There was a problem hiding this comment.
There's no validation that keys and values arrays have the same length before calling zip. If the arrays have different lengths, zip will silently truncate to the shorter array's length, potentially losing event data. Consider adding a check to return Result.failure() if the lengths don't match, or log a warning if this is intentional behavior.
| val notificationDao = entryPoints.notificationDao() | ||
|
|
||
| return try { | ||
| val eventData = keys.zip(values).toMap() |
There was a problem hiding this comment.
Type safety issue: getStringArray returns Array<String>? where elements can be null according to Android's API contract, even though the original data from Map.keys.toTypedArray() contains only non-null strings. When calling keys.zip(values).toMap(), null elements would create a Map<String?, String?>, but fireEvent expects Map<String, Any> with non-nullable keys and values. The code should filter out nulls before creating the map or use mapNotNull and filterNotNull to ensure type safety, for example: val eventData = keys.filterNotNull().zip(values.filterNotNull()).toMap().
| val eventData = keys.zip(values).toMap() | |
| val eventData = keys | |
| .zip(values) | |
| .mapNotNull { (key, value) -> | |
| if (key != null && value != null) { | |
| key to value | |
| } else { | |
| null | |
| } | |
| } | |
| .toMap() |
| class NotificationDeleteWorkerTest { | ||
|
|
||
| private val serverManager: ServerManager = mockk() | ||
| private val notificationDao: NotificationDao = mockk() | ||
| private val integrationRepository: IntegrationRepository = mockk(relaxed = true) | ||
| private val context: Context = mockk() | ||
| private val workerParams: WorkerParameters = mockk(relaxed = true) | ||
|
|
||
| @BeforeEach | ||
| fun setup() { | ||
| every { context.applicationContext } returns context | ||
| coEvery { serverManager.integrationRepository(any()) } returns integrationRepository | ||
|
|
||
| mockkStatic(EntryPoints::class) | ||
| every { | ||
| EntryPoints.get(any(), NotificationDeleteWorkerEntryPoint::class.java) | ||
| } returns mockk { | ||
| every { serverManager() } returns serverManager | ||
| every { notificationDao() } returns notificationDao | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `Given valid input when doWork then fire event and return success`() = runTest { | ||
| val eventData = mapOf("action" to "cleared", "tag" to "test-tag") | ||
| val databaseId = 42L | ||
| val serverId = 5 | ||
| setupWorkerInput(databaseId = databaseId, eventData = eventData) | ||
| coEvery { notificationDao.get(databaseId.toInt()) } returns notificationItem(serverId = serverId) | ||
|
|
||
| val worker = NotificationDeleteWorker(context, workerParams) | ||
| val result = worker.doWork() | ||
|
|
||
| assertEquals(ListenableWorker.Result.success(), result) | ||
| coVerify(exactly = 1) { | ||
| serverManager.integrationRepository(serverId) | ||
| integrationRepository.fireEvent("mobile_app_notification_cleared", eventData) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `Given notification not in database when doWork then use active server and return success`() = runTest { | ||
| val eventData = mapOf("action" to "cleared") | ||
| val databaseId = 99L | ||
| setupWorkerInput(databaseId = databaseId, eventData = eventData) | ||
| coEvery { notificationDao.get(databaseId.toInt()) } returns null | ||
|
|
||
| val worker = NotificationDeleteWorker(context, workerParams) | ||
| val result = worker.doWork() | ||
|
|
||
| assertEquals(ListenableWorker.Result.success(), result) | ||
| coVerify(exactly = 1) { | ||
| serverManager.integrationRepository(ServerManager.SERVER_ID_ACTIVE) | ||
| integrationRepository.fireEvent("mobile_app_notification_cleared", eventData) | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun `Given missing event data when doWork then return failure`() = runTest { | ||
| every { workerParams.inputData } returns Data.Builder() | ||
| .putLong("database_id", 1L) | ||
| .build() | ||
|
|
||
| val worker = NotificationDeleteWorker(context, workerParams) | ||
| val result = worker.doWork() | ||
|
|
||
| assertEquals(ListenableWorker.Result.failure(), result) | ||
| coVerify(exactly = 0) { integrationRepository.fireEvent(any(), any()) } | ||
| } | ||
|
|
||
| @Test | ||
| fun `Given server throws when doWork then return failure`() = runTest { | ||
| val eventData = mapOf("action" to "cleared") | ||
| val databaseId = 42L | ||
| setupWorkerInput(databaseId = databaseId, eventData = eventData) | ||
| coEvery { notificationDao.get(databaseId.toInt()) } returns notificationItem(serverId = 1) | ||
| coEvery { integrationRepository.fireEvent(any(), any()) } throws IllegalStateException("Server unavailable") | ||
|
|
||
| val worker = NotificationDeleteWorker(context, workerParams) | ||
| val result = worker.doWork() | ||
|
|
||
| assertEquals(ListenableWorker.Result.failure(), result) | ||
| } | ||
|
|
||
| private fun setupWorkerInput(databaseId: Long, eventData: Map<String, String>) { | ||
| every { workerParams.inputData } returns Data.Builder() | ||
| .putLong("database_id", databaseId) | ||
| .putStringArray("event_data_keys", eventData.keys.toTypedArray()) | ||
| .putStringArray("event_data_values", eventData.values.toTypedArray()) | ||
| .build() | ||
| } | ||
|
|
||
| private fun notificationItem(serverId: Int): NotificationItem = | ||
| NotificationItem( | ||
| id = 1, | ||
| received = 0L, | ||
| message = "test", | ||
| data = "{}", | ||
| source = "test", | ||
| serverId = serverId, | ||
| ) | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing test coverage for the case where the keys and values arrays have different lengths. Since zip truncates to the shorter array's length, this edge case should be tested to ensure it's handled correctly (or documented as intended behavior).
| val eventDataValues = intent.getStringArrayExtra(EXTRA_DATA_VALUES) ?: emptyArray() | ||
| val group = intent.getStringExtra(EXTRA_NOTIFICATION_GROUP) | ||
| val groupId = intent.getIntExtra(EXTRA_NOTIFICATION_GROUP_ID, -1) | ||
| val databaseId = intent.getLongExtra(EXTRA_NOTIFICATION_DB, 0) |
There was a problem hiding this comment.
When databaseId is null, putExtra will store null, but getLongExtra will return the default value of 0 regardless of whether null was stored or the key is missing. This means you cannot distinguish between a missing database ID and an actual ID of 0. Consider using hasExtra() to check if the key exists before retrieving the value, or handle the null case explicitly by storing a sentinel value like -1 for missing database IDs.
|
@jpelgrom I don't know yet what to do for these ANR I'm not sure there is a "right" way |
Summary
BroadcastReceiver should remain small since it is meant to have a short life we should migrate to the worker API. This is the first PR to showcase how we could do it for the NotificationDelete.
Checklist
Any other notes
It is based on #6459