fix(federation): uploads not working properly #37064
Conversation
|
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 |
|
WalkthroughAdds matrixRoomId propagation through media handling: updates event handler to pass data.room_id into media processing, extends MatrixMediaService methods to accept matrixRoomId and store it as federation.mrid, updates FederationMatrix to pass matrixRoomId for local files, and extends IUpload.federation with a new mrid field. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Matrix as Matrix Event
participant Msg as message() handler
participant MediaH as handleMediaMessage()
participant Svc as MatrixMediaService
participant Store as Upload Storage
Note over Matrix,Msg: Incoming Matrix message
Matrix->>Msg: event (data.room_id, content)
alt Remote media (mxc://)
Msg->>MediaH: handleMediaMessage(..., matrixRoomId=data.room_id)
MediaH->>Svc: downloadAndStoreRemoteFile(mxcUri, matrixRoomId, metadata)
Svc->>Store: save file (+federation.mrid)
Store-->>Svc: fileId
Svc-->>MediaH: fileId
else Local file to send
Msg->>Svc: prepareLocalFileForMatrix(fileId, serverName, matrixRoomId)
Svc->>Store: update upload (+federation.mrid)
Store-->>Svc: ok
Svc-->>Msg: prepared media info
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
rodrigok
force-pushed
the
chore/federation-backup-fix-upload
branch
from
699e5d8 to
a98d239
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (2)
49-56: Backfill mrid when MXC already existsIf a file already has
mxcUribut nomrid(existing data before this change), we return early and never setmrid, leaving federation metadata incomplete. Update it before returning.Apply:
- if (file.federation?.mxcUri) { - return file.federation.mxcUri; - } + if (file.federation?.mxcUri) { + if (!file.federation.mrid || file.federation.mrid !== matrixRoomId) { + await Uploads.setFederationInfo(fileId, { + mxcUri: file.federation.mxcUri, + mrid: matrixRoomId, + serverName: file.federation.serverName ?? serverName, + mediaId: file.federation.mediaId ?? fileId, + }); + } + return file.federation.mxcUri; + }
107-111: Backfill mrid for existing remote uploadsEarly return skips setting
mridfor files that already exist, causing missing room context on older records.Apply:
- if (uploadAlreadyExists) { - return uploadAlreadyExists._id; - } + if (uploadAlreadyExists) { + if (!uploadAlreadyExists.federation?.mrid || uploadAlreadyExists.federation.mrid !== matrixRoomId) { + await Uploads.setFederationInfo(uploadAlreadyExists._id, { + mxcUri, + mrid: matrixRoomId, + serverName: parts.serverName, + mediaId: parts.mediaId, + }); + } + return uploadAlreadyExists._id; + }
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/events/message.ts (1)
230-240: Use of data.room_id – optional consistency tweakYou could pass
room.federation.mridhere (already validated above) to keep a single source of truth, but current usage is fine.
📜 Review details
Configuration used: CodeRabbit 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 (4)
ee/packages/federation-matrix/src/FederationMatrix.ts(1 hunks)ee/packages/federation-matrix/src/events/message.ts(3 hunks)ee/packages/federation-matrix/src/services/MatrixMediaService.ts(4 hunks)packages/core-typings/src/IUpload.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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:
ee/packages/federation-matrix/src/services/MatrixMediaService.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/events/message.ts (1)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-151)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-151)
⏰ 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: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
ee/packages/federation-matrix/src/events/message.ts (2)
25-35: handleMediaMessage signature extended – OKAdding
matrixRoomIdkeeps room context through media ingestion.
46-52: Pass matrixRoomId to downloadAndStoreRemoteFile – OKCorrectly propagates room context into stored federation info.
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
431-431: All MatrixMediaService calls updated to new signature
No remaining old-arity calls to prepareLocalFileForMatrix or downloadAndStoreRemoteFile.packages/core-typings/src/IUpload.ts (1)
62-67: AllsetFederationInfocallers supplymrid– no breaking changes
Detected invocations inMatrixMediaService(both branches) and model methods all include the newmridfield, andRequired<IUpload>['federation']enforces it at compile time.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #37064 +/- ##
===========================================================
+ Coverage 69.76% 69.79% +0.02%
===========================================================
Files 3022 3022
Lines 103465 103465
Branches 18400 18398 -2
===========================================================
+ Hits 72186 72209 +23
+ Misses 29401 29378 -23
Partials 1878 1878
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Linked to RocketChat/homeserver#225
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit