From 5a2977134beff8d8bf5e623d8e27aefd297c15e6 Mon Sep 17 00:00:00 2001 From: Jenna Vassar Date: Tue, 9 Jun 2026 18:05:22 -0700 Subject: [PATCH] Gate out-of-window jump-to-unread on genuinely unread displayable content The jump-to-unread FAB could surface an OutOfWindow target when the fully-read marker event was either loaded-but-not-displayed (state/filtered events) or when there was nothing displayable to jump to. Add two guards to the recompute: - Require roomInfo.numUnreadMessages > 0, so the FAB only appears when there is genuinely unread "interesting" content (never state/hidden events). - Add Timeline.isEventLoaded to distinguish "in the loaded window but not rendered" from "genuinely outside the window", falling back to the SDK only after the cheap display-index check (isKnown) misses. The RustTimeline implementation reads the replay cache first and releases the EventTimelineItem native handle via use {} when probing the SDK. Adds presenter tests for the loaded-but-not-displayed and no-unread-messages branches. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../impl/timeline/TimelinePresenter.kt | 34 +++++--- .../impl/timeline/TimelinePresenterTest.kt | 77 ++++++++++++++++++- .../libraries/matrix/api/timeline/Timeline.kt | 7 ++ .../matrix/impl/timeline/RustTimeline.kt | 18 +++++ .../matrix/test/timeline/FakeTimeline.kt | 3 + 5 files changed, 128 insertions(+), 11 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt index 1e399332e4..00039dc1af 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt @@ -312,23 +312,39 @@ class TimelinePresenter( LaunchedEffect(roomInfo.fullyReadEventId) { suppressJumpToUnread.value = false } - LaunchedEffect(timelineItems.map { it.identifier() }, displayJumpToUnread, roomInfo.fullyReadEventId, suppressJumpToUnread.value) { + LaunchedEffect( + timelineItems.map { it.identifier() }, + displayJumpToUnread, + roomInfo.fullyReadEventId, + roomInfo.numUnreadMessages, + suppressJumpToUnread.value, + ) { if (!displayJumpToUnread || suppressJumpToUnread.value) { jumpToUnread.value = JumpToUnreadState.Hidden return@LaunchedEffect } val items = timelineItems val fullyReadEventId = roomInfo.fullyReadEventId - jumpToUnread.value = withContext(dispatchers.computation) { - val markerIndex = items.indexOfFirst { + val hasUnreadMessages = roomInfo.numUnreadMessages > 0 + val markerIndex = withContext(dispatchers.computation) { + items.indexOfFirst { (it as? TimelineItem.Virtual)?.model is TimelineItemReadMarkerModel } - when { - markerIndex >= 0 -> JumpToUnreadState.InWindow(markerIndex) - fullyReadEventId != null && items.isNotEmpty() && !timelineItemIndexer.isKnown(fullyReadEventId) -> - JumpToUnreadState.OutOfWindow(fullyReadEventId) - else -> JumpToUnreadState.Hidden - } + } + jumpToUnread.value = when { + markerIndex >= 0 -> JumpToUnreadState.InWindow(markerIndex) + // Out-of-window only when there is genuinely unread *displayable* content + // (numUnreadMessages counts "interesting" messages, never state/hidden events) AND + // the marker event isn't merely an in-window item we don't render. isKnown is the + // cheap display-index check; isEventLoaded falls back to the SDK to tell + // "in window but not displayed" apart from "genuinely out of window". + fullyReadEventId != null && + hasUnreadMessages && + items.isNotEmpty() && + !timelineItemIndexer.isKnown(fullyReadEventId) && + !timelineController.activeTimelineFlow().value.isEventLoaded(fullyReadEventId) -> + JumpToUnreadState.OutOfWindow(fullyReadEventId) + else -> JumpToUnreadState.Hidden } } diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt index bd40fccf71..f8f23b35f1 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt @@ -608,7 +608,9 @@ class TimelinePresenterTest { liveTimeline = timeline, baseRoom = FakeBaseRoom( roomPermissions = roomPermissions(), - initialRoomInfo = aRoomInfo(fullyReadEventId = fullyReadEventId), + // There is genuinely unread *displayable* content, and the marker event isn't loaded + // (isEventLoaded defaults to false), so the FAB targets the out-of-window marker. + initialRoomInfo = aRoomInfo(fullyReadEventId = fullyReadEventId, numUnreadMessages = 1), ), ) val presenter = createTimelinePresenter( @@ -632,6 +634,77 @@ class TimelinePresenterTest { } } + @Test + fun `present - jumpToUnread is Hidden when the marker event is loaded in the window but not displayed`() = runTest { + val timelineItems = MutableStateFlow(emptyList()) + // The marker event is in the loaded window (e.g. a state/filtered event) but never rendered. + val fullyReadEventId = EventId("\$loaded-but-not-displayed") + val timeline = FakeTimeline(timelineItems = timelineItems).apply { + isEventLoadedLambda = { it == fullyReadEventId } + } + val room = FakeJoinedRoom( + liveTimeline = timeline, + baseRoom = FakeBaseRoom( + roomPermissions = roomPermissions(), + initialRoomInfo = aRoomInfo(fullyReadEventId = fullyReadEventId, numUnreadMessages = 1), + ), + ) + val presenter = createTimelinePresenter( + timeline = timeline, + room = room, + featureFlagService = FakeFeatureFlagService(initialState = mapOf(FeatureFlags.JumpToUnread.key to true)), + ) + presenter.test { + awaitFirstItem() + // Displayed items don't contain the marker event, but the SDK reports it as loaded. + timelineItems.emit( + listOf( + MatrixTimelineItem.Event(UniqueId("1"), anEventTimelineItem(eventId = AN_EVENT_ID, content = aMessageContent())), + MatrixTimelineItem.Event(UniqueId("2"), anEventTimelineItem(eventId = AN_EVENT_ID_2, content = aMessageContent())), + ) + ) + advanceUntilIdle() + // It must never be OutOfWindow — there is nothing displayable to jump to. + val drained = consumeItemsUntilTimeout() + assertThat(drained.any { it.jumpToUnread is JumpToUnreadState.OutOfWindow }).isFalse() + assertThat(drained.last().jumpToUnread).isEqualTo(JumpToUnreadState.Hidden) + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `present - jumpToUnread is Hidden when out of window but there are no unread messages`() = runTest { + val timelineItems = MutableStateFlow(emptyList()) + val fullyReadEventId = EventId("\$older-than-loaded-window") + // isEventLoaded defaults to false (genuinely out of window), but numUnreadMessages is 0. + val timeline = FakeTimeline(timelineItems = timelineItems) + val room = FakeJoinedRoom( + liveTimeline = timeline, + baseRoom = FakeBaseRoom( + roomPermissions = roomPermissions(), + initialRoomInfo = aRoomInfo(fullyReadEventId = fullyReadEventId, numUnreadMessages = 0), + ), + ) + val presenter = createTimelinePresenter( + timeline = timeline, + room = room, + featureFlagService = FakeFeatureFlagService(initialState = mapOf(FeatureFlags.JumpToUnread.key to true)), + ) + presenter.test { + awaitFirstItem() + timelineItems.emit( + listOf( + MatrixTimelineItem.Event(UniqueId("1"), anEventTimelineItem(eventId = AN_EVENT_ID, content = aMessageContent())), + MatrixTimelineItem.Event(UniqueId("2"), anEventTimelineItem(eventId = AN_EVENT_ID_2, content = aMessageContent())), + ) + ) + advanceUntilIdle() + val drained = consumeItemsUntilTimeout() + assertThat(drained.any { it.jumpToUnread != JumpToUnreadState.Hidden }).isFalse() + cancelAndIgnoreRemainingEvents() + } + } + @Test fun `present - jumpToUnread is Hidden when fullyReadEventId IS in the loaded window`() = runTest { val timelineItems = MutableStateFlow(emptyList()) @@ -766,7 +839,7 @@ class TimelinePresenterTest { liveTimeline = timeline, baseRoom = FakeBaseRoom( roomPermissions = roomPermissions(), - initialRoomInfo = aRoomInfo(fullyReadEventId = fullyReadEventId), + initialRoomInfo = aRoomInfo(fullyReadEventId = fullyReadEventId, numUnreadMessages = 1), ), ) val presenter = createTimelinePresenter( diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/Timeline.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/Timeline.kt index fe73230dce..93f69088b4 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/Timeline.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/Timeline.kt @@ -216,6 +216,13 @@ interface Timeline : AutoCloseable { suspend fun loadReplyDetails(eventId: EventId): InReplyTo + /** + * Returns true if [eventId] is currently loaded in this timeline's window, even if the display + * layer does not render it (e.g. filtered or state events). This distinguishes "in the window + * but not displayed" from "genuinely outside the loaded window". + */ + suspend fun isEventLoaded(eventId: EventId): Boolean + /** * Adds a new pinned event by sending an updated `m.room.pinned_events` * event containing the new event id. diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt index d44676d34a..13d9b3cf63 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt @@ -637,6 +637,24 @@ class RustTimeline( } } + override suspend fun isEventLoaded(eventId: EventId): Boolean = withContext(dispatcher) { + val isInLoadedItems = _timelineItems.replayCache.firstOrNull().orEmpty().any { timelineItem -> + timelineItem is MatrixTimelineItem.Event && timelineItem.eventId == eventId + } + if (isInLoadedItems) { + // Displayed events are caught above. The SDK-level list still holds events the display + // layer later drops, so this avoids the FFI call for in-window-but-not-rendered events. + true + } else { + // getEventTimelineItemByEventId throws when the event isn't in the loaded window. + // EventTimelineItem is a Disposable wrapping a native handle, so release it once we've + // confirmed presence — otherwise the handle lingers until the GC cleaner runs. + runCatchingExceptions { + inner.getEventTimelineItemByEventId(eventId.value).use { /* presence is all we need */ } + }.isSuccess + } + } + override suspend fun pinEvent(eventId: EventId): Result = withContext(dispatcher) { runCatchingExceptions { inner.pinEvent(eventId = eventId.value) diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/timeline/FakeTimeline.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/timeline/FakeTimeline.kt index fcc7057dbe..3696f16607 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/timeline/FakeTimeline.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/timeline/FakeTimeline.kt @@ -436,6 +436,9 @@ class FakeTimeline( override suspend fun loadReplyDetails(eventId: EventId) = loadReplyDetailsLambda(eventId) + var isEventLoadedLambda: (eventId: EventId) -> Boolean = { false } + override suspend fun isEventLoaded(eventId: EventId): Boolean = isEventLoadedLambda(eventId) + var pinEventLambda: (eventId: EventId) -> Result = { lambdaError() } override suspend fun pinEvent(eventId: EventId): Result { return pinEventLambda(eventId)