fix(federation): delete message #37112
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 |
|
|
Caution Review failedThe pull request is closed. WalkthroughRemoves the uid/user dependency from federated message deletion, adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant S as App Server
participant H as Federation Hook
participant F as FederationMatrix
participant M as Matrix HS
U->>S: Delete message request
S->>S: Load room with { lastMessage, prid, mid, federated, federation }
S->>H: afterDeleteMessage(room, message)
H->>F: deleteMessage(room.federation.mrid, message)
F->>M: redactMessage(matrixRoomId, matrixEventId)
M-->>F: redaction result
F-->>H: done
H-->>S: done
S-->>U: acknowledge deletion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
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 |
ricardogarim
force-pushed
the
fix/federation-delete-message
branch
from
b616983 to
edb8dd8
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: 1
📜 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)
apps/meteor/app/lib/server/functions/deleteMessage.ts(1 hunks)apps/meteor/ee/server/hooks/federation/index.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)packages/core-services/src/types/IFederationMatrixService.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:
apps/meteor/ee/server/hooks/federation/index.ts
🧬 Code graph analysis (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
packages/core-services/src/types/IFederationMatrixService.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)
apps/meteor/ee/server/hooks/federation/index.ts (2)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-239)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(53-994)
🪛 GitHub Actions: CI
ee/packages/federation-matrix/src/FederationMatrix.ts
[error] 637-637: src/FederationMatrix.ts(637,58): TS2554: Expected 3 arguments, but got 2.
🔇 Additional comments (6)
apps/meteor/app/lib/server/functions/deleteMessage.ts (1)
38-38: LGTM! Projection correctly includes federation data.The addition of
federation: 1to the projection is necessary to support the updated federation deletion flow, whereroom.federation.mridis accessed in theafterDeleteMessagecallback.apps/meteor/ee/server/hooks/federation/index.ts (2)
2-2: LGTM! Removed unused import.The
isUserNativeFederatedimport is no longer needed after removing user-based logic from the deletion flow.
60-67: Remove redundant guard for room.federation
shouldPerformFederationActionis a type guard narrowingroomtoIRoomNativeFederated(whosefederation.mridis always defined).Likely an incorrect or invalid review comment.
packages/core-services/src/types/IFederationMatrixService.ts (1)
13-13: LGTM! Interface signature correctly simplified.The removal of the
uidparameter fromdeleteMessagealigns with the PR objective to eliminate user dependency from federated message deletion.ee/packages/federation-matrix/src/FederationMatrix.ts (2)
619-619: LGTM! Method signature correctly updated.The
deleteMessagemethod signature now matches the updatedIFederationMatrixServiceinterface by removing theuidparameter.
636-636: Address TODO comment about message author.The TODO at line 636 notes that
message.u?.usernameis not the user who removed the message. While theuidparameter has been removed, consider whether the redaction should track who performed the deletion for audit purposes.Verify if the Matrix federation protocol requires tracking who performed the redaction, or if anonymous redaction is acceptable for your use case.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/apps/federation-service/package.json (1)
28-28: Confirm all workspace references bump to 0.1.13.This package now pulls
@rocket.chat/federation-sdk@0.1.13. Please make sure every other workspace package (and the lockfile) using this SDK is bumped to the same version so we avoid a mixed workspace state.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
ee/apps/federation-service/package.json(1 hunks)ee/packages/federation-matrix/package.json(1 hunks)
⏰ 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: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/packages/federation-matrix/package.json (1)
41-41: Looks good—dependency bump aligns with the service.This keeps the matrix package in sync with the service after the API signature change.
ggazzo
force-pushed
the
fix/federation-delete-message
branch
from
939e948 to
66f8103
Compare
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
https://rocketchat.atlassian.net/browse/FDR-186
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Improvements
Chores