fix(ui): prevent auto-animate from overriding user-initiated sheet animations#3740
Open
kairosci wants to merge 1 commit into
Open
fix(ui): prevent auto-animate from overriding user-initiated sheet animations#3740kairosci wants to merge 1 commit into
kairosci wants to merge 1 commit into
Conversation
Contributor
📝 WalkthroughWalkthroughCollapse, initial anchor animation, and drag snap now coerce computed targets into the animatable's current bounds. Initial animateTo and drag snap calls are wrapped to rethrow CancellationException but ignore other animation errors. ChangesBottomSheet bounds-coercion & resilient animations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt`:
- Around line 345-354: The race is caused by incrementing
activeAnimations.intValue inside the coroutine body of the launchAnimation
lambda so recompositions can see 0 before the coroutine starts; move the
increment so it happens synchronously before coroutineScope.launch is invoked
(keep the decrement in the finally block inside the launched coroutine), i.e.
update the launchAnimation closure that references activeAnimations.intValue and
coroutineScope.launch so it increments activeAnimations.intValue immediately
prior to calling coroutineScope.launch (while leaving the existing try/finally
decrement inside the launched coroutine), ensuring calls from
state.expandSoft()/collapseSoft()/dismiss() are observed by the remember(...)
logic that checks activeAnimations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bcb241f7-1287-4e44-bf77-6f8de35670dd
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt
The previous approach used a workaround (activeAnimations counter) to prevent the remember block from overriding user animations. However, the true root cause of the queue sheet becoming permanently unresponsive (until app restart) was the CoroutineScope getting cancelled. During layout passes, the bounds could momentarily collapse (e.g., expandedBound < collapsedBound). When the remember block triggered animatable.animateTo() with an out-of-bounds initialValue, it threw an IllegalArgumentException. Since this exception was unhandled, it propagated and killed the rememberCoroutineScope, silently breaking all future interactions. This commit resolves the root cause by clamping the target values within the Animatable's bounds and catching non-cancellation exceptions to keep the CoroutineScope alive. It also removes the activeAnimations workaround, which caused regressions by ignoring valid layout updates during active animations and manual drags.
kairosci
force-pushed
the
feat/queue-not-opening
branch
from
85acddd to
aa989ba
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt (2)
364-368: ⚡ Quick winAdd logging for ignored snapTo exceptions.
Same concern as the
animateToexception handling: silently ignoring non-cancellation exceptions during drag gestures makes debugging difficult.📝 Proposed enhancement: log ignored exceptions
+import android.util.Log + +private const val TAG = "BottomSheet" + // ... in DraggableState try { animatable.snapTo(target) } catch (e: Exception) { if (e is kotlinx.coroutines.CancellationException) throw e + Log.w(TAG, "Drag snap animation failed, ignoring to prevent scope death", e) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt` around lines 364 - 368, The catch block around animatable.snapTo in BottomSheet.kt currently swallows all non-cancellation exceptions; update the catch to rethrow kotlinx.coroutines.CancellationException but log any other Exception (include the exception message and stacktrace) before continuing. Specifically, in the try/catch that calls animatable.snapTo(target) inside the BottomSheet composable/drag handler, keep the "if (e is CancellationException) throw e" check and add a logging call (e.g., Log.e or the project's logger/Timber) such as Log.e("BottomSheet", "snapTo failed during drag", e) to record the ignored exception. Ensure you import the logger you use.
351-356: ⚡ Quick winAdd logging for ignored animation exceptions.
While the CancellationException-preserving pattern is correct and consistent with the codebase, silently ignoring non-cancellation exceptions could hide bugs and make debugging difficult. Consider logging ignored exceptions.
📝 Proposed enhancement: log ignored exceptions
+import android.util.Log + +private const val TAG = "BottomSheet" + // ... in rememberBottomSheetState coroutineScope.launch { try { animatable.animateTo(targetValue, NavigationBarAnimationSpec) } catch (e: Exception) { if (e is kotlinx.coroutines.CancellationException) throw e - // Ignore unexpected animation errors to prevent coroutine scope death + Log.w(TAG, "Initial anchor animation failed, ignoring to prevent scope death", e) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt` around lines 351 - 356, The catch block around animatable.animateTo(targetValue, NavigationBarAnimationSpec) currently swallows non-cancellation exceptions; update it to log the caught exception (while still rethrowing kotlinx.coroutines.CancellationException) so unexpected animation errors are recorded for debugging—add a warning/error log call (e.g., via Android Log/Timber or the project logger) inside the catch branch for non-CancellationException cases and include the exception and a short contextual message mentioning animatable.animateTo/NavigationBarAnimationSpec.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt`:
- Around line 340-341: The current logic uses val lowerBound =
dismissedBound.coerceAtMost(expandedBound) which can invert semantics when
expandedBound < dismissedBound; change this to use the dismissed bound directly
and call animatable.updateBounds(dismissedBound, expandedBound) so the lower
bound always represents the dismissed (bottom) position, or alternatively add an
explicit validation that throws/ logs if expandedBound < dismissedBound before
calling animatable.updateBounds to fail fast; update references: dismissedBound,
expandedBound, lowerBound and animatable.updateBounds accordingly.
- Around line 186-187: The call to Dp.coerceIn uses animatable.lowerBound and
animatable.upperBound which are nullable (Dp?), so update the expression to
mirror the class's existing pattern by force-unwrapping the bounds (use
animatable.lowerBound!! and animatable.upperBound!!) when computing target
before calling animatable.animateTo(collapsedBound.coerceIn(...), animationSpec)
so the non-nullable overload of coerceIn is used; reference symbols:
collapsedBound, animatable, lowerBound, upperBound, coerceIn, animateTo.
---
Nitpick comments:
In `@app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt`:
- Around line 364-368: The catch block around animatable.snapTo in
BottomSheet.kt currently swallows all non-cancellation exceptions; update the
catch to rethrow kotlinx.coroutines.CancellationException but log any other
Exception (include the exception message and stacktrace) before continuing.
Specifically, in the try/catch that calls animatable.snapTo(target) inside the
BottomSheet composable/drag handler, keep the "if (e is CancellationException)
throw e" check and add a logging call (e.g., Log.e or the project's
logger/Timber) such as Log.e("BottomSheet", "snapTo failed during drag", e) to
record the ignored exception. Ensure you import the logger you use.
- Around line 351-356: The catch block around animatable.animateTo(targetValue,
NavigationBarAnimationSpec) currently swallows non-cancellation exceptions;
update it to log the caught exception (while still rethrowing
kotlinx.coroutines.CancellationException) so unexpected animation errors are
recorded for debugging—add a warning/error log call (e.g., via Android
Log/Timber or the project logger) inside the catch branch for
non-CancellationException cases and include the exception and a short contextual
message mentioning animatable.animateTo/NavigationBarAnimationSpec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c62c2f9-f6d0-4724-89c5-7a188c703a9e
📒 Files selected for processing (1)
app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt
Comment on lines
+186
to
+187
| val target = collapsedBound.coerceIn(animatable.lowerBound, animatable.upperBound) | ||
| animatable.animateTo(target, animationSpec) |
Contributor
There was a problem hiding this comment.
Verify nullable bounds handling in coerceIn.
animatable.lowerBound and animatable.upperBound are Dp? (nullable), but Dp.coerceIn expects non-null parameters. The existing pattern in this class (lines 160, 163) uses !! to force-unwrap when accessing bounds. Either the nullable bounds should be force-unwrapped here, or there's a special API overload that handles nullables.
While the PR states the build succeeds, this appears to be a type mismatch that warrants verification.
Run the following script to verify the Compose Animatable API for coerceIn with nullable bounds:
#!/bin/bash
# Description: Check how lowerBound/upperBound are used with coerceIn in the codebase
# Check the pattern of accessing animatable bounds
rg -n -C3 'animatable\.(lowerBound|upperBound)' --type=ktDoes Compose Animatable's Dp.coerceIn method accept nullable Dp parameters for minimumValue and maximumValue?
🛡️ Proposed fix if force-unwrap is needed
- val target = collapsedBound.coerceIn(animatable.lowerBound, animatable.upperBound)
+ val target = collapsedBound.coerceIn(animatable.lowerBound!!, animatable.upperBound!!)
animatable.animateTo(target, animationSpec)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt` around
lines 186 - 187, The call to Dp.coerceIn uses animatable.lowerBound and
animatable.upperBound which are nullable (Dp?), so update the expression to
mirror the class's existing pattern by force-unwrapping the bounds (use
animatable.lowerBound!! and animatable.upperBound!!) when computing target
before calling animatable.animateTo(collapsedBound.coerceIn(...), animationSpec)
so the non-nullable overload of coerceIn is used; reference symbols:
collapsedBound, animatable, lowerBound, upperBound, coerceIn, animateTo.
Comment on lines
+340
to
+341
| val lowerBound = dismissedBound.coerceAtMost(expandedBound) | ||
| animatable.updateBounds(lowerBound, expandedBound) |
Contributor
There was a problem hiding this comment.
Logic error: coerceAtMost produces semantically incorrect lowerBound when bounds are inverted.
If expandedBound < dismissedBound (invalid configuration), this code sets lowerBound = expandedBound, meaning the "lower" bound would be the expanded position—semantically backwards. The animatable's lower bound should represent the dismissed (bottom) position, not the expanded (top) position.
🔧 Proposed fix: use dismissed bound directly or validate configuration
Option 1 (preferred): Use dismissedBound directly as lower bound
- val lowerBound = dismissedBound.coerceAtMost(expandedBound)
- animatable.updateBounds(lowerBound, expandedBound)
+ animatable.updateBounds(dismissedBound, expandedBound)Option 2: Validate configuration and fail fast
+ require(dismissedBound <= expandedBound) {
+ "Invalid BottomSheet bounds: dismissedBound ($dismissedBound) must be <= expandedBound ($expandedBound)"
+ }
val lowerBound = dismissedBound.coerceAtMost(expandedBound)
animatable.updateBounds(lowerBound, expandedBound)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| val lowerBound = dismissedBound.coerceAtMost(expandedBound) | |
| animatable.updateBounds(lowerBound, expandedBound) | |
| animatable.updateBounds(dismissedBound, expandedBound) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/kotlin/com/metrolist/music/ui/component/BottomSheet.kt` around
lines 340 - 341, The current logic uses val lowerBound =
dismissedBound.coerceAtMost(expandedBound) which can invert semantics when
expandedBound < dismissedBound; change this to use the dismissed bound directly
and call animatable.updateBounds(dismissedBound, expandedBound) so the lower
bound always represents the dismissed (bottom) position, or alternatively add an
explicit validation that throws/ logs if expandedBound < dismissedBound before
calling animatable.updateBounds to fail fast; update references: dismissedBound,
expandedBound, lowerBound and animatable.updateBounds accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The queue sheet occasionally becomes unresponsive — tapping the queue button shows the tap animation but the queue panel does not open. The issue appears randomly during normal usage and requires a full app restart to recover.
Cause
The
remember(dismissedBound, expandedBound, collapsedBound, coroutineScope)block inrememberBottomSheetStatelaunchesanimatable.animateTo(initialValue, NavigationBarAnimationSpec)as a side effect every time any key changes. The queue sheet'sexpandedBoundkey tracksstate.expandedBound(the Player sheet'sanimatable.upperBound!!) anddismissedBounddepends onWindowInsets.systemBars.asPaddingValues().calculateBottomPadding(). When these values fluctuate during normal usage (inset changes, player bound updates), the auto-animate cancels any in-progress user animation fromexpandSoft()mid-flight, creating a race condition that can leave the sheet in a broken state.Solution
activeAnimationscounter (MutableIntState) that tracks whether user-initiated expand/collapse/dismiss animations are in progresslaunchAnimationwrapper that increments/decrements the counter around all user-initiated animation coroutinesrememberblock's auto-animate now checksactiveAnimations == 0before launching — if the user is actively animating, the auto-animate is skipped and the user's animation continues uninterruptedlaunchAnimationtoBottomSheetStateas a constructor parameter (with a safe default for backward compatibility)Testing
./gradlew :app:assembleFossDebugRelated Issues
Summary by CodeRabbit
Refactor
Bug Fixes