fix: clean up lastMessage field when deleting user to prevent ghost m… #37360

tanayyo1 · 2025-11-01T10:02:07Z

Proposed changes (including videos or screenshots)

Problem

When a user is deleted from Rocket.Chat, their messages are properly removed from the rocketchat_message collection, but the lastMessage field in rocketchat_room documents is not cleaned up. This field contains embedded user data that becomes stale after user deletion, causing "ghost messages" to appear across all clients.

Root Cause

The lastMessage field structure:

lastMessage: {
  _id: "msg123",
  msg: "Hello world",
  u: { _id: "user123", username: "deleteduser", name: "John Doe" }  // ← Embedded user data
}

<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit

* **Bug Fixes**
  * Prevented deleted users’ messages from remaining as a room’s last message; rooms now reset or update last-message references to a visible prior message to avoid stale user data.

* **Tests**
  * Added end-to-end tests validating last-message cleanup and proper fallback to the previous visible message when a user is deleted.

<sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub>
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

…essages (RocketChat#36885) - Add cleanup logic in deleteUser function to update lastMessage field - Find rooms where deleted user had the last message - Update lastMessage to previous valid message or unset if none exists - Add comprehensive test coverage for the fix Fixes: RocketChat#36885

changeset-bot · 2025-11-01T10:02:12Z

⚠️ No Changeset found

Latest commit: 660cdfa

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

dionisio-bot · 2025-11-01T10:02:12Z

Looks like this PR is not ready to merge, because of the following issues:

This PR is missing the 'stat: QA assured' label
This PR is missing the required milestone or project

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

CLAassistant · 2025-11-01T10:02:14Z

All committers have signed the CLA.

coderabbitai · 2025-11-01T10:02:35Z

Walkthrough

Adds post-deletion cleanup to reset room.lastMessage when a deleted user's message was stored as the room's lastMessage. If Message_ErasureType is Delete and Store_Last_Message is enabled, affected rooms are scanned and lastMessage is replaced with the most recent visible message.

Changes

Cohort / File(s)	Summary
User Deletion Cleanup Implementation `apps/meteor/app/lib/server/functions/deleteUser.ts`	Adds logic to locate rooms whose `lastMessage` references the deleted user, fetch each room's most recent visible message, and update the room's `lastMessage` to that message when message erasure and last-message storage settings require it.
End-to-End Test Suite `apps/meteor/tests/end-to-end/api/users.ts`	Adds a "lastMessage cleanup (Issue `#36885`)" test suite with two tests: one asserting `lastMessage` removal or change after deleting a user whose message was last, and another asserting `lastMessage` updates to the previous (admin) message. Tests use beforeEach/afterEach setup; similar/duplicated cases appear in nearby insertion points.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant DeleteUser as DeleteUserHandler
    participant RoomDB as RoomDB
    participant MessageDB as MessageDB

    Client->>DeleteUser: Request delete user
    DeleteUser->>DeleteUser: Check Message_ErasureType & Store_Last_Message
    alt Erase=Delete and Store_Last_Message=true
        DeleteUser->>RoomDB: Find rooms where lastMessage.user == deletedUser
        RoomDB-->>DeleteUser: Matching rooms list
        loop For each matching room
            DeleteUser->>MessageDB: Query last visible message for room
            MessageDB-->>DeleteUser: Last visible message (or none)
            DeleteUser->>RoomDB: Update room.lastMessage (set to message or null)
        end
    end
    DeleteUser-->>Client: Confirm user deleted and lastMessage cleaned

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Areas needing extra attention:
- Correctness of the message query that identifies the "last visible message" (filtering, ordering, and visibility rules).
- Atomicity/consistency of room updates to avoid races (concurrent messages or deletions).
- The duplicated test cases in apps/meteor/tests/end-to-end/api/users.ts — verify whether duplication is intentional or should be removed/merged.

Possibly related PRs

fix: Orphan team when last owner user deleted #36807 — Modifies deleteUser.ts and user-deletion flow; closely related changes to deletion behavior and return/ownership logic.

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

lucas-a-pelegrino

Poem

🐰
I hopped through rooms where last ghosts lay,
Found hiding messages and cleared the way.
Now when a user fades from view,
The last true line will shine anew.
No phantom names — just honest chat today. ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: fixing lastMessage field cleanup when deleting users to prevent ghost messages, which directly aligns with the PR's primary objective and the code changes.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

Provide your own instructions using the high_level_summary_instructions setting.
Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

📝 Description — Summarize the main change in 50–60 words, explaining what was done.

📓 References — List relevant issues, discussions, documentation, or related PRs.

📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.

📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |

✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (4)

apps/meteor/app/lib/server/functions/deleteUser.ts (2)
100-103: Consider adding error handling for the cleanup operations.

If Messages.getLastVisibleUserMessageSentByRoomId or Rooms.resetLastMessageById fail, the entire user deletion could fail. Consider wrapping the loop in a try-catch or handling errors per-room to prevent blocking the deletion process.
 for (const room of roomsWithDeletedUserLastMessage) {
+  try {
     const lastMessageNotDeleted = await Messages.getLastVisibleUserMessageSentByRoomId(room._id);
     await Rooms.resetLastMessageById(room._id, lastMessageNotDeleted);
+  } catch (error) {
+    // Log error but continue with deletion
+    console.error(`Failed to reset lastMessage for room ${room._id}:`, error);
+  }
 }
100-103: Consider performance implications of sequential processing.

The synchronous loop processes rooms one at a time. For users who authored the last message in many rooms, this could slow down the deletion process. Consider using Promise.all or batch processing.
-for (const room of roomsWithDeletedUserLastMessage) {
-  const lastMessageNotDeleted = await Messages.getLastVisibleUserMessageSentByRoomId(room._id);
-  await Rooms.resetLastMessageById(room._id, lastMessageNotDeleted);
-}
+await Promise.all(
+  roomsWithDeletedUserLastMessage.map(async (room) => {
+    const lastMessageNotDeleted = await Messages.getLastVisibleUserMessageSentByRoomId(room._id);
+    await Rooms.resetLastMessageById(room._id, lastMessageNotDeleted);
+  })
+);
apps/meteor/tests/end-to-end/api/users.ts (2)
3386-3420: Clarify expected behavior in test assertions.

The test doesn't clearly assert the expected state after deletion. Since the test only sends one message (from the target user), after deletion there should be no messages left in the room. The assertion should explicitly verify that lastMessage is either undefined or null, not just check if it exists.

Apply this diff to make the expected behavior explicit:
 // Verify lastMessage is cleaned up
 const roomAfterDelete = await request
   .get(api('channels.info'))
   .set(credentials)
   .query({ roomId: room._id })
   .expect(200);

-// lastMessage should either be undefined or point to a different valid message
-if (roomAfterDelete.body.channel.lastMessage) {
-  expect(roomAfterDelete.body.channel.lastMessage.u).to.not.have.property('_id', targetUser._id);
-}
+// Since the target user's message was the only message, lastMessage should be undefined after cleanup
+expect(roomAfterDelete.body.channel.lastMessage).to.be.undefined;
3422-3468: Refactor test setup for clarity.

The test setup is confusing because beforeEach sends a message from targetUser that becomes irrelevant in this test scenario. The test then sends an admin message followed by another targetUser message. This creates three messages total, making the flow harder to understand.

Consider moving the admin message to beforeEach and removing the redundant first target user message from the test, or better yet, create a separate beforeEach for this specific test.
-it('should update lastMessage to previous message when deleting user whose message was the last', async () => {
+it('should update lastMessage to previous message when deleting user whose message was the last', async () => {
   await updatePermission('delete-user', ['admin']);

-  // Send another message as admin user BEFORE the target user's message
+  // Clear the existing message and set up a clean test scenario
+  // Send admin message first
   await request
     .post(api('chat.sendMessage'))
     .set(credentials)
     .send({
       message: {
         rid: room._id,
-        msg: 'Admin message before target user message',
+        msg: 'Admin message',
       },
     })
     .expect(200);

   // Send the target user message (will be lastMessage)
   const targetUserCredentials = await login(targetUser.username, password);
   await request
     .post(api('chat.sendMessage'))
     .set(targetUserCredentials)
     .send({
       message: {
         rid: room._id,
-        msg: 'Target user last message',
+        msg: 'Target user message',
       },
     })
     .expect(200);
Alternatively, consider using a separate describe block with its own beforeEach that doesn't send the initial target user message.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9a18288 and ae2a43a.

📒 Files selected for processing (2)

apps/meteor/app/lib/server/functions/deleteUser.ts (1 hunks)
apps/meteor/tests/end-to-end/api/users.ts (1 hunks)

🧰 Additional context used

🧠 Learnings (12)