fix: prevent file content buffering in user uploads #38071
Conversation
🦋 Changeset detectedLatest commit: 0b8781e 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 |
|
Looks like this PR is ready to merge! 🎉 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Busboy-based MultipartUploadHandler that streams uploads to temp files, threads the raw IncomingMessage into API action contexts, and updates router, middleware, file upload core, UFS, endpoints, apps bridge, and tests to operate on temp-file paths (string) or streams instead of buffering file contents in memory. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Logger as Logger
participant Router as Router
participant Handler as Handler
participant MUH as MultipartUploadHandler
participant Busboy as Busboy
participant FS as FS
participant Store as Store
Client->>Logger: POST /upload (multipart/form-data)
Logger->>Router: forward request (no multipart parsing)
Router->>Handler: invoke handler with c.env.incoming
Handler->>MUH: parseRequest(incoming, options)
MUH->>Busboy: feed request stream
Busboy->>FS: stream file -> temp path (apply transforms)
FS-->>MUH: temp file written (size recorded)
MUH-->>Handler: return { file: ParsedUpload, fields }
Handler->>Store: insert(fileData, file.tempFilePath)
Store->>FS: finalize and cleanup temp file
Store-->>Handler: upload result
Handler-->>Client: 200 OK / upload details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38071 +/- ##
===========================================
- Coverage 70.64% 70.60% -0.05%
===========================================
Files 3133 3133
Lines 108373 108382 +9
Branches 19492 19521 +29
===========================================
- Hits 76564 76518 -46
- Misses 29813 29872 +59
+ Partials 1996 1992 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 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/16 18:56 (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, 16 Jan 2026 18:56:30 GMT |
ricardogarim
force-pushed
the
refactor/s3-large-uploads
branch
3 times, most recently
from
bb808eb to
301d878
Compare
ricardogarim
force-pushed
the
refactor/s3-large-uploads
branch
2 times, most recently
from
b287bf2 to
9a488cc
Compare
d-gubert
force-pushed
the
refactor/s3-large-uploads
branch
3 times, most recently
from
9f30d70 to
e408d2d
Compare
d-gubert
force-pushed
the
refactor/s3-large-uploads
branch
from
37cce45 to
8ddd8a2
Compare
|
@coderabbitai review |
✅ 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/meteor/server/ufs/ufs-methods.ts (1)
72-76: Potential temp file leak in error path.When an error occurs before
store.writeis invoked (e.g.,findOnefails,validatefails, orcreateReadStreamthrows), the catch block removes the file from the store but doesn't clean up the temp file. This could leave orphaned temp files on disk.Consider adding temp file cleanup in the catch block:
🐛 Proposed fix
} catch (err: any) { // If write failed, remove the file await store.removeById(fileId, { session: options?.session }); + void removeTempFile(); reject(new Meteor.Error('ufs: cannot upload file', err)); }apps/meteor/ee/server/apps/communication/rest.ts (2)
331-348: Missing temp file cleanup after reading.After reading the uploaded file with
readFile(file.tempFilePath), the temporary file is not cleaned up. This could accumulate disk space over time, especially for large app packages.Proposed fix
if (!file) { return API.v1.failure({ error: 'No file was uploaded' }); } buff = await readFile(file.tempFilePath); + await UploadService.cleanup(file.tempFilePath); permissionsGranted = (() => {
826-843: Missing temp file cleanup in update route.Same issue as the install route - the temporary file at
file.tempFilePathis not cleaned up after reading.Proposed fix
if (!file) { return API.v1.failure({ error: 'No file was uploaded' }); } buff = await readFile(file.tempFilePath); + await UploadService.cleanup(file.tempFilePath); permissionsGranted = (() => {apps/meteor/app/api/server/v1/emoji-custom.ts (1)
118-141: Missing temp file cleanup in create route.After reading the emoji file buffer, the temporary file should be cleaned up. Consider adding cleanup in both success and error paths.
Proposed fix with try/finally for cleanup
if (!file) { throw new Meteor.Error('error-no-file-uploaded', 'No file was uploaded'); } - const fileBuffer = await readFile(file.tempFilePath); + let fileBuffer: Buffer; + try { + fileBuffer = await readFile(file.tempFilePath); + } finally { + await UploadService.cleanup(file.tempFilePath); + } const isUploadable = await Media.isImage(fileBuffer);apps/meteor/app/api/server/v1/users.ts (1)
275-306: Missing temp file cleanup after successful avatar upload.After calling
setUserAvatar, the temp file atfile.tempFilePathis never cleaned up. This could lead to disk space exhaustion over time.Proposed fix
- await setUserAvatar(user, await readFile(file.tempFilePath), file.mimetype, 'rest'); + try { + await setUserAvatar(user, await readFile(file.tempFilePath), file.mimetype, 'rest'); + } finally { + await UploadService.cleanup(file.tempFilePath); + }
🤖 Fix all issues with AI agents
In @apps/meteor/app/api/server/lib/UploadService.ts:
- Around line 176-178: The filesLimit event handler in UploadService currently
rejects with a plain string ('Just 1 file is allowed'), causing inconsistent
error types versus other limit events; change the rejection to throw/reject a
Meteor.Error with a matching error code/message (e.g., new
Meteor.Error('files-limit', 'Just 1 file is allowed')) in the
bb.on('filesLimit', ...) handler so it matches the other limit event handlers
like the ones that use Meteor.Error.
- Around line 129-136: The transform streams in UploadService.ts (the loop that
reassigns currentStream using options.transforms and the final
currentStream.pipe(writeStream)) do not attach 'error' handlers, so transform
failures may never reject the upload promise; add 'error' listeners on each
transform instance as you iterate (and also on the initial file stream and
writeStream) that forward the error to the upload's reject callback or a shared
error handler, unpipe/destroy streams on error to prevent resource leaks, and
ensure the same handler only runs once (e.g., by using a boolean flag or
once-wrapped callback) so any transform or writeStream error reliably aborts the
operation and rejects the promise.
In @apps/meteor/app/api/server/v1/assets.ts:
- Around line 40-41: The temp file at file.tempFilePath is never removed after
reading; update the code around the readFile and
RocketChatAssets.setAssetWithBuffer call to ensure the temp file is deleted
(fs.unlink/await unlink) in a finally block so it runs on success or error—use
file.tempFilePath as the target for removal and keep the existing use of
fileBuffer and RocketChatAssets.setAssetWithBuffer to set the asset.
- Around line 26-28: The endpoint in apps/meteor/app/api/server/v1/assets.ts
currently throws a raw Error("No file was uploaded") which is inconsistent with
other endpoints; replace that raw throw with the project-standard error handling
(either call API.v1.failure('No file was uploaded') to return a proper API
response or throw a Meteor.Error with an appropriate error id/message) so the
client receives a structured 400-style failure; locate the check for file
presence in the upload handler (the block containing if (!file) { ... }) and
swap the throw for the consistent API.v1.failure(...) or new Meteor.Error(...)
usage used elsewhere.
In @apps/meteor/app/api/server/v1/emoji-custom.ts:
- Around line 184-198: The update route reads an uploaded temp file but never
deletes it, leaving orphaned temp files; wrap the file handling in a try/finally
(or ensure a finally branch) around the
readFile/insertOrUpdateEmoji/uploadEmojiCustomWithBuffer sequence and call the
async unlink on file.tempFilePath to remove the temporary file whether the
operation succeeds or fails; update the code paths that use isNewFile,
file.tempFilePath, readFile, insertOrUpdateEmoji, and
uploadEmojiCustomWithBuffer to perform the cleanup, and import/use
fs.promises.unlink (or equivalent) to delete the temp file.
In @apps/meteor/app/file-upload/server/lib/FileUpload.ts:
- Around line 391-419: The code caches the setting into shouldStripExif but
later re-reads settings.get('Message_Attachments_Strip_Exif') when deciding to
strip metadata; change that conditional to use the cached shouldStripExif
variable instead (in the branch after rotated check), i.e., replace the direct
settings.get call with shouldStripExif so
UploadService.stripExifFromFile(tmpFile) is invoked consistently based on the
previously read shouldStripExif value.
In @apps/meteor/server/ufs/ufs-store.ts:
- Around line 168-171: The finishHandler currently returns early when
file.complete is true without invoking the caller callback, which can leave the
writer waiting; update finishHandler so that when file.complete is true it calls
the existing callback with the existing file (e.g., callback(null, file) or the
appropriate success signature used elsewhere) before returning, and ensure you
preserve the callback-once semantics (don’t call it twice) by checking any local
"done" / "called" flag or reusing the same guard used elsewhere in this
function.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/lib/UploadService.ts (1)
68-68: Consider typing thelimitsobject.Using
anybypasses type safety. Busboy accepts a typed limits object.Suggested typing
- const limits: any = { files: 1 }; + const limits: busboy.Limits = { files: 1 };
📜 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 (19)
apps/meteor/app/api/server/definition.tsapps/meteor/app/api/server/lib/UploadService.tsapps/meteor/app/api/server/middlewares/logger.tsapps/meteor/app/api/server/router.tsapps/meteor/app/api/server/v1/assets.tsapps/meteor/app/api/server/v1/emoji-custom.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/app/api/server/v1/users.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.tsapps/meteor/app/livechat/imports/server/rest/upload.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/server/startup/initialData.tsapps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/server/ufs/ufs-methods.tsapps/meteor/server/ufs/ufs-store.tsapps/meteor/tests/e2e/image-upload.spec.tspackages/http-router/src/Router.spec.tspackages/http-router/src/Router.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
apps/meteor/server/ufs/ufs-methods.tsapps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/app/api/server/definition.tsapps/meteor/app/api/server/middlewares/logger.tsapps/meteor/server/ufs/ufs-local.spec.tspackages/http-router/src/Router.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/api/server/lib/UploadService.tsapps/meteor/server/startup/initialData.tsapps/meteor/app/api/server/v1/users.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/api/server/v1/emoji-custom.tspackages/http-router/src/Router.tsapps/meteor/app/file-upload/server/lib/FileUpload.tsapps/meteor/app/api/server/router.tsapps/meteor/app/livechat/imports/server/rest/upload.tsapps/meteor/server/ufs/ufs-store.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/app/api/server/v1/assets.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/server/ufs/ufs-local.spec.tspackages/http-router/src/Router.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/image-upload.spec.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/image-upload.spec.ts
🧠 Learnings (20)
📚 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/server/ufs/ufs-methods.tsapps/meteor/server/ufs/ufs-store.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/server/ufs/ufs-local.spec.tspackages/http-router/src/Router.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/ee/server/apps/communication/rest.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-09-15T13:10:30.049Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
Applied to files:
packages/http-router/src/Router.spec.tspackages/http-router/src/Router.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
packages/http-router/src/Router.spec.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/startup/initialData.tsapps/meteor/app/api/server/v1/rooms.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/server/startup/initialData.tsapps/meteor/app/api/server/v1/users.tsapps/meteor/app/api/server/v1/rooms.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/api/server/v1/rooms.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
apps/meteor/app/api/server/v1/rooms.ts
🧬 Code graph analysis (8)
apps/meteor/app/api/server/lib/UploadService.ts (1)
packages/core-services/src/index.ts (1)
MeteorError(54-54)
apps/meteor/app/api/server/v1/users.ts (1)
apps/meteor/app/api/server/lib/UploadService.ts (1)
UploadService(31-201)
apps/meteor/ee/server/apps/communication/rest.ts (1)
apps/meteor/app/api/server/lib/UploadService.ts (1)
UploadService(31-201)
apps/meteor/app/api/server/v1/emoji-custom.ts (2)
apps/meteor/app/api/server/lib/UploadService.ts (1)
UploadService(31-201)packages/core-services/src/index.ts (1)
Media(163-163)
apps/meteor/app/file-upload/server/lib/FileUpload.ts (2)
apps/meteor/app/api/server/lib/UploadService.ts (1)
UploadService(31-201)apps/meteor/server/ufs/ufs-methods.ts (1)
ufsComplete(10-78)
apps/meteor/app/livechat/imports/server/rest/upload.ts (2)
apps/meteor/app/api/server/lib/UploadService.ts (1)
UploadService(31-201)apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
FileUpload(113-694)
apps/meteor/app/api/server/v1/rooms.ts (1)
apps/meteor/app/api/server/lib/UploadService.ts (1)
UploadService(31-201)
apps/meteor/app/api/server/v1/assets.ts (1)
apps/meteor/app/api/server/lib/UploadService.ts (1)
UploadService(31-201)
⏰ 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). (60)
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
🔇 Additional comments (21)
apps/meteor/server/ufs/ufs-local.spec.ts (1)
99-104: LGTM!The test correctly reflects the updated behavior where
unlinkis now called twice (for both temp and final files). The inline comment provides good clarity on the expected behavior.apps/meteor/server/ufs/ufs-methods.ts (1)
22-25: LGTM!The simplified Promise-based cleanup is a good improvement. Catching errors and logging a warning is appropriate for cleanup operations—it ensures the upload flow isn't interrupted by cleanup failures while still providing visibility for debugging.
apps/meteor/server/startup/initialData.ts (1)
157-157: LGTM! Simplifies avatar upload by eliminating unnecessary stream conversion.This change removes the intermediate
bufferToStream()step, passing the buffer directly toinsert(). Theinsertmethod signature on the FileUpload store explicitly acceptsstreamOrBuffer: stream.Readable | Buffer | string, so passing the Buffer directly is correct. This aligns with the PR's goal of streamlining upload handling and reducing resource overhead.apps/meteor/server/ufs/ufs-store.ts (2)
173-213: LGTM — proper error handling for async finalization.Wrapping the finalization logic in try/catch ensures that any exceptions during file completion are captured and propagated through the callback rather than being silently swallowed or causing unhandled rejections.
234-236: LGTM — idempotent cleanup with proper ENOENT handling.Using
fs.promises.unlinkwith a catch that ignoresENOENTis the correct pattern for cleanup operations: if the temp file is already gone, that's the desired state.apps/meteor/tests/e2e/image-upload.spec.ts (2)
9-16: LGTM - Proper test setup for new execution order.The test correctly saves both
Message_Attachments_Strip_ExifandFileUpload_RotateImagessettings inbeforeAlland restores them inafterAll, maintaining proper test isolation.
47-62: LGTM - Clear test scenario with helpful comment.The comment at lines 50-51 appropriately documents the dependency between image rotation and EXIF stripping order, ensuring the test properly validates EXIF stripping behavior in isolation.
apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts (1)
34-34: LGTM - UploadService stub added.The stub for
UploadServicefollows the existing proxyquire pattern used throughout this test file, correctly isolating the FileUpload module from the new dependency.packages/http-router/src/Router.spec.ts (1)
449-486: Test correctly validates multipart handling without auto-parsing.The test properly verifies that:
- Multipart/form-data is not auto-parsed by the router
- Raw request is available via
c.env.incomingfor manual parsingThe
c.env.incomingis populated by thehonoAdapterForExpressmiddleware, which passes the Express request object as the Hono context's env variable. The test will work as written.packages/http-router/src/Router.ts (1)
163-166: LGTM - Multipart parsing delegation to UploadService.This change correctly avoids consuming the request body stream at the router level for multipart requests, enabling streaming-based handling via
UploadService.parse(). The empty object fallback is appropriate since multipart routes will parse the body manually.The pattern is that multipart upload routes do not define body validators—they handle their own validation after parsing with
UploadService.parse(). This design avoids the theoretical issue of validation failing on an empty body object.apps/meteor/app/api/server/v1/rooms.ts (1)
200-235: LGTM - Proper use of UploadService with fileStore.The implementation correctly uses
this.incoming(IncomingMessage) forUploadService.parseand passesfile.tempFilePathtofileStore.insert. This approach avoids buffering the entire file in memory. ThefileStore.insertmethod handles the temporary file by renaming it to the UFS temp directory, thenufsCompleteis called which pipes the file to the storage backend and cleans up the temp file viafs.promises.unlinkupon completion.apps/meteor/app/api/server/definition.ts (1)
1-2: LGTM - Type extension for IncomingMessage access.The addition of
incoming: IncomingMessagetoActionThisproperly exposes the raw Node.js request needed byUploadService.parse. This is a clean type-level change supporting the streaming upload refactor.apps/meteor/app/livechat/imports/server/rest/upload.ts (2)
36-41: LGTM - Size limit handling is correct.The conditional
maxFileSize > -1 ? maxFileSize : undefinedcorrectly disables the size limit whenmaxFileSizeis -1 (meaning no limit), allowingUploadService.parseto skip file size enforcement.
43-73: LGTM - File validation and insertion flow.The migration to
UploadService.parseis well-implemented:
- Properly validates file presence and MIME type
- Correctly maps parsed file properties to the details object
- Passes
file.tempFilePathtofileStore.insert, which aligns with the updated_doInsertmethod that handles string paths via renameapps/meteor/app/api/server/router.ts (1)
9-27: LGTM - Clean type extension for raw request access.The
HonoContexttype andAPIActionContextextension correctly expose the Node.jsIncomingMessagethrough Hono's context bindings. This enablesUploadService.parseto work with both the Fetch APIRequestand the raw Node.js stream.apps/meteor/app/api/server/lib/UploadService.ts (3)
114-117: Missing temp file cleanup on field mismatch rejection.When the field name doesn't match, the stream is resumed but the file event handler returns after rejecting. If a temp file was partially created, it won't be cleaned up.
However, looking more closely, at this point
tempFilePathhasn't been assigned yet (it's assigned after this check at line 125), so this is actually fine.
31-62: LGTM - Cleanup and EXIF stripping utilities.The
cleanupmethod properly handles errors gracefully with a warning. ThestripExifFromFilemethod correctly uses a temporary.strippedfile to avoid data loss if the transform fails, and cleans up on error.
91-94: Confirm TypeScript target compatibility withPromise.withResolvers.
Promise.withResolversis available in Node.js 22.0.0+; the project requires Node.js 22.16.0, so runtime support is present. However, TypeScript is configured with targetes2018(inapps/meteor/tsconfig.json), which predates ES2024 whenPromise.withResolverswas introduced. This may cause TypeScript compilation to fail or produce type errors unless the TypeScript library definitions include it. Verify that the TypeScript lib includes Promise.withResolvers or adjust the target/lib configuration accordingly.apps/meteor/app/file-upload/server/lib/FileUpload.ts (3)
805-831: LGTM - Validation flow with content type handling.The
_validateFilemethod properly:
- Returns early if no filter check exists
- Converts
Readablestreams to buffers for validation (necessary since validation may need full content)- Handles
Uint8ArraytoBufferconversion for services compatibility- Passes
undefinedfor string paths since file content validation requires buffer
833-856: LGTM - Flexible content handling in_doInsert.The updated
_doInsertcorrectly handles:
string: Treated as a file path, renamed to temp locationArrayBufferView(Buffer/Uint8Array): Written directly to temp fileReadable: Piped to temp file with proper stream completion viafinished()The
finished()helper with{ cleanup: true }ensures proper resource cleanup on errors.
858-865: LGTM - Clean separation of validation and insertion.The
insertmethod now properly separates concerns:
- Validates content via
_validateFile(which may convert streams to buffers)- Delegates to
_doInsertfor actual file creationThis ensures validation occurs before any file is written to disk.
d-gubert
force-pushed
the
refactor/s3-large-uploads
branch
3 times, most recently
from
51ec79d to
396ebd5
Compare
d-gubert
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 16 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/lib/MultipartUploadHandler.ts">
<violation number="1" location="apps/meteor/app/api/server/lib/MultipartUploadHandler.ts:88">
P2: `parseRequest` never resolves/rejects when the required file is missing, because `writeStreamFinished` stays `false` and the `finish` handler can’t trip `tryResolve`. Initialize `writeStreamFinished` to `true` so the no-file path rejects with `error-no-file` instead of hanging.</violation>
</file>
<file name="apps/meteor/server/ufs/ufs-local.spec.ts">
<violation number="1" location="apps/meteor/server/ufs/ufs-local.spec.ts:103">
P2: `LocalStore.delete` only calls `fs.promises.unlink` once, so this test should keep asserting `calledOnce`. Expecting two calls will cause the test to fail immediately.</violation>
</file>
<file name="apps/meteor/app/file-upload/server/lib/FileUpload.ts">
<violation number="1" location="apps/meteor/app/file-upload/server/lib/FileUpload.ts:416">
P2: Stripping EXIF metadata even when rotation is disabled removes the only orientation data, so images that rely on EXIF rotation now display incorrectly when `Message_Attachments_Strip_Exif` is true but `FileUpload_RotateImages` is false.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
d-gubert
force-pushed
the
refactor/s3-large-uploads
branch
from
0b0841f to
92bada3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/meteor/app/api/server/lib/MultipartUploadHandler.ts`:
- Around line 129-135: The attached transform error handlers only call
file.destroy(), which can leave the writeStream open and the multipart promise
unresolved; update the error handling for each transform in
MultipartUploadHandler so that on 'error' it also destroys/closes the
writeStream, rejects the active upload promise (or calls the upload failure
callback), and ensures temp file cleanup; reference options.transforms,
transform.on('error', ...), file.destroy, currentStream, and writeStream and
make the handler perform file.destroy(), writeStream.destroy()/end(), and
surface the error to the awaiting promise so cleanup and rejection occur.
In `@apps/meteor/app/livechat/imports/server/rest/upload.ts`:
- Around line 49-53: The file validation branch in the upload handler currently
returns API.v1.failure when fileUploadIsValidContentType(file.mimetype) is false
but never removes the temporary file created by parseRequest; modify the branch
in the handler that checks fileUploadIsValidContentType to synchronously or
asynchronously delete the temp file (using fs.unlink or fs.unlinkSync)
referenced by the parsed file object (e.g., file.path or file.tmpPath returned
from parseRequest) inside a try/catch before returning API.v1.failure so the
temp file is always cleaned up on validation failure.
In `@packages/http-router/src/Router.ts`:
- Around line 158-162: The early-return when contentType includes
'multipart/form-data' is correct but the two inline comments must be removed;
edit the Router.ts block containing the contentType check (the condition
referencing contentType and returning parsedBody) to keep the conditional return
as-is and delete the comment lines that reference UploadService.parse or
file-upload rationale (tests are in Router.spec.ts and uploads are handled by
UploadService.parse), leaving only the if
(contentType?.includes('multipart/form-data')) { return parsedBody; } behavior.
🧹 Nitpick comments (5)
.changeset/twelve-sheep-accept.md (1)
6-6: Consider expanding the description to highlight key benefits.The description accurately captures the core improvement but could be more informative for changelog readers. Consider mentioning:
- Prevents crashes during large file uploads
- Reduces memory consumption and resource usage
- Streams files directly to disk instead of buffering in memory
📝 Suggested enhancement
-Improves file upload flow to prevent buffering of contents in memory +Improves file upload flow to prevent buffering of contents in memory, reducing memory consumption and preventing crashes during large file uploads by streaming files directly to diskpackages/http-router/src/Router.ts (1)
159-160: Remove inline comments to comply with no-comments guideline.Inline comments in implementation files are discouraged here. Consider removing these and rely on the PR description/ADR for rationale.
As per coding guidelines, avoid code comments in the implementation.♻️ Suggested change
- // Don't parse multipart here, routes handle it manually via UploadService.parse() - // since multipart/form-data is only used for file uploadsapps/meteor/tests/e2e/image-upload.spec.ts (2)
47-52: Encode intent in the describe name instead of inline comment.The new comment is explanatory but conflicts with the “avoid code comments in implementation” guideline. Consider moving the rationale into the describe title and removing the comment.
♻️ Proposed refactor
- test.describe('strip exif enabled', () => { + test.describe('strip exif enabled (rotation disabled)', () => { test.beforeAll(async ({ api }) => { await setSettingValueById(api, 'Message_Attachments_Strip_Exif', true); - // Image rotation now happens before EXIF stripping, so we need to disable it to properly test it await setSettingValueById(api, 'FileUpload_RotateImages', false); });As per coding guidelines, avoid code comments in implementation.
9-16: Type settings as booleans for accurate test typing.These settings are boolean configs stored to preserve state; keeping them as
unknownweakens type safety. Narrow tobooleanwith an explicit cast after retrieval, consistent with howretention-policy.spec.tshandles similar boolean settings. This aligns with the Playwright test typing guideline.♻️ Proposed refactor
- let defaultStripSetting: unknown; - let defaultRotateSetting: unknown; + let defaultStripSetting: boolean; + let defaultRotateSetting: boolean; let poHomeChannel: HomeChannel; let targetChannel: string; test.beforeAll(async ({ api }) => { - defaultStripSetting = await getSettingValueById(api, 'Message_Attachments_Strip_Exif'); - defaultRotateSetting = await getSettingValueById(api, 'FileUpload_RotateImages'); + defaultStripSetting = (await getSettingValueById(api, 'Message_Attachments_Strip_Exif')) as boolean; + defaultRotateSetting = (await getSettingValueById(api, 'FileUpload_RotateImages')) as boolean;apps/meteor/app/api/server/lib/MultipartUploadHandler.ts (1)
192-201: Minor: Inconsistent rejection style.Line 196 uses
return Promise.reject(...)while other rejection paths use thereject()callback. Consider usingreject()directly for consistency, or return early before piping if no body exists.Suggested consistency fix
if (request instanceof IncomingMessage) { request.pipe(bb); } else { if (!request.body) { - return Promise.reject(new MeteorError('error-no-body', 'Request has no body')); + reject(new MeteorError('error-no-body', 'Request has no body')); + return promise; } const nodeStream = Readable.fromWeb(request.body as any); nodeStream.pipe(bb); }
📜 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 (19)
.changeset/twelve-sheep-accept.mdapps/meteor/app/api/server/definition.tsapps/meteor/app/api/server/lib/MultipartUploadHandler.tsapps/meteor/app/api/server/middlewares/logger.tsapps/meteor/app/api/server/router.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.tsapps/meteor/app/livechat/imports/server/rest/upload.tsapps/meteor/server/startup/initialData.tsapps/meteor/server/ufs/ufs-filter.tsapps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/server/ufs/ufs-methods.tsapps/meteor/server/ufs/ufs-store.tsapps/meteor/tests/e2e/image-upload.spec.tsapps/meteor/tests/end-to-end/api/rooms.tspackages/http-router/src/Router.spec.tspackages/http-router/src/Router.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/meteor/server/startup/initialData.ts
- apps/meteor/app/apps/server/bridges/listeners.ts
- apps/meteor/server/ufs/ufs-store.ts
- apps/meteor/app/api/server/v1/rooms.ts
- packages/http-router/src/Router.spec.ts
- apps/meteor/server/ufs/ufs-methods.ts
- apps/meteor/tests/end-to-end/api/rooms.ts
- apps/meteor/app/api/server/definition.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
apps/meteor/server/ufs/ufs-local.spec.tspackages/http-router/src/Router.tsapps/meteor/server/ufs/ufs-filter.tsapps/meteor/app/api/server/router.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.tsapps/meteor/app/api/server/lib/MultipartUploadHandler.tsapps/meteor/app/livechat/imports/server/rest/upload.tsapps/meteor/app/api/server/middlewares/logger.tsapps/meteor/tests/e2e/image-upload.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
apps/meteor/tests/e2e/**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created inapps/meteor/tests/e2e/directory
Avoid usingpage.locator()in Playwright tests - always prefer semantic locators such aspage.getByRole(),page.getByLabel(),page.getByText(), orpage.getByTitle()
Usetest.beforeAll()andtest.afterAll()for setup/teardown in Playwright tests
Usetest.step()for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test,page,expect) for consistency in test files
Prefer web-first assertions (toBeVisible,toHaveText, etc.) in Playwright tests
Useexpectmatchers for assertions (toEqual,toContain,toBeTruthy,toHaveLength, etc.) instead ofassertstatements in Playwright tests
Usepage.waitFor()with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts
Files:
apps/meteor/tests/e2e/image-upload.spec.ts
apps/meteor/tests/e2e/**/*.{ts,spec.ts}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests
Files:
apps/meteor/tests/e2e/image-upload.spec.ts
🧠 Learnings (19)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:20.672Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/server/ufs/ufs-local.spec.tsapps/meteor/tests/e2e/image-upload.spec.ts
📚 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/server/ufs/ufs-local.spec.ts
📚 Learning: 2026-01-15T22:03:20.672Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:20.672Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
.changeset/twelve-sheep-accept.mdapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.tsapps/meteor/app/api/server/lib/MultipartUploadHandler.tsapps/meteor/app/livechat/imports/server/rest/upload.ts
📚 Learning: 2025-09-15T13:10:30.049Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: packages/http-router/src/Router.ts:416-425
Timestamp: 2025-09-15T13:10:30.049Z
Learning: In packages/http-router/src/Router.ts, the dispatch() method's use of replaceAll('//', '/') on the full path is acceptable because URL normalization and query string handling is performed by the caller function before dispatch() is invoked.
Applied to files:
packages/http-router/src/Router.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
packages/http-router/src/Router.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
packages/http-router/src/Router.ts
📚 Learning: 2025-10-07T14:53:31.040Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36942
File: apps/meteor/client/lib/e2ee/binary.ts:12-25
Timestamp: 2025-10-07T14:53:31.040Z
Learning: In apps/meteor/client/lib/e2ee/binary.ts, the Binary.toArrayBuffer function uses charCodeAt() for Latin-1 encoding rather than TextEncoder for UTF-8 encoding. The team has decided to keep this approach for E2EE password/key material encoding.
Applied to files:
apps/meteor/app/file-upload/server/lib/FileUpload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/tests/e2e/image-upload.spec.ts
🧬 Code graph analysis (2)
apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
apps/meteor/app/api/server/lib/MultipartUploadHandler.ts (1)
MultipartUploadHandler(31-205)
apps/meteor/app/livechat/imports/server/rest/upload.ts (2)
apps/meteor/app/api/server/lib/MultipartUploadHandler.ts (1)
MultipartUploadHandler(31-205)apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
FileUpload(113-694)
⏰ 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)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (16)
apps/meteor/server/ufs/ufs-local.spec.ts (1)
102-103: LGTM!The updated assertion and comment correctly reflect the new temp-file cleanup behavior introduced by the streaming upload refactor. The two unlink calls (temp file + actual file via
removeById()) are expected behavior as confirmed in prior discussion..changeset/twelve-sheep-accept.md (1)
1-4: Verify that "patch" is the appropriate semver bump level.The PR introduces a new
MultipartUploadHandlerclass, updates_doInsertto accept file paths, and refactors the upload pipeline significantly. Depending on whether these changes affect public APIs or only internal implementation:
- If the public API remains compatible and this is an internal refactor,
patchis appropriate.- If new public classes/methods are exposed, consider
minor.- If existing public signatures changed in breaking ways, consider
major.Please confirm that the affected APIs are internal or that backwards compatibility is maintained for public consumers of these packages.
apps/meteor/app/api/server/middlewares/logger.ts (1)
13-21: LGTM!The conditional logic correctly avoids consuming the request body stream for multipart requests, which is essential for the new streaming upload pipeline. Using
includes()properly handles content-type headers with boundary parameters.apps/meteor/app/api/server/router.ts (1)
9-15: LGTM!The
HonoContexttype alias and theincomingfield inAPIActionContextare correctly aligned, enabling API handlers to access the rawIncomingMessagefor stream-based multipart parsing.Also applies to: 26-27
apps/meteor/server/ufs/ufs-filter.ts (1)
10-10: LGTM!The type widening from
ReadableStream | BuffertoBuffer | stringcorrectly supports the new file-path-based content handling, maintaining backward compatibility for existingBufferusage while enabling string paths for the streaming upload pipeline.Also applies to: 62-62, 140-140
apps/meteor/app/api/server/lib/MultipartUploadHandler.ts (3)
180-182: LGTM - filesLimit error handling fixed.The
filesLimitevent now correctly rejects with aMeteorErrorfor consistent error handling, addressing the previous review concern.
85-102: LGTM - Promise resolution logic is sound.The
filePendingCountapproach correctly handles both the "no file uploaded" case (rejecting witherror-no-fileunlessfileOptional) and ensures the promise doesn't hang when Busboy finishes before the write completes.
46-62: LGTM - stripExifFromFile implementation.The pipeline-based EXIF stripping with atomic rename and proper cleanup on error is well implemented.
apps/meteor/app/livechat/imports/server/rest/upload.ts (1)
65-68: The null check is unnecessary—fileStore.insert()throws on error rather than returning falsy.The
insertmethod always returns a Promise that either resolves with anIUploadobject or throws an exception (viaufsComplete). Temporary files are automatically cleaned up byufsCompleteon success (line 60) and on any failure (lines 51, 74). Remove theif (!uploadedFile)check and use try-catch if error handling is needed.apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts (1)
34-34: Good test harness alignment with multipart handler.Stubbing the new dependency keeps the proxyquire setup consistent with the updated upload pipeline.
apps/meteor/app/file-upload/server/lib/FileUpload.ts (6)
8-10: Import updates fit the new streaming flow.These additions align with the new temp-file and stream-based handling.
Also applies to: 34-34
139-194: Verify app event payload compatibility with path-based content.Line 139 and Line 194 now allow
contentto be a string path; please confirm app bridge typings and any IPreFileUpload consumers handle string paths (not only Buffer).
391-419: EXIF handling flow looks solid.The cached settings and size updates integrate cleanly with the new MultipartUploadHandler path.
805-830: Confirm stream buffering won’t reintroduce large memory spikes.Line 815–819 buffers the entire stream; please verify only bounded-size streams reach this path (or enforce a size cap) so large uploads don’t regress memory usage.
834-852: Check Node runtime support forfinished(..., { cleanup: true }).If your supported Node versions don’t include the
cleanupoption, considerpipeline(or manual teardown) to avoid runtime errors.
859-863: Validation-before-insert flow is clean.Nice consolidation with
_validateFilebefore_doInsert.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
In my testing, the images having wrong orientation are not being re-oriented as expected, even when disabling Message_Attachments_Strip_Exif setting.
This works on develop when Message_Attachments_Strip_Exif setting is disabled. It should work regardless of the setting value.
yash-rajpal
left a comment
yash-rajpal
left a comment
There was a problem hiding this comment.
After internal discussion, we decided to fix the image orientation behavior with different PR.
Proposed changes (including videos or screenshots)
Refactors the file upload pipeline to use streams instead of buffers, fixing memory issues that caused app crashes during large file uploads.
Key changes:
MultipartUploadHandlerclass centralizes stream-based multipart handling_doInsertnow accepts file paths, avoiding re-reading files into memory after they've been written to temp storageIssue(s)
Further comments
The previous implementation buffered entire files in memory at multiple stages (parsing, validation, processing), causing memory exhaustion on large uploads. This refactoring ensures files flow through streams to disk, significantly reducing memory footprint.
Benchmark and implementation discussions: open.rocket.chat
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.