Skip to content

Conversation

@ggazzo
Copy link
Member

@ggazzo ggazzo commented Sep 20, 2025

Proposed changes (including videos or screenshots)

Issue(s)

Steps to test or reproduce

Further comments

Summary by CodeRabbit

  • New Features

    • Newly created rooms and newly created federated users now include federation metadata for improved interoperability.
  • Bug Fixes

    • Federation actions now consistently respect whether federation is enabled, reducing cases where actions were incorrectly blocked by readiness checks.
    • Streamlined decision logic for federation-related message actions to avoid unnecessary origin checks, improving reliability.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Sep 20, 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

@ggazzo ggazzo added this to the 7.11.0 milestone Sep 20, 2025
@changeset-bot
Copy link

changeset-bot bot commented Sep 20, 2025

🦋 Changeset detected

Latest commit: 482eab6

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

This PR includes changesets to release 41 packages
Name Type
@rocket.chat/meteor Minor
@rocket.chat/ui-client Major
@rocket.chat/gazzodown Major
@rocket.chat/ui-voip Major
@rocket.chat/web-ui-registration Major
@rocket.chat/fuselage-ui-kit Major
@rocket.chat/livechat Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/core-typings Minor
@rocket.chat/rest-typings Minor
@rocket.chat/api-client Patch
@rocket.chat/apps Patch
@rocket.chat/core-services Patch
@rocket.chat/cron Patch
@rocket.chat/ddp-client Patch
@rocket.chat/freeswitch Patch
@rocket.chat/http-router Patch
@rocket.chat/model-typings Patch
@rocket.chat/ui-avatar Major
@rocket.chat/ui-contexts Major
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/federation-service Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/license Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/pdf-worker Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/models Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/mock-providers Patch
@rocket.chat/ui-video-conf Major
@rocket.chat/instance-status Patch
@rocket.chat/omni-core 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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 20, 2025

Walkthrough

Adds federation metadata to room and user creation paths, removes federation readiness checks in utilities, and adjusts message hook gating to depend on room federation state and global enablement.

Changes

Cohort / File(s) Summary
Room creation metadata
apps/meteor/app/lib/server/functions/createRoom.ts
Conditionally adds federated: true and federation: { version: 1 } to room props when extraData.federated is truthy.
Federation utils readiness removal
apps/meteor/server/services/federation/utils.ts
Removes readiness checks; throwIfFederationNotEnabledOrNotReady() now only verifies enabled. Deletes throwIfFederationEnabledButNotReady and throwIfFederationNotReady.
Message hook gating update
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts
Drops isMessageFromNativeFederation check; shouldPerformAction now depends on isRoomFederated, isRoomNativeFederated, and isFederationEnabled(). Marks message param unused.
Matrix invite user metadata
ee/packages/federation-matrix/src/api/_matrix/invite.ts, ee/packages/federation-matrix/src/events/invite.ts
On creating a new user during invite/join, adds federation: { version: 1 } to the inserted user document.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Server
  participant FederationUtils as Federation Utils
  participant MsgHook as BeforeFederationActions

  Client->>Server: Message pre-action hook
  Server->>MsgHook: shouldPerformAction(_message, room)
  MsgHook->>FederationUtils: isFederationEnabled()
  alt Federation enabled AND room federated/native-federated
    MsgHook-->>Server: true
  else Not enabled or not federated
    MsgHook-->>Server: false
  end
Loading
sequenceDiagram
  participant Caller as Any Caller
  participant Utils as throwIfFederationNotEnabledOrNotReady

  Note over Caller,Utils: Old flow
  Caller->>Utils: check()
  Utils-->>Caller: throws if disabled OR not ready

  Note over Caller,Utils: New flow
  Caller->>Utils: check()
  Utils-->>Caller: throws only if disabled (no readiness check)
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 through rooms where flags now gleam,
A tiny stamp: federation’s theme.
Invites land, new users sprout,
With versioned paws, no doubt about.
Readiness napped—still, gates stand true.
Thump-thump! The matrix grew. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix(federation): add missing federation fields" is concise, single-sentence, and accurately reflects the primary change in the changeset—adding federation metadata to room and user creation paths—so it communicates the main intent to reviewers. It is clear and not noisy, though it does not mention the unrelated readiness-check removals present in the PR. Overall the title is sufficiently specific for a teammate scanning history to understand the primary change.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/fix-missing-fields

📜 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 eb4aed7 and 482eab6.

📒 Files selected for processing (5)
  • apps/meteor/app/lib/server/functions/createRoom.ts (1 hunks)
  • apps/meteor/server/services/federation/utils.ts (0 hunks)
  • apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (1 hunks)
  • ee/packages/federation-matrix/src/api/_matrix/invite.ts (1 hunks)
  • ee/packages/federation-matrix/src/events/invite.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • apps/meteor/server/services/federation/utils.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/server/services/messages/hooks/BeforeFederationActions.ts
🧬 Code graph analysis (1)
apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (2)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
  • FederationActions (6-30)
packages/core-typings/src/IRoom.ts (3)
  • IRoom (21-95)
  • isRoomFederated (120-120)
  • isRoomNativeFederated (122-123)
⏰ 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 (6)
ee/packages/federation-matrix/src/events/invite.ts (1)

33-35: Consistent federation metadata addition looks good.

The addition of the federation: { version: 1 } field to newly created user objects aligns with the broader pattern across the codebase to add federation metadata to entities. This ensures consistency with how federation versioning is handled.

ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)

214-216: Consistent federation metadata addition - matches pattern.

The federation metadata { version: 1 } is being consistently added to user objects during the join process, which aligns with the same changes made in the invite.ts event handler and room creation logic.

apps/meteor/app/lib/server/functions/createRoom.ts (1)

193-198: Good conditional federation metadata addition.

The conditional spread pattern properly adds both federated: true and federation: { version: 1 } when extraData.federated is truthy. This maintains backward compatibility while adding the new versioning structure for federated rooms.

apps/meteor/server/services/messages/hooks/BeforeFederationActions.ts (3)

1-1: Import cleanup looks appropriate.

The removal of isMessageFromNativeFederation from the import aligns with the changes to the shouldPerformAction method logic that no longer uses message-origin checks.


7-7: Parameter name change indicates unused variable.

The parameter name change from message to _message correctly indicates that the message parameter is now unused in the updated logic, following TypeScript conventions for unused parameters.


12-16: Simplified federation action gating looks correct.

The updated logic now:

  1. Returns true for non-federated rooms (allows action)
  2. Returns false for federated rooms that aren't native federated
  3. For native federated rooms, gates the action based on isFederationEnabled()

This simplification removes message-origin checks and focuses purely on room federation state and global enablement, which aligns with the PR's objective to fix missing federation fields rather than validate message origins.


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

@codecov
Copy link

codecov bot commented Sep 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.33%. Comparing base (eb4aed7) to head (482eab6).
⚠️ Report is 2 commits behind head on chore/federation-backup.

Additional details and impacted files

Impacted file tree graph

@@                     Coverage Diff                     @@
##           chore/federation-backup   #37015      +/-   ##
===========================================================
+ Coverage                    67.32%   67.33%   +0.01%     
===========================================================
  Files                         3342     3342              
  Lines                       113403   113401       -2     
  Branches                     20705    20724      +19     
===========================================================
+ Hits                         76350    76360      +10     
+ Misses                       34449    34437      -12     
  Partials                      2604     2604              
Flag Coverage Δ
e2e 56.91% <ø> (-0.03%) ⬇️
unit 71.26% <ø> (+<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.

@ggazzo ggazzo changed the base branch from chore/federation-backup to chore/feat-typing-fed September 22, 2025 00:06
@ggazzo ggazzo changed the title refactor(federation): improve user activity event handling fix(federation): add missing federation fields Sep 22, 2025
@ggazzo ggazzo force-pushed the chore/fix-missing-fields branch from 1106f8e to 7a338e4 Compare September 22, 2025 03:57
@sampaiodiego sampaiodiego force-pushed the chore/fix-missing-fields branch from 7a338e4 to 482eab6 Compare September 22, 2025 14:01
@sampaiodiego sampaiodiego changed the base branch from chore/feat-typing-fed to chore/federation-backup September 22, 2025 14:01
@sampaiodiego sampaiodiego marked this pull request as ready for review September 22, 2025 14:03
@sampaiodiego sampaiodiego requested a review from a team as a code owner September 22, 2025 14:03
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