From d13ee0f250c68f65bf6e451ecca1e38e5c4aea72 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Sat, 31 Oct 2026 17:20:07 -0400 Subject: [PATCH 1/2] fix(navigation): stop wrap-content sheets spinning in measure The host Invite sheet froze the app (Bugsnag 6ae543e9; 10 of 11 ANR dumps in a host's bugreport). Every main thread was inside compose-unstyled's Sheet measure, in updateContentHeight and shouldUseContentHeightForLayout. The wrap-content detent returned the sheetHeight compose-unstyled passes in, which puts the sheet in its content-height mode: measure writes the content's height into the sheet state and reads it back in the same pass. Scenes sit in a SharedTransitionLayout, so the sheet is measured twice a frame. The Invite sheet's Approve crossfade uses a SizeTransform, so it reports its target height to the lookahead pass and the animated height to the main pass. Each write invalidates the other, and the frame never ends. WrapContentDetent never reads sheetHeight. A layout modifier measures the content at its natural height (up to 92.5% of the window) and records it after layout, and the sheet's detents are re-resolved when it changes. Modal wrap-content sheets and InWindowBottomSheet both use it. WrapContentSheetLayoutLoopTest flips an AnimatedContent with a SizeTransform while a wrap-content sheet opens inside a SharedTransitionLayout. Before this change it spun at full CPU in the same frames as the device dumps; now it passes. --- .../scenes/WrapContentSheetLayoutLoopTest.kt | 115 ++++++++++++++++++ .../navigation/scenes/InWindowBottomSheet.kt | 13 +- .../scenes/ModalBottomSheetSceneStrategy.kt | 21 ++-- .../navigation/scenes/WrapContentDetent.kt | 85 +++++++++++++ 4 files changed, 217 insertions(+), 17 deletions(-) create mode 100644 ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt create mode 100644 ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt diff --git a/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt b/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt new file mode 100644 index 0000000000..e2de1c0903 --- /dev/null +++ b/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt @@ -0,0 +1,115 @@ +package com.getcode.navigation.scenes + +import androidx.compose.animation.AnimatedContent +import androidx.compose.animation.SharedTransitionLayout +import androidx.compose.animation.SizeTransform +import androidx.compose.animation.core.tween +import androidx.compose.animation.fadeIn +import androidx.compose.animation.fadeOut +import androidx.compose.animation.togetherWith +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height +import androidx.compose.runtime.CompositionLocalProvider +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue +import androidx.compose.ui.Modifier +import androidx.compose.ui.layout.layout +import androidx.compose.ui.test.junit4.createComposeRule +import androidx.compose.ui.unit.dp +import androidx.navigation3.runtime.NavEntry +import androidx.navigation3.runtime.NavKey +import com.getcode.navigation.AppHome +import com.getcode.navigation.DemoSheet +import com.getcode.navigation.NavMetadataKeys +import com.getcode.navigation.core.EmptyCodeNavigator +import com.getcode.navigation.core.LocalCodeNavigator +import com.getcode.navigation.scrim.LocalScrimController +import com.getcode.navigation.scrim.ScrimController +import com.getcode.navigation.testNavigator +import com.getcode.theme.DesignSystem +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * A wrap-content sheet whose content animates its height while the sheet is still opening. + * + * The app hosts every scene in a `SharedTransitionLayout`, a lookahead scope, so the sheet is + * measured twice a frame. `AnimatedContent` with a [SizeTransform] reports its target height to + * the lookahead pass and the animated height to the main pass, and compose-unstyled's `Sheet` + * writes each into its state during measure. While the sheet moves, the two writes keep + * invalidating each other and the frame never ends: the host Invite sheet's ANR, when a group's + * Approve New Members loads as on just after the sheet appears. + */ +@RunWith(RobolectricTestRunner::class) +class WrapContentSheetLayoutLoopTest { + + @get:Rule + val composeTestRule = createComposeRule() + + @Test + fun `content that animates its height while the sheet opens does not spin layout`() { + var tall by mutableStateOf(false) + var measures = 0 + val sheetEntry = NavEntry(key = DemoSheet) { + AnimatedContent( + targetState = tall, + modifier = Modifier + .fillMaxWidth() + .layout { measurable, constraints -> + // A spinning layout never returns to the test; fail out of it instead. + check(++measures < MeasureLimit) { "sheet layout did not settle" } + val placeable = measurable.measure(constraints) + layout(placeable.width, placeable.height) { placeable.place(0, 0) } + }, + transitionSpec = { + fadeIn(tween(300)) togetherWith fadeOut(tween(300)) using + SizeTransform(clip = false) { _, _ -> tween(300) } + }, + label = "test", + ) { isTall -> + Box(Modifier.fillMaxWidth().height(if (isTall) 240.dp else 160.dp)) + } + } + val baseEntry = NavEntry(key = AppHome) { Box(Modifier.fillMaxSize()) } + val scene = ModalBottomSheetScene( + key = DemoSheet, + previousEntries = listOf(baseEntry), + overlaidEntries = listOf(baseEntry), + entry = sheetEntry, + sheetProperties = BottomSheetProperties(), + onBack = {}, + metadata = mapOf(NavMetadataKeys.IsWrapContentSheet.key to true), + navResultStore = EmptyCodeNavigator.resultStore, + lastNavKey = { AppHome }, + ) + + composeTestRule.mainClock.autoAdvance = false + composeTestRule.setContent { + DesignSystem { + CompositionLocalProvider( + LocalCodeNavigator provides testNavigator(AppHome, DemoSheet), + LocalScrimController provides ScrimController(), + ) { + SharedTransitionLayout { scene.content() } + } + } + } + + // Partway up, the content learns it is taller, as the host sheet does when the group's + // Approve New Members setting arrives. + composeTestRule.mainClock.advanceTimeBy(64) + tall = true + repeat(60) { composeTestRule.mainClock.advanceTimeByFrame() } + composeTestRule.mainClock.autoAdvance = true + composeTestRule.waitForIdle() + } + + private companion object { + const val MeasureLimit = 5_000 + } +} diff --git a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/InWindowBottomSheet.kt b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/InWindowBottomSheet.kt index 7234527a52..ca673a410a 100644 --- a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/InWindowBottomSheet.kt +++ b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/InWindowBottomSheet.kt @@ -44,6 +44,8 @@ class InWindowSheetState internal constructor( ) { internal lateinit var sheet: BottomSheetState + internal val wrapContent = WrapContentDetent() + /** True once [hide] has been asked for: the caller closed it, so no dismiss request follows. */ internal var hidingByCaller by mutableStateOf(false) private set @@ -61,7 +63,7 @@ fun rememberInWindowSheetState(isLocked: () -> Boolean = { false }): InWindowShe val state = remember { InWindowSheetState { locked.value() } } state.sheet = rememberBottomSheetState( initialDetent = SheetDetent.Hidden, - detents = listOf(SheetDetent.Hidden, WrapContent), + detents = listOf(SheetDetent.Hidden, state.wrapContent.detent), confirmDetentChange = { detent -> detent != SheetDetent.Hidden || state.hidingByCaller || !state.isLocked() }, @@ -95,7 +97,8 @@ fun InWindowBottomSheet( val scope = rememberCoroutineScope() val currentOnDismiss by rememberUpdatedState(onDismissRequest) - LaunchedEffect(sheet) { sheet.animateTo(WrapContent) } + LaunchedEffect(sheet) { sheet.animateTo(state.wrapContent.detent) } + state.wrapContent.Track(sheet) // [UnstyledBottomSheet] has no dismissal callback: watch for it settling Hidden after it was // shown, the way the navigation sheets do, and ignore a close the caller asked for itself. @@ -119,7 +122,7 @@ fun InWindowBottomSheet( modifier = Modifier .fillMaxSize() .drawBehind { - val progress = sheet.progress(SheetDetent.Hidden, WrapContent).coerceIn(0f, 1f) + val progress = sheet.progress(SheetDetent.Hidden, state.wrapContent.detent).coerceIn(0f, 1f) drawRect(scrimColor.copy(alpha = scrimColor.alpha * progress)) } .then(if (dismissOnScrimTap) Modifier.noRippleClickable(onClick = requestClose) else { @@ -138,7 +141,9 @@ fun InWindowBottomSheet( .clip(RoundedCornerShape(topStart = 28.dp, topEnd = 28.dp)) .background(containerColor), ) { - content() + Box(with(state.wrapContent) { Modifier.fillMaxWidth().content() }) { + content() + } } } } diff --git a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/ModalBottomSheetSceneStrategy.kt b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/ModalBottomSheetSceneStrategy.kt index d50c80772c..ffb3b8a741 100644 --- a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/ModalBottomSheetSceneStrategy.kt +++ b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/ModalBottomSheetSceneStrategy.kt @@ -71,18 +71,6 @@ private val Expanded = SheetDetent("expanded") { containerHeight, _ -> containerHeight * 0.925f } -/** - * Resting detent for a [com.getcode.navigation.WrapContentSheet]: the height its content asked for, - * never more than [Expanded] would have given it. - * - * The detent is what sizes the sheet — compose-unstyled lays the sheet out to the detent height, not - * to the content — so dropping `fillMaxHeight()` from the sheet body is not enough on its own. This - * is the half of wrap-content that makes the sheet hug three lines instead of claiming the screen. - */ -internal val WrapContent = SheetDetent("wrap-content") { containerHeight, sheetHeight -> - sheetHeight.coerceAtMost(containerHeight * 0.925f) -} - /** * Resting — and, for short content, only — detent for a [com.getcode.navigation.HalfSheet]. * @@ -176,10 +164,13 @@ internal class ModalBottomSheetScene constructor( val isHalfSheet = metadata[NavMetadataKeys.IsHalfSheet.key] as? Boolean ?: false val isFullscreen = metadata[NavMetadataKeys.IsFullscreenSheet.key] as? Boolean ?: false + // Sized from the content's recorded height, never from the sheet's own measure; + // see [WrapContentDetent]. + val wrapContent = rememberWrapContentDetent() val restingDetent = when { isHalfSheet -> Half isFullscreen -> Fullscreen - isWrapContent -> WrapContent + isWrapContent -> wrapContent.detent else -> Expanded } @@ -205,6 +196,8 @@ internal class ModalBottomSheetScene constructor( }, ) + if (isWrapContent && !isHalfSheet) wrapContent.Track(sheetState) + // Offer the expanded detent only while there is something below the fold to expand into, // so a sheet that already fits can't be dragged up into empty space. Never withdraw it // while the sheet is sitting at (or heading toward) it. @@ -388,6 +381,8 @@ internal class ModalBottomSheetScene constructor( .then( if (!isWrapContent) { Modifier.fillMaxHeight() + } else if (!isHalfSheet) { + with(wrapContent) { Modifier.content() } } else Modifier ) // Every other sheet rests below the status bar and so never had diff --git a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt new file mode 100644 index 0000000000..af79ce188a --- /dev/null +++ b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt @@ -0,0 +1,85 @@ +package com.getcode.navigation.scenes + +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.Stable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.runtime.snapshotFlow +import androidx.compose.ui.Modifier +import androidx.compose.ui.layout.layout +import androidx.compose.ui.layout.onSizeChanged +import androidx.compose.ui.platform.LocalDensity +import androidx.compose.ui.platform.LocalWindowInfo +import androidx.compose.ui.unit.Dp +import androidx.compose.ui.unit.constrainHeight +import androidx.compose.ui.unit.isSpecified +import com.composeunstyled.BottomSheetState +import com.composeunstyled.SheetDetent +import kotlinx.coroutines.flow.drop +import kotlin.math.roundToInt + +/** The most of its container a wrap-content sheet may take: the same as [Expanded]. */ +internal const val WrapContentMaxFraction = 0.925f + +/** + * Resting detent for a sheet that hugs its content: the height the content asked for, never more + * than [WrapContentMaxFraction] of the container. + * + * compose-unstyled lays a sheet out to its detent height, not to the content. A detent that + * returns the `sheetHeight` the library hands it puts the sheet in the library's content-height + * mode, where the sheet's measure writes the content's height into its state and reads it back in + * the same pass. Every scene sits in a `SharedTransitionLayout`, so the sheet is measured twice a + * frame; content whose height animates (an `AnimatedContent` with a `SizeTransform`) reports one + * height to the lookahead pass and another to the main pass, each write invalidates the other, + * and the frame never finishes. That was the host Invite sheet's ANR. + * + * This detent never reads `sheetHeight`, so the library treats it as fixed. [content] records the + * content's own height after layout, and [Track] re-resolves the detents when it changes, a frame + * later rather than inside the measure pass. + */ +@Stable +internal class WrapContentDetent { + private var contentHeight by mutableStateOf(Dp.Unspecified) + + val detent = SheetDetent("wrap-content") { containerHeight, _ -> + val max = containerHeight * WrapContentMaxFraction + // Unmeasured, it opens toward the cap; the first layout pulls it in to the content. + if (contentHeight.isSpecified) contentHeight.coerceAtMost(max) else max + } + + /** + * Measures the sheet's content at its natural height, up to the cap, and records it. The + * content is laid out at the top of whatever height the sheet currently has, so it is clipped + * while the sheet catches up with it rather than squeezed. + */ + @Composable + fun Modifier.content(): Modifier { + val density = LocalDensity.current + val maxHeightPx = (LocalWindowInfo.current.containerSize.height * WrapContentMaxFraction) + .roundToInt() + return layout { measurable, constraints -> + val placeable = measurable.measure( + constraints.copy(minHeight = 0, maxHeight = maxHeightPx.coerceAtLeast(0)) + ) + layout(placeable.width, constraints.constrainHeight(placeable.height)) { + placeable.place(0, 0) + } + }.onSizeChanged { contentHeight = with(density) { it.height.toDp() } } + } + + /** Moves [state]'s anchors whenever the content's recorded height changes. */ + @Composable + fun Track(state: BottomSheetState) { + LaunchedEffect(state) { + snapshotFlow { contentHeight } + .drop(1) + .collect { state.invalidateDetents() } + } + } +} + +@Composable +internal fun rememberWrapContentDetent(): WrapContentDetent = remember { WrapContentDetent() } From ac1e4f3710bd8ddc781c0f3c6fbaa24af195e700 Mon Sep 17 00:00:00 2001 From: Brandon McAnsh Date: Sat, 31 Oct 2026 17:26:49 -0400 Subject: [PATCH 2/2] test(navigation): time out the sheet loop test instead of counting measures The loop spins inside the sheet's own measure, so the counter in the test's layout modifier never ran. A 10 minute test task timeout makes a regression fail. Also treat a zero content height as unmeasured. --- ui/navigation/build.gradle.kts | 8 ++++++++ .../scenes/WrapContentSheetLayoutLoopTest.kt | 18 ++++-------------- .../navigation/scenes/WrapContentDetent.kt | 8 ++++++-- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/ui/navigation/build.gradle.kts b/ui/navigation/build.gradle.kts index 27266d9bf8..08015a2ea7 100644 --- a/ui/navigation/build.gradle.kts +++ b/ui/navigation/build.gradle.kts @@ -1,3 +1,5 @@ +import java.time.Duration + plugins { alias(libs.plugins.flipcash.kmp.app.library) alias(libs.plugins.compose.compiler) @@ -59,3 +61,9 @@ dependencies { add("kspJvm", libs.dagger.compiler) add("jvmMainImplementation", libs.dagger) } + +// WrapContentSheetLayoutLoopTest fails by hanging: a sheet stuck in a measure loop never returns +// to the test. Cap the run so that shows up as a failure instead of eating the CI job. +tasks.withType().configureEach { + timeout.set(Duration.ofMinutes(10)) +} diff --git a/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt b/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt index e2de1c0903..c370240495 100644 --- a/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt +++ b/ui/navigation/src/androidHostTest/kotlin/com/getcode/navigation/scenes/WrapContentSheetLayoutLoopTest.kt @@ -16,7 +16,6 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier -import androidx.compose.ui.layout.layout import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.unit.dp import androidx.navigation3.runtime.NavEntry @@ -44,6 +43,9 @@ import org.robolectric.RobolectricTestRunner * writes each into its state during measure. While the sheet moves, the two writes keep * invalidating each other and the frame never ends: the host Invite sheet's ANR, when a group's * Approve New Members loads as on just after the sheet appears. + * + * A regression hangs rather than failing an assertion: the loop spins inside the sheet's own + * measure and never returns here. The module's test task timeout turns that into a failure. */ @RunWith(RobolectricTestRunner::class) class WrapContentSheetLayoutLoopTest { @@ -54,18 +56,10 @@ class WrapContentSheetLayoutLoopTest { @Test fun `content that animates its height while the sheet opens does not spin layout`() { var tall by mutableStateOf(false) - var measures = 0 val sheetEntry = NavEntry(key = DemoSheet) { AnimatedContent( targetState = tall, - modifier = Modifier - .fillMaxWidth() - .layout { measurable, constraints -> - // A spinning layout never returns to the test; fail out of it instead. - check(++measures < MeasureLimit) { "sheet layout did not settle" } - val placeable = measurable.measure(constraints) - layout(placeable.width, placeable.height) { placeable.place(0, 0) } - }, + modifier = Modifier.fillMaxWidth(), transitionSpec = { fadeIn(tween(300)) togetherWith fadeOut(tween(300)) using SizeTransform(clip = false) { _, _ -> tween(300) } @@ -108,8 +102,4 @@ class WrapContentSheetLayoutLoopTest { composeTestRule.mainClock.autoAdvance = true composeTestRule.waitForIdle() } - - private companion object { - const val MeasureLimit = 5_000 - } } diff --git a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt index af79ce188a..71ed4b3e1d 100644 --- a/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt +++ b/ui/navigation/src/androidMain/kotlin/com/getcode/navigation/scenes/WrapContentDetent.kt @@ -15,6 +15,7 @@ import androidx.compose.ui.platform.LocalDensity import androidx.compose.ui.platform.LocalWindowInfo import androidx.compose.ui.unit.Dp import androidx.compose.ui.unit.constrainHeight +import androidx.compose.ui.unit.dp import androidx.compose.ui.unit.isSpecified import com.composeunstyled.BottomSheetState import com.composeunstyled.SheetDetent @@ -46,8 +47,9 @@ internal class WrapContentDetent { val detent = SheetDetent("wrap-content") { containerHeight, _ -> val max = containerHeight * WrapContentMaxFraction - // Unmeasured, it opens toward the cap; the first layout pulls it in to the content. - if (contentHeight.isSpecified) contentHeight.coerceAtMost(max) else max + // Unmeasured (or measured against a window that had no size yet), it opens toward the + // cap; the first real layout pulls it in to the content. Zero would sit on Hidden. + if (contentHeight.isSpecified && contentHeight > 0.dp) contentHeight.coerceAtMost(max) else max } /** @@ -74,6 +76,8 @@ internal class WrapContentDetent { @Composable fun Track(state: BottomSheetState) { LaunchedEffect(state) { + // The first value is whatever the sheet's opening measure already resolved; only + // later changes need the anchors moved. snapshotFlow { contentHeight } .drop(1) .collect { state.invalidateDetents() }