chore(federation): add settings watcher #37289
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 |
|
WalkthroughRefactors federation startup and matrix setup to be settings-driven and idempotent: adds a CRITICAL_SETTINGS watcher and registration flags in federation.ts, and implements lazy container initialization, stronger domain validation, and config hot-update in setup.ts. (45 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Settings as Settings Service
participant FedStartup as federation.ts:startFederationService
participant FedSetup as setup.ts:setupFederationMatrix
participant Container as Federation Container
Note over Settings,FedStartup: CRITICAL_SETTINGS change triggers watcher
Settings->>FedStartup: notify change
FedStartup->>FedStartup: read enabled flag
alt federation enabled
FedStartup->>FedStartup: ensure serviceRegistered
FedStartup->>FedSetup: call setupFederationMatrix()
alt first initialization (containerInitialized = false)
FedSetup->>FedSetup: validate domain & license
FedSetup->>Container: createFederationContainer()
Container-->>FedSetup: container created
FedSetup->>FedSetup: create & register event handlers
FedSetup->>FedSetup: set containerInitialized = true
else already initialized
FedSetup->>Container: getAllServices()
Container-->>FedSetup: services returned
FedSetup->>FedSetup: update in-memory config (no re-registration)
end
FedStartup->>FedStartup: register routes & streamer listener (if needed)
else federation disabled
FedStartup->>FedStartup: skip registration and listeners
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Notes for reviewers:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 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(4 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.tsapps/meteor/ee/server/startup/federation.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/setup.ts (2)
packages/core-services/src/index.ts (1)
License(162-162)ee/packages/federation-matrix/src/events/index.ts (1)
registerEvents(11-23)
apps/meteor/ee/server/startup/federation.ts (2)
ee/packages/federation-matrix/src/setup.ts (1)
setupFederationMatrix(41-124)apps/meteor/ee/server/api/federation.ts (1)
registerFederationRoutes(43-52)
🪛 GitHub Actions: CI
ee/packages/federation-matrix/src/setup.ts
[error] 120-120: TS2339: Property 'updateConfig' does not exist on type 'ConfigService'.
🔇 Additional comments (7)
ee/packages/federation-matrix/src/setup.ts (3)
4-4: Verify SDK exportsgetAllServices.The import looks correct. Ensure that
getAllServicesis exported from@rocket.chat/federation-sdkand matches the version referenced by the related homeserver PR #289.
12-13: LGTM!The module-level flag correctly prevents duplicate container initialization across multiple invocations of
setupFederationMatrix.
45-48: LGTM!The early return correctly short-circuits when federation is disabled, avoiding unnecessary configuration work.
apps/meteor/ee/server/startup/federation.ts (4)
6-6: LGTM!The import and
CRITICAL_SETTINGSlist are well-defined and cover all the essential federation configuration parameters that should trigger reinitialization.Also applies to: 12-22
24-26: LGTM!The flags correctly prevent duplicate service and watcher registrations.
48-53: Settings watcher implementation is correct.The watcher is properly guarded and will correctly trigger reinitialization when critical settings change. However, ensure the duplicate registration issues flagged above (routes and StreamerCentral listener) are addressed to make this pattern safe.
54-57: LGTM!The error handling is appropriate—logs the failure and rethrows to propagate the error.
| } else { | ||
| const services = getAllServices(); | ||
| services.config.updateConfig(configValues); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix TypeScript compilation error: updateConfig method does not exist.
The pipeline failure indicates that updateConfig is not a property of ConfigService. According to the PR description, this relies on homeserver PR #289 which implements the method.
Verify the following:
Additionally, check the package version of @rocket.chat/federation-sdk to ensure it includes the updateConfig method:
If the method doesn't exist yet, this PR should either:
- Wait for the SDK to be updated with the
updateConfigmethod - Update the SDK dependency version to include the new method
- Add a type assertion or type extension as a temporary workaround (not recommended)
🏁 Script executed:
#!/bin/bash
# Check if updateConfig method exists in ConfigService type definition
ast-grep --pattern $'class ConfigService {
$$$
updateConfig($$$) {
$$$
}
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the version of federation-sdk in package.json files
fd -e json -x cat {} | jq -r 'select(.dependencies["@rocket.chat/federation-sdk"] or .devDependencies["@rocket.chat/federation-sdk"]) | {file: input_filename, version: (.dependencies["@rocket.chat/federation-sdk"] // .devDependencies["@rocket.chat/federation-sdk"])}'Length of output: 66
I need to take a different approach with simpler, more targeted searches. Let me verify the codebase more directly:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ConfigService definition (could be class, interface, or type)
rg -nA5 "ConfigService\s*[=\{:]" --type ts --type tsx
Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for getAllServices function definition
rg -nA3 "function getAllServices\|const getAllServices" --type ts --type tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Look at the actual setup.ts file around the problematic lines
cat ee/packages/federation-matrix/src/setup.ts | head -130 | tail -30Length of output: 568
🏁 Script executed:
#!/bin/bash
# Search for updateConfig method definition anywhere
rg -nB2 -A5 "updateConfig\s*\(" --type ts --type tsxLength of output: 27
🏁 Script executed:
#!/bin/bash
# Simple check of federation-sdk version in main package.json
grep -A2 -B2 "federation-sdk" package.jsonLength of output: 0
Let me fix the commands and retry with corrected search patterns:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for ConfigService definition with simpler pattern
rg -nA5 "class ConfigService|interface ConfigService|type ConfigService" --type ts
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for getAllServices function with simpler pattern
rg -nA3 "getAllServices" --type ts | head -50Length of output: 3057
🏁 Script executed:
#!/bin/bash
# Check package.json in the federation-matrix directory
find ee/packages/federation-matrix -name "package.json" -exec grep -l "federation-sdk" {} \;Length of output: 42
🏁 Script executed:
#!/bin/bash
# Look for updateConfig definition
rg -nB2 -A5 "updateConfig" --type ts | head -100Length of output: 1426
🏁 Script executed:
#!/bin/bash
# Check federation-matrix package.json for dependencies
cat ee/packages/federation-matrix/package.json | grep -A5 -B5 "dependencies\|federation"Length of output: 642
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for federation-sdk in node_modules or check if it's installed
find . -path "*/node_modules/@rocket.chat/federation-sdk*" -type d | head -5
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for federation-sdk source files or look for ConfigService type definitions
rg -l "federation-sdk" --type ts | grep -E "(node_modules|dist)" | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's a federation-sdk package source in the workspace
find ee/packages -name "federation-sdk*" -o -name "*federation*sdk*" | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any type definitions or imports of ConfigService to understand its type
rg "ConfigService" --type ts -B2 -A2 | head -80Length of output: 903
---
🌐 Web query:
```web
@rocket.chat/federation-sdk updateConfig method ConfigService
💡 Result:
Do you want the method signature, usage example, or the source code for ConfigService.updateConfig? I can look it up — shall I search the repo/npm docs now?
---
🌐 Web query:
```web
@rocket.chat/federation-sdk version 0.2.0 npm package
💡 Result:
I couldn't find a published @rocket.chat/federation-sdk version 0.2.0 on the public registries. The package appears to have 0.1.x releases (latest listed ~0.1.18); no 0.2.0 release is shown. See registry listings. [1][2]
If you intended to install the latest published release, use:
npm install @rocket.chat/federation-sdk@latest
(Replace @latest with a specific 0.1.x version if you need one.) [1][2]
Sources:
[1] Socket.dev package page for @rocket.chat/federation-sdk (versions list).
[2] Libraries.io npm listing for @rocket.chat/federation-sdk.
Dependency blocker: @rocket.chat/federation-sdk v0.2.0 does not exist yet.
The package appears to have 0.1.x releases; no 0.2.0 release is shown. The code at line 120 calls services.config.updateConfig(configValues), but this method does not exist in any publicly available version of the SDK. Since the PR description references an unreleased homeserver PR (#289) that implements updateConfig, this indicates v0.2.0 is still in development.
Action required:
- Wait for
@rocket.chat/federation-sdkv0.2.0 to be released with theupdateConfigmethod, or - Update the version constraint in
ee/packages/federation-matrix/package.jsononce the SDK is published
🧰 Tools
🪛 GitHub Actions: CI
[error] 120-120: TS2339: Property 'updateConfig' does not exist on type 'ConfigService'.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/setup.ts around lines 118 to 121, the code
calls services.config.updateConfig(configValues) which depends on
@rocket.chat/federation-sdk v0.2.0 that is not yet released; either remove or
guard this call until the SDK is published: replace the direct call with a
runtime check like if (typeof services.config.updateConfig === 'function')
services.config.updateConfig(configValues) or comment/remove the call and add a
TODO and revert to the last supported SDK behavior; additionally update
ee/packages/federation-matrix/package.json to pin the dependency to the correct
released SDK version (or update the constraint to ^0.2.0 once the SDK is
published) so the code and package.json remain consistent.
There was a problem hiding this comment.
@coderabbitai, the library exists in the Homeserver project (PR #289), and once it’s reviewed there, we’ll release an update to bump the library version here.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
replaced by #37357 |
Related to RocketChat/homeserver#289 that implements
updateConfigmethod.Jira Issue: FDR-156.
Summary by CodeRabbit
New Features
Bug Fixes
Chores