Use attachment url instead of index for determining the initial page in the media preview#6265
Open
Use attachment url instead of index for determining the initial page in the media preview#6265
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
WalkthroughThe PR refactors media attachment preview selection from index-based positioning to URL-based selection across multiple components, including activity intents, contracts, composables, and data structures. This replaces integer position parameters with string URL parameters throughout the media gallery preview flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/MediaAttachmentContent.kt (1)
121-149:⚠️ Potential issue | 🟡 MinorFix the deprecation quick-fix for
onItemClick.The deprecated overload's
ReplaceWithforwardsonItemClick = onItemClickdirectly, but the replacement overload expects(MediaAttachmentClickData) -> Unit. IDE-assisted migration will generate non-compiling code unless theReplaceWithadapts the old callback signature.🛠️ Proposed fix
`@Deprecated`( message = "Use the overload that takes onItemClick as a single parameter of type MediaAttachmentClickData.", replaceWith = ReplaceWith( "MediaAttachmentContent(" + "state = attachmentState, " + "modifier = modifier, " + "maximumNumberOfPreviewedItems = maximumNumberOfPreviewedItems, " + "skipEnrichUrl = skipEnrichUrl, " + - "onItemClick = onItemClick, " + + "onItemClick = { data -> onItemClick(" + + "data.mediaGalleryPreviewLauncher, " + + "data.message, " + + "data.selectedAttachmentUrl, " + + "data.videoThumbnailsEnabled, " + + "data.downloadAttachmentUriGenerator, " + + "data.downloadRequestInterceptor, " + + "data.streamCdnImageResizing, " + + "data.skipEnrichUrl" + + ") }, " + "itemOverlayContent = itemOverlayContent" + ")", ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/MediaAttachmentContent.kt` around lines 121 - 149, The ReplaceWith for the deprecated MediaAttachmentContent must wrap the old multi-parameter onItemClick into the new single-parameter signature; update the ReplaceWith expression so it passes onItemClick = { data: MediaAttachmentClickData -> onItemClick( data.mediaGalleryPreviewLauncher, data.message, data.selectedAttachmentUrl, data.videoThumbnailsEnabled, data.downloadAttachmentUriGenerator, data.downloadRequestInterceptor, data.streamCdnImageResizing, data.skipEnrichUrl ) } (or equivalent lambda) when calling the new MediaAttachmentContent overload, referencing MediaAttachmentContent, the deprecated onItemClick parameter, and MediaAttachmentClickData so IDE quick-fix produces compilable code.
🧹 Nitpick comments (2)
stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreenTest.kt (1)
118-118: Add one snapshot that exercises URL-based selection.All updated cases pass
selectedAttachmentUrl = null, so they only cover the default first-page path. Please add a multi-attachment snapshot with a non-null URL pointing at a non-first item so this PR's actual resolver logic is covered.As per coding guidelines,
**/stream-chat-android-compose/**/*Test.kt: Add Paparazzi snapshots for Compose UI regressions and runverifyPaparazziDebug.Also applies to: 135-135, 152-152, 169-169, 186-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreenTest.kt` at line 118, Add a new Paparazzi snapshot case in MediaGalleryPreviewScreenTest that passes a non-null selectedAttachmentUrl that matches a URL of a non-first attachment to exercise URL-based selection; locate the existing test function(s) where selectedAttachmentUrl = null is passed (look for the test methods in MediaGalleryPreviewScreenTest that call the composable under test) and add a sibling test or expand one case to set selectedAttachmentUrl = "<url-of-second-attachment>" (use the same attachment list used in the other snapshots) so the resolver logic selects a non-first page, then run verifyPaparazziDebug to ensure the snapshot is added.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewActivity.kt (1)
155-155: Normalize blank selected URLs tonullbefore matching.An empty string is unlikely to match any attachment URL but is still treated as a provided selector. Converting blanks to
nullkeeps fallback behavior explicit.Suggested diff
- val selectedAttachmentUrl = intent?.getStringExtra(KeySelectedAttachmentUrl) + val selectedAttachmentUrl = intent?.getStringExtra(KeySelectedAttachmentUrl)?.takeIf { it.isNotBlank() } ... - putExtra(KeySelectedAttachmentUrl, selectedAttachmentUrl) + putExtra(KeySelectedAttachmentUrl, selectedAttachmentUrl?.takeIf { it.isNotBlank() })Also applies to: 543-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewActivity.kt` at line 155, In MediaGalleryPreviewActivity, normalize the selectedAttachmentUrl obtained via intent?.getStringExtra(KeySelectedAttachmentUrl) to null when it's blank (empty or only whitespace) before any matching logic so an empty string doesn't block fallback behavior; replace the direct assignment with a trimmed check (if the retrieved string.isNullOrBlank() then null else the trimmed string) and apply the same change to the other occurrence that reads KeySelectedAttachmentUrl (the second usage noted in the file) so both matching points treat blank selectors as absent.
🤖 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-compose/api/stream-chat-android-compose.api`:
- Around line 583-594: Add migration notes to the changelog documenting the API
change from position-based parameters to the URL-based selectedAttachmentUrl:
explain that MediaAttachmentClickData no longer exposes
attachmentPosition/initialPosition and callers should pass selectedAttachmentUrl
(the absolute attachment URL to open) instead; update references for
MediaGalleryPreviewActivity, MediaGalleryPreviewContract, and
MediaGalleryPreviewScreen to show the new parameter name and expected semantics,
include a short example mapping (attachmentPosition/initialPosition ->
selectedAttachmentUrl) and note behavioral differences for callers relying on
index-based navigation in CHANGELOG.md or a dedicated migration section.
- Around line 583-594: The preview selection currently finds the clicked
attachment by using indexOfFirst on imagePreviewUrl which picks the first
matching URL and breaks when duplicate URLs exist; update the selection logic in
the preview-launching code (the place that reads getSelectedAttachmentUrl() and
getMessage() / message.attachments) to prefer a stable tiebreaker: first try to
match by the original attachment index passed through the component (retain and
use the attachment index from the click handler or an explicit attachmentId
field), and only fall back to matching by imagePreviewUrl when index/id is not
available; add unit tests that simulate a Message with duplicate imagePreviewUrl
entries to assert the correct preview index is chosen and note the change in the
CHANGELOG.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewActivity.kt`:
- Line 514: Add a breaking-change note to CHANGELOG.md under the v7 section
documenting that MediaGalleryPreviewActivity.getIntent changed its selection
parameter from an index-based attachmentPosition:Int to a URL-based
selectedAttachmentUrl:String?; instruct users to migrate by passing the
attachment's preview URL instead of position (e.g., use attachment.imageUrl or
attachment.videoThumbnailUrl when constructing the new selectedAttachmentUrl)
and include an example mapping: attachmentPosition -> find the attachment at
that index and use its imageUrl/videoThumbnailUrl as selectedAttachmentUrl for
getIntent.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt`:
- Around line 364-367: Replace the fragile indexOfFirst lookup for
startingPosition with a stable multi-key match: iterate filteredAttachments with
mapIndexed and pick the first entry where imagePreviewUrl ==
selectedAttachmentUrl AND a stable tiebreaker matches (preferably an attachment
identifier or the filtered index). Concretely, compute startingPosition by using
filteredAttachments.mapIndexed { index, att -> index to att } and find the pair
where att.imagePreviewUrl == selectedAttachmentUrl && (att.id ==
selectedAttachmentId || index == selectedAttachmentFilteredIndex), falling back
to 0 if none found; this uses filteredAttachments, selectedAttachmentUrl,
imagePreviewUrl, startingPosition (and add selectedAttachmentId or
selectedAttachmentFilteredIndex state if not present) to ensure deterministic
selection.
---
Outside diff comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/MediaAttachmentContent.kt`:
- Around line 121-149: The ReplaceWith for the deprecated MediaAttachmentContent
must wrap the old multi-parameter onItemClick into the new single-parameter
signature; update the ReplaceWith expression so it passes onItemClick = { data:
MediaAttachmentClickData -> onItemClick( data.mediaGalleryPreviewLauncher,
data.message, data.selectedAttachmentUrl, data.videoThumbnailsEnabled,
data.downloadAttachmentUriGenerator, data.downloadRequestInterceptor,
data.streamCdnImageResizing, data.skipEnrichUrl ) } (or equivalent lambda) when
calling the new MediaAttachmentContent overload, referencing
MediaAttachmentContent, the deprecated onItemClick parameter, and
MediaAttachmentClickData so IDE quick-fix produces compilable code.
---
Nitpick comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewActivity.kt`:
- Line 155: In MediaGalleryPreviewActivity, normalize the selectedAttachmentUrl
obtained via intent?.getStringExtra(KeySelectedAttachmentUrl) to null when it's
blank (empty or only whitespace) before any matching logic so an empty string
doesn't block fallback behavior; replace the direct assignment with a trimmed
check (if the retrieved string.isNullOrBlank() then null else the trimmed
string) and apply the same change to the other occurrence that reads
KeySelectedAttachmentUrl (the second usage noted in the file) so both matching
points treat blank selectors as absent.
In
`@stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreenTest.kt`:
- Line 118: Add a new Paparazzi snapshot case in MediaGalleryPreviewScreenTest
that passes a non-null selectedAttachmentUrl that matches a URL of a non-first
attachment to exercise URL-based selection; locate the existing test function(s)
where selectedAttachmentUrl = null is passed (look for the test methods in
MediaGalleryPreviewScreenTest that call the composable under test) and add a
sibling test or expand one case to set selectedAttachmentUrl =
"<url-of-second-attachment>" (use the same attachment list used in the other
snapshots) so the resolver logic selects a non-first page, then run
verifyPaparazziDebug to ensure the snapshot is added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b77eb064-ecd6-4215-99cc-2dae67c1db30
📒 Files selected for processing (6)
stream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/MediaAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewActivity.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewContract.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreenTest.kt
...java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewActivity.kt
Show resolved
Hide resolved
...n/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt
Outdated
Show resolved
Hide resolved
gpunto
force-pushed
the
media-preview_replace-attachment-index-w-url
branch
from
a94e335 to
3cf65b5
Compare
…in the media preview
gpunto
force-pushed
the
media-preview_replace-attachment-index-w-url
branch
from
3cf65b5 to
1ae7e5e
Compare
|
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
Use attachment url instead of index for determining the initial page in the media preview (addressing the discussion in #6247 (comment))
Implementation
🎨 UI Changes
None
Testing
Open the media gallery by clicking on an image/video attachment in a message and verify that the initially selected media is the one you clicked
Summary by CodeRabbit
Release Notes