Changed trailingIcon type from val to var #811
Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix an issue where multiple stackable snackbars don't display the close icon (trailing icon) by changing the trailingIcon property from val to var in the SnackBarItemModel data class. Additionally, the code now uses non-null assertion operators (!!) when accessing the trailingIcon property.
Key changes:
- Changed
trailingIconfrom immutable (val) to mutable (var) inSnackBarItemModel - Added non-null assertion operators (
!!) to alltrailingIconaccesses in the rendering logic
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val style: SnackbarStyle = SnackbarStyle.Neutral, | ||
| val leadingIcon: FluentIcon? = null, | ||
| val trailingIcon: FluentIcon? = null, | ||
| var trailingIcon: FluentIcon? = null, |
There was a problem hiding this comment.
Changing trailingIcon from val to var in a data class marked with @Stable is problematic. The @Stable annotation tells Compose that the object is immutable, but introducing a mutable property violates this contract. This can lead to recomposition issues where changes to the trailingIcon won't trigger proper UI updates. Consider creating a new instance of the model with the updated trailingIcon using the data class copy() method instead of mutating the property.
| var trailingIcon: FluentIcon? = null, | |
| val trailingIcon: FluentIcon? = null, |
| } | ||
|
|
||
| if (model.trailingIcon != null && model.trailingIcon.isIconAvailable()) { | ||
| if (model.trailingIcon != null && model.trailingIcon!!.isIconAvailable()) { |
There was a problem hiding this comment.
The non-null assertion operator !! is used here despite the null check immediately before. This is redundant and risky. Consider using smart casting by extracting the trailingIcon to a local variable after the null check, or use a safe call operator with let block to handle the non-null case.
| if (model.trailingIcon!!.onClick != null) { | ||
| Modifier.clickable( | ||
| interactionSource = remember { MutableInteractionSource() }, | ||
| indication = rememberRipple(), | ||
| enabled = true, | ||
| role = Role.Image, | ||
| onClick = model.trailingIcon.onClick!! | ||
| onClick = model.trailingIcon!!.onClick!! | ||
| ) | ||
| } else Modifier | ||
| ) | ||
| ) { | ||
| Icon( | ||
| model.trailingIcon, | ||
| model.trailingIcon!!, |
There was a problem hiding this comment.
Multiple non-null assertion operators !! are used here and in lines 801 and 807. Since trailingIcon was already verified as non-null in line 790, Kotlin smart casting should work with a local variable. Refactor this code block to use a local variable or a let block to avoid the repeated use of !!.
3 participants
Problem: When we create multiple stackable snackbars, the trailing ones don't have the cross sign for closing it
Root cause: Trailing icon is immutable
Fix: Changed trailing icon from val to var
Validations
Build succeeded and behaviour is now correct