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

…cation is disabled for the time being).

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/fmLfee

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

Codecov Report

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

Project coverage is 80.10%. Comparing base (9234de6) to head (bd27b50).
Report is 17 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ibraries/push/impl/push/OnRedactedEventReceived.kt	37.14%	21 Missing and 1 partial ⚠️
...ifications/NotificationBroadcastReceiverHandler.kt	20.00%	4 Missing ⚠️
...sh/impl/notifications/NotificationResolverQueue.kt	87.09%	0 Missing and 4 partials ⚠️
...atrix/impl/notification/RustNotificationService.kt	81.25%	1 Missing and 2 partials ⚠️
.../libraries/push/impl/push/SyncOnNotifiableEvent.kt	80.00%	0 Missing and 3 partials ⚠️
.../notifications/DefaultNotificationDrawerManager.kt	0.00%	2 Missing ⚠️
...ications/DefaultOnMissedCallNotificationHandler.kt	0.00%	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.09%   80.10%           
========================================
  Files         2129     2130    +1     
  Lines        56416    56493   +77     
  Branches      7052     7070   +18     
========================================
+ Hits         45188    45254   +66     
- Misses        8811     8815    +4     
- Partials      2417     2424    +7

☔ 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.

…s inside the same time window

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.

…ions` to its constructor

sonarqubecloud · 2025-05-19T11:01:47Z

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 added 3 commits May 14, 2025 09:38

Notification batching PoC

e2724c7

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

8778a1b

…cation is disabled for the time being).

Try restoring SyncOnNotifiableEvent behaviour

77d63f9

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

jmartinesp added 3 commits May 15, 2025 09:36

Reduce batch window time that was incresed for testing

5167305

Make tests build again (they need refactor and fixes)

ddc046c

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

735a88e

bmarty reviewed May 15, 2025

View reviewed changes

jmartinesp added 3 commits May 16, 2025 08:46

Try improving return type for NotifiableEventResolver.resolveEvents

6b517e1

Add some docs

e5735c4

Add test for DefaultPushHandler when receiving several notification…

ad25233

…s inside the same time window

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 added 3 commits May 16, 2025 17:55

Revert leftovers of experiment on notification priority

d9bb5b7

Move sessionId parameter from `RustNotificationService.getNotificat…

b5ece84

…ions` to its constructor

Try using a debounce strategy instead of a time window one

bd27b50

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

Notification events resolving and rendering in batches #4722

Are you sure you want to change the base?

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!

sonarqubecloud bot commented May 19, 2025

Quality Gate passed

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