Conversation
* fix: resolve notification deserialization and user click issues - Make `NotificationDto.body` nullable to prevent silent deserialization exceptions on non-JSON data. - Extract `senderId` from DTO and map it to `actorId` in domain and UI models. - Update `NotificationItem` to pass the correct `actorId` to `onUserClick` instead of the display name. * fix: re-trigger CI build for Kilo Code Review - Make `NotificationDto.body` nullable to prevent silent deserialization exceptions on non-JSON data. - Extract `senderId` from DTO and map it to `actorId` in domain and UI models. - Update `NotificationItem` to pass the correct `actorId` to `onUserClick` instead of the display name. * fix: re-trigger CI build for Kilo Code Review again - Make `NotificationDto.body` nullable to prevent silent deserialization exceptions on non-JSON data. - Extract `senderId` from DTO and map it to `actorId` in domain and UI models. - Update `NotificationItem` to pass the correct `actorId` to `onUserClick` instead of the display name.
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![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 implements real-time message reactions and enhances notification navigation by utilizing actor IDs. Key updates include a new reaction subscription in ChatSubscriptionDelegate, state management for incoming reactions in ChatViewModel, and schema adjustments for reaction types. Feedback identifies a logic error in reaction counting that ignores removals, suggests scoping reaction subscriptions to specific chats for efficiency, and recommends safer null-handling for actor IDs in the UI.
| current.updateById(reaction.messageId) { msg -> | ||
| val counts = msg.reactions.toMutableMap() | ||
| counts[reactionType] = (counts[reactionType] ?: 0) + 1 | ||
| val userReaction = if (reaction.userId == currentUserId) reactionType else msg.userReaction | ||
| msg.copy(reactions = counts, userReaction = userReaction) | ||
| } |
There was a problem hiding this comment.
The logic for handling incoming real-time reactions is flawed. It assumes every reaction event is an addition and unconditionally increments the count with (counts[reactionType] ?: 0) + 1. This doesn't handle cases where a user removes a reaction, which will lead to incorrect reaction counts on other clients' devices. The real-time event should either contain information about whether it's an addition or removal, or the client should refetch the counts to ensure correctness. The current implementation will lead to UI inconsistencies.
| } | ||
|
|
||
| reactionSubscriptionJob = viewModelScope.launch { | ||
| subscribeToMessageReactionsUseCase().collect { reaction -> |
There was a problem hiding this comment.
The subscription to message reactions appears to be global rather than scoped to the current chat. Other subscriptions in this class, such as for messages and typing status, use the chatId. Subscribing to all reactions for the user could be inefficient if the volume of reactions is high, as the client would process events for chats that are not currently open. Consider scoping this subscription to the chatId if the backend supports it to improve performance.
| subscribeToMessageReactionsUseCase().collect { reaction -> | |
| subscribeToMessageReactionsUseCase(chatId).collect { reaction -> |
| contentDescription = "Avatar", | ||
| size = Sizes.IconGiant, | ||
| onClick = { onUserClick(actorNameString) } | ||
| onClick = { onUserClick(notification.actorId ?: "") } |
There was a problem hiding this comment.
Passing an empty string to onUserClick when notification.actorId is null might lead to unexpected behavior, such as attempting to navigate to a profile with an empty ID. It would be safer to prevent the click action altogether if the ID is not available.
| onClick = { onUserClick(notification.actorId ?: "") } | |
| onClick = { notification.actorId?.let(onUserClick) } |
| style = MaterialTheme.typography.bodyLarge, | ||
| fontWeight = FontWeight.Bold, | ||
| modifier = Modifier.clickable(onClick = { onUserClick(actorNameString) }) | ||
| modifier = Modifier.clickable(onClick = { onUserClick(notification.actorId ?: "") }) |
There was a problem hiding this comment.
Passing an empty string to onUserClick when notification.actorId is null might lead to unexpected behavior, such as attempting to navigate to a profile with an empty ID. It would be safer to prevent the click action altogether if the ID is not available.
| modifier = Modifier.clickable(onClick = { onUserClick(notification.actorId ?: "") }) | |
| modifier = Modifier.clickable(onClick = { notification.actorId?.let(onUserClick) }) |
| @SerialName("message_id") val messageId: String, | ||
| @SerialName("user_id") val userId: String, | ||
| @SerialName("reaction_emoji") val reactionEmoji: String, | ||
| @SerialName("reaction_type") val reactionEmoji: String, |
There was a problem hiding this comment.
The property name reactionEmoji is now misleading as it's mapped to the reaction_type field in JSON. To improve code clarity and consistency with the backend schema, consider renaming the property to reactionType as well.
| @SerialName("reaction_type") val reactionEmoji: String, | |
| @SerialName("reaction_type") val reactionType: String, |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge This PR successfully addresses several bugs:
Files Reviewed (13 files)
Reviewed by minimax-m2.5-20260211 · 513,289 tokens |
1 participant
Fix notifications deserialization and user click bugs (#38)
NotificationDto.bodynullable to prevent silent deserialization exceptions on non-JSON data.senderIdfrom DTO and map it toactorIdin domain and UI models.NotificationItemto pass the correctactorIdtoonUserClickinstead of the display name.NotificationDto.bodynullable to prevent silent deserialization exceptions on non-JSON data.senderIdfrom DTO and map it toactorIdin domain and UI models.NotificationItemto pass the correctactorIdtoonUserClickinstead of the display name.NotificationDto.bodynullable to prevent silent deserialization exceptions on non-JSON data.senderIdfrom DTO and map it toactorIdin domain and UI models.NotificationItemto pass the correctactorIdtoonUserClickinstead of the display name.