feat: implement delete message feature in chat room #573
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request implements message deletion functionality in the room chat feature. It adds an Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Room Chat UI
participant Controller as Room Chat Controller
participant Appwrite as Appwrite Backend
participant RealtimeDB as Realtime Subscription
User->>UI: Long-press message
UI->>UI: Show context menu
User->>UI: Tap "Delete"
UI->>UI: Show confirmation dialog
User->>UI: Confirm deletion
UI->>Controller: deleteMessage(messageId)
activate Controller
Controller->>Controller: Find message in list
Controller->>Controller: Mark isdeleted=true, clear content
Controller->>Appwrite: Update message document
activate Appwrite
Appwrite->>Appwrite: Persist deletion
Appwrite-->>Appwrite: Broadcast realtime event
Appwrite-->>Controller: Update confirmation
deactivate Appwrite
Controller->>Controller: Send deletion notification
Controller->>UI: Rebuild with updated message
deactivate Controller
RealtimeDB->>UI: Propagate isdeleted from subscription
UI->>UI: Render deleted message placeholder
User->>User: Sees "This message was deleted"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Rationale: The changes introduce a focused feature across three files with clear intent. Model updates are straightforward (single field addition with proper serialization). Controller logic is simple (deletion marking and notification dispatch). However, the UI layer introduces moderate complexity with conditional rendering branches, permission checks, context menus, and state-dependent styling that require careful verification of logic paths. Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
|
🎉 Welcome @4555jan!
We appreciate your contribution! 🚀 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/models/message.dart (1)
69-84: Null-safe deserialization for deleted messages
contentcan be null in the DB after deletion; assigning null to a non-nullableStringwill crash at runtime. Guard it.Message.fromJson(Map<String, dynamic> json) : roomId = json['roomId'], @@ - isEdited = json['isEdited'], - content = json['content'], + isEdited = json['isEdited'], + content = (json['content'] as String?) ?? '', creationDateTime = DateTime.parse(json['creationDateTime']), - isdeleted = json['isdeleted'] ?? false, + isdeleted = json['isdeleted'] ?? false, replyTo = json['replyTo'] != null ? ReplyTo.fromJson(json['replyTo']) : null;lib/controllers/room_chat_controller.dart (1)
280-298: Treat deletions separately in realtime updates; guard index −1Avoid “Edited” notifications for deletions and handle out‑of‑order updates.
- if (action == 'update') { + if (action == 'update') { Message updatedMessage = Message.fromJson(data.payload); - var index = messages.indexWhere( + final index = messages.indexWhere( (msg) => msg.messageId == updatedMessage.messageId, ); - messages[index] = messages[index].copyWith( + if (index == -1) { + // Out-of-order update; insert or ignore + messages.add(updatedMessage); + } else { + messages[index] = messages[index].copyWith( content: updatedMessage.content, isEdited: updatedMessage.isEdited, isdeleted: updatedMessage.isdeleted, - ); - if (appwriteRoom != null) { - auth.flutterLocalNotificationsPlugin.show( - 0, - 'Message Edited in ${appwriteRoom?.name ?? appwriteUpcommingRoom!.name}', - '${updatedMessage.creatorName} updated his message: ${updatedMessage.content}', - notificationDetails, - ); - } + ); + } + if (appwriteRoom != null) { + final deleted = updatedMessage.isdeleted; + final title = deleted + ? 'Message Deleted in ${appwriteRoom?.name ?? appwriteUpcommingRoom!.name}' + : 'Message Edited in ${appwriteRoom?.name ?? appwriteUpcommingRoom!.name}'; + final body = deleted + ? '${updatedMessage.creatorName} deleted a message' + : '${updatedMessage.creatorName} updated his message: ${updatedMessage.content}'; + auth.flutterLocalNotificationsPlugin.show( + 0, + title, + body, + notificationDetails, + ); + } }
🧹 Nitpick comments (3)
lib/views/screens/room_chat_screen.dart (3)
121-127: Only enable delete for non-deleted messagesAvoid opening a menu that can only cancel.
- canDelete: - auth.appwriteUser.$id == - chatController.messages[index].creatorId, + canDelete: + auth.appwriteUser.$id == + chatController.messages[index].creatorId && + !chatController.messages[index].isdeleted,
206-243: Add “Edit” to the context menu (when allowed)Matches the PR UX (“Edit, Delete, Cancel”) and improves discoverability.
void _showMessageContextMenu(BuildContext context) { showModalBottomSheet( @@ child: Wrap( children: [ + if (widget.canEdit && !widget.message.isdeleted) + ListTile( + leading: const Icon(Icons.edit), + title: const Text('Edit'), + onTap: () { + Navigator.pop(context); + startEditing(); + }, + ), if (widget.canDelete && !widget.message.isdeleted) ///delete option ListTile(
414-435: Deleted placeholder copy/UIClear and unobtrusive. Consider i18n in a follow-up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/controllers/room_chat_controller.dart(4 hunks)lib/models/message.dart(7 hunks)lib/views/screens/room_chat_screen.dart(10 hunks)
🔇 Additional comments (6)
lib/controllers/room_chat_controller.dart (1)
137-138: Initialize deletion flag on new messagesGood: defaulting to not deleted.
If you adopt
isDeletednaming, update here too.lib/views/screens/room_chat_screen.dart (4)
115-118: Wire-up delete actionHook looks correct.
147-160: ChatMessageItem API additionsConstructor and fields look consistent.
306-309: Visual state for deleted messagesGood UX affordance with greyed bubble.
344-346: Hide reply preview on deletedCorrect guard.
lib/models/message.dart (1)
11-11: Verify complete refactoring scope across all PR filesThe grep results show
isdeletedis referenced in three files, but the diff only coverslib/models/message.dart. Iflib/views/screens/room_chat_screen.dart(8 usages) andlib/controllers/room_chat_controller.dart(4 usages) are included in this PR, they must also be updated to useisDeletedconsistently. Confirm that all files have been refactored and no usages of the old snake_case field name remain.
| /// delete method | ||
| Future<void> deleteMessage(String messageId) async { | ||
| try { | ||
| Message messageToDelete = messages.firstWhere( | ||
| (msg) => msg.messageId == messageId, | ||
| ); | ||
| messageToDelete = messageToDelete.copyWith(content: '', isdeleted: true); | ||
|
|
||
| await databases.updateDocument( | ||
| databaseId: masterDatabaseId, | ||
| collectionId: chatMessagesCollectionId, | ||
| documentId: messageId, | ||
| data: messageToDelete.toJsonForUpload(), | ||
| ); | ||
| if (appwriteUpcommingRoom != null) { | ||
| log('Sending notification for deleted message'); | ||
| var body = json.encode({ | ||
| 'roomId': appwriteUpcommingRoom?.id, | ||
| 'payload': { | ||
| 'title': 'Message Deleted in ${appwriteUpcommingRoom?.name}', | ||
| 'body': '${messageToDelete.creatorName} deleted a message', | ||
| }, | ||
| }); | ||
| var results = await functions.createExecution( | ||
| functionId: sendMessageNotificationFunctionID, | ||
| body: body.toString(), | ||
| ); | ||
| log(results.status); | ||
| } | ||
| log('Message deleted successfully'); | ||
| } catch (e) { | ||
| log('Error deleting message: $e'); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Harden delete path: enforce ownership, handle not-found
Prevent deleting others’ messages client-side (in addition to backend ACLs) and avoid firstWhere throw.
/// delete method
Future<void> deleteMessage(String messageId) async {
try {
- Message messageToDelete = messages.firstWhere(
- (msg) => msg.messageId == messageId,
- );
- messageToDelete = messageToDelete.copyWith(content: '', isdeleted: true);
+ final idx = messages.indexWhere((m) => m.messageId == messageId);
+ if (idx == -1) {
+ log('Message not found: $messageId');
+ return;
+ }
+ var messageToDelete = messages[idx];
+ if (messageToDelete.creatorId != auth.appwriteUser.$id) {
+ log('Forbidden: cannot delete others\' messages');
+ return;
+ }
+ messageToDelete =
+ messageToDelete.copyWith(content: '', isdeleted: true);
@@
- data: messageToDelete.toJsonForUpload(),
+ data: {
+ // Update only the fields that change
+ 'content': '',
+ 'isdeleted': true,
+ },
);Confirm backend rules also restrict updates to message owner to avoid client bypass.
I'll verify the review comment by examining the codebase for the relevant concerns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find Message model definition
echo "=== Searching for Message model definition ==="
fd -e dart | xargs rg -n "class Message" -A 10 | head -50
Length of output: 549
🏁 Script executed:
#!/bin/bash
# Search for auth object and appwriteUser usage
echo "=== Searching for auth object and appwriteUser ==="
rg -n "auth\.appwriteUser" lib/controllers/ -C 2Length of output: 496
🏁 Script executed:
#!/bin/bash
# Check the Message model fields, especially creatorId and creatorName
echo "=== Searching for creatorId field in Message ==="
rg -n "creatorId|creatorName" lib/ -C 1 | head -60Length of output: 3646
🏁 Script executed:
#!/bin/bash
# Look for toJsonForUpload method to understand what data is being sent
echo "=== Searching for toJsonForUpload implementation ==="
rg -n "toJsonForUpload" lib/ -A 5 | head -50Length of output: 1870
🏁 Script executed:
#!/bin/bash
# Check if there are any Appwrite ACL or permission configurations
echo "=== Searching for Appwrite permissions and ACL configuration ==="
rg -n "permissions|Permissions|ACL|acl" lib/ | grep -i "message\|delete\|document" | head -30Length of output: 64
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for serverless function implementations that might handle backend validation
echo "=== Searching for function handlers and backend validation ==="
rg -n "sendMessageNotification|Function|handler" lib/ -C 2 | head -50
Length of output: 3336
🏁 Script executed:
#!/bin/bash
# Look for any Appwrite Rules configuration files
echo "=== Searching for Appwrite config files ==="
fd -e json -e yaml -e yml | xargs rg -l "appwrite\|permission\|rule" 2>/dev/null | head -20Length of output: 43
🏁 Script executed:
#!/bin/bash
# Check for any backend-related configuration or environment setup
echo "=== Searching for Appwrite database/collection initialization ==="
rg -n "chatMessagesCollectionId|createDocument|updateDocument" lib/ -B 2 -A 2 | head -80Length of output: 5266
🏁 Script executed:
#!/bin/bash
# Look at the complete deleteMessage implementation including surrounding context
echo "=== Full deleteMessage method context ==="
rg -n "deleteMessage" lib/controllers/room_chat_controller.dart -B 2 -A 35Length of output: 1374
Add ownership check and not-found handling to deleteMessage() method
The review identifies legitimate security and error-handling gaps in the current implementation.
Verified issues:
• Missing ownership validation: deleteMessage() lacks a client-side check despite the UI layer already enforcing it (room_chat_screen.dart lines 121-126). Calling the method directly bypasses this protection.
• Unhandled not-found case: firstWhere() throws StateError if the message doesn't exist, caught generically without distinction from other errors.
• Full object serialization: toJsonForUpload() sends all fields including creatorId—though the proposed change only updates content and isdeleted, which is the safer approach.
Backend validation note: No Appwrite ACL configuration is visible in the codebase; backend rules must be verified separately via Appwrite console or documentation to confirm message owner restrictions are enforced server-side.
The proposed changes are sound and should be applied.
| updatedMessage = updatedMessage.copyWith( | ||
| content: newContent, | ||
| isEdited: true, | ||
| isdeleted: false, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Don’t resurrect deleted messages on edit
Editing currently forces isdeleted: false. Block edits for deleted messages and keep the flag unchanged.
updatedMessage = updatedMessage.copyWith(
content: newContent,
isEdited: true,
- isdeleted: false,
);
+ if (updatedMessage.isdeleted) {
+ log('Editing a deleted message is not allowed');
+ return;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In lib/controllers/room_chat_controller.dart around lines 185-190, the copyWith
call forces isdeleted: false which resurrects deleted messages; change the logic
to first check the existing message's isdeleted flag and if it's true, block the
edit (return/throw or no-op) so deleted messages cannot be edited, otherwise
perform the copyWith but do not override isdeleted (preserve the original flag)
— remove the hardcoded isdeleted: false and ensure edits only proceed for
non-deleted messages.
|
Please confirm that you have tested this functionality on more than 1 device (multiple users in the same chat session) |
|
Well I tested on 2 emulators and the video is about that basically if you want 2 real android device video I can share that also @M4dhav |
|
Please share a video recording of the functionality on 2 emulators |
|
@M4dhav i uploaded the video .....what u think about this in the 2nd emulator the after the long press the it still opens should i remove it from there and only the person who messaged it.. allow to open that |
|
@M4dhav in this change dialog box will only open if the sender wants to delete the the message |
|
Since there is a variable added to the backend, you will need to link a. backend PR along with this. |
|
I think it would be better to merge this once that PR is opened as well. |
|
@M4dhav sorry by that I meant is there any front-end related issue or something? |
| log(appwriteUpcommingRoom.toString()); | ||
| } | ||
|
|
||
| /// delete method |
There was a problem hiding this comment.
Remember to add tests for this function
| if (appwriteUpcommingRoom != null) { | ||
| log('Sending notification for deleted message'); | ||
| var body = json.encode({ | ||
| 'roomId': appwriteUpcommingRoom?.id, | ||
| 'payload': { | ||
| 'title': 'Message Deleted in ${appwriteUpcommingRoom?.name}', | ||
| 'body': '${messageToDelete.creatorName} deleted a message', | ||
| }, | ||
| }); | ||
| var results = await functions.createExecution( | ||
| functionId: sendMessageNotificationFunctionID, | ||
| body: body.toString(), | ||
| ); | ||
| log(results.status); | ||
| } | ||
| log('Message deleted successfully'); | ||
| } catch (e) { | ||
| log('Error deleting message: $e'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
No need to send notifications on deleted messages, we can remove tis
| !chatController.messages[index].isEdited, | ||
| !chatController.messages[index].isdeleted, |
There was a problem hiding this comment.
messages cannot be edited twice, so the isEdited check also needs to be there
| Row( | ||
| children: [ | ||
| Icon( | ||
| Icons.delete_outline, | ||
| color: Colors.grey[600], | ||
| size: 18, | ||
| ), |
There was a problem hiding this comment.
we should reove the icon, the representation would be better with just text. subsequently, row can be removed as well
|
@M4dhav I am supposed to add test for delete method only right? |
|
Yes, as a general rule of thumb going forward, contributors are recommended to add tests for any functionality they implement that can be easily tested via unit/widget tests. |
|
@M4dhav i added the attribute to appwrite.json which wil basically create it messages collection when any dev first time runs the script please review it as well i even run the entire script after the change |
|
Backend PR is ready to merge, once you have completed your changes here then I will merge both the PRs at once |
|
@M4dhav i am also supoosed to add mocks file too for the test ? |
|
If your tests need mocks then yes |
|
@M4dhav in tests it is bascially tests for deletes a message Handles message not found error Handles database errors and in the recent pr i added because in the last pr i changed some existing code which was unrelated to my change please check/review if i made any major mistakes again i will fix it asap |
|
@M4dhav "messages cannot be edited twice, so the isEdited check also needs to be there" sorry in my last pr i misundershood this and fixed it in the recent one now messages cannot be edited twice and also made changes in the test description of room_chat_controller |
2 participants
Description
Implemented delete message feature with soft delete functionality. Messages are not permanently removed from the database but are marked as deleted and display "This message was deleted" in the UI. Users can delete their own messages through a long-press context menu, similar to WhatsApp's UX pattern.
Fixes #571
screen-recording-2025-10-21-192333_A3sFcoA6.mp4
Type of change
Changes Made
1. Message Model (
lib/models/message.dart)isdeletedboolean field to track deleted messagesfromJson(),toJson(),toJsonForUpload(), andcopyWith()methodsfalsefor backward compatibility2. RoomChatController (
lib/controllers/room_chat_controller.dart)deleteMessage()method for soft-deleting messages (setsisDeleted: true, clears content)sendMessage()to initializeisdeleted: false3. RoomChatScreen (
lib/views/screens/room_chat_screen.dart)_showMessageContextMenuandconfirm deleteHow Has This Been Tested
Test Scenarios:
Test Configuration:
Database Changes Required
isdeletedboolean attribute to the chat messages collectionisdeleted(case-sensitive)falseChecklist:
Maintainer Checklist
feature,enhancement,`)