regression(native-federation): fix not working with micro services #37110
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 |
|
|
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. WalkthroughReplaces the standalone federation-service with an in-repo federation-matrix flow: adds setupFederationMatrix and getFederationRoutes, refactors FederationMatrix to lifecycle-based initialization, removes IFederationMatrixService.getAllRoutes and ee/apps/federation-service package, and makes Settings.getValueById generic. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Meteor as Meteor EE Startup
participant Settings as Settings
participant License as License
participant Setup as setupFederationMatrix
participant Fed as FederationMatrix
participant WebApp as WebApp.rawConnectHandlers
Meteor->>Setup: setupFederationMatrix(instanceId)
Setup->>Settings: read federation settings
Setup->>License: check License.hasModule('federation')
alt enabled & valid domain
Setup->>Setup: create Config / Emitter / Container
Setup->>Fed: register events (typing/presence)
Meteor->>Fed: api.registerService(new FederationMatrix())
Meteor->>WebApp: getFederationRoutes()
WebApp->>WebApp: register /_matrix and /.well-known handlers
else disabled/invalid
Setup-->>Meteor: log and exit early
end
sequenceDiagram
autonumber
participant Client as Remote HS
participant WebApp as WebApp Handlers
participant Routes as getFederationRoutes()
participant Subroutes as Matrix Subroutes
participant FedService as FederationMatrix
Client->>WebApp: HTTP request (/_matrix/... or /.well-known/...)
WebApp->>Routes: dispatch
Routes->>Subroutes: apply middleware (auth/license/domain)
Subroutes->>FedService: invoke handler (keys/rooms/media/tx...)
FedService-->>Subroutes: response
Subroutes-->>WebApp: send response
WebApp-->>Client: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the native federation service to resolve compatibility issues with microservices architecture by simplifying the service initialization and removing the standalone federation service app.
- Refactored federation matrix service initialization to use a setup function instead of complex create method
- Removed dedicated federation-service microservice app and its configuration files
- Simplified federation routes registration and moved HTTP route management to separate modules
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/models/src/models/Settings.ts | Added generic type support to getValueById method for type-safe setting retrieval |
| packages/model-typings/src/models/ISettingsModel.ts | Updated interface to support generic typing for getValueById method |
| packages/core-services/src/types/IFederationMatrixService.ts | Removed getAllRoutes method from service interface |
| ee/packages/federation-matrix/tsconfig.json | Changed entry point from FederationMatrix.ts to index.ts |
| ee/packages/federation-matrix/src/setup.ts | New setup function for initializing federation with configuration validation |
| ee/packages/federation-matrix/src/index.ts | New entry point exporting all federation matrix functionality |
| ee/packages/federation-matrix/src/api/routes.ts | New module for managing federation HTTP routes |
| ee/packages/federation-matrix/src/FederationMatrix.ts | Refactored service class to remove initialization complexity and route management |
| ee/packages/federation-matrix/package.json | Updated main entry point to use new index.js |
| ee/apps/federation-service/* | Removed entire federation-service microservice app |
| apps/meteor/package.json | Removed federation-service dependency |
| apps/meteor/ee/server/startup/federation.ts | Simplified federation startup using new setup function |
| apps/meteor/ee/server/api/federation.ts | Updated to use new route registration approach |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } else if (_id === 'Federation_Service_EDU_Process_Typing' && typeof value === 'boolean') { | ||
| this.processEDUTyping = value; | ||
| } else if (_id === 'Federation_Service_EDU_Process_Presence' && typeof value === 'boolean') { | ||
| this.processEDUTyping = value; |
There was a problem hiding this comment.
Should be setting this.processEDUPresence = value instead of this.processEDUTyping = value for the 'Federation_Service_EDU_Process_Presence' setting.
| this.processEDUTyping = value; | |
| this.processEDUPresence = value; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/api/federation.ts (1)
16-23: RequirematrixIdsin the schema to avoid runtime crashes
FederationMatrix.verifyMatrixIds()(ee/packages/federation-matrix/src/FederationMatrix.ts) assumes it receives an array and immediately maps over it. If a caller omitsmatrixIds, the current schema lets the request through and we end up invoking.maponundefined, yielding a 500 instead of a clean validation error. Please markmatrixIdsas required so Ajv blocks malformed requests up front.Apply this diff to make the field mandatory:
query: ajv.compile<{ matrixIds: string[]; }>({ type: 'object', properties: { matrixIds: { type: 'array', items: { type: 'string' } }, }, + required: ['matrixIds'], }),
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
98-100: Settings retrieval pattern has code duplication with setup.ts.Lines 98-100 retrieve the same settings (
Federation_Service_Domain,Federation_Service_EDU_Process_Typing,Federation_Service_EDU_Process_Presence) as lines 37, 45-46 in setup.ts. This duplication could lead to inconsistencies if the settings IDs or default values change.Consider extracting a shared function to retrieve federation settings:
// In a shared module: export async function getFederationSettings() { return { serverName: (await Settings.getValueById<string>('Federation_Service_Domain')) || '', processEDUTyping: (await Settings.getValueById<boolean>('Federation_Service_EDU_Process_Typing')) || false, processEDUPresence: (await Settings.getValueById<boolean>('Federation_Service_EDU_Process_Presence')) || false, }; }Then use it in both files:
const { serverName, processEDUTyping, processEDUPresence } = await getFederationSettings();ee/packages/federation-matrix/src/setup.ts (1)
36-37: Enforce runtime type validation inSettings.getValueById<T>
The current implementation inpackages/models/src/models/Settings.tssimply does:return setting?.value as T;without any runtime checks against
T. If the stored value’s type doesn’t match the requested generic, you’ll get a silent cast error at runtime. Introduce a type‐guard or explicit validation (e.g.typeofchecks or a schema validator) to ensuresetting.valuetruly conforms toTbefore returning.
📜 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 (18)
apps/meteor/ee/server/api/federation.ts(2 hunks)apps/meteor/ee/server/startup/federation.ts(1 hunks)apps/meteor/package.json(0 hunks)ee/apps/federation-service/.eslintrc.json(0 hunks)ee/apps/federation-service/CHANGELOG.md(0 hunks)ee/apps/federation-service/package.json(0 hunks)ee/apps/federation-service/src/config.ts(0 hunks)ee/apps/federation-service/src/service.ts(0 hunks)ee/apps/federation-service/tsconfig.json(0 hunks)ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(5 hunks)ee/packages/federation-matrix/src/api/routes.ts(1 hunks)ee/packages/federation-matrix/src/index.ts(1 hunks)ee/packages/federation-matrix/src/setup.ts(1 hunks)ee/packages/federation-matrix/tsconfig.json(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(0 hunks)packages/model-typings/src/models/ISettingsModel.ts(2 hunks)packages/models/src/models/Settings.ts(2 hunks)
💤 Files with no reviewable changes (8)
- packages/core-services/src/types/IFederationMatrixService.ts
- ee/apps/federation-service/.eslintrc.json
- apps/meteor/package.json
- ee/apps/federation-service/package.json
- ee/apps/federation-service/tsconfig.json
- ee/apps/federation-service/src/service.ts
- ee/apps/federation-service/src/config.ts
- ee/apps/federation-service/CHANGELOG.md
🧰 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:
ee/packages/federation-matrix/src/setup.tsapps/meteor/ee/server/startup/federation.tsapps/meteor/ee/server/api/federation.tsee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (5)
ee/packages/federation-matrix/src/setup.ts (3)
ee/packages/federation-matrix/src/index.ts (1)
setupFederationMatrix(9-9)packages/core-services/src/index.ts (1)
License(162-162)ee/packages/federation-matrix/src/events/index.ts (1)
registerEvents(11-22)
ee/packages/federation-matrix/src/api/routes.ts (12)
ee/packages/federation-matrix/src/api/middlewares/isFederationEnabled.ts (1)
isFederationEnabledMiddleware(4-9)ee/packages/federation-matrix/src/api/middlewares/isLicenseEnabled.ts (1)
isLicenseEnabledMiddleware(4-9)ee/packages/federation-matrix/src/api/_matrix/key/server.ts (1)
getKeyServerRoutes(35-56)ee/packages/federation-matrix/src/api/_matrix/versions.ts (1)
getFederationVersionsRoutes(28-54)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedMiddleware(36-61)ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
getMatrixInviteRoutes(295-351)ee/packages/federation-matrix/src/api/_matrix/profiles.ts (1)
getMatrixProfilesRoutes(352-499)ee/packages/federation-matrix/src/api/_matrix/rooms.ts (1)
getMatrixRoomsRoutes(124-207)ee/packages/federation-matrix/src/api/_matrix/send-join.ts (1)
getMatrixSendJoinRoutes(224-250)ee/packages/federation-matrix/src/api/_matrix/transactions.ts (1)
getMatrixTransactionsRoutes(316-506)ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
getMatrixMediaRoutes(76-153)ee/packages/federation-matrix/src/api/.well-known/server.ts (1)
getWellKnownRoutes(22-46)
apps/meteor/ee/server/startup/federation.ts (3)
ee/packages/federation-matrix/src/setup.ts (1)
setupFederationMatrix(35-103)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(28-887)apps/meteor/ee/server/api/federation.ts (1)
registerFederationRoutes(43-52)
apps/meteor/ee/server/api/federation.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(28-887)ee/packages/federation-matrix/src/api/routes.ts (1)
getFederationRoutes(17-39)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)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: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
packages/model-typings/src/models/ISettingsModel.ts (1)
1-16: Generic signature reads well.The generic return type lets callers opt into stronger typing without breaking default behavior. 👍
packages/models/src/models/Settings.ts (1)
24-28: Implementation matches the new typing.Returning
setting?.value as Tkeeps runtime the same while exposing the new compile-time flexibility.ee/packages/federation-matrix/src/setup.ts (1)
8-33: Domain validation logic looks solid.The validation correctly checks for empty strings, uppercase letters, and URL validity. Good use of console.error for logging validation failures.
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
816-839: Approve typing notification implementation.The
notifyUserTypingmethod correctly checks theprocessEDUTypingflag and validates the room and user before sending typing notifications. The use ofvoidon line 838 is appropriate for fire-and-forget behavior.
61-63: Remove unnecessary multi-file search for processEDUPresence
TheprocessEDUPresenceflag is only referenced inFederationMatrix.ts, so runningrgacross the repo is redundant. After fixing the assignment on line 52, ensure your unit tests cover EDU-presence gating.Likely an incorrect or invalid review comment.
| async created(): Promise<void> { | ||
| // although this is async function, it is not awaited, so we need to register the listeners before everything else | ||
| this.onEvent('watch.settings', async ({ clientAction, setting }): Promise<void> => { | ||
| if (clientAction === 'removed') { | ||
| return; | ||
| } | ||
|
|
||
| static async create(instanceId: string, emitter?: Emitter<HomeserverEventSignatures>): Promise<FederationMatrix> { | ||
| const instance = new FederationMatrix(emitter); | ||
| const settingsSigningAlg = await Settings.get<string>('Federation_Service_Matrix_Signing_Algorithm'); | ||
| const settingsSigningVersion = await Settings.get<string>('Federation_Service_Matrix_Signing_Version'); | ||
| const settingsSigningKey = await Settings.get<string>('Federation_Service_Matrix_Signing_Key'); | ||
| const serverHostname = (await Settings.get<string>('Federation_Service_Domain')).trim(); | ||
|
|
||
| instance.serverName = serverHostname; | ||
|
|
||
| instance.processEDUTyping = await Settings.get<boolean>('Federation_Service_EDU_Process_Typing'); | ||
| instance.processEDUPresence = await Settings.get<boolean>('Federation_Service_EDU_Process_Presence'); | ||
|
|
||
| const mongoUri = process.env.MONGO_URL || 'mongodb://localhost:3001/meteor'; | ||
|
|
||
| const dbName = process.env.DATABASE_NAME || new URL(mongoUri).pathname.slice(1); | ||
|
|
||
| const config = new ConfigService({ | ||
| instanceId, | ||
| serverName: serverHostname, | ||
| keyRefreshInterval: Number.parseInt(process.env.MATRIX_KEY_REFRESH_INTERVAL || '60', 10), | ||
| matrixDomain: serverHostname, | ||
| version: process.env.SERVER_VERSION || '1.0', | ||
| port: Number.parseInt(process.env.SERVER_PORT || '8080', 10), | ||
| signingKey: `${settingsSigningAlg} ${settingsSigningVersion} ${settingsSigningKey}`, | ||
| signingKeyPath: process.env.CONFIG_FOLDER || './rocketchat.signing.key', | ||
| database: { | ||
| uri: mongoUri, | ||
| name: dbName, | ||
| poolSize: Number.parseInt(process.env.DATABASE_POOL_SIZE || '10', 10), | ||
| }, | ||
| media: { | ||
| maxFileSize: Number.parseInt(process.env.MEDIA_MAX_FILE_SIZE || '100', 10) * 1024 * 1024, | ||
| allowedMimeTypes: process.env.MEDIA_ALLOWED_MIME_TYPES?.split(',') || [ | ||
| 'image/jpeg', | ||
| 'image/png', | ||
| 'image/gif', | ||
| 'image/webp', | ||
| 'text/plain', | ||
| 'application/pdf', | ||
| 'video/mp4', | ||
| 'audio/mpeg', | ||
| 'audio/ogg', | ||
| ], | ||
| enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, | ||
| rateLimits: { | ||
| uploadPerMinute: Number.parseInt(process.env.MEDIA_UPLOAD_RATE_LIMIT || '10', 10), | ||
| downloadPerMinute: Number.parseInt(process.env.MEDIA_DOWNLOAD_RATE_LIMIT || '60', 10), | ||
| }, | ||
| }, | ||
| const { _id, value } = setting; | ||
| if (_id === 'Federation_Service_Domain' && typeof value === 'string') { | ||
| this.serverName = value; | ||
| } else if (_id === 'Federation_Service_EDU_Process_Typing' && typeof value === 'boolean') { | ||
| this.processEDUTyping = value; | ||
| } else if (_id === 'Federation_Service_EDU_Process_Presence' && typeof value === 'boolean') { | ||
| this.processEDUTyping = value; | ||
| } else { | ||
| return; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Avoid async operations in lifecycle hooks without proper coordination.
The created() method registers an async event handler for watch.settings (line 41), but the comment on line 40 notes that the function is not awaited. This means the handler registration completes synchronously, but if settings updates arrive before the initialization at lines 98-108 completes, this.homeserverServices may be undefined, causing runtime errors in other methods.
Consider one of these approaches:
- Initialize critical fields (
serverName,processEDUTyping,processEDUPresence) before registering the watcher. - Add guards in methods that depend on
this.homeserverServicesto handle undefined state gracefully. - Make the watcher registration await initialization completion.
Current code already initializes these fields at lines 98-100, so the issue is mitigated, but the comment suggests this is fragile.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/FederationMatrix.ts around lines 39-56, the
created() lifecycle registers an async 'watch.settings' handler before critical
fields (serverName, processEDUTyping, processEDUPresence / homeserverServices)
are guaranteed initialized, risking undefined access; fix by initializing those
critical fields (read current settings or set safe defaults) before you register
the watcher, or alternatively gate the handler with a simple initializationReady
flag/promise and await it inside the handler (or delay registration until after
lines 98-108 finish) so any incoming setting events cannot observe an
uninitialized homeserverServices or related state.
| try { | ||
| registerEvents(this.eventHandler, this.serverName, { typing: this.processEDUTyping, presence: this.processEDUPresence }); | ||
| } catch (error) { | ||
| this.logger.warn('Homeserver module not available, running in limited mode'); | ||
| } | ||
| } | ||
|
|
||
| getAllRoutes() { | ||
| return this.httpRoutes; | ||
| } | ||
| this.homeserverServices = getAllServices(); | ||
|
|
||
| getServerName(): string { | ||
| return this.serverName; | ||
| MatrixMediaService.setHomeserverServices(this.homeserverServices); | ||
| } catch (err) { | ||
| this.logger.warn({ msg: 'Homeserver module not available, running in limited mode', err }); | ||
| } |
There was a problem hiding this comment.
Improve error handling and service availability checks.
The try-catch block (lines 102-108) catches errors during getAllServices() and logs a warning, but doesn't set a flag to indicate limited mode. Later methods check if (!this.homeserverServices) but this check will always be false after the catch block, leading to potential null pointer exceptions.
Apply this pattern to properly handle missing services:
try {
this.homeserverServices = getAllServices();
MatrixMediaService.setHomeserverServices(this.homeserverServices);
} catch (err) {
this.logger.warn({ msg: 'Homeserver module not available, running in limited mode', err });
+ // Explicitly set to undefined so checks throughout the class work correctly
+ this.homeserverServices = undefined as any;
}Better yet, check the result after the call:
try {
- this.homeserverServices = getAllServices();
-
- MatrixMediaService.setHomeserverServices(this.homeserverServices);
+ const services = getAllServices();
+ if (services) {
+ this.homeserverServices = services;
+ MatrixMediaService.setHomeserverServices(this.homeserverServices);
+ } else {
+ this.logger.warn('Homeserver module not available, running in limited mode');
+ }
} catch (err) {
this.logger.warn({ msg: 'Homeserver module not available, running in limited mode', err });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| registerEvents(this.eventHandler, this.serverName, { typing: this.processEDUTyping, presence: this.processEDUPresence }); | |
| } catch (error) { | |
| this.logger.warn('Homeserver module not available, running in limited mode'); | |
| } | |
| } | |
| getAllRoutes() { | |
| return this.httpRoutes; | |
| } | |
| this.homeserverServices = getAllServices(); | |
| getServerName(): string { | |
| return this.serverName; | |
| MatrixMediaService.setHomeserverServices(this.homeserverServices); | |
| } catch (err) { | |
| this.logger.warn({ msg: 'Homeserver module not available, running in limited mode', err }); | |
| } | |
| try { | |
| - this.homeserverServices = getAllServices(); | |
| - | |
| const services = getAllServices(); | |
| if (services) { | |
| this.homeserverServices = services; | |
| MatrixMediaService.setHomeserverServices(this.homeserverServices); | |
| } else { | |
| this.logger.warn('Homeserver module not available, running in limited mode'); | |
| } | |
| } catch (err) { | |
| this.logger.warn({ msg: 'Homeserver module not available, running in limited mode', err }); | |
| } |
| const mongoUri = process.env.MONGO_URL || 'mongodb://localhost:3001/meteor'; | ||
| const dbName = process.env.DATABASE_NAME || new URL(mongoUri).pathname.slice(1); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify MongoDB URI parsing fallback.
The code parses DATABASE_NAME from MONGO_URL using new URL(mongoUri).pathname.slice(1), but if MONGO_URL is malformed or the pathname is empty, this could fail or produce an incorrect database name.
🏁 Script executed:
#!/bin/bash
# Check if there are any validation utilities or tests for MONGO_URL parsing
rg -nP --type=ts -C3 'MONGO_URL.*pathname|DATABASE_NAME' -g '!node_modules' -g '!dist'Length of output: 148
#!/bin/bash
set -eux
# Locate the setup.ts file
fd "setup.ts" ee/packages/federation-matrix/src
# Print lines 1–100 to inspect the parsing code
sed -n '1,100p' ee/packages/federation-matrix/src/setup.ts
# Search within that file for MONGO_URL usage
rg -n -C3 "MONGO_URL" ee/packages/federation-matrix/src/setup.ts#!/bin/bash
set -eux
# Locate and display setup.ts
fd --full-path "ee/packages/federation-matrix/src/setup.ts"
sed -n '1,80p' ee/packages/federation-matrix/src/setup.ts
# Search specifically for MONGO_URL and DATABASE_NAME usage
rg -n -C2 "MONGO_URL|DATABASE_NAME" ee/packages/federation-matrix/src/setup.ts🏁 Script executed:
#!/bin/bash
set -eux
# Locate setup.ts
fd --full-path "setup.ts" ee/packages/federation-matrix/src
# Print lines around the parsing logic
sed -n '40,80p' ee/packages/federation-matrix/src/setup.ts
# Search that range for fallback or validation logic
rg -n -C2 "new URL" ee/packages/federation-matrix/src/setup.ts
rg -n -C2 "slice" ee/packages/federation-matrix/src/setup.ts
rg -n -C2 "DATABASE_NAME" ee/packages/federation-matrix/src/setup.tsLength of output: 2910
Add validation and fallback for MongoDB URI parsing
Wrap the new URL(mongoUri).pathname.slice(1) call in a try/catch (at ee/packages/federation-matrix/src/setup.ts lines 52–53) and provide a sensible default or explicit error if the URI is malformed or its pathname is empty.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/setup.ts around lines 52 to 53, wrap the
new URL(mongoUri).pathname.slice(1) call in a try/catch and validate the parsed
pathname; if URL parsing fails or the pathname is empty, fall back to a sensible
default (e.g., 'meteor') or use an explicit error path (throw or
processLogger.error and process.exit(1)) so the app doesn't continue with an
invalid DB name; ensure you prefer process.env.DATABASE_NAME when present and
only use the parsed value when valid.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37110 +/- ##
================================================
Coverage 67.39% 67.39%
================================================
Files 3328 3330 +2
Lines 113353 113493 +140
Branches 20565 20601 +36
================================================
+ Hits 76389 76488 +99
- Misses 34359 34385 +26
- Partials 2605 2620 +15
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
ee/packages/federation-matrix/src/setup.ts (2)
52-54: Add validation and fallback for MongoDB URI parsing.The MongoDB URI parsing on line 54 using
new URL(mongoUri).pathname.slice(1)can fail if:
- The
mongoUriis malformed- The pathname is empty (would result in empty string as database name)
Wrap the parsing in a try-catch and validate the result:
// TODO are these required? const mongoUri = process.env.MONGO_URL || 'mongodb://localhost:3001/meteor'; - const dbName = process.env.DATABASE_NAME || new URL(mongoUri).pathname.slice(1); + let dbName = process.env.DATABASE_NAME; + if (!dbName) { + try { + const parsed = new URL(mongoUri).pathname.slice(1); + dbName = parsed || 'meteor'; + } catch (error) { + console.error('Failed to parse MONGO_URL, using default database name', error); + dbName = 'meteor'; + } + }
83-83: Fix redundant boolean comparison.Line 83 uses
process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, which always evaluates totrueregardless of the environment variable value. This makes the conditional check ineffective.Apply this diff:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS !== 'false',Or if you want strict opt-in behavior:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true',ee/packages/federation-matrix/src/FederationMatrix.ts (3)
52-53: Fix copy-paste error in settings watcher.Line 52 incorrectly assigns the
Federation_Service_EDU_Process_Presencesetting value tothis.processEDUTypinginstead ofthis.processEDUPresence. This is a critical logic bug that will cause presence events to be processed incorrectly.Apply this diff:
} else if (_id === 'Federation_Service_EDU_Process_Presence' && typeof value === 'boolean') { - this.processEDUTyping = value; + this.processEDUPresence = value; }
40-55: Race condition: settings watcher may fire before initialization completes.The
created()method registers the settings watcher immediately (line 42), buthomeserverServicesisn't initialized until lines 101-107. If a settings change event arrives during this window and triggers the handler, methods accessingthis.homeserverServices(like the presence handler at line 85) may encounter undefined state.Consider initializing
homeserverServicesbefore registering the watcher, or add a guard in the presence handler:async created(): Promise<void> { + this.serverName = (await Settings.getValueById<string>('Federation_Service_Domain')) || ''; + this.processEDUTyping = (await Settings.getValueById<boolean>('Federation_Service_EDU_Process_Typing')) || false; + this.processEDUPresence = (await Settings.getValueById<boolean>('Federation_Service_EDU_Process_Presence')) || false; + + try { + this.homeserverServices = getAllServices(); + MatrixMediaService.setHomeserverServices(this.homeserverServices); + } catch (err) { + this.logger.warn({ msg: 'Homeserver module not available, running in limited mode', err }); + } + // although this is async function, it is not awaited, so we need to register the listeners before everything else this.onEvent('watch.settings', async ({ clientAction, setting }): Promise<void> => {
101-108: Improve error handling for service initialization.If
getAllServices()throws an exception before assignment (line 102),this.homeserverServicesremains uninitialized (not explicitlyundefined). Methods throughout the class checkif (!this.homeserverServices), but this guard may not work correctly with an uninitialized field.Explicitly handle the undefined state:
+ this.homeserverServices = undefined; + try { this.homeserverServices = getAllServices(); MatrixMediaService.setHomeserverServices(this.homeserverServices); } catch (err) { this.logger.warn({ msg: 'Homeserver module not available, running in limited mode', err }); }This ensures the field has a defined value even when initialization fails.
🧹 Nitpick comments (4)
ee/packages/federation-matrix/src/setup.ts (1)
9-34: Consider using structured logging.The
validateDomainfunction usesconsole.errorfor error messages. Since this file imports and uses other infrastructure likeSettingsandLicense, consider using a proper logger (e.g.,Loggerfrom@rocket.chat/logger) for consistency with the rest of the codebase and better observability.Import and use Logger:
+import { Logger } from '@rocket.chat/logger'; + +const processLogger = new Logger('federation-matrix:setup'); + function validateDomain(domain: string): boolean { const value = domain.trim(); if (!value) { - console.error('The Federation domain is not set'); + processLogger.error('The Federation domain is not set'); return false; } if (value.toLowerCase() !== value) { - console.error(`The Federation domain "${value}" cannot have uppercase letters`); + processLogger.error(`The Federation domain "${value}" cannot have uppercase letters`); return false; } try { const valid = new URL(`https://${value}`).hostname === value; if (!valid) { throw new Error(); } } catch { - console.error(`The configured Federation domain "${value}" is not valid`); + processLogger.error(`The configured Federation domain "${value}" is not valid`); return false; } return true; }Similarly update line 42:
- console.log('Federation service is disabled'); + processLogger.info('Federation service is disabled');ee/packages/federation-matrix/src/FederationMatrix.ts (3)
36-36: Consider making homeserverServices nullable in type declaration.The
homeserverServicesfield is declared without initialization and without a nullable type. Methods throughout the class checkif (!this.homeserverServices), but TypeScript won't enforce this guard if the field is typed as non-nullable.Make the type explicitly nullable:
- private homeserverServices: HomeserverServices; + private homeserverServices: HomeserverServices | undefined;This provides better type safety and makes it clear that the field may be uninitialized.
85-93: Consider error handling for fire-and-forget async call.Line 85 uses
voidto explicitly ignore the promise returned bysendPresenceUpdateToRooms. While this fire-and-forget pattern may be intentional for performance, any errors in the presence update will be silently lost.Consider adding error handling:
- void this.homeserverServices.edu.sendPresenceUpdateToRooms( + this.homeserverServices.edu.sendPresenceUpdateToRooms( [ { user_id: localUser.federation.mui, presence: statusMap[user.status] || 'offline', }, ], roomsUserIsMemberOf.map(({ externalRoomId }) => externalRoomId).filter(Boolean), - ); + ).catch((error) => { + this.logger.error('Failed to send presence update', error); + });
837-837: Consider error handling for fire-and-forget async call.Line 837 uses
voidto ignore the promise fromsendTypingNotification. Similar to the presence update at line 85, errors will be silently lost.Add error handling:
- void this.homeserverServices.edu.sendTypingNotification(room.federation.mrid, userMui, isTyping); + this.homeserverServices.edu.sendTypingNotification(room.federation.mrid, userMui, isTyping) + .catch((error) => { + this.logger.error('Failed to send typing notification', error); + });
📜 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 (2)
ee/packages/federation-matrix/src/FederationMatrix.ts(5 hunks)ee/packages/federation-matrix/src/setup.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/setup.tsee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/setup.ts (3)
ee/packages/federation-matrix/src/index.ts (1)
setupFederationMatrix(9-9)packages/core-services/src/index.ts (1)
License(162-162)ee/packages/federation-matrix/src/events/index.ts (1)
registerEvents(11-22)
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: CodeQL-Build
- GitHub Check: CodeQL-Build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/ee/server/startup/federation.ts (1)
17-21: Stop typing events are reported as "still typing"
activity.includes('user-typing')returns true for both the start ('user-typing') and stop ('user-typing-off'/'user-typing-stopped') notifications, so Matrix peers never receive the stop signal. Remote clients keep showing the user typing indefinitely.Apply this diff to fix:
- void FederationMatrixService.notifyUserTyping(rid, user, activity.includes('user-typing')); + const isTyping = activity === 'user-typing'; + void FederationMatrixService.notifyUserTyping(rid, user, isTyping);
🧹 Nitpick comments (2)
apps/meteor/ee/server/startup/federation.ts (2)
1-2: Consider clearer import aliasing to avoid confusion.Both imports use
FederationMatrixas the identifier (one aliased, one not). While line 1's alias resolves the immediate name collision, the setup is subtle:new FederationMatrix()(line 14) refers to the@rocket.chat/federation-matrixclass, whileFederationMatrixService(line 21) refers to the service proxy from@rocket.chat/core-services. Consider aliasing both for clarity.-import { api, FederationMatrix as FederationMatrixService } from '@rocket.chat/core-services'; -import { FederationMatrix, setupFederationMatrix } from '@rocket.chat/federation-matrix'; +import { api, FederationMatrix as FederationMatrixServiceProxy } from '@rocket.chat/core-services'; +import { FederationMatrix as FederationMatrixClass, setupFederationMatrix } from '@rocket.chat/federation-matrix';Then update usage on lines 14 and 21:
api.registerService(new FederationMatrixClass()); // ... void FederationMatrixServiceProxy.notifyUserTyping(rid, user, activity.includes('user-typing'));
16-16: Consider moving event handler to service initialization.The TODO suggests relocating the
StreamerCentrallistener to the service or setup module, which would improve separation of concerns and make the startup function cleaner.Would you like me to help refactor this event handler into the federation-matrix setup module?
📜 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 (1)
apps/meteor/ee/server/startup/federation.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/startup/federation.ts
🧬 Code graph analysis (1)
apps/meteor/ee/server/startup/federation.ts (4)
ee/packages/federation-matrix/src/setup.ts (1)
setupFederationMatrix(36-104)packages/models/src/index.ts (1)
InstanceStatus(164-164)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(27-886)apps/meteor/ee/server/api/federation.ts (1)
registerFederationRoutes(43-52)
⏰ 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). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/ee/server/startup/federation.ts (3)
12-12: LGTM!The call to
setupFederationMatrix(InstanceStatus.id())correctly passes the instance identifier. Error propagation to the caller is appropriate here.
14-14: LGTM!Service registration follows setup and correctly instantiates the
FederationMatrixclass.
25-29: Verify partial startup behavior is intended.The try/catch logs route registration errors but does not rethrow, allowing
startFederationServiceto complete successfully even when routes fail to register. This means the federation service instance is registered (line 14) and typing events are wired (line 17), but Matrix HTTP endpoints may be unavailable. Confirm this graceful degradation is the desired behavior.If route registration should be critical, rethrow the error:
try { await registerFederationRoutes(); } catch (error) { logger.error('Failed to start federation-matrix service:', error); + throw error; }
sampaiodiego
force-pushed
the
native-federation-ms
branch
from
cec935a to
93c3395
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
85-93: Guard presence updates until homeserver services are readyWhen the settings watcher flips
processEDUPresenceto true beforegetAllServices()finishes (or when the homeserver module is unavailable), this path dereferencesthis.homeserverServices, throwing and breaking the event loop. Please bail out when the services aren’t ready.- void this.homeserverServices.edu.sendPresenceUpdateToRooms( + const services = this.homeserverServices; + if (!services) { + this.logger.warn('Skipping presence EDU: homeserver services not initialized'); + return; + } + + void services.edu.sendPresenceUpdateToRooms(
257-279: Fix remote member lookup when ensuring bridged usersThis block compares
federation.muiagainstmemberId(the Mongo_id), so it never finds existing remote users and inserts duplicates whoseusername/federation.muiare set to that_id. That corrupts the user directory and breaks future lookups.- let memberId: string | undefined; + let memberId: string | undefined; if (typeof member === 'string') { memberMatrixUserId = member.startsWith('@') ? member : `@${member}`; memberId = undefined; } else if (member.username?.includes(':')) { memberMatrixUserId = member.username.startsWith('@') ? member.username : `@${member.username}`; memberId = member._id; } else { continue; } @@ - if (memberId) { - const existingMemberMatrixUserId = await Users.findOne({ 'federation.mui': memberId }); - if (!existingMemberMatrixUserId) { + if (memberId) { + const matrixUserId = memberMatrixUserId; + const existingMemberMatrixUser = await Users.findOne({ 'federation.mui': matrixUserId }); + if (!existingMemberMatrixUser) { const newUser = { - username: memberId, - name: memberId, + username: matrixUserId, + name: typeof member === 'string' ? matrixUserId : member.username || matrixUserId, type: 'user' as const, status: UserStatus.OFFLINE, active: true, roles: ['user'], requirePasswordChange: false, federated: true, federation: { version: 1, - mui: memberId, - origin: memberMatrixUserId.split(':').pop(), + mui: matrixUserId, + origin: matrixUserId.split(':').pop(), }, createdAt: new Date(), _updatedAt: new Date(), }; await Users.insertOne(newUser);
815-838: Typing EDU path needs the same homeserver guardIf typing EDUs are enabled while the homeserver services aren’t initialized, this dereference will throw just like the presence handler. Please guard it the same way.
- void this.homeserverServices.edu.sendTypingNotification(room.federation.mrid, userMui, isTyping); + const services = this.homeserverServices; + if (!services) { + this.logger.warn('Skipping typing EDU: homeserver services not initialized'); + return; + } + + void services.edu.sendTypingNotification(room.federation.mrid, userMui, isTyping);
♻️ Duplicate comments (2)
ee/packages/federation-matrix/src/setup.ts (2)
52-55: Still need a safe fallback when parsing the MongoDB nameThis path throws on malformed
MONGO_URLand quietly produces an empty string when the URL omits the DB segment (e.g.,mongodb://host). Please add the try/catch + fallback we discussed earlier so we either use a sane default (like'meteor') or abort with a clear error.
83-88:enableThumbnailsflag is inverted
process.env.MEDIA_ENABLE_THUMBNAILS !== 'true'makes the feature false when the env var is explicitly'true', and still true when it’s'false'. Please flip the condition so'true'enables and'false'disables (defaulting to true when unset).- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS !== 'true', + enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS !== 'false',
🧹 Nitpick comments (1)
packages/models/src/models/Settings.ts (1)
1-1: Require explicit<T>parameters forgetValueByIdat call sites
- In
OAuth2Client.getUserAuthorizationUrl, changetoconst clientId = await …getValueById(`${this.config.alias}-oauth-client-id`);const clientId = await …getValueById<string>(`${this.config.alias}-oauth-client-id`);- In
OAuth2Client.refreshUserAccessToken, likewise use
getValueById<string>(…)for both client-ID and client-secret.This ensures the returned value is correctly typed as
stringrather than the broadSettingValueunion.
📜 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 (18)
apps/meteor/ee/server/api/federation.ts(2 hunks)apps/meteor/ee/server/startup/federation.ts(1 hunks)apps/meteor/package.json(0 hunks)ee/apps/federation-service/.eslintrc.json(0 hunks)ee/apps/federation-service/CHANGELOG.md(0 hunks)ee/apps/federation-service/package.json(0 hunks)ee/apps/federation-service/src/config.ts(0 hunks)ee/apps/federation-service/src/service.ts(0 hunks)ee/apps/federation-service/tsconfig.json(0 hunks)ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(5 hunks)ee/packages/federation-matrix/src/api/routes.ts(1 hunks)ee/packages/federation-matrix/src/index.ts(1 hunks)ee/packages/federation-matrix/src/setup.ts(1 hunks)ee/packages/federation-matrix/tsconfig.json(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(0 hunks)packages/model-typings/src/models/ISettingsModel.ts(2 hunks)packages/models/src/models/Settings.ts(2 hunks)
💤 Files with no reviewable changes (8)
- ee/apps/federation-service/CHANGELOG.md
- ee/apps/federation-service/src/config.ts
- ee/apps/federation-service/tsconfig.json
- ee/apps/federation-service/.eslintrc.json
- ee/apps/federation-service/package.json
- packages/core-services/src/types/IFederationMatrixService.ts
- ee/apps/federation-service/src/service.ts
- apps/meteor/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- ee/packages/federation-matrix/package.json
- packages/model-typings/src/models/ISettingsModel.ts
- ee/packages/federation-matrix/src/index.ts
- ee/packages/federation-matrix/tsconfig.json
🧰 Additional context used
🧠 Learnings (3)
📓 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:
ee/packages/federation-matrix/src/setup.tsapps/meteor/ee/server/startup/federation.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/api/federation.ts
📚 Learning: 2025-10-01T12:20:13.136Z
Learnt from: sampaiodiego
PR: RocketChat/Rocket.Chat#37110
File: apps/meteor/ee/server/startup/federation.ts:17-21
Timestamp: 2025-10-01T12:20:13.136Z
Learning: In Rocket.Chat's StreamerCentral broadcast for 'notify-room' events with 'user-activity', the `activity` argument is a string (e.g., 'user-typing') when a user starts typing and an empty array when the user stops typing.
Applied to files:
apps/meteor/ee/server/startup/federation.ts
🧬 Code graph analysis (5)
ee/packages/federation-matrix/src/setup.ts (3)
ee/packages/federation-matrix/src/index.ts (1)
setupFederationMatrix(9-9)packages/core-services/src/index.ts (1)
License(162-162)ee/packages/federation-matrix/src/events/index.ts (1)
registerEvents(11-22)
ee/packages/federation-matrix/src/api/routes.ts (12)
ee/packages/federation-matrix/src/api/middlewares/isFederationEnabled.ts (1)
isFederationEnabledMiddleware(4-9)ee/packages/federation-matrix/src/api/middlewares/isLicenseEnabled.ts (1)
isLicenseEnabledMiddleware(4-9)ee/packages/federation-matrix/src/api/_matrix/key/server.ts (1)
getKeyServerRoutes(35-56)ee/packages/federation-matrix/src/api/_matrix/versions.ts (1)
getFederationVersionsRoutes(28-54)ee/packages/federation-matrix/src/api/middlewares/isFederationDomainAllowed.ts (1)
isFederationDomainAllowedMiddleware(36-61)ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
getMatrixInviteRoutes(295-351)ee/packages/federation-matrix/src/api/_matrix/profiles.ts (1)
getMatrixProfilesRoutes(352-499)ee/packages/federation-matrix/src/api/_matrix/rooms.ts (1)
getMatrixRoomsRoutes(124-207)ee/packages/federation-matrix/src/api/_matrix/send-join.ts (1)
getMatrixSendJoinRoutes(224-250)ee/packages/federation-matrix/src/api/_matrix/transactions.ts (1)
getMatrixTransactionsRoutes(316-506)ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
getMatrixMediaRoutes(76-153)ee/packages/federation-matrix/src/api/.well-known/server.ts (1)
getWellKnownRoutes(22-46)
apps/meteor/ee/server/startup/federation.ts (3)
ee/packages/federation-matrix/src/setup.ts (1)
setupFederationMatrix(36-104)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(27-886)apps/meteor/ee/server/api/federation.ts (1)
registerFederationRoutes(43-52)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-151)
apps/meteor/ee/server/api/federation.ts (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(27-886)ee/packages/federation-matrix/src/index.ts (2)
FederationMatrix(3-3)getFederationRoutes(7-7)ee/packages/federation-matrix/src/api/routes.ts (1)
getFederationRoutes(17-39)
⏰ 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
| await setupFederationMatrix(InstanceStatus.id()); | ||
|
|
||
| await api.destroyService(federationMatrixService); | ||
| federationMatrixService = undefined; | ||
| }; | ||
| api.registerService(new FederationMatrix()); | ||
|
|
||
| if (shouldStartService()) { | ||
| await startService(); | ||
| } | ||
|
|
||
| void License.onLicense('federation', async () => { | ||
| logger.debug('Federation license became available'); | ||
| if (shouldStartService()) { | ||
| await startService(); | ||
| // TODO move to service/setup? | ||
| StreamerCentral.on('broadcast', (name, eventName, args) => { | ||
| if (name === 'notify-room' && eventName.endsWith('user-activity')) { | ||
| const [rid] = eventName.split('/'); | ||
| const [user, activity] = args; | ||
| void FederationMatrixService.notifyUserTyping(rid, user, activity.includes('user-typing')); | ||
| } | ||
| }); | ||
|
|
||
| License.onInvalidateLicense(async () => { | ||
| logger.debug('License invalidated, checking federation module'); | ||
| if (!shouldStartService()) { | ||
| await stopService(); | ||
| } | ||
| }); | ||
|
|
||
| settings.watch('Federation_Service_Enabled', async (enabled) => { | ||
| logger.debug('Federation_Service_Enabled setting changed:', enabled); | ||
| if (shouldStartService()) { | ||
| await startService(); | ||
| } else { | ||
| await stopService(); | ||
| } | ||
| }); | ||
|
|
||
| settings.watch<string>('Federation_Service_Domain', async (domain) => { | ||
| logger.debug('Federation_Service_Domain setting changed:', domain); | ||
| if (shouldStartService()) { | ||
| if (domain.toLowerCase() !== federationMatrixService?.getServerName().toLowerCase()) { | ||
| await stopService(); | ||
| } | ||
| await startService(); | ||
| } else { | ||
| await stopService(); | ||
| } | ||
| }); | ||
| try { | ||
| await registerFederationRoutes(); | ||
| } catch (error) { | ||
| logger.error('Failed to start federation-matrix service:', error); | ||
| } |
There was a problem hiding this comment.
Don't run the rest of startup when federation is disabled
setupFederationMatrix returns before calling createFederationContainer whenever the module is off (no license or the Federation_Service_Enabled flag is false). We still instantiate FederationMatrix and call registerFederationRoutes(). Both paths hit getAllServices(), which throws “Federation container not initialized”, so every non-federated deployment now logs “Failed to start federation-matrix service” during boot. Please short-circuit after setup and skip service/route registration when it reports disabled. For example:
- await setupFederationMatrix(InstanceStatus.id());
-
- api.registerService(new FederationMatrix());
+ const isEnabled = await setupFederationMatrix(InstanceStatus.id());
+ if (!isEnabled) {
+ logger.info('Federation service disabled; skipping federation-matrix startup');
+ return;
+ }
+
+ api.registerService(new FederationMatrix());(This requires setupFederationMatrix to return that boolean.) Based on ee/packages/federation-matrix/src/setup.ts.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/meteor/ee/server/startup/federation.ts around lines 12 to 29,
setupFederationMatrix can return early when federation is disabled, but the
current code continues and instantiates services/routes causing errors; update
the startup to inspect the boolean return value from await
setupFederationMatrix(InstanceStatus.id()) and if it returns false, immediately
return (skip api.registerService, the StreamerCentral broadcast handler, and
registerFederationRoutes calls) so service/route registration only happens when
federation is enabled.
| export const getFederationRoutes = (): { matrix: Router<'/_matrix'>; wellKnown: Router<'/.well-known'> } => { | ||
| const homeserverServices = getAllServices(); | ||
|
|
||
| const matrix = new Router('/_matrix'); | ||
| const wellKnown = new Router('/.well-known'); | ||
|
|
||
| matrix | ||
| .use(isFederationEnabledMiddleware) | ||
| .use(isLicenseEnabledMiddleware) | ||
| .use(getKeyServerRoutes(homeserverServices)) | ||
| .use(getFederationVersionsRoutes(homeserverServices)) | ||
| .use(isFederationDomainAllowedMiddleware) | ||
| .use(getMatrixInviteRoutes(homeserverServices)) | ||
| .use(getMatrixProfilesRoutes(homeserverServices)) | ||
| .use(getMatrixRoomsRoutes(homeserverServices)) | ||
| .use(getMatrixSendJoinRoutes(homeserverServices)) | ||
| .use(getMatrixTransactionsRoutes(homeserverServices)) | ||
| .use(getMatrixMediaRoutes(homeserverServices)); | ||
|
|
||
| wellKnown.use(isFederationEnabledMiddleware).use(isLicenseEnabledMiddleware).use(getWellKnownRoutes(homeserverServices)); | ||
|
|
||
| return { matrix, wellKnown }; |
There was a problem hiding this comment.
Defensive guard around getAllServices() is missing
getAllServices() throws until createFederationContainer() runs. If the license is disabled or setup fails, this function will now crash the route wiring and prevent the server from booting. Please wrap the call, bail out (or return empty routers), and log a warning instead of letting the exception propagate.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/routes.ts around lines 17 to 38,
getAllServices() can throw before createFederationContainer() runs which will
crash route wiring; wrap the call in a try/catch, and on error log a warning
including the error details and return empty Router instances for '/_matrix' and
'/.well-known' (so no further federation route helpers are invoked on an
undefined services object), rather than letting the exception propagate and
prevent server boot.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ee/packages/federation-matrix/src/setup.ts (1)
50-51: Add error handling for MongoDB URI parsing.The
new URL(mongoUri).pathname.slice(1)call can throw ifMONGO_URLis malformed or produce an empty string if the pathname is just"/". This should be wrapped in a try/catch block with validation.Apply this diff to add error handling:
- const mongoUri = process.env.MONGO_URL || 'mongodb://localhost:3001/meteor'; - const dbName = process.env.DATABASE_NAME || new URL(mongoUri).pathname.slice(1); + const mongoUri = process.env.MONGO_URL || 'mongodb://localhost:3001/meteor'; + let dbName = process.env.DATABASE_NAME; + if (!dbName) { + try { + const parsed = new URL(mongoUri); + dbName = parsed.pathname.slice(1); + if (!dbName) { + logger.error('MongoDB URI pathname is empty; defaulting to "meteor"'); + dbName = 'meteor'; + } + } catch (error) { + logger.error('Failed to parse MONGO_URL; defaulting database name to "meteor"', error); + dbName = 'meteor'; + } + }apps/meteor/ee/server/startup/federation.ts (1)
13-22: Move service and route registration after enablement check.Lines 15 and 17 unconditionally register the federation service and routes before checking
isEnabledat line 20. When federation is disabled (no license, feature flag off, or invalid domain),setupFederationMatrixreturnsfalse, but the service and routes are already registered. This causes "Federation container not initialized" errors when the registered endpoints are accessed.Apply this diff to guard registration:
const isEnabled = await setupFederationMatrix(InstanceStatus.id()); + if (!isEnabled) { + logger.info('Federation service disabled; skipping federation-matrix startup'); + return; + } api.registerService(new FederationMatrix()); await registerFederationRoutes(); - // only registers the typing listener if the service is enabled - if (!isEnabled) { - return; - } - // TODO move to service/setup? StreamerCentral.on('broadcast', (name, eventName, args) => {
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/setup.ts (1)
88-100: Consider checking service enablement before container initialization.The federation container is initialized (lines 90-95) before checking if the service is enabled (line 97). If federation is disabled (no license, feature flag off, or invalid domain), the function returns
falsebut the container has already been created, which may allocate resources unnecessarily.Consider reordering to validate enablement first:
- const eventHandler = new Emitter<HomeserverEventSignatures>(); - - await createFederationContainer( - { - emitter: eventHandler, - }, - config, - ); - const serviceEnabled = (await License.hasModule('federation')) && settingEnabled && validateDomain(serverName); if (!serviceEnabled) { return false; } + + const eventHandler = new Emitter<HomeserverEventSignatures>(); + + await createFederationContainer( + { + emitter: eventHandler, + }, + config, + );apps/meteor/ee/server/startup/federation.ts (1)
32-34: Consider whether silent failure is appropriate.The catch block logs the error but doesn't re-throw or return an error indicator. This means startup failures are silent to the caller. If federation startup failure should be communicated (e.g., for monitoring or alerting), consider re-throwing or returning a status.
📜 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 (2)
apps/meteor/ee/server/startup/federation.ts(1 hunks)ee/packages/federation-matrix/src/setup.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/startup/federation.ts
📚 Learning: 2025-10-01T12:20:13.136Z
Learnt from: sampaiodiego
PR: RocketChat/Rocket.Chat#37110
File: apps/meteor/ee/server/startup/federation.ts:17-21
Timestamp: 2025-10-01T12:20:13.136Z
Learning: In Rocket.Chat's StreamerCentral broadcast for 'notify-room' events with 'user-activity', the `activity` argument is a string (e.g., 'user-typing') when a user starts typing and an empty array when the user stops typing.
Applied to files:
apps/meteor/ee/server/startup/federation.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/setup.ts (3)
ee/packages/federation-matrix/src/index.ts (1)
setupFederationMatrix(9-9)packages/core-services/src/index.ts (1)
License(162-162)ee/packages/federation-matrix/src/events/index.ts (1)
registerEvents(11-22)
apps/meteor/ee/server/startup/federation.ts (3)
ee/packages/federation-matrix/src/setup.ts (1)
setupFederationMatrix(39-108)packages/models/src/index.ts (1)
InstanceStatus(164-164)apps/meteor/ee/server/api/federation.ts (1)
registerFederationRoutes(43-52)
⏰ 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/setup.ts (5)
12-37: LGTM! Domain validation is thorough.The validation logic correctly handles empty, uppercase, and malformed domain inputs with appropriate error logging.
40-48: LGTM! Settings loading is safe.All settings are loaded with appropriate type assertions and fallback defaults.
53-79: Configuration setup looks good aside from the thumbnail flag.The
ConfigServiceconstruction uses appropriate defaults and proper integer parsing with radix 10. The allowed MIME types and rate limits are sensible.Also applies to: 81-86
102-105: LGTM! Event registration is correctly gated.Events are only registered when the service is enabled, with appropriate EDU processing flags.
107-107: LGTM! Return value correctly reflects service state.The function returns
trueonly when federation is fully enabled and initialized.apps/meteor/ee/server/startup/federation.ts (1)
24-31: LGTM! Typing notification logic is correct.The
activity.includes('user-typing')check correctly handles both the start event (whenactivityis the string'user-typing') and the stop event (whenactivityis an empty array). Based on learnings.
| 'audio/mpeg', | ||
| 'audio/ogg', | ||
| ], | ||
| enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS !== 'true', |
There was a problem hiding this comment.
Fix inverted boolean logic for thumbnail enablement.
Line 80 uses process.env.MEDIA_ENABLE_THUMBNAILS !== 'true', which means thumbnails are disabled when the environment variable is 'true' and enabled otherwise. This is inverted.
Apply this diff to fix the logic:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS !== 'true',
+ enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS !== 'false',Or for strict opt-in behavior (thumbnails only when explicitly enabled):
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS !== 'true',
+ enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS !== 'true', | |
| enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS !== 'false', |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/setup.ts around line 80, the boolean is
inverted: currently enableThumbnails is set using
process.env.MEDIA_ENABLE_THUMBNAILS !== 'true' which disables thumbnails when
the env var is 'true'; change the expression to
process.env.MEDIA_ENABLE_THUMBNAILS === 'true' so thumbnails are enabled only
when the environment variable is explicitly set to 'true'.
Proposed changes (including videos or screenshots)
Issue(s)
FDR-190
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Refactor
Chores