Skip to content

Conversation

@rodrigok
Copy link
Member

@rodrigok rodrigok commented Oct 10, 2025

https://rocketchat.atlassian.net/browse/FDR-237

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • New Features

    • Streamlined invitations for newly created native-federated rooms by sending a single batch invite to all non-owner members after federation is confirmed.
  • Refactor

    • Replaced per-member invitation loop with a consolidated flow that only runs when a room is confirmed native-federated.
  • Impact

    • Faster setup for federated rooms and fewer redundant invite operations; unchanged behavior for non-federated rooms.

Copilot AI review requested due to automatic review settings October 10, 2025 21:20
@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Oct 10, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Oct 10, 2025

⚠️ No Changeset found

Latest commit: 36f53dc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where local and remote users couldn't be added together via the room creation modal in the federation system. The fix refactors the room creation logic to properly handle inviting members to federated rooms.

  • Removes inline user invitation logic from room creation
  • Adds proper federated room validation before inviting users
  • Delegates user invitation to a dedicated method for better handling of mixed local/remote users

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


await this.homeserverServices.invite.inviteUserToRoom(userIdSchema.parse(member), matrixRoomResult.room_id, matrixUserId);
if (federatedRoom && isRoomNativeFederated(federatedRoom)) {
await this.inviteUsersToRoom(federatedRoom, members.filter(m => m != owner.username), owner);
Copy link

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

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

Use strict inequality operator (!==) instead of loose inequality (!=) for better type safety and consistency.

Suggested change
await this.inviteUsersToRoom(federatedRoom, members.filter(m => m != owner.username), owner);
await this.inviteUsersToRoom(federatedRoom, members.filter(m => m !== owner.username), owner);

Copilot uses AI. Check for mistakes.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 10, 2025

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

After creating and marking a Matrix room as federated, the code now loads the federated room record and, if the room is native-federated, performs a single consolidated inviteUsersToRoom(roomId, members[]) call for all non-owner members instead of inviting each member individually. Non-native or non-federated rooms keep prior behavior.

Changes

Cohort / File(s) Summary of changes
Federated room invite flow consolidation
ee/packages/federation-matrix/src/FederationMatrix.ts
After room creation and marking as federated, fetches the federated room (Rooms.findOneById), checks isRoomNativeFederated(federatedRoom), filters out the owner, and issues a single inviteUsersToRoom(roomId, members[]) call instead of per-member invites. Non-native or non-federated paths are unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant App as App Server
  participant Matrix as Matrix HS
  participant FedStore as Federated Room Store

  User->>App: Create federated room (with members)
  App->>Matrix: createRoom()
  Matrix-->>App: roomId
  App->>FedStore: Rooms.findOneById(roomId)
  FedStore-->>App: federatedRoom | null

  alt federatedRoom exists AND isRoomNativeFederated(federatedRoom)
    App->>App: filter members (exclude owner)
    App->>Matrix: inviteUsersToRoom(roomId, members[])
    Matrix-->>App: invites sent
  else Non-federated or non-native
    App->>App: Skip batch invites (retain prior per-member or no-invite behavior)
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

stat: ready to merge, stat: QA assured

Suggested reviewers

  • ggazzo
  • ricardogarim

Poem

I hop, I nibble, then I zoom,
One basket holds each friend’s room.
No single invites left to send,
A single call — and all transcend.
Rabbity cheers — the party’s mend! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title accurately captures the main change, specifically fixing the inability to add both local and remote users in the room creation modal by reworking the federated invitation flow. It directly reflects the bug being addressed without extraneous detail or vagueness.
Linked Issues Check ✅ Passed The changes implement a consolidated batch invitation for all non-owner members in federated rooms, ensuring that multiple external homeserver users selected in the creation modal are invited successfully after room creation. This directly addresses FDR-237’s requirement to add all selected cross-homeserver users and eliminates the previous behavior where external members were missing.
Out of Scope Changes Check ✅ Passed The only modifications apply to the federated room invitation flow within FederationMatrix.ts, directly targeting the bug outlined in FDR-237. No unrelated files or features were altered, and there are no extraneous changes outside the scope of fixing the room creation invite behavior.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ 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/federation-cant-add-local-and-remote-on-creationg

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

📥 Commits

Reviewing files that changed from the base of the PR and between feb682a and 36f53dc.

📒 Files selected for processing (1)
  • ee/packages/federation-matrix/src/FederationMatrix.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • ee/packages/federation-matrix/src/FederationMatrix.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.

@rodrigok rodrigok added this to the 7.11.0 milestone Oct 10, 2025
@codecov
Copy link

codecov bot commented Oct 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.35%. Comparing base (4bd409d) to head (36f53dc).
⚠️ Report is 1 commits behind head on release-7.11.0.

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##           release-7.11.0   #37208      +/-   ##
==================================================
- Coverage           66.37%   66.35%   -0.02%     
==================================================
  Files                3386     3386              
  Lines              115619   115619              
  Branches            21351    21356       +5     
==================================================
- Hits                76739    76723      -16     
- Misses              36275    36286      +11     
- Partials             2605     2610       +5     
Flag Coverage Δ
e2e 57.24% <ø> (-0.05%) ⬇️
unit 71.23% <ø> (+<0.01%) ⬆️

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.

@rodrigok rodrigok force-pushed the fix/federation-cant-add-local-and-remote-on-creationg branch from 785f505 to feb682a Compare October 10, 2025 23:23
@rodrigok rodrigok force-pushed the fix/federation-cant-add-local-and-remote-on-creationg branch from feb682a to 36f53dc Compare October 11, 2025 00:33
@ggazzo ggazzo merged commit 79c2a9b into release-7.11.0 Oct 11, 2025
47 of 49 checks passed
@ggazzo ggazzo deleted the fix/federation-cant-add-local-and-remote-on-creationg branch October 11, 2025 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants