From 3acbb803703a1d208243087d3d8fcb8eae049d8a Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Mon, 11 May 2026 11:55:00 -0400 Subject: [PATCH] fix(navigation): prevent duplicate Sheet key crash Broaden sheet dedup in CodeNavigator.navigate() to match on toString() (which mirrors SaveableStateProvider contentKey) instead of structural equality. This prevents the race where pendingSheetDismiss callbacks push a duplicate Sheet entry onto the NavBackStack. Also wrap the pendingSheetDismiss callback in a single Snapshot.withMutableSnapshot for atomicity. Signed-off-by: Brandon McAnsh --- .../app/core/extensions/CodeNavigator.kt | 11 ++-- .../app/router/internal/NavigateToTest.kt | 50 +++++++++++++++++++ .../getcode/navigation/core/CodeNavigator.kt | 10 ++-- 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/extensions/CodeNavigator.kt b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/extensions/CodeNavigator.kt index 1ebc57e90..2d05dd168 100644 --- a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/extensions/CodeNavigator.kt +++ b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/extensions/CodeNavigator.kt @@ -1,5 +1,6 @@ package com.flipcash.app.core.extensions +import androidx.compose.runtime.snapshots.Snapshot import androidx.navigation3.runtime.NavKey import com.flipcash.app.core.AppRoute import com.getcode.navigation.core.CodeNavigator @@ -39,10 +40,12 @@ fun CodeNavigator.navigateTo(routes: List, options: NavOptions = NavOpti // The callback is invoked by ModalBottomSheetScene after the dismiss // animation completes and the old entry is removed from the backstack. pendingSheetDismiss = { - sheetGeneration++ - resolved.forEachIndexed { index, route -> - val navOptions = if (index == 0) options else NavOptions() - navigate(route, navOptions) + Snapshot.withMutableSnapshot { + sheetGeneration++ + resolved.forEachIndexed { index, route -> + val navOptions = if (index == 0) options else NavOptions() + navigate(route, navOptions) + } } } } else { diff --git a/apps/flipcash/shared/router/src/test/kotlin/com/flipcash/app/router/internal/NavigateToTest.kt b/apps/flipcash/shared/router/src/test/kotlin/com/flipcash/app/router/internal/NavigateToTest.kt index 24c0b5a90..1593b4ea9 100644 --- a/apps/flipcash/shared/router/src/test/kotlin/com/flipcash/app/router/internal/NavigateToTest.kt +++ b/apps/flipcash/shared/router/src/test/kotlin/com/flipcash/app/router/internal/NavigateToTest.kt @@ -226,5 +226,55 @@ class NavigateToTest { } } + @Test + fun `navigate deduplicates identical sheet that was not removed by onBack`() { + // Reproduces the production crash: a stale Sheet(Wallet,[]) remains on the + // backstack after onBack removed the wrong entry during a dismiss animation. + // A subsequent navigate for the same Sheet must not produce a duplicate. + val navigator = createNavigator( + AppRoute.Main.Scanner, + AppRoute.Main.Sheet(AppRoute.Sheets.Wallet), + ) + + // Simulate: something pushed on top during dismiss, onBack removed that + // instead of the sheet, so the old sheet is still here. + // Now navigate to the same sheet again. + navigator.navigate( + AppRoute.Main.Sheet(AppRoute.Sheets.Wallet), + NavOptions(debugRouting = false), + ) + + val sheets = navigator.backStack.filterIsInstance() + assertEquals(1, sheets.size, "Expected exactly one Sheet on the backstack") + } + + @Test + fun `double navigateTo with pending dismiss does not produce duplicate sheets`() { + val navigator = createNavigator( + AppRoute.Main.Scanner, + AppRoute.Main.Sheet(AppRoute.Sheets.Wallet), + ) + + // First navigate sets pendingSheetDismiss + navigator.navigateTo(listOf(AppRoute.Sheets.Menu), options = quietOptions) + assertNotNull(navigator.pendingSheetDismiss) + + // Second navigate overwrites pendingSheetDismiss + navigator.navigateTo(listOf(AppRoute.Sheets.Menu), options = quietOptions) + + // Simulate: onBack removes old sheet, then callback fires + navigator.backStack.removeAt(navigator.backStack.lastIndex) + navigator.pendingSheetDismiss!!.invoke() + + // Simulate a stale callback also firing navigate for the same sheet + navigator.navigate( + AppRoute.Main.Sheet(AppRoute.Sheets.Menu), + NavOptions(debugRouting = false), + ) + + val sheets = navigator.backStack.filterIsInstance() + assertEquals(1, sheets.size, "Expected exactly one Sheet on the backstack") + } + // endregion } diff --git a/ui/navigation/src/main/kotlin/com/getcode/navigation/core/CodeNavigator.kt b/ui/navigation/src/main/kotlin/com/getcode/navigation/core/CodeNavigator.kt index 75e38e6f5..4fcf8405f 100644 --- a/ui/navigation/src/main/kotlin/com/getcode/navigation/core/CodeNavigator.kt +++ b/ui/navigation/src/main/kotlin/com/getcode/navigation/core/CodeNavigator.kt @@ -115,7 +115,7 @@ data class CodeNavigator( if (backStack.isNotEmpty()) backStack.removeAt(backStack.lastIndex) } if (route is Sheet) { - backStack.removeAll { it == route } + backStack.removeAll { it is Sheet && it.toString() == route.toString() } } backStack.add(route) } @@ -126,10 +126,10 @@ data class CodeNavigator( if (route is Sheet || currentRouteKey != route) { // Sheet routes must be unique on the backstack — // SaveableStateProvider uses contentKey (toString()) and - // crashes on duplicates. Remove any existing instance - // before pushing the new one. + // crashes on duplicates. Remove any entry whose contentKey + // (toString()) matches before pushing the new one. if (route is Sheet) { - backStack.removeAll { it == route } + backStack.removeAll { it is Sheet && it.toString() == route.toString() } } backStack.add(route) } @@ -139,7 +139,7 @@ data class CodeNavigator( Snapshot.withMutableSnapshot { if (route is Sheet || currentRouteKey != route) { if (route is Sheet) { - backStack.removeAll { it == route } + backStack.removeAll { it is Sheet && it.toString() == route.toString() } } backStack.add(route) val lastIndex = backStack.lastIndex - 1