Split notifications for messages in threads #5595

jmartinesp · 2025-10-23T11:43:13Z

Content

Splits room notifications and thread notifications so they're grouped separately in the notification drawer if the associated feature flag is enabled.
Add deep linking navigation to threads from their notifications.
Following a notification (threaded or not) will focus in the event inside the room.
Make sure redactions work for threads too.
When opening a thread, this will also clear the notifications for that thread.

Known issues:

Opening a thread from a notification will also clear the notifications for the parent room, since we very briefly visit that room's screen.
Sometimes the notifications don't display the associated avatar. It's not always the same rooms/threads, it seems quite random. I'm not sure if it started with this PR.

Motivation and context

Implements #5581.

Screenshots / GIFs

Tests

Enable the threads flag in labs.
Receive a notification for a room/DM.
Receive a notification for one or several threads in a room/DM.
Tap on the room notification, check it takes you to the latest event in the notification.
Then tap on the thread notification, which should now open the thread.
Exit both the thread and the room.
Receive a couple of new notifications for room and thread.
Open the room from the room list. The notifications associated to it should be gone.
Do the same for the thread, opening it from the timeline.
Exit the thread.
Receive a new notification for a thread.
Redact that event after it's received.
Check the notification is also redacted.

Tested devices

Physical
Emulator
OS version(s): 14, 16

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

...rc/main/kotlin/io/element/android/libraries/push/api/notifications/NotificationIdProvider.kt

github-actions · 2025-10-23T12:10:14Z

📱 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/RzG8DU

codecov · 2025-10-23T14:44:14Z

Codecov Report

❌ Patch coverage is 83.24022% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.78%. Comparing base (6c3b280) to head (05aeef8).

Files with missing lines	Patch %	Lines
...ifications/NotificationBroadcastReceiverHandler.kt	31.25%	4 Missing and 7 partials ⚠️
...push/impl/notifications/NotificationDataFactory.kt	81.81%	0 Missing and 4 partials ⚠️
...atures/messages/impl/timeline/TimelinePresenter.kt	91.17%	2 Missing and 1 partial ⚠️
.../impl/notifications/ActiveNotificationsProvider.kt	50.00%	2 Missing ⚠️
...mpl/notifications/factories/NotificationCreator.kt	92.59%	1 Missing and 1 partial ⚠️
...ibraries/push/impl/push/OnRedactedEventReceived.kt	93.54%	1 Missing and 1 partial ⚠️
...lement/android/appnav/room/RoomNavigationTarget.kt	66.66%	1 Missing ⚠️
...ndroid/features/messages/api/MessagesEntryPoint.kt	50.00%	1 Missing ⚠️
...d/libraries/deeplink/impl/DefaultDeeplinkParser.kt	66.66%	0 Missing and 1 partial ⚠️
...ies/matrix/impl/notification/NotificationMapper.kt	0.00%	1 Missing ⚠️
... and 2 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5595      +/-   ##
===========================================
+ Coverage    79.72%   79.78%   +0.05%     
===========================================
  Files         2395     2395              
  Lines        65012    65078      +66     
  Branches      8262     8294      +32     
===========================================
+ Hits         51831    51920      +89     
+ Misses       10224    10189      -35     
- Partials      2957     2969      +12

☔ 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

Amazing work, thanks!
A few remarks after a first review.

app/src/main/kotlin/io/element/android/x/intent/DefaultIntentProvider.kt

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

...nk/impl/src/main/kotlin/io/element/android/libraries/deeplink/impl/DefaultDeepLinkCreator.kt

...id/libraries/push/impl/notifications/conversations/DefaultNotificationConversationService.kt

...otlin/io/element/android/libraries/push/impl/notifications/factories/PendingIntentFactory.kt

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

...n/kotlin/io/element/android/libraries/push/impl/notifications/ActiveNotificationsProvider.kt

bmarty

Adding the patch to the comment does not work, here it is: attachThread.patch

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

bmarty · 2025-10-27T13:58:31Z

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

-            it.conversationTitle = roomName.takeIf { roomIsGroup }
-            it.isGroupConversation = roomIsGroup
+            it.conversationTitle = if (isThread) {
+                stringProvider.getString(CommonStrings.notification_thread_in_room, roomName)


For DM the title is a bit strange since the room name is the other user name:

tests/testutils/src/main/kotlin/io/element/android/tests/testutils/lambda/Assertions.kt

bmarty

One question.

bmarty · 2025-10-29T13:09:54Z

appnav/src/main/kotlin/io/element/android/appnav/RootFlowNode.kt


+    private suspend fun RoomFlowNode.maybeAttachThread(threadId: ThreadId?, focusedEventId: EventId?) {
+        if (threadId != null) {
+            waitForNavTargetAttached { it is RoomFlowNode.NavTarget.JoinedRoom }


I do not understand why this line is necessary (it was not in my patch).
RoomFlowNode.attachThread already contains waitForChildAttached<JoinedRoomFlowNode>(). This is the same thing, no? Or am I missing something? I am getting confused with all those Nodes :)

From our EXA devs chat:

there was another interesting bug
LoggedInFlowNode.attachRoom creates a new RoomFlowNode and waits until a RoomFlowNode is present
however, what happens if you do this while a RoomFlowNode was already present? the operation returns the existing one :D
and since that one is about to be disposed, anything pushed into it is ignored

You can test this by removing this code and:

Opening a room.

Receiving a notification for a thread while the room is visible (of that room or some other).

Opening the notification.

Instead of the thread UI being pushed, you'll see the messages screen for that room being pushed, and the thread screen is nowhere to be found. If you attach a debugger, you'll see the attach thread operation does happen... in the previous RoomFlowNode, not the one we just replaced it with.

I will try to repro. But for me the problem you described was fixed by this code:

element-x-android/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

Line 521 in cfcb821

return waitForChildAttached<RoomFlowNode, NavTarget> {

I have commented out the code and I do not have the issue you describe

Ah sorry, I thought we were talking about that other one. This one may not be necessary. If it's not in your tests, we can just remove it, since I probably added it to check what was causing the weird navigation and forgot to remove it later.

bmarty · 2025-10-29T22:08:38Z

Failing test is not failing locally...

…eature flag is enabled. Otherwise, set the `threadId` to null so it'll behave as usual. It's done this way to avoid having to inject `FeatureFlagService` in several places.

…he latest event in the list of messages of the notification tapped

…dId` value, then because of the `focusedEventId` one

…vent

This seemed to fix the issue about occasionally missing large icons on my local tests.

…CATION_ID`+ `ForegroundServiceType` is used instead

…Service

…ultActiveNotificationsProvider.getMessageNotificationsForRoom`

@bmarty

Thanks @bmarty!

…vigating to a thread

…for now.

… now

sonarqubecloud · 2025-10-30T08:32:50Z

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 the PR-Feature For a new feature label Oct 23, 2025

jmartinesp force-pushed the feat/add-thread-notifications-and-deeplink-navigation branch from c07eab0 to 68fb2fb Compare October 23, 2025 12:02

jmartinesp commented Oct 23, 2025

View reviewed changes

...rc/main/kotlin/io/element/android/libraries/push/api/notifications/NotificationIdProvider.kt Outdated Show resolved Hide resolved

jmartinesp force-pushed the feat/add-thread-notifications-and-deeplink-navigation branch from 68fb2fb to a2fd3e5 Compare October 23, 2025 13:37

jmartinesp marked this pull request as ready for review October 23, 2025 14:10

jmartinesp requested a review from a team as a code owner October 23, 2025 14:10

jmartinesp requested review from bmarty and removed request for a team October 23, 2025 14:10

jmartinesp mentioned this pull request Oct 24, 2025

Update notification style #5607

Merged

9 tasks

bmarty reviewed Oct 24, 2025

View reviewed changes

jmartinesp force-pushed the feat/add-thread-notifications-and-deeplink-navigation branch from a2bb283 to 692635e Compare October 27, 2025 11:09

bmarty reviewed Oct 27, 2025

View reviewed changes

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt Outdated Show resolved Hide resolved

bmarty reviewed Oct 27, 2025

View reviewed changes

jmartinesp force-pushed the feat/add-thread-notifications-and-deeplink-navigation branch 2 times, most recently from c2f84ab to cfcb821 Compare October 28, 2025 12:32

jmartinesp requested a review from bmarty October 28, 2025 16:47

bmarty reviewed Oct 29, 2025

View reviewed changes

tests/testutils/src/main/kotlin/io/element/android/tests/testutils/lambda/Assertions.kt Outdated Show resolved Hide resolved

bmarty reviewed Oct 29, 2025

View reviewed changes

jmartinesp force-pushed the feat/add-thread-notifications-and-deeplink-navigation branch 2 times, most recently from c5a1b31 to 6a7db17 Compare October 29, 2025 17:54

jmartinesp added 7 commits October 30, 2025 08:39

Separate thread notifications into their own notifications when the f…

8e5bf26

…eature flag is enabled. Otherwise, set the `threadId` to null so it'll behave as usual. It's done this way to avoid having to inject `FeatureFlagService` in several places.

Add permalink navigation to threads from notifications, focusing on t…

ce1e6e3

…he latest event in the list of messages of the notification tapped

Fix redactions in threads

afc9130

Fix and add tests

a7b2495

Clear notifications for a thread when visiting it

9be020b

Fix opening a thread happening twice, first because of the `openThrea…

4417ae8

…dId` value, then because of the `focusedEventId` one

Make opening a room through a notification also focus on the latest e…

e6a9321

…vent

jmartinesp and others added 17 commits October 30, 2025 08:39

Add helper NotificationCreator.messageTag function

c812cc1

Try using Icon instead of Bitmap for largeIcon

9382c47

This seemed to fix the issue about occasionally missing large icons on my local tests.

Reorder createOpenThreadPendingIntent parameters

7bfcdfb

Remove unused ROOM_CALL_NOTIFICATION_ID: `FOREGROUND_SERVICE_NOTIFI…

512267d

…CATION_ID`+ `ForegroundServiceType` is used instead

Reorder parameters in IntentProvider.getViewRoomIntent

2313a04

Remove unnecessary apply

86ae942

Simplify DefaultDeepLinkCreator

eda5327

Revert removal of OS version check in DefaultNotificationConversation…

85baa0d

…Service

Use NotificationCreator.messageTag to get the expected tag in `Defa…

4579ad5

…ultActiveNotificationsProvider.getMessageNotificationsForRoom`

Fix lint issue

c76c332

Refactor navigation to thread.

a4c5fb9

Thanks @bmarty!

Make sure the main timeline focuses on the thread root id too when na…

718c3c2

…vigating to a thread

Revert incorrect Assertions change

c0dc6bc

Handle "Mark as read" action for thread notification, but disable it …

462fde9

…for now.

Fix minor issues

3ab0324

Add advanceUntilIdle to try to unblock test failing in CI

1634f65

Restore mark as read actions for threads, using timeline.markAsRead…

6a9ca09

… now

jmartinesp force-pushed the feat/add-thread-notifications-and-deeplink-navigation branch from a109496 to 6a9ca09 Compare October 30, 2025 07:51

Fix NotificationBroadcastReceiverHandlerTest tests

05aeef8

jmartinesp requested a review from bmarty October 30, 2025 11:21

Split notifications for messages in threads #5595

Are you sure you want to change the base?

Split notifications for messages in threads #5595

Conversation

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

Uh oh!

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

codecov bot commented Oct 23, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmarty Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

bmarty Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

jmartinesp Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

bmarty Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

bmarty Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

jmartinesp Oct 29, 2025

Choose a reason for hiding this comment

Uh oh!

bmarty commented Oct 29, 2025

Uh oh!

sonarqubecloud bot commented Oct 30, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmartinesp commented Oct 23, 2025 •

edited

Loading

github-actions bot commented Oct 23, 2025 •

edited

Loading

codecov bot commented Oct 23, 2025 •

edited

Loading