Skip to content

Conversation

@pierre-lehnen-rc
Copy link
Contributor

@pierre-lehnen-rc pierre-lehnen-rc commented Jan 6, 2026

Proposed changes (including videos or screenshots)

Issue(s)

CORE-1602

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • Bug Fixes
    • Fixed an issue where deleting a message with multiple file attachments would not consistently remove all associated files; all attached files are now reliably deleted when a message is removed.

✏️ Tip: You can customize this high-level summary in your review settings.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Jan 6, 2026

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Jan 6, 2026

🦋 Changeset detected

Latest commit: 2311bee

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 40 packages
Name Type
@rocket.chat/models Patch
@rocket.chat/meteor Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch
@rocket.chat/ui-client Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/abac Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/media-calls Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/network-broker Patch
@rocket.chat/core-typings Patch
@rocket.chat/rest-typings Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/ddp-client Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/http-router Patch
@rocket.chat/livechat Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/ui-voip Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/license Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@pierre-lehnen-rc pierre-lehnen-rc marked this pull request as ready for review January 6, 2026 18:46
@pierre-lehnen-rc pierre-lehnen-rc requested a review from a team as a code owner January 6, 2026 18:46
@pierre-lehnen-rc pierre-lehnen-rc added this to the 8.1.0 milestone Jan 6, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 6, 2026

Walkthrough

Patch for @rocket.chat/models fixes a message file-deletion bug by updating getMessageByFileId to match file IDs stored either in a singular file field or inside a files array.

Changes

Cohort / File(s) Summary
Release metadata
\.changeset/smart-carpets-clean.md
New changeset declaring a patch release for @rocket.chat/models that documents a fix for incomplete deletion of files attached to messages.
Query logic fix
packages/models/src/models/Messages.ts
getMessageByFileId expanded to match file IDs in either file._id or files._id, enabling correct detection when attachments are stored as a single file or an array files.

Sequence Diagram(s)

(omitted — change is a localized model query update and does not introduce multi-component sequential flow)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

Suggested reviewers

  • KevLehman
  • tassoevan
  • ricardogarim

Poem

🐰 I nudge through fields both one and many,
Sniffing out files so none are left uncanny.
From file to files I hop with delight,
Tidying messages through day and night. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly and specifically describes the main fix: addressing the issue where deleting a message with multiple files doesn't delete every file.
Linked Issues check ✅ Passed The code change directly addresses CORE-1602 by modifying the getMessageByFileId function to match files in both singular file field and files array, enabling proper file deletion.
Out of Scope Changes check ✅ Passed All changes are scoped to fixing the file deletion issue: a changeset entry documenting the fix and modification to the Messages.ts query logic to handle multiple file references.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/delete-message-multi-files

📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 8f04e4a and e59b315.

📒 Files selected for processing (1)
  • packages/models/src/models/Messages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/models/src/models/Messages.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). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 2 files

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/models/src/models/Messages.ts (2)

56-56: Add index for files._id to match file._id index.

Since the query now checks both file._id and files._id (line 1518), you should add a corresponding sparse index for files._id to ensure good query performance:

🔎 Proposed index addition
 { key: { 'file._id': 1 }, sparse: true },
+{ key: { 'files._id': 1 }, sparse: true },
 { key: { 'mentions.username': 1 }, sparse: true },

1522-1526: Apply the same fix to getMessageByFileIdAndUsername.

This method only checks file._id, which means it won't find messages with multiple files stored in the files array. For consistency with the fix in getMessageByFileId, this method should also check both fields.

🔎 Proposed fix
 getMessageByFileIdAndUsername(fileID: string, userId: string): Promise<IMessage | null> {
 	const query = {
-		'file._id': fileID,
+		$or: [{ 'file._id': fileID }, { 'files._id': fileID }],
 		'u._id': userId,
 	};
🤖 Fix all issues with AI Agents
In @packages/models/src/models/Messages.ts:
- Around line 1517-1519: Add a Mongo index for the array path by defining an
index on 'files._id' (e.g., schema.index({ 'files._id': 1 })) to match the
existing 'file._id' index, and update the query logic in
getMessageByFileIdAndUsername and findFilesByUserId to mirror getMessageByFileId
by using a $or that checks both 'file._id' and 'files._id' so messages with
either a single file or files array are matched.
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between d3511ea and 8f04e4a.

📒 Files selected for processing (2)
  • .changeset/smart-carpets-clean.md
  • packages/models/src/models/Messages.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation

Files:

  • packages/models/src/models/Messages.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). (3)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build
🔇 Additional comments (1)
.changeset/smart-carpets-clean.md (1)

1-5: LGTM!

The changeset accurately documents the patch and provides a clear description of the fix.

@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.69%. Comparing base (7fd6b3e) to head (2311bee).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #38079      +/-   ##
===========================================
+ Coverage    70.68%   70.69%   +0.01%     
===========================================
  Files         3148     3148              
  Lines       108850   108850              
  Branches     19547    19565      +18     
===========================================
+ Hits         76937    76948      +11     
+ Misses       29916    29899      -17     
- Partials      1997     2003       +6     
Flag Coverage Δ
e2e 60.18% <ø> (-0.06%) ⬇️
e2e-api 48.41% <ø> (-0.04%) ⬇️
unit 71.81% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 6, 2026

📦 Docker Image Size Report

➡️ Changes

Service Current Baseline Change Percent
sum of all images 0B 0B 0B
account-service 0B 0B 0B
authorization-service 0B 0B 0B
ddp-streamer-service 0B 0B 0B
omnichannel-transcript-service 0B 0B 0B
presence-service 0B 0B 0B
queue-worker-service 0B 0B 0B
rocketchat 0B 0B 0B

📊 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/12 15:26 (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]
Loading

Statistics (last 30 days):

  • 📊 Average: 1.5GiB
  • ⬇️ Minimum: 1.4GiB
  • ⬆️ Maximum: 1.6GiB
  • 🎯 Current PR: 0B
ℹ️ About this report

This report compares Docker image sizes from this build against the develop baseline.

  • Tag: pr-38079
  • Baseline: develop
  • Timestamp: 2026-01-12 15:26:37 UTC
  • Historical data points: 30

Updated: Mon, 12 Jan 2026 15:26:38 GMT

@pierre-lehnen-rc pierre-lehnen-rc added the stat: QA assured Means it has been tested and approved by a company insider label Jan 9, 2026
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jan 9, 2026
@pierre-lehnen-rc pierre-lehnen-rc removed the stat: QA assured Means it has been tested and approved by a company insider label Jan 9, 2026
@dougfabris dougfabris removed the stat: ready to merge PR tested and approved waiting for merge label Jan 9, 2026
@pierre-lehnen-rc pierre-lehnen-rc added the stat: QA assured Means it has been tested and approved by a company insider label Jan 12, 2026
@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label Jan 12, 2026
@kodiakhq kodiakhq bot merged commit 020dfbc into develop Jan 12, 2026
44 checks passed
@kodiakhq kodiakhq bot deleted the fix/delete-message-multi-files branch January 12, 2026 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants