fix: thumbnails are not removed when a file is deleted automatically on user removal #38105
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: bd502ff 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 |
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
WalkthroughExtends user-deletion file cleanup to handle messages referencing either a single Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
Admin->>Server: request deleteUser(userId)
Server->>MessagesModel: findFilesByUserId(userId)
MessagesModel->>Database: query { $or: [file._id, files._id] } + projection
Database-->>MessagesModel: cursor (items with file and/or files)
Server->>Cursor: iterate items
loop per item
Cursor-->>Server: item { file?, files? }
alt item.files exists
Server->>Store: deleteById(fileId) for each files._id
end
alt item.file exists and not in files
Server->>Store: deleteById(file._id)
end
end
Server-->>Admin: deletedRooms result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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 |
![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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/models/src/models/Messages.ts (1)
771-777: Query logic is correct, but consider adding an index forfiles._id.The updated implementation correctly handles both single file references and files arrays using an
$orcondition. The projection appropriately includes both fields.However, there's an index for
file._id(line 56) but no corresponding index forfiles._id. Since this query is part of user deletion (typically infrequent), it may not be performance-critical, but for repositories with many messages, adding an index forfiles._idcould improve query performance.apps/meteor/app/lib/server/functions/deleteUser.ts (1)
76-87: File deletion logic correctly handles both single and multiple files, including thumbnails.The implementation properly:
- Extracts all file IDs from the
filesarray (which includes both regular files and thumbnails stored as separate records withtypeGroup: 'thumb')- Deletes each file individually in sequence via
for await- Additionally deletes the legacy
file._idif it exists and wasn't already deletedThe use of sequential
for awaitinstead ofPromise.allis intentional, as indicated by the commit message. Thumbnails are automatically handled because they exist as separate upload records in thefilesarray alongside their parent files.However, if
store.deleteByIdthrows an error for any file, the entire deletion process halts. Consider wrapping individual deletions in try-catch blocks to ensure all files are attempted even if some fail:🛡️ Optional error handling improvement
for await (const { file, files } of cursor) { const fileIds = files?.map(({ _id }) => _id) || []; for await (const fileId of fileIds) { if (fileId) { + try { await store.deleteById(fileId); + } catch (error) { + console.error(`Failed to delete file ${fileId}:`, error); + } } } if (file?._id && !fileIds.includes(file._id)) { + try { await store.deleteById(file._id); + } catch (error) { + console.error(`Failed to delete file ${file._id}:`, error); + } } }
📜 Review details
Configuration used: Organization 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.
📒 Files selected for processing (4)
.changeset/stupid-keys-double.mdapps/meteor/app/lib/server/functions/deleteUser.tspackages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.tsapps/meteor/app/lib/server/functions/deleteUser.ts
🧠 Learnings (1)
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
apps/meteor/app/lib/server/functions/deleteUser.ts
🧬 Code graph analysis (2)
packages/model-typings/src/models/IMessagesModel.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(145-242)
packages/models/src/models/Messages.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(145-242)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
packages/model-typings/src/models/IMessagesModel.ts (1)
154-154: LGTM! Type signature correctly updated to include files array.The return type now includes both
fileandfilesfields, which aligns with the implementation changes and enables proper handling of messages with multiple file attachments..changeset/stupid-keys-double.md (1)
1-7: LGTM! Changeset properly documents the fix.The changeset correctly identifies the affected packages and clearly describes the fix for thumbnail removal during user deletion.
📦 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/09 07:10 (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: Fri, 09 Jan 2026 07:10:00 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38105 +/- ##
===========================================
+ Coverage 70.69% 70.71% +0.02%
===========================================
Files 3146 3146
Lines 108829 108829
Branches 19564 19598 +34
===========================================
+ Hits 76933 76961 +28
+ Misses 29896 29868 -28
Partials 2000 2000
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Issue(s)
CORE-1621
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.