From d4b7240462a33a06567661f3079e2f6b8d3412f1 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 07:02:27 +0000 Subject: [PATCH] Fix rapid-navigation misclicks and speed up screen transitions navigation-compose 2.8.5's default transition is a 700ms crossfade during which both the outgoing and incoming screens stay composed and clickable, so a fast double-tap (e.g. Home -> Reviews -> back -> Backlog) could land on the screen still fading out underneath instead of the intended destination. Replace the default crossfade with explicit 300ms directional slide+fade transitions, and gate every navigate()/popBackStack() call in ThePatientGamerHelperNavGraph behind a check that the triggering NavBackStackEntry has reached RESUMED (its own transition fully settled) before firing. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01MRCs4kTRXqukszUoU8XrrW --- CHANGELOG.md | 15 ++ .../ThePatientGamerHelperNavGraph.kt | 210 +++++++++++++----- docs/test-plan.md | 25 +++ 3 files changed, 191 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 12cf570..62af352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,21 @@ versioning follows the app's `versionName` in `app/build.gradle.kts`. ## [Unreleased] +- **Fixed rapid navigation taps landing on the wrong screen, and gave screen transitions a modern, faster feel.** + Navigating quickly (e.g. Home → Reviews → back → Backlog done in rapid succession) could + register the tap on the screen still fading out underneath instead of the intended + destination — reported live on-device, reproducible whenever two navigation actions happened + within the transition window. Root cause: `navigation-compose` 2.8.5's default transition is a + 700ms crossfade during which both the outgoing and incoming screens stay composed and + clickable at the same time, so a fast second tap could hit whichever one happened to occupy + that screen coordinate. `ThePatientGamerHelperNavGraph` now (1) declares explicit, faster + slide+fade transitions (300ms, Material-style directional push/pop) instead of the default + crossfade, and (2) gates every `navigate()`/`popBackStack()` call behind a check that the + *specific* `NavBackStackEntry` triggering it has actually reached `RESUMED` — the officially + documented signal that its own transition has fully settled — so a stray tap on a screen still + mid-transition is ignored rather than misrouted. Also addresses the general "navigation feels + slow" complaint, since 300ms directional transitions read as snappier than the previous + generic 700ms fade. - **Fixed a `release.yml` bug that could burn a version number on a failed signed build.** The workflow used to commit and push the `versionName`/`versionCode` bump and the cut `CHANGELOG.md` section to `main` *before* attempting the signed build. Found on the sibling diff --git a/app/src/main/java/com/marcogn/thepatientgamerhelper/ui/navigation/ThePatientGamerHelperNavGraph.kt b/app/src/main/java/com/marcogn/thepatientgamerhelper/ui/navigation/ThePatientGamerHelperNavGraph.kt index 2a6a907..ec1b5cf 100644 --- a/app/src/main/java/com/marcogn/thepatientgamerhelper/ui/navigation/ThePatientGamerHelperNavGraph.kt +++ b/app/src/main/java/com/marcogn/thepatientgamerhelper/ui/navigation/ThePatientGamerHelperNavGraph.kt @@ -1,5 +1,14 @@ package com.marcogn.thepatientgamerhelper.ui.navigation +import androidx.compose.animation.AnimatedContentTransitionScope +import androidx.compose.animation.EnterTransition +import androidx.compose.animation.ExitTransition +import androidx.compose.animation.core.FastOutSlowInEasing +import androidx.compose.animation.core.tween +import androidx.compose.animation.fadeIn +import androidx.compose.animation.fadeOut +import androidx.compose.animation.slideInHorizontally +import androidx.compose.animation.slideOutHorizontally import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding @@ -23,6 +32,8 @@ import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource import androidx.compose.ui.unit.dp +import androidx.lifecycle.Lifecycle +import androidx.navigation.NavBackStackEntry import androidx.navigation.NavHostController import androidx.navigation.compose.NavHost import androidx.navigation.compose.composable @@ -41,6 +52,42 @@ import com.marcogn.thepatientgamerhelper.ui.settings.SettingsScreen import com.marcogn.thepatientgamerhelper.ui.stats.StatsScreen import kotlinx.coroutines.launch +// A NavBackStackEntry only reaches RESUMED once its enter/exit transition animation has +// fully completed and it is settled on top of the back stack (see NavHost/AnimatedContent +// docs). Gating every navigate()/popBackStack() call behind this check on the *specific* +// entry that owns the callback is the officially recommended fix for a fast double-tap +// (e.g. back then immediately tapping another destination) landing on a screen that is +// still being composed/torn down mid-transition instead of the intended one. +private fun NavBackStackEntry.lifecycleIsResumed() = + lifecycle.currentState == Lifecycle.State.RESUMED + +private const val NAV_ANIM_DURATION_MS = 300 + +private val navEnterTransition: AnimatedContentTransitionScope.() -> EnterTransition = { + slideInHorizontally( + animationSpec = tween(NAV_ANIM_DURATION_MS, easing = FastOutSlowInEasing), + initialOffsetX = { fullWidth -> fullWidth }, + ) + fadeIn(animationSpec = tween(NAV_ANIM_DURATION_MS)) +} +private val navExitTransition: AnimatedContentTransitionScope.() -> ExitTransition = { + slideOutHorizontally( + animationSpec = tween(NAV_ANIM_DURATION_MS, easing = FastOutSlowInEasing), + targetOffsetX = { fullWidth -> -fullWidth / 4 }, + ) + fadeOut(animationSpec = tween(NAV_ANIM_DURATION_MS)) +} +private val navPopEnterTransition: AnimatedContentTransitionScope.() -> EnterTransition = { + slideInHorizontally( + animationSpec = tween(NAV_ANIM_DURATION_MS, easing = FastOutSlowInEasing), + initialOffsetX = { fullWidth -> -fullWidth / 4 }, + ) + fadeIn(animationSpec = tween(NAV_ANIM_DURATION_MS)) +} +private val navPopExitTransition: AnimatedContentTransitionScope.() -> ExitTransition = { + slideOutHorizontally( + animationSpec = tween(NAV_ANIM_DURATION_MS, easing = FastOutSlowInEasing), + targetOffsetX = { fullWidth -> fullWidth }, + ) + fadeOut(animationSpec = tween(NAV_ANIM_DURATION_MS)) +} + @OptIn(ExperimentalMaterial3Api::class) @Composable fun ThePatientGamerHelperNavGraph(navController: NavHostController = rememberNavController()) { @@ -49,11 +96,15 @@ fun ThePatientGamerHelperNavGraph(navController: NavHostController = rememberNav val openDrawer: () -> Unit = { scope.launch { drawerState.open() } } val navigateFromDrawer: (Destination) -> Unit = { destination -> - scope.launch { drawerState.close() } - navController.navigate(destination) { - popUpTo(Destination.Home) { saveState = true } - launchSingleTop = true - restoreState = true + // Guards against a drawer tap landing while the current screen is still mid + // transition (see lifecycleIsResumed() above) - same race as forward/back taps. + if (navController.currentBackStackEntry?.lifecycleIsResumed() != false) { + scope.launch { drawerState.close() } + navController.navigate(destination) { + popUpTo(Destination.Home) { saveState = true } + launchSingleTop = true + restoreState = true + } } } @@ -99,26 +150,35 @@ fun ThePatientGamerHelperNavGraph(navController: NavHostController = rememberNav } }, ) { - NavHost(navController = navController, startDestination = Destination.Home) { - composable { + NavHost( + navController = navController, + startDestination = Destination.Home, + enterTransition = navEnterTransition, + exitTransition = navExitTransition, + popEnterTransition = navPopEnterTransition, + popExitTransition = navPopExitTransition, + ) { + composable { entry -> HomeScreen( onMenuClick = openDrawer, - onReviewsClick = { navController.navigate(Destination.Library) }, - onBacklogClick = { navController.navigate(Destination.Backlog) }, - onStatsClick = { navController.navigate(Destination.Stats) }, + onReviewsClick = { if (entry.lifecycleIsResumed()) navController.navigate(Destination.Library) }, + onBacklogClick = { if (entry.lifecycleIsResumed()) navController.navigate(Destination.Backlog) }, + onStatsClick = { if (entry.lifecycleIsResumed()) navController.navigate(Destination.Stats) }, ) } - composable { + composable { entry -> LibraryScreen( onMenuClick = openDrawer, - onReviewClick = { id -> navController.navigate(Destination.Detail(id)) }, - onAddClick = { navController.navigate(Destination.Form()) }, + onReviewClick = { id -> + if (entry.lifecycleIsResumed()) navController.navigate(Destination.Detail(id)) + }, + onAddClick = { if (entry.lifecycleIsResumed()) navController.navigate(Destination.Form()) }, ) } composable { StatsScreen(onMenuClick = openDrawer) } - composable { + composable { entry -> SettingsScreen( onBack = { // Not a plain popBackStack(): that destroys the entry outright, clearing @@ -127,20 +187,24 @@ fun ThePatientGamerHelperNavGraph(navController: NavHostController = rememberNav // back arrow is the only way to leave it. Mirrors navigateFromDrawer's // popUpTo/saveState/restoreState so the entry (and its login state) is // preserved across drawer round-trips, same as Library/Backlog/Stats. - navController.navigate(Destination.Home) { - popUpTo(Destination.Home) { saveState = true } - launchSingleTop = true - restoreState = true + if (entry.lifecycleIsResumed()) { + navController.navigate(Destination.Home) { + popUpTo(Destination.Home) { saveState = true } + launchSingleTop = true + restoreState = true + } } }, ) } - composable { + composable { entry -> DetailScreen( - onBack = { navController.popBackStack() }, - onEdit = { id -> navController.navigate(Destination.Form(id)) }, + onBack = { if (entry.lifecycleIsResumed()) navController.popBackStack() }, + onEdit = { id -> if (entry.lifecycleIsResumed()) navController.navigate(Destination.Form(id)) }, onDeleted = { - navController.popBackStack(Destination.Library, inclusive = false) + if (entry.lifecycleIsResumed()) { + navController.popBackStack(Destination.Library, inclusive = false) + } }, ) } @@ -148,66 +212,94 @@ fun ThePatientGamerHelperNavGraph(navController: NavHostController = rememberNav val route = backStackEntry.toRoute() ReviewFormScreen( onSaved = { id -> - navController.navigate(Destination.Detail(id)) { - popUpTo(Destination.Library) + if (backStackEntry.lifecycleIsResumed()) { + navController.navigate(Destination.Detail(id)) { + popUpTo(Destination.Library) + } } }, onCancel = { - if (route.backlogItemId != null) { - // Opened from the backlog's "want to write a review?" prompt: leaving - // (with or without an implicit draft save, see ReviewFormViewModel.onBackPressed) - // should land on the reviews library, not back into the Backlog stack it came from. - // Deliberately NOT saveState = true here: this popUpTo always pops the - // Backlog -> BacklogListDetail -> BacklogItemDetail -> Form chain we're - // discarding on purpose, and NavController's saveState/restoreState keys - // saved back-stack "islands" by the id of the *first* entry above the - // popUpTo target (Backlog here) with a write-once guard per key. Saving it - // poisoned the drawer's "Backlog" entry: the next drawer tap to Backlog - // (restoreState = true in navigateFromDrawer) restored this exact stale - // chain wholesale, landing back on this Form/Detail screen instead of the - // Backlog list, popping straight back past it on the next back press. See - // REG-13 in docs/test-plan.md. - navController.navigate(Destination.Library) { - popUpTo(Destination.Home) - launchSingleTop = true - restoreState = true + if (backStackEntry.lifecycleIsResumed()) { + if (route.backlogItemId != null) { + // Opened from the backlog's "want to write a review?" prompt: leaving + // (with or without an implicit draft save, see ReviewFormViewModel.onBackPressed) + // should land on the reviews library, not back into the Backlog stack it came from. + // Deliberately NOT saveState = true here: this popUpTo always pops the + // Backlog -> BacklogListDetail -> BacklogItemDetail -> Form chain we're + // discarding on purpose, and NavController's saveState/restoreState keys + // saved back-stack "islands" by the id of the *first* entry above the + // popUpTo target (Backlog here) with a write-once guard per key. Saving it + // poisoned the drawer's "Backlog" entry: the next drawer tap to Backlog + // (restoreState = true in navigateFromDrawer) restored this exact stale + // chain wholesale, landing back on this Form/Detail screen instead of the + // Backlog list, popping straight back past it on the next back press. See + // REG-13 in docs/test-plan.md. + navController.navigate(Destination.Library) { + popUpTo(Destination.Home) + launchSingleTop = true + restoreState = true + } + } else { + navController.popBackStack() } - } else { - navController.popBackStack() } }, ) } - composable { + composable { entry -> BacklogScreen( onMenuClick = openDrawer, - onListClick = { listId -> navController.navigate(Destination.BacklogListDetail(listId)) }, - onItemClick = { itemId -> navController.navigate(Destination.BacklogItemDetail(itemId)) }, + onListClick = { listId -> + if (entry.lifecycleIsResumed()) { + navController.navigate(Destination.BacklogListDetail(listId)) + } + }, + onItemClick = { itemId -> + if (entry.lifecycleIsResumed()) { + navController.navigate(Destination.BacklogItemDetail(itemId)) + } + }, ) } composable { backStackEntry -> val listId = backStackEntry.toRoute().listId BacklogListDetailScreen( - onBack = { navController.popBackStack() }, - onAddItemClick = { navController.navigate(Destination.BacklogItemForm(listId)) }, - onItemClick = { itemId -> navController.navigate(Destination.BacklogItemDetail(itemId)) }, + onBack = { if (backStackEntry.lifecycleIsResumed()) navController.popBackStack() }, + onAddItemClick = { + if (backStackEntry.lifecycleIsResumed()) { + navController.navigate(Destination.BacklogItemForm(listId)) + } + }, + onItemClick = { itemId -> + if (backStackEntry.lifecycleIsResumed()) { + navController.navigate(Destination.BacklogItemDetail(itemId)) + } + }, ) } - composable { + composable { entry -> BacklogItemFormScreen( - onSaved = { navController.popBackStack() }, - onCancel = { navController.popBackStack() }, + onSaved = { if (entry.lifecycleIsResumed()) navController.popBackStack() }, + onCancel = { if (entry.lifecycleIsResumed()) navController.popBackStack() }, ) } - composable { + composable { entry -> BacklogItemDetailScreen( - onBack = { navController.popBackStack() }, - onEdit = { itemId, listId -> navController.navigate(Destination.BacklogItemForm(listId, itemId)) }, - onDeleted = { navController.popBackStack() }, + onBack = { if (entry.lifecycleIsResumed()) navController.popBackStack() }, + onEdit = { itemId, listId -> + if (entry.lifecycleIsResumed()) { + navController.navigate(Destination.BacklogItemForm(listId, itemId)) + } + }, + onDeleted = { if (entry.lifecycleIsResumed()) navController.popBackStack() }, onWriteReview = { itemId -> - navController.navigate(Destination.Form(backlogItemId = itemId)) { launchSingleTop = true } + if (entry.lifecycleIsResumed()) { + navController.navigate(Destination.Form(backlogItemId = itemId)) { launchSingleTop = true } + } + }, + onOpenReview = { reviewId -> + if (entry.lifecycleIsResumed()) navController.navigate(Destination.Detail(reviewId)) }, - onOpenReview = { reviewId -> navController.navigate(Destination.Detail(reviewId)) }, ) } } diff --git a/docs/test-plan.md b/docs/test-plan.md index b53d6c7..1e34f95 100644 --- a/docs/test-plan.md +++ b/docs/test-plan.md @@ -1459,6 +1459,23 @@ review alone — all the more reason not to skip them in future test rounds. the final search POST — now sent on every request. Verify online search from the backlog item form actually returns `hltbMainStoryHours`/etc. instead of the 404/error message. +- [ ] **REG-15** Navigating quickly (e.g. Home → Reviews → back → Backlog + done in fast succession) could register the second tap on the + screen still fading out underneath instead of the intended + destination — reported from real device use. Root cause: + `navigation-compose`'s default 700ms crossfade keeps both the + outgoing and incoming screens composed and clickable for the whole + duration. Fixed in `ThePatientGamerHelperNavGraph.kt`: explicit + 300ms directional slide+fade transitions replace the default + crossfade, and every `navigate()`/`popBackStack()` call is gated on + the triggering `NavBackStackEntry` having reached `RESUMED` (i.e. + its own transition has fully settled) before it's allowed to fire. + Verify by repeatedly and quickly chaining navigation actions across + Home/Library/Backlog/Stats/Settings (via drawer and in-screen + buttons) and confirming the tap always lands on the intended + destination, never a screen left over from the previous transition; + also confirm transitions now feel like a normal, snappy Android + push/pop instead of the previous slow generic fade. ## Update history for this plan @@ -1530,3 +1547,11 @@ review alone — all the more reason not to skip them in future test rounds. references it. Not yet manually verified on device — SET-16e is specifically what needs re-confirming before this can be considered closed. +- 2026-08-21 — REG-15 added: a real navigation bug reported from device + use (a fast tap sequence across Home/Library/Backlog could land on a + screen still fading out from the previous transition instead of the + intended destination), root-caused to `navigation-compose`'s default + 700ms crossfade keeping both screens composed and clickable, and fixed + in `ThePatientGamerHelperNavGraph.kt` with explicit 300ms directional + transitions plus a `lifecycleIsResumed()` guard on every navigation + call. Not yet manually verified on device.