Update audio recording and file attachments design#6137
Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe PR refactors audio playback speed management to track speeds per-audio track using an audioHash, removes legacy theme classes (MessageTheme, AudioRecordingAttachmentTheme, FileAttachmentTheme) from the compose API, simplifies ChatTheme by eliminating own/other message theme parameters, and updates UI components for audio and file attachments to use simplified styling via MessageStyling utility functions. Changes
Sequence DiagramsequenceDiagram
participant UI as UI Component
participant Controller as AudioPlayerController
participant Player as StreamAudioPlayer
participant PlayerImpl as MediaPlayer
UI->>Controller: changeSpeed(attachment)
activate Controller
Controller->>Player: changeSpeed(audioHash)
activate Player
Note over Player: Lookup current speed from speedMap<br/>or use INITIAL_SPEED
Player->>Player: Calculate next speed<br/>(1.0x → 1.5x → 2.0x → 1.0x)
alt Audio Currently Playing
Player->>PlayerImpl: speed = newSpeed
activate PlayerImpl
PlayerImpl-->>Player: ✓
deactivate PlayerImpl
end
Player->>Player: speedMap[audioHash] = newSpeed
Player->>Player: publishSpeed(audioHash, newSpeed)
Player-->>Controller: return newSpeed (Float)
deactivate Player
Controller->>Controller: Update speeds map<br/>Update state
Controller-->>UI: Emit updated AudioPlayerState
deactivate Controller
UI->>UI: Display new speed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/audio/StreamAudioPlayer.kt (1)
312-317:⚠️ Potential issue | 🔴 CriticalBug:
currentIndexcompared againstaudioHashinstead ofcurrentAudioHash.Line 313 compares
currentIndex(the index inaudioTracks) withaudioHash(a hash identifier). These are different domains and the comparison is almost certainly wrong. It should becurrentAudioHash == audioHash.🐛 Proposed fix
override fun getCurrentPositionInMs(audioHash: Int): Int { - if (currentIndex == audioHash) { + if (currentAudioHash == audioHash) { return mediaPlayer.currentPosition } return seekMap[audioHash] ?: 0 }stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/AudioRecordAttachmentPreviewContent.kt (1)
87-91:⚠️ Potential issue | 🟡 MinorStale KDoc: "fallback content for unsupported attachments" doesn't describe this component.
This KDoc appears to be a copy-paste from another component. It should describe the audio recording preview content item.
📝 Suggested fix
-/** - * Represents fallback content for unsupported attachments. - * - * `@param` modifier Modifier for styling. - */ +/** + * Represents a single audio recording attachment item in the composer preview. + * + * `@param` modifier Modifier for styling. + * `@param` attachment The audio recording attachment to display. + * `@param` playerState The state of the audio player. + * `@param` onPlayToggleClick Callback when the play/pause button is clicked. + * `@param` onThumbDragStart Callback when the waveform thumb drag starts. + * `@param` onThumbDragStop Callback when the waveform thumb drag stops. + * `@param` onAttachmentRemoved Callback when the attachment is removed. + */
🤖 Fix all issues with AI agents
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/AudioRecordAttachmentContent.kt`:
- Around line 252-257: AudioRecordAttachmentContentItemBase currently lacks the
isMine flag so timer text always uses chatTextIncoming; update the call sites
that instantiate AudioRecordAttachmentContentItemBase to pass the message
ownership boolean (isMine) through, add an isMine parameter to
AudioRecordAttachmentContentItemBase, and change its timerTextColor logic in
AudioRecordAttachmentContent.kt so that when playing it still uses
ChatTheme.colors.accentPrimary, otherwise it selects
ChatTheme.colors.chatTextOutgoing when isMine is true and
ChatTheme.colors.chatTextIncoming when false (mirror the pattern used in
FileAttachmentContent.kt).
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/StreamColors.kt`:
- Around line 481-482: The slate color variables in StreamColors.kt have
inverted luminance: slate300 (Color(0xFFA3ACBA)) is darker than slate400
(Color(0xFFB8BEC4)); update the values to match the design spec by either
swapping the hex values between slate300 and slate400 or replacing them with the
correct hex codes from the design spec so that higher numeric shade (slate400)
is a darker color than slate300; ensure the change is made where slate300 and
slate400 are declared in StreamColors.kt.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/WaveformSliderStyle.kt`:
- Line 61: The TODO about WaveformThumbStyle in WaveformSliderStyle.kt must be
resolved rather than left in the public API: either remove the class from the
public surface or make its intent explicit. If you want to keep it
private/unstable, annotate or move WaveformThumbStyle as internal (e.g., add an
`@InternalStreamChatApi` annotation or move it to an internal package) and remove
the TODO; if you intend it as a supported API, remove the TODO and add KDoc
describing its purpose and public contract (constructor parameters and usage) or
mark it `@Deprecated` with guidance if you plan to remove it later—update
references in WaveformSliderStyle to match the chosen option.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.kt`:
- Around line 58-61: The branch handling size.height == Dp.Infinity incorrectly
uses fillMaxSize(), which fills both dimensions and prevents the subsequent
.width(size.width) from constraining width; change fillMaxSize() to
fillMaxHeight() in that conditional (the modifier chain in the size.height ==
Dp.Infinity case) so the height is filled while width(size.width) still applies,
and add the corresponding compose modifier import if missing.
In
`@stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/state/messages/list/AudioPlayerState.kt`:
- Around line 79-81: In AudioPlayerState.stringify(), the concatenation of
"seekTo.size=${seekTo.size}" and "speeds.size=${speeds.size}" lacks a separator
causing outputs like "seekTo.size=0speeds.size=0)"; update the stringify()
implementation (method stringify in class AudioPlayerState) to insert a clear
separator (e.g., ", " or " | ") between those two interpolations so the
resulting string reads "seekTo.size=0, speeds.size=0)".
🧹 Nitpick comments (6)
stream-chat-android-client/src/test/java/io/getstream/chat/android/client/audio/StreamMediaPlayerTest.kt (1)
180-194: Potential flaky test ifrandomInt()produces the same value for both hashes.While the collision probability is negligible (~1 in 2³²), you could make this fully deterministic by using fixed distinct values or adding a guard assertion.
💡 Optional: add a precondition or use fixed values
- val audioHash1 = randomInt() - val audioHash2 = randomInt() + val audioHash1 = 1 + val audioHash2 = 2Or add a guard:
val audioHash1 = randomInt() var audioHash2 = randomInt() while (audioHash2 == audioHash1) audioHash2 = randomInt()stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/audio/PlaybackTimer.kt (1)
49-72: Preview usesChatPreviewThemeinstead of@StreamPreviewhelpers.The coding guidelines state that Compose previews should use
@StreamPreviewhelpers. The rest of the file's preview composables in the codebase may follow the same pattern, so this might be an existing convention — flagging for awareness.As per coding guidelines,
**/stream-chat-android-compose/**/*.kt: "Compose previews should use@StreamPreviewhelpers".stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/AudioRecordAttachmentContent.kt (2)
306-323:SpeedButtonuses rawText+clickableinstead of a semantically accessible button.The
PlaybackToggleButtonproperly usesStreamButton(which appliesRole.Button), butSpeedButtonmanually composesTextwith a.clickablemodifier. This means it lacks button semantics for accessibility (TalkBack won't announce it as a button).Consider wrapping with
StreamButtonfor consistency, or at minimum adding.semantics { role = Role.Button }.
325-345: Preview usesChatPreviewThemeinstead of@StreamPreviewhelpers.The coding guidelines state that Compose previews should use
@StreamPreviewhelpers. This applies to both this file and the preview content file. IfChatPreviewThemeis the established pattern and@StreamPreviewrefers to it, this is fine — but worth confirming consistency. Based on learnings: "Compose previews should use StreamPreview helpers."stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/FileAttachmentContent.kt (2)
154-155: Open TODO:// TODO [G.] semantic value?This TODO questions whether
fileAttachmentShapeshould have a semantic name/token. Note thatAudioRecordAttachmentContent.kt(line 110) uses the sameRoundedCornerShape(StreamTokens.radiusLg)inline. If you resolve this by promoting the shape to a shared token (e.g., inStreamTokensorChatTheme.shapes), both files could reference it.Would you like me to open an issue to track unifying the attachment shape into a shared token?
258-272: Preview functions use bareChatThemewhile audio previews useChatPreviewTheme.
ChatPreviewThemebuilds aChatClientbefore wrapping inChatTheme. If these previews don't requireChatClient, bareChatThemeis fine, but the inconsistency across files in this PR is worth noting. Consider aligning on one pattern.
...ava/io/getstream/chat/android/compose/ui/attachments/content/AudioRecordAttachmentContent.kt
Outdated
Show resolved
Hide resolved
...hat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/StreamColors.kt
Show resolved
Hide resolved
...roid-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/WaveformSliderStyle.kt
Outdated
Show resolved
Hide resolved
...hat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/ModifierUtils.kt
Outdated
Show resolved
Hide resolved
.../src/main/kotlin/io/getstream/chat/android/ui/common/state/messages/list/AudioPlayerState.kt
Outdated
Show resolved
Hide resolved
gpunto
force-pushed
the
redesign/message-file-audio
branch
3 times, most recently
from
7a23b74 to
136d3b1
Compare
gpunto
force-pushed
the
redesign/message-file-audio
branch
from
136d3b1 to
58545bc
Compare
|
@coderabbitai review |
gpunto
force-pushed
the
redesign/message-file-audio
branch
from
a31d2b7 to
603f3a2
Compare
VelikovPetar
left a comment
VelikovPetar
left a comment
There was a problem hiding this comment.
Left couple of questions but looks pretty good!
.../src/main/kotlin/io/getstream/chat/android/ui/common/state/messages/list/AudioPlayerState.kt
Show resolved
Hide resolved
.../main/java/io/getstream/chat/android/compose/ui/attachments/content/FileAttachmentContent.kt
Show resolved
Hide resolved
|
2 participants
Goal
Update audio recording and file attachments design. For audio recording, this also entailed changing the playback speed logic to allow for 1) setting different speed per attachment and 2) changing speed while the audio is not playing
Implementation
Updated designs and audio speed controlling logic. Now it's handled per audio hash instead of being tied to a single aucio.
🎨 UI Changes
Testing
Can be tested in the sample by checking the designs and that the audio recording
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Theme Updates