Fix chat message pagination bugs#36
Conversation
- Moved the `isLoadingMore` block to the bottom of `LazyColumn` in `ChatMessageList` to render it at the top in a reverse layout. - Added a guard `!isLoadingMore` in `ChatMessageList`'s `LaunchedEffect` to avoid duplicate load more API calls. - Added `.onFailure` handler to `loadMoreMessages` in `ChatViewModel` to surface loading failures instead of failing silently.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
What this PR does well:
The changes are clean and address the three pagination bugs appropriately. Reviewed by minimax-m2.5 · 107,487 tokens |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces error handling for loading older messages in the ChatViewModel and optimizes the ChatMessageList by preventing redundant load calls and moving the loading indicator to the bottom of the list. Review feedback highlights a potential race condition where error messages from a previous chat could persist after switching views, and suggests refining the loading logic to ensure users can retry failed requests without manual scrolling.
| }.onFailure { e -> | ||
| _error.value = "Failed to load older messages: ${e.message}" |
There was a problem hiding this comment.
There is a potential race condition here. If the user switches to a different chat while loadMoreMessages is in progress, the error message from the previous chat could be shown in the new chat's UI. You should verify that the chatId for which the request was made still matches the currentChatId before updating the state.
Note: This same check should also be applied to the onSuccess block (lines 340-347) to prevent mixing messages from different chats, although those lines are not part of this specific diff hunk.
}.onFailure { e ->
if (chatId == currentChatId) {
_error.value = "Failed to load older messages: ${e.message}"
}| } | ||
| LaunchedEffect(shouldLoadMore.value) { | ||
| if (shouldLoadMore.value) onLoadMore() | ||
| if (shouldLoadMore.value && !isLoadingMore) onLoadMore() |
There was a problem hiding this comment.
Adding the !isLoadingMore check prevents redundant calls while a request is in flight. However, since the LaunchedEffect is only keyed on shouldLoadMore.value, if a request fails and isLoadingMore returns to false, the effect will not re-trigger even if the user remains at the top of the list. The user would have to scroll down and back up to try again. Consider adding isLoadingMore to the LaunchedEffect keys or providing a manual retry mechanism in the UI when an error occurs.
TheRealAshik
merged commit f419d23
into
voice-feature-13089441991693882151
* feat: implement end-to-end voice messaging with PNG carrier - Created VoiceEncoder to encode audio bytes inside a PNG carrier. - Added VoiceRecorder to record mic audio to m4a format with amplitude flow. - Added VoiceUploadService to encode and upload voice messages to ImgBB. - Added VoiceDownloadCache to fetch, decode, and cache voice messages. - Replaced AudioPlayer with VoiceMessagePlayer in MediaPlayers.kt featuring download state and custom Canvas waveform. - Updated ChatInputBar to include hold-to-record mic button, swipe-to-cancel drag gesture, and animated recording indicator row. - Updated ChatScreen to manage permissions, voice recorder state, and send uploaded voice URLs. - Updated AndroidManifest and strings for audio permission and UI hints. * 🐛 fix: voice message not sending after mic release - Fix R.raw.carrier_97b resource reference in VoiceUploadService - Merge two conflicting pointerInput blocks into single awaitEachGesture handler - Add optimistic message in sendVoiceMessage before server response - Remove empty StorageConfig() arg from ChatScreen upload call * 💄 style: reduce bubble padding to Spacing.Small on both axes * 🐛 fix: positionChange() returns Boolean, use position.x - previousPosition.x for swipe detection * 🐛 fix: add OkHttp auth interceptor to Coil singleton loader so AsyncImage gets Supabase headers * 🐛 fix: trim trailing slash from Supabase URL to prevent double-slash in storage URLs * 🐛 fix: don't wrap full http URLs in constructStorageUrl — causes broken image URLs * 🐛 fix: fall back to SynapseConfig.IMGBB_API_KEY when no key is stored in DB or secure storage * fix: address code review and ci compilation issues for voice feature - Fixed data flow in ChatScreen to pass the remote ImgBB URL to `uploadAndSendMedia`, bypassing standard local file upload, as required by architecture constraints. - Removed hallucinated `sendVoiceMessage` from ChatViewModel. - Fixed an `IllegalArgumentException` compilation risk in `VoiceUploadService` due to incorrect `ByteReadChannel` chunk length calculation. - Extracted all hardcoded string literals into `strings.xml`. - Substituted hardcoded values (like 4.dp or 2.dp) with `Sizes` and `Spacing` theme values in `MediaPlayers.kt`. - Replaced the nonexistent `getPlayerInstance` method call with a visual-only toggle for playback speed to avoid compile failures in `MediaPlayers.kt`. * Fix chat message pagination bugs (#36) - Moved the `isLoadingMore` block to the bottom of `LazyColumn` in `ChatMessageList` to render it at the top in a reverse layout. - Added a guard `!isLoadingMore` in `ChatMessageList`'s `LaunchedEffect` to avoid duplicate load more API calls. - Added `.onFailure` handler to `loadMoreMessages` in `ChatViewModel` to surface loading failures instead of failing silently.
1 participant
Fix three pagination bugs in chat messages.
PR created automatically by Jules for task 4293990280079677989 started by @TheRealAshik