chore: onSettingChanged hook for services#38615
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new onSettingChanged API to ServiceClass for subscribing to individual setting updates and replaces several onEvent('watch.settings', ...) listeners with per-setting handlers that validate setting.value, optionally ignore specified client actions, and update local service state. Changes
Sequence Diagram(s)sequenceDiagram
participant EventBus as watch.settings
participant ServiceClass
participant ServiceImpl as Service (Instance/Account/Federation)
participant Settings as SettingStore
EventBus->>ServiceClass: emit { clientAction, setting }
ServiceClass->>ServiceClass: route event to per-setting listeners
ServiceClass->>ServiceImpl: invoke onSettingChanged callback({ clientAction, setting })
ServiceImpl->>Settings: inspect setting.value (type check / ignoreActions)
ServiceImpl-->>ServiceImpl: update internal state when valid
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "02/12 20:42", "02/12 22:57 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 12 Feb 2026 22:57:50 GMT |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38615 +/- ##
===========================================
- Coverage 70.50% 70.45% -0.05%
===========================================
Files 3174 3174
Lines 111063 111077 +14
Branches 20040 20031 -9
===========================================
- Hits 78300 78260 -40
- Misses 30718 30777 +59
+ Partials 2045 2040 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core-services/src/types/ServiceClass.ts`:
- Around line 115-119: The try/catch is not awaiting the async callback so
rejections escape and become unhandled promise rejections; update the invocation
of settingHandler.cb in ServiceClass (the handler call currently inside the try)
to await settingHandler.cb({ clientAction, setting }) (or explicitly handle its
returned Promise with .catch) and ensure the enclosing method is async or
properly returns a Promise so the await is valid; this ensures thrown errors are
caught by the surrounding try/catch instead of leaking.
🧹 Nitpick comments (3)
packages/core-services/src/types/ServiceClass.ts (2)
60-60:Mapallows only one callback per setting — is this intentional?Using a
Map<string, ...>means a secondonSettingChangedcall for the samesettingIdsilently overwrites the first handler. If multiple parts of a service (or a subclass) need to react to the same setting, this will drop listeners. AMap<string, Array<...>>would avoid this, though if one-handler-per-setting is by design, a warning or error on duplicate registration would make the contract explicit.
101-104: Guard condition is tightly coupled to the Map's initial state.
registerEventListenerrelies onsettingEvents.size !== 0to determine if thewatch.settingslistener has already been added. This works today because the method is always called immediately beforeset, but a future refactor (e.g., removing entries) could cause size to return to 0, leading to a duplicate listener registration. A dedicated boolean flag would be more robust.Proposed fix
+ private settingListenerRegistered = false; + private registerEventListener() { - if (this.settingEvents.size !== 0) { + if (this.settingListenerRegistered) { return; } + this.settingListenerRegistered = true; this.onEvent('watch.settings', async ({ clientAction, setting }): Promise<void> => {ee/apps/account-service/src/Account.ts (1)
20-23: Unsafeas numbercast — consider a type guard like the other call-sites.FederationMatrix guards with
typeof value === 'boolean'before assignment. HerevalueisISetting['value'](a broad union) and is cast without validation. Atypeof value === 'number'guard would be consistent with the pattern established elsewhere in this PR.Proposed fix
this.onSettingChanged( 'Accounts_LoginExpiration', async ({ setting }): Promise<void> => { const { value } = setting; - - this.loginExpiration = getLoginExpirationInDays(value as number); + if (typeof value === 'number') { + this.loginExpiration = getLoginExpirationInDays(value); + } }, ['removed'], );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/meteor/ee/server/local-services/instance/service.tsapps/meteor/tests/unit/server/services/instance/service.tests.tsee/apps/account-service/src/Account.tsee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/events/Events.tspackages/core-services/src/types/ServiceClass.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/core-services/src/events/Events.tsee/apps/account-service/src/Account.tsapps/meteor/tests/unit/server/services/instance/service.tests.tsee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/ServiceClass.tsapps/meteor/ee/server/local-services/instance/service.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
📚 Learning: 2026-01-27T20:57:56.529Z
Learnt from: nazabucciarelli
Repo: RocketChat/Rocket.Chat PR: 38294
File: apps/meteor/server/hooks/sauMonitorHooks.ts:0-0
Timestamp: 2026-01-27T20:57:56.529Z
Learning: In Rocket.Chat, the `accounts.login` event listened to by DeviceManagementService is only broadcast when running in microservices mode (via DDPStreamer), not in monolith mode. The `Accounts.onLogin` hook in sauMonitorHooks.ts runs in monolith deployments. These are mutually exclusive deployment modes, so there's no duplication of event emissions between these two code paths.
Applied to files:
ee/apps/account-service/src/Account.tsapps/meteor/ee/server/local-services/instance/service.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
ee/apps/account-service/src/Account.tsapps/meteor/tests/unit/server/services/instance/service.tests.tsee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/ServiceClass.tsapps/meteor/ee/server/local-services/instance/service.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/unit/server/services/instance/service.tests.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/unit/server/services/instance/service.tests.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/local-services/instance/service.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (1)
packages/core-services/src/types/ServiceClass.ts (1)
packages/core-services/src/events/Events.ts (2)
EventSignatures(56-309)ClientAction(41-41)
⏰ 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). (6)
- GitHub Check: 🚢 Build Docker (amd64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, rocketchat, coverage)
- GitHub Check: 🚢 Build Docker (amd64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, authorization-service, queue-worker-service, ddp-streamer-service, cove...
- GitHub Check: 🚢 Build Docker (arm64, account-service, presence-service, omnichannel-transcript-service, cove...
- GitHub Check: 🚢 Build Docker (amd64, rocketchat, coverage)
🔇 Additional comments (4)
packages/core-services/src/events/Events.ts (1)
41-41: LGTM!Exporting
ClientActionis the right call to make it available for the newonSettingChangedAPI.ee/packages/federation-matrix/src/FederationMatrix.ts (1)
44-86: Clean migration to per-setting listeners.The pattern is consistent across all four settings: type-guarded callbacks,
['removed']in ignoreActions, and separate initial-value fetches at lines 128–131. The register-then-fetch ordering avoids a race between initialization and event delivery.apps/meteor/ee/server/local-services/instance/service.ts (1)
53-69: Clean migration with proper guards.Good use of both type validation (
typeof value !== 'boolean') and value-unchanged check to avoid unnecessary state updates.apps/meteor/tests/unit/server/services/instance/service.tests.ts (1)
18-22: Mock updated correctly.The stub enables the constructor to execute without errors. Consider adding an assertion that
onSettingChangedwas called with'Troubleshoot_Disable_Instance_Broadcast'and['removed']to verify the registration wiring.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ricardogarim
left a comment
ricardogarim
left a comment
There was a problem hiding this comment.
For brainstorming: have you considered emitting granular events like settings.{SETTING_NAME} at the source instead of filtering at the service level? This way services could subscribe directly to the settings they care about without needing the wrapper.
Also, would worth documenting this pattern in an ADR for future reference.
This was one of the ideas, but it would generate a bit of noise, for example, when registering listeners if you register 100 settings to watch, then it would mean you will have 100 event listeners attached, and those events are going into moleculer |
additionally to this, we know that settings don't change very often, so having a single event to receive all settings changes is not that costly. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core-services/src/types/ServiceClass.ts (1)
83-85:⚠️ Potential issue | 🟠 Major
removeAllListenersdoesn't clearsettingsemitter or reset the guard flag.After
removeAllListeners()runs, thewatch.settingshandler onthis.eventsis gone, butsettingListenerActivestaystrue. Any subsequent call toonSettingChangedskips re-registration, so thesettingsemitter's listeners become orphaned — they never fire and they leak.Proposed fix
removeAllListeners(): void { this.events.removeAllListeners(); + this.settings.removeAllListeners(); + this.settingListenerActive = false; }
🧹 Nitpick comments (2)
packages/core-services/src/types/ServiceClass.ts (2)
60-60: Consider increasingmaxListenerson thesettingsemitter.Node.js
EventEmitterdefaults to 10 listeners before emitting aMaxListenersExceededWarning. A service watching more than 10 distinct settings (each registers a listener) will trigger this warning. Since each setting ID gets its own event name this is unlikely to hit per-event, but if a single setting is watched multiple times (e.g., across mixins or re-registrations), it could surface.Proposed fix
- protected settings = new EventEmitter(); + protected settings = new EventEmitter({ captureRejections: true }); + + constructor() { + super(); + this.settings.setMaxListeners(0); // unlimited — one listener per watched setting + this.emit = this.emit.bind(this); + }Note: since
ServiceClassdoesn't extend another class, drop thesuper()call — just set it in the existing constructor:constructor() { this.emit = this.emit.bind(this); + this.settings.setMaxListeners(0); }Also applies to: 128-138
133-137: Silent error swallowing hinders debugging.The
catch { // noop }block discards all errors from setting-change callbacks. At minimum, a debug/warn-level log would help operators diagnose misbehaving handlers without changing the "don't crash" intent.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core-services/src/types/ServiceClass.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/core-services/src/types/ServiceClass.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
packages/core-services/src/types/ServiceClass.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
packages/core-services/src/types/ServiceClass.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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
packages/core-services/src/types/ServiceClass.ts (2)
42-46: Interface addition looks good.The
onSettingChangedsignature is well-typed with proper use ofISetting['_id'], the async callback, and the optionalignoreActionsfilter.
107-119: Registration logic is clean.The one-time guard and re-emission by
_idare straightforward. The prior review concern about missingawaiton the callback has been addressed in the current code (line 134).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/core-services/src/types/ServiceClass.ts">
<violation number="1" location="packages/core-services/src/types/ServiceClass.ts:60">
P1: `removeAllListeners()` does not clean up the new `settings` EventEmitter or reset `settingListenerActive`. After `removeAllListeners()` is called (e.g., during service teardown via `LocalBroker`), the `watch.settings` handler is removed from `this.events`, but `settingListenerActive` remains `true`. Any subsequent call to `onSettingChanged()` will skip re-registering the bridge listener, silently breaking setting change delivery. Additionally, the `settings` EventEmitter retains stale listeners.
Update `removeAllListeners()` to also reset the settings state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/core-services/src/types/ServiceClass.ts`:
- Around line 83-86: removeAllListeners() currently clears this.events and
this.settings but leaves the settingListenerActive flag true, which causes
registerEventListener() to early-return and prevents re-establishing the
watch.settings → per-setting bridge when onSettingChanged() runs; update
removeAllListeners() to also set this.settingListenerActive = false so
subsequent onSettingChanged() calls can re-register per-setting listeners via
registerEventListener().
🧹 Nitpick comments (2)
packages/core-services/src/types/ServiceClass.ts (2)
108-120: Async listeners onEventEmitter.emitare fire-and-forget — consider documenting or mitigating.
this.settings.emit(…)on line 118 invokes async listeners synchronously; their returned promises are discarded. In this design the inner try/catch (line 134-138) guards against rejections, so it works today, but any future listener that throws before the try-block (or if the catch itself throws) will produce an unhandled promise rejection. A small mitigation is to add a safety.catchon the emit side or use an async-safe emitter.This is acceptable for now given the narrow scope, but worth keeping in mind.
134-138: Silentcatchswallows all errors — consider at least logging.Silently catching every error (
// noop) makes it very hard to diagnose why a setting-change handler isn't taking effect. Adebug/warn-level log with the setting id would be low-cost and high-value.Also, per coding guidelines, the
// noopcomment should be avoided. As per coding guidelines: "Avoid code comments in the implementation."Proposed improvement
try { await cb({ clientAction, setting }); - } catch { - // noop + } catch (err) { + console.error(`onSettingChanged handler for "${settingId}" failed`, err); }(Or use the service's logger if available.)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/meteor/ee/server/local-services/instance/service.tsee/apps/account-service/src/Account.tsee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/ServiceClass.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/ee/server/local-services/instance/service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
ee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/ServiceClass.tsee/apps/account-service/src/Account.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/ServiceClass.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/ServiceClass.tsee/apps/account-service/src/Account.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (1)
packages/core-services/src/types/ServiceClass.ts (1)
packages/core-services/src/events/Events.ts (1)
ClientAction(41-41)
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
packages/core-services/src/types/ServiceClass.ts (1)
42-46: LGTM — clean API surface for the new hook.The
onSettingChangedinterface declaration is well-typed and concise. DefaultingignoreActionsto optional keeps the call sites clean.ee/apps/account-service/src/Account.ts (1)
18-22: Clean migration toonSettingChanged— initial value correctly loaded instarted().The handler only fires on subsequent changes, while
started()(line 41-44) covers the initial load from the database. This avoids the footgun of assuming the hook fires immediately (unlike the oldsettings.watchMultiple()which did). The defaultignoreActions: ['removed']is appropriate here.ee/packages/federation-matrix/src/FederationMatrix.ts (1)
44-70: Clean decomposition — individual handlers are easier to read and maintain than the prior combined listener.Each handler correctly type-guards
valuebefore assignment and relies on the defaultignoreActions: ['removed']. Initial state is properly bootstrapped from the DB at lines 112-115.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
KevLehman
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1820
Steps to test or reproduce
Further comments
https://github.com/RocketChat/adrs/pull/166
Summary by CodeRabbit