Scope message cell ripple to the bubble shape#6425
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a passive ripple modifier, enables ripple indications on attachments and quoted messages, refactors MessageText pointerInput to selectively consume gestures only on interactive annotations, centralizes haptic long-press handling, and adds tests plus an API listing update. ChangesVisual Feedback and Gesture Handling for Messages and Attachments
🎯 3 (Moderate) | ⏱️ ~20 minutes 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 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 |
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
25e0c5b to
93b3ed3
Compare
|
@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
|
@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
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.
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.
5c61504 to
9b0fd36
Compare
|
@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
ClickableText's onLongPress callback used to call onLongItemClick(message) without haptic feedback. The cell's combinedClickable.onLongClick — which handles long-press for every other region — does fire haptic. Long-press on a link / mention / email character was the one path where the action menu opened silently, because the cell is shielded there (ClickableText consumes the down). Capture LocalHapticFeedback at the MessageText composable level and include the same haptic call in the lambda passed to ClickableText, so the user feels the press acknowledgement regardless of which character they long-press.
The cell and ClickableText each fired HapticFeedbackType.LongPress before calling the upstream onLongItemClick. Attachments (image / file / giphy / link / quoted-reply preview) did not, so long-press on those felt different. "Long-press anywhere in a message cell fires haptic before the upstream handler" is one rule; restating it at every leaf is duplication and easy to forget when adding a new clickable. Wrap the onLongItemClick callback once at the top of MessageContainer via a small rememberHapticLongClick helper. The wrapped callback is what flows down through every existing parameter slot — cell combinedClickable, MessageText.ClickableText, attachment combinedClickables (via AttachmentState), and the quoted-reply preview — so every leaf calls onLongItemClick(message) and gets the haptic transparently. The cell's canOpenActions gate still works because the gate runs before the wrapped call: a long-press on a deleted or uploading message neither fires haptic nor dispatches. Net: one helper, one wrapping line, no duplicated haptic calls in the leaf gesture handlers. Attachments now have parity with text and the rest of the cell. No public API change (the helper is private and the callback contract from outside the cell is unchanged).
|
@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)
109-119:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRipple may bleed past the rounded background corners.
When
!shouldBeFullSize, the modifier chain appliesbackground(color, fileAttachmentShape)but never clips the layout to that shape, so the newly enabled ripple will animate as a rectangle and visually overflow the rounded corners. The other attachment renderers in this PR (Giphy, Link) explicitly.clip(RoundedCornerShape(...))beforecombinedClickablefor exactly this reason.🎨 Proposed fix to clip the ripple to the rounded shape
modifier = Modifier .fillMaxWidth() .applyIf(!shouldBeFullSize) { val color = MessageStyling.attachmentBackgroundColor(attachmentState.isMine) padding(MessageStyling.messageSectionPadding) + .clip(fileAttachmentShape) .background(color, fileAttachmentShape) } .combinedClickable( indication = ripple(),🤖 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 109 - 119, The ripple is overflowing the rounded background because the modifier applies background(fileAttachmentShape) but doesn't clip; in FileAttachmentContent (where shouldBeFullSize is checked) add a clip using the same fileAttachmentShape before the combinedClickable so the ripple is constrained to the rounded corners (i.e., when !shouldBeFullSize, apply padding(...).background(color, fileAttachmentShape).clip(fileAttachmentShape) prior to combinedClickable that calls onItemClick and attachmentState.onLongItemClick).
🧹 Nitpick comments (3)
stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/util/PassiveRippleTest.kt (2)
151-167: 💤 Low valueConsider asserting cleanup state in addition to no-crash.
The test verifies the gesture completes without crashing, which is valuable. An optional strengthening would be to attach a
MutableInteractionSourceyou control to thepassiveRipple(via a small testable overload, or by reading from the indication chain) and assert that aPressInteraction.Cancelis emitted when the pointer leaves bounds — that would lock in the cancel-vs-release branch as a behavior contract.🤖 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/test/kotlin/io/getstream/chat/android/compose/ui/util/PassiveRippleTest.kt` around lines 151 - 167, Update the test in PassiveRippleTest (`drag out of bounds during gesture does not crash`) to also assert cleanup by supplying a controllable MutableInteractionSource to the passiveRipple and verifying it receives a PressInteraction.Cancel when the pointer is moved out of bounds; specifically, create a MutableInteractionSource, pass it into passiveRipple (or use a testable overload/wrapper), perform the same touch sequence via composeTestRule.performTouchInput, then collect emissions from the interaction source and assert that a PressInteraction.Cancel was emitted (and no PressInteraction.Release), ensuring the cancel branch is validated.
112-149: 💤 Low valueGood coverage of the "consumed-down" branch.
This test confirms that an inner
combinedClickableconsumes the down so the outer combinedClickable does not fire — and implicitly that the parent'spassiveRipple()(which usesawaitFirstDown(requireUnconsumed = true)) won't emit a press for that gesture either. Consider adding a complementary assertion (or a separate test) that verifies the parent'spassiveRippleinteraction source doesn't emit aPressInteractionin this scenario, since the propagation rule is the main contract beyond stock combinedClickable behavior.🤖 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/test/kotlin/io/getstream/chat/android/compose/ui/util/PassiveRippleTest.kt` around lines 112 - 149, Add an assertion that the parent's passiveRipple interaction source emits no PressInteraction when the inner clickable consumes the down: create a MutableInteractionSource instance (e.g., parentInteractionSource = MutableInteractionSource()), pass it into the outer Modifier.passiveRipple(interactionSource = parentInteractionSource) so the test can observe interactions, perform the same touch on the "consumer" node, and then collect/inspect parentInteractionSource.interactions (or snapshot its events) to verify no PressInteraction was emitted; update PassiveRippleTest (the `inner consuming clickable does not trigger outer combinedClickable` test) to include this check after performing the touch.stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/components/messages/MessageTextTest.kt (1)
35-58: 🏗️ Heavy liftAdd one interaction test for the new pointer-input contract.
These snapshots lock down rendering, but the risky change in this PR is gesture routing: non-interactive presses should bubble to the cell, while interactive presses/long-presses must be consumed locally. A small compose interaction test around
MessageTextwould cover the regression surface that snapshots and helper-unit tests miss.🤖 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/test/kotlin/io/getstream/chat/android/compose/ui/components/messages/MessageTextTest.kt` around lines 35 - 58, Add a Compose interaction test in MessageTextTest (next to existing tests like `plain text`, `text with url`, etc.) that exercises the pointer-input contract on the MessageText composable(s) (e.g., MessageTextPlain or MessageTextWithUrl): use the Compose testing APIs to perform a tap and a long-press on the MessageText node and assert that interactive targets (links/mentions) consume the gestures locally while non-interactive areas allow the gesture to bubble (e.g., to the surrounding cell). Name the new test clearly (e.g., `pointer input contract`) and reuse the existing snapshot scaffolding (`snapshotWithDarkModeRow`) to host the composable so the test focuses on gesture routing assertions rather than rendering.
🤖 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 109-119: The ripple is overflowing the rounded background because
the modifier applies background(fileAttachmentShape) but doesn't clip; in
FileAttachmentContent (where shouldBeFullSize is checked) add a clip using the
same fileAttachmentShape before the combinedClickable so the ripple is
constrained to the rounded corners (i.e., when !shouldBeFullSize, apply
padding(...).background(color, fileAttachmentShape).clip(fileAttachmentShape)
prior to combinedClickable that calls onItemClick and
attachmentState.onLongItemClick).
---
Nitpick comments:
In
`@stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/components/messages/MessageTextTest.kt`:
- Around line 35-58: Add a Compose interaction test in MessageTextTest (next to
existing tests like `plain text`, `text with url`, etc.) that exercises the
pointer-input contract on the MessageText composable(s) (e.g., MessageTextPlain
or MessageTextWithUrl): use the Compose testing APIs to perform a tap and a
long-press on the MessageText node and assert that interactive targets
(links/mentions) consume the gestures locally while non-interactive areas allow
the gesture to bubble (e.g., to the surrounding cell). Name the new test clearly
(e.g., `pointer input contract`) and reuse the existing snapshot scaffolding
(`snapshotWithDarkModeRow`) to host the composable so the test focuses on
gesture routing assertions rather than rendering.
In
`@stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/util/PassiveRippleTest.kt`:
- Around line 151-167: Update the test in PassiveRippleTest (`drag out of bounds
during gesture does not crash`) to also assert cleanup by supplying a
controllable MutableInteractionSource to the passiveRipple and verifying it
receives a PressInteraction.Cancel when the pointer is moved out of bounds;
specifically, create a MutableInteractionSource, pass it into passiveRipple (or
use a testable overload/wrapper), perform the same touch sequence via
composeTestRule.performTouchInput, then collect emissions from the interaction
source and assert that a PressInteraction.Cancel was emitted (and no
PressInteraction.Release), ensuring the cancel branch is validated.
- Around line 112-149: Add an assertion that the parent's passiveRipple
interaction source emits no PressInteraction when the inner clickable consumes
the down: create a MutableInteractionSource instance (e.g.,
parentInteractionSource = MutableInteractionSource()), pass it into the outer
Modifier.passiveRipple(interactionSource = parentInteractionSource) so the test
can observe interactions, perform the same touch on the "consumer" node, and
then collect/inspect parentInteractionSource.interactions (or snapshot its
events) to verify no PressInteraction was emitted; update PassiveRippleTest (the
`inner consuming clickable does not trigger outer combinedClickable` test) to
include this check after performing the touch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0500c655-74af-40c8-8229-4abdea48f4b6
⛔ 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
|


Goal
Move the ripple from the message cell to the bubble while keeping the cell-wide hit area.
Implementation
MessageContainer'scombinedClickablestill handles thread-open and action-menu dispatch with thecanOpenThread/canOpenActionsgates. Itsindicationisnull— the cell no longer renders a ripple itself.Modifier.passiveRipple()(new, inModifierUtils.kt). A non-consumingpointerInputemitsPressInteractioninto a local source and renders a bounded position-aware ripple. Applied to the message-contentColumnand to the inner pollColumn. Skips when an inner clickable child (attachment, quoted reply, link character) has already consumed the down — no double ripple.combinedClickable + ripple()(giphy, file, and link gainedripple()to match the image attachment).ClickableTexthelper uses a customawaitEachGesturethat only consumes when the press lands on an interactive character. Non-link character taps propagate normally — bubble ripples, cell fires.rememberHapticLongClickhelper. Every leaf (cell, attachments, quoted reply, text, link characters) gets haptic for free, no duplicatedperformHapticFeedbackcalls at each call site.Important
Decisions worth flagging
passiveRippleis named after what it does, not where it is used. Reusable for any layout that wants visual press feedback while delegating click handling to a surrounding (or inner) clickable.MessageText.ClickableTextis intentional. Compose Foundation'sLinkAnnotationAPI was evaluated and rejected: in 1.9.x it has no long-press hook and its innercombinedClickableconsumes the down, which would shield the cell's long-press without a clean way to layer one on top.🎨 UI Changes
Screen_recording_20260507_165648.webm
Screen_recording_20260508_130005.webm
Testing
Open any channel in the Compose sample, then:
Summary by CodeRabbit
Bug Fixes
New Features
Tests