Code review:

- Gate the new notificationsFollowing / notificationsEveryone fan-out in updateFeedsWith / deleteNotes on splitNotificationsEnabled.value so the default-off case stops doing two extra filter scans per event bundle. - Lazy-init NotificationFeedFilter.overrideFollowLists so the SharingStarted.Eagerly topNavFilter pipeline only starts when the split UI actually opens the filter. - Switch SplitNotificationsScaffold to rememberForeverPagerState so the active tab survives recomposition, matching HomeScreen/DiscoverScreen. - Promote the duplicated "Notification" last-read key string to a NOTIFICATION_LAST_READ_KEY const. - Move the opaque-background fix into SummaryBar itself so callers don't need a band-aid Modifier.background on their topBar Column. - Trim narrating / WHAT comments; keep only the deep-link scroll WHY.
2026-05-12 08:44:51 +02:00
parent 972bce75c5
commit b91e84b416
7 changed files with 46 additions and 55 deletions
@@ -83,6 +83,7 @@ object PagerStateKeys {
    const val HOME_SCREEN = "PagerHome"
    const val DISCOVER_SCREEN = "PagerDiscover"
    const val POLLS_SCREEN = "PagerPolls"
+    const val NOTIFICATION_SCREEN = "PagerNotification"
 }

@Composable
@@ -109,11 +109,6 @@ class AccountFeedContentStates(
    val articlesFeed = FeedContentState(ArticlesFeedFilter(account), scope, LocalCache)

    val notifications = CardFeedContentState(NotificationFeedFilter(account), scope)
-
-    // Split-notifications mode (Issue #197): when [AccountSettings.splitNotificationsEnabled]
-    // is on, the screen renders these two pinned-mode feeds in tabs instead of [notifications].
-    // Following = kind:3 follow list members only; Everyone = global. Both are always
-    // constructed so updateFeedsWith can fan out incoming events without rebuilding state.
    val notificationsFollowing = CardFeedContentState(NotificationFeedFilter(account, TopFilter.AllFollows), scope)
    val notificationsEveryone = CardFeedContentState(NotificationFeedFilter(account, TopFilter.Global), scope)

@@ -192,8 +187,10 @@ class AccountFeedContentStates(
        articlesFeed.updateFeedWith(newNotes)

        notifications.updateFeedWith(newNotes)
-        notificationsFollowing.updateFeedWith(newNotes)
-        notificationsEveryone.updateFeedWith(newNotes)
+        if (account.settings.splitNotificationsEnabled.value) {
+            notificationsFollowing.updateFeedWith(newNotes)
+            notificationsEveryone.updateFeedWith(newNotes)
+        }
        notificationSummary.invalidateInsertData(newNotes)

        drafts.updateFeedWith(newNotes)
@@ -242,8 +239,10 @@ class AccountFeedContentStates(
        articlesFeed.deleteFromFeed(newNotes)

        notifications.deleteFromFeed(newNotes)
-        notificationsFollowing.deleteFromFeed(newNotes)
-        notificationsEveryone.deleteFromFeed(newNotes)
+        if (account.settings.splitNotificationsEnabled.value) {
+            notificationsFollowing.deleteFromFeed(newNotes)
+            notificationsEveryone.deleteFromFeed(newNotes)
+        }
        notificationSummary.invalidateInsertData(newNotes)

        drafts.deleteFromFeed(newNotes)
@@ -81,6 +81,7 @@ import com.vitorpamplona.amethyst.ui.note.showAmount
 import com.vitorpamplona.amethyst.ui.note.showAmountInteger
 import com.vitorpamplona.amethyst.ui.screen.UiSettingsState
 import com.vitorpamplona.amethyst.ui.screen.loggedIn.notifications.CombinedZap
+import com.vitorpamplona.amethyst.ui.screen.loggedIn.notifications.NOTIFICATION_LAST_READ_KEY
 import com.vitorpamplona.amethyst.ui.screen.loggedIn.relays.eventsync.EventSync
 import com.vitorpamplona.amethyst.ui.stringRes
 import com.vitorpamplona.amethyst.ui.tor.TorSettingsFlow
@@ -319,16 +320,13 @@ class AccountViewModel(

    @OptIn(ExperimentalCoroutinesApi::class)
    val notificationHasNewItems =
-        // Issue #197: when split-notifications is ON, the bottom-bar dot must glow only
-        // for items in the Following tab. When OFF, fall back to the user-selected single
-        // feed. Switch sources on the toggle flow so changing the setting takes effect
-        // without a restart.
+        // When split-notifications is on, the badge tracks only the Following feed.
        account.settings.splitNotificationsEnabled
            .flatMapLatest { isSplit ->
                val source =
                    if (isSplit) feedStates.notificationsFollowing else feedStates.notifications
                combineTransform(
-                    account.loadLastReadFlow("Notification"),
+                    account.loadLastReadFlow(NOTIFICATION_LAST_READ_KEY),
                    source.feedContent
                        .flatMapLatest {
                            if (it is CardFeedState.Loaded) {
@@ -347,7 +345,7 @@ class AccountViewModel(
                    } else {
                        feedStates.notifications
                    }
-                val lastRead = account.loadLastReadFlow("Notification").value
+                val lastRead = account.loadLastReadFlow(NOTIFICATION_LAST_READ_KEY).value
                val cards = source.feedContent.value
                if (cards is CardFeedState.Loaded) {
                    val newestItemCreatedAt =
@@ -20,11 +20,9 @@
 */
 package com.vitorpamplona.amethyst.ui.screen.loggedIn.notifications

-import androidx.compose.foundation.background
 import androidx.compose.foundation.layout.Column
 import androidx.compose.foundation.pager.HorizontalPager
 import androidx.compose.foundation.pager.PagerState
-import androidx.compose.foundation.pager.rememberPagerState
 import androidx.compose.material3.MaterialTheme
 import androidx.compose.material3.SecondaryTabRow
 import androidx.compose.material3.Tab
@@ -33,15 +31,16 @@ import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.rememberCoroutineScope
-import androidx.compose.ui.Modifier
 import androidx.lifecycle.compose.collectAsStateWithLifecycle
 import com.vitorpamplona.amethyst.R
 import com.vitorpamplona.amethyst.model.UiSettingsFlow
 import com.vitorpamplona.amethyst.ui.components.SelectNotificationProvider
+import com.vitorpamplona.amethyst.ui.feeds.PagerStateKeys
 import com.vitorpamplona.amethyst.ui.feeds.RefresheableBox
 import com.vitorpamplona.amethyst.ui.feeds.ScrollStateKeys
 import com.vitorpamplona.amethyst.ui.feeds.WatchScrollToTop
 import com.vitorpamplona.amethyst.ui.feeds.rememberForeverLazyListState
+import com.vitorpamplona.amethyst.ui.feeds.rememberForeverPagerState
 import com.vitorpamplona.amethyst.ui.layouts.DisappearingScaffold
 import com.vitorpamplona.amethyst.ui.navigation.bottombars.AppBottomBar
 import com.vitorpamplona.amethyst.ui.navigation.navs.INav
@@ -51,6 +50,8 @@ import com.vitorpamplona.amethyst.ui.stringRes
 import com.vitorpamplona.amethyst.ui.theme.TabRowHeight
 import kotlinx.coroutines.launch

+const val NOTIFICATION_LAST_READ_KEY = "Notification"
+
@Composable
 fun NotificationScreen(
    scrollToEventId: String? = null,
@@ -124,10 +125,7 @@ private fun SingleNotificationsScaffold(
    DisappearingScaffold(
        isInvertedLayout = false,
        topBar = {
-            // DisappearingScaffold layers content under a translating topBar; without an
-            // opaque background, feed items scroll visibly through the SummaryBar gap
-            // between NotificationTopBar and the divider.
-            Column(modifier = Modifier.background(MaterialTheme.colorScheme.background)) {
+            Column {
                NotificationTopBar(accountViewModel, nav, showSpinner = true)
                SummaryBar(state = notifSummaryState)
            }
@@ -163,18 +161,13 @@ private fun SplitNotificationsScaffold(
    accountViewModel: AccountViewModel,
    nav: INav,
 ) {
-    val pagerState = rememberPagerState(pageCount = { 2 })
+    val pagerState = rememberForeverPagerState(key = PagerStateKeys.NOTIFICATION_SCREEN) { 2 }
    val coroutineScope = rememberCoroutineScope()

    DisappearingScaffold(
        isInvertedLayout = false,
        topBar = {
-            // Mirror HomeScreen: the TabRow lives in the topBar Column so it inherits
-            // the scaffold's status-bar inset handling and scrolls together with the
-            // title + summary instead of floating into the status bar area.
-            // Opaque background hides feed items that scroll under the topBar through
-            // the SummaryBar gap (which has no container of its own).
-            Column(modifier = Modifier.background(MaterialTheme.colorScheme.background)) {
+            Column {
                NotificationTopBar(accountViewModel, nav, showSpinner = false)
                SummaryBar(state = notifSummaryState)
                SecondaryTabRow(
@@ -240,7 +233,7 @@ private fun SingleNotificationsBody(
            accountViewModel = accountViewModel,
            listState = listState,
            nav = nav,
-            routeForLastRead = "Notification",
+            routeForLastRead = NOTIFICATION_LAST_READ_KEY,
            scrollToEventId = scrollToEventId,
            headerContent = { ObserveInboxRelayListAndDisplayIfNotFound(accountViewModel, nav) },
        )
@@ -306,10 +299,7 @@ private fun NotificationPagerPage(
            accountViewModel = accountViewModel,
            listState = listState,
            nav = nav,
-            // Both tabs share the "Notification" last-read marker (Section H #7): the
-            // unread indicator is gated to the Following tab elsewhere, so a single
-            // marker keeps migration trivial and avoids a stale marker per tab.
-            routeForLastRead = "Notification",
+            routeForLastRead = NOTIFICATION_LAST_READ_KEY,
            scrollToEventId = scrollToEventId,
            headerContent = { ObserveInboxRelayListAndDisplayIfNotFound(accountViewModel, nav) },
        )
@@ -25,7 +25,9 @@ import androidx.compose.animation.expandVertically
 import androidx.compose.animation.shrinkVertically
 import androidx.compose.animation.slideInVertically
 import androidx.compose.animation.slideOutVertically
+import androidx.compose.foundation.background
 import androidx.compose.foundation.clickable
+import androidx.compose.foundation.layout.Column
 import androidx.compose.foundation.layout.Row
 import androidx.compose.foundation.layout.padding
 import androidx.compose.material3.MaterialTheme
@@ -46,21 +48,25 @@ import com.vitorpamplona.amethyst.ui.theme.chartStyle
 fun SummaryBar(state: NotificationSummaryState) {
    var showChart by remember { mutableStateOf(false) }

-    UserReactionsRow(state) { showChart = !showChart }
+    // Opaque background: DisappearingScaffold layers feed content under the topBar
+    // on scroll, and SummaryBar otherwise has no container of its own.
+    Column(modifier = Modifier.background(MaterialTheme.colorScheme.background)) {
+        UserReactionsRow(state) { showChart = !showChart }

-    AnimatedVisibility(
-        visible = showChart,
-        enter = slideInVertically() + expandVertically(),
-        exit = slideOutVertically() + shrinkVertically(),
-    ) {
-        Row(
-            modifier =
-                Modifier
-                    .padding(vertical = 0.dp, horizontal = 20.dp)
-                    .clickable(onClick = { showChart = !showChart }),
+        AnimatedVisibility(
+            visible = showChart,
+            enter = slideInVertically() + expandVertically(),
+            exit = slideOutVertically() + shrinkVertically(),
        ) {
-            ProvideVicoTheme(MaterialTheme.colorScheme.chartStyle) {
-                ObserveAndShowChart(state)
+            Row(
+                modifier =
+                    Modifier
+                        .padding(vertical = 0.dp, horizontal = 20.dp)
+                        .clickable(onClick = { showChart = !showChart }),
+            ) {
+                ProvideVicoTheme(MaterialTheme.colorScheme.chartStyle) {
+                    ObserveAndShowChart(state)
+                }
            }
        }
    }
@@ -52,8 +52,6 @@ fun NotificationTopBar(
                onChange = accountViewModel.account.settings::changeDefaultNotificationFollowList,
            )
        } else {
-            // Split-notifications (Issue #197): tabs replace the list-filter spinner, so
-            // render a plain title in the top bar's content slot.
            Text(text = stringRes(R.string.route_notifications))
        }
    }
@@ -81,13 +81,12 @@ class NotificationFeedFilter(
    val account: Account,
    val modeOverride: TopFilter? = null,
 ) : AdditiveFeedFilter<Note>() {
-    // When [modeOverride] is set (split-notifications tabs), build a dedicated
-    // IFeedTopNavFilter flow so this filter is independent of the user-controlled
-    // [Account.liveNotificationFollowLists]. The Following tab pins AllFollows; the
-    // Everyone tab pins Global. When the override is null we fall back to the
-    // shared, spinner-driven flow used by the single-feed layout.
-    private val overrideFollowLists: StateFlow<IFeedTopNavFilter>? =
+    // Pin to modeOverride for split-tab mode; otherwise follow the spinner.
+    // Lazy so the eagerly-collected topNavFilter pipeline is only built when
+    // the split UI actually opens this filter.
+    private val overrideFollowLists: StateFlow<IFeedTopNavFilter>? by lazy {
        modeOverride?.let { account.topNavFilterFlow(MutableStateFlow(it)) }
+    }

    companion object {
        val ADDRESSABLE_KINDS =