Scope message cell ripple to the bubble shape#6425
Draft
Scope message cell ripple to the bubble shape#6425
Conversation
Contributor
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
WalkthroughThis PR adds passive ripple support, enables ripple indications on attachments and quoted messages, refactors MessageText pointerInput to selectively consume gestures only on interactive annotations, and adds tests for annotation handling and passive ripple behavior. ChangesVisual Feedback and Gesture Handling for Messages and Attachments
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
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
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.kt`:
- Around line 183-189: The Column's combinedClickable currently uses a no-op
onClick which swallows taps; replace that no-op by invoking the parent click
intent so bubble taps still trigger the MessageContainer thread-open behavior.
In MessageContent.kt update the combinedClickable onClick to call the parent
handler that's passed into this composable (e.g., invoke the onMessageClick /
onThreadOpen callback or forward the existing click lambda used by
MessageContainer) instead of {}, keeping onLongClick as
onLongItemClick(message).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d453df3f-1b17-4f76-9aaa-fe5ec0fdb5bc
📒 Files selected for processing (9)
stream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/FileAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/GiphyAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/LinkAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/PollMessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/messages/list/MessageContainer.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactory.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactoryParams.kt
andremion
force-pushed
the
fix/sdk-message-cell-ripple-scope
branch
2 times, most recently
from
25e0c5b to
93b3ed3
Compare
Contributor
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageText.kt (1)
184-184: ⚡ Quick win
isInteractiveAtas apointerInputkey restarts the gesture on every recomposition
isInteractiveAtis an inline lambda defined in the calling composable body that capturesannotations. Becauseannotationsis produced bystyledText.getStringAnnotations(...)withoutremember, it's a freshListinstance on every recomposition. Since Kotlin lambdas don't overrideequals,isInteractiveAtis always a new, inequal instance →pointerInputrestarts its coroutine on every recomposition, cancelling any in-flight gesture.The simplest fix is to remember
annotationsso that the captured reference is stable whenstyledTexthasn't changed:♻️ Proposed fix
In
MessageText, change line 107 from:- val annotations = styledText.getStringAnnotations(0, styledText.lastIndex) + val annotations = remember(styledText) { styledText.getStringAnnotations(0, styledText.lastIndex) }This makes
annotationsthe same instance across recompositions whilestyledTextis stable, stabilising the captured closure and preventing spuriouspointerInputrestarts.🤖 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 `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageText.kt` at line 184, The pointerInput coroutine is being restarted every recomposition because the inline lambda isInteractiveAt closes over a freshly created annotations list; stabilize this by remembering the annotations produced by styledText.getStringAnnotations(...) inside MessageText (use remember keyed on styledText) so the closure captures a stable List instance and pointerInput( onClick, onLongPress, isInteractiveAt ) no longer restarts unnecessarily; update the code that builds annotations to use remember(styledText) { styledText.getStringAnnotations(...) } and keep references to isInteractiveAt and pressIndicator as before.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.kt (1)
121-134: 🏗️ Heavy liftConsider migrating
passiveRipplefromModifier.composedtoModifier.Nodeto improve performance in message list rendering.The official Android Compose documentation confirms that
Modifier.Nodeis "the most performant way to create custom modifiers" and thatModifier.composedshould be reserved for cases requiring composable-style construction. SincepassiveRipple()is applied to every message bubble inMessageContentandPollMessageContent, migrating toModifier.Node(implementingPointerInputModifierNode+DelegatableNodefor coroutine scope and indication delegation) would eliminate recomposition overhead on every composition pass.This refactoring can be deferred pending the planned migration to Compose Foundation's
LinkAnnotationAPI (documented inMessageText.kt), but it's worth tracking as a follow-up performance improvement.🤖 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 `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.kt` around lines 121 - 134, The passiveRipple Modifier currently uses Modifier.composed which causes extra recompositions for every message bubble; refactor passiveRipple to implement a Modifier.Node-based solution: create a custom node implementing PointerInputModifierNode (or PointerInputNode) that owns a MutableInteractionSource and coroutine scope (use DelegatableNode to delegate indication behavior), move the pointerInput gesture logic (awaitEachGesture/awaitFirstDown/waitForUpOrCancellation and emitting PressInteraction events) into the node's pointer handling, and wire the ripple() indication via the node-based indication API so passiveRipple returns the new Node-based modifier instead of using Modifier.composed.
🤖 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
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageText.kt`:
- Around line 119-124: The range check uses an inclusive range with
AnnotatedString.Range.end (which is exclusive), causing an off-by-one: update
both the isInteractiveAt lambda and the onClick handler that iterate annotations
(referencing annotations.fastAny, AnnotationTagUrl, AnnotationTagEmail,
AnnotationTagMention and the ann.start/ann.end checks) to test membership using
an exclusive-end check (e.g., use ann.start until ann.end or an explicit
ann.start <= offset && offset < ann.end) so the character at ann.end is not
considered part of the annotation.
- Around line 184-207: The long-press path in the pointer handler is broken
because AwaitPointerEventScope.withTimeoutOrNull (used in pressIndicator inside
the Modifier.pointerInput lambda) returns null on timeout and never throws, so
the catch for PointerEventTimeoutCancellationException is unreachable; change
the call from withTimeoutOrNull(viewConfiguration.longPressTimeoutMillis) to
withTimeout(viewConfiguration.longPressTimeoutMillis) so the timeout throws and
the existing catch block calls onLongPress() and then consumeUntilUp(), and
remove the now-unused import kotlinx.coroutines.withTimeoutOrNull.
---
Nitpick comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageText.kt`:
- Line 184: The pointerInput coroutine is being restarted every recomposition
because the inline lambda isInteractiveAt closes over a freshly created
annotations list; stabilize this by remembering the annotations produced by
styledText.getStringAnnotations(...) inside MessageText (use remember keyed on
styledText) so the closure captures a stable List instance and pointerInput(
onClick, onLongPress, isInteractiveAt ) no longer restarts unnecessarily; update
the code that builds annotations to use remember(styledText) {
styledText.getStringAnnotations(...) } and keep references to isInteractiveAt
and pressIndicator as before.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.kt`:
- Around line 121-134: The passiveRipple Modifier currently uses
Modifier.composed which causes extra recompositions for every message bubble;
refactor passiveRipple to implement a Modifier.Node-based solution: create a
custom node implementing PointerInputModifierNode (or PointerInputNode) that
owns a MutableInteractionSource and coroutine scope (use DelegatableNode to
delegate indication behavior), move the pointerInput gesture logic
(awaitEachGesture/awaitFirstDown/waitForUpOrCancellation and emitting
PressInteraction events) into the node's pointer handling, and wire the ripple()
indication via the node-based indication API so passiveRipple returns the new
Node-based modifier instead of using Modifier.composed.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6d977287-0dbe-4398-820e-7cc1fb488c08
📒 Files selected for processing (8)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/FileAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/GiphyAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/LinkAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageText.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/PollMessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/messages/list/MessageContainer.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.kt
Contributor
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/FileAttachmentContent.kt (1)
107-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClip the file-item ripple to
fileAttachmentShape.The ripple effect bleeds outside the rounded file attachment bubble. In Compose,
background(shape)paints the shape but does not clip interactive effects—only.clip(shape)constrains ripple drawing to the shape boundary. Apply clipping before the ripple:Suggested fix
modifier = Modifier .fillMaxWidth() .applyIf(!shouldBeFullSize) { val color = MessageStyling.attachmentBackgroundColor(attachmentState.isMine) padding(MessageStyling.messageSectionPadding) + .clip(fileAttachmentShape) - .background(color, fileAttachmentShape) + .background(color) } .combinedClickable( indication = ripple(), interactionSource = remember { MutableInteractionSource() }, onClick = { onItemClick(previewHandlers, attachment) }, onLongClick = { attachmentState.onLongItemClick(message) }, ),🤖 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 `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/FileAttachmentContent.kt` around lines 107 - 119, The ripple is bleeding outside the rounded bubble because the modifier uses background(fileAttachmentShape) but doesn't clip the clickable ripple; update the modifier chain in FileAttachmentContent.kt (the block that builds the Modifier when !shouldBeFullSize) to call clip(fileAttachmentShape) before applying combinedClickable (i.e., ensure .clip(fileAttachmentShape) appears prior to .combinedClickable so the ripple is constrained), keeping the existing background, padding, and combinedClickable calls and referencing fileAttachmentShape, shouldBeFullSize, and the combinedClickable usage.
🤖 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.
Outside diff comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/FileAttachmentContent.kt`:
- Around line 107-119: The ripple is bleeding outside the rounded bubble because
the modifier uses background(fileAttachmentShape) but doesn't clip the clickable
ripple; update the modifier chain in FileAttachmentContent.kt (the block that
builds the Modifier when !shouldBeFullSize) to call clip(fileAttachmentShape)
before applying combinedClickable (i.e., ensure .clip(fileAttachmentShape)
appears prior to .combinedClickable so the ripple is constrained), keeping the
existing background, padding, and combinedClickable calls and referencing
fileAttachmentShape, shouldBeFullSize, and the combinedClickable usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3b5ebf80-73d4-4202-b04b-4d6df84a761b
📒 Files selected for processing (10)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/FileAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/GiphyAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/LinkAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageText.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/PollMessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/messages/list/MessageContainer.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/components/messages/MessageTextTest.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/util/PassiveRippleTest.kt
auto-merge was automatically disabled
May 8, 2026 15:43
Pull request was converted to draft
Image attachments already render a ripple on tap via their combinedClickable. Giphy, file rows, link previews and the quoted- message preview block had indication = null, leaving them without visual feedback. Match the image-attachment pattern across all four so every interactive surface inside a message bubble ripples consistently on press.
Wrap the message-content Column inside DefaultMessageRegularContent with combinedClickable + ripple(). The Column owns the click + ripple for the entire bubble interior (text, spacer, and any space around inner attachments). Inner attachment clickables (image, file, giphy, link, quoted) still consume their own taps and ripple in their own bounds. This replaces the earlier params-based bubble ripple (which had a position-translation issue between the cell's interaction source and the bubble's local coords). With the click and the ripple at the same layout node, press positions are captured in Column-local coords and the ripple renders correctly regardless of message alignment or bubble width.
Restore the ripple feedback on the bubble when the user long-presses in the avatar gap (or any cell area outside the bubble). The column-level clickable inside DefaultMessageRegularContent only fires for taps inside the bubble; cell-area presses go to MessageContainer's combinedClickable, which has indication = null. Without forwarding the cell's interaction source, those presses had no visual feedback. Add interactionSource to MessageBubbleParams and MessageContentParams. The cell hoists its MutableInteractionSource and threads it via MessageContainer -> factory.MessageContent -> DefaultMessageContent -> RegularMessageContent / PollMessageContent -> factory.MessageBubble. The MessageBubble factory default applies clip(shape).indication( source, ripple(bounded = false)) when the source is non-null, synchronised with the cell's press state. Two ripple paths now coexist by design: - Tap inside the bubble: column's combinedClickable consumes, column-level bounded ripple fires (position-aware). - Tap in avatar gap: cell's combinedClickable handles, the cell's source emits, bubble's params indication renders an unbounded ripple from the bubble centre. Both fire on the correct trigger; no double rippling thanks to gesture consumption rules.
The Column-level combinedClickable inside DefaultMessageRegularContent
and the inner PollMessageContent consumed taps with onClick = {} and
fired a raw onLongItemClick(message) without haptic. Two regressions
followed: tapping a thread-start message inside the bubble no longer
opened the thread (the cell's onClick was shadowed), and bubble
long-press lost the haptic feedback that the cell triggered for
avatar-gap presses.
Hoist onItemClick and onItemLongClick as named lambdas in
MessageContainer, both wrapping the canOpenThread / canOpenActions
gates and the haptic call. Plumb them through MessageContentParams,
MessageRegularContentParams, the public DefaultMessageContent /
RegularMessageContent / MessageContent / PollMessageContent
composables, and the factory defaults so the bubble Column can call
them directly.
This keeps canOpenActions in a single place (MessageContainer) and
removes the LocalHapticFeedback usage from MessageContent.kt and
PollMessageContent.kt: the bubble Column no longer needs to know
about action-permission rules or haptic policy. The inner private
PollMessageContent overload also drops its now-unused
onLongItemClick: (Message) -> Unit parameter.
The previous names (onItemClick / onItemLongClick) collided with the pre-existing onLongItemClick: (Message) -> Unit, which still exists for attachment routing. Reading MessageContentParams meant disambiguating three near-identical names with different shapes and purposes. Rename the new pair to onBubbleClick / onBubbleLongClick: the names now encode where the gesture happens and remove the word-swap collision. Add a one-line comment in MessageContainer near the hoisted lambdas explaining why they are extracted (shared with the bubble Column via MessageContentParams so in-bubble gestures mirror the cell).
Drop the onBubbleClick / onBubbleLongClick callback chain plus the interactionSource forwarding introduced earlier in this branch. The bubble Column now uses a single internal Modifier (passiveRipple) that renders a bounded position-aware ripple via a non-consuming pointerInput. The cell's combinedClickable stays as the single owner of click and long-click logic, and inner clickable children (attachments, quoted-reply previews) keep their own ripples — they are filtered out at the Column level via awaitFirstDown(requireUnconsumed = true), so they do not double-fire. passiveRipple is named after what it does (renders a ripple on press without claiming the gesture), not where it is used. It lives in ui/util and is reusable beyond message bubbles. Behaviour change: avatar-gap presses no longer render an unbounded ripple inside the bubble (the cell-source-driven indication on MessageBubble is removed). Cell click and long-press still fire there; only the visual feedback in that region is dropped, matching the WhatsApp pattern where the avatar gap is a dead zone visually. Net public API: MessageBubbleParams loses interactionSource; MessageContentParams and MessageRegularContentParams lose onBubbleClick, onBubbleLongClick, and interactionSource; DefaultMessageContent / RegularMessageContent / MessageContent / PollMessageContent lose the same trailing params. Surface shrinks back below the pre-attempt baseline.
The local ClickableText helper used detectTapGestures, which consumes the down event on every press inside the text. With the bubble Column's passiveRipple gating on awaitFirstDown(requireUnconsumed = true), that consumption blocked the bubble ripple AND the cell's combinedClickable for any tap on a message containing a link, mention, or email — even when the touched character was plain text. Replace detectTapGestures with a custom awaitEachGesture loop that only consumes when the down position lands on a character carrying an interactive annotation. Non-link character taps propagate to ancestors normally: the bubble ripples (passiveRipple sees unconsumed down) and the cell fires its onClick / onLongClick (thread-open, haptic, action menu). Tap and long-press on link characters keep their existing handlers via the same withTimeoutOrNull + waitForUpOrCancellation flow as detectTapGestures. Note: this is a workaround on top of the legacy string-annotation plumbing. The cleaner direction is migrating to Compose Foundation's LinkAnnotation API, which handles non-link tap propagation natively and would let us delete this entire custom detector. Tracked as a follow-up.
Two correctness fixes raised in PR review: - AnnotatedString.Range.end is exclusive, but the membership checks used ann.start..ann.end (inclusive). A tap on the character immediately after a link/mention/email was treated as part of the annotation. Switch both call sites to ann.start until ann.end. - The long-press branch used kotlinx.coroutines.withTimeoutOrNull, which returns null on timeout and never throws. The catch (_: PointerEventTimeoutCancellationException) block was unreachable, so onLongPress() was never invoked on long-press of a link/mention/email. Switch to AwaitPointerEventScope.withTimeout (the throwing variant matching Compose Foundation's own waitForLongPress) and drop the now-unused kotlinx.coroutines.withTimeoutOrNull import.
Pull two internal helpers out of inline lambdas in MessageText.kt: - AnnotatedString.Range<String>.isInteractiveTag() — true for URL, email, and mention tags. - List<AnnotatedString.Range<String>>.hasInteractiveAt(offset) — true when any range in the list both has an interactive tag and covers the given offset, with exclusive-end semantics matching AnnotatedString.Range.end. Replaces the inline lambdas in the public MessageText composable with member references at the call sites. Add MessageTextTest covering the predicate matrix: every interactive tag, non-interactive tags, empty list, inclusive-start and exclusive-end boundaries (locks in the recent off-by-one fix), and mixed annotation lists. Pure JUnit 5 + kluent, no Compose runtime.
Extract the click-dispatch logic out of MessageText.ClickableText's inline lambda into an internal `handleAnnotationClick` helper. Same behaviour with one tightening: the original code silently fell through to URL handling for any non-mention annotation (treating its `item` as a URL); the helper uses an explicit `when` over the three interactive tags (Mention, URL, Email) and ignores anything else. The bubble predicate already restricts the click handler to interactive positions, so the tightening only affects pathological input. Add tests: - PassiveRippleTest (Compose UI tests via createComposeRule + Robolectric) covers the four reachable branches of Modifier.passiveRipple(): tap propagates to outer combinedClickable, long-press propagates to outer onLongClick, an inner consuming clickable shields the parent, and drag-out-of-bounds exercises the Cancel branch without crashing. - MessageTextTest gains eight pure-JUnit cases for handleAnnotationClick covering URL with and without onLinkClick, email, mention with resolved user, mention with unknown username, position outside any annotation, non-interactive tag, and empty annotation item. Also drop the redundant `requireUnconsumed = true` arguments at the two awaitFirstDown call sites — `true` is the default. The named boolean rule applies when a non-default value is being passed.
andremion
force-pushed
the
fix/sdk-message-cell-ripple-scope
branch
from
e3357d0 to
5c61504
Compare
Apply audit findings from the gesture review: - handleAnnotationClick now filters firstOrNull by isInteractiveTag. Previously, when a non-interactive annotation (e.g. a custom decoration added through AnnotatedMessageTextBuilder) overlapped a URL/email/mention range, the first-overlapping non-interactive one was returned and the when fell through silently. The link never opened. Locked in by a new MessageTextHelpersTest case covering the overlap. - ClickableText's pointerInput now keys on Unit and reads its callbacks through rememberUpdatedState. The block is no longer cancelled and restarted each composition because the caller allocates fresh lambdas. - styledText.getStringAnnotations is wrapped in remember(styledText) so the list is not reallocated on every recomposition. - A small WHY comment is added on the two non-obvious gesture decisions in ClickableText (the early-return for non-interactive characters, and consumeUntilUp after long-press). Tests: - The previous MessageTextTest is renamed to MessageTextHelpersTest to free the canonical name for the new snapshot test, matching the codebase convention (e.g. ReactionsMenuContentTest). - MessageTextTest is the new Paparazzi snapshot suite. It covers plain text, URL, email, mention, and URL + mention scenarios. The fixtures live next to the production code as internal preview-friendly composables (MessageTextPlain, MessageTextWithUrl, ...) so the @Preview composables and the snapshot tests share the same definitions. The earlier audit also recommended Compose UI gesture tests (tap / long-press / drag-out). Those are not included: the gesture loop relies on withTimeout inside awaitPointerEventScope, which the Robolectric test environment does not drive reliably for this case. The behaviour stays under manual QA.
andremion
force-pushed
the
fix/sdk-message-cell-ripple-scope
branch
from
5c61504 to
9b0fd36
Compare
Contributor
Author
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.kt (1)
121-134: 💤 Low valueConsider migrating
passiveRippleto aModifier.Nodeimplementation for better list performance.
Modifier.composed { ... }creates subcomposition overhead and is no longer recommended for performance-sensitive code. Since this modifier is applied per message bubble across potentially long lists, migrating toModifier.Nodewould eliminate this cost. UseSuspendingPointerInputModifierNode(wrapped in aModifierNodeElement) to handle the gesture logic and emitPressInteractionevents, combined withModifier.indication(source, ripple())for the ripple effect. The Node-based pattern has been stable since Compose 1.5+ and is the modern approach. Composed still works, so this is deferable—happy to leave for a follow-up.🤖 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 `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.kt` around lines 121 - 134, The passiveRipple Modifier should be migrated from a composed-based implementation to a Node-based implementation to avoid subcomposition overhead: replace internal fun Modifier.passiveRipple(): Modifier = composed { ... } with a ModifierNodeElement that provides a SuspendingPointerInputModifierNode (or a custom Modifier.Node implementing PointerInputModifier) which runs the gesture loop (awaitEachGesture, awaitFirstDown, waitForUpOrCancellation) and emits PressInteraction events to a remembered MutableInteractionSource via coroutine scope; keep using Modifier.indication(source, ripple()) (or call indication from the node) to attach the ripple; ensure the node remembers the MutableInteractionSource and coroutine scope and properly launches source.emit(PressInteraction.Press/Release/Cancel) to replicate the existing behavior from passiveRipple.
🤖 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.
Nitpick comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.kt`:
- Around line 121-134: The passiveRipple Modifier should be migrated from a
composed-based implementation to a Node-based implementation to avoid
subcomposition overhead: replace internal fun Modifier.passiveRipple(): Modifier
= composed { ... } with a ModifierNodeElement that provides a
SuspendingPointerInputModifierNode (or a custom Modifier.Node implementing
PointerInputModifier) which runs the gesture loop (awaitEachGesture,
awaitFirstDown, waitForUpOrCancellation) and emits PressInteraction events to a
remembered MutableInteractionSource via coroutine scope; keep using
Modifier.indication(source, ripple()) (or call indication from the node) to
attach the ripple; ensure the node remembers the MutableInteractionSource and
coroutine scope and properly launches
source.emit(PressInteraction.Press/Release/Cancel) to replicate the existing
behavior from passiveRipple.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 34e831e7-35bd-475b-b31b-f47fc21595b8
⛔ Files ignored due to path filters (5)
stream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.components.messages_MessageTextTest_plain_text.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.components.messages_MessageTextTest_text_with_email.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.components.messages_MessageTextTest_text_with_mention.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.components.messages_MessageTextTest_text_with_url.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.components.messages_MessageTextTest_text_with_url_and_mention.pngis excluded by!**/*.png
📒 Files selected for processing (12)
stream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/FileAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/GiphyAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/LinkAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageText.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/PollMessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/messages/list/MessageContainer.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/components/messages/MessageTextHelpersTest.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/components/messages/MessageTextTest.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/util/PassiveRippleTest.kt
|
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.
Goal
Move the ripple from the message cell to the bubble while keeping the cell-wide hit area.
Implementation
Single click owner, decoupled passive ripple.
combinedClickablestays the only owner of click and long-press logic (thread-open, action menu, haptic, and thecanOpenThread/canOpenActionsgates). Itsindicationisnull— the cell never renders a ripple itself.Modifier.passiveRipple()(inModifierUtils.kt) is applied to the message-contentColumninsideDefaultMessageRegularContentand the innerPollMessageContent. It uses a non-consumingpointerInput { awaitEachGesture { ... } }to emitPressInteraction.Press/Release/Cancelinto a localMutableInteractionSourceand renders a bounded position-aware ripple viaModifier.indication(source, ripple()). Because it does not consume the gesture, the cell'scombinedClickablestill fires for in-bubble taps.awaitFirstDown(requireUnconsumed = true)makespassiveRippleskip when an inner clickable child (image, file, giphy, link, quoted-message preview) has already consumed the down. The inner region renders only its own ripple — no double-fire.Inner clickables. Giphy, file, link, and quoted-message previews now render bounded ripples inside their own content shapes, aligned with the image attachment that already had one.
Text-with-link bubbles. The local
ClickableTexthelper inMessageText.ktpreviously useddetectTapGestures, which consumes the down on every press inside the text — blocking bothpassiveRippleand the cell's click handler whenever a message contained a link/mention/email. Replaced with a customawaitEachGestureblock that only consumes when the down lands on a character carrying an interactive annotation. Non-link character taps now propagate normally: bubble ripples, cell fires itsonClick/onLongClick.Important
Decisions worth flagging for review
passiveRippleis named after what it does, not where it is used. Lives inui/utiland is reusable for any layout that wants visual press feedback while delegating click handling to a parent (or inner) clickable.onBubbleClick/onBubbleLongClickand aninteractionSourcethroughMessageContentParams,MessageRegularContentParams, and several public composables. All of that is gone — the cell owns click semantics, the Column owns ripple semantics, and the two are decoupled by Compose's gesture-consumption rules. Net public API surface is below the pre-attempt baseline.MessageText.ClickableTextis intentional. Compose Foundation'sLinkAnnotationAPI was evaluated as a replacement and rejected: in 1.9.x it has no long-press hook, and its innercombinedClickableconsumes the down — that would shield the cell's long-press without a clean way to layer one on top. The current detector keeps tap and long-press in a single state machine and stays naturally compatible with the cell-level long-press for non-link characters. The rejection note is recorded in the KDoc onClickableText.🎨 UI Changes
Screen_recording_20260507_165648.webm
Screen_recording_20260508_130005.webm
Testing
Summary by CodeRabbit