Notification events resolving and rendering in batches #4722

jmartinesp · 2025-05-14T16:45:17Z

Content

Added NotificationResolverQueue type that enqueues notification fetching requests received in a certain time window, and resolves them in batches, emitting the results when done.
DefaultPushHandler now subscribes to these results and also processes them in batches, categorising the items and redirecting them to their callbacks (onRedactedEventReceived, handleRingingCallEvent, etc.).
DefaultNotificationDrawerManager now has a onNotifiableEventsReceived to render several notifications at the same time.

Motivation and context

The latest SDK version allows clients to fetch several notifications as a group so we need to run a single sliding sync request for them. However, since the items were returned at the same time and immediately sent to the NotificationManager so they can be displayed, the OS thought we were spamming the notifications and 'muted' (or discarded) some of them. Instead, we had to rework how we process the resolved notifications.

Tests

Send several messages in a very small time window in a room. If done correctly, both messages should be added to the notification at the same time.
Redact some of those messages, it should display as 'Redacted message' or something similar in the notification drawer.
Start a call in a DM, you should receive the ringing call notification with no issues.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 24
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request title will be used in the release note, it clearly define what will change for the user
Pull request includes screenshots or videos if containing UI changes
You've made a self review of your PR

github-actions · 2025-05-14T16:52:58Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/TwdhKK

codecov · 2025-05-15T11:19:02Z

Codecov Report

Attention: Patch coverage is 76.96335% with 44 lines in your changes missing coverage. Please review.

Project coverage is 80.41%. Comparing base (9b9d75a) to head (a4f1595).
Report is 4 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ibraries/push/impl/push/OnRedactedEventReceived.kt	37.14%	21 Missing and 1 partial ⚠️
.../libraries/push/impl/push/SyncOnNotifiableEvent.kt	72.22%	0 Missing and 5 partials ⚠️
...ifications/NotificationBroadcastReceiverHandler.kt	20.00%	4 Missing ⚠️
...atrix/impl/notification/RustNotificationService.kt	81.25%	1 Missing and 2 partials ⚠️
.../notifications/DefaultNotificationDrawerManager.kt	0.00%	2 Missing ⚠️
...ications/DefaultOnMissedCallNotificationHandler.kt	0.00%	0 Missing and 2 partials ⚠️
...sh/impl/notifications/NotificationResolverQueue.kt	93.10%	0 Missing and 2 partials ⚠️
...oid/libraries/push/impl/push/DefaultPushHandler.kt	96.22%	0 Missing and 2 partials ⚠️
...raries/push/impl/push/OnNotifiableEventReceived.kt	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4722      +/-   ##
===========================================
+ Coverage    80.40%   80.41%   +0.01%     
===========================================
  Files         2143     2144       +1     
  Lines        56678    56751      +73     
  Branches      7096     7112      +16     
===========================================
+ Hits         45570    45635      +65     
- Misses        8681     8684       +3     
- Partials      2427     2432       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bmarty

Thanks for improving this part!
Some early remarks.

bmarty · 2025-05-15T07:20:29Z

.../src/main/kotlin/io/element/android/libraries/matrix/api/notification/NotificationService.kt


 interface NotificationService {
-    suspend fun getNotification(roomId: RoomId, eventId: EventId): Result<NotificationData?>
+    suspend fun getNotification(sessionId: SessionId, roomId: RoomId, eventId: EventId): Result<NotificationData?>
+    suspend fun getNotifications(sessionId: SessionId, ids: Map<RoomId, List<EventId>>): Result<Map<EventId, NotificationData?>>


At first sight, it's strange that the return type is not
Result<Map<RoomId, List<NotificationData>>>

It was like that originally, but where it's immediately used later (here) having the notifications grouped by room id doesn't really help, we'd have to first search for the room, then use notifications.find { it.eventId == eventId }. Using having the id -> data mapping is more optimised.

bmarty · 2025-05-15T13:56:23Z

...otlin/io/element/android/libraries/push/impl/notifications/DefaultNotifiableEventResolver.kt

+    suspend fun resolveEvents(
+        sessionId: SessionId,
+        notificationEventRequests: List<NotificationEventRequest>
+    ): Result<Map<EventId, ResolvedPushEvent?>>


Same type of remark, why don't we have a return type like:
Map<RoomId, List<ResolvedPushEvent>>?

Here it might actually make sense to do that for the pushHistoryService.onSuccess and failure callbacks, searching might be a bit faster even if later we need to either iterate by room then event id or just flatten everything.

Maybe it would even make sense to use the NotificationEventRequest as the key itself? It has the session id, room and event ids to make fetching a single operation.

bmarty

More comments. I will test the code on a real device.

bmarty · 2025-05-16T14:38:36Z

.../element/android/libraries/push/impl/notifications/DefaultOnMissedCallNotificationHandler.kt

@@ -30,8 +30,9 @@ class DefaultOnMissedCallNotificationHandler @Inject constructor(
        // Resolve the event and add a notification for it, at this point it should no longer be a ringing one
        val notificationData = matrixClientProvider.getOrRestore(sessionId).getOrNull()
            ?.notificationService()
-            ?.getNotification(roomId, eventId)
+            ?.getNotifications(sessionId, mapOf(roomId to listOf(eventId)))


It looks weird to have to provide a sessionId here, since we are using a method from a service of a matrix client.

True, but RustNotificationService doesn't have a reference to the matrix client. Maybe I should just add a SessionId as a constructor param for it. WDYT?

Maybe I should just add a SessionId as a constructor param for it. WDYT?

Yes please. We're doing it for RustRoomFactory already for instance.

~~RustRoomFactory doesn't use injection though. Adding it here would mean creating a factory abstraction.~~

Never mind, I looked at the wrong component. I have a severe case of the Mondays, it seems.

bmarty · 2025-05-16T14:42:43Z

...ain/kotlin/io/element/android/libraries/push/impl/notifications/NotificationResolverQueue.kt

+        coroutineScope.launch {
+            while (coroutineScope.isActive) {
+                // Wait for a batch of requests to be received in a specified time window
+                delay(BATCH_WINDOW_MS.milliseconds)


Are you sure this code will behave as a first throttler?
If we get a Push just before the delay is ended, it will trigger the request without waiting for any other potential push. I have not checked in practice how much time we have between mutliple pending push received from Firebase though.

It doesn't have to act as a throttle operation, but as a window one, grouping events that happen in an interval in batches. Those batches won't always be optimal, since as you say, if you receive one after one window closes it'll be processed on the next one.

If you think it's worth it implementing it more in a throttle/debounce way (without discarding items as those operators usually do) I can try to do that. There is the possibility that this means we chain lots of 'received item, wait for X ms' operations and end up delaying the processing for longer, but it's not really likely to happen in the real world except maybe for when you have several pushes in firebase that couldn't be delivered, the device reconnects and it starts receiving the pending pushes.

Done in bd27b50, it seems to work quite well.

bmarty · 2025-05-16T14:44:10Z

...kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreator.kt

+                val notificationManager = NotificationManagerCompat.from(context)
+                val previousNotification = notificationManager.activeNotifications.find { it.tag == roomInfo.roomId.value }
+                val elapsed = System.currentTimeMillis() - (previousNotification?.postTime ?: 0L)
+                Timber.d("Creating noisy notification for room ${roomInfo.roomId.value} with elapsed time $elapsed")


Move this log into the if block (not sure)

Ooops, this should actually be gone. It was done for an experiment and I removed part of it, but apparently I kept this by mistake.

bmarty

A few remarks, else I think we can merge this PR to make is live in the next nightlies for a few days

bmarty · 2025-05-26T12:33:32Z

...ain/kotlin/io/element/android/libraries/push/impl/notifications/NotificationResolverQueue.kt

+
+        delay(BATCH_WINDOW_MS.milliseconds)
+
+        if (!isActive) return@launch


Is this line mandatory? I mean if the job is cancelled during the delay, I do not think this line and the following will be executed.

You're right, it's not needed. delay will check if the context is alive for the whole duration of the function being suspended.

bmarty · 2025-05-26T12:34:19Z

...ain/kotlin/io/element/android/libraries/push/impl/notifications/NotificationResolverQueue.kt

    /**
     * Enqueues a notification event request to be resolved.
     * The request will be processed in batches, so it may not be resolved immediately.
     *
     * @param request The notification event request to enqueue.
     */
    suspend fun enqueue(request: NotificationEventRequest) {
+        // Cancel previous processing job if it exists, acting as a debounce operation
+        Timber.d("Cancelling job: $currentProcessingJob")
+        currentProcessingJob?.cancel()


It should cancel only a delay operation, just reading the code I am afraid that it can cancel a started treatment, but I may be wrong

That's what I'm trying to do: the 'enqueue' job is saved and will be cancelled when a new one arrives. Internally, it waits for the time window and launches a new coroutine in the parent one, which should be independent AFAICT. Maybe we could use a SupervisorJob to ensure the 'enqueue' job being cancelled won't affect any other child coroutine of the injected scope.

…cation is disabled for the time being).

…s inside the same time window

…ions` to its constructor

… cancelling it doesn't affect their siblings

sonarqubecloud · 2025-05-26T13:56:25Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

jmartinesp changed the title ~~Notification batching~~ WIP: Notification batching May 14, 2025

bmarty reviewed May 15, 2025

View reviewed changes

jmartinesp changed the title ~~WIP: Notification batching~~ Notification events resolving and rendering in batches May 16, 2025

jmartinesp added the PR-Misc For other changes label May 16, 2025

jmartinesp marked this pull request as ready for review May 16, 2025 08:23

jmartinesp requested a review from a team as a code owner May 16, 2025 08:23

jmartinesp requested review from bmarty and removed request for a team May 16, 2025 08:23

bmarty reviewed May 16, 2025

View reviewed changes

jmartinesp requested a review from bmarty May 19, 2025 10:50

bmarty approved these changes May 26, 2025

View reviewed changes

jmartinesp added 14 commits May 26, 2025 14:43

Notification batching PoC

0e62fcf

Try batching display and removal of notifications too (sync on notifi…

95415b7

…cation is disabled for the time being).

Try restoring SyncOnNotifiableEvent behaviour

25aa0e9

Reduce batch window time that was incresed for testing

0ca9396

Make tests build again (they need refactor and fixes)

ea098fc

Fix tests, remove duplicates of methods added for lists of notifications

5b2c50c

Try improving return type for NotifiableEventResolver.resolveEvents

dbfbeaa

Add some docs

de55c8f

Add test for DefaultPushHandler when receiving several notification…

a61a398

…s inside the same time window

Revert leftovers of experiment on notification priority

fabe06c

Move sessionId parameter from `RustNotificationService.getNotificat…

5b27af2

…ions` to its constructor

Try using a debounce strategy instead of a time window one

49bb187

Rename coroutineScope to appCoroutineScope, remove isActive checks

2e62188

Launch the processQueue coroutine with a SupervisorJob to make sure…

a4f1595

… cancelling it doesn't affect their siblings

jmartinesp force-pushed the feat/process-notifications-in-batches branch from bd27b50 to a4f1595 Compare May 26, 2025 13:41

jmartinesp merged commit f455085 into develop May 26, 2025
30 of 31 checks passed

jmartinesp deleted the feat/process-notifications-in-batches branch May 26, 2025 15:10


		delay(BATCH_WINDOW_MS.milliseconds)

		if (!isActive) return@launch

Notification events resolving and rendering in batches #4722

Notification events resolving and rendering in batches #4722

Conversation

jmartinesp commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Content

Motivation and context

Tests

Tested devices

Checklist

Uh oh!

github-actions bot commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmartinesp May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 26, 2025

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

jmartinesp commented May 14, 2025 •

edited

Loading

github-actions bot commented May 14, 2025 •

edited

Loading

codecov bot commented May 15, 2025 •

edited

Loading

jmartinesp May 19, 2025 •

edited

Loading