refactor(api): migrate integrations.* endpoints from addRoute to new chained router API#39374
Closed
smirk-dev wants to merge 3 commits intoRocketChat:developfrom
Closed
Conversation
… API Replaces all 6 API.v1.addRoute() calls in integrations.ts with the new chained router pattern (API.v1.get().post().put()), following the migration ongoing as part of the REST API definitions modernization effort. Changes: - integrations.create (POST): addRoute → .post() with body validator - integrations.history (GET): addRoute → .get() with query validator - integrations.list (GET): addRoute → .get() with query validator - integrations.remove (POST): addRoute → .post(), removes inline check() - integrations.get (GET): addRoute → .get() with query validator - integrations.update (PUT): addRoute → .put() with body validator All 6 endpoints now have AJV response schemas (200/400/401/403) enabling automatic OpenAPI documentation generation. The IntegrationsEndpoints type is removed from the centralized rest-typings Endpoints interface and is now declared via 'declare module' directly in the route file (ExtractRoutesFromAPI pattern), making the route file the single source of truth for its type contract.
|
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
Contributor
WalkthroughMigrates integrations REST endpoints to the chained router with AJV request/response schemas and module-augmented route typings; removes Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as API.v1.integrations (router)
participant Validator as AJV Validator
participant Auth as Permission Check
participant Controller as Integrations Controller
participant DB as Database
Client->>Router: HTTP request (create/get/list/update/remove/history)
Router->>Validator: Validate request body/params
Validator-->>Router: valid / errors
Router->>Auth: Check permissions
Auth-->>Router: allowed / denied
Router->>Controller: Invoke handler
Controller->>DB: Read/Write integration data
DB-->>Controller: Result
Controller-->>Router: Build response (AJV response schema)
Router-->>Client: HTTP response (200/4xx)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 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. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/integrations.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/integrations.ts:46">
P1: The integrations.* routes are registered twice: the new chained API definitions were added, but the legacy API.v1.addRoute blocks for the same endpoints still remain below. This risks duplicate-route errors or handler overrides and should be cleaned up during the migration.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| const integrationsEndpoints = API.v1 | ||
| .post( | ||
| 'integrations.create', |
Contributor
There was a problem hiding this comment.
P1: The integrations.* routes are registered twice: the new chained API definitions were added, but the legacy API.v1.addRoute blocks for the same endpoints still remain below. This risks duplicate-route errors or handler overrides and should be cleaned up during the migration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/integrations.ts, line 46:
<comment>The integrations.* routes are registered twice: the new chained API definitions were added, but the legacy API.v1.addRoute blocks for the same endpoints still remain below. This risks duplicate-route errors or handler overrides and should be cleaned up during the migration.</comment>
<file context>
@@ -22,10 +26,314 @@ import { updateIncomingIntegration } from '../../../integrations/server/methods/
+
+const integrationsEndpoints = API.v1
+ .post(
+ 'integrations.create',
+ {
+ authRequired: true,
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/refactor-integrations-api-chained-pattern.md:
- Around line 8-10: The changeset claims all API.v1.addRoute registrations and
the inline check() were removed, but the code still contains legacy
API.v1.addRoute calls and a check() usage in integrations.ts; update either the
code or the changeset: either remove the legacy block (the API.v1.addRoute
registrations starting around the integrations module and the check() call used
by integrations.remove) so the changeset matches the code, or edit the changeset
bullets to accurately reflect that those legacy addRoute registrations and the
inline check() remain; ensure you reference and modify the API.v1.addRoute calls
and the integrations.remove check() usage when making the fix.
In `@apps/meteor/app/api/server/v1/integrations.ts`:
- Around line 44-327: Duplicate route registrations exist because legacy
API.v1.addRoute calls for the same endpoints are still present alongside the new
chained integrationsEndpoints; remove the old API.v1.addRoute blocks that
register integrations.create, integrations.history, integrations.list,
integrations.remove, integrations.get, and integrations.update so only the
integrationsEndpoints chain defines those routes, and ensure
integrationsEndpoints is exported/registered where required (look for the
integrationsEndpoints variable and any API.v1.addRoute usages referencing the
same route names to delete).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 197afdd1-2faa-4bdc-a6f6-667880f887ed
📒 Files selected for processing (3)
.changeset/refactor-integrations-api-chained-pattern.mdapps/meteor/app/api/server/v1/integrations.tspackages/rest-typings/src/index.ts
💤 Files with no reviewable changes (1)
- packages/rest-typings/src/index.ts
📜 Review details
🧰 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/v1/integrations.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/api/server/v1/integrations.ts.changeset/refactor-integrations-api-chained-pattern.md
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/integrations.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/integrations.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/v1/integrations.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/v1/integrations.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/refactor-integrations-api-chained-pattern.md
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/integrations.ts (1)
34-42: Remove| nullfrom line 285 type annotation.Line 285 declares
ajv.compile<{ integration: IIntegration | null }>(integrationResponseSchema), but the handler never returns null in a 200 success response—it returns failure for missing integrations. The type annotation is incorrect and inconsistent with other endpoints (lines 51, 201, 259) which correctly useIIntegrationwithout nullability. The schema is correct as-is.> Likely an incorrect or invalid review comment.
Comment on lines
+8
to
+10
| - Replaces all 6 `addRoute` calls with chained `.get()`, `.post()`, and `.put()` calls | ||
| - Removes the inline `check()` call from `integrations.remove` in favor of type-safe validator guard | ||
| - Adds AJV response schemas for all 6 endpoints (200/400/401/403 responses) |
Contributor
There was a problem hiding this comment.
Update changeset claims to match the current implementation.
These bullets state all addRoute and inline check() usage were removed, but integrations.ts still contains legacy API.v1.addRoute registrations (starting at Line 337) and a check() call (Line 493). Please either remove the legacy block in code or reword this changeset accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/refactor-integrations-api-chained-pattern.md around lines 8 - 10,
The changeset claims all API.v1.addRoute registrations and the inline check()
were removed, but the code still contains legacy API.v1.addRoute calls and a
check() usage in integrations.ts; update either the code or the changeset:
either remove the legacy block (the API.v1.addRoute registrations starting
around the integrations module and the check() call used by integrations.remove)
so the changeset matches the code, or edit the changeset bullets to accurately
reflect that those legacy addRoute registrations and the inline check() remain;
ensure you reference and modify the API.v1.addRoute calls and the
integrations.remove check() usage when making the fix.
Comment on lines
+44
to
+327
| const integrationsEndpoints = API.v1 | ||
| .post( | ||
| 'integrations.create', | ||
| { | ||
| authRequired: true, | ||
| body: isIntegrationsCreateProps, | ||
| response: { | ||
| 200: ajv.compile<{ integration: IIntegration }>(integrationResponseSchema), | ||
| 400: validateBadRequestErrorResponse, | ||
| 401: validateUnauthorizedErrorResponse, | ||
| }, | ||
| }, | ||
| async function action() { | ||
| switch (this.bodyParams.type) { | ||
| case 'webhook-outgoing': | ||
| return API.v1.success({ integration: await addOutgoingIntegration(this.userId, this.bodyParams as INewOutgoingIntegration) }); | ||
| case 'webhook-incoming': | ||
| return API.v1.success({ integration: await addIncomingIntegration(this.userId, this.bodyParams as INewIncomingIntegration) }); | ||
| default: | ||
| return API.v1.failure('Invalid integration type.'); | ||
| } | ||
| }, | ||
| ) | ||
| .get( | ||
| 'integrations.history', | ||
| { | ||
| authRequired: true, | ||
| query: isIntegrationsHistoryProps, | ||
| permissionsRequired: { permissions: ['manage-outgoing-integrations', 'manage-own-outgoing-integrations'], operation: 'hasAny' }, | ||
| response: { | ||
| 200: ajv.compile<PaginatedResult<{ history: IIntegrationHistory[]; items: number }>>({ | ||
| type: 'object', | ||
| properties: { | ||
| history: { type: 'array', items: { type: 'object', additionalProperties: true } }, | ||
| offset: { type: 'number' }, | ||
| count: { type: 'number' }, | ||
| items: { type: 'number' }, | ||
| total: { type: 'number' }, | ||
| success: { type: 'boolean', enum: [true] }, | ||
| }, | ||
| required: ['history', 'offset', 'count', 'items', 'total', 'success'], | ||
| additionalProperties: false, | ||
| }), | ||
| 400: validateBadRequestErrorResponse, | ||
| 401: validateUnauthorizedErrorResponse, | ||
| 403: validateForbiddenErrorResponse, | ||
| }, | ||
| }, | ||
| async function action() { | ||
| const { userId, queryParams } = this; | ||
|
|
||
| if (!queryParams.id || queryParams.id.trim() === '') { | ||
| return API.v1.failure('Invalid integration id.'); | ||
| } | ||
|
|
||
| const { id } = queryParams; | ||
| const { offset, count } = await getPaginationItems(this.queryParams); | ||
| const { sort, fields: projection, query } = await this.parseJsonQuery(); | ||
| const ourQuery = Object.assign(await mountIntegrationHistoryQueryBasedOnPermissions(userId, id), query); | ||
|
|
||
| const { cursor, totalCount } = IntegrationHistory.findPaginated(ourQuery, { | ||
| sort: sort || { _updatedAt: -1 }, | ||
| skip: offset, | ||
| limit: count, | ||
| projection, | ||
| }); | ||
|
|
||
| const [history, total] = await Promise.all([cursor.toArray(), totalCount]); | ||
|
|
||
| return API.v1.success({ | ||
| history, | ||
| offset, | ||
| items: history.length, | ||
| count: history.length, | ||
| total, | ||
| }); | ||
| }, | ||
| ) | ||
| .get( | ||
| 'integrations.list', | ||
| { | ||
| authRequired: true, | ||
| query: isIntegrationsListProps, | ||
| permissionsRequired: { | ||
| permissions: [ | ||
| 'manage-outgoing-integrations', | ||
| 'manage-own-outgoing-integrations', | ||
| 'manage-incoming-integrations', | ||
| 'manage-own-incoming-integrations', | ||
| ], | ||
| operation: 'hasAny', | ||
| }, | ||
| response: { | ||
| 200: ajv.compile<PaginatedResult<{ integrations: IIntegration[]; items: number }>>({ | ||
| type: 'object', | ||
| properties: { | ||
| integrations: { type: 'array', items: { type: 'object', additionalProperties: true } }, | ||
| offset: { type: 'number' }, | ||
| count: { type: 'number' }, | ||
| items: { type: 'number' }, | ||
| total: { type: 'number' }, | ||
| success: { type: 'boolean', enum: [true] }, | ||
| }, | ||
| required: ['integrations', 'offset', 'count', 'items', 'total', 'success'], | ||
| additionalProperties: false, | ||
| }), | ||
| 400: validateBadRequestErrorResponse, | ||
| 401: validateUnauthorizedErrorResponse, | ||
| 403: validateForbiddenErrorResponse, | ||
| }, | ||
| }, | ||
| async function action() { | ||
| const { offset, count } = await getPaginationItems(this.queryParams); | ||
| const { sort, fields, query } = await this.parseJsonQuery(); | ||
| const { name, type } = this.queryParams; | ||
|
|
||
| const filter = { | ||
| ...query, | ||
| ...(name ? { name: { $regex: escapeRegExp(name as string), $options: 'i' } } : {}), | ||
| ...(type ? { type } : {}), | ||
| }; | ||
|
|
||
| const ourQuery = Object.assign(await mountIntegrationQueryBasedOnPermissions(this.userId), filter) as Filter<IIntegration>; | ||
|
|
||
| const { cursor, totalCount } = Integrations.findPaginated(ourQuery, { | ||
| sort: sort || { ts: -1 }, | ||
| skip: offset, | ||
| limit: count, | ||
| projection: fields, | ||
| }); | ||
|
|
||
| const [integrations, total] = await Promise.all([cursor.toArray(), totalCount]); | ||
|
|
||
| return API.v1.success({ | ||
| integrations, | ||
| offset, | ||
| items: integrations.length, | ||
| count: integrations.length, | ||
| total, | ||
| }); | ||
| }, | ||
| ) | ||
| .post( | ||
| 'integrations.remove', | ||
| { | ||
| authRequired: true, | ||
| body: isIntegrationsRemoveProps, | ||
| permissionsRequired: { | ||
| permissions: [ | ||
| 'manage-outgoing-integrations', | ||
| 'manage-own-outgoing-integrations', | ||
| 'manage-incoming-integrations', | ||
| 'manage-own-incoming-integrations', | ||
| ], | ||
| operation: 'hasAny', | ||
| }, | ||
| response: { | ||
| 200: ajv.compile<{ integration: IIntegration }>(integrationResponseSchema), | ||
| 400: validateBadRequestErrorResponse, | ||
| 401: validateUnauthorizedErrorResponse, | ||
| 403: validateForbiddenErrorResponse, | ||
| }, | ||
| }, | ||
| async function action() { | ||
| const { bodyParams } = this; | ||
|
|
||
| let integration: IIntegration | null = null; | ||
| switch (bodyParams.type) { | ||
| case 'webhook-outgoing': | ||
| if (!bodyParams.target_url && !bodyParams.integrationId) { | ||
| return API.v1.failure('An integrationId or target_url needs to be provided.'); | ||
| } | ||
|
|
||
| if (bodyParams.target_url) { | ||
| integration = await Integrations.findOne({ urls: bodyParams.target_url }); | ||
| } else if (bodyParams.integrationId) { | ||
| integration = await Integrations.findOne({ _id: bodyParams.integrationId }); | ||
| } | ||
|
|
||
| if (!integration) { | ||
| return API.v1.failure('No integration found.'); | ||
| } | ||
|
|
||
| await deleteOutgoingIntegration(integration._id, this.userId); | ||
|
|
||
| return API.v1.success({ | ||
| integration, | ||
| }); | ||
| case 'webhook-incoming': | ||
| if (!bodyParams.integrationId) { | ||
| return API.v1.failure('An integrationId needs to be provided.'); | ||
| } | ||
|
|
||
| integration = await Integrations.findOne({ _id: bodyParams.integrationId }); | ||
|
|
||
| if (!integration) { | ||
| return API.v1.failure('No integration found.'); | ||
| } | ||
|
|
||
| await deleteIncomingIntegration(integration._id, this.userId); | ||
|
|
||
| return API.v1.success({ | ||
| integration, | ||
| }); | ||
| default: | ||
| return API.v1.failure('Invalid integration type.'); | ||
| } | ||
| }, | ||
| ) | ||
| .get( | ||
| 'integrations.get', | ||
| { | ||
| authRequired: true, | ||
| query: isIntegrationsGetProps, | ||
| response: { | ||
| 200: ajv.compile<{ integration: IIntegration }>(integrationResponseSchema), | ||
| 400: validateBadRequestErrorResponse, | ||
| 401: validateUnauthorizedErrorResponse, | ||
| }, | ||
| }, | ||
| async function action() { | ||
| const { integrationId, createdBy } = this.queryParams; | ||
| if (!integrationId) { | ||
| return API.v1.failure('The query parameter "integrationId" is required.'); | ||
| } | ||
|
|
||
| return API.v1.success({ | ||
| integration: await findOneIntegration({ | ||
| userId: this.userId, | ||
| integrationId, | ||
| createdBy, | ||
| }), | ||
| }); | ||
| }, | ||
| ) | ||
| .put( | ||
| 'integrations.update', | ||
| { | ||
| authRequired: true, | ||
| body: isIntegrationsUpdateProps, | ||
| response: { | ||
| 200: ajv.compile<{ integration: IIntegration | null }>(integrationResponseSchema), | ||
| 400: validateBadRequestErrorResponse, | ||
| 401: validateUnauthorizedErrorResponse, | ||
| }, | ||
| }, | ||
| async function action() { | ||
| const { bodyParams } = this; | ||
|
|
||
| let integration; | ||
| switch (bodyParams.type) { | ||
| case 'webhook-outgoing': | ||
| if (bodyParams.target_url) { | ||
| integration = await Integrations.findOne({ urls: bodyParams.target_url }); | ||
| } else if (bodyParams.integrationId) { | ||
| integration = await Integrations.findOne({ _id: bodyParams.integrationId }); | ||
| } | ||
|
|
||
| if (!integration) { | ||
| return API.v1.failure('No integration found.'); | ||
| } | ||
|
|
||
| await updateOutgoingIntegration(this.userId, integration._id, bodyParams); | ||
|
|
||
| return API.v1.success({ | ||
| integration: await Integrations.findOne({ _id: integration._id }), | ||
| }); | ||
| case 'webhook-incoming': | ||
| integration = await Integrations.findOne({ _id: bodyParams.integrationId }); | ||
|
|
||
| if (!integration) { | ||
| return API.v1.failure('No integration found.'); | ||
| } | ||
|
|
||
| await updateIncomingIntegration(this.userId, integration._id, bodyParams); | ||
|
|
||
| return API.v1.success({ | ||
| integration: await Integrations.findOne({ _id: integration._id }), | ||
| }); | ||
| default: | ||
| return API.v1.failure('Invalid integration type.'); | ||
| } | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Remove legacy API.v1.addRoute registrations before merging.
The new chained routes are added here, but the legacy registrations for the same paths still exist starting at Line 337. This creates duplicate route definitions (integrations.create/history/list/remove/get/update) and can cause registration conflicts or shadowing at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/integrations.ts` around lines 44 - 327,
Duplicate route registrations exist because legacy API.v1.addRoute calls for the
same endpoints are still present alongside the new chained
integrationsEndpoints; remove the old API.v1.addRoute blocks that register
integrations.create, integrations.history, integrations.list,
integrations.remove, integrations.get, and integrations.update so only the
integrationsEndpoints chain defines those routes, and ensure
integrationsEndpoints is exported/registered where required (look for the
integrationsEndpoints variable and any API.v1.addRoute usages referencing the
same route names to delete).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts (1)
107-119: Optional: cacheuserIdSetarray conversion onceLine 107, Line 113, and Line 119 each spread
userIdSetinto a new array. You can allocate once and reuse for slightly lower overhead and clearer intent.Suggested refactor
const { toAll, toHere, mentionIds } = mentions; const userIdSet = new Set([...mentionIds, ...highlightIds]); + const userIds = [...userIdSet]; const unreadCount = getUnreadSettingCount(room.t); const unreadAllMessages = unreadCount === 'all_messages'; const userMentionInc = getUserMentions(room.t, unreadCount as Exclude<UnreadCountType, 'group_mentions_only'>); const groupMentionInc = getGroupMentions(room.t, unreadCount as Exclude<UnreadCountType, 'user_mentions_only'>); const subs = await Subscriptions.findByRoomIdAndNotAlertOrOpenExcludingUserIds({ roomId: room._id, uidsExclude: [message.u._id], - uidsInclude: [...userIdSet], + uidsInclude: userIds, onlyRead: !toAll && !toHere && !unreadAllMessages, }).toArray(); if (userIdSet.size) { - await Subscriptions.incUserMentionsAndUnreadForRoomIdAndUserIds(room._id, [...userIdSet], 1, userMentionInc); + await Subscriptions.incUserMentionsAndUnreadForRoomIdAndUserIds(room._id, userIds, 1, userMentionInc); } else if (toAll || toHere) { await Subscriptions.incGroupMentionsAndUnreadForRoomIdExcludingUserId(room._id, message.u._id, 1, groupMentionInc); } if (!toAll && !toHere && unreadAllMessages) { - await Subscriptions.incUnreadForRoomIdExcludingUserIds(room._id, [...userIdSet, message.u._id], 1); + await Subscriptions.incUnreadForRoomIdExcludingUserIds(room._id, [...userIds, message.u._id], 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts` around lines 107 - 119, The code repeatedly spreads userIdSet into arrays; allocate a single array const userIds = [...userIdSet] once and reuse it in the calls to Subscriptions.incUserMentionsAndUnreadForRoomIdAndUserIds(room._id, userIds, ...), Subscriptions.incGroupMentionsAndUnreadForRoomIdExcludingUserId(...), and Subscriptions.incUnreadForRoomIdExcludingUserIds(room._id, [...userIds, message.u._id], ...) to avoid redundant conversions and clarify intent; update all references to use that cached userIds variable where applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts`:
- Around line 107-119: The code repeatedly spreads userIdSet into arrays;
allocate a single array const userIds = [...userIdSet] once and reuse it in the
calls to Subscriptions.incUserMentionsAndUnreadForRoomIdAndUserIds(room._id,
userIds, ...),
Subscriptions.incGroupMentionsAndUnreadForRoomIdExcludingUserId(...), and
Subscriptions.incUnreadForRoomIdExcludingUserIds(room._id, [...userIds,
message.u._id], ...) to avoid redundant conversions and clarify intent; update
all references to use that cached userIds variable where applicable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f697849-4fc8-4ab4-9500-7974bea9b88f
📒 Files selected for processing (1)
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
📜 Review details
🧰 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/lib/server/lib/notifyUsersOnMessage.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 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/lib/server/lib/notifyUsersOnMessage.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/lib/server/lib/notifyUsersOnMessage.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts
🔇 Additional comments (1)
apps/meteor/app/lib/server/lib/notifyUsersOnMessage.ts (1)
95-129: Set-based mention handling looks correct and cleanerThe switch to
Seton Line 95 and.has()on Line 129 preserves behavior while making membership checks cheaper and keeping deduplication explicit. Nice refactor.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/apps/server/bridges/listeners.ts`:
- Around line 249-258: The current catch on executeListener in defaultEvent
swallows errors and returns undefined, breaking contracts for handlers like
IUIKitInteractionHandler, IUIKitLivechatInteractionHandler and IPreEmailSent
that must return concrete responses; update defaultEvent (or the code path that
calls this.orch.getManager().getListenerManager().executeListener) to log the
error via this.orch.getRocketChatLogger().error(...) and then rethrow the error
(or propagate it) so callers receive the failure instead of undefined, or
alternatively detect those interactive handler types
(IUIKitInteractionHandler/IUIKitLivechatInteractionHandler/IPreEmailSent) and
handle them in a separate path that returns a proper
IUIKitResponse/IEmailDescriptor even on error; ensure you reference
executeListener and defaultEvent when making the change.
- Around line 410-415: The current catch in the default switch path (used by
messageEvent(), roomEvent(), and livechatEvent()) logs errors and returns
undefined, which breaks pre-hook semantics; change the error handling so that
for pre-event hooks (IPre* hooks routed through those default cases) the catch
still logs via this.orch.getRocketChatLogger().error but then rethrows the error
(or returns a rejected promise) instead of swallowing it, while leaving
post-event hook behavior unchanged; locate the calls to
this.orch.getManager().getListenerManager().executeListener(...) in
messageEvent, roomEvent, and livechatEvent and update their .catch handlers to
log then rethrow for pre-hooks so prevent/extend/modify and pre-livechat-create
maintain their veto/transform side effects.
In `@packages/apps-engine/src/server/managers/AppListenerManager.ts`:
- Line 374: The IListenerExecutor[AppInterface.IPostMessageUpdated].result type
mismatches executePostMessageUpdated() (which currently resolves void) causing
executeListener(AppInterface.IPostMessageUpdated, ...) to advertise an IMessage
but actually return undefined; fix by making executePostMessageUpdated(...)
explicitly return the updated IMessage (or adjust the IListenerExecutor generic
for AppInterface.IPostMessageUpdated to void if callers should not receive a
message). Locate executePostMessageUpdated and update its
implementation/signature to return the updated IMessage (ensure any internal
awaits/callbacks produce and return that IMessage) and update any related type
declarations (IListenerExecutor[AppInterface.IPostMessageUpdated].result) so
they match the actual return type used by executeListener.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76f0007b-15e6-4c4c-a8f2-d4a2ba359d5f
📒 Files selected for processing (2)
apps/meteor/app/apps/server/bridges/listeners.tspackages/apps-engine/src/server/managers/AppListenerManager.ts
📜 Review details
⏰ 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 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/apps/server/bridges/listeners.tspackages/apps-engine/src/server/managers/AppListenerManager.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:44.506Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/apps/server/bridges/listeners.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/apps/server/bridges/listeners.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/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/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 (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-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.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/apps/server/bridges/listeners.tspackages/apps-engine/src/server/managers/AppListenerManager.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/apps/server/bridges/listeners.tspackages/apps-engine/src/server/managers/AppListenerManager.ts
🔇 Additional comments (1)
packages/apps-engine/src/server/managers/AppListenerManager.ts (1)
360-366: Good fix: these branches now stop switch fallthrough and propagate listener failures.Returning the promises here makes
executeListener()wait on the post-listener handlers instead of resolving early and continuing into later cases.Also applies to: 393-410
Comment on lines
+249
to
+258
| return this.orch | ||
| .getManager() | ||
| .getListenerManager() | ||
| .executeListener(args.event, args.payload[0]) | ||
| .catch((error) => { | ||
| this.orch.getRocketChatLogger().error({ | ||
| msg: `AppListenerBridge: error handling ${args.event}`, | ||
| err: error, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify which default-event listeners flow through this branch.
rg -n -C3 'IUIKitInteractionHandler|IUIKitLivechatInteractionHandler|IPreEmailSent' \
apps/meteor/app/apps/server/bridges/listeners.ts \
packages/apps-engine
# Inspect executeListener contracts/callers for those handlers.
rg -n -C5 'executeListener\(' \
apps/meteor/app/apps/server/bridges/listeners.ts \
packages/apps-engine/src/server/managers/AppListenerManager.tsRepository: RocketChat/Rocket.Chat
Length of output: 35578
🏁 Script executed:
# Check how the return value from defaultEvent is used
rg -n -B2 -A2 'defaultEvent\s*\(' apps/meteor/app/apps/server/bridges/listeners.ts
# Check the executeListener method signatures to confirm return types
rg -n 'executeListener' packages/apps-engine/src/server/managers/AppListenerManager.ts | head -20
# Verify the actual implementation of executeUIKitInteraction and executePreEmailSent
rg -n -A10 'private async executeUIKitInteraction\|private async executePreEmailSent' packages/apps-engine/src/server/managers/AppListenerManager.tsRepository: RocketChat/Rocket.Chat
Length of output: 362
🏁 Script executed:
# Find where defaultEvent is called and how the return value is used
rg -n -B5 -A10 'defaultEvent' apps/meteor/app/apps/server/bridges/listeners.ts | grep -A15 'return this.defaultEvent'
# Check the method that calls defaultEvent to see context
rg -n -B20 'return this.defaultEvent' apps/meteor/app/apps/server/bridges/listeners.ts | head -50
# Verify AppListenerManager executeListener return type
rg -n 'executeListener.*Promise' packages/apps-engine/src/server/managers/AppListenerManager.tsRepository: RocketChat/Rocket.Chat
Length of output: 1493
🏁 Script executed:
# Get more context on how UIKit handlers are executed
rg -n -A15 'executeUIKitInteraction\(' packages/apps-engine/src/server/managers/AppListenerManager.ts
# Check if responses from executeListener are validated or used
rg -n -B3 -A8 'executeUIKitInteraction\|executeUIKitLivechatInteraction\|executePreEmailSent' packages/apps-engine/src/server/managers/AppListenerManager.ts | head -80Repository: RocketChat/Rocket.Chat
Length of output: 1793
🏁 Script executed:
# Check the full executeUIKitInteraction and executeUIKitLivechatInteraction implementations
rg -n -A30 'private async executeUIKitInteraction\(' packages/apps-engine/src/server/managers/AppListenerManager.ts
# Check executePreEmailSent implementation
rg -n -A30 'private async executePreEmailSent\(' packages/apps-engine/src/server/managers/AppListenerManager.ts
# Find out what IUIKitResponse is and what callers expect
rg -n 'interface IUIKitResponse' packages/apps-engineRepository: RocketChat/Rocket.Chat
Length of output: 2485
🏁 Script executed:
# Check where the result of defaultEvent is used in the calling code
rg -n -B10 -A10 'case AppInterface.IUIKitInteractionHandler' apps/meteor/app/apps/server/bridges/listeners.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check what calls defaultEvent and how the result is used
rg -n 'event\(' apps/meteor/app/apps/server/bridges/listeners.ts | head -5
# Find the parent method that dispatches to defaultEvent
rg -n -B50 'return this.defaultEvent' apps/meteor/app/apps/server/bridges/listeners.ts | grep -A50 'async event'
# Check how the result of event method is handled
rg -n -B5 -A10 '\.event\(' apps/meteor/app/apps/server/bridges/listeners.tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look at the complete IUIKitResponse definition
rg -n -A10 'export interface IUIKitResponse' packages/apps-engineRepository: RocketChat/Rocket.Chat
Length of output: 1064
🏁 Script executed:
# Verify the full method signature and caller context
rg -n -B30 'async event' apps/meteor/app/apps/server/bridges/listeners.ts | head -80
# Check if there's a parent method that calls event() and how it uses the result
rg -n 'AppListenerBridge' apps/meteor/app/apps/server/bridges/listeners.ts | head -5Repository: RocketChat/Rocket.Chat
Length of output: 546
🏁 Script executed:
# Find the actual callers of the AppListenerBridge.event method
rg -n '\.event\(' apps/meteor --type ts | grep -v node_modules | head -20Repository: RocketChat/Rocket.Chat
Length of output: 553
🏁 Script executed:
# Find where AppListenerBridge methods are called
rg -n 'appListenerBridge\|AppListenerBridge' apps/meteor --type ts -A3 -B3 | grep -E 'event|defaultEvent' | head -30
# Check the orchestrator to see how it calls AppListenerBridge
rg -n 'new AppListenerBridge\|AppListenerBridge' apps/meteor/app/apps/server --type ts -B2 -A2
# Find callers of event() method on AppListenerBridge
fd -t f '\.ts$' apps/meteor/app/apps/server | xargs rg -l 'AppListenerBridge' | head -5Repository: RocketChat/Rocket.Chat
Length of output: 114
🏁 Script executed:
# Look at the orchestrator file to see how event() is called
rg -n '\.event\(' packages/apps-engine/src/server/AppServerOrchestrator.ts | head -10
# Get full context of how event method return is used
rg -n -B5 -A15 'async event.*HandleDefaultEvent' apps/meteor/app/apps/server/bridges/listeners.tsRepository: RocketChat/Rocket.Chat
Length of output: 162
🏁 Script executed:
# Find the event method definition more carefully
rg -n -B5 'async event\(' apps/meteor/app/apps/server/bridges/listeners.ts | head -40
# Find all calls to AppListenerBridge in the codebase
rg -n 'listenerBridge\|AppListenerBridge' apps/meteor/app/apps/server --type ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Look for where the return value of event() is actually used
rg -n '\.event\(' apps/meteor/app/apps/server --type ts -B2 -A5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Read the full listeners.ts file to understand the event method structure
wc -l apps/meteor/app/apps/server/bridges/listeners.ts
# Get the event method definition
sed -n '100,250p' apps/meteor/app/apps/server/bridges/listeners.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 5864
🏁 Script executed:
# Search for how this bridge is instantiated and used
rg -n 'listenerBridge' apps/meteor/app/apps/server --type ts -B3 -A3 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Get the complete defaultEvent implementation
sed -n '149,160p' apps/meteor/app/apps/server/bridges/listeners.tsRepository: RocketChat/Rocket.Chat
Length of output: 431
🏁 Script executed:
# Verify the learnings about retry UI in sendMessage to understand the pattern
cat -n apps/meteor/client/lib/chats/flows/sendMessage.ts | sed -n '35,50p'Repository: RocketChat/Rocket.Chat
Length of output: 565
🏁 Script executed:
# Get the complete code snippet for lines 249-258
sed -n '248,259p' apps/meteor/app/apps/server/bridges/listeners.tsRepository: RocketChat/Rocket.Chat
Length of output: 404
🏁 Script executed:
# Verify that there's no return statement in the catch block
sed -n '253,258p' apps/meteor/app/apps/server/bridges/listeners.ts | grep -E 'return|throw'Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Verify how the return value from defaultEvent is actually used by tracing the call path
rg -n 'handleEvent' apps/meteor/app/apps/server --type ts -B2 -A5 | head -40
# Check if any consuming code validates the IUIKitResponse or IEmailDescriptor
rg -n 'IUIKitResponse|IEmailDescriptor' apps/meteor/app/apps/server --type ts -B3 -A3 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 764
Don't convert listener failures into undefined for request/response handlers.
The .catch() block in defaultEvent logs the error but implicitly returns undefined when an error occurs. This breaks the contract for IUIKitInteractionHandler, IUIKitLivechatInteractionHandler, and IPreEmailSent, which expect typed responses:
IUIKitInteractionHandlerandIUIKitLivechatInteractionHandlermust returnIUIKitResponse(withsuccess: boolean) to communicate back to the UIIPreEmailSentmust returnIEmailDescriptorto allow email modification before sending
Returning undefined instead of the required response type will cause type mismatches and silent failures. Either rethrow the error after logging, or split these interactive handlers into a separate method path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/apps/server/bridges/listeners.ts` around lines 249 - 258, The
current catch on executeListener in defaultEvent swallows errors and returns
undefined, breaking contracts for handlers like IUIKitInteractionHandler,
IUIKitLivechatInteractionHandler and IPreEmailSent that must return concrete
responses; update defaultEvent (or the code path that calls
this.orch.getManager().getListenerManager().executeListener) to log the error
via this.orch.getRocketChatLogger().error(...) and then rethrow the error (or
propagate it) so callers receive the failure instead of undefined, or
alternatively detect those interactive handler types
(IUIKitInteractionHandler/IUIKitLivechatInteractionHandler/IPreEmailSent) and
handle them in a separate path that returns a proper
IUIKitResponse/IEmailDescriptor even on error; ensure you reference
executeListener and defaultEvent when making the change.
Comment on lines
+410
to
+415
| return this.orch.getManager().getListenerManager().executeListener(args.event, msg).catch((error) => { | ||
| this.orch.getRocketChatLogger().error({ | ||
| msg: `AppListenerBridge: error handling ${args.event}`, | ||
| err: error, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the result contracts for pre-message/pre-room listeners.
rg -n -C4 'IPreMessageSentPrevent|IPreMessageSentExtend|IPreMessageSentModify|IPreMessageDeletePrevent|IPreMessageUpdatedPrevent|IPreMessageUpdatedExtend|IPreMessageUpdatedModify|IPreRoomCreatePrevent|IPreRoomCreateExtend|IPreRoomCreateModify|IPreRoomDeletePrevent' \
packages/apps-engine/src/server/managers/AppListenerManager.ts
# Verify that livechat pre-create listeners propagate failures.
rg -n -C4 'executePreLivechatRoomCreatePrevent|IPreLivechatRoomCreatePrevent' \
packages/apps-engine/src/server/managers/AppListenerManager.ts
# Show the new catch blocks that currently swallow those failures.
rg -n -C3 '\.catch\(\(error\)\s*=>\s*\{' \
apps/meteor/app/apps/server/bridges/listeners.tsRepository: RocketChat/Rocket.Chat
Length of output: 17380
🏁 Script executed:
cat -n apps/meteor/app/apps/server/bridges/listeners.ts | head -450 | tail -100Repository: RocketChat/Rocket.Chat
Length of output: 4006
🏁 Script executed:
cat -n apps/meteor/app/apps/server/bridges/listeners.ts | sed -n '200,420p'Repository: RocketChat/Rocket.Chat
Length of output: 9148
🏁 Script executed:
cat -n apps/meteor/app/apps/server/bridges/listeners.ts | sed -n '430,480p'Repository: RocketChat/Rocket.Chat
Length of output: 2222
🏁 Script executed:
cat -n apps/meteor/app/apps/server/bridges/listeners.ts | sed -n '600,680p'Repository: RocketChat/Rocket.Chat
Length of output: 3573
🏁 Script executed:
cat -n apps/meteor/app/apps/server/bridges/listeners.ts | sed -n '480,610p'Repository: RocketChat/Rocket.Chat
Length of output: 5348
🏁 Script executed:
# Let's verify what happens with the pre-hooks by checking how they are processed in AppListenerManager
cat -n packages/apps-engine/src/server/managers/AppListenerManager.ts | sed -n '465,530p'Repository: RocketChat/Rocket.Chat
Length of output: 2404
🏁 Script executed:
# Check the executeListener method to understand the dispatch mechanism
cat -n packages/apps-engine/src/server/managers/AppListenerManager.ts | sed -n '340,430p'Repository: RocketChat/Rocket.Chat
Length of output: 5347
Pre-listener failures now fail open.
The pre-event hooks (IPreMessageSentPrevent/Extend/Modify, IPreMessageDeletePrevent, IPreMessageUpdatedPrevent/Extend/Modify, IPreRoomCreatePrevent/Extend/Modify, IPreRoomDeletePrevent, IPreLivechatRoomCreatePrevent) all route through the default switch cases in messageEvent() (line 410), roomEvent() (line 472), and livechatEvent() (line 620), where their errors are caught and logged while returning undefined. This breaks the return-type contract:
- Prevent hooks expect
boolean:undefinedis falsy, so blocked operations proceed. - Extend/Modify hooks expect entity:
undefineddiscards transformations. - Pre-livechat-create expects
voidwith side effects: errors are silently suppressed.
Only post-event hooks (which have explicit catch blocks with proper side-effect patterns) should swallow errors. Pre-hooks must rethrow after logging to preserve their veto/transform semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/apps/server/bridges/listeners.ts` around lines 410 - 415, The
current catch in the default switch path (used by messageEvent(), roomEvent(),
and livechatEvent()) logs errors and returns undefined, which breaks pre-hook
semantics; change the error handling so that for pre-event hooks (IPre* hooks
routed through those default cases) the catch still logs via
this.orch.getRocketChatLogger().error but then rethrows the error (or returns a
rejected promise) instead of swallowing it, while leaving post-event hook
behavior unchanged; locate the calls to
this.orch.getManager().getListenerManager().executeListener(...) in
messageEvent, roomEvent, and livechatEvent and update their .catch handlers to
log then rethrow for pre-hooks so prevent/extend/modify and pre-livechat-create
maintain their veto/transform side effects.
| case AppInterface.IPostMessageUpdated: | ||
| this.executePostMessageUpdated(data as IMessage); | ||
| return; | ||
| return this.executePostMessageUpdated(data as IMessage); |
Contributor
There was a problem hiding this comment.
IPostMessageUpdated still returns the wrong type.
This branch now correctly returns, but IListenerExecutor[AppInterface.IPostMessageUpdated].result is still IMessage while executePostMessageUpdated() resolves void. That leaves executeListener(AppInterface.IPostMessageUpdated, ...) advertising a message and actually yielding undefined.
Suggested fix
[AppInterface.IPostMessageUpdated]: {
args: [IMessage];
- result: IMessage;
+ result: void;
};If callers are supposed to receive the updated message back, then executePostMessageUpdated() should return it explicitly instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/apps-engine/src/server/managers/AppListenerManager.ts` at line 374,
The IListenerExecutor[AppInterface.IPostMessageUpdated].result type mismatches
executePostMessageUpdated() (which currently resolves void) causing
executeListener(AppInterface.IPostMessageUpdated, ...) to advertise an IMessage
but actually return undefined; fix by making executePostMessageUpdated(...)
explicitly return the updated IMessage (or adjust the IListenerExecutor generic
for AppInterface.IPostMessageUpdated to void if callers should not receive a
message). Locate executePostMessageUpdated and update its
implementation/signature to return the updated IMessage (ensure any internal
awaits/callbacks produce and return that IMessage) and update any related type
declarations (IListenerExecutor[AppInterface.IPostMessageUpdated].result) so
they match the actual return type used by executeListener.
d-gubert
previously requested changes
Mar 11, 2026
Member
There was a problem hiding this comment.
This file is out of scope for this PR, please revert
Contributor
Author
There was a problem hiding this comment.
just did sir, is that fine?
Member
There was a problem hiding this comment.
You seem to have actually mixed this up with your other PR, based on this commit 44f036a
Contributor
Author
There was a problem hiding this comment.
You're right, my apologies! I've now removed commit 44f036a entirely from this branch via rebase. The branch now only contains the integrations API changes. AppListenerManager.ts is identical to develop
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/apps-engine/src/server/managers/AppListenerManager.ts">
<violation number="1">
P1: `executeListener` now fire-and-forgets several async post-event handlers, causing early resolution and loss of error propagation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
smirk-dev
force-pushed
the
refactor/integrations-api-chained-pattern
branch
from
25f2cde to
59ba26f
Compare
Member
|
Hey @smirk-dev, thank you for your contribution! 🙏 This endpoint has already been migrated as part of #39553, which consolidated all OpenAPI migrations to make it easier to test and validate everything as a single unit. That PR has been merged into |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Continues the ongoing REST API migration effort by converting all 6
integrations.*endpoints from the deprecatedAPI.v1.addRoute()pattern to the new chained router pattern (API.v1.get().post().put()).This follows the same pattern established in already-migrated files like
permissions.ts,ldap.ts, andcall-history.ts.Changes
integrations.createaddRoute.post()integrations.historyaddRoute.get()integrations.listaddRoute.get()integrations.removeaddRoute+ inlinecheck().post()integrations.getaddRoute.get()integrations.updateaddRoute.put()Key improvements
meteor/check— the inlinecheck(Match.ObjectIncluding({ integrationId: String }))inintegrations.removewas redundant;isIntegrationsRemovePropsalready validates this. Removes the Meteor dependency from this file.IntegrationsEndpointsis removed from the centralizedrest-typingsEndpointsinterface and is now declared viadeclare module+ExtractRoutesFromAPIdirectly in the route file, consistent with the pattern used by other migrated endpoints.Type safety
Zero TypeScript errors after migration. The
export * from './v1/integrations'inrest-typings/src/index.tsis preserved, so all validator functions (isIntegrationsCreateProps, etc.) remain publicly available.Summary by CodeRabbit
Refactor
Documentation
Bug Fixes