Add `ActiveRoomHolder` to manage the active room for a session #4758

jmartinesp · 2025-05-21T12:16:00Z

Content

What the title says

Motivation and context

This allows us to reuse these rooms for several actions that would otherwise create a new instance of the room, which, if the room is already available, would cause delays of several seconds since the live timeline can take a while to restore if it contains many events.

Tests

These actions should be noticeably faster:

Starting or joining a call.
Sharing media from outside the app.
Syncing when a notification is received.
Replying to a notification.

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

This allows us to reuse these rooms for several actions that would otherwise create a new instance of the room, which, if the room is already available, would cause delays of several seconds since the live timeline can take a while to restore if it contains many events.

github-actions · 2025-05-21T12:21: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/hMexZx

codecov · 2025-05-21T12:29:46Z

Codecov Report

Attention: Patch coverage is 66.03774% with 18 lines in your changes missing coverage. Please review.

Project coverage is 80.32%. Comparing base (8e87bf2) to head (ec2e448).
Report is 18 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ment/android/features/share/impl/SharePresenter.kt	50.00%	1 Missing and 4 partials ⚠️
.../libraries/push/impl/push/SyncOnNotifiableEvent.kt	68.75%	1 Missing and 4 partials ⚠️
...droid/features/call/impl/ui/CallScreenPresenter.kt	40.00%	0 Missing and 3 partials ⚠️
...roid/services/appnavstate/api/ActiveRoomsHolder.kt	75.00%	0 Missing and 3 partials ⚠️
...ifications/NotificationBroadcastReceiverHandler.kt	50.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4758      +/-   ##
===========================================
- Coverage    80.32%   80.32%   -0.01%     
===========================================
  Files         2127     2128       +1     
  Lines        56319    56351      +32     
  Branches      7055     7068      +13     
===========================================
+ Hits         45241    45266      +25     
+ Misses        8661     8659       -2     
- Partials      2417     2426       +9

☔ 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

Some remarks

bmarty · 2025-05-21T13:56:41Z

...navstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/ActiveRoomHolder.kt

+class ActiveRoomHolder @Inject constructor() {
+    private val rooms = ConcurrentHashMap<SessionId, JoinedRoom>()
+
+    fun addRoom(room: JoinedRoom) {


Maybe change to setRoom? Or store a Stack of rooms to fix the issue with permalink?

...navstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/ActiveRoomHolder.kt

bmarty · 2025-05-21T14:01:37Z

appnav/src/main/kotlin/io/element/android/appnav/room/joined/JoinedRoomLoadedFlowNode.kt

@@ -95,6 +98,7 @@ class JoinedRoomLoadedFlowNode @AssistedInject constructor(
            },
            onDestroy = {
                Timber.v("OnDestroy")
+                activeRoomHolder.removeRoom(inputs.room.sessionId)


This does not work well with permalink. If a room A opens a room B, when going back to room A, the holder will be empty.

… held by the `ActiveRoomHolder`

bmarty

Thanks for the update. A few more comments.

bmarty · 2025-05-23T12:23:09Z

appnav/src/main/kotlin/io/element/android/appnav/room/joined/JoinedRoomLoadedFlowNode.kt

@@ -95,6 +98,7 @@ class JoinedRoomLoadedFlowNode @AssistedInject constructor(
            },
            onDestroy = {
                Timber.v("OnDestroy")
+                activeRoomHolder.removeRoom(inputs.room.sessionId)


I still believe that this would be useful to provide the roomId to the removeRoom api, to double check that we are removing the correct room, and not the last added one.

Also one question, what will happen if there is an active call in the room, but the user closes the timeline? I guess the only issue is that there will be no cache, so not a real problem.

I still believe that this would be useful to provide the roomId to the removeRoom api, to double check that we are removing the correct room, and not the last added one.

Makes sense.

Also one question, what will happen if there is an active call in the room, but the user closes the timeline? I guess the only issue is that there will be no cache, so not a real problem.

I'd say nothing really. A call needs the sync to work and an active timeline to start the call, but if it's already running there should be no issues, at least with the current features.

bmarty · 2025-05-23T12:25:14Z

...impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt

 ) : ClearCacheUseCase {
    override suspend fun invoke() = withContext(coroutineDispatchers.io) {
+        // Active rooms should be disposed of before clearing the cache
+        activeRoomHolder.removeRoom(matrixClient.sessionId)?.destroy()


Now that activeRoomHolder stores a list of rooms, I think this code needs to be updated to destroy all the rooms.

bmarty · 2025-05-23T12:26:35Z

...navstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/ActiveRoomHolder.kt

+ * This works as a FILO (First In Last Out) stack, meaning that the last room added for a session will be the first one to be removed.
+ */
+@SingleIn(AppScope::class)
+class ActiveRoomHolder @Inject constructor() {


Maybe rename to ActiveRoomsHolder?

…d with a session before clearing the cache

… which room to remove

…nstance twice, remove outdated docs

sonarqubecloud · 2025-05-23T14:18:22Z

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 2 commits May 21, 2025 14:11

Add tests

2e54b2a

jmartinesp requested a review from a team as a code owner May 21, 2025 12:16

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

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

bmarty reviewed May 21, 2025

View reviewed changes

jmartinesp added 3 commits May 22, 2025 14:19

Try fixing the review comments

0f3d2c4

Fix ending a call destroying the underlying room, since this might be…

9b5fa77

… held by the `ActiveRoomHolder`

Also fix share presenter closing the room if it's held

f7549de

jmartinesp force-pushed the misc/add-active-room-holder branch from 86da4f7 to f7549de Compare May 22, 2025 14:07

bmarty reviewed May 23, 2025

View reviewed changes

jmartinesp added 4 commits May 23, 2025 16:01

Rename ActiveRoomHolder to ActiveRoomsHolder

51ec859

Add ActiveRoomsHolder.clear(sessionId) to clear all rooms associate…

9fc2729

…d with a session before clearing the cache

Add roomId param to ActiveRoomsHolder.removeRoom so we can decide…

beb2952

… which room to remove

Change the list of active rooms to a set so you can't have the same i…

ec2e448

…nstance twice, remove outdated docs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add `ActiveRoomHolder` to manage the active room for a session #4758

Add `ActiveRoomHolder` to manage the active room for a session #4758

jmartinesp commented May 21, 2025 •

edited

Loading

Uh oh!

github-actions bot commented May 21, 2025 •

edited

Loading

Uh oh!

codecov bot commented May 21, 2025 •

edited

Loading

Uh oh!

bmarty left a comment

Uh oh!

bmarty May 21, 2025

Uh oh!

Uh oh!

bmarty May 21, 2025

Uh oh!

bmarty left a comment

Uh oh!

bmarty May 23, 2025

Uh oh!

jmartinesp May 23, 2025

Uh oh!

bmarty May 23, 2025

Uh oh!

bmarty May 23, 2025

Uh oh!

sonarqubecloud bot commented May 23, 2025

Uh oh!

Uh oh!

Add ActiveRoomHolder to manage the active room for a session #4758

Are you sure you want to change the base?

Add ActiveRoomHolder to manage the active room for a session #4758

Conversation

jmartinesp commented May 21, 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 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 21, 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!

bmarty May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bmarty May 21, 2025

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

bmarty May 23, 2025

Choose a reason for hiding this comment

Uh oh!

jmartinesp May 23, 2025

Choose a reason for hiding this comment

Uh oh!

bmarty May 23, 2025

Choose a reason for hiding this comment

Uh oh!

bmarty May 23, 2025

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 23, 2025

Quality Gate passed

Uh oh!

Uh oh!

Add `ActiveRoomHolder` to manage the active room for a session #4758

Add `ActiveRoomHolder` to manage the active room for a session #4758

jmartinesp commented May 21, 2025 •

edited

Loading

github-actions bot commented May 21, 2025 •

edited

Loading

codecov bot commented May 21, 2025 •

edited

Loading