test: add tests for image upload with rotation and EXIF#39252
Open
jessicaschelly wants to merge 5 commits intodevelopfrom
Open
test: add tests for image upload with rotation and EXIF#39252jessicaschelly wants to merge 5 commits intodevelopfrom
jessicaschelly wants to merge 5 commits intodevelopfrom
Conversation
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 |
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new end-to-end API test that verifies image upload rotation, EXIF stripping, and thumbnail generation using an EXIF-orientation image; toggles related settings, validates image metadata before/after upload, tests both enabled/disabled rotation states, and restores settings / cleans up. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
🧹 Nitpick comments (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts (1)
92-94: Restore per-test setting changes in afinallyblock.This test mutates a global setting and relies on suite teardown to restore it. Restoring inside the test keeps cases more isolated and safer for future test additions.
♻️ Suggested isolation improvement
it('should NOT rotate pixels when FileUpload_RotateImages is disabled', async () => { - await updateSetting('FileUpload_RotateImages', false); - - const requestConfig: IRequestConfig = { request, credentials: userCredentials }; - const { message } = await uploadFileToRC(testRoom._id, testImagePath, 'no-rotation-test', requestConfig); - const uploadMessage = message as IMessage; - - expect(uploadMessage).to.have.property('attachments'); - const attachment = uploadMessage.attachments?.find((item) => item.title === testImageName); - expect(attachment).to.be.an('object'); - - const fileUrl = (attachment as { title_link?: string }).title_link; - expect(fileUrl).to.be.a('string'); - - const originalBuffer = await downloadBuffer(fileUrl as string, userCredentials); - const originalMetadata = await sharp(originalBuffer).metadata(); - - expect(originalMetadata.width).to.equal(719); - expect(originalMetadata.height).to.equal(479); + await updateSetting('FileUpload_RotateImages', false); + try { + const requestConfig: IRequestConfig = { request, credentials: userCredentials }; + const { message } = await uploadFileToRC(testRoom._id, testImagePath, 'no-rotation-test', requestConfig); + const uploadMessage = message as IMessage; + + expect(uploadMessage).to.have.property('attachments'); + const attachment = uploadMessage.attachments?.find((item) => item.title === testImageName); + expect(attachment).to.be.an('object'); + + const fileUrl = (attachment as { title_link?: string }).title_link; + expect(fileUrl).to.be.a('string'); + + const originalBuffer = await downloadBuffer(fileUrl as string, userCredentials); + const originalMetadata = await sharp(originalBuffer).metadata(); + + expect(originalMetadata.width).to.equal(719); + expect(originalMetadata.height).to.equal(479); + } finally { + await updateSetting('FileUpload_RotateImages', true); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts` around lines 92 - 94, Save the current value of the FileUpload_RotateImages setting before mutating it, perform the test with updateSetting('FileUpload_RotateImages', false) inside a try block, and then restore the original value in a finally block using updateSetting(...) so the global setting is always reverted; reference the test "should NOT rotate pixels when FileUpload_RotateImages is disabled" and the updateSetting call to locate where to add const original = await getSetting('FileUpload_RotateImages') (or equivalent) and the try/finally wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts`:
- Around line 49-56: The after() teardown should be made resilient to partial
failures: replace the Promise.all([...]) with Promise.allSettled([...]) and only
call deleteRoom({ type: 'p', roomId: testRoom._id }) if testRoom is defined and
deleteUser(user) if user is defined; keep the updateSetting(...) calls
unconditional but include them in the settled array so a rejected cleanup does
not short-circuit other teardowns and errors are reported without masking the
original failure.
---
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts`:
- Around line 92-94: Save the current value of the FileUpload_RotateImages
setting before mutating it, perform the test with
updateSetting('FileUpload_RotateImages', false) inside a try block, and then
restore the original value in a finally block using updateSetting(...) so the
global setting is always reverted; reference the test "should NOT rotate pixels
when FileUpload_RotateImages is disabled" and the updateSetting call to locate
where to add const original = await getSetting('FileUpload_RotateImages') (or
equivalent) and the try/finally wrapper.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
apps/meteor/tests/mocks/files/exif-orientation-6.jpgis excluded by!**/*.jpg
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📜 Review details
⏰ 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 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/tests/end-to-end/api/file-upload-image-rotation.ts
🧠 Learnings (13)
📚 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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts (3)
packages/core-typings/src/ISetting.ts (1)
SettingValue(11-20)apps/meteor/tests/data/file.helper.ts (1)
uploadFileToRC(18-58)packages/core-typings/src/IMessage/MessageAttachment/Files/ImageAttachmentProps.ts (1)
ImageAttachmentProps(5-15)
🪛 Biome (2.4.4)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
[error] 35-47: Duplicate before hook found.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts (1)
59-90: Strong coverage for the rotation + EXIF + thumbnail path.The assertions validate source metadata, uploaded file transformation, EXIF removal, and thumbnail orientation in one flow. Nice end-to-end coverage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39252 +/- ##
===========================================
+ Coverage 70.73% 70.86% +0.12%
===========================================
Files 3195 3207 +12
Lines 113108 113353 +245
Branches 20460 20549 +89
===========================================
+ Hits 80009 80324 +315
+ Misses 31056 30979 -77
- Partials 2043 2050 +7
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts:50">
P2: Using Promise.allSettled here hides cleanup failures, which can leave settings/rooms/users in a bad state and cause cross-test contamination. Prefer failing the test when cleanup fails, or explicitly handle failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts (1)
49-56:⚠️ Potential issue | 🟠 MajorUse
Promise.allSettledin teardown to prevent cleanup errors from masking root failures.At Line 50,
Promise.allcan fail fast when one cleanup promise rejects, which makes teardown less resilient and can hide the primary failure signal.Promise.allSettledis safer for teardown hooks.💡 Proposed hardening
after(async () => { - await Promise.all([ - updateSetting('FileUpload_RotateImages', rotateImagesSetting), - updateSetting('Message_Attachments_Strip_Exif', stripExifSetting), - updateSetting('Message_Attachments_Thumbnails_Enabled', thumbnailsEnabledSetting), - testRoom ? deleteRoom({ type: 'p', roomId: testRoom._id }) : Promise.resolve(), - user ? deleteUser(user) : Promise.resolve(), - ]); + const cleanup: Promise<unknown>[] = [ + updateSetting('FileUpload_RotateImages', rotateImagesSetting), + updateSetting('Message_Attachments_Strip_Exif', stripExifSetting), + updateSetting('Message_Attachments_Thumbnails_Enabled', thumbnailsEnabledSetting), + ]; + + if (testRoom?._id) { + cleanup.push(deleteRoom({ type: 'p', roomId: testRoom._id })); + } + if (user) { + cleanup.push(deleteUser(user)); + } + + await Promise.allSettled(cleanup); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts` around lines 49 - 56, The teardown currently uses Promise.all which will reject-fast and can let a single cleanup error mask the original test failure; replace the Promise.all call in the after hook with Promise.allSettled so updateSetting, deleteRoom, and deleteUser calls (and the conditional testRoom/user branches referring to rotateImagesSetting, stripExifSetting, thumbnailsEnabledSetting, testRoom, and user) all run to completion and you still await their settled results, optionally logging or ignoring individual rejections rather than throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts`:
- Around line 49-56: The teardown currently uses Promise.all which will
reject-fast and can let a single cleanup error mask the original test failure;
replace the Promise.all call in the after hook with Promise.allSettled so
updateSetting, deleteRoom, and deleteUser calls (and the conditional
testRoom/user branches referring to rotateImagesSetting, stripExifSetting,
thumbnailsEnabledSetting, testRoom, and user) all run to completion and you
still await their settled results, optionally logging or ignoring individual
rejections rather than throwing.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📜 Review details
⏰ 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 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/tests/end-to-end/api/file-upload-image-rotation.ts
🧠 Learnings (17)
📚 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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-03-02T16:31:30.612Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39250
File: apps/meteor/tests/end-to-end/api/livechat/07-queue.ts:1084-1094
Timestamp: 2026-03-02T16:31:30.612Z
Learning: In E2E API tests at apps/meteor/tests/end-to-end/api/livechat/, using sleep(1000) after updateSetting() or updateEESetting() calls in test setup hooks is acceptable and intentional to allow omnichannel settings to propagate their side effects.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
🪛 Biome (2.4.4)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
[error] 35-47: Duplicate before hook found.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts (1)
59-111: Good coverage on rotation behavior and metadata expectations.The assertions around fixture orientation, post-upload dimensions, EXIF stripping, thumbnail orientation, and disabled-rotation behavior are solid and aligned with the PR objective.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts (1)
49-56:⚠️ Potential issue | 🟠 MajorUse non-short-circuit teardown aggregation.
The teardown still uses
Promise.all, so a single rejected cleanup promise fails the hook immediately and can mask subsequent cleanup outcomes. Switch toPromise.allSettledhere.Suggested fix
after(async () => { - await Promise.all([ + await Promise.allSettled([ updateSetting('FileUpload_RotateImages', rotateImagesSetting), updateSetting('Message_Attachments_Strip_Exif', stripExifSetting), updateSetting('Message_Attachments_Thumbnails_Enabled', thumbnailsEnabledSetting), testRoom ? deleteRoom({ type: 'p', roomId: testRoom._id }) : Promise.resolve(), user ? deleteUser(user) : Promise.resolve(), ]); });#!/bin/bash # Verify teardown uses non-short-circuit cleanup in this test file. rg -n -C3 'after\(async \(\) =>|Promise\.all\(|Promise\.allSettled\(' apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts` around lines 49 - 56, The teardown uses Promise.all which short-circuits on the first rejection; change the await Promise.all([...]) call in the after hook to await Promise.allSettled([...]) so all cleanup actions (the updateSetting calls and the conditional deleteRoom/deleteUser promises referencing testRoom and user and functions updateSetting, deleteRoom, deleteUser) run to completion and their results are collected without throwing on the first failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts`:
- Around line 49-56: The teardown uses Promise.all which short-circuits on the
first rejection; change the await Promise.all([...]) call in the after hook to
await Promise.allSettled([...]) so all cleanup actions (the updateSetting calls
and the conditional deleteRoom/deleteUser promises referencing testRoom and user
and functions updateSetting, deleteRoom, deleteUser) run to completion and their
results are collected without throwing on the first failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70963c8a-f31d-4eaf-b707-64608506fde2
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📜 Review details
⏰ 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 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/tests/end-to-end/api/file-upload-image-rotation.ts
🧠 Learnings (17)
📚 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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-03-02T16:31:41.304Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39250
File: apps/meteor/tests/end-to-end/api/livechat/07-queue.ts:1084-1094
Timestamp: 2026-03-02T16:31:41.304Z
Learning: In E2E API tests at apps/meteor/tests/end-to-end/api/livechat/, using sleep(1000) after updateSetting() or updateEESetting() calls in test setup hooks is acceptable and intentional to allow omnichannel settings to propagate their side effects.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
🪛 Biome (2.4.4)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
[error] 35-47: Duplicate before hook found.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (2)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts (2)
59-90: Solid end-to-end validation for rotated upload path.This test correctly validates fixture EXIF orientation, post-upload physical rotation, EXIF stripping, and thumbnail orientation from the processed file.
92-111: Good negative coverage for rotation-disabled behavior.The disabled-setting scenario is validated against original dimensions, which cleanly covers the expected non-rotation path.
jessicaschelly
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
jessicaschelly
removed
stat: ready to merge
| const thumbBuffer = await downloadBuffer(thumbUrl as string, userCredentials); | ||
| const thumbMetadata = await sharp(thumbBuffer).metadata(); | ||
|
|
||
| expect(thumbMetadata.width).to.be.lessThan(thumbMetadata.height as number); |
Contributor
There was a problem hiding this comment.
maybe here is more accurate if we assert that thumbMetadata.width and thumbMetadata.height are a specific number instead of 'height less than width'
Member
Author
There was a problem hiding this comment.
hmm, yeah, it makes sense.. I kept the orientation-based assertion because the test is more focused on rotation behavior, not thumbnail sizing config
if i assert exact thumbnail dimensions, the test gets tied to Message_Attachments_Thumbnails_Width/Height, so it could fail whenever those settings change even though rotation is still working correctly, but i can change the setting to a specific width/height, what do you think?
Contributor
There was a problem hiding this comment.
i'm with you then, ignore my comment. It'd be out of the scope of this task to verify whether the thumbnail sizing or config is correct, but if we don't have any test checking it I'd suggest creating a separate task for it
Contributor
There was a problem hiding this comment.
But as I see here: This has to be applied to both the original image being uploaded and the thumbnail generated in the server
Member
Author
There was a problem hiding this comment.
agreed! it mentions the orientation should be applied, so we have the assert for the orientation (height less than width = portrait), but not the sizing.. I could add here the size no problem, i was just worried about tying this test with another setting, wdyt?
Contributor
There was a problem hiding this comment.
as you are validating both width and height instead of just orientation above, I thought it could be more accurate to validate in the same way the thumbnail (perhaps adding Message_Attachments_Thumbnails_Width settings on the before), not 100% sure maybe someone can help us with a 3rd pov 👀
KevLehman
requested changes
Mar 4, 2026
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts:54">
P2: Teardown cleanup is now unsafe: unconditional `deleteRoom`/`deleteUser` can throw when setup fails before `user`/`testRoom` are initialized, making failures flaky and harder to diagnose.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts (1)
49-56:⚠️ Potential issue | 🟠 MajorHarden teardown to avoid masking the original failure.
On Line 50,
Promise.allshort-circuits on first rejection, and direct dereference on Line 54 (testRoom._id) can throw if setup partially failed. This can hide the real test failure cause.Suggested hardening
after(async () => { - await Promise.all([ - updateSetting('FileUpload_RotateImages', rotateImagesSetting), - updateSetting('Message_Attachments_Strip_Exif', stripExifSetting), - updateSetting('Message_Attachments_Thumbnails_Enabled', thumbnailsEnabledSetting), - deleteRoom({ type: 'p', roomId: testRoom._id }), - deleteUser(user), - ]); + const cleanup: Promise<unknown>[] = [ + updateSetting('FileUpload_RotateImages', rotateImagesSetting), + updateSetting('Message_Attachments_Strip_Exif', stripExifSetting), + updateSetting('Message_Attachments_Thumbnails_Enabled', thumbnailsEnabledSetting), + ]; + + if (testRoom?._id) { + cleanup.push(deleteRoom({ type: 'p', roomId: testRoom._id })); + } + if (user) { + cleanup.push(deleteUser(user)); + } + + await Promise.allSettled(cleanup); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts` around lines 49 - 56, The teardown uses Promise.all which short-circuits on the first rejection and directly dereferences testRoom._id, risking masking the original test failure; change the after hook to use Promise.allSettled so all cleanup tasks run even if one fails, and guard the room id dereference (e.g., use optional chaining or a conditional) when calling deleteRoom to avoid thrown errors if setup failed; ensure the cleanup calls referenced (updateSetting, deleteRoom, deleteUser) remain included so every task is attempted and handle/report individual failures instead of letting one rejection abort the rest.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts`:
- Around line 49-56: The teardown uses Promise.all which short-circuits on the
first rejection and directly dereferences testRoom._id, risking masking the
original test failure; change the after hook to use Promise.allSettled so all
cleanup tasks run even if one fails, and guard the room id dereference (e.g.,
use optional chaining or a conditional) when calling deleteRoom to avoid thrown
errors if setup failed; ensure the cleanup calls referenced (updateSetting,
deleteRoom, deleteUser) remain included so every task is attempted and
handle/report individual failures instead of letting one rejection abort the
rest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5db26f79-f93d-496c-b9ab-a06e25c40a7e
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📜 Review details
⏰ 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: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build (coverage)
🧰 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/tests/end-to-end/api/file-upload-image-rotation.ts
🧠 Learnings (19)
📚 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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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/end-to-end/api/file-upload-image-rotation.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-03-02T16:31:41.304Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39250
File: apps/meteor/tests/end-to-end/api/livechat/07-queue.ts:1084-1094
Timestamp: 2026-03-02T16:31:41.304Z
Learning: In E2E API tests at apps/meteor/tests/end-to-end/api/livechat/, using sleep(1000) after updateSetting() or updateEESetting() calls in test setup hooks is acceptable and intentional to allow omnichannel settings to propagate their side effects.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.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/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
🪛 Biome (2.4.4)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts
[error] 35-47: Duplicate before hook found.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (1)
apps/meteor/tests/end-to-end/api/file-upload-image-rotation.ts (1)
59-84: Coverage and assertions look solid for rotation/EXIF scenarios.These tests validate the intended behavior end-to-end: input EXIF orientation, rotated pixel dimensions when enabled, EXIF stripping, thumbnail orientation consistency, and no pixel rotation when disabled.
Also applies to: 86-104, 115-131
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Add tests for image rotation on file upload to verify the
FileUpload_RotateImagessetting works correctly.Test coverage:
Test fixture:
exif-orientation-6.jpg(18KB) - lightweight test image with EXIF orientation metadataIssue(s)
CORE-1563
QA-112
Steps to test or reproduce
Further comments
The test uses
sharpto verify image metadata before and after upload, ensuring the server correctly handles EXIF orientation data.Summary by CodeRabbit