Skip to content

Commit 3acbb80

Browse files
committed
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 <git@bmcreations.dev>
1 parent 1e0b336 commit 3acbb80

3 files changed

Lines changed: 62 additions & 9 deletions

File tree

apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/extensions/CodeNavigator.kt

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package com.flipcash.app.core.extensions
22

3+
import androidx.compose.runtime.snapshots.Snapshot
34
import androidx.navigation3.runtime.NavKey
45
import com.flipcash.app.core.AppRoute
56
import com.getcode.navigation.core.CodeNavigator
@@ -39,10 +40,12 @@ fun CodeNavigator.navigateTo(routes: List<NavKey>, options: NavOptions = NavOpti
3940
// The callback is invoked by ModalBottomSheetScene after the dismiss
4041
// animation completes and the old entry is removed from the backstack.
4142
pendingSheetDismiss = {
42-
sheetGeneration++
43-
resolved.forEachIndexed { index, route ->
44-
val navOptions = if (index == 0) options else NavOptions()
45-
navigate(route, navOptions)
43+
Snapshot.withMutableSnapshot {
44+
sheetGeneration++
45+
resolved.forEachIndexed { index, route ->
46+
val navOptions = if (index == 0) options else NavOptions()
47+
navigate(route, navOptions)
48+
}
4649
}
4750
}
4851
} else {

apps/flipcash/shared/router/src/test/kotlin/com/flipcash/app/router/internal/NavigateToTest.kt

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,5 +226,55 @@ class NavigateToTest {
226226
}
227227
}
228228

229+
@Test
230+
fun `navigate deduplicates identical sheet that was not removed by onBack`() {
231+
// Reproduces the production crash: a stale Sheet(Wallet,[]) remains on the
232+
// backstack after onBack removed the wrong entry during a dismiss animation.
233+
// A subsequent navigate for the same Sheet must not produce a duplicate.
234+
val navigator = createNavigator(
235+
AppRoute.Main.Scanner,
236+
AppRoute.Main.Sheet(AppRoute.Sheets.Wallet),
237+
)
238+
239+
// Simulate: something pushed on top during dismiss, onBack removed that
240+
// instead of the sheet, so the old sheet is still here.
241+
// Now navigate to the same sheet again.
242+
navigator.navigate(
243+
AppRoute.Main.Sheet(AppRoute.Sheets.Wallet),
244+
NavOptions(debugRouting = false),
245+
)
246+
247+
val sheets = navigator.backStack.filterIsInstance<AppRoute.Main.Sheet>()
248+
assertEquals(1, sheets.size, "Expected exactly one Sheet on the backstack")
249+
}
250+
251+
@Test
252+
fun `double navigateTo with pending dismiss does not produce duplicate sheets`() {
253+
val navigator = createNavigator(
254+
AppRoute.Main.Scanner,
255+
AppRoute.Main.Sheet(AppRoute.Sheets.Wallet),
256+
)
257+
258+
// First navigate sets pendingSheetDismiss
259+
navigator.navigateTo(listOf(AppRoute.Sheets.Menu), options = quietOptions)
260+
assertNotNull(navigator.pendingSheetDismiss)
261+
262+
// Second navigate overwrites pendingSheetDismiss
263+
navigator.navigateTo(listOf(AppRoute.Sheets.Menu), options = quietOptions)
264+
265+
// Simulate: onBack removes old sheet, then callback fires
266+
navigator.backStack.removeAt(navigator.backStack.lastIndex)
267+
navigator.pendingSheetDismiss!!.invoke()
268+
269+
// Simulate a stale callback also firing navigate for the same sheet
270+
navigator.navigate(
271+
AppRoute.Main.Sheet(AppRoute.Sheets.Menu),
272+
NavOptions(debugRouting = false),
273+
)
274+
275+
val sheets = navigator.backStack.filterIsInstance<AppRoute.Main.Sheet>()
276+
assertEquals(1, sheets.size, "Expected exactly one Sheet on the backstack")
277+
}
278+
229279
// endregion
230280
}

ui/navigation/src/main/kotlin/com/getcode/navigation/core/CodeNavigator.kt

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ data class CodeNavigator(
115115
if (backStack.isNotEmpty()) backStack.removeAt(backStack.lastIndex)
116116
}
117117
if (route is Sheet) {
118-
backStack.removeAll { it == route }
118+
backStack.removeAll { it is Sheet && it.toString() == route.toString() }
119119
}
120120
backStack.add(route)
121121
}
@@ -126,10 +126,10 @@ data class CodeNavigator(
126126
if (route is Sheet || currentRouteKey != route) {
127127
// Sheet routes must be unique on the backstack —
128128
// SaveableStateProvider uses contentKey (toString()) and
129-
// crashes on duplicates. Remove any existing instance
130-
// before pushing the new one.
129+
// crashes on duplicates. Remove any entry whose contentKey
130+
// (toString()) matches before pushing the new one.
131131
if (route is Sheet) {
132-
backStack.removeAll { it == route }
132+
backStack.removeAll { it is Sheet && it.toString() == route.toString() }
133133
}
134134
backStack.add(route)
135135
}
@@ -139,7 +139,7 @@ data class CodeNavigator(
139139
Snapshot.withMutableSnapshot {
140140
if (route is Sheet || currentRouteKey != route) {
141141
if (route is Sheet) {
142-
backStack.removeAll { it == route }
142+
backStack.removeAll { it is Sheet && it.toString() == route.toString() }
143143
}
144144
backStack.add(route)
145145
val lastIndex = backStack.lastIndex - 1

0 commit comments

Comments
 (0)