fix: removing files from e2ee rooms do not remove their messages #38379
+78
−22
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 25b152c The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughThis PR ensures removed attachments from encrypted messages are consistently represented and handled: adds a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UploadService
participant MessagesDB
participant Decryptor
Note over Client,MessagesDB: Removal flow for encrypted message with file
Client->>UploadService: Request remove message / file
activate UploadService
UploadService->>MessagesDB: Update message attachments -> `removed-file` (type, color, text, fileId?)
deactivate UploadService
Note over Client,Decryptor: Client receives updated message
Client->>Decryptor: Decrypt message content
activate Decryptor
Decryptor->>Decryptor: Detect `removed-file` attachments (isRemovedFileAttachment)
Decryptor->>Client: Return decrypted message with removed-file placeholders
deactivate Decryptor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38379 +/- ##
===========================================
+ Coverage 70.78% 70.85% +0.07%
===========================================
Files 3159 3160 +1
Lines 109401 109788 +387
Branches 19700 19709 +9
===========================================
+ Hits 77435 77786 +351
- Misses 29929 29975 +46
+ Partials 2037 2027 -10
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/28 16:41 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 28 Jan 2026 16:41:34 GMT |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts`:
- Around line 672-678: The second early-return check "if
(!deletedAttachments.length) { return message; }" is redundant because
"!deletedAttachments.length" is already part of the earlier compound condition;
remove the duplicate if-block and keep only the initial guard that checks
deletedAttachments, message.attachments, and content.attachments so the function
returns once when there are no deleted attachments or missing attachments (refer
to the variables deletedAttachments, message.attachments, and
content.attachments to locate the redundant check).
🧹 Nitpick comments (1)
apps/meteor/client/lib/e2ee/rocketchat.e2e.room.ts (1)
680-701: Consider using.find()instead of manual loop for cleaner code.The manual
for...ofloop can be simplified.♻️ Suggested refactor
message.attachments = message.attachments.map((att) => { if (!isFileAttachment(att)) { return att; } if (deletedAllAttachment) { return deletedAllAttachment; } const fileId = att.fileId || message.file?._id; if (!fileId) { return att; } - for (const removedAttachment of deletedAttachments) { - if (removedAttachment.fileId === fileId) { - return removedAttachment; - } - } - - return att; + return deletedAttachments.find((removed) => removed.fileId === fileId) ?? att; });
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
On e2ee messages, the link between the message and the file is also being encrypted. This makes the server unable to delete files based on messages and also unable to delete messages based on files.
This PR changes the backend to save the metadata of the encrypted file on the message (just like it does for non-encrypted messages), so that any code that checks for links between files and messages can still find it.
It also changes the message to keep a list of files that have been deleted on the server so that the client may ignore any references to those files on decrypted data.
The only thing being saved to the message is related to the encrypted version of the file, so attributes like name, format and size will not reflect the real file - as the server doesn't have any info about it.
Issue(s)
CORE-1776
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.