feat: new endpoint to delete uploaded files individually #38173
Merged
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: 0085ab5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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
WalkthroughAdds a new authenticated REST endpoint POST /api/v1/uploads.delete and server-side UploadService methods to authorize and delete individual uploads (including derived thumbnails), updates message attachments when files are removed, and exposes related types, model queries, and client-side expiresAt handling. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant API as "API: /api/v1/uploads.delete"
participant UploadSvc as UploadService
participant Auth as Authorization
participant Model as Uploads Model / DB
participant Message as Message Store
Client->>API: POST { fileId }
API->>API: validate body (isUploadsDeleteParams)
API->>UploadSvc: canDeleteFile(userId, file, msg?)
UploadSvc->>Auth: check message perms / room access
Auth-->>UploadSvc: permission result
UploadSvc-->>API: canDelete (bool)
API->>UploadSvc: deleteFile(user, fileId, msg?)
UploadSvc->>Model: findAllByOriginalFileId(fileId) → derived files
UploadSvc->>Model: remove main file
UploadSvc->>Model: remove derived files (iterate)
UploadSvc->>Message: updateMessageRemovingFiles(message, removedFileIds)
Message->>Model: save updated message
UploadSvc-->>API: { deletedFiles: [...] }
API-->>Client: 200 OK with deletedFiles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
pierre-lehnen-rc
force-pushed
the
feat/uploads.delete
branch
from
c3739d3 to
f8361ae
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38173 +/- ##
===========================================
- Coverage 70.80% 70.77% -0.03%
===========================================
Files 3159 3159
Lines 109364 109401 +37
Branches 19680 19884 +204
===========================================
- Hits 77432 77426 -6
- Misses 29897 29946 +49
+ Partials 2035 2029 -6
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/27 15:50 (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: Tue, 27 Jan 2026 15:50:53 GMT |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 14 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/meteor/app/api/server/v1/uploads.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/uploads.ts:32">
P2: Response schema disallows the `success` field that `API.v1.success()` always adds, so response validation will fail when `additionalProperties: false` is enforced. Add `success` to the schema and required list.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 2
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/v1/uploads.ts`:
- Around line 43-71: The endpoint definition uploadsDeleteEndpoint currently
declares its params under query and reads fileId from this.queryParams; change
the API.v1.post options to use body instead of query, update the validator
reference (isUploadsDeleteParams) to the body schema variant if one exists or
modify it to validate request body, and inside the action handler replace
this.queryParams usage with this.bodyParams (read fileId from body). Ensure the
request validation and any tests are updated accordingly so the POST accepts
fileId in the request body consistent with other v1 POST endpoints.
In `@packages/models/src/models/Uploads.ts`:
- Around line 96-99: The query in findAllByOriginalId is using the wrong field
name; update the filter in the method findAllByOriginalId to query { originalId:
originalFileId } (or rename the parameter to originalId and pass it directly) so
it matches the IUpload interface's originalId property; ensure the FindOptions
usage remains unchanged and run/adjust any related tests that expect derived
uploads to be found/deleted.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/uploads.ts (1)
86-88: Prefer self-documenting code over inline comments.Consider extracting the permission check into a helper and drop inline comments to keep the implementation clean. As per coding guidelines, ...
KevLehman
reviewed
Jan 20, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/model-typings/src/models/IUploadsModel.ts`:
- Around line 21-22: The implementation of findAllByOriginalFileId has a
required parameter but the interface marks options as optional; update the
implementation of findAllByOriginalFileId(originalFileId: string, options?:
FindOptions<IUpload>) to make options optional (match the interface) and, if you
need a default, apply it inside the function body (e.g., const opts = options ??
{}) so the runtime behavior remains the same while the signature matches
IUploadsModel and uses the FindOptions<IUpload> type.
KevLehman
reviewed
Jan 22, 2026
tassoevan
previously approved these changes
Jan 23, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/server/services/upload/service.ts`:
- Around line 71-83: The code updates the message
(this.updateMessageRemovingFiles) before calling removeFileAndDerivates, which
can leave the UI inconsistent if store.deleteById fails; to fix, call await
this.removeFileAndDerivates(fileId, additionalFiles) first, capture its returned
deletedFiles, and only after successful deletion call
this.updateMessageRemovingFiles(msg, deletedFiles, user) (when msg is present),
then return the deletedFiles; ensure errors from removeFileAndDerivates
propagate so the message is not edited on failure.
🧹 Nitpick comments (3)
apps/meteor/server/services/upload/service.ts (3)
52-68: Remove inline comments incanDeleteFile.
The comments at Line 61 and Line 66 aren’t necessary given the readability of the code.As per coding guidelines, avoid code comments in the implementation.♻️ Suggested change
- // If file is not confirmed and was sent by the same user if (file.expiresAt && file.userId === userId) { return canAccessRoomIdAsync(file.rid, userId); } - // It's a confirmed file but it has no message, so use data from the file to run message delete permission checks const msgForValidation = { u: { _id: file.userId }, ts: file.uploadedAt, rid: file.rid };
85-105: Remove inline comments inremoveFileAndDerivates.
The comments at Line 90, Line 93, and Line 95 can be removed without losing clarity.As per coding guidelines, avoid code comments in the implementation.♻️ Suggested change
- // Delete the main file first; await store.deleteById(fileId); - // The main file is already deleted; From here forward we'll return a success response even if some sub-process fails const deletedFiles: IUpload['_id'][] = [fileId]; - // Delete them one by one as the store may include requests to external services for await (const id of additionalFiles) {
112-121: Remove inline comment inupdateMessageRemovingFiles.
The comment at Line 118 is unnecessary given the surrounding logic.As per coding guidelines, avoid code comments in the implementation.♻️ Suggested change
- // If the attachment doesn't have a `fileId`, we assume it's an old message with only one file, in which case checking the id is not needed if (attachment.fileId && !filesToRemove.includes(attachment.fileId)) {
KevLehman
approved these changes
Jan 26, 2026
tassoevan
approved these changes
Jan 26, 2026
Contributor
Author
|
Setting it back to draft while we reconsider how encrypted files should be handled. |
pierre-lehnen-rc
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
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)
Issue(s)
CORE-1666
Steps to test or reproduce
Further comments
API tests for this endpoint will be added to #32703
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.