chore: Standardize setting service usage across services#38673
chore: Standardize setting service usage across services#38673kodiakhq[bot] merged 15 commits intodevelopfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (13)
WalkthroughThis PR standardizes settings usage by migrating from model-based Settings.getValueById()/updateValueById to core-services Settings.get()/set(), adds Settings.set to the service API, and introduces lifecycle caching and onSettingChanged listeners in several services and tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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/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 22:57", "02/13 22:38", "02/16 14:04", "02/18 23:15", "02/19 20:19", "02/19 20:52 (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.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, 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.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, 0.13]
line "rocketchat" [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.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 19 Feb 2026 20:52:22 GMT |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38673 +/- ##
===========================================
- Coverage 70.58% 70.54% -0.05%
===========================================
Files 3182 3182
Lines 112442 112471 +29
Branches 20378 20397 +19
===========================================
- Hits 79365 79340 -25
- Misses 31025 31077 +52
- Partials 2052 2054 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
/jira CORE |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/presence/src/Presence.ts (1)
54-54:⚠️ Potential issue | 🟡 MinorState/setting divergence if
Settings.set()rejects insidevoid validateAvailability().
this.broadcastEnabled = false(line 313) executes synchronously before theawait Settings.set(...)on line 315. If the service-layer call fails, the in-memory flag is alreadyfalsewhile the persisted setting remainsfalse(broadcast enabled), leaving the service in a permanently degraded state with no recovery path — silent because the call site usesvoid.With the old direct-model
updateValueByIdthis was less likely; a service call adds a transient-failure surface (e.g., settings service not yet registered at startup).🛡️ Proposed fix — handle rejection inside `validateAvailability`
- if (this.getTotalConnections() > MAX_CONNECTIONS) { - this.broadcastEnabled = false; - - await Settings.set('Presence_broadcast_disabled', true); - } + if (this.getTotalConnections() > MAX_CONNECTIONS) { + try { + await Settings.set('Presence_broadcast_disabled', true); + this.broadcastEnabled = false; + } catch { + // setting persistence failed; leave broadcastEnabled unchanged + } + }Alternatively, wrap
void this.validateAvailability()(line 54) with a.catch()and log the error there, and move the state mutation after the await insidevalidateAvailability.Also applies to: 307-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/presence/src/Presence.ts` at line 54, The call to validateAvailability currently sets the in-memory flag broadcastEnabled synchronously before awaiting Settings.set, which can leave the service permanently out-of-sync if the Settings.set call rejects; update validateAvailability (and the similar block around the current broadcastEnabled/Settings.set usage) so that any Settings.set rejection is handled: move the mutation of this.broadcastEnabled to after the await that persists the setting, and ensure the promise returned by validateAvailability is caught (either by adding a try/catch inside validateAvailability that logs errors and reverts state on failure, or by keeping the callsite but adding .catch(...) to the void this.validateAvailability() invocation and logging the error). Reference validateAvailability, this.broadcastEnabled, and Settings.set when making the change.
🧹 Nitpick comments (6)
apps/meteor/ee/server/local-services/instance/service.ts (1)
187-188: Missing|| falsefallback — inconsistent withFederationMatrix.tspattern.
Settings.get<T>returnsPromise<T>but the underlyingSettingValueunion includesundefined. If the setting isn't cached yet, the assignment silently sets aboolean-typed field toundefined. CompareFederationMatrix.tslines 115–117 which consistently apply|| false. While the field defaulting to falsy (undefined) means the broadcast still proceeds safely, the inconsistency is a type-safety gap.♻️ Proposed fix
- this.troubleshootDisableInstanceBroadcast = await Settings.get<boolean>('Troubleshoot_Disable_Instance_Broadcast'); + this.troubleshootDisableInstanceBroadcast = (await Settings.get<boolean>('Troubleshoot_Disable_Instance_Broadcast')) || false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/local-services/instance/service.ts` around lines 187 - 188, The assignment to this.troubleshootDisableInstanceBroadcast uses Settings.get<boolean>('Troubleshoot_Disable_Instance_Broadcast') which can resolve to undefined, leaving a boolean-typed field undefined; update the assignment so it defaults to false (e.g. append "|| false") like the pattern used in FederationMatrix where settings are normalized; locate the usage of troubleshootDisableInstanceBroadcast and Settings.get in service.ts and apply the same fallback to ensure type-safety and consistent behavior.apps/meteor/server/modules/notifications/notifications.module.ts (1)
202-204: Stale TODO — caching is now implemented.The
// TODO consider using something to cache settingscomment is resolved by the switch toSettings.get, which reads from the in-memoryCachedSettingsstore and no longer round-trips to the DB on every typing event.♻️ Proposed fix
- // TODO consider using something to cache settings const key = (await Settings.get('UI_Use_Real_Name')) ? 'name' : 'username';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/modules/notifications/notifications.module.ts` around lines 202 - 204, Remove the stale TODO comment above the Settings.get call in notifications.module.ts — the cache is already used via Settings.get/CachedSettings, so delete the line "// TODO consider using something to cache settings" and leave the existing logic that computes key = (await Settings.get('UI_Use_Real_Name')) ? 'name' : 'username' intact (no other code changes).apps/meteor/server/services/settings/service.ts (1)
17-22:void notifyOnSettingChangedByIdswallows rejections and may leave the cache stale.
notifyOnSettingChangedByIdawaitsSettings.findOneByIdinternally, so it can reject (e.g., on a DB error). Withvoid, that rejection becomes unhandled — no warning/error surfaces to the caller, listeners don't get notified, and the in-memory cache (read bySettings.get) is left stale until the next update. The DB write has succeeded at that point, creating an inconsistency that persists until the next setting change.♻️ Proposed fix
async set<T extends SettingValue>(settingId: string, value: T): Promise<void> { const update = await Settings.updateValueById(settingId, value); if (update.modifiedCount) { - void notifyOnSettingChangedById(settingId); + await notifyOnSettingChangedById(settingId); } }If fire-and-forget is intentional, at minimum add a
.catchto surface errors:- void notifyOnSettingChangedById(settingId); + notifyOnSettingChangedById(settingId).catch((err) => SystemLogger.error({ msg: 'Failed to notify setting change', settingId, err }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/settings/service.ts` around lines 17 - 22, The current set method calls notifyOnSettingChangedById with "void", which swallows rejections and can leave the in-memory cache stale after a successful Settings.updateValueById; change this to either await notifyOnSettingChangedById(settingId) so callers observe failures, or keep fire-and-forget but append a .catch that logs the error and rethrows or at least reports it (use processLogger or existing logger) so DB/findOne failures from notifyOnSettingChangedById are surfaced; update the async set<T extends SettingValue>(settingId: string, value: T) method (and any callers) accordingly to ensure cache-consistency and error visibility.ee/packages/abac/src/can-access-object.spec.ts (2)
159-161: RedundantafterAll— outerbeforeEachalready provides isolation
afterAllat lines 159–161 resetsservice.decisionCacheTimeout = 300after thedecision cache behaviorsuite. However, the outerbeforeEachat line 56 runsservice = new AbacService()before every subsequent test (including those inquery shape robustness), creating a fresh instance withdecisionCacheTimeout = 60. TheafterAllreset is applied to an instance that will be replaced and has no effect on test isolation.♻️ Suggested cleanup
describe('decision cache behavior', () => { - afterAll(() => { - service.decisionCacheTimeout = 300; - }); - it('uses cached decision (returns true) when within cache TTL...', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/can-access-object.spec.ts` around lines 159 - 161, The afterAll block that sets service.decisionCacheTimeout = 300 is redundant because each test's outer beforeEach constructs a fresh service (new AbacService()) with decisionCacheTimeout defaulting to 60; remove the afterAll(...) teardown that mutates service.decisionCacheTimeout in the "decision cache behavior" suite (or, if persistent teardown was actually intended across tests, replace it with an afterEach that resets the same instance), referencing the service variable and the decisionCacheTimeout property to locate and remove or replace the block.
5-5: Stale variable name and dead mock setup
mockSettingsGetValueById(line 5) now backsSettings.get(line 34), notgetValueById— the name should be updated to avoid confusion. More importantly,mockSettingsGetValueById.mockResolvedValue(300)inbeforeEachis never exercised:AbacService's constructor only invokesonSettingChanged(mocked asjest.fn()), andstarted()is never called in these tests, soSettings.getis never reached. The setup is misleading dead code.♻️ Suggested cleanup
-const mockSettingsGetValueById = jest.fn(); +const mockSettingsGet = jest.fn(); // ... jest.mock('@rocket.chat/core-services', () => ({ ServiceClass: class { onSettingChanged = jest.fn(); }, // ... Settings: { - get: (...args: any[]) => mockSettingsGetValueById(...args), + get: (...args: any[]) => mockSettingsGet(...args), }, })); // In beforeEach: - mockSettingsGetValueById.mockResolvedValue(300); // 5 minute cacheAlso applies to: 34-34, 55-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/abac/src/can-access-object.spec.ts` at line 5, Rename the stale mock variable mockSettingsGetValueById to reflect what it actually mocks (e.g., mockSettingsGet) and either remove the dead setup mockSettingsGet.mockResolvedValue(300) from beforeEach or make the tests exercise Settings.get by calling the AbacService.started() method (or otherwise invoking the code path that calls Settings.get) so the mock is meaningful; update any references to the old name (the jest.fn() declaration and where it is passed to Settings.get) and remove unused mock setup to eliminate misleading dead code.ee/packages/omnichannel-services/src/OmnichannelTranscript.ts (1)
84-84: TODO comment in implementationAs per coding guidelines, code comments should be avoided in implementation files. The
// TODO: cache these with memcomment should either be tracked as an issue externally or addressed in this PR.Would you like me to draft the
mem-based caching wrapper for thestarted()settings block, or open a new issue to track this?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/omnichannel-services/src/OmnichannelTranscript.ts` at line 84, Remove the inline TODO comment and implement a mem-based cache for the settings retrieval used in the started() block of OmnichannelTranscript: wrap the function that fetches the settings (the code inside the started() settings block) with the mem cache library so repeated calls return cached settings, configure an appropriate TTL and keying strategy, and replace the direct call in started() to use the cached wrapper; if you prefer not to implement it now, remove the TODO and create a tracked issue instead mentioning OmnichannelTranscript.started and the need for a mem cache.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/meteor/ee/server/local-services/instance/service.tsapps/meteor/server/modules/notifications/notifications.module.tsapps/meteor/server/services/authorization/canAccessRoom.tsapps/meteor/server/services/nps/service.tsapps/meteor/server/services/settings/service.tsapps/meteor/tests/unit/server/services/nps/spec.tests.tsee/apps/account-service/src/Account.tsee/packages/abac/src/can-access-object.spec.tsee/packages/abac/src/index.tsee/packages/abac/src/service.spec.tsee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/user-auto-removal.spec.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/omnichannel-services/src/OmnichannelTranscript.spec.tsee/packages/omnichannel-services/src/OmnichannelTranscript.tsee/packages/presence/src/Presence.tspackages/core-services/src/types/ISettingsService.tspackages/models/src/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/abac/src/user-auto-removal.spec.tsee/packages/omnichannel-services/src/OmnichannelTranscript.spec.tsapps/meteor/ee/server/local-services/instance/service.tsapps/meteor/server/services/settings/service.tsee/packages/presence/src/Presence.tspackages/core-services/src/types/ISettingsService.tsapps/meteor/server/services/nps/service.tsee/packages/abac/src/can-access-object.spec.tsapps/meteor/server/modules/notifications/notifications.module.tsee/packages/abac/src/subject-attributes-validations.spec.tsee/apps/account-service/src/Account.tsee/packages/abac/src/service.spec.tsee/packages/omnichannel-services/src/OmnichannelTranscript.tsapps/meteor/tests/unit/server/services/nps/spec.tests.tsee/packages/abac/src/index.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/server/services/authorization/canAccessRoom.tspackages/models/src/index.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/omnichannel-services/src/OmnichannelTranscript.spec.tsee/packages/abac/src/can-access-object.spec.tsee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/service.spec.ts
🧠 Learnings (26)
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/omnichannel-services/src/OmnichannelTranscript.spec.tsee/packages/abac/src/can-access-object.spec.tsee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/service.spec.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/abac/src/user-auto-removal.spec.tsapps/meteor/ee/server/local-services/instance/service.tsapps/meteor/server/services/settings/service.tsee/packages/presence/src/Presence.tsapps/meteor/server/modules/notifications/notifications.module.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/server/services/authorization/canAccessRoom.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/abac/src/can-access-object.spec.tsee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/index.tsapps/meteor/server/services/authorization/canAccessRoom.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
ee/packages/abac/src/user-auto-removal.spec.tsapps/meteor/server/modules/notifications/notifications.module.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:
ee/packages/abac/src/user-auto-removal.spec.tsee/packages/omnichannel-services/src/OmnichannelTranscript.spec.tsee/packages/abac/src/can-access-object.spec.tsee/packages/abac/src/subject-attributes-validations.spec.tsee/packages/abac/src/service.spec.tsapps/meteor/tests/unit/server/services/nps/spec.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:
ee/packages/abac/src/user-auto-removal.spec.tsapps/meteor/tests/unit/server/services/nps/spec.tests.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:
apps/meteor/ee/server/local-services/instance/service.tsapps/meteor/server/services/settings/service.tsee/packages/presence/src/Presence.tsapps/meteor/server/modules/notifications/notifications.module.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:
apps/meteor/ee/server/local-services/instance/service.tsee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/can-access-object.spec.tsee/packages/omnichannel-services/src/OmnichannelTranscript.tsapps/meteor/server/services/authorization/canAccessRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/abac/src/can-access-object.spec.tsapps/meteor/server/services/authorization/canAccessRoom.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
ee/packages/abac/src/can-access-object.spec.tsee/packages/abac/src/index.tsapps/meteor/server/services/authorization/canAccessRoom.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
ee/packages/abac/src/can-access-object.spec.tsee/packages/abac/src/index.tsapps/meteor/server/services/authorization/canAccessRoom.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
ee/packages/abac/src/can-access-object.spec.tsee/packages/abac/src/index.tsapps/meteor/server/services/authorization/canAccessRoom.ts
📚 Learning: 2025-11-17T22:38:48.631Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.631Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.
Applied to files:
apps/meteor/server/modules/notifications/notifications.module.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:
apps/meteor/server/modules/notifications/notifications.module.tsee/packages/abac/src/index.tsee/packages/federation-matrix/src/FederationMatrix.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/unit/server/services/nps/spec.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/nps/spec.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/nps/spec.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 : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/nps/spec.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/unit/server/services/nps/spec.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/unit/server/services/nps/spec.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/nps/spec.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 clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/nps/spec.tests.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
ee/packages/abac/src/index.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/server/services/authorization/canAccessRoom.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
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/server/services/authorization/canAccessRoom.ts
🧬 Code graph analysis (8)
apps/meteor/ee/server/local-services/instance/service.ts (1)
packages/models/src/index.ts (1)
Settings(188-188)
apps/meteor/server/services/settings/service.ts (2)
packages/core-typings/src/ISetting.ts (1)
SettingValue(11-20)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnSettingChangedById(359-367)
packages/core-services/src/types/ISettingsService.ts (1)
packages/core-typings/src/ISetting.ts (1)
SettingValue(11-20)
apps/meteor/server/services/nps/service.ts (1)
packages/models/src/index.ts (1)
Settings(188-188)
ee/apps/account-service/src/Account.ts (1)
packages/models/src/index.ts (1)
Settings(188-188)
apps/meteor/tests/unit/server/services/nps/spec.tests.ts (1)
apps/meteor/server/services/nps/service.ts (1)
NPSService(13-192)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/models/src/index.ts (1)
Settings(188-188)
apps/meteor/server/services/authorization/canAccessRoom.ts (2)
packages/models/src/index.ts (1)
Settings(188-188)packages/core-services/src/index.ts (1)
License(155-155)
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (17)
ee/apps/account-service/src/Account.ts (3)
1-1: LGTM — import migration is correct.
Settingsis now sourced from@rocket.chat/core-servicesalongsideServiceClass, consistent with the PR's goal of removing model-based access from services.
17-22: LGTM — reactive listener pattern is correct.
40-44: No issue —getLoginExpirationInDays()already handles undefined and invalid inputs defensively.The
getLoginExpirationInDays()function has explicit guards that validate the input and return the default value (90 days) if the argument isundefined,NaN, zero, negative, or non-numeric. The test suite confirms this behavior with cases likeexpect(getLoginExpirationInDays(undefined)).toBe(90). Passingexpirydirectly to the function is safe and will never silently corruptloginExpiration.The minor inconsistency with the constructor's
value as numbercast is a style detail, not a functional concern, since the function is robust to any input type.Likely an incorrect or invalid review comment.
ee/packages/presence/src/Presence.ts (2)
84-100: LGTM —Settings.set()instarted()andtoggleBroadcast()are correct.Both sites properly await the call; errors in
started()are intentionally caught, and errors intoggleBroadcast()propagate naturally to the caller.Also applies to: 127-137
2-2: ✅Settings.set()exists in@rocket.chat/core-serviceswith compatible signature.The method is properly exported and typed as
set<T extends SettingValue>(settingId: string, value: T): Promise<void>. TheSettingValuetype includesboolean, so calls likeSettings.set('Presence_broadcast_disabled', <bool>)are fully compatible.packages/models/src/index.ts (2)
186-188: LGTM —Settingsproxy intentionally retained for non-service consumers.
ISettingsModel(line 55) and theproxifyexport (line 188) are still needed by the main Rocket.Chat app context, which registersISettingsModelthrough its own model-registration path. The removal is correctly scoped toregisterServiceModelsonly.
211-244: No issue - Settings model is registered separately in the main Meteor app.
ISettingsModelis correctly removed fromregisterServiceModelsbecause that function is only used by separate microservice processes (ddp-streamer, presence-service, etc.) that do not need Settings access. The main Meteor app maintains independent model registration inapps/meteor/server/models.tsline 154, whereISettingsModelis still registered viaregisterModel('ISettingsModel', new SettingsRaw(db, trashCollection)). SettingsService runs in the main app context where the Settings proxy will resolve correctly.ee/packages/omnichannel-services/src/OmnichannelTranscript.spec.ts (1)
17-35: LGTM — mock surface correctly extended.The
onSettingChanged = jest.fn()addition toServiceClassaligns with the new lifecycle usage inOmnichannelTranscript, andSettings.getreturning''(falsy) is safe for any boolean-gated logic in the implementation.packages/core-services/src/types/ISettingsService.ts (1)
3-6: LGTM — clean interface addition.The
setsignature mirrorsget's generic constraint and is consistent withSettingValue.apps/meteor/server/services/nps/service.ts (1)
4-7: LGTM — migration to in-memory settings cache is clean.All three
Settings.get<boolean>call sites use!npsEnabledwhich correctly handlesfalse,undefined, andnull.apps/meteor/server/services/authorization/canAccessRoom.ts (1)
1-5: LGTM — import migration is clean.
Settings.get<boolean>is used correctly at both call sites; truthiness checks handle potentialundefinedreturns safely.ee/packages/federation-matrix/src/FederationMatrix.ts (1)
111-118: LGTM — lifecycle split is correct and consistent.
|| false/|| ''fallbacks properly guard againstundefinedreturns fromSettings.get, and splitting initial seeding intostarted()while keeping live-update listeners increated()is the correct pattern mirrored across the codebase.apps/meteor/tests/unit/server/services/nps/spec.tests.ts (1)
19-48: LGTM — clean migration fromSettings.getValueByIdtoSettings.getStub wiring, per-test
withArgsconfiguration, andbeforeEachreset are all consistent with the updatedNPSServiceimplementation.ee/packages/abac/src/subject-attributes-validations.spec.ts (1)
9-11: LGTM — minimal mock updateAdding
onSettingChanged = jest.fn()to theServiceClassmock is the right minimal change to prevent theAbacServiceconstructor from failing;Settingsdoes not need to be mocked here sinceaddSubjectAttributesnever reachesSettings.get.ee/packages/abac/src/user-auto-removal.spec.ts (1)
10-12: LGTM — same minimal mock fix as in other spec filesee/packages/abac/src/service.spec.ts (1)
62-73: LGTM — partial mock pattern is correctUsing
jest.requireActualto preserveMeteorErrorwhile overriding onlyServiceClassandRoomis the right approach for these unit tests, where error type assertions depend on the real implementation.ee/packages/abac/src/index.ts (1)
66-68: The type signature ofSettings.get<T>explicitly returnsPromise<T>, notPromise<T | undefined>. Rocket.Chat's settings system ensures every setting has a value—either the configured value or a fallback (per theAbac_Cache_Decision_Time_Secondsdefinition with default 300). The direct assignment instarted()is correct and safely follows the type contract. The guard inonSettingChangedis defensive programming beyond what the type system requires, not a necessary safeguard. There is no type violation or runtime risk in the code as written.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ee/packages/omnichannel-services/src/OmnichannelTranscript.ts`:
- Around line 83-103: The started() method assigns settings directly but
CachedSettings.get<T>() can return undefined; add null/undefined guards and
sensible fallbacks before assigning to fields like this.siteName,
this.dateFormat, this.timeAndDateFormat, this.serverLanguage,
this.reportingTimezone, this.defaultCustomTimezone and this.showSystemMessages
(e.g., default strings or enums) so downstream code (e.g., getTimezone() and
WorkerData) never receives undefined, and remove the TODO comment; ensure you
handle the 'custom' reportingTimezone path by only using
this.defaultCustomTimezone when it is a non-empty string.
---
Outside diff comments:
In `@ee/packages/presence/src/Presence.ts`:
- Line 54: The call to validateAvailability currently sets the in-memory flag
broadcastEnabled synchronously before awaiting Settings.set, which can leave the
service permanently out-of-sync if the Settings.set call rejects; update
validateAvailability (and the similar block around the current
broadcastEnabled/Settings.set usage) so that any Settings.set rejection is
handled: move the mutation of this.broadcastEnabled to after the await that
persists the setting, and ensure the promise returned by validateAvailability is
caught (either by adding a try/catch inside validateAvailability that logs
errors and reverts state on failure, or by keeping the callsite but adding
.catch(...) to the void this.validateAvailability() invocation and logging the
error). Reference validateAvailability, this.broadcastEnabled, and Settings.set
when making the change.
---
Nitpick comments:
In `@apps/meteor/ee/server/local-services/instance/service.ts`:
- Around line 187-188: The assignment to
this.troubleshootDisableInstanceBroadcast uses
Settings.get<boolean>('Troubleshoot_Disable_Instance_Broadcast') which can
resolve to undefined, leaving a boolean-typed field undefined; update the
assignment so it defaults to false (e.g. append "|| false") like the pattern
used in FederationMatrix where settings are normalized; locate the usage of
troubleshootDisableInstanceBroadcast and Settings.get in service.ts and apply
the same fallback to ensure type-safety and consistent behavior.
In `@apps/meteor/server/modules/notifications/notifications.module.ts`:
- Around line 202-204: Remove the stale TODO comment above the Settings.get call
in notifications.module.ts — the cache is already used via
Settings.get/CachedSettings, so delete the line "// TODO consider using
something to cache settings" and leave the existing logic that computes key =
(await Settings.get('UI_Use_Real_Name')) ? 'name' : 'username' intact (no other
code changes).
In `@apps/meteor/server/services/settings/service.ts`:
- Around line 17-22: The current set method calls notifyOnSettingChangedById
with "void", which swallows rejections and can leave the in-memory cache stale
after a successful Settings.updateValueById; change this to either await
notifyOnSettingChangedById(settingId) so callers observe failures, or keep
fire-and-forget but append a .catch that logs the error and rethrows or at least
reports it (use processLogger or existing logger) so DB/findOne failures from
notifyOnSettingChangedById are surfaced; update the async set<T extends
SettingValue>(settingId: string, value: T) method (and any callers) accordingly
to ensure cache-consistency and error visibility.
In `@ee/packages/abac/src/can-access-object.spec.ts`:
- Around line 159-161: The afterAll block that sets service.decisionCacheTimeout
= 300 is redundant because each test's outer beforeEach constructs a fresh
service (new AbacService()) with decisionCacheTimeout defaulting to 60; remove
the afterAll(...) teardown that mutates service.decisionCacheTimeout in the
"decision cache behavior" suite (or, if persistent teardown was actually
intended across tests, replace it with an afterEach that resets the same
instance), referencing the service variable and the decisionCacheTimeout
property to locate and remove or replace the block.
- Line 5: Rename the stale mock variable mockSettingsGetValueById to reflect
what it actually mocks (e.g., mockSettingsGet) and either remove the dead setup
mockSettingsGet.mockResolvedValue(300) from beforeEach or make the tests
exercise Settings.get by calling the AbacService.started() method (or otherwise
invoking the code path that calls Settings.get) so the mock is meaningful;
update any references to the old name (the jest.fn() declaration and where it is
passed to Settings.get) and remove unused mock setup to eliminate misleading
dead code.
In `@ee/packages/omnichannel-services/src/OmnichannelTranscript.ts`:
- Line 84: Remove the inline TODO comment and implement a mem-based cache for
the settings retrieval used in the started() block of OmnichannelTranscript:
wrap the function that fetches the settings (the code inside the started()
settings block) with the mem cache library so repeated calls return cached
settings, configure an appropriate TTL and keying strategy, and replace the
direct call in started() to use the cached wrapper; if you prefer not to
implement it now, remove the TODO and create a tracked issue instead mentioning
OmnichannelTranscript.started and the need for a mem cache.
KevLehman
added
the
stat: QA assured
dionisio-bot
bot
removed
the
stat: QA assured
dionisio-bot
bot
added
stat: QA assured
dionisio-bot
bot
added
stat: ready to merge
dionisio-bot
bot
added
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
This also removes the
Settingsmodel from the list of available models on services.Task: CORE-1849
Summary by CodeRabbit