fix: Image orientation issues #38082
Open
+46
−12
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: 4de70cd The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughMoves EXIF-stripping out of the rooms.media route and into the FileUpload validation flow, making EXIF removal conditional on the Message_Attachments_Strip_Exif setting; adds ExifTransformer usage and temporary-file handling during stripping; updates a changeset documenting the patch. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FileUploadService
participant ExifTransformer
participant TempFS
participant Storage
Client->>FileUploadService: Upload file (validate phase)
FileUploadService->>FileUploadService: check Message_Attachments_Strip_Exif
alt strip EXIF enabled
FileUploadService->>TempFS: read tmpFile into stream
TempFS->>ExifTransformer: pipe stream to ExifTransformer
ExifTransformer->>TempFS: write stripped output to temp path
TempFS->>FileUploadService: rename temp -> original tmpFile
end
FileUploadService->>Storage: proceed to store file / compute size
FileUploadService->>Client: return upload result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @apps/meteor/app/file-upload/server/lib/FileUpload.ts:
- Around line 408-414: The EXIF stripping block uses blocking
fs.readFileSync/fs.writeFileSync and lacks error handling; replace these with
async fs/promises methods and wrap the operation in try/catch. Specifically,
inside the branch that checks settings.get('Message_Attachments_Strip_Exif') and
the code that calls Media.stripExifFromBuffer(tmpBuffer), use await
fsPromises.readFile(tmpFile) and await fsPromises.writeFile(tmpFile,
strippedFileBuffer) (or the already imported async fs binding used elsewhere),
and catch errors from readFile, Media.stripExifFromBuffer, and writeFile to
log/handle them and prevent crashing the upload; also replace the later
fs.lstatSync call with await fsPromises.lstat(tmpFile) for consistency.
📜 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 (3)
.changeset/dry-pumpkins-design.mdapps/meteor/app/api/server/v1/rooms.tsapps/meteor/app/file-upload/server/lib/FileUpload.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:
apps/meteor/app/file-upload/server/lib/FileUpload.tsapps/meteor/app/api/server/v1/rooms.ts
🧠 Learnings (5)
📚 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-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/app/api/server/v1/rooms.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/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/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 (1)
apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
packages/core-services/src/index.ts (1)
Media(163-163)
⏰ 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 (3)
apps/meteor/app/api/server/v1/rooms.ts (2)
1-1: LGTM - Import cleanup aligns with centralized EXIF handling.Removing the Media import is correct since EXIF stripping for attachments has been relocated to FileUpload.ts. This simplifies the dependency footprint in the rooms API module.
211-211: LGTM - Correct use of const for immutable reference.Changing to const is appropriate since
fileBufferis not reassigned within the function scope (it's only read at lines 231 and 240).apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
13-13: Media.stripExifFromBuffer method exists with correct signature and is properly used.The method is correctly defined in
IMediaServiceasstripExifFromBuffer(buffer: Buffer): Promise<Buffer>, implemented in the image service, and properly imported and awaited at line 412 in FileUpload.ts. No issues to address.
KevLehman
previously approved these changes
Jan 6, 2026
Member
KevLehman
left a comment
KevLehman
left a comment
There was a problem hiding this comment.
(would love to have a way to test this, but maybe we have no automated way)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38082 +/- ##
===========================================
- Coverage 70.66% 70.65% -0.01%
===========================================
Files 3145 3147 +2
Lines 108772 108921 +149
Branches 19577 19612 +35
===========================================
+ Hits 76862 76961 +99
- Misses 29907 29965 +58
+ Partials 2003 1995 -8
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/15 12:12 (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: Thu, 15 Jan 2026 12:12:53 GMT |
d-gubert
requested changes
Jan 8, 2026
Comment on lines
411
to
414
| const fileBuffer = fs.readFileSync(tmpFile); | ||
| const strippedFileBuffer = await Media.stripExifFromBuffer(fileBuffer); | ||
| fs.writeFileSync(tmpFile, strippedFileBuffer); | ||
| } |
Member
There was a problem hiding this comment.
Please do not use the sync versions of the FS methods, especially when writing a potentially large amount of data, as it blocks the event loop.
Also, here the server will end up buffering the file contents 2 additional times: one reading the file, and another one with the MediaService call. You could unify this by calling Media.stripExifFromStream, passing a read stream from fs.createReadStream - this would prevent any buffering from happening
Member
There was a problem hiding this comment.
Aside from that, sharp already removes exif metadata when rotating the image, so we don't need to call the media service in those cases
Member
|
PR #38071 is working to reduce memory usage on file uploads, it looks like it might conflict with this implementation |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/meteor/app/file-upload/server/lib/FileUpload.ts`:
- Around line 399-401: Remove the leftover debug console.log in FileUpload.ts:
locate the line "console.log('rotating image', file._id)" right after the
s.rotate().withMetadata().toFile(...) call and delete it or replace it with a
structured SystemLogger.debug call (e.g., include context like file._id) if this
event should be observable in logs; ensure no raw console.* calls remain in the
FileUpload class/method.
- Line 872: Remove the debug console.trace('insert') call in FileUpload.ts (the
stray statement in the FileUpload insert flow) to stop printing stack traces on
every insert; replace it with a proper logging call if needed (e.g., use the
existing processLogger or server-side logger at debug/trace level) or simply
delete the line from the insert handler/function to avoid polluting server logs.
- Around line 410-441: Replace the debug console.log and cleanup commented-out
snippets, add proper transformer error handling, and guard unlink so it won't
throw if the temp file was never created: specifically, in the EXIF strip block
that uses tmpFile, exifTmpPath, ExifTransformer, readStream and writeStream,
remove the commented-out pipeline code, replace console.log('ERROR', err) with
SystemLogger.error(`Error stripping exif from image: ${err}`) (or extend the
existing SystemLogger call), attach an 'error' listener to exifTransformer that
rejects the Promise like the other streams, and wrap the await
unlink(exifTmpPath) in a safe check or try/catch (or check fs.exists before
unlink) so unlink won't throw when exifTmpPath was never created.
♻️ Duplicate comments (1)
apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
443-443: Fix incorrectawaiton synchronouslstatSync.
fs.lstatSyncis synchronous—awaithas no effect here. Use the async version for consistency with the rest of the file.- const { size } = await fs.lstatSync(tmpFile); + const { size } = await fs.promises.lstat(tmpFile);
🧹 Nitpick comments (1)
apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
8-8: Remove unused import.The
pipelineimport is not used anywhere in the file—the code at line 433 that would have used it is commented out.-import { pipeline } from 'stream/promises';
📜 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 (1)
apps/meteor/app/file-upload/server/lib/FileUpload.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:
apps/meteor/app/file-upload/server/lib/FileUpload.ts
⏰ 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
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+399
to
+401
| // By default, the metadata is not preserved when rotating the image. | ||
| await s.rotate().withMetadata().toFile(`${tmpFile}.tmp`); | ||
| console.log('rotating image', file._id); |
Contributor
There was a problem hiding this comment.
Remove debug console.log statement.
Debug logging left in production code. Either remove it or use SystemLogger.debug() if this logging is intentional for observability.
// By default, the metadata is not preserved when rotating the image.
await s.rotate().withMetadata().toFile(`${tmpFile}.tmp`);
- console.log('rotating image', file._id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // By default, the metadata is not preserved when rotating the image. | |
| await s.rotate().withMetadata().toFile(`${tmpFile}.tmp`); | |
| console.log('rotating image', file._id); | |
| // By default, the metadata is not preserved when rotating the image. | |
| await s.rotate().withMetadata().toFile(`${tmpFile}.tmp`); |
🤖 Prompt for AI Agents
In `@apps/meteor/app/file-upload/server/lib/FileUpload.ts` around lines 399 - 401,
Remove the leftover debug console.log in FileUpload.ts: locate the line
"console.log('rotating image', file._id)" right after the
s.rotate().withMetadata().toFile(...) call and delete it or replace it with a
structured SystemLogger.debug call (e.g., include context like file._id) if this
event should be observable in logs; ensure no raw console.* calls remain in the
FileUpload class/method.
Comment on lines
+410
to
+441
| const stripExif = settings.get('Message_Attachments_Strip_Exif'); | ||
|
|
||
| if (stripExif) { | ||
| const exifTmpPath = `${tmpFile}.exif-stripped`; | ||
|
|
||
| try { | ||
| await new Promise<void>((resolve, reject) => { | ||
| const readStream = fs.createReadStream(tmpFile); | ||
| const writeStream = fs.createWriteStream(exifTmpPath); | ||
| const exifTransformer = new ExifTransformer(); | ||
|
|
||
| readStream.pipe(exifTransformer).pipe(writeStream); | ||
| writeStream.on('finish', () => resolve()); | ||
| readStream.on('error', reject); | ||
| writeStream.on('error', reject); | ||
| }); | ||
| // No need to check mime. Library will ignore any files without exif/xmp tags (like BMP, ico, PDF, etc) | ||
| // const exifTransformer = new ExifTransformer(); | ||
| // const readStream = fs.createReadStream(tmpFile); | ||
| // const writeStream = fs.createWriteStream(exifTmpPath); | ||
|
|
||
| // readStream.pipe(exifTransformer).pipe(writeStream); | ||
|
|
||
| // await pipeline(fs.createReadStream(tmpFile), exifTransformer, fs.createWriteStream(exifTmpPath)); | ||
|
|
||
| await rename(exifTmpPath, tmpFile); | ||
| } catch (err) { | ||
| console.log('ERROR', err); | ||
| await unlink(exifTmpPath); | ||
| SystemLogger.error(`Error stripping exif from image: ${err}`); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Clean up debug artifacts and add missing error handling.
Several issues in the EXIF stripping logic:
- Line 437: Replace
console.log('ERROR', err)with proper logging—it's redundant with line 439 anyway. - Lines 426-433: Remove commented-out code.
- Line 419: Missing error handler for
exifTransformerstream—if the transformer emits an error, the promise won't reject. - Line 438:
unlink(exifTmpPath)may throw if the file was never created (e.g., error occurred duringcreateReadStream).
🛠️ Proposed fix
if (stripExif) {
const exifTmpPath = `${tmpFile}.exif-stripped`;
try {
await new Promise<void>((resolve, reject) => {
const readStream = fs.createReadStream(tmpFile);
const writeStream = fs.createWriteStream(exifTmpPath);
const exifTransformer = new ExifTransformer();
readStream.pipe(exifTransformer).pipe(writeStream);
writeStream.on('finish', () => resolve());
readStream.on('error', reject);
+ exifTransformer.on('error', reject);
writeStream.on('error', reject);
});
- // No need to check mime. Library will ignore any files without exif/xmp tags (like BMP, ico, PDF, etc)
- // const exifTransformer = new ExifTransformer();
- // const readStream = fs.createReadStream(tmpFile);
- // const writeStream = fs.createWriteStream(exifTmpPath);
-
- // readStream.pipe(exifTransformer).pipe(writeStream);
-
- // await pipeline(fs.createReadStream(tmpFile), exifTransformer, fs.createWriteStream(exifTmpPath));
await rename(exifTmpPath, tmpFile);
} catch (err) {
- console.log('ERROR', err);
- await unlink(exifTmpPath);
+ await unlink(exifTmpPath).catch(() => {});
SystemLogger.error(`Error stripping exif from image: ${err}`);
}
}🤖 Prompt for AI Agents
In `@apps/meteor/app/file-upload/server/lib/FileUpload.ts` around lines 410 - 441,
Replace the debug console.log and cleanup commented-out snippets, add proper
transformer error handling, and guard unlink so it won't throw if the temp file
was never created: specifically, in the EXIF strip block that uses tmpFile,
exifTmpPath, ExifTransformer, readStream and writeStream, remove the
commented-out pipeline code, replace console.log('ERROR', err) with
SystemLogger.error(`Error stripping exif from image: ${err}`) (or extend the
existing SystemLogger call), attach an 'error' listener to exifTransformer that
rejects the Promise like the other streams, and wrap the await
unlink(exifTmpPath) in a safe check or try/catch (or check fs.exists before
unlink) so unlink won't throw when exifTmpPath was never created.
| await filter.check(fileData, streamOrBuffer); | ||
| } | ||
|
|
||
| console.trace('insert'); |
Contributor
There was a problem hiding this comment.
Remove debug console.trace statement.
This debug statement will print a stack trace to the console on every file insert operation, polluting server logs.
- console.trace('insert');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.trace('insert'); |
🤖 Prompt for AI Agents
In `@apps/meteor/app/file-upload/server/lib/FileUpload.ts` at line 872, Remove the
debug console.trace('insert') call in FileUpload.ts (the stray statement in the
FileUpload insert flow) to stop printing stack traces on every insert; replace
it with a proper logging call if needed (e.g., use the existing processLogger or
server-side logger at debug/trace level) or simply delete the line from the
insert handler/function to avoid polluting server logs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 1 file (changes from recent commits).
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/file-upload/server/lib/FileUpload.ts">
<violation number="1" location="apps/meteor/app/file-upload/server/lib/FileUpload.ts:401">
P2: Debug `console.log` statement should be removed before merging to production. Use `SystemLogger` if logging is needed.</violation>
<violation number="2" location="apps/meteor/app/file-upload/server/lib/FileUpload.ts:419">
P2: Missing error handler for `exifTransformer` stream. If the transformer encounters an error, it won't be caught. Add `exifTransformer.on('error', reject);` to properly handle transformer errors.</violation>
<violation number="3" location="apps/meteor/app/file-upload/server/lib/FileUpload.ts:872">
P2: Debug `console.trace` statement should be removed before merging to production.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| await filter.check(fileData, streamOrBuffer); | ||
| } | ||
|
|
||
| console.trace('insert'); |
Contributor
There was a problem hiding this comment.
P2: Debug console.trace statement should be removed before merging to production.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/file-upload/server/lib/FileUpload.ts, line 872:
<comment>Debug `console.trace` statement should be removed before merging to production.</comment>
<file context>
@@ -842,7 +869,7 @@ export class FileUploadClass {
await filter.check(fileData, streamOrBuffer);
}
-
+ console.trace('insert');
return this._doInsert(fileData, streamOrBuffer, { session: options?.session });
}
</file context>
|
|
||
| // By default, the metadata is not preserved when rotating the image. | ||
| await s.rotate().withMetadata().toFile(`${tmpFile}.tmp`); | ||
| console.log('rotating image', file._id); |
Contributor
There was a problem hiding this comment.
P2: Debug console.log statement should be removed before merging to production. Use SystemLogger if logging is needed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/file-upload/server/lib/FileUpload.ts, line 401:
<comment>Debug `console.log` statement should be removed before merging to production. Use `SystemLogger` if logging is needed.</comment>
<file context>
@@ -395,8 +396,9 @@ export const FileUpload = {
-
+ // By default, the metadata is not preserved when rotating the image.
+ await s.rotate().withMetadata().toFile(`${tmpFile}.tmp`);
+ console.log('rotating image', file._id);
await unlink(tmpFile);
</file context>
Comment on lines
+419
to
+424
| const exifTransformer = new ExifTransformer(); | ||
|
|
||
| readStream.pipe(exifTransformer).pipe(writeStream); | ||
| writeStream.on('finish', () => resolve()); | ||
| readStream.on('error', reject); | ||
| writeStream.on('error', reject); |
Contributor
There was a problem hiding this comment.
P2: Missing error handler for exifTransformer stream. If the transformer encounters an error, it won't be caught. Add exifTransformer.on('error', reject); to properly handle transformer errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/file-upload/server/lib/FileUpload.ts, line 419:
<comment>Missing error handler for `exifTransformer` stream. If the transformer encounters an error, it won't be caught. Add `exifTransformer.on('error', reject);` to properly handle transformer errors.</comment>
<file context>
@@ -408,9 +410,34 @@ export const FileUpload = {
+ await new Promise<void>((resolve, reject) => {
+ const readStream = fs.createReadStream(tmpFile);
+ const writeStream = fs.createWriteStream(exifTmpPath);
+ const exifTransformer = new ExifTransformer();
+
+ readStream.pipe(exifTransformer).pipe(writeStream);
</file context>
Suggested change
| const exifTransformer = new ExifTransformer(); | |
| readStream.pipe(exifTransformer).pipe(writeStream); | |
| writeStream.on('finish', () => resolve()); | |
| readStream.on('error', reject); | |
| writeStream.on('error', reject); | |
| const exifTransformer = new ExifTransformer(); | |
| readStream.pipe(exifTransformer).pipe(writeStream); | |
| writeStream.on('finish', () => resolve()); | |
| readStream.on('error', reject); | |
| writeStream.on('error', reject); | |
| exifTransformer.on('error', reject); |
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)
Some issues were encountered with image orientation when
Message_Attachments_Strip_Exifis enabled. This was because, exif metadata was being removed before re-orienting the image. This PR fixes this by changing the flow of processes. Now it first re-orients the image and later strips away the exif metadata.Issue(s)
Message_Attachments_Strip_ExifSteps to test or reproduce
Further comments
CORE-1563
Only downside I see to this approach is not being much memory efficient, because we need to convert the image files to buffer and then we also need to maintain a second buffer of similar size to keep the stripped buffer.
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.