From fb6925ecb8b99f322b466cd7ecd9d0fb36e2e1e1 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Wed, 20 May 2026 09:40:39 -0400 Subject: [PATCH] fix(navigation): prevent duplicate Sheet key crash in single-route navigateTo The single-route navigateTo(route) extension called navigate() directly without checking whether a sheet was already open. This allowed a second Sheet(TokenDiscovery) to land on the backstack during a dismiss animation, crashing SaveableStateProvider with 'Key was used multiple times'. - Single-route navigateTo now defers via pendingSheetDismiss when a sheet is already present, matching the multi-route overload behavior - Add Sheet dedup guard to the ClearAll navigate branch - Add async duplicate Sheet sanitization in AppNavHost as a safety net - Add 4 test cases covering the single-route dismiss-then-replace flow --- .../app/core/extensions/CodeNavigator.kt | 14 +++- .../app/router/internal/NavigateToTest.kt | 64 +++++++++++++++++++ .../com/getcode/navigation/AppNavHost.kt | 26 ++++++++ .../getcode/navigation/core/CodeNavigator.kt | 3 + 4 files changed, 106 insertions(+), 1 deletion(-) 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 2d05dd168..ee224e920 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 @@ -16,7 +16,19 @@ fun CodeNavigator.navigateTo(route: NavKey, options: NavOptions = NavOptions()) } else { route } - navigate(destination, options) + val needsSheet = destination is AppRoute.Main.Sheet + val hasSheet = backStack.any { it is AppRoute.Main.Sheet } + + if (hasSheet && needsSheet) { + pendingSheetDismiss = { + Snapshot.withMutableSnapshot { + sheetGeneration++ + navigate(destination, options) + } + } + } else { + navigate(destination, options) + } } /** 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 1593b4ea9..169058851 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 @@ -277,4 +277,68 @@ class NavigateToTest { } // endregion + + // region Single-route navigateTo dismiss-then-replace + + @Test + fun `single-route navigateTo with existing sheet sets pendingSheetDismiss`() { + val navigator = createNavigator( + AppRoute.Main.Scanner, + AppRoute.Main.Sheet(AppRoute.Sheets.Wallet), + ) + + navigator.navigateTo(AppRoute.Sheets.TokenDiscovery, options = quietOptions) + + assertNotNull(navigator.pendingSheetDismiss) + // Backstack unchanged until the callback fires + assertEquals(2, navigator.backStack.size) + } + + @Test + fun `single-route navigateTo callback navigates to new sheet after dismiss`() { + val navigator = createNavigator( + AppRoute.Main.Scanner, + AppRoute.Main.Sheet(AppRoute.Sheets.Wallet), + ) + + navigator.navigateTo(AppRoute.Sheets.TokenDiscovery, options = quietOptions) + + // Simulate dismiss: remove old sheet entry, then callback fires + navigator.backStack.removeAt(navigator.backStack.lastIndex) + navigator.pendingSheetDismiss!!.invoke() + + val last = navigator.backStack.last() + assertIs(last) + assertEquals(AppRoute.Sheets.TokenDiscovery, last.initialRoute) + } + + @Test + fun `single-route navigateTo increments sheetGeneration on dismiss-replace`() { + val navigator = createNavigator( + AppRoute.Main.Scanner, + AppRoute.Main.Sheet(AppRoute.Sheets.Wallet), + ) + val initialGeneration = navigator.sheetGeneration + + navigator.navigateTo(AppRoute.Sheets.TokenDiscovery, options = quietOptions) + + // Simulate dismiss + navigator.backStack.removeAt(navigator.backStack.lastIndex) + navigator.pendingSheetDismiss!!.invoke() + + assertEquals(initialGeneration + 1, navigator.sheetGeneration) + } + + @Test + fun `single-route navigateTo without existing sheet navigates directly`() { + val navigator = createNavigator(AppRoute.Main.Scanner) + + navigator.navigateTo(AppRoute.Sheets.TokenDiscovery, options = quietOptions) + + assertNull(navigator.pendingSheetDismiss) + assertEquals(2, navigator.backStack.size) + assertIs(navigator.backStack.last()) + } + + // endregion } diff --git a/ui/navigation/src/main/kotlin/com/getcode/navigation/AppNavHost.kt b/ui/navigation/src/main/kotlin/com/getcode/navigation/AppNavHost.kt index af3bcb0c9..d3fa53b9f 100644 --- a/ui/navigation/src/main/kotlin/com/getcode/navigation/AppNavHost.kt +++ b/ui/navigation/src/main/kotlin/com/getcode/navigation/AppNavHost.kt @@ -17,6 +17,8 @@ import androidx.navigation3.scene.OverlayScene import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.snapshotFlow +import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.luminance import androidx.compose.ui.graphics.toArgb @@ -35,6 +37,7 @@ import com.getcode.navigation.decorators.rememberNavResultScopeEntryDecorator import com.getcode.navigation.results.NavResultStateRegistry import com.getcode.navigation.results.rememberNavResultStateRegistry import com.getcode.theme.CodeTheme +import timber.log.Timber @OptIn(ExperimentalMaterial3Api::class) @Composable @@ -60,6 +63,29 @@ fun AppNavHost( ) { ChangeSystemBarsTheme(CodeTheme.colors.background.luminance() < 0.5f) + // Safety net: async duplicate Sheet sanitization. + // Cannot prevent a same-frame crash, but cleans up residual duplicates + // from unforeseen race conditions before the next frame renders. + LaunchedEffect(navigator.backStack) { + snapshotFlow { navigator.backStack.toList() } + .collect { stack -> + val seen = mutableSetOf() + val toRemove = mutableListOf() + for (i in stack.lastIndex downTo 0) { + val entry = stack[i] + if (entry is Sheet && !seen.add(entry.toString())) { + toRemove.add(i) + } + } + if (toRemove.isNotEmpty()) { + Timber.w("Duplicate Sheet keys detected, sanitizing backstack") + Snapshot.withMutableSnapshot { + toRemove.forEach { navigator.backStack.removeAt(it) } + } + } + } + } + NavDisplay( backStack = navigator.backStack, onBack = onBack ?: { 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 4fcf8405f..12d1aa18e 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 @@ -99,6 +99,9 @@ data class CodeNavigator( NavOptions.PopUpTo.ClearAll -> { Snapshot.withMutableSnapshot { if (currentRouteKey != route) { + if (route is Sheet) { + backStack.removeAll { it is Sheet && it.toString() == route.toString() } + } backStack.add(route) // Remove all entries before the new one while (backStack.size > 1) {