chore: refactor listeners.js to typescript #37985
Conversation
|
|
Looks like this PR is ready to merge! 🎉 |
WalkthroughReplaces bridge-based listener calls with a unified Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Server as Rocket.Chat Server
participant AppsSelf as Apps.self
participant Orchestrator as App Orchestrator
participant Converters as Converters
participant ListenerMgr as AppListenerManager
Client->>Server: action (send/update/delete room/message)
Server->>AppsSelf: Apps.self?.triggerEvent(AppEvents.X, payload)
AppsSelf->>Orchestrator: handleEvent({ event, payload })
Orchestrator->>Converters: convert domain -> app payload
Orchestrator->>ListenerMgr: execute listener
ListenerMgr-->>Orchestrator: result (prevent/modify/ok)
Orchestrator-->>AppsSelf: return result
AppsSelf-->>Server: propagate result
alt prevented
Server->>Client: abort / throw
else allowed/modified
Server->>Server: continue (persist, notify)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37985 +/- ##
===========================================
+ Coverage 70.68% 70.70% +0.02%
===========================================
Files 3148 3147 -1
Lines 108850 108850
Branches 19547 19607 +60
===========================================
+ Hits 76937 76961 +24
+ Misses 29916 29886 -30
- Partials 1997 2003 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 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/18 22:53", "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", "01/14 14:33 (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.00]
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.00]
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.00]
line "omnichannel-transcript-service" [0.14, 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.00]
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.00]
line "queue-worker-service" [0.14, 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.00]
line "rocketchat" [0.35, 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.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 14 Jan 2026 14:33:57 GMT |
d-gubert
force-pushed
the
chore/convert-app-listeners-ts
branch
4 times, most recently
from
c7fad72 to
b7a9aca
Compare
![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: 5
🧹 Nitpick comments (2)
apps/meteor/app/livechat/server/lib/departmentsLib.ts (2)
140-143: Consider addingisLoaded()guard for consistency.Other files (e.g.,
eraseRoom.ts,sendMessage.ts) guardtriggerEventcalls withApps.self?.isLoaded(). While optional chaining handles the null case, theisLoaded()check ensures apps are actually loaded before triggering events, which may have performance implications.🔎 Proposed fix
// Disable event if (department?.enabled && !departmentDB?.enabled) { await callbacks.run('livechat.afterDepartmentDisabled', departmentDB); - void Apps.self?.triggerEvent(AppEvents.IPostLivechatDepartmentDisabled, { department: departmentDB }); + if (Apps.self?.isLoaded()) { + void Apps.self?.triggerEvent(AppEvents.IPostLivechatDepartmentDisabled, { department: departmentDB }); + } }
271-273: Same consistency concern as above.Consider adding an
isLoaded()guard here as well for consistency with other event dispatch sites.🔎 Proposed fix
await callbacks.run('livechat.afterRemoveDepartment', { department, agentsIds: removedAgents.map(({ agentId }) => agentId) }); - void Apps.self?.triggerEvent(AppEvents.IPostLivechatDepartmentRemoved, { department }); + if (Apps.self?.isLoaded()) { + void Apps.self?.triggerEvent(AppEvents.IPostLivechatDepartmentRemoved, { department }); + } return ret;
📜 Review details
Configuration used: Organization 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 (14)
apps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.jsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/livechat/server/lib/departmentsLib.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/server/services/messages/service.tspackages/apps-engine/src/server/managers/AppListenerManager.ts
💤 Files with no reviewable changes (1)
- apps/meteor/app/apps/server/bridges/listeners.js
🧰 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:
packages/apps-engine/src/server/managers/AppListenerManager.tsapps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/livechat/server/lib/departmentsLib.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/server/services/messages/service.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/app/lib/server/functions/sendMessage.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:
apps/meteor/app/api/server/lib/eraseTeam.spec.ts
🧠 Learnings (21)
📚 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:
apps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/server/lib/eraseRoom.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:
apps/meteor/server/lib/eraseRoom.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/api/server/lib/eraseTeam.spec.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:
apps/meteor/server/lib/eraseRoom.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/api/server/lib/eraseTeam.spec.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/lib/eraseRoom.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/livechat/server/lib/RoutingManager.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/lib/eraseRoom.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/lib/server/functions/sendMessage.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:
apps/meteor/server/lib/eraseRoom.ts
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
apps/meteor/server/lib/eraseRoom.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/server/lib/eraseRoom.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
apps/meteor/app/lib/server/functions/updateMessage.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:
apps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/server/services/messages/service.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/server/services/messages/service.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/lib/server/functions/sendMessage.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.ts
🧬 Code graph analysis (2)
apps/meteor/app/lib/server/functions/updateMessage.ts (1)
packages/core-services/src/index.ts (1)
Apps(152-152)
apps/meteor/app/lib/server/functions/sendMessage.ts (2)
packages/core-services/src/index.ts (1)
Apps(152-152)apps/meteor/app/apps/server/bridges/listeners.ts (1)
messageEvent(107-171)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/listeners.ts
[error] 115-115: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 121-121: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 129-129: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 136-136: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 143-143: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 150-150: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 182-182: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 190-190: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 222-222: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 314-317: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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 (21)
apps/meteor/app/livechat/server/lib/RoutingManager.ts (1)
289-289: LGTM! Event dispatch migration follows the new pattern.The migration from bridge-based event invocation to
Apps.self?.triggerEventis correct, maintaining the fire-and-forget behavior with proper optional chaining.apps/meteor/app/api/server/lib/eraseTeam.ts (2)
78-82: LGTM! Prevention event dispatch correctly migrated.The prevention pattern is properly implemented with optional chaining and correct control flow.
91-93: LGTM! Post-deletion event dispatch migrated correctly.Fire-and-forget pattern with optional chaining is appropriate for post-action events.
apps/meteor/app/lib/server/functions/deleteMessage.ts (2)
40-43: LGTM! Prevention event dispatch migrated correctly.The prevention pattern with error throwing on
preventis correctly implemented.
103-105: TheIPostMessageDeletedevent is correctly invoked with(deletedMsg, user). ThetriggerEventAPI accepts variadic arguments (...payload: any[]), and the listener bridge adapter inapps/meteor/app/apps/server/bridges/listeners.tsproperly transforms these two arguments into the expectedIMessageDeleteContextstructure. No changes needed.apps/meteor/ee/server/apps/communication/uikit.ts (5)
1-1: LGTM! Type abstraction improvement.Using
IAppServerOrchestratorinterface instead of concrete class improves type safety and testability.Also applies to: 196-199
216-217: LGTM! Conditional converter invocation is correct.The ternary operators correctly prevent converter calls when IDs are absent, avoiding unnecessary operations.
233-236: LGTM! Event interface selection logic is correct.The conditional selection between
IUIKitInteractionHandlerandIUIKitLivechatInteractionHandlerbased on visitor presence is appropriate.
311-312: LGTM! Consistent conditional converter pattern.The conditional converter invocation matches the pattern used elsewhere in the file.
330-330: LGTM! Event dispatch uses AppEvents enum correctly.Consistent with the pattern used throughout the file.
packages/apps-engine/src/server/managers/AppListenerManager.ts (1)
35-35: LGTM! Interface export enables bridge refactoring.Making
IListenerExecutorpublic allows the new TypeScript bridge implementation to properly type event handlers.apps/meteor/app/livechat/server/lib/closeRoom.ts (1)
98-99: LGTM! Livechat room closed events migrated correctly.Both the deprecated and current event dispatches use the new triggerEvent pattern correctly, with the deprecation notice properly preserved.
apps/meteor/app/lib/server/functions/updateMessage.ts (3)
28-31: LGTM! Prevention event dispatch migrated correctly.The prevention pattern with error throwing is properly implemented.
33-38: LGTM! Sequential event processing is correct.The extend-then-modify pattern correctly chains the events, with proper type checking before assignment.
88-92: LGTM! Post-update event dispatch migrated correctly.Fire-and-forget pattern with optional chaining is appropriate.
apps/meteor/server/services/messages/service.ts (1)
1-1: LGTM! System message event dispatch migrated correctly.The import addition and triggerEvent usage follow the established pattern throughout this refactor.
Also applies to: 198-200
apps/meteor/app/api/server/lib/eraseTeam.spec.ts (1)
43-46: LGTM!The test stubs are correctly updated to mock the new
Apps.self.triggerEventAPI. The pattern is consistent across all stub configurations:
isLoadedreturns a boolean via function calltriggerEventis a sinon stub that returns appropriate values for each test scenarioAlso applies to: 195-198, 247-250
apps/meteor/server/lib/eraseRoom.ts (1)
43-48: LGTM!The event dispatch migration is correctly implemented:
- Pre-delete prevention properly awaits the result and checks for truthy value to throw
- Post-delete notification uses
voidfor fire-and-forget semantics- Both calls are guarded by
Apps.self?.isLoaded()checkAlso applies to: 59-61
apps/meteor/app/lib/server/functions/sendMessage.ts (2)
233-251: LGTM!The pre-send event flow is correctly implemented:
- Prevent check properly short-circuits on truthy result
- Extend/Modify chain correctly nests the calls so extend runs first, then modify receives the extended message
- Result validation with
validateMessageafter app modifications prevents malicious changes
282-286: LGTM!Post-send event dispatch correctly distinguishes between system messages (
IPostSystemMessageSent) and regular messages (IPostMessageSent) using the message type fieldt. Fire-and-forget withvoidis appropriate here.apps/meteor/app/apps/server/bridges/listeners.ts (1)
32-33: LGTM on class structure and type definitions.The
AppListenerBridgeclass is well-structured with clear separation of concerns. The type definitions forLivechatTransferData,LivechatAgentData,UserStatusChangedData, andUserCrudDataare appropriate for the internal payload shapes.
d-gubert
force-pushed
the
chore/convert-app-listeners-ts
branch
from
b7a9aca to
6a04195
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @apps/meteor/app/apps/server/bridges/listeners.ts:
- Around line 313-320: The default case in the switch needs a block to avoid TDZ
for the const declaration; wrap the default branch body in braces and move the
const room declaration and subsequent await/return inside that block so the code
uses block scoping (i.e., change the default: handling around
this.orch.getConverters().get('rooms').convertRoom(...) and the return
this.orch.getManager().getListenerManager().executeListener(inte, room) to be
enclosed in { ... }).
- Around line 323-365: The userEvent method lacks an explicit return for
unexpected interface values; update the switch in userEvent to include a
default/fallback that explicitly returns (or throws) a value so the
Promise<unknown> is deterministic—e.g., add a default case after the existing
AppInterface cases that either returns Promise.resolve(undefined) or throws an
Error including the unexpected inte value; reference the userEvent function and
AppInterface enum to locate and modify the switch and ensure the method always
returns.
- Around line 219-230: The switch case for
AppInterface.IPostLivechatAgentAssigned / IPostLivechatAgentUnassigned declares
const agentData inside the case which is not block-scoped; wrap the case body in
braces to create a new block so const agentData is properly scoped. Locate the
switch handling these cases (the case labels
AppInterface.IPostLivechatAgentAssigned and
AppInterface.IPostLivechatAgentUnassigned) and enclose the logic that computes
agentData and calls
this.orch.getManager().getListenerManager().executeListener(...) in { ... } to
prevent TDZ/hoisting issues and ensure converters
(this.orch.getConverters().get('rooms') / get('users')) are used inside the new
block.
- Around line 178-199: The switch inside the params IIFE declares consts in
multiple case clauses which aren’t block-scoped; wrap the bodies of the cases
for AppInterface.IPreRoomUserJoined/AppInterface.IPostRoomUserJoined (the
joiningUser and invitingUser declarations) and
AppInterface.IPreRoomUserLeave/AppInterface.IPostRoomUserLeave (the leavingUser
and removedBy declarations) in braces so each case has its own block scope and
avoids TDZ errors; keep the returned objects the same and only add { ... }
around those case bodies.
- Around line 112-159: The switch inside the params IIFE creates block-scoped
consts in each case which can leak/trigger TDZ; to fix, wrap each case body in
its own brace block (e.g., case AppInterface.IPostMessageDeleted: { const
[userDeleted] = ...; return {...}; } ), doing the same for IPostMessageReacted,
IPostMessageFollowed, IPostMessagePinned, IPostMessageStarred and
IPostMessageReported so each const is confined to its case block and the default
return stays unchanged.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/sendMessage.ts (1)
234-243: The nested triggerEvent pattern is already safe due to the guard clause.The code at lines 240-243 is wrapped by the
if (Apps.self?.isLoaded())check at line 233, which ensuresApps.selfis available throughout the entire block. Additionally,Apps.selfis never reassigned in the codebase, making the optional chaining pattern consistent and safe. No race conditions exist between the inner and outer calls.If readability is a concern, the nested pattern could optionally be refactored to extract the extended message first, but this is not necessary for correctness.
📜 Review details
Configuration used: Organization 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 (14)
apps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.jsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/livechat/server/lib/departmentsLib.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/server/services/messages/service.tspackages/apps-engine/src/server/managers/AppListenerManager.ts
💤 Files with no reviewable changes (1)
- apps/meteor/app/apps/server/bridges/listeners.js
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/meteor/app/api/server/lib/eraseTeam.spec.ts
- apps/meteor/app/livechat/server/lib/RoutingManager.ts
- apps/meteor/server/services/messages/service.ts
- packages/apps-engine/src/server/managers/AppListenerManager.ts
- apps/meteor/app/livechat/server/lib/departmentsLib.ts
- apps/meteor/app/lib/server/functions/updateMessage.ts
- apps/meteor/server/lib/eraseRoom.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:
apps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/lib/server/functions/sendMessage.ts
🧠 Learnings (8)
📚 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:
apps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.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/app/livechat/server/lib/closeRoom.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.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:
apps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.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/app/livechat/server/lib/closeRoom.tsapps/meteor/app/lib/server/functions/sendMessage.ts
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.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:
apps/meteor/app/api/server/lib/eraseTeam.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:
apps/meteor/app/lib/server/functions/deleteMessage.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/server/functions/sendMessage.ts
🧬 Code graph analysis (2)
apps/meteor/app/apps/server/bridges/listeners.ts (2)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)packages/apps-engine/src/server/managers/AppListenerManager.ts (1)
IListenerExecutor(35-241)
apps/meteor/app/lib/server/functions/sendMessage.ts (2)
packages/core-services/src/index.ts (1)
Apps(152-152)apps/meteor/app/apps/server/bridges/listeners.ts (1)
messageEvent(107-171)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/listeners.ts
[error] 115-115: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 121-121: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 129-129: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 136-136: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 143-143: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 150-150: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 182-182: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 190-190: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 222-222: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 314-317: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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 (17)
apps/meteor/app/api/server/lib/eraseTeam.ts (1)
77-82: LGTM! Clean migration to direct event triggering.The refactor from bridge-based dispatch to
Apps.self?.triggerEvent()maintains the existing control flow correctly:
- Line 78: Prevention event properly awaited with early return on prevent
- Line 92: Post-deletion event appropriately fire-and-forget with
void- Both guarded by
isLoaded()checksAlso applies to: 91-93
apps/meteor/app/livechat/server/lib/closeRoom.ts (1)
93-100: LGTM! Event migration preserves deferred execution pattern.The refactor correctly:
- Maintains
process.nextTickdeferral to avoid blocking the transaction- Uses
voidfor fire-and-forget event dispatch- Keeps the deprecated event for backwards compatibility as noted in the comment
- Aligns with the broader Apps framework migration
apps/meteor/ee/server/apps/communication/uikit.ts (3)
196-198: LGTM! Interface-based dependency improves testability.Changing from
AppServerOrchestratorconcrete class toIAppServerOrchestratorinterface follows dependency inversion principle and makes the class easier to test with mocks.
216-217: LGTM! Conditional conversions prevent unnecessary lookups.Making message and room conversions conditional on ID presence is a solid improvement:
- Avoids unnecessary database/converter calls when IDs are absent
- Returns explicit
undefinedfor cleaner downstream handling- Pattern is consistent across all interaction types
Also applies to: 311-312
233-233: LGTM! AppEvents enum constants improve type safety.Replacing string literals with
AppEventsenum values provides:
- Compile-time validation of event names
- Better IDE support with autocomplete
- Prevents typos and invalid event references
- Line 233 correctly selects handler based on visitor presence
Also applies to: 264-264, 289-289, 330-330
apps/meteor/app/lib/server/functions/deleteMessage.ts (2)
40-43: LGTM! Pre-delete prevention correctly migrated to Apps.self?.triggerEvent.The migration from bridge-based event to
Apps.self?.triggerEvent(AppEvents.IPreMessageDeletePrevent, deletedMsg)is correct. The optional chaining ensures graceful handling when Apps is not loaded, and the prevention logic properly throws an error when an app blocks the deletion.
103-105: LGTM! Post-delete event properly dispatched.The post-delete notification correctly uses
Apps.self?.triggerEvent(AppEvents.IPostMessageDeleted, deletedMsg, user)and is properly guarded by thedeletedMsgexistence check.apps/meteor/app/lib/server/functions/sendMessage.ts (2)
1-1: LGTM! Import updated correctly.The addition of
AppEventsandAppsimports from@rocket.chat/appsaligns with the TypeScript refactoring objective.
284-285: LGTM! Post-message event dispatch correctly implemented.The conditional selection between
AppEvents.IPostSystemMessageSentandAppEvents.IPostMessageSentbased onmessage.tcorrectly distinguishes system messages from regular messages, and thevoidoperator appropriately indicates fire-and-forget semantics.apps/meteor/app/apps/server/bridges/listeners.ts (8)
1-6: LGTM!The imports are properly structured with type-only imports where appropriate, following TypeScript best practices.
8-30: LGTM!The internal type definitions are clear, properly typed, and appropriately scoped for this module.
32-33: LGTM!Constructor properly initializes the orchestrator as a private readonly field.
35-96: LGTM!The event routing logic comprehensively maps all AppInterface events to their respective handlers. The type assertion on line 95 is necessary and well-documented.
98-105: LGTM!The default event handler appropriately delegates payload validation to the listener manager.
231-256: LGTM!The room transfer event handling properly validates required entities and throws descriptive errors. The type assertion on line 252 is justified by the comment about incorrect type definitions in apps-engine.
258-285: LGTM!The guest and room saved event handlers properly validate entities and throw descriptive errors when not found.
287-311: LGTM!The department event handlers properly validate entities and provide appropriate error handling.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @apps/meteor/app/apps/server/bridges/listeners.ts:
- Around line 401-421: The error strings interpolate the departmentData object
directly (in the IPostLivechatDepartmentDisabled and
IPostLivechatDepartmentRemoved cases), causing “[object Object]” output; update
the thrown errors in these branches (the lines that throw new Error(`Department
${departmentData} not found`)) to include a concrete identifier such as
departmentData._id or departmentData.id and/or departmentData.name (e.g.,
`Department ${departmentData._id} not found`) or use
JSON.stringify(departmentData) if you need full context, so the message is
human-readable and debuggable.
- Around line 427-433: The error messages use roomData (an IRoom object) which
stringifies to [object Object]; update the throw statements in this block so
they reference roomData._id (or another identifying field like roomData.rid)
instead of roomData, e.g., use `Room ${roomData._id} not found` and `Room
${roomData._id} is not a livechat room` while keeping the existing checks around
isLivechatRoom(room).
- Around line 300-327: The switch in the anonymous async block improperly
declares consts directly in case clauses (Biome lint error) and the default
branch bypasses executeListener so room create/delete events never run; fix by
wrapping each case body that declares locals in braces (e.g., case
AppInterface.IPreRoomUserJoined: { const [, joiningUser, invitingUser] =
args.payload; ... } ) and replace the default return rm with a call to
this.orch.getManager().getListenerManager().executeListener(args.event, { room:
rm, ...(payload-dependent fields) }) or explicitly add cases for room
create/delete that call executeListener so all events (including
IPreRoomCreatePrevent, IPostRoomCreate, etc.) invoke the listener.
- Around line 220-293: The switch cases use unscoped const bindings and the
default branch bypasses listeners; wrap each case body in braces to give the
const declarations block scope (e.g., the
AppInterface.IPostMessageDeleted/IPostMessageReacted/... cases where you declare
userDeleted/userReacted/etc.), and change the default branch to invoke
this.orch.getManager().getListenerManager().executeListener(args.event, {
message: msg }) (or pass converted payload as needed) instead of returning msg
so events not explicitly listed still run their listeners; keep the post-execute
handling (checking for boolean/undefined and converting via
this.orch.getConverters().get('messages').convertAppMessage) unchanged.
🧹 Nitpick comments (3)
apps/meteor/app/apps/server/bridges/listeners.ts (3)
72-94: Type overlap inHandleRoomEventunion may cause confusion.The events
IPreRoomUserJoined,IPostRoomUserJoined,IPreRoomUserLeave, andIPostRoomUserLeaveappear in both union branches (lines 74, 78 with[IRoom, IUser, IUser]payload, and lines 89-92 with[IRoom]payload). This overlap means the discriminated union won't narrow properly for these events—TypeScript cannot infer which payload shape applies.If the intent is to support both payload shapes for these events, consider making this explicit with separate type guards. Otherwise, remove the duplicates from lines 89-92.
♻️ Suggested fix: remove duplicates from the second branch
type HandleRoomEvent = | { event: AppInterface.IPreRoomUserJoined | AppInterface.IPostRoomUserJoined; payload: [IRoom, IUser, IUser]; } | { event: AppInterface.IPreRoomUserLeave | AppInterface.IPostRoomUserLeave; payload: [IRoom, IUser, IUser]; } | { event: | AppInterface.IPreRoomCreatePrevent | AppInterface.IPreRoomCreateExtend | AppInterface.IPreRoomCreateModify | AppInterface.IPostRoomCreate | AppInterface.IPreRoomDeletePrevent - | AppInterface.IPostRoomDeleted - | AppInterface.IPreRoomUserJoined - | AppInterface.IPostRoomUserJoined - | AppInterface.IPreRoomUserLeave - | AppInterface.IPostRoomUserLeave; + | AppInterface.IPostRoomDeleted; payload: [IRoom]; };
336-348: Add block scope around the switch case declaration.Line 340 declares
const [agentData]without block scope (Biome error). Wrap in braces for proper scoping.♻️ Suggested fix
switch (args.event) { case AppInterface.IPostLivechatAgentAssigned: - case AppInterface.IPostLivechatAgentUnassigned: - const [agentData] = args.payload; + case AppInterface.IPostLivechatAgentUnassigned: { + const [agentData] = args.payload; return this.orch .getManager() .getListenerManager() .executeListener(args.event, { room: (await this.orch.getConverters().get('rooms').convertRoom(agentData.room)) as IAppsLivechatRoom, agent: this.orch.getConverters().get('users').convertToApp(agentData.user), }); + }
423-436: Add block scope around the default case declarations.Lines 424-425 declare
constvariables without block scope (Biome error).♻️ Suggested fix
- default: - const [roomData] = args.payload; - const room = await this.orch.getConverters().get('rooms').convertRoom(roomData); + default: { + const [roomData] = args.payload; + const room = await this.orch.getConverters().get('rooms').convertRoom(roomData); if (!room) { throw new Error(`Room ${roomData} is not a livechat room`); } if (!isLivechatRoom(room)) { throw new Error(`Room ${roomData} is not a livechat room`); } return this.orch.getManager().getListenerManager().executeListener(args.event, room); + }
📜 Review details
Configuration used: Organization 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/app/apps/server/bridges/listeners.tsapps/meteor/ee/server/apps/orchestrator.js
🧰 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:
apps/meteor/ee/server/apps/orchestrator.jsapps/meteor/app/apps/server/bridges/listeners.ts
🧠 Learnings (4)
📚 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:
apps/meteor/app/apps/server/bridges/listeners.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:
apps/meteor/app/apps/server/bridges/listeners.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/app/apps/server/bridges/listeners.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/app/apps/server/bridges/listeners.ts
🧬 Code graph analysis (1)
apps/meteor/app/apps/server/bridges/listeners.ts (2)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)packages/apps-engine/src/definition/rooms/index.ts (6)
IPreRoomCreatePrevent(19-19)IPreRoomCreateExtend(17-17)IPreRoomCreateModify(18-18)IPostRoomCreate(15-15)IPreRoomDeletePrevent(20-20)IPostRoomDeleted(16-16)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/listeners.ts
[error] 223-223: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 232-232: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 243-243: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 253-253: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 263-263: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 273-273: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 304-304: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 315-315: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 340-340: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 424-424: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 425-425: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
apps/meteor/ee/server/apps/orchestrator.js (1)
340-355: Interface change from spread arguments to structured object is correctly implemented.The call to
handleEventnow passes a single object{ event, payload }instead of spreadingevent, ...payloadas separate arguments. This matches the newAppListenerBridge.handleEventsignature which expects aHandleEventparameter—a union type where each variant has botheventandpayloadproperties. The change aligns with the PR's goal of introducing structured payloads and is properly typed.apps/meteor/app/apps/server/bridges/listeners.ts (3)
1-31: LGTM!Type definitions are well-structured with clear field types. The internal type aliases appropriately capture the event data structures needed for the bridge.
146-206: LGTM!The event routing in
handleEventis well-organized with clear categorization of events to their respective handlers. The deprecation comment forILivechatRoomClosedHandleris a good practice.
439-479: LGTM!The
userEventmethod correctly uses block scoping for all cases and properly constructs the context objects. The handling of optionalperformedByandpreviousUserfields aligns with the type definitions.
d-gubert
force-pushed
the
chore/convert-app-listeners-ts
branch
from
716e687 to
3b4a84c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @apps/meteor/app/apps/server/bridges/listeners.ts:
- Around line 401-421: The error messages in the IPostLivechatDepartmentDisabled
and IPostLivechatDepartmentRemoved cases interpolate the departmentData object
directly, producing "[object Object]"; change the throw to include a meaningful
identifier (e.g., departmentData._id or departmentData.id or
departmentData.name) or fallback to JSON.stringify(departmentData) so the
message is informative; update the throw lines in the two cases (within
listeners.ts where convertDepartment is called) to something like: throw new
Error(`Department ${departmentData._id || departmentData.id ||
departmentData.name || JSON.stringify(departmentData)} not found`).
- Around line 220-285: The switch on args.event (cases like
AppInterface.IPostMessageDeleted, IPostMessageReacted, IPostMessageFollowed,
IPostMessagePinned, IPostMessageStarred, IPostMessageReported) declares consts
(userDeleted, userReacted, userFollowed, userPinned, userStarred, userReported,
etc.) that can leak across cases; wrap each case body in its own block ({ ... })
so those const declarations are block-scoped and cannot be accessed by other
cases, keeping the existing return/executeListener logic unchanged.
- Around line 300-327: The switch cases in the anonymous async IIFE that assigns
result declare consts (e.g., "const [, joiningUser, invitingUser]" and "const [,
leavingUser, removedBy]") directly in case clauses which creates scope issues;
fix by wrapping each case body in braces so those const declarations are
block-scoped (add `{ ... }` around the code for the
IPreRoomUserJoined/IPostRoomUserJoined block and likewise for the
IPreRoomUserLeave/IPostRoomUserLeave block), keeping the existing calls to
this.orch.getManager().getListenerManager().executeListener(...) unchanged.
- Around line 336-348: The switch case for
AppInterface.IPostLivechatAgentAssigned /
AppInterface.IPostLivechatAgentUnassigned in the livechatEvent method declares a
const (const [agentData]) without a block scope; wrap the case body in braces to
create a block scope around that declaration and the subsequent return so the
const is properly scoped (i.e., add { ... } around the code that currently
follows those two case labels inside livechatEvent).
- Around line 423-436: In the switch default case in listeners.ts, wrap the
const declarations for roomData and room in a block (e.g., { ... }) to provide
proper block scoping; then change the thrown Error messages to include a unique
identifier from roomData (such as roomData._id or roomData.id) instead of
interpolating the whole object, and update the checks around convertRoom
(getConverters().get('rooms').convertRoom) and isLivechatRoom to use those
block-scoped variables before calling
getListenerManager().executeListener(args.event, room).
📜 Review details
Configuration used: Organization 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 (14)
apps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.jsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/livechat/server/lib/departmentsLib.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/server/lib/eraseRoom.tsapps/meteor/server/services/messages/service.ts
💤 Files with no reviewable changes (1)
- apps/meteor/app/apps/server/bridges/listeners.js
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/meteor/app/livechat/server/lib/departmentsLib.ts
- apps/meteor/ee/server/apps/orchestrator.js
- apps/meteor/app/livechat/server/lib/RoutingManager.ts
- apps/meteor/app/lib/server/functions/updateMessage.ts
- apps/meteor/app/lib/server/functions/deleteMessage.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:
apps/meteor/server/services/messages/service.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/app/livechat/server/lib/closeRoom.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:
apps/meteor/app/api/server/lib/eraseTeam.spec.ts
🧠 Learnings (21)
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/server/services/messages/service.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/apps/server/bridges/listeners.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:
apps/meteor/server/services/messages/service.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:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/livechat/server/lib/closeRoom.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:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/livechat/server/lib/closeRoom.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/app/api/server/lib/eraseTeam.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/livechat/server/lib/closeRoom.ts
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/server/lib/eraseRoom.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:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.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:
apps/meteor/server/lib/eraseRoom.tsapps/meteor/app/apps/server/bridges/listeners.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/server/lib/eraseRoom.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/app/lib/server/functions/sendMessage.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/livechat/server/lib/closeRoom.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/app/apps/server/bridges/listeners.tsapps/meteor/app/api/server/lib/eraseTeam.spec.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/app/apps/server/bridges/listeners.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.ts
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/functions/sendMessage.ts (2)
packages/core-services/src/index.ts (1)
Apps(152-152)apps/meteor/app/apps/server/bridges/listeners.ts (1)
messageEvent(215-293)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/listeners.ts
[error] 223-223: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 232-232: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 243-243: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 253-253: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 263-263: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 273-273: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 304-304: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 315-315: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 340-340: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 424-424: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 425-425: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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 (21)
apps/meteor/server/services/messages/service.ts (2)
1-1: LGTM!Import correctly brings in
AppEventsenum for type-safe event dispatch.
204-206: LGTM!Clean migration from bridge-based listener call to the unified
Apps.self?.triggerEventpattern with typedAppEventsenum. The guard check and fire-and-forget pattern are appropriate here.apps/meteor/server/lib/eraseRoom.ts (2)
43-48: LGTM!The migration from
Apps.getBridges()?.getListenerBridge().roomEvent()toApps.self?.triggerEvent()is correctly implemented. TheisLoaded()guard ensures the Apps engine is ready before triggering events.
59-61: LGTM!The post-delete event is correctly fired as fire-and-forget using
void. This maintains the original behavior where the deletion completes regardless of whether apps process the event.apps/meteor/ee/server/apps/communication/uikit.ts (5)
1-1: LGTM!The import correctly brings in
AppEventsfor enum-based event references andIAppServerOrchestratorfor interface-based typing.
196-199: LGTM!Using the
IAppServerOrchestratorinterface type instead of the concrete class improves flexibility and follows dependency inversion principles.
216-217: LGTM!The conditional conversion pattern (
mid ? await ... : undefined) is cleaner and explicitly handles the case when IDs are not present, avoiding potential undefined access issues.
233-235: LGTM!Using
AppEvents.IUIKitInteractionHandlerandAppEvents.IUIKitLivechatInteractionHandlerenum values instead of string literals improves type safety and enables compile-time checking.
311-312: LGTM!Consistent application of the conditional conversion pattern for
roomandmessagein theactionButtoncase.apps/meteor/app/lib/server/functions/sendMessage.ts (2)
233-251: LGTM!The migration to
Apps.self?.triggerEventfor pre-message events is well implemented:
- Prevention check correctly returns early when apps prevent sending
- The extend → modify chain preserves the original event flow
- Re-validation after app modifications is a good defensive practice
282-286: LGTM!The post-send event dispatch correctly differentiates between system messages (
IPostSystemMessageSent) and regular messages (IPostMessageSent) using themessage.tproperty. The fire-and-forget pattern usingvoidis appropriate here.apps/meteor/app/api/server/lib/eraseTeam.ts (2)
77-82: LGTM!The pre-delete prevention check is correctly implemented using
Apps.self?.triggerEvent. The return offalse(rather than throwing) aligns with the "loose validation" semantics of this function.
91-93: LGTM!Post-delete event dispatch is consistent with the pattern in
eraseRoom.ts, using fire-and-forget semantics.apps/meteor/app/api/server/lib/eraseTeam.spec.ts (3)
43-46: LGTM!The default Apps stub correctly models the new interface with
Apps.self.isLoaded()returningfalseand a stubbedtriggerEvent. This ensures tests run without app interference by default.
190-219: LGTM!The test correctly verifies that when
triggerEventreturnstrueforIPreRoomDeletePrevent, the deletion is prevented andfalseis returned. The stub structure properly reflects the newApps.self.triggerEventAPI.
242-272: LGTM!The test properly verifies the success path:
- First
triggerEventcall (pre-delete) returnsfalse(no prevention)- Second
triggerEventcall (post-delete) firesroomEventStub.calledTwiceassertion confirms both events were triggeredapps/meteor/app/livechat/server/lib/closeRoom.ts (1)
93-100: LGTM!The migration from the bridge-based
livechatEventdispatch to the unifiedApps.self?.triggerEventAPI is clean and consistent with the broader refactoring pattern across the codebase. The deprecation comment is properly retained, and thevoidkeyword appropriately handles the floating promises.apps/meteor/app/apps/server/bridges/listeners.ts (4)
1-144: LGTM!The type definitions are well-structured using discriminated union types that provide type safety for event handling. The
HandleEventunion and its constituent types clearly define the expected payload shapes for each event category.
146-206: LGTM!The
handleEventmethod correctly routes events to their specialized handlers. The complexity exemption is justified given the exhaustive switch statement for event routing.
208-213: LGTM!The
defaultEventmethod serves as an appropriate fallback for events that don't require specialized handling.
439-479: LGTM!The
userEventmethod properly uses block scoping for all switch cases. The event handling for login/logout, status changes, and CRUD operations is well-structured with appropriate context construction.
ggazzo
force-pushed
the
chore/convert-app-listeners-ts
branch
from
3b4a84c to
4915155
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @apps/meteor/app/apps/server/bridges/listeners.ts:
- Around line 72-94: The discriminated union HandleRoomEvent currently maps the
same event discriminants (IPreRoomUserJoined, IPostRoomUserJoined,
IPreRoomUserLeave, IPostRoomUserLeave) to two different payload shapes which
breaks type narrowing; update the union so each event appears only once with its
correct payload by removing those four duplicate event types from the third
union member (the member with payload: [IRoom]) and keep them only in the
appropriate union member with payload: [IRoom, IUser, IUser], ensuring each
event name in HandleRoomEvent uniquely corresponds to one payload shape.
- Around line 149-151: Remove the debug console.log in the async
handleEvent(args: HandleEvent): Promise<any> method; delete the line
"console.log('args', args)" (or replace it with a proper server logger call if
intentional) so that production server logs are not polluted, ensuring any
required event details are logged via the existing logging mechanism instead of
console.log.
🧹 Nitpick comments (4)
apps/meteor/app/apps/server/bridges/listeners.ts (4)
221-286: Wrap switch case declarations in blocks to prevent scope leakage.Variables declared with
constinside switch cases without blocks can be accessed from other cases, leading to potential bugs. Biome flags this as an error.♻️ Suggested fix - wrap each case in a block
const result = await (() => { switch (args.event) { - case AppInterface.IPostMessageDeleted: - const [, userDeleted] = args.payload; + case AppInterface.IPostMessageDeleted: { + const [, userDeleted] = args.payload; return this.orch .getManager() .getListenerManager() .executeListener(args.event, { message: msg, user: this.orch.getConverters().get('users').convertToApp(userDeleted), }); - case AppInterface.IPostMessageReacted: - const [, userReacted, reaction, isReacted] = args.payload; + } + case AppInterface.IPostMessageReacted: { + const [, userReacted, reaction, isReacted] = args.payload; return this.orch .getManager() .getListenerManager() .executeListener(args.event, { message: msg, user: this.orch.getConverters().get('users').convertToApp(userReacted), reaction, isReacted, }); + } // ... similar wrapping needed for remaining cases
340-351: Wrap switch case declarations in blocks forlivechatEvent.The
const agentDatadeclaration on line 344 and declarations in the default case (lines 428-429) need block scoping to prevent scope leakage issues flagged by static analysis.♻️ Suggested fix for agent assigned/unassigned case
switch (args.event) { - case AppInterface.IPostLivechatAgentAssigned: - case AppInterface.IPostLivechatAgentUnassigned: - const [agentData] = args.payload; + case AppInterface.IPostLivechatAgentAssigned: + case AppInterface.IPostLivechatAgentUnassigned: { + const [agentData] = args.payload; return this.orch .getManager() .getListenerManager() .executeListener(args.event, { room: (await this.orch.getConverters().get('rooms').convertRoom(agentData.room)) as IAppsLivechatRoom, agent: this.orch.getConverters().get('users').convertToApp(agentData.user), }); + }
427-440: Default case needs block scoping for its declarations.The
roomDataandroomdeclarations in the default case need to be wrapped in a block.♻️ Suggested fix
- default: - const [roomData] = args.payload; - const room = await this.orch.getConverters().get('rooms').convertRoom(roomData); + default: { + const [roomData] = args.payload; + const room = await this.orch.getConverters().get('rooms').convertRoom(roomData); if (!room) { throw new Error(`Room ${roomData} not found`); } if (!isLivechatRoom(room)) { throw new Error(`Room ${roomData} is not a livechat room`); } return this.orch.getManager().getListenerManager().executeListener(args.event, room); + }
209-214: Consider adding a TODO or improving type safety fordefaultEvent.The
as anycast on line 213 bypasses type checking. While this may be necessary for the fallback handler, it could hide type mismatches for events that should have specific handling.
📜 Review details
Configuration used: Organization 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 (15)
apps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.jsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/livechat/server/lib/departmentsLib.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/server/lib/eraseRoom.tsapps/meteor/server/services/messages/service.tspackages/apps-engine/src/server/managers/AppListenerManager.ts
💤 Files with no reviewable changes (1)
- apps/meteor/app/apps/server/bridges/listeners.js
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/meteor/app/lib/server/functions/deleteMessage.ts
- apps/meteor/ee/server/apps/orchestrator.js
- apps/meteor/app/api/server/lib/eraseTeam.spec.ts
- apps/meteor/app/livechat/server/lib/departmentsLib.ts
- packages/apps-engine/src/server/managers/AppListenerManager.ts
- apps/meteor/app/livechat/server/lib/RoutingManager.ts
- apps/meteor/app/livechat/server/lib/closeRoom.ts
- apps/meteor/app/lib/server/functions/sendMessage.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:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/server/services/messages/service.tsapps/meteor/app/lib/server/functions/updateMessage.ts
🧠 Learnings (12)
📚 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/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/server/lib/eraseRoom.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:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/server/lib/eraseRoom.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:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/server/lib/eraseRoom.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/app/apps/server/bridges/listeners.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/app/apps/server/bridges/listeners.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:
apps/meteor/app/apps/server/bridges/listeners.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:
apps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/server/lib/eraseRoom.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/app/apps/server/bridges/listeners.ts
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
apps/meteor/server/lib/eraseRoom.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/server/lib/eraseRoom.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/server/services/messages/service.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:
apps/meteor/server/services/messages/service.ts
🧬 Code graph analysis (1)
apps/meteor/app/apps/server/bridges/listeners.ts (3)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)packages/apps-engine/src/definition/rooms/index.ts (6)
IPreRoomCreatePrevent(19-19)IPreRoomCreateExtend(17-17)IPreRoomCreateModify(18-18)IPostRoomCreate(15-15)IPreRoomDeletePrevent(20-20)IPostRoomDeleted(16-16)apps/meteor/client/lib/rooms/roomCoordinator.tsx (1)
isLivechatRoom(98-100)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/listeners.ts
[error] 224-224: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 233-233: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 244-244: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 254-254: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 264-264: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 274-274: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 308-308: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 319-319: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 344-344: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 428-428: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 429-429: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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 (13)
apps/meteor/app/api/server/lib/eraseTeam.ts (1)
77-93: LGTM! Event dispatch migration looks correct.The migration from bridge-based calls to
Apps.self?.triggerEventpreserves the existing behavior:
- Pre-delete prevention returns early if an app blocks the deletion
- Post-delete notification is correctly fire-and-forget with
voidMinor observation: The optional chaining on
Apps.self?.triggerEvent(lines 78, 92) is redundant after theApps.self?.isLoaded()guard, but this is harmless and consistent with the pattern used across other files in this PR.apps/meteor/app/lib/server/functions/updateMessage.ts (2)
27-39: LGTM! Event dispatch migration correctly preserves extend/modify chain.The migration maintains the correct sequencing:
IPreMessageUpdatedPrevent- blocks update if app prevents itIPreMessageUpdatedExtend- allows apps to extend the messageIPreMessageUpdatedModify- allows apps to modify the extended resultThe
typeof result === 'object'check at line 36 safely handles potentialundefinedreturns from the optional chaining.
88-92: LGTM! Post-update event correctly fires asynchronously.The
voidkeyword appropriately indicates fire-and-forget semantics for the post-update notification.apps/meteor/server/lib/eraseRoom.ts (1)
43-61: LGTM! Event dispatch migration consistent with strict validation pattern.The migration correctly preserves:
- Pre-delete prevention that throws
Meteor.Errorwhen blocked (strict validation)- Post-delete fire-and-forget notification with
voidThis aligns with the sibling function
eraseRoomLooseValidationin eraseTeam.ts, which uses the same pattern but returnsfalseinstead of throwing.apps/meteor/server/services/messages/service.ts (2)
1-1: LGTM! Import updated for AppEvents enum.The import correctly brings in
AppEventsalongsideAppsfor type-safe event dispatch.
204-206: LGTM! System message event dispatch migrated correctly.The fire-and-forget pattern with
voidis appropriate for post-system-message notifications.apps/meteor/ee/server/apps/communication/uikit.ts (5)
1-1: LGTM! Imports updated for type-safe orchestrator interface and events.The import correctly brings in
AppEventsfor type-safe event names andIAppServerOrchestratorinterface for better decoupling.
196-202: LGTM! Interface type improves flexibility.Changing from concrete
AppServerOrchestratortoIAppServerOrchestratorinterface improves testability and decoupling.
216-217: LGTM! Conditional conversion prevents unnecessary async operations.The explicit conditional pattern
mid ? await ... : undefinedis cleaner and avoids unnecessary converter calls when IDs are absent.
233-235: LGTM! Event interface selection uses type-safe enum.The ternary correctly routes to
IUIKitLivechatInteractionHandlerfor visitor interactions andIUIKitInteractionHandlerotherwise, using theAppEventsenum for type safety.
264-264: LGTM! All triggerEvent calls consistently use AppEvents enum.The migration from string literals to
AppEvents.IUIKitInteractionHandlerprovides compile-time safety and consistency across all interaction types (viewClosed, viewSubmit, actionButton).Also applies to: 289-289, 311-312, 330-330
apps/meteor/app/apps/server/bridges/listeners.ts (2)
304-331: Non-null assertions may cause runtime errors if user conversion fails.Lines 314 and 325 use
!assertions onconvertToAppresults forjoiningUserandleavingUser. If the conversion returnsnullorundefined, this will propagate an invalid value to the listener.Consider adding validation or using a more defensive approach.
Additionally, this switch also needs block scoping for the
constdeclarations (same issue asmessageEvent).
443-483: Good use of block scoping in switch cases.This method correctly wraps all switch case declarations in blocks, which is the pattern that should be applied to the other event handler methods (
messageEvent,roomEvent,livechatEvent).Consider adding a
defaultcase that either throws an error or returns a sentinel value to ensure exhaustive handling if new user events are added in the future.
d-gubert
force-pushed
the
chore/convert-app-listeners-ts
branch
from
4915155 to
0b28672
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @apps/meteor/app/apps/server/bridges/listeners.ts:
- Around line 427-440: In the switch default branch the const declarations
(const [roomData] and const room) leak into the switch scope; wrap the default
case body in a block (add { ... } after default:) so roomData and room are
block-scoped, i.e. enclose the await
this.orch.getConverters().get('rooms').convertRoom(...) call, the
isLivechatRoom(room) check, and the return of
this.orch.getManager().getListenerManager().executeListener(...) inside the new
braces to limit scope and avoid redeclaration conflicts.
- Around line 342-351: The const declaration in the switch cases for
AppInterface.IPostLivechatAgentAssigned and
AppInterface.IPostLivechatAgentUnassigned can leak into later cases; wrap those
two case labels in a block { ... } so that const [agentData] = args.payload and
the subsequent await this.orch.getConverters().get('rooms').convertRoom(...) and
this.orch.getConverters().get('users').convertToApp(agentData.user) are scoped
locally before calling
this.orch.getManager().getListenerManager().executeListener(...).
- Around line 72-94: The HandleRoomEvent union is ambiguous because
IPreRoomUserJoined, IPostRoomUserJoined, IPreRoomUserLeave, and
IPostRoomUserLeave are declared in both the 3-element payload variant and the
1-element payload variant; remove those four event names from the single-element
branch so that the 3-element variant (payload: [IRoom, IUser, IUser])
exclusively handles them and the remaining room lifecycle events
(IPreRoomCreatePrevent, IPreRoomCreateExtend, IPreRoomCreateModify,
IPostRoomCreate, IPreRoomDeletePrevent, IPostRoomDeleted) remain in the
single-element branch (payload: [IRoom]).
- Line 151: Remove the temporary debug statement console.log('args', args) from
the listener in listeners.ts; locate the occurrence inside the relevant event
handler function (around the listener where args is referenced) and delete it,
or replace it with an appropriate structured log via the existing logger (e.g.,
processLogger or debug utility) if persistent logging is required.
- Around line 221-286: The switch cases inside the IIFE assigning to result
declare const variables (e.g., userDeleted, userReacted, reaction, isReacted,
userFollowed, isFollowed, userPinned, isPinned, userStarred, isStarred,
userReported, reason) which can leak between cases; fix this by wrapping each
case body in its own block (add { } around the code after each case label) so
the const declarations are block-scoped; apply this to the cases for
AppInterface.IPostMessageDeleted, IPostMessageReacted, IPostMessageFollowed,
IPostMessagePinned, IPostMessageStarred and IPostMessageReported while keeping
the existing calls to
this.orch.getManager().getListenerManager().executeListener(...) intact.
- Around line 304-331: The switch cases handling
AppInterface.IPreRoomUserJoined/IPostRoomUserJoined and
AppInterface.IPreRoomUserLeave/IPostRoomUserLeave declare consts from
args.payload in the case bodies, so wrap each case body in braces ({ ... }) to
create a block scope around the const declarations; update the branches that
call this.orch.getManager().getListenerManager().executeListener(...) so the
const [, joiningUser, invitingUser] and const [, leavingUser, removedBy] are
scoped inside their respective block-wrapped cases and avoid duplicate-const
errors.
🧹 Nitpick comments (9)
apps/meteor/app/lib/server/functions/updateMessage.ts (2)
27-39: Redundant optional chaining inside guarded block.The outer condition on line 27 already ensures
Apps.selfis truthy, so the optional chaining (?.) on lines 28, 33, and 34 is unnecessary. While harmless, removing it would improve clarity.Suggested simplification
if (message && Apps.self && Apps.isLoaded()) { - const prevent = await Apps.self?.triggerEvent(AppEvents.IPreMessageUpdatedPrevent, messageData); + const prevent = await Apps.self.triggerEvent(AppEvents.IPreMessageUpdatedPrevent, messageData); if (prevent) { throw new Meteor.Error('error-app-prevented-updating', 'A Rocket.Chat App prevented the message updating.'); } - let result = await Apps.self?.triggerEvent(AppEvents.IPreMessageUpdatedExtend, messageData); - result = await Apps.self?.triggerEvent(AppEvents.IPreMessageUpdatedModify, result); + let result = await Apps.self.triggerEvent(AppEvents.IPreMessageUpdatedExtend, messageData); + result = await Apps.self.triggerEvent(AppEvents.IPreMessageUpdatedModify, result); if (typeof result === 'object') { Object.assign(messageData, result); } }
88-92: Same redundancy pattern for post-update event.Similar to above,
Apps.self?.on line 91 is redundant since line 88 already guards withApps.self?.isLoaded().Suggested simplification
if (Apps.self?.isLoaded()) { // This returns a promise, but it won't mutate anything about the message // so, we don't really care if it is successful or fails - void Apps.self?.triggerEvent(AppEvents.IPostMessageUpdated, messageData); + void Apps.self.triggerEvent(AppEvents.IPostMessageUpdated, messageData); }apps/meteor/app/lib/server/functions/sendMessage.ts (2)
233-251: Redundant optional chaining inside guarded block.Lines 234 and 240-242 use
Apps.self?.inside a block already guarded byApps.self?.isLoaded(). Since the guard ensuresApps.selfis truthy, the optional chaining is redundant.Suggested simplification
if (Apps.self?.isLoaded()) { - const prevent = await Apps.self?.triggerEvent(AppEvents.IPreMessageSentPrevent, message); + const prevent = await Apps.self.triggerEvent(AppEvents.IPreMessageSentPrevent, message); if (prevent) { return; } - const result = await Apps.self?.triggerEvent( + const result = await Apps.self.triggerEvent( AppEvents.IPreMessageSentModify, - await Apps.self?.triggerEvent(AppEvents.IPreMessageSentExtend, message), + await Apps.self.triggerEvent(AppEvents.IPreMessageSentExtend, message), ); if (typeof result === 'object') { message = Object.assign(message, result); // Some app may have inserted malicious/invalid values in the message, let's check it again await validateMessage(message, room, user); } }
282-286: Same redundancy pattern for post-send events.The optional chaining on line 285 is redundant since line 282 already ensures
Apps.selfis truthy.Suggested simplification
if (Apps.self?.isLoaded()) { // If the message has a type (system message), we should notify the listener about it const messageEvent = message.t ? AppEvents.IPostSystemMessageSent : AppEvents.IPostMessageSent; - void Apps.self?.triggerEvent(messageEvent, message); + void Apps.self.triggerEvent(messageEvent, message); }apps/meteor/app/api/server/lib/eraseTeam.ts (2)
77-82: Redundant optional chaining afterisLoaded()check.Since
Apps.self?.isLoaded()at line 77 is truthy,Apps.selfis guaranteed to be defined inside the block. The?.on line 78 is redundant.Suggested fix
if (Apps.self?.isLoaded()) { - const prevent = await Apps.self?.triggerEvent(AppEvents.IPreRoomDeletePrevent, room); + const prevent = await Apps.self.triggerEvent(AppEvents.IPreRoomDeletePrevent, room); if (prevent) { return false; } }
91-93: Same redundant optional chaining pattern.The
?.on line 92 is unnecessary sinceApps.selfis already confirmed to exist by theisLoaded()check.Suggested fix
if (Apps.self?.isLoaded()) { - void Apps.self?.triggerEvent(AppEvents.IPostRoomDeleted, room); + void Apps.self.triggerEvent(AppEvents.IPostRoomDeleted, room); }apps/meteor/server/lib/eraseRoom.ts (2)
43-48: Redundant optional chaining inside guarded block.Inside the
if (Apps.self?.isLoaded())block,Apps.selfis guaranteed to be defined, so the?.on line 44 is redundant. This is harmless but slightly inconsistent.✨ Suggested simplification
if (Apps.self?.isLoaded()) { - const prevent = await Apps.self?.triggerEvent(AppEvents.IPreRoomDeletePrevent, room); + const prevent = await Apps.self.triggerEvent(AppEvents.IPreRoomDeletePrevent, room); if (prevent) { throw new Meteor.Error('error-app-prevented-deleting', 'A Rocket.Chat App prevented the room erasing.'); } }
59-61: Same redundant optional chaining; fire-and-forget pattern is correct.Same nitpick as above:
Apps.self.triggerEventcan be used directly inside the guarded block. Thevoidprefix correctly signals fire-and-forget semantics for the post-delete event.✨ Suggested simplification
if (Apps.self?.isLoaded()) { - void Apps.self?.triggerEvent(AppEvents.IPostRoomDeleted, room); + void Apps.self.triggerEvent(AppEvents.IPostRoomDeleted, room); }apps/meteor/app/apps/server/bridges/listeners.ts (1)
443-483: Consider adding exhaustive check for future-proofing.The switch handles all current
HandleUserEventvariants, but adding an exhaustive check helps catch missed cases if new events are added later.Suggested pattern
case AppInterface.IPostUserUpdated: { const [crudData] = args.payload; const context: IUserUpdateContext = { user: this.orch.getConverters().get('users').convertToApp(crudData.user), performedBy: this.orch.getConverters().get('users').convertToApp(crudData.performedBy), previousData: this.orch.getConverters().get('users').convertToApp(crudData.previousUser), }; return this.orch.getManager().getListenerManager().executeListener(args.event, context); } + default: { + const _exhaustiveCheck: never = args; + throw new Error(`Unhandled user event: ${(_exhaustiveCheck as HandleUserEvent).event}`); + } } }
📜 Review details
Configuration used: Organization 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 (15)
apps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/app/apps/server/bridges/listeners.jsapps/meteor/app/apps/server/bridges/listeners.tsapps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/livechat/server/lib/departmentsLib.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/server/lib/eraseRoom.tsapps/meteor/server/services/messages/service.tspackages/apps-engine/src/server/managers/AppListenerManager.ts
💤 Files with no reviewable changes (1)
- apps/meteor/app/apps/server/bridges/listeners.js
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/ee/server/apps/orchestrator.js
- apps/meteor/app/livechat/server/lib/closeRoom.ts
- packages/apps-engine/src/server/managers/AppListenerManager.ts
- apps/meteor/server/services/messages/service.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:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/app/lib/server/functions/updateMessage.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/livechat/server/lib/departmentsLib.tsapps/meteor/app/lib/server/functions/sendMessage.tsapps/meteor/app/apps/server/bridges/listeners.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:
apps/meteor/app/api/server/lib/eraseTeam.spec.ts
🧠 Learnings (20)
📚 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:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/apps/server/bridges/listeners.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:
apps/meteor/app/api/server/lib/eraseTeam.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/apps/server/bridges/listeners.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/app/api/server/lib/eraseTeam.tsapps/meteor/server/lib/eraseRoom.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/apps/server/bridges/listeners.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:
apps/meteor/server/lib/eraseRoom.tsapps/meteor/app/lib/server/functions/deleteMessage.tsapps/meteor/app/apps/server/bridges/listeners.ts
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
apps/meteor/server/lib/eraseRoom.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/server/lib/eraseRoom.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/app/api/server/lib/eraseTeam.spec.tsapps/meteor/app/apps/server/bridges/listeners.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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/app/api/server/lib/eraseTeam.spec.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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.spec.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/app/lib/server/functions/sendMessage.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/app/lib/server/functions/sendMessage.tsapps/meteor/app/apps/server/bridges/listeners.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:
apps/meteor/app/apps/server/bridges/listeners.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/app/apps/server/bridges/listeners.ts
🧬 Code graph analysis (3)
apps/meteor/app/lib/server/functions/updateMessage.ts (1)
packages/core-services/src/index.ts (1)
Apps(152-152)
apps/meteor/app/lib/server/functions/sendMessage.ts (2)
packages/core-services/src/index.ts (1)
Apps(152-152)apps/meteor/app/apps/server/bridges/listeners.ts (1)
messageEvent(216-297)
apps/meteor/app/apps/server/bridges/listeners.ts (2)
packages/apps/src/AppsEngine.ts (1)
AppInterface(13-13)packages/apps-engine/src/definition/rooms/index.ts (6)
IPreRoomCreatePrevent(19-19)IPreRoomCreateExtend(17-17)IPreRoomCreateModify(18-18)IPostRoomCreate(15-15)IPreRoomDeletePrevent(20-20)IPostRoomDeleted(16-16)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/listeners.ts
[error] 224-224: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 233-233: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 244-244: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 254-254: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 264-264: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 274-274: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 308-308: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 319-319: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 344-344: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 428-428: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 429-429: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ 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 (18)
apps/meteor/app/livechat/server/lib/RoutingManager.ts (1)
294-294: LGTM!The migration from the Bridges-based
getListenerBridge().livechatEvent()to the unifiedApps.self?.triggerEvent()API is correctly implemented. The fire-and-forget semantics are preserved withvoid, and proper null safety is maintained via optional chaining.apps/meteor/app/livechat/server/lib/departmentsLib.ts (2)
139-143: LGTM - Clean refactor to unified event API.The change from
getBridges()?.getListenerBridge().livechatEvent()toApps.self?.triggerEvent()simplifies the call chain while preserving the fire-and-forget semantics withvoidand null safety via optional chaining.
271-272: LGTM - Consistent with the new event triggering pattern.Same refactor pattern applied here for the department removal event, maintaining behavioral parity with the previous implementation.
apps/meteor/app/lib/server/functions/deleteMessage.ts (2)
39-43: LGTM - Safe handling of optional Apps.The optional chaining correctly handles the case where
Apps.selfmight be undefined. If Apps aren't loaded,preventwill beundefined(falsy), and execution continues normally without throwing.
103-105: LGTM - Post-delete event properly guarded.The post-delete event is correctly wrapped with the
deletedMsgcheck and uses appropriate optional chaining forApps.self. Thevoidkeyword correctly indicates the fire-and-forget pattern.apps/meteor/app/lib/server/functions/sendMessage.ts (1)
1-1: LGTM - Import updated correctly.The import now includes
AppEventsalongsideApps, which provides type-safe event names for thetriggerEventcalls throughout the file.apps/meteor/app/api/server/lib/eraseTeam.spec.ts (3)
43-48: LGTM!The stub structure correctly mirrors the new
Apps.self.triggerEventAPI surface, replacing the previous bridge-based chain. DefaultisLoaded: () => falseensures Apps events are skipped in tests that don't need them.
190-219: LGTM!The test correctly verifies that when
triggerEventreturnstrueforIPreRoomDeletePrevent, the deletion is blocked and the function returnsfalse. The stub setup properly reflects the new API.
242-272: LGTM!The test properly verifies the success path: first
triggerEventcall (pre-delete) returnsfalseto allow deletion, then second call (post-delete) is invoked. The assertionsinon.assert.calledTwice(roomEventStub)confirms both events are triggered.apps/meteor/ee/server/apps/communication/uikit.ts (6)
1-1: LGTM!Good use of the interface type
IAppServerOrchestratorandAppEventsconstant imports, aligning with the dependency inversion principle and improving type safety.
196-199: LGTM!Using the interface
IAppServerOrchestratorinstead of the concrete class improves testability and follows the dependency inversion principle.
216-217: LGTM!The conditional conversion (
mid ? ... : undefined) correctly handles cases wheremid/ridare not present in the request body, avoiding unnecessary conversion calls.
233-235: LGTM!Using
AppEvents.IUIKitInteractionHandlerandAppEvents.IUIKitLivechatInteractionHandlerconstants instead of string literals improves type safety and makes the code more resilient to refactoring.
264-264: LGTM!Consistent use of
AppEvents.IUIKitInteractionHandlerconstant across all handler cases (viewClosed,viewSubmit,actionButton) aligns with the broader refactor pattern.Also applies to: 289-289, 330-330
311-312: LGTM!Same safe conditional conversion pattern as in
blockActioncase, correctly handling optionalrid/midfields.apps/meteor/app/apps/server/bridges/listeners.ts (3)
209-214: LGTM with note onas anycast.The
as anycast is pragmatic here sincedefaultEventis a fallback handler for events not explicitly handled. The type variability makes strict typing impractical in this context.
1-31: Type definitions are well-structured.The internal type aliases (
LivechatTransferData,LivechatAgentData,UserStatusChangedData,UserCrudData) provide clear contracts for the event payloads. Good use of discriminated unions for type safety.
146-147: LGTM!Clean constructor with proper dependency injection pattern.
ggazzo
force-pushed
the
chore/convert-app-listeners-ts
branch
from
0b28672 to
69b79aa
Compare
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
Refactor
listeners.jstolisteners.ts.There were some adaptations that were required to properly type this correctly, both in the file itself and others.
Issue(s)
ARCH-1917
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Refactor
Chores
Tests
Notes
✏️ Tip: You can customize this high-level summary in your review settings.