From 339819928a99f0ca84997f544288e0c3d1d4219a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 18:18:15 +0000 Subject: [PATCH 1/2] fix(nav): land on the tab root when a nav-bar item is tapped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On tablets the navigation rail stays on screen while the user is deep inside a tab, so — unlike the phone bottom bar, which hides itself off tab roots — it can be tapped from anywhere. Tapping Home from there came back to whatever screen the user had left open rather than to the feed. `popUpTo(Home) { inclusive = false; saveState = true }` files the popped entries under the popUpTo target's own destination id as well as the popped tab's (the `if (!inclusive)` branch of NavControllerImpl.executePopOperations), so the following `navigate(Home) { restoreState = true }` handed Home back the stack that was saved on the way out of some other tab. navBottomBar now does two things before the navigate: - drops whatever the user pushed on top of the tab root they are in, without saving it, so no deep stack is ever eligible for a later restore; - pops back to the tab when it is already on the stack — always true for Home, and for the tab the user is inside — instead of navigating to it. The tab root keeps the ViewModelStore and scroll position it already has, and Home never goes through restoreState at all. Phones are unaffected: the bottom bar is only reachable from a tab root, so there is nothing to pop and the net stack is the same as before. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014PdeozanQPzJ5kzFTDhMnZ --- .../amethyst/ui/navigation/navs/Nav.kt | 48 ++++++++ .../ui/navigation/NavBottomBarStackTest.kt | 107 ++++++++++++++++++ .../ui/navigation/NavImeSettleTest.kt | 9 ++ 3 files changed, 164 insertions(+) create mode 100644 amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt index ac0adde7bc..652e515fb9 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt @@ -30,6 +30,7 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.setValue import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner import androidx.navigation.NavBackStackEntry +import androidx.navigation.NavDestination.Companion.hasRoute import androidx.navigation.NavGraph.Companion.findStartDestination import androidx.navigation.NavHostController import com.vitorpamplona.amethyst.ui.navigation.BOTTOM_NAV_ROOT_KEY @@ -102,6 +103,34 @@ class Nav( override fun navBottomBar(route: Route) { navigationScope.launch { ime.settle() + + // A nav-bar tap asks for a tab, never for whatever the user pushed on top of one. Drop + // those pushes first, and without saving them, so the restoreState below can never hand + // a deep stack back. On phones this is always a no-op — AppBottomBar hides itself off + // tab roots, so the bar is only ever tapped from one — but the large-screen rail stays + // on screen the whole time and is routinely tapped from three screens deep. + popPushesAboveTabRoot() + + // Home always sits at the bottom of the stack (it is the graph's start destination and + // the anchor of the popUpTo below), and so does the tab the user is already inside: + // popping back to it beats navigating to it. The tab root keeps the ViewModelStore and + // scroll position it already has, and the branch above it is saved under its own tab, + // exactly as the navigate path would have saved it. + // + // Home in particular MUST come back this way. `popUpTo(Home) { inclusive = false; + // saveState = true }` files the popped entries under the popUpTo target's own + // destination id as well as the popped tab's — the `if (!inclusive)` branch of + // NavControllerImpl.executePopOperations — so `navigate(Home) { restoreState = true }` + // hands Home back whatever was popped on the way out of some *other* tab. That is how + // tapping Home on the rail came back to the thread the user had left open three screens + // deep in another tab instead of to the home feed. + val existing = runCatching { controller.getBackStackEntry(route) }.getOrNull() + if (existing != null) { + controller.popBackStack(route, inclusive = false, saveState = true) + existing.savedStateHandle.set(BOTTOM_NAV_ROOT_KEY, true) + return@launch + } + controller.navigate(route) { // Clear sibling bottom-nav entries but keep Home (the start // destination) below, so back-swipe from any tab returns to @@ -146,6 +175,25 @@ class Nav( } } + /** + * Drops every entry the user pushed on top of the tab root they are currently in, discarding + * that state rather than saving it — a nav-bar tap is a request for a tab, and nothing above a + * tab root should survive to be replayed by a later `restoreState`. + * + * Stops at the first entry stamped [BOTTOM_NAV_ROOT_KEY] by [navBottomBar], or at Home, which + * is a tab root the user may never have tapped because the graph starts there. Terminates + * either way: every iteration that does not return removes one entry from a finite back stack, + * and [NavHostController.popBackStack] reports false once there is nothing left to pop. + */ + private fun popPushesAboveTabRoot() { + while (true) { + val top = controller.currentBackStackEntry ?: return + if (top.isBottomNavRoot()) return + if (top.destination.hasRoute(Route.Home::class)) return + if (!controller.popBackStack()) return + } + } + @Composable override fun canPop(): Boolean { // Decide the back arrow / bottom-bar visibility from THIS screen's own diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt new file mode 100644 index 0000000000..2d8150cac5 --- /dev/null +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt @@ -0,0 +1,107 @@ +/* + * Copyright (c) 2025 Vitor Pamplona + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to use, + * copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the + * Software, and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS + * FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN + * AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION + * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ +package com.vitorpamplona.amethyst.ui.navigation + +import androidx.navigation.NavBackStackEntry +import androidx.navigation.NavHostController +import androidx.navigation.NavOptionsBuilder +import com.vitorpamplona.amethyst.ui.navigation.navs.Nav +import com.vitorpamplona.amethyst.ui.navigation.routes.Route +import io.mockk.every +import io.mockk.mockk +import io.mockk.verify +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runTest +import org.junit.Test + +/** + * A nav-bar tap must land on the tab itself, never on whatever the user had pushed on top of one. + * + * The phone bottom bar gets this for free — it hides itself off tab roots, so it can only ever be + * tapped from one. The large-screen navigation rail stays on screen the whole time, which is where + * the gap showed: tapping Home from a thread three screens deep came back to that thread instead of + * the feed. + * + * Two things had to be true for that, and each test below pins one of them down: + * + * 1. Pushes above a tab root are dropped, unsaved, on the way out. Anything saved is eligible to be + * replayed by a later `restoreState`. + * 2. A tab that is already on the back stack — always the case for Home, which is the graph's start + * destination — is reached by popping back to it, not by navigating to it. + * + * (2) is what actually broke. `popUpTo(Home) { inclusive = false; saveState = true }` files the + * popped entries under the popUpTo target's own destination id as well as the popped tab's — see + * the `if (!inclusive)` branch of `NavControllerImpl.executePopOperations` — so a later + * `navigate(Home) { restoreState = true }` handed Home back the stack the user had left behind in + * some *other* tab. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class NavBottomBarStackTest { + /** A back-stack entry that is or isn't a tab root, as far as [isBottomNavRoot] can tell. */ + private fun entry(isTabRoot: Boolean): NavBackStackEntry = + mockk(relaxed = true) { + every { savedStateHandle.get(BOTTOM_NAV_ROOT_KEY) } returns isTabRoot + } + + @Test + fun dropsWhatTheUserPushedOnTopOfTheTabBeforeSwitchingTabs() = + runTest { + // Home > Note > Profile, with Home stamped as the tab root the two pushes sit on. + val controller = + mockk(relaxed = true) { + every { currentBackStackEntry } returnsMany + listOf(entry(isTabRoot = false), entry(isTabRoot = false), entry(isTabRoot = true)) + every { popBackStack() } returns true + // Messages is not on the stack until the navigate below puts it there. + every { getBackStackEntry(Route.Message) } throws + IllegalArgumentException("No destination is on the NavController's back stack") andThen + entry(isTabRoot = true) + } + + Nav(controller, this).navBottomBar(Route.Message) + advanceUntilIdle() + + // Exactly the two pushes, and no more: the tab root itself stays, keeping its + // ViewModelStore for the tab's own restore. + verify(exactly = 2) { controller.popBackStack() } + verify(exactly = 1) { controller.navigate(Route.Message, any Unit>()) } + } + + @Test + fun popsBackToATabThatIsAlreadyOnTheStackInsteadOfNavigatingToIt() = + runTest { + val home = entry(isTabRoot = true) + val controller = + mockk(relaxed = true) { + every { currentBackStackEntry } returns home + every { getBackStackEntry(Route.Home) } returns home + } + + Nav(controller, this).navBottomBar(Route.Home) + advanceUntilIdle() + + // Popping back to Home keeps the feed's own state and, unlike navigating, cannot pick up + // a saved stack that popUpTo(Home) filed under Home's destination id. + verify(exactly = 1) { controller.popBackStack(Route.Home, inclusive = false, saveState = true) } + verify(exactly = 0) { controller.navigate(any(), any Unit>()) } + } +} diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt index 79d95b500b..878810bfcc 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt @@ -20,6 +20,7 @@ */ package com.vitorpamplona.amethyst.ui.navigation +import androidx.navigation.NavBackStackEntry import androidx.navigation.NavHostController import androidx.navigation.NavOptionsBuilder import com.vitorpamplona.amethyst.ui.navigation.navs.ImeSettler @@ -50,6 +51,14 @@ import org.junit.Test class NavImeSettleTest { private fun controllerRecording(order: MutableList): NavHostController = mockk(relaxed = true) { + // An empty back stack: nothing is pushed over a tab root and no tab is already on it, + // so navBottomBar takes its navigate path rather than the pop-back-to-the-tab shortcut. + // The tab lands on the stack once that navigate runs, which is what stops navBottomBar + // from falling through to its second, sibling-destination navigate. + every { popBackStack() } returns false + every { getBackStackEntry(any()) } throws + IllegalArgumentException("No destination is on the NavController's back stack") andThen + mockk(relaxed = true) every { navigate(any(), any Unit>()) } answers { order.add("navigate") } every { navigate(any()) } answers { order.add("navigate") } From 49fad0c619e982f74a13a4fb631b815af167b2f3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 21:21:00 +0000 Subject: [PATCH 2/2] refactor(nav): fold the tab-tap fix into one navigate path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut grew navBottomBar to three exits: drop the pushes, pop back to the tab when it was already on the stack, or navigate. The middle one was doing by hand what the navigate already does. Once the pushes are gone the stack is [Home] or [Home, tab], so the only thing the navigate gets wrong is Home — the popUpTo anchor, whose own destination id a non-inclusive saveState pop also files the popped entries under. Saying so directly, `restoreState = route != Route.Home`, replaces the whole branch; the already-there case falls out of the same guard nav() uses, and Home keeps its ViewModelStore through launchSingleTop rather than through a pop. The tests now assert the contract (the NavOptions the tap builds) rather than which controller method was called, and NavImeSettleTest needs no stubs again. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_014PdeozanQPzJ5kzFTDhMnZ --- .../amethyst/ui/navigation/navs/Nav.kt | 32 ++++---- .../ui/navigation/NavBottomBarStackTest.kt | 75 +++++++++++-------- .../ui/navigation/NavImeSettleTest.kt | 9 --- 3 files changed, 56 insertions(+), 60 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt index 652e515fb9..53f02d6c7e 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/navigation/navs/Nav.kt @@ -111,23 +111,11 @@ class Nav( // on screen the whole time and is routinely tapped from three screens deep. popPushesAboveTabRoot() - // Home always sits at the bottom of the stack (it is the graph's start destination and - // the anchor of the popUpTo below), and so does the tab the user is already inside: - // popping back to it beats navigating to it. The tab root keeps the ViewModelStore and - // scroll position it already has, and the branch above it is saved under its own tab, - // exactly as the navigate path would have saved it. - // - // Home in particular MUST come back this way. `popUpTo(Home) { inclusive = false; - // saveState = true }` files the popped entries under the popUpTo target's own - // destination id as well as the popped tab's — the `if (!inclusive)` branch of - // NavControllerImpl.executePopOperations — so `navigate(Home) { restoreState = true }` - // hands Home back whatever was popped on the way out of some *other* tab. That is how - // tapping Home on the rail came back to the thread the user had left open three screens - // deep in another tab instead of to the home feed. - val existing = runCatching { controller.getBackStackEntry(route) }.getOrNull() - if (existing != null) { - controller.popBackStack(route, inclusive = false, saveState = true) - existing.savedStateHandle.set(BOTTOM_NAV_ROOT_KEY, true) + // Dropping those pushes is often the whole job — re-tapping the tab the user is inside, + // and every tap of Home from somewhere inside Home, end here. Same already-there guard + // nav() uses, so a parameterized tab compares by its filled route. + if (getRouteWithArguments(route::class, controller) == route) { + controller.currentBackStackEntry?.savedStateHandle?.set(BOTTOM_NAV_ROOT_KEY, true) return@launch } @@ -146,7 +134,15 @@ class Nav( saveState = true } launchSingleTop = true - restoreState = true + // ...but never onto Home. A non-inclusive popUpTo files the popped entries under the + // popUpTo TARGET's own destination id as well as the popped tab's — the + // `if (!inclusive)` branch of NavControllerImpl.executePopOperations — so restoring + // here would hand Home the stack that was saved on the way out of some *other* tab. + // That is how tapping Home on the rail came back to a thread instead of the feed. + // Home is the anchor, so it is never popped and has no saved stack of its own to + // miss: launchSingleTop reuses the entry already sitting there, ViewModelStore and + // all. + restoreState = route != Route.Home } // Mark this entry as a tab root: hides the back arrow in canPop // and skips the horizontal slide in composableFromEnd. diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt index 2d8150cac5..c5cac66bbb 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavBottomBarStackTest.kt @@ -22,15 +22,21 @@ package com.vitorpamplona.amethyst.ui.navigation import androidx.navigation.NavBackStackEntry import androidx.navigation.NavHostController +import androidx.navigation.NavOptions import androidx.navigation.NavOptionsBuilder +import androidx.navigation.navOptions import com.vitorpamplona.amethyst.ui.navigation.navs.Nav import com.vitorpamplona.amethyst.ui.navigation.routes.Route import io.mockk.every import io.mockk.mockk +import io.mockk.slot import io.mockk.verify import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue import org.junit.Test /** @@ -41,67 +47,70 @@ import org.junit.Test * the gap showed: tapping Home from a thread three screens deep came back to that thread instead of * the feed. * - * Two things had to be true for that, and each test below pins one of them down: + * Two rules keep that from happening, one per test below: * - * 1. Pushes above a tab root are dropped, unsaved, on the way out. Anything saved is eligible to be + * 1. Pushes above a tab root are dropped, unsaved, on the way out — anything saved is eligible to be * replayed by a later `restoreState`. - * 2. A tab that is already on the back stack — always the case for Home, which is the graph's start - * destination — is reached by popping back to it, not by navigating to it. - * - * (2) is what actually broke. `popUpTo(Home) { inclusive = false; saveState = true }` files the - * popped entries under the popUpTo target's own destination id as well as the popped tab's — see - * the `if (!inclusive)` branch of `NavControllerImpl.executePopOperations` — so a later - * `navigate(Home) { restoreState = true }` handed Home back the stack the user had left behind in - * some *other* tab. + * 2. Home is never restored onto. `popUpTo(Home) { inclusive = false; saveState = true }` files the + * popped entries under the popUpTo target's own destination id as well as the popped tab's — see + * the `if (!inclusive)` branch of `NavControllerImpl.executePopOperations` — so + * `navigate(Home) { restoreState = true }` handed Home back the stack the user had left behind in + * some *other* tab. Every other tab restores as before; that is what keeps its ViewModelStore. */ @OptIn(ExperimentalCoroutinesApi::class) class NavBottomBarStackTest { - /** A back-stack entry that is or isn't a tab root, as far as [isBottomNavRoot] can tell. */ + /** A back-stack entry that is, or isn't, a tab root as far as [isBottomNavRoot] can tell. */ private fun entry(isTabRoot: Boolean): NavBackStackEntry = mockk(relaxed = true) { every { savedStateHandle.get(BOTTOM_NAV_ROOT_KEY) } returns isTabRoot } + /** + * Taps [route] on a controller whose back stack holds none of the nav-bar destinations, and + * returns the options the resulting navigate was built with. + */ + private fun TestScope.optionsForTapping(route: Route): NavOptions { + val options = slot Unit>() + val controller = + mockk(relaxed = true) { + every { currentBackStackEntry } returns entry(isTabRoot = true) + every { navigate(route, capture(options)) } returns Unit + } + + Nav(controller, this).navBottomBar(route) + advanceUntilIdle() + + return navOptions(options.captured) + } + @Test fun dropsWhatTheUserPushedOnTopOfTheTabBeforeSwitchingTabs() = runTest { - // Home > Note > Profile, with Home stamped as the tab root the two pushes sit on. + // Home > Note > Profile: two pushes sitting on a tab root. val controller = mockk(relaxed = true) { every { currentBackStackEntry } returnsMany listOf(entry(isTabRoot = false), entry(isTabRoot = false), entry(isTabRoot = true)) every { popBackStack() } returns true - // Messages is not on the stack until the navigate below puts it there. - every { getBackStackEntry(Route.Message) } throws - IllegalArgumentException("No destination is on the NavController's back stack") andThen - entry(isTabRoot = true) } Nav(controller, this).navBottomBar(Route.Message) advanceUntilIdle() - // Exactly the two pushes, and no more: the tab root itself stays, keeping its - // ViewModelStore for the tab's own restore. + // Exactly the two pushes and no more: the tab root itself stays, so the navigate below + // saves a tab root rather than a branch that could be replayed later. verify(exactly = 2) { controller.popBackStack() } - verify(exactly = 1) { controller.navigate(Route.Message, any Unit>()) } } @Test - fun popsBackToATabThatIsAlreadyOnTheStackInsteadOfNavigatingToIt() = + fun neverRestoresASavedStackOntoHome() = runTest { - val home = entry(isTabRoot = true) - val controller = - mockk(relaxed = true) { - every { currentBackStackEntry } returns home - every { getBackStackEntry(Route.Home) } returns home - } + assertFalse(optionsForTapping(Route.Home).shouldRestoreState()) + } - Nav(controller, this).navBottomBar(Route.Home) - advanceUntilIdle() - - // Popping back to Home keeps the feed's own state and, unlike navigating, cannot pick up - // a saved stack that popUpTo(Home) filed under Home's destination id. - verify(exactly = 1) { controller.popBackStack(Route.Home, inclusive = false, saveState = true) } - verify(exactly = 0) { controller.navigate(any(), any Unit>()) } + @Test + fun restoresTheSavedStackOfEveryOtherTab() = + runTest { + assertTrue(optionsForTapping(Route.Message).shouldRestoreState()) } } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt index 878810bfcc..79d95b500b 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/navigation/NavImeSettleTest.kt @@ -20,7 +20,6 @@ */ package com.vitorpamplona.amethyst.ui.navigation -import androidx.navigation.NavBackStackEntry import androidx.navigation.NavHostController import androidx.navigation.NavOptionsBuilder import com.vitorpamplona.amethyst.ui.navigation.navs.ImeSettler @@ -51,14 +50,6 @@ import org.junit.Test class NavImeSettleTest { private fun controllerRecording(order: MutableList): NavHostController = mockk(relaxed = true) { - // An empty back stack: nothing is pushed over a tab root and no tab is already on it, - // so navBottomBar takes its navigate path rather than the pop-back-to-the-tab shortcut. - // The tab lands on the stack once that navigate runs, which is what stops navBottomBar - // from falling through to its second, sibling-destination navigate. - every { popBackStack() } returns false - every { getBackStackEntry(any()) } throws - IllegalArgumentException("No destination is on the NavController's back stack") andThen - mockk(relaxed = true) every { navigate(any(), any Unit>()) } answers { order.add("navigate") } every { navigate(any()) } answers { order.add("navigate") }