Open
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
|
WalkthroughThis PR introduces attachment URL preservation across message update operations. Changes include making a helper method Changes
Sequence DiagramsequenceDiagram
actor Client
participant ChannelEventHandler as Event Handler
participant ChannelLogic as Channel Logic
participant ChannelState as Channel State
participant Validator as AttachmentUrlValidator
participant Messages as Message Store
Client->>ChannelEventHandler: Reaction Event
ChannelEventHandler->>ChannelLogic: Update message with reaction
ChannelLogic->>ChannelState: upsertMessages(messages, preserveAttachmentUrls=true)
ChannelState->>Validator: updateValidAttachmentsUrl(newMessages, oldMessages)
Validator->>Validator: Preserve URL signatures from old state
Validator->>ChannelState: Return messages with preserved URLs
ChannelState->>Messages: Store updated message
ChannelState-->>Client: Message state updated with reaction + preserved URLs
Client->>ChannelEventHandler: Message Update Event
ChannelEventHandler->>ChannelState: updateMessageFromEvent(message, enrich)
ChannelState->>Validator: Check attachment URLs in old message
Validator->>ChannelState: Return message with preserved URLs
ChannelState->>ChannelState: Apply enrich lambda for additional fields
ChannelState->>Messages: Store final merged message
ChannelState-->>Client: Message state updated with merged fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImplMessagesTest.kt (1)
786-899: Consider adding one multi-attachment regression case.A case with 2+ image attachments (including reordered incoming attachments) would harden this against index/order-based preservation regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImplMessagesTest.kt` around lines 786 - 899, Add a regression test in ChannelStateImplMessagesTest that covers messages with multiple image attachments (use createMessage(...) and build attachments via Attachment(...)), set the original message with at least two attachments where one attachment uses streamCdnImageUrl, then upsert an updated message whose attachments are reordered and have newStreamCdnImageUrl values; call channelState.upsertMessage (and/or channelState.upsertMessages with preserveAttachmentUrls = true) and assert that the attachment that originally had the valid streamCdnImageUrl still preserves that URL after the upsert while other attachments reflect the updated URLs, to guard against index/order-based preservation regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImpl.kt`:
- Around line 422-425: The preserved-URL Message produced by
preserveAttachmentUrls(...) is only applied inside internal lists and never
returned to callers, so external code (e.g., updateQuotedMessageReferences) can
still see and propagate the original re-signed imageUrl; modify updateMessage
and the other update path (the one at lines indicated in the review) so that
updateMessageById(...) returns the merged/updated Message (apply
preserveAttachmentUrls and return that Message) and propagate that returned
Message out to callers, or alternatively call preserveAttachmentUrls again
inside updateQuotedMessageReferences(...) to ensure quoted/secondary fan-out
always receives the URL-preserved Message; reference the functions
updateMessage, updateMessageById, preserveAttachmentUrls, and
updateQuotedMessageReferences when making the change.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/channel/controller/WhenHandleEvent.kt`:
- Around line 101-106: The test stubs for
attachmentUrlValidator.updateValidAttachmentsUrl(...) are ineffective because
attachmentUrlValidator is not injected into the SUT (ChannelLogicLegacyImpl) and
channelStateLogic is a mock; fix by wiring the real or properly injected
attachmentUrlValidator into the ChannelLogicLegacyImpl instance used by the
tests (or replace channelStateLogic mock with a spy that delegates to the
provided attachmentUrlValidator), then remove the disconnected whenever(...)
stubs and instead let the SUT call the real
attachmentUrlValidator.updateValidAttachmentsUrl(...) (or verify via the spy) so
the tests exercise the actual URL-preservation logic.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelEventHandlerImplTest.kt`:
- Around line 595-598: The test currently calls the captured lambda with the
same Message twice, which masks bugs where the implementation might ignore the
`new` parameter; change the captor invocation to pass two distinct Message
instances (e.g., `oldMessage` and `newMessage`) and then assert that
`result.ownReactions` matches the `oldMessage`'s ownReactions and that at least
one field specific to `newMessage` (such as text or id) is present in the result
to ensure `new` is being used; apply the same change to the other similar
assertions that use the argumentCaptor with updateMessageFromEvent (the blocks
around the other mentioned occurrences).
---
Nitpick comments:
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImplMessagesTest.kt`:
- Around line 786-899: Add a regression test in ChannelStateImplMessagesTest
that covers messages with multiple image attachments (use createMessage(...) and
build attachments via Attachment(...)), set the original message with at least
two attachments where one attachment uses streamCdnImageUrl, then upsert an
updated message whose attachments are reordered and have newStreamCdnImageUrl
values; call channelState.upsertMessage (and/or channelState.upsertMessages with
preserveAttachmentUrls = true) and assert that the attachment that originally
had the valid streamCdnImageUrl still preserves that URL after the upsert while
other attachments reflect the updated URLs, to guard against index/order-based
preservation regressions.
🪄 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: 9102003d-6977-47db-a13e-c66d25acdc30
📒 Files selected for processing (10)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/message/attachments/internal/AttachmentUrlValidator.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelEventHandlerImpl.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelLogicImpl.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImpl.ktstream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/utils/internal/List.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/channel/controller/WhenHandleEvent.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelEventHandlerImplTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelLogicImplTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/legacy/ChannelStateLogicTest.ktstream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImplMessagesTest.kt
Comment on lines
422
to
+425
| fun updateMessage(message: Message) { | ||
| updateMessageById(message.id) { message } | ||
| updateMessageById(message.id) { old -> | ||
| preserveAttachmentUrls(message, old) | ||
| } |
There was a problem hiding this comment.
The URL-preserved Message never escapes these helpers.
Both update paths only apply preserveAttachmentUrls(...) inside the internal lists. Callers still hold the raw event payload, so any secondary fan-out can reintroduce the re-signed imageUrl; MessageUpdatedEvent -> updateQuotedMessageReferences(...) is the current example, which means quoted reply previews can still flicker. Return the merged Message, or preserve again inside updateQuotedMessageReferences.
Also applies to: 457-459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/state/plugin/state/channel/internal/ChannelStateImpl.kt`
around lines 422 - 425, The preserved-URL Message produced by
preserveAttachmentUrls(...) is only applied inside internal lists and never
returned to callers, so external code (e.g., updateQuotedMessageReferences) can
still see and propagate the original re-signed imageUrl; modify updateMessage
and the other update path (the one at lines indicated in the review) so that
updateMessageById(...) returns the merged/updated Message (apply
preserveAttachmentUrls and return that Message) and propagate that returned
Message out to callers, or alternatively call preserveAttachmentUrls again
inside updateQuotedMessageReferences(...) to ensure quoted/secondary fan-out
always receives the URL-preserved Message; reference the functions
updateMessage, updateMessageById, preserveAttachmentUrls, and
updateQuotedMessageReferences when making the change.
Comment on lines
+101
to
+106
| whenever( | ||
| attachmentUrlValidator.updateValidAttachmentsUrl( | ||
| any<List<Message>>(), | ||
| any<Map<String, Message>>(), | ||
| ), | ||
| ) doAnswer { invocation -> |
There was a problem hiding this comment.
These attachmentUrlValidator stubs are disconnected from the SUT.
attachmentUrlValidator is never passed to ChannelLogicLegacyImpl, and channelStateLogic is a mock, so changing updateValidAttachmentsUrl(...) stubbing here doesn't affect the exercised code. These tests still won't catch regressions in legacy URL-preservation wiring until the real collaborator is injected.
Also applies to: 132-137, 286-291, 342-347, 382-387
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/channel/controller/WhenHandleEvent.kt`
around lines 101 - 106, The test stubs for
attachmentUrlValidator.updateValidAttachmentsUrl(...) are ineffective because
attachmentUrlValidator is not injected into the SUT (ChannelLogicLegacyImpl) and
channelStateLogic is a mock; fix by wiring the real or properly injected
attachmentUrlValidator into the ChannelLogicLegacyImpl instance used by the
tests (or replace channelStateLogic mock with a spy that delegates to the
provided attachmentUrlValidator), then remove the disconnected whenever(...)
stubs and instead let the SUT call the real
attachmentUrlValidator.updateValidAttachmentsUrl(...) (or verify via the spy) so
the tests exercise the actual URL-preservation logic.
Comment on lines
+595
to
598
| val captor = argumentCaptor<(Message, Message) -> Message>() | ||
| verify(state).updateMessageFromEvent(eq(message), captor.capture()) | ||
| val result = captor.firstValue(message, message) | ||
| assertEquals(ownReactions, result.ownReactions) |
There was a problem hiding this comment.
Use different old and new messages in these captor assertions.
Each test calls the captured (old, new) lambda with the same Message instance twice, so it still passes if the implementation ignores new and just returns old. Pass distinct inputs and assert one field from new plus ownReactions from old.
Also applies to: 619-622, 643-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/state/plugin/logic/channel/internal/ChannelEventHandlerImplTest.kt`
around lines 595 - 598, The test currently calls the captured lambda with the
same Message twice, which masks bugs where the implementation might ignore the
`new` parameter; change the captor invocation to pass two distinct Message
instances (e.g., `oldMessage` and `newMessage`) and then assert that
`result.ownReactions` matches the `oldMessage`'s ownReactions and that at least
one field specific to `newMessage` (such as text or id) is present in the result
to ensure `new` is being used; apply the same change to the other similar
assertions that use the argumentCaptor with updateMessageFromEvent (the blocks
around the other mentioned occurrences).
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
Image attachments flicker on message updates (reactions, pins and similar...)
The reason for this is that responses/events which update existing messages with image attachments, also deliver
imageattachments with a differently signedimageUrl. The fix re-introduces theAttachmentUrlValidatorlogic in the newChannelLogicImpl(already existing in theChannelLogicLegacyImpl).Note: This PR just introduces the old mechanism for by-passing this issue. We should explore different approaches for handling this (perhaps on image caching level), as with this 'fix', we basically store 'outdated' image data in the message list.
Implementation
attachmentUrlValidator.updateValidAttachmentsUrlinvocations on every message update inChannelLogicImpl🎨 UI Changes
flicker_reaction_before.mp4
flicker_reaction_after.mp4
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Refactor