Open
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
gpunto
changed the title
WalkthroughIntroduces lifecycle-managed Player state and video playback controls for the media gallery, expands MediaGalleryPreviewScreen APIs (new player/onMediaClick parameters and Function7 lambda arities), adds topContent handling in ChannelMediaAttachmentsPreviewBottomBarParams, replaces the options overlay with a ModalBottomSheet, and consolidates UI/controls across pager/footer. Changes
Sequence DiagramsequenceDiagram
participant UI as MediaGalleryPreviewScreen
participant PSM as rememberMediaGalleryPlayerState
participant PS as MediaGalleryPlayerState
participant GME as GalleryMediaEffect
participant Pager as MediaGalleryPager
participant VPC as VideoPlaybackControls
participant Player as Player
UI->>PSM: initialize(onPlaybackError)
PSM->>PS: create MediaGalleryPlayerState
PSM->>Player: create & bind to lifecycle
PSM-->>UI: return playerState
UI->>GME: GalleryMediaEffect(playerState, currentPage, attachments)
GME->>PS: select current attachment
alt attachment is video
GME->>Player: prepare MediaItem(url)
GME->>Player: seek(savedPosition)
GME->>Player: play
end
UI->>Pager: provide player & onMediaClick
Pager->>VPC: mount (player)
VPC->>Player: attach listener & poll position
User->>VPC: interact (play/pause/seek/speed)
VPC->>Player: play/seek/setPlaybackParameters
Player-->>VPC: listener updates state
VPC-->>UI: update UI
User->>Pager: tap media (onMediaClick)
Pager->>UI: toggle immersive mode
UI->>UI: hide/show header & bottom bar
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.kt (1)
223-229:⚠️ Potential issue | 🟠 MajorDon't hard-disable the only controller on this player view.
StreamMediaPlayerContentis still used directly bystream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaPreviewActivity.kt:64-84, and that screen does not render the new overlay controls. On Line 228, forcinguseController = falseremoves pause/seek UI there, while the existingshowController()call becomes a no-op.🎛️ Suggested shape
internal fun createPlayerView( context: Context, player: Player, + useController: Boolean = true, ): PlayerView { val playerView = LayoutInflater.from(context) .inflate(R.layout.stream_compose_player_view, null) as PlayerView return playerView.apply { this.player = player - useController = false + this.useController = useController setShowBuffering(PlayerView.SHOW_BUFFERING_NEVER) } }Then keep the default in
StreamMediaPlayerContent, and passuseController = falseonly from the redesigned gallery path.🤖 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/internal/StreamMediaPlayerContent.kt` around lines 223 - 229, createPlayerView currently forces useController = false which breaks controls for callers like MediaPreviewActivity and makes showController() a no-op; remove the hard-disable (delete or stop setting useController in createPlayerView inside StreamMediaPlayerContent) so the PlayerView uses its default controller behavior, and instead set useController = false only from the redesigned gallery code path that requires it (i.e., update the caller that intentionally wants no controller to explicitly set playerView.useController = false after createPlayerView).stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/channel/attachments/ChannelMediaAttachmentsPreviewScreen.kt (1)
171-178:⚠️ Potential issue | 🟠 MajorThis pager wiring is only half of the new gallery flow.
By passing
player = playerState.playerhere you opt intoMediaGalleryVideoPage, which now uses a controllerless player view. Because this screen still keeps the old bottom bar and does not passonMediaClick, videos here end up with no replacement playback controls and media taps cannot trigger immersive mode.🤖 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/channel/attachments/ChannelMediaAttachmentsPreviewScreen.kt` around lines 171 - 178, The pager is being forced into the new controllerless video page by passing playerState.player to MediaGalleryPager, which prevents the old bottom bar controls and onMediaClick handling; remove the player = playerState.player argument (or pass null) so MediaGalleryPager/MediaGalleryVideoPage falls back to the controller-backed view, and add/forward an onMediaClick callback from ChannelMediaAttachmentsPreviewScreen into MediaGalleryPager (and ensure the existing bottom bar code still renders) so taps trigger immersive mode and playback controls are present.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPage.kt (1)
204-223:⚠️ Potential issue | 🔴 CriticalImport
withTimeoutOrNullbefore landing this.Line 208 calls
withTimeoutOrNull, but this file never importskotlinx.coroutines.withTimeoutOrNull, so the new tap/double-tap block will not compile.🔧 Proposed fix
import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.withTimeoutOrNull import kotlin.math.abs🤖 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/internal/MediaGalleryPage.kt` around lines 204 - 223, The new double-tap logic uses withTimeoutOrNull but the file lacks the kotlinx.coroutines import, so add an import for kotlinx.coroutines.withTimeoutOrNull at the top of the file; locate the pointerInput block (contains coroutineScope, awaitEachGesture, awaitFirstDown, DoubleTapTimeoutMs and withTimeoutOrNull) and add the missing import so the withTimeoutOrNull call compiles.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt (1)
147-155:⚠️ Potential issue | 🟡 MinorInconsistent header modifier between stateful and stateless overloads.
The stateful overload uses
Modifier.fillMaxWidth()(line 149) while the stateless overload usesModifier.fillMaxWidth().height(56.dp)(lines 296-298). This inconsistency means the header will have different default heights depending on which overload is used.Consider aligning both defaults:
Proposed fix
header: `@Composable` (attachments: List<Attachment>, currentPage: Int) -> Unit = { _, _ -> MediaGalleryPreviewHeader( - modifier = Modifier.fillMaxWidth(), + modifier = Modifier + .fillMaxWidth() + .height(56.dp), message = viewModel.message, connectionState = viewModel.connectionState, onLeadingContentClick = onHeaderLeadingContentClick, onTrailingContentClick = onHeaderTrailingContentClick, ) },🤖 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/MediaGalleryPreviewScreen.kt` around lines 147 - 155, The stateful overload's header parameter uses Modifier.fillMaxWidth() while the stateless overload sets Modifier.fillMaxWidth().height(56.dp), causing different default header heights; update the stateful overload's header default (the header: `@Composable` (attachments: List<Attachment>, currentPage: Int) -> Unit lambda that calls MediaGalleryPreviewHeader) to use the same modifier as the stateless version (add .height(56.dp) to Modifier.fillMaxWidth()) so both overloads render the header at the same default height, ensuring consistency for MediaGalleryPreviewHeader across both implementations.
🧹 Nitpick comments (6)
stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewActivityTest.kt (3)
142-156: Test lacks behavior assertion.Similar to the delete test,
should save on save option clickonly exercises the click without verifying save behavior. Thescenariovariable is unused. Consider adding mock verification or renaming to clarify the test's actual scope.🤖 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/MediaGalleryPreviewActivityTest.kt` around lines 142 - 156, The test should assert that the save action occurs rather than only performing the click: update the test `should save on save option click` to either (A) verify the save flow by mocking or spying the component that handles saves (e.g., inject a mock save handler into `MediaGalleryPreviewActivity` or observe a result/Intent from `ActivityScenario`), and assert that the handler was called or the expected Intent/result was produced after clicking the "Save media" option, or (B) if you intentionally only exercise UI interaction, rename the test to reflect that it only performs the click; use the existing `createIntent(message)`/`PreviewMessageData.messageWithUserAndAttachment` and `ActivityScenario.launchActivityForResult<MediaGalleryPreviewActivity>(intent)`/`composeTestRule` to obtain and assert the expected behavior.
124-140: Test lacks behavior assertion.The test name
should delete on delete option clicksuggests it verifies delete behavior, but the test only exercises the click without asserting any outcome. Thescenariovariable is unused. Consider either:
- Adding an assertion (e.g., verify delete API was called via mock verification)
- Renaming to clarify intent (e.g.,
should not crash on delete option click)🤖 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/MediaGalleryPreviewActivityTest.kt` around lines 124 - 140, The test function `should delete on delete option click` in MediaGalleryPreviewActivityTest currently only performs a click and doesn't assert behavior; update the test to either verify the delete flow was triggered (for example, mock and verify the chat client's delete method or repository method was called after clicking the "Delete" option—use the existing mocks like `mockClientState`/client mock or whatever deletion API mock exists in the test harness and assert it was invoked with `PreviewMessageData.messageWithUserAndAttachment`), or rename the test to something like `should not crash on delete option click` if no behavior should be asserted; modify `should delete on delete option click` (or add verification) and use the `scenario`/mocks to perform a verify assertion.
158-168: Inconsistent interaction pattern.The
should share on share option clicktest still usesperformClick()directly (line 166) while all the other option click tests were migrated to the semantic action pattern. Consider aligning for consistency, or document why the share button differs (e.g., it's a toolbar icon vs. bottom sheet item).♻️ Suggested alignment (if applicable)
If the Share button should follow the same pattern:
- composeTestRule.onNodeWithContentDescription("Share").performClick() + composeTestRule.onNodeWithContentDescription("Share") + .performSemanticsAction(SemanticsActions.OnClick)🤖 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/MediaGalleryPreviewActivityTest.kt` around lines 158 - 168, The test MediaGalleryPreviewActivityTest.should share on share option click uses composeTestRule.onNodeWithContentDescription("Share").performClick(), which is inconsistent with the other option-click tests that use the semantic-action pattern; replace the direct performClick call with the same semantic-action invocation used elsewhere (i.e., invoke the node's semantic action for click) so the test aligns with the other option click tests, or if the Share control is intentionally different (toolbar icon vs bottom-sheet item), add a short comment in the test explaining why performClick is kept.stream-chat-android-compose/api/stream-chat-android-compose.api (1)
864-865: Document thisMediaGalleryPreviewScreenAPI break in the v7 migration notes.These overloads change the public parameter model and the exposed slot callback shape, so custom preview integrations will need source updates. If this is not already covered elsewhere in the PR, please add an entry to
MIGRATION_TO_V7.mdor the changelog rather than introducing compatibility shims.Based on learnings, PRs against major version branches (e.g.,
v7) are allowed to introduce breaking public API changes, and maintainers prefer documenting migration paths inMIGRATION_TO_V7.md/CHANGELOGfor function signature changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/api/stream-chat-android-compose.api` around lines 864 - 865, The public overloads of MediaGalleryPreviewScreen changed parameter types and callback shapes; document this breaking change in the v7 migration notes by adding an entry to MIGRATION_TO_V7.md (or the CHANGELOG) explaining the new signatures (both MediaGalleryPreviewScreen overloads), what changed (model-based parameters vs viewmodel-based parameters and updated FunctionX slot types), and provide a minimal migration snippet showing how callers should adapt custom preview integrations to the new parameter list and callback shapes; reference the exact symbol name MediaGalleryPreviewScreen and list both overload variants so users can find and update usages.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt (2)
722-725: Consider updating content description to match the new icon.The icon changed from a close (X) to a back arrow, but the content description still references "cancel" (
R.string.stream_compose_cancel). For accessibility consistency, consider using a "back" or "navigate back" description.🤖 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/MediaGalleryPreviewScreen.kt` around lines 722 - 725, The Icon in MediaGalleryPreviewScreen.kt now uses R.drawable.stream_compose_ic_arrow_back but still sets contentDescription via stringResource(R.string.stream_compose_cancel); update the contentDescription to a more accurate string (e.g., R.string.stream_compose_back or R.string.stream_compose_navigate_back) and replace the reference in the Icon's contentDescription call so accessibility announces "back" instead of "cancel".
594-603: Consider showing a placeholder when player is null for video attachments.When
playeris null (e.g., during initialization), video pages render nothing. This could result in a blank page momentarily. Consider showing the thumbnail as a placeholder until the player is ready.Suggested improvement
attachment.isVideo() -> { - player?.let { + if (player != null) { MediaGalleryVideoPage( modifier = Modifier.fillMaxSize(), - player = it, + player = player, thumbnailUrl = attachment.thumbUrl, onClick = onMediaClick, ) + } else { + // Show thumbnail placeholder while player initializes + MediaGalleryImagePage( + attachment = attachment, + pagerState = pagerState, + page = page, + onTap = onMediaClick, + ) } }🤖 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/MediaGalleryPreviewScreen.kt` around lines 594 - 603, The video branch currently returns nothing when player is null which causes blank pages; update the attachment.isVideo() branch to render a thumbnail placeholder while the player initializes by either (a) passing a nullable player into MediaGalleryVideoPage and have it show the thumbnailUrl when player == null, or (b) render a small MediaGalleryVideoPlaceholder composable that uses attachment.thumbUrl and onMediaClick until player is available, then swap to MediaGalleryVideoPage with the ready player; reference attachment.isVideo(), player, MediaGalleryVideoPage, thumbnailUrl/thumbUrl, and onMediaClick to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPage.kt`:
- Around line 204-223: The new double-tap logic uses withTimeoutOrNull but the
file lacks the kotlinx.coroutines import, so add an import for
kotlinx.coroutines.withTimeoutOrNull at the top of the file; locate the
pointerInput block (contains coroutineScope, awaitEachGesture, awaitFirstDown,
DoubleTapTimeoutMs and withTimeoutOrNull) and add the missing import so the
withTimeoutOrNull call compiles.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.kt`:
- Around line 223-229: createPlayerView currently forces useController = false
which breaks controls for callers like MediaPreviewActivity and makes
showController() a no-op; remove the hard-disable (delete or stop setting
useController in createPlayerView inside StreamMediaPlayerContent) so the
PlayerView uses its default controller behavior, and instead set useController =
false only from the redesigned gallery code path that requires it (i.e., update
the caller that intentionally wants no controller to explicitly set
playerView.useController = false after createPlayerView).
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt`:
- Around line 147-155: The stateful overload's header parameter uses
Modifier.fillMaxWidth() while the stateless overload sets
Modifier.fillMaxWidth().height(56.dp), causing different default header heights;
update the stateful overload's header default (the header: `@Composable`
(attachments: List<Attachment>, currentPage: Int) -> Unit lambda that calls
MediaGalleryPreviewHeader) to use the same modifier as the stateless version
(add .height(56.dp) to Modifier.fillMaxWidth()) so both overloads render the
header at the same default height, ensuring consistency for
MediaGalleryPreviewHeader across both implementations.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/channel/attachments/ChannelMediaAttachmentsPreviewScreen.kt`:
- Around line 171-178: The pager is being forced into the new controllerless
video page by passing playerState.player to MediaGalleryPager, which prevents
the old bottom bar controls and onMediaClick handling; remove the player =
playerState.player argument (or pass null) so
MediaGalleryPager/MediaGalleryVideoPage falls back to the controller-backed
view, and add/forward an onMediaClick callback from
ChannelMediaAttachmentsPreviewScreen into MediaGalleryPager (and ensure the
existing bottom bar code still renders) so taps trigger immersive mode and
playback controls are present.
---
Nitpick comments:
In `@stream-chat-android-compose/api/stream-chat-android-compose.api`:
- Around line 864-865: The public overloads of MediaGalleryPreviewScreen changed
parameter types and callback shapes; document this breaking change in the v7
migration notes by adding an entry to MIGRATION_TO_V7.md (or the CHANGELOG)
explaining the new signatures (both MediaGalleryPreviewScreen overloads), what
changed (model-based parameters vs viewmodel-based parameters and updated
FunctionX slot types), and provide a minimal migration snippet showing how
callers should adapt custom preview integrations to the new parameter list and
callback shapes; reference the exact symbol name MediaGalleryPreviewScreen and
list both overload variants so users can find and update usages.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt`:
- Around line 722-725: The Icon in MediaGalleryPreviewScreen.kt now uses
R.drawable.stream_compose_ic_arrow_back but still sets contentDescription via
stringResource(R.string.stream_compose_cancel); update the contentDescription to
a more accurate string (e.g., R.string.stream_compose_back or
R.string.stream_compose_navigate_back) and replace the reference in the Icon's
contentDescription call so accessibility announces "back" instead of "cancel".
- Around line 594-603: The video branch currently returns nothing when player is
null which causes blank pages; update the attachment.isVideo() branch to render
a thumbnail placeholder while the player initializes by either (a) passing a
nullable player into MediaGalleryVideoPage and have it show the thumbnailUrl
when player == null, or (b) render a small MediaGalleryVideoPlaceholder
composable that uses attachment.thumbUrl and onMediaClick until player is
available, then swap to MediaGalleryVideoPage with the ready player; reference
attachment.isVideo(), player, MediaGalleryVideoPage, thumbnailUrl/thumbUrl, and
onMediaClick to locate and implement the change.
In
`@stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewActivityTest.kt`:
- Around line 142-156: The test should assert that the save action occurs rather
than only performing the click: update the test `should save on save option
click` to either (A) verify the save flow by mocking or spying the component
that handles saves (e.g., inject a mock save handler into
`MediaGalleryPreviewActivity` or observe a result/Intent from
`ActivityScenario`), and assert that the handler was called or the expected
Intent/result was produced after clicking the "Save media" option, or (B) if you
intentionally only exercise UI interaction, rename the test to reflect that it
only performs the click; use the existing
`createIntent(message)`/`PreviewMessageData.messageWithUserAndAttachment` and
`ActivityScenario.launchActivityForResult<MediaGalleryPreviewActivity>(intent)`/`composeTestRule`
to obtain and assert the expected behavior.
- Around line 124-140: The test function `should delete on delete option click`
in MediaGalleryPreviewActivityTest currently only performs a click and doesn't
assert behavior; update the test to either verify the delete flow was triggered
(for example, mock and verify the chat client's delete method or repository
method was called after clicking the "Delete" option—use the existing mocks like
`mockClientState`/client mock or whatever deletion API mock exists in the test
harness and assert it was invoked with
`PreviewMessageData.messageWithUserAndAttachment`), or rename the test to
something like `should not crash on delete option click` if no behavior should
be asserted; modify `should delete on delete option click` (or add verification)
and use the `scenario`/mocks to perform a verify assertion.
- Around line 158-168: The test MediaGalleryPreviewActivityTest.should share on
share option click uses
composeTestRule.onNodeWithContentDescription("Share").performClick(), which is
inconsistent with the other option-click tests that use the semantic-action
pattern; replace the direct performClick call with the same semantic-action
invocation used elsewhere (i.e., invoke the node's semantic action for click) so
the test aligns with the other option click tests, or if the Share control is
intentionally different (toolbar icon vs bottom-sheet item), add a short comment
in the test explaining why performClick is kept.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9519d05c-80e6-46e8-92d8-85a853212021
⛔ Files ignored due to path filters (11)
stream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewOptionsMenuTest_media_gallery_options_menu_for_other_user.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewOptionsMenuTest_media_gallery_options_menu_for_own_user.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_header_connecting.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_header_message_without_id.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_header_offline.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_header_online.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_connected.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_offline.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_with_gallery_bottom_sheet.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_with_options_menu.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_with_share_large_file_prompt.pngis excluded by!**/*.png
📒 Files selected for processing (11)
stream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/AudioRecordAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewOptionsMenu.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPage.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPlayerState.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/VideoPlaybackControls.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/channel/attachments/ChannelMediaAttachmentsPreviewScreen.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/button/SpeedButton.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewActivityTest.kt
gpunto
force-pushed
the
redesign/media-gallery
branch
from
6bfc276 to
d5532cf
Compare
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
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/preview/MediaGalleryPreviewScreen.kt (1)
355-367:⚠️ Potential issue | 🟠 MajorRemap
initialPageafter filtering attachments.Once files/audio/link attachments are removed, the raw
initialPageno longer points at the same media item. For example, selecting[file, image, video]at index1now starts on the video becausefilteredAttachments[1]is a different attachment. DerivestartingPositionfrom the originally selected attachment instead of reusing the original index.Representative fix
- val startingPosition = if (initialPage !in filteredAttachments.indices) 0 else initialPage + val initialAttachment = message.attachments.getOrNull(initialPage) + val startingPosition = filteredAttachments + .indexOf(initialAttachment) + .takeIf { it >= 0 } ?: 0🤖 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/MediaGalleryPreviewScreen.kt` around lines 355 - 367, The computed startingPosition currently reuses the raw initialPage index which becomes invalid after filtering; instead, lookup the originally selected attachment via message.attachments.getOrNull(initialPage) and find its index in filteredAttachments (use filteredAttachments.indexOf(selectedAttachment)); set startingPosition to that found index or 0 if not present, and pass it to rememberPagerState (references: filteredAttachments, initialPage, startingPosition, pagerState, message.attachments).
🧹 Nitpick comments (1)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt (1)
161-195: Document the new slot state contract.These public overloads now expose shared playback state via
playerand an immersive-mode hook viaonMediaClick, but the KDoc above them still describescontent/footeras generic slots. Please document when the player may be unavailable and whatonMediaClickis expected to drive so custom implementations have an explicit state contract.As per coding guidelines,
**/*.kt: Document public APIs with KDoc, including thread expectations and state notes.Also applies to: 308-342
🤖 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/MediaGalleryPreviewScreen.kt` around lines 161 - 195, Update the public KDoc for the MediaGalleryPreviewScreen overloads that expose the content and footer slots to explicitly document the slot state contract: state when the Player parameter may be null (e.g., before async player initialization, on unsupported media types, or when playback is disabled), that the player represents shared playback state across pages, and any threading expectations; describe that onMediaClick is an immersive-mode hook the caller should invoke to toggle fullscreen/immersive playback and that implementations must respect and not mutate the shared Player state directly; reference the slot parameters by name (content, footer), the player parameter, and the onMediaClick callback so custom slot implementations know when to render VideoPlaybackControls vs. static previews.
🤖 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 864-865: Record the breaking API change for the
MediaGalleryPreviewScreen composable in CHANGELOG.md under the
stream-chat-android-compose UNRELEASED section: add an entry in the "⚠️ Changed"
(or "❌ Removed" if you prefer) subsection documenting that
MediaGalleryPreviewScreen now has new overload signatures (mention
MediaGalleryPreviewScreen and the introduction of Function7-based overloads and
updated parameter lists), describe the impact on callers (existing overload
shapes/signatures changed) and include guidance for migration (which overload to
use or how to adapt call sites).
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPage.kt`:
- Around line 208-210: The code in MediaGalleryPage.kt uses withTimeoutOrNull
inside the double-tap handling (see the block referencing DoubleTapTimeoutMs and
awaitFirstDown), but the coroutine extension import is missing; add the
kotlinx.coroutines.withTimeoutOrNull import at the top of the file so the call
in the MediaGalleryPage (the double-tap gesture coroutine using awaitFirstDown
and DoubleTapTimeoutMs) resolves.
- Line 25: The file imports two clickable functions; alias the foundation one to
make usage explicit: change the import of androidx.compose.foundation.clickable
to an aliased name (e.g., foundationClickable) and leave
io.getstream.chat.android.compose.ui.util.clickable as-is; then update call
sites that require the foundation-specific parameters (interactionSource,
indication) to use foundationClickable (the simplified wrapper calls keep using
the util clickable), ensuring references in MediaGalleryPage (notably the call
around the interactionSource/indication usage) are updated to the aliased
symbol.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPlayerState.kt`:
- Around line 116-129: The LaunchedEffect currently watches only currentPage and
playerState.player, so it won't re-run when the attachments list is mutated
in-place; update the LaunchedEffect key to include attachments (e.g.,
LaunchedEffect(currentPage, playerState.player, attachments)) so the effect
reruns when attachments change and the media prep logic in the block (using
attachments.getOrNull(currentPage), lastPreparedPage, player.setMediaItem,
player.prepare) stays in sync with the latest list.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/VideoPlaybackControls.kt`:
- Around line 199-205: The UI uses animatedProgress/progress directly in Compose
fractions (e.g., fillMaxWidth(fraction = animatedProgress) and
PlaybackThumb(parentWidthPx = widthPx)), but ExoPlayer can produce progress
outside [0f,1f]; clamp progress to 0f..1f before any Compose fraction use to
satisfy the `@FloatRange` contract. Locate the progress computation in
VideoPlaybackControls (and any derived animatedProgress) and replace usages with
a clamped value (e.g., progress.coerceIn(0f, 1f) or an explicit min/max) so both
fillMaxWidth(fraction = ...) and PlaybackThumb receive a guaranteed-in-range
fraction.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt`:
- Around line 720-729: The Icon inside StreamButton (in
MediaGalleryPreviewScreen/StreamButton usage) uses contentDescription =
stringResource(id = R.string.stream_compose_cancel) but the visual is a back
arrow; update the contentDescription to a back/navigation string (e.g.
stringResource(id = R.string.stream_compose_back) or stringResource(id =
R.string.stream_compose_navigate_up)), and if that resource doesn't exist add a
new localized string resource (e.g. "Back" or "Navigate up") and reference it
from the Icon to ensure screen readers announce the correct action.
---
Outside diff comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt`:
- Around line 355-367: The computed startingPosition currently reuses the raw
initialPage index which becomes invalid after filtering; instead, lookup the
originally selected attachment via message.attachments.getOrNull(initialPage)
and find its index in filteredAttachments (use
filteredAttachments.indexOf(selectedAttachment)); set startingPosition to that
found index or 0 if not present, and pass it to rememberPagerState (references:
filteredAttachments, initialPage, startingPosition, pagerState,
message.attachments).
---
Nitpick comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt`:
- Around line 161-195: Update the public KDoc for the MediaGalleryPreviewScreen
overloads that expose the content and footer slots to explicitly document the
slot state contract: state when the Player parameter may be null (e.g., before
async player initialization, on unsupported media types, or when playback is
disabled), that the player represents shared playback state across pages, and
any threading expectations; describe that onMediaClick is an immersive-mode hook
the caller should invoke to toggle fullscreen/immersive playback and that
implementations must respect and not mutate the shared Player state directly;
reference the slot parameters by name (content, footer), the player parameter,
and the onMediaClick callback so custom slot implementations know when to render
VideoPlaybackControls vs. static previews.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 19b297ec-dca9-4795-a88e-7b23988f078c
⛔ Files ignored due to path filters (20)
stream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewOptionsMenuTest_media_gallery_options_menu_for_other_user.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewOptionsMenuTest_media_gallery_options_menu_for_own_user.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_footer_connected.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_footer_offline.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_footer_sharing_in_progress.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_header_connecting.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_header_message_without_id.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_header_offline.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_header_online.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_connected.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_offline.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_with_gallery_bottom_sheet.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_with_options_menu.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.preview_MediaGalleryPreviewScreenTest_media_gallery_screen_with_share_large_file_prompt.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.attachments_ChannelMediaAttachmentsPreviewContentTest_content.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.attachments_ChannelMediaAttachmentsPreviewContentTest_content_in_dark_mode.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.attachments_ChannelMediaAttachmentsPreviewContentTest_preparing_to_share_content.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.attachments_ChannelMediaAttachmentsPreviewContentTest_preparing_to_share_content_in_dark_mode.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channels_ChannelListHeaderTest_connecting,_no_user.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channels_ChannelListHeaderTest_connecting,_with_user.pngis excluded by!**/*.png
📒 Files selected for processing (15)
stream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/AudioRecordAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewOptionsMenu.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPage.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPlayerState.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/VideoPlaybackControls.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/channel/attachments/ChannelMediaAttachmentsPreviewScreen.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/NetworkLoadingIndicator.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/button/SpeedButton.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.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/attachments/preview/MediaGalleryPreviewActivityTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/button/SpeedButton.kt
- stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/StreamMediaPlayerContent.kt
...n/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPage.kt
Show resolved
Hide resolved
...n/java/io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPage.kt
Show resolved
Hide resolved
...io/getstream/chat/android/compose/ui/attachments/preview/internal/MediaGalleryPlayerState.kt
Show resolved
Hide resolved
...a/io/getstream/chat/android/compose/ui/attachments/preview/internal/VideoPlaybackControls.kt
Show resolved
Hide resolved
...n/java/io/getstream/chat/android/compose/ui/attachments/preview/MediaGalleryPreviewScreen.kt
Show resolved
Hide resolved
andremion
approved these changes
Mar 16, 2026
Contributor
andremion
left a comment
andremion
left a comment
There was a problem hiding this comment.
Good job! 👏🏻
Just a couple of remarks.
Just for my understanding. This PR won't apply to all the UI we see on Figma, right? Is it a part of a bigger task?
| } | ||
| } | ||
| } | ||
| val startingPosition = if (initialPage !in filteredAttachments.indices) 0 else initialPage |
Contributor
There was a problem hiding this comment.
If the original message has, let's say, [file, image, video] and the user tapped the video (index 2), filteredAttachments becomes [image, video]. Since 2 !in 0..1, the fallback to 0 shows the image instead of the selected video.
Maybe you need to resolve the originally selected attachment first, then find its index in the filtered list
Contributor
Author
There was a problem hiding this comment.
So, I investigated this and I realized that it works as is, but it's fragile.
initialPage is already an index that refers to a filtered list. It originally comes from MediaAttachmentContent/MultipleMediaAttachmentsColumns. It's positionInColumn here or attachmentIndex here.
Now, I personally think we should pass instead an attachment identifier and then we resolve the position off of that, but what would be a reliable identifier? Maybe Attachment.imagePreviewUrl is good enough since that's what we're using for rendering remote media attachment previews (resolved in Attachment.imagePreviewData)?
I also thought we could use the whole Attachment instead of a position/ID, but we are going through an Intent and we can't easily put an attachment in a bundle.
| ) { | ||
| var lastPreparedPage by remember { mutableIntStateOf(-1) } | ||
|
|
||
| LaunchedEffect(currentPage, playerState.player) { |
Contributor
There was a problem hiding this comment.
This effect reads attachments.getOrNull(currentPage) but doesn't key on attachments. If the list changes while the user stays on the same page (e.g. an attachment is deleted server-side and the message updates), the effect won't re-run to prepare the new media item.
| ) { | ||
| Icon( | ||
| painter = painterResource(id = R.drawable.stream_compose_ic_close), | ||
| painter = painterResource(id = R.drawable.stream_compose_ic_arrow_back), |
Contributor
There was a problem hiding this comment.
The icon is a back arrow, but the content description resolves to "Cancel". Shouldn't we update the content description to "Back" (I believe we already have this string)?
Contributor
VelikovPetar
left a comment
VelikovPetar
left a comment
There was a problem hiding this comment.
Another additional question: Do we have some new UI for playing audio files (full screen)? Because they are currently handled by the MediaPreviewActivity - and since it is no longer used for videos (when opening a video in a message with mixed attachments), perhaps we can consider reworking it (of course, nothing urgent, just pointing it out).
| val colors = ChatTheme.colors | ||
| val textColor = if (enabled) colors.controlPlaybackToggleText else colors.textDisabled | ||
| val borderColor = if (enabled) outlineColor else colors.borderUtilityDisabled | ||
| Text( |
Contributor
There was a problem hiding this comment.
Very small thing: When the button has value 1.5x it becomes wider compared to when it has values 1x or 2x. This makes the slider in front of it to shrink, which makes it a bit strange (visually). Not sure if it would make sense to have the speed button with fixed width (to match the 1.5x width). (sam thing applies for the audio recording attachments)
Contributor
There was a problem hiding this comment.
This snapshot looks strange 🤔
Contributor
There was a problem hiding this comment.
We are missing the menu here as well.
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
3 participants
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
Redesign the media gallery preview screen
Implementation
VideoPlaybackControlsoverlayMediaGalleryPlayerStateto encapsulate player lifecycle and state managementSpeedButtoncomponent from the audio recording speed control🎨 UI Changes
Testing
Summary by CodeRabbit