Updates inbox: make it scroll, and make the release row actually open (#984)

android/app/src/main/java/com/noop/ui/AppRoot.kt

@@ -64,6 +64,8 @@ import androidx.compose.material3.NavigationDrawerItem
 import androidx.compose.material3.NavigationDrawerItemDefaults
 import androidx.compose.material3.Scaffold
 import androidx.compose.material3.Surface
+import androidx.compose.ui.window.Dialog
+import androidx.compose.ui.window.DialogProperties
 import androidx.compose.material3.Text
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
@@ -272,6 +274,9 @@ fun AppRoot(viewModel: AppViewModel = viewModel()) {
     val context = androidx.compose.ui.platform.LocalContext.current
     val updateStore = remember { UpdateStore.from(context) }
     var showUpdatesInbox by remember { mutableStateOf(false) }
+    // #984: the changelog sheet a What's New inbox row opens. Held here (not inside the inbox) so it
+    // survives the inbox sheet closing — the tap dismisses the inbox and presents this over the app.
+    var showWhatsNewFromInbox by remember { mutableStateOf(false) }
 
     run {
         Scaffold(
@@ -526,13 +531,21 @@ fun AppRoot(viewModel: AppViewModel = viewModel()) {
                     store = updateStore,
                     onClose = { showUpdatesInbox = false },
                     onDeepLink = { key ->
-                        // Map the inbox deep-link key to a route (only known keys route). "trends" is
-                        // the one real poster's target today; unknown keys just close the sheet.
-                        val route = when (key) {
-                            "trends" -> Destination.Trends.route
-                            else -> null
+                        // Map the inbox deep-link key to a route (only known keys route); unknown keys
+                        // just close the sheet.
+                        //
+                        // #984: What's New is NOT a nav destination — it is a full-screen sheet, the same
+                        // one Settings › About opens — so it gets handled here rather than through the
+                        // route table. Before this it fell to `else` and the tap did nothing at all.
+                        if (key == UpdateStore.WHATS_NEW_DEEP_LINK) {
+                            showWhatsNewFromInbox = true
+                        } else {
+                            val route = when (key) {
+                                "trends" -> Destination.Trends.route
+                                else -> null
+                            }
+                            if (route != null && route != currentRoute) nav.navigateTopLevel(route)
                         }
-                        if (route != null && route != currentRoute) nav.navigateTopLevel(route)
                     },
                     onRestore = { cardId ->
                         // Flip the shared dismissed flag back off so the card reappears, and signal a
@@ -543,6 +556,20 @@ fun AppRoot(viewModel: AppViewModel = viewModel()) {
                 )
             }
         }
+
+        // #984: the changelog a What's New inbox row opens. Full-screen Dialog, the same idiom
+        // Settings > About uses for this sheet — What's New is not a nav destination, so it cannot be
+        // reached through the route table the other deep-link keys use.
+        if (showWhatsNewFromInbox) {
+            Dialog(
+                onDismissRequest = { showWhatsNewFromInbox = false },
+                properties = DialogProperties(usePlatformDefaultWidth = false),
+            ) {
+                Surface(modifier = Modifier.fillMaxSize(), color = Palette.surfaceBase) {
+                    WhatsNewSheet(onClose = { showWhatsNewFromInbox = false })
+                }
+            }
+        }
     }
 }
 

android/app/src/main/java/com/noop/ui/UpdateStore.kt

@@ -269,6 +269,10 @@ class UpdateStore private constructor(private val prefs: SharedPreferences) {
                 kind = UpdateKind.WHATS_NEW,
                 title = if (title.isEmpty()) "What's new in NOOP $version" else title,
                 message = "NOOP $version is here — tap to read what's new.",
+                // #984: this row promised "tap to read what's new" while carrying NO deep link, so the
+                // tap resolved to nothing and only marked it read. Every release since the inbox shipped
+                // has posted an entry that could not be opened.
+                deepLink = WHATS_NEW_DEEP_LINK,
             ),
         )
     }
@@ -296,6 +300,24 @@ class UpdateStore private constructor(private val prefs: SharedPreferences) {
         private const val KEY_ITEMS = "updates.items"
         private const val KEY_LAST_SEEDED = "updates.lastSeededWhatsNewVersion"
 
+        /** Deep-link key a What's New row routes to (#984). `AppRoot`'s `onDeepLink` maps it to the
+         *  changelog sheet; any key it does not know is ignored, so this must stay in step with it. */
+        const val WHATS_NEW_DEEP_LINK = "whatsNew"
+
+        /**
+         * What a tap on [item] should open, or null when the row is purely informational (#984).
+         *
+         * Pure and separate from the Compose row so the rule is unit-testable — the store's own I/O is
+         * SharedPreferences + org.json and therefore not reachable from a plain JVM test (same
+         * constraint `NapStoreTest` documents), so this is the part of the fix that CAN be pinned.
+         *
+         * The kind fallback exists for rows posted BEFORE the fix: a What's New item already sitting in
+         * someone's inbox carries no `deepLink`, and without this it would stay inert until the next
+         * release replaced it. New rows carry the key themselves.
+         */
+        fun deepLinkTarget(item: UpdateItem): String? =
+            item.deepLink ?: WHATS_NEW_DEEP_LINK.takeIf { item.kind == UpdateKind.WHATS_NEW }
+
         /** Inbox guard-rails (#521). Cap the informational ([UpdateKind.READING]/[WHATS_NEW]) backlog and
          *  collapse an identical informational post landing within this window into the existing row, so
          *  background recompute ticks can't grow the inbox unbounded or re-post the same row on a loop. */

android/app/src/main/java/com/noop/ui/UpdatesInboxScreen.kt

@@ -10,12 +10,14 @@ import androidx.compose.foundation.layout.Column
 import androidx.compose.foundation.layout.Row
 import androidx.compose.foundation.layout.Spacer
 import androidx.compose.foundation.layout.fillMaxWidth
+import androidx.compose.foundation.verticalScroll
 import androidx.compose.foundation.layout.padding
 import androidx.compose.foundation.layout.size
 import androidx.compose.foundation.layout.width
 import androidx.compose.foundation.shape.CircleShape
 import androidx.compose.foundation.shape.RoundedCornerShape
 import androidx.compose.foundation.clickable
+import androidx.compose.foundation.rememberScrollState
 import androidx.compose.foundation.interaction.MutableInteractionSource
 import androidx.compose.foundation.background
 import androidx.compose.foundation.layout.fillMaxSize
@@ -85,6 +87,15 @@ fun UpdatesInboxScreen(
     Column(
         modifier = Modifier
             .fillMaxWidth()
+            // #984: the sheet could not scroll. `ModalBottomSheet` hands its content a plain
+            // `ColumnScope` and does NOT scroll it, so everything past the bottom of the screen was
+            // simply clipped and unreachable — the footer's Clear all / Mark all read included.
+            //
+            // It is also half of why a tapped row looked deleted: marking it read moves it out of "New"
+            // and down into "Earlier", which is rendered below, i.e. into the part that could not be
+            // reached. Safe to wrap because nothing in this screen is itself lazily scrolling; a
+            // LazyColumn nested in a verticalScroll would crash on an infinite height constraint.
+            .verticalScroll(rememberScrollState())
             .padding(horizontal = 20.dp)
             .padding(bottom = 24.dp),
         verticalArrangement = Arrangement.spacedBy(Metrics.sectionGap),
@@ -413,7 +424,9 @@ private fun handleTap(
     onClose: () -> Unit,
 ) {
     store.markRead(item.id)
-    val key = item.deepLink ?: return
+    // #984: resolved by UpdateStore.deepLinkTarget so the rule is testable (it also covers What's New
+    // rows posted before this fix, which carry no deepLink of their own).
+    val key = UpdateStore.deepLinkTarget(item) ?: return
     onDeepLink(key)
     onClose()
 }

android/app/src/test/java/com/noop/ui/UpdateDeepLinkTest.kt

@@ -0,0 +1,74 @@
+package com.noop.ui
+
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNull
+import org.junit.Test
+
+/**
+ * #984 — what a tap on an inbox row opens.
+ *
+ * The bug: the What's New row was posted with NO `deepLink` while its own message read "tap to read
+ * what's new", and the tap handler bailed out on a null link. So every release since the inbox shipped
+ * posted an entry that could not be opened — the tap only marked it read, which moved it from "New"
+ * down into "Earlier" on a sheet that could not scroll, and it looked deleted.
+ *
+ * [UpdateStore.deepLinkTarget] is the rule, pulled out of the Compose row so it can be pinned here.
+ * The store's own persistence is SharedPreferences + org.json and is not reachable from a plain JVM
+ * test (the same constraint `NapStoreTest` documents), so this is the half of the fix that CAN be
+ * tested — the seeding itself is covered by the Android build, not by a unit test.
+ */
+class UpdateDeepLinkTest {
+
+    private fun item(kind: UpdateKind, deepLink: String? = null) =
+        UpdateItem(kind = kind, title = "t", message = "m", deepLink = deepLink)
+
+    /** The regression itself: a What's New row opens the changelog even with no link of its own. */
+    @Test
+    fun whatsNewResolvesToTheChangelogWithoutAnExplicitLink() {
+        assertEquals(UpdateStore.WHATS_NEW_DEEP_LINK,
+            UpdateStore.deepLinkTarget(item(UpdateKind.WHATS_NEW)))
+    }
+
+    /**
+     * The fallback is what rescues rows ALREADY sitting in someone's inbox. Those were posted before
+     * the fix with `deepLink = null`, and without this they would stay inert until a later release
+     * replaced them — which is exactly the state the bug was reported from.
+     */
+    @Test
+    fun aWhatsNewRowPostedBeforeTheFixStillOpens() {
+        val legacyRow = UpdateItem(kind = UpdateKind.WHATS_NEW, title = "NOOP 9.2.3", message = "m")
+        assertNull("precondition: the old row carries no link", legacyRow.deepLink)
+        assertEquals(UpdateStore.WHATS_NEW_DEEP_LINK, UpdateStore.deepLinkTarget(legacyRow))
+    }
+
+    /** An explicit link always wins, so the fallback can never hijack a row that names its own target. */
+    @Test
+    fun anExplicitLinkIsNotOverriddenByTheFallback() {
+        assertEquals("trends",
+            UpdateStore.deepLinkTarget(item(UpdateKind.WHATS_NEW, deepLink = "trends")))
+    }
+
+    /** Purely informational kinds stay inert — the fallback is scoped to What's New, not to everything. */
+    @Test
+    fun otherKindsWithoutALinkStayInformational() {
+        assertNull(UpdateStore.deepLinkTarget(item(UpdateKind.STRAP_ALERT)))
+        assertNull(UpdateStore.deepLinkTarget(item(UpdateKind.DISMISSED_CARD)))
+        assertNull(UpdateStore.deepLinkTarget(item(UpdateKind.READING)))
+    }
+
+    /** Those kinds still route when they DO carry a link (the "reading" rows point at Trends). */
+    @Test
+    fun otherKindsStillRouteWhenTheyCarryALink() {
+        assertEquals("trends", UpdateStore.deepLinkTarget(item(UpdateKind.READING, deepLink = "trends")))
+    }
+
+    /**
+     * The key is a contract with `AppRoot`'s `onDeepLink`, which ignores any key it does not recognise —
+     * a silent no-op, i.e. the exact failure mode this issue was. Pinned so a rename cannot reintroduce
+     * it quietly on one side.
+     */
+    @Test
+    fun theChangelogKeyIsStable() {
+        assertEquals("whatsNew", UpdateStore.WHATS_NEW_DEEP_LINK)
+    }
+}