regression(federation): fix mixed DM invite not working #37924
Conversation
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant RC as Rocket.Chat Server
participant Matrix as Matrix Homeserver (Synapse)
participant User as Remote User (Matrix or non-native)
Note over RC,Matrix: DM creation & invite flow (changed)
RC->>Matrix: createRoom / invite (for each member)
alt member is native federated
RC->>Matrix: invite using parsed federated userId
else non-native user
RC->>Matrix: construct `@username:serverName` and invite
end
Matrix-->>User: delivery of invite
User-->>Matrix: accept invite
Matrix-->>RC: notify accept (removed subsequent performAcceptRoomInvite call)
RC->>RC: updateDirectMessageRoomName(room, ignoreStatusFromSubs?)
Note right of RC: skip renaming for subs in `ignoreStatusFromSubs` when status is INVITED
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #37924 +/- ##
=================================================
+ Coverage 71.73% 71.74% +0.01%
=================================================
Files 1453 1453
Lines 76689 76689
Branches 11133 11117 -16
=================================================
+ Hits 55013 55022 +9
+ Misses 21249 21237 -12
- Partials 427 430 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 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/15 22:28", "11/16 01:28", "11/17 23:50", "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/22 23:53 (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]
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]
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]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 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]
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]
line "queue-worker-service" [0.14, 0.14, 0.14, 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]
line "rocketchat" [0.36, 0.36, 0.35, 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.35]
Statistics (last 27 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 22 Dec 2025 23:53:43 GMT |
sampaiodiego
force-pushed
the
regression-federation-mixed-dm-invite-not-working
branch
2 times, most recently
from
cae2a6d to
f980dc3
Compare
sampaiodiego
force-pushed
the
regression-federation-mixed-dm-invite-not-working
branch
from
f980dc3 to
e7d880e
Compare
ggazzo
added
the
stat: QA assured
![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
🧹 Nitpick comments (1)
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts (1)
1207-1262: Test setup correctly establishes DM priority scenario.The restructured test setup creates the federated DM first (making it older), then the non-federated DM. This correctly tests the DM priority logic where non-federated rooms are preferred over federated ones when finding duplicate DMs with the same users, as implemented in the sort order
{ federated: 1, ts: 1 }in Rooms.ts.Minor suggestion: The comment on line 1249 "which should be returned on duplication" could be clarified to "which should be returned on duplication due to being non-federated" to make the priority logic more explicit.
📜 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 (2)
ee/packages/federation-matrix/tests/end-to-end/dms.spec.tspackages/models/src/models/Rooms.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.tspackages/models/src/models/Rooms.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
🧠 Learnings (10)
📓 Common learnings
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.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
📚 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:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.tspackages/models/src/models/Rooms.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:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.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:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.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:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.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:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.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:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.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: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
packages/models/src/models/Rooms.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: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
packages/models/src/models/Rooms.ts
🧬 Code graph analysis (1)
packages/models/src/models/Rooms.ts (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(20-94)packages/apps-engine/src/definition/rooms/IRoom.ts (1)
IRoom(4-28)packages/apps/src/AppsEngine.ts (1)
IRoom(16-16)
⏰ 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: CodeQL-Build
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts (4)
1100-1103: Test activation aligns with regression fix.The test "should accept the invitation by the Rocket.Chat user" is now enabled after being marked as failing. This correctly reflects that the mixed DM invite acceptance issue has been fixed.
1488-1497: Retry logic improves test reliability.The added retry block properly waits for the asynchronous join processing to complete by verifying the subscription no longer has an INVITED status. This handles eventual consistency correctly and aligns with the pattern used elsewhere in the test suite.
1516-1540: Test restructuring improves clarity and adds cleanup verification.The separation of the leave flow into its own test case improves readability and test isolation. The added verification (lines 1528-1539) properly confirms that the room is removed from the user's active rooms list after leaving, using retry logic to handle asynchronous cleanup operations.
1542-1559: Test rename accurately reflects the scenario.The renamed test "should have two DMs with same users" clearly describes the state after rcUser2 leaves the 3-person federated DM - there are now two federated DMs with the same two users (rcUser1 and hs1Admin). The test logic correctly verifies this scenario.
packages/models/src/models/Rooms.ts (1)
900-906: No action needed. The hardcoded sort{ federated: 1, ts: 1 }is safe—all existing callers pass onlyprojectionoptions or no options at all, so the sort override won't break any current code. Adding explanatory comments contradicts the coding guidelines provided (avoid code comments in TypeScript/JavaScript implementation).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
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="ee/packages/federation-matrix/src/FederationMatrix.ts">
<violation number="1" location="ee/packages/federation-matrix/src/FederationMatrix.ts:296">
P2: Inconsistent user ID validation: constructed user ID for non-native federated users should be validated with `userIdSchema.parse()` for consistency with `inviteUsersToRoom` (line 553) and to ensure proper type safety.</violation>
</file>
<file name="ee/packages/federation-matrix/tests/end-to-end/room.spec.ts">
<violation number="1" location="ee/packages/federation-matrix/tests/end-to-end/room.spec.ts:1616">
P1: Missing `await` before `retry()` call. The `retry` function is async and returns a Promise, so without `await`, the test will complete immediately without running the assertions, causing false positive results.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| try { | ||
| await federationSDK.inviteUserToRoom( | ||
| userIdSchema.parse(member.username), | ||
| isUserNativeFederated(member) ? userIdSchema.parse(member.username) : `@${member.username}:${this.serverName}`, |
There was a problem hiding this comment.
P2: Inconsistent user ID validation: constructed user ID for non-native federated users should be validated with userIdSchema.parse() for consistency with inviteUsersToRoom (line 553) and to ensure proper type safety.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At ee/packages/federation-matrix/src/FederationMatrix.ts, line 296:
<comment>Inconsistent user ID validation: constructed user ID for non-native federated users should be validated with `userIdSchema.parse()` for consistency with `inviteUsersToRoom` (line 553) and to ensure proper type safety.</comment>
<file context>
@@ -291,13 +291,9 @@ export class FederationMatrix extends ServiceClass implements IFederationMatrixS
try {
await federationSDK.inviteUserToRoom(
- userIdSchema.parse(member.username),
+ isUserNativeFederated(member) ? userIdSchema.parse(member.username) : `@${member.username}:${this.serverName}`,
roomIdSchema.parse(matrixRoomResult.room_id),
userIdSchema.parse(actualMatrixUserId),
</file context>
| isUserNativeFederated(member) ? userIdSchema.parse(member.username) : `@${member.username}:${this.serverName}`, | |
| userIdSchema.parse(isUserNativeFederated(member) ? member.username : `@${member.username}:${this.serverName}`), |
There was a problem hiding this comment.
I've fixed the inconsistent user ID validation at line 296 in ee/packages/federation-matrix/src/FederationMatrix.ts.
The issue: When inviting non-native federated users to a group DM room, the constructed user ID (@${member.username}:${this.serverName}) was passed directly to federationSDK.inviteUserToRoom() without validation through userIdSchema.parse().
The fix: Now both branches of the conditional are validated through userIdSchema.parse():
userIdSchema.parse(isUserNativeFederated(member) ? member.username : `@${member.username}:${this.serverName}`)This makes the code consistent with the pattern used in inviteUsersToRoom (line 553) and ensures proper type safety for the UserID branded type.
PR: #37937
|
|
…7924) Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Proposed changes (including videos or screenshots)
Issue(s)
FB-164
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.