refactor: Stricten types over UiKit implementation APIs#38804
refactor: Stricten types over UiKit implementation APIs#38804
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughRefactors UiKit-related public types: splits a generic UiKitCoreAppPayload into three specific payload types, tightens function signatures and return types across core services and modules, consolidates UI interaction routing, and narrows integration/webhook user typing and a video-conference return shape. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38804 +/- ##
===========================================
+ Coverage 70.58% 70.63% +0.05%
===========================================
Files 3180 3180
Lines 111934 111939 +5
Branches 20245 20234 -11
===========================================
+ Hits 79012 79072 +60
+ Misses 30868 30814 -54
+ Partials 2054 2053 -1
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/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "02/12 22:57", "02/13 22:38", "02/16 14:04", "02/18 23:15", "02/19 15:16", "02/19 16:54 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 19 Feb 2026 16:54:58 GMT |
|
/jira ARCH-1935 |
tassoevan
force-pushed
the
refactor/uikit-coreapp-types
branch
from
f547885 to
112f0e7
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 16 files
Prompt for AI agents (all 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/rest-typings/src/apps/index.ts">
<violation number="1" location="packages/rest-typings/src/apps/index.ts:260">
P2: `Record<never, never>` widens the return type to `{}` and makes the `UiKit.ServerInteraction` branch redundant, which defeats the stricter typing this change aims for. Use a stricter empty-object type (e.g., `Record<string, never>`) or a nullable type if the response can be empty.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/app/integrations/server/api/api.ts (1)
206-208:⚠️ Potential issue | 🟠 MajorType safety issue:
result.userbypassesusernamerequirement.The
processIncomingRequest()method explicitly returnsPromise<any>, soresult.useris typed asany. Assigninganytothis.user: RequiredField<IUser, 'username'>silently bypasses theusernamerequirement—TypeScript won't error, but a runtimeundefinedusername could be assigned. Add a type guard or assertion to ensureresult.userhas the requiredusernamefield before assignment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/integrations/server/api/api.ts` around lines 206 - 208, processIncomingRequest() returns Promise<any>, so assigning result.user to this.user: RequiredField<IUser,'username'> can bypass the required username; add a runtime/type guard before assignment to ensure result.user is an object with a non-empty username (e.g., check typeof result.user === 'object' && result.user?.username), or narrow the type via a userHasUsername(result.user) type predicate, and only assign to this.user when the check passes (otherwise handle the missing username path or throw/return an error).apps/meteor/server/modules/core-apps/nps.module.ts (1)
36-69:⚠️ Potential issue | 🟠 Major
userIdis validated afterNPS.vote— move the guard earlier.
NPS.voteon line 56 is called with a potentiallyundefineduserId(whenpayload.useris undefined). The!userIdguard on line 64 only protectsBanner.dismiss, not the vote itself.Suggested fix
async viewSubmit(payload: UiKitCoreAppViewSubmitPayload): Promise<undefined> { if (!payload.payload?.view?.state || !payload.payload?.view?.id) { throw new Error('Invalid payload'); } const { payload: { view: { state, id: viewId }, }, user: { _id: userId, roles } = {}, } = payload; + if (!userId) { + throw new Error('invalid user'); + } + const [npsId] = Object.keys(state); const bannerId = viewId.replace(`${npsId}-`, ''); const { [npsId]: { 'nps-score': score, comment }, } = state; await NPS.vote({ npsId, userId, comment: String(comment), roles, score: Number(score), }); - if (!userId) { - throw new Error('invalid user'); - } - await Banner.dismiss(userId, bannerId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/modules/core-apps/nps.module.ts` around lines 36 - 69, The code currently calls NPS.vote with a possibly undefined userId; move the user existence guard so it runs immediately after destructuring payload.user (after extracting userId and roles) and before invoking NPS.vote — if userId is falsy throw the same 'invalid user' Error; then proceed to call NPS.vote and Banner.dismiss as before (use the existing userId, npsId, score, comment, and roles variables).
🧹 Nitpick comments (5)
apps/meteor/ee/server/apps/communication/uikit.ts (1)
143-144: Remove inline comments from implementation code.Lines 143, 161, and 185 add comments explaining the
??operator behavior. As per coding guidelines, avoid code comments in the implementation.Suggested diff
- // Using ?? to always send something in the response, even if the app had no result. res.send(result ?? {});(Apply the same removal at lines 161 and 185.)
Also applies to: 161-162, 185-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/apps/communication/uikit.ts` around lines 143 - 144, Remove the inline explanatory comments that sit directly above the response sends; specifically delete the comment lines that reference the "Using ?? to always send something..." and similar comments around the res.send(result ?? {}) calls so the implementation contains only the expression calls (occurrences to update: the res.send(result ?? {}) calls in this file at the three spots where those comments appear). Keep the res.send(result ?? {}) behavior unchanged—only remove the comment lines.packages/core-services/src/types/IUiKitCoreApp.ts (1)
6-28: Well-structured payload types — one concern about visitor type duplication.The three specialized payload types provide clear contracts. Note that the
visitorshape (lines 13–26) is duplicated verbatim inapps/meteor/ee/server/apps/communication/uikit.ts(lines 98–110). Consider extracting a sharedUiKitVisitortype incore-servicesorcore-typingsto avoid drift between the two definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core-services/src/types/IUiKitCoreApp.ts` around lines 6 - 28, Extract the duplicated visitor shape into a shared type (e.g., UiKitVisitor) and replace the inline visitor union in UiKitCoreAppBlockActionPayload with this new type; update the other occurrence in apps/meteor/ee/server/apps/communication/uikit.ts to import and use UiKitVisitor as well so both places reference the single definition (ensure fields: id, username, name?, department?, updatedAt?, token, phone?, visitorEmails?, livechatData?, status? are preserved and exported from core-services or core-typings).apps/meteor/server/services/uikit-core-app/service.ts (1)
33-60:if (!service)guards are unreachable —getAppModulealready throws on missing modules.
getAppModule(line 16) throws'invalid service name'when the module is not found, so the null checks on lines 37, 46, and 55 can never be reached. Additionally,blockAction/viewClosedreturnundefinedwhileviewSubmituses a barereturn— these should be consistent if kept.Simplify by removing dead guards
async blockAction(payload: UiKitCoreAppBlockActionPayload) { const { appId } = payload; - const service = getAppModule(appId); - if (!service) return undefined; - return service.blockAction?.(payload); } async viewClosed(payload: UiKitCoreAppViewClosedPayload) { const { appId } = payload; - const service = getAppModule(appId); - if (!service) return undefined; - return service.viewClosed?.(payload); } async viewSubmit(payload: UiKitCoreAppViewSubmitPayload) { const { appId } = payload; - const service = getAppModule(appId); - if (!service) { - return; - } - return service.viewSubmit?.(payload); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/uikit-core-app/service.ts` around lines 33 - 60, The null-check guards after calling getAppModule in blockAction, viewClosed, and viewSubmit are dead code because getAppModule already throws on a missing module; remove those if (!service) checks and simply call and return the optional handlers directly (e.g., return service.blockAction?.(payload), return service.viewClosed?.(payload), return service.viewSubmit?.(payload)); if you prefer to keep guards for clarity, make their return behavior consistent across all three methods (use explicit return undefined rather than a bare return). Ensure references to getAppModule, blockAction, viewClosed, and viewSubmit are updated accordingly.apps/meteor/server/modules/core-apps/mention.module.ts (1)
103-103: Unsafe cast toIUser & { username: string }—usernameis optional onIUser.If the guard from the suggestion above is added, consider also validating
user.usernameto avoid a runtime mismatch withprocessWebhookMessage's expectations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/modules/core-apps/mention.module.ts` at line 103, The code currently force-casts `user` to `IUser & { username: string }` which is unsafe because `IUser.username` is optional; instead, add a runtime guard that checks `user` exists and `typeof user.username === 'string'` before casting or passing to `processWebhookMessage`, and if the check fails return/throw an appropriate error; update the call site that uses the casted value (the `user as IUser & { username: string }` occurrence and the `processWebhookMessage` invocation) to only pass the user after the guard so the type matches runtime guarantees.apps/meteor/server/modules/core-apps/cloudAnnouncements.module.ts (1)
149-252:as anycasts on view objects (lines 224, 244) weaken the type safety this PR aims to improve.These casts exist to bridge the internal payload view format to the
UiKit.UserInteractionshape. Consider introducing a narrower intermediate type or extending the UiKit types to avoidanyhere, even if deferred to a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/modules/core-apps/cloudAnnouncements.module.ts` around lines 149 - 252, The getInteraction function uses unsafe "as any" casts for view objects in the 'viewClosed' and 'viewSubmit' branches which bypass type safety; instead define a narrow intermediate interface matching the internal payload view shape (or extend UiKit types) and map/convert the payload.view to that interface before assigning to the returned UiKit.UserInteraction object (i.e., in getInteraction, replace view as any with a typed local variable like internalView: InternalView and use only the explicitly mapped fields such as viewId/view and any allowed properties), ensuring you update return objects in the viewClosed and viewSubmit branches to use the typed value rather than any.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/meteor/app/integrations/server/api/api.tsapps/meteor/app/integrations/server/lib/triggerHandler.tsapps/meteor/app/lib/server/functions/processWebhookMessage.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/server/modules/core-apps/banner.module.tsapps/meteor/server/modules/core-apps/cloudAnnouncements.module.tsapps/meteor/server/modules/core-apps/cloudSubscriptionCommunication.module.tsapps/meteor/server/modules/core-apps/mention.module.tsapps/meteor/server/modules/core-apps/nps.module.tsapps/meteor/server/modules/core-apps/videoconf.module.tsapps/meteor/server/services/uikit-core-app/service.tsapps/meteor/server/services/video-conference/service.tspackages/core-services/src/index.tspackages/core-services/src/types/IUiKitCoreApp.tspackages/core-services/src/types/IVideoConfService.tspackages/rest-typings/src/apps/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/rest-typings/src/apps/index.tspackages/core-services/src/types/IVideoConfService.tspackages/core-services/src/index.tsapps/meteor/server/modules/core-apps/banner.module.tsapps/meteor/server/modules/core-apps/cloudSubscriptionCommunication.module.tsapps/meteor/app/lib/server/functions/processWebhookMessage.tsapps/meteor/server/services/video-conference/service.tspackages/core-services/src/types/IUiKitCoreApp.tsapps/meteor/app/integrations/server/lib/triggerHandler.tsapps/meteor/server/modules/core-apps/mention.module.tsapps/meteor/ee/server/apps/communication/uikit.tsapps/meteor/server/modules/core-apps/nps.module.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/server/modules/core-apps/videoconf.module.tsapps/meteor/server/modules/core-apps/cloudAnnouncements.module.tsapps/meteor/server/services/uikit-core-app/service.ts
🧠 Learnings (12)
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/server/modules/core-apps/cloudSubscriptionCommunication.module.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/functions/processWebhookMessage.tsapps/meteor/app/integrations/server/lib/triggerHandler.tsapps/meteor/server/modules/core-apps/mention.module.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/server/modules/core-apps/cloudAnnouncements.module.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/functions/processWebhookMessage.tsapps/meteor/server/modules/core-apps/mention.module.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/lib/server/functions/processWebhookMessage.tsapps/meteor/server/modules/core-apps/mention.module.ts
📚 Learning: 2025-12-18T15:18:31.688Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37773
File: apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx:24-34
Timestamp: 2025-12-18T15:18:31.688Z
Learning: In apps/meteor/client/views/mediaCallHistory/MediaCallHistoryInternal.tsx, for internal call history items, the item.contactId is guaranteed to always match either the caller.id or callee.id in the call data, so the contact resolution in getContact will never result in undefined.
Applied to files:
apps/meteor/server/services/video-conference/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/app/integrations/server/lib/triggerHandler.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/integrations/server/lib/triggerHandler.ts
📚 Learning: 2026-01-19T18:17:46.433Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
Applied to files:
apps/meteor/app/integrations/server/lib/triggerHandler.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/server/modules/core-apps/mention.module.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/server/modules/core-apps/mention.module.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/modules/core-apps/mention.module.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: In Rocket.Chat test files, 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). It does not accept slug-like constructions such as concatenating usernames together.
Applied to files:
apps/meteor/server/modules/core-apps/mention.module.ts
🧬 Code graph analysis (7)
apps/meteor/server/modules/core-apps/banner.module.ts (2)
packages/core-services/src/index.ts (2)
IUiKitCoreApp(130-130)UiKitCoreAppViewClosedPayload(128-128)packages/core-services/src/types/IUiKitCoreApp.ts (2)
IUiKitCoreApp(48-54)UiKitCoreAppViewClosedPayload(30-41)
apps/meteor/server/modules/core-apps/cloudSubscriptionCommunication.module.ts (2)
apps/meteor/server/modules/core-apps/cloudAnnouncements.module.ts (1)
CloudAnnouncementsModule(32-293)packages/core-services/src/types/IUiKitCoreApp.ts (1)
UiKitCoreAppViewClosedPayload(30-41)
apps/meteor/app/lib/server/functions/processWebhookMessage.ts (1)
packages/core-typings/src/utils.ts (1)
RequiredField(23-23)
packages/core-services/src/types/IUiKitCoreApp.ts (2)
packages/core-services/src/index.ts (5)
UiKitCoreAppBlockActionPayload(127-127)UiKitCoreAppViewClosedPayload(128-128)UiKitCoreAppViewSubmitPayload(129-129)IUiKitCoreApp(130-130)IUiKitCoreAppService(131-131)apps/meteor/app/search/server/model/SearchProvider.ts (1)
key(31-33)
apps/meteor/app/integrations/server/lib/triggerHandler.ts (2)
apps/meteor/app/lib/server/functions/processWebhookMessage.ts (1)
processWebhookMessage(144-204)packages/core-typings/src/utils.ts (1)
RequiredField(23-23)
apps/meteor/ee/server/apps/communication/uikit.ts (2)
packages/rest-typings/src/index.ts (1)
OperationResult(191-193)packages/core-services/src/index.ts (1)
UiKitCoreApp(166-166)
apps/meteor/app/integrations/server/api/api.ts (1)
packages/core-typings/src/utils.ts (1)
RequiredField(23-23)
⏰ 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 (16)
apps/meteor/app/integrations/server/lib/triggerHandler.ts (1)
184-184: Type assertion simplified correctly.The old
user as IUser & { username: RequiredField<IUser, 'username'> }expanded to{ username: IUser & { username: string } }, makingusernamethe wrong type (IUser). The newuser as RequiredField<IUser, 'username'>correctly expands toIUser & { username: string }, and the assertion is safe sinceuseris always fetched viafindOneByUsernameIgnoringCase.apps/meteor/app/integrations/server/api/api.ts (1)
51-51: Type simplification is correct and consistent with the updatedprocessWebhookMessagesignature.apps/meteor/app/lib/server/functions/processWebhookMessage.ts (1)
132-153: Type fix is accurate —usernameis now correctlystringinstead ofIUser & {...}.The previous
IUser & { username: RequiredField<IUser, 'username'> }expandedusernametoIUser & { username: string }(anIUserobject) rather than astring.RequiredField<IUser, 'username'>correctly resolves toIUser & { username: string }. All three overload sites are consistently updated.apps/meteor/server/modules/core-apps/banner.module.ts (1)
2-9: LGTM — type narrowing is valid and accurate.
Promise<UiKit.ServerInteraction>is a valid covariant override of the interface'sPromise<UiKit.ServerInteraction | undefined>, and the implementation never actually returnsundefined(it either throws or returns a value), so the narrower return type is semantically accurate.packages/rest-typings/src/apps/index.ts (1)
259-261: Good improvement —Record<string, never>correctly models the empty-response case.
Record<string, never>is the canonical TypeScript empty-object type and is preferable to{}(which accepts any non-nullish value) orany. The unionUiKit.ServerInteraction | Record<string, never>accurately models the two observable response shapes.apps/meteor/server/modules/core-apps/cloudSubscriptionCommunication.module.ts (1)
1-9: LGTM — mirrors the same valid covariant return type pattern asBannerModule.The
overridekeyword correctly narrowsPromise<UiKit.ServerInteraction | undefined>(parent) toPromise<UiKit.ServerInteraction>(child), which TypeScript allows and which is accurate given the method always throws or returns.packages/core-services/src/index.ts (1)
127-129: No internal consumers ofUiKitCoreAppPayloadexist, but verify external package compatibility.The removal of
UiKitCoreAppPayloadfrom public exports is a breaking change for external consumers of@rocket.chat/core-services. While no internal code in this repository depends on it, users of this package importingUiKitCoreAppPayloadwill experience compilation failures.packages/core-services/src/types/IVideoConfService.ts (1)
24-24: Type change verified as safe and appropriate.
UiKit.ModalSurfaceLayoutis properly defined inpackages/ui-kit/src/surfaces/modal/UiKitParserModal.tsasModalSurfaceLayoutBlock[], a narrower subset of genericLayoutBlock[](restricted to ActionsBlock, ContextBlock, DividerBlock, ImageBlock, InputBlock, SectionBlock, and CalloutBlock).The return type change from
Promise<UiKit.LayoutBlock[]>toPromise<UiKit.ModalSurfaceLayout>is safe narrowing. The single call site atapps/meteor/server/modules/core-apps/videoconf.module.ts:27uses the result in amodal.openaction, which is semantically compatible. The implementation cast is appropriate and type-safe.apps/meteor/server/modules/core-apps/videoconf.module.ts (1)
1-56: LGTM — type tightening aligns with the new payload types.The updated import and signature correctly adopt
UiKitCoreAppBlockActionPayloadandUiKit.ServerInteraction | undefined. The addition ofappIdandblockIdon the close button (lines 43–44) rounds out the stricter typing for button elements. The implicitundefinedreturn for non-'info'actions matches the interface contract.apps/meteor/ee/server/apps/communication/uikit.ts (2)
113-198: Solid refactor — core-app routing extracted cleanly withnext()fallthrough.The
isCoreAppguard at lines 116–118 correctly short-circuits tonext()for non-core apps, and theswitchoverreq.body.typeexhaustively handles the three interaction types with a clear default error. Theresult ?? {}pattern aligns with the REST typing that returnsServerInteraction | Record<string, never>.
209-212: Response typing tightened — looks correct.The
routeHandlerresponse type now explicitly usesOperationResult<'POST', '/apps/ui.interaction/:id'>, ensuring type-safe responses for the Apps Engine handler path.packages/core-services/src/types/IUiKitCoreApp.ts (2)
48-61: Interface definitions are clean and consistent.Optional methods on
IUiKitCoreAppand required methods onIUiKitCoreAppService(with the service dispatching via optional chaining inservice.ts) is a correct pattern that allows core apps to implement only the handlers they need.
30-41:stateis correctly required onUiKitCoreAppViewClosedPayload.view.The
statefield is always initialized and passed in allviewClosedpayloads—it may be an empty object if no blocks have values, but it is never absent. TheisClearedflag (line 39) indicates whether the view's content was cleared from the UI before closing, not whether state is missing. The type definition accurately reflects the implementation.Likely an incorrect or invalid review comment.
apps/meteor/server/modules/core-apps/cloudAnnouncements.module.ts (2)
43-92: Clean adaptation of the three specialized payload types — LGTM.
blockActionandviewSubmitcorrectly delegate tohandlePayload, whileviewClosedadds its own banner-dismiss logic before firing off the cloud notification. Return types are consistent with the interface contract.
118-144: Type guard for visitor is well done.The
'visitor' in payload && payload.visitorguard on line 131 correctly narrows the union toUiKitCoreAppBlockActionPayload(the only variant with avisitorfield) before accessing visitor properties.apps/meteor/server/services/video-conference/service.ts (1)
175-218: Return type change is correctly implemented —ModalSurfaceLayoutis a simple array type.The definition of
UiKit.ModalSurfaceLayoutisModalSurfaceLayoutBlock[](packages/ui-kit/src/surfaces/modal/UiKitParserModal.ts), whereModalSurfaceLayoutBlockis a union of valid block types includingSectionBlock. The empty-array return on line 182 is valid, the cast on line 205 is appropriate, and the literal section block on lines 208–217 is correctly typed. No changes needed.
🤖 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/server/modules/core-apps/mention.module.ts`:
- Around line 33-34: Replace the non-null assertions on payload.user and
payload.room by adding an early guard that checks whether payload.user and
payload.room are defined before proceeding; inside the function where user and
room are assigned (currently using const user = payload.user! and const room =
payload.room!), return early (or throw a controlled error/log and return) if
either is undefined, then safely assign const user = payload.user and const room
= payload.room for the rest of mention.module.ts logic so you preserve the
stricter typing and avoid runtime crashes.
---
Outside diff comments:
In `@apps/meteor/app/integrations/server/api/api.ts`:
- Around line 206-208: processIncomingRequest() returns Promise<any>, so
assigning result.user to this.user: RequiredField<IUser,'username'> can bypass
the required username; add a runtime/type guard before assignment to ensure
result.user is an object with a non-empty username (e.g., check typeof
result.user === 'object' && result.user?.username), or narrow the type via a
userHasUsername(result.user) type predicate, and only assign to this.user when
the check passes (otherwise handle the missing username path or throw/return an
error).
In `@apps/meteor/server/modules/core-apps/nps.module.ts`:
- Around line 36-69: The code currently calls NPS.vote with a possibly undefined
userId; move the user existence guard so it runs immediately after destructuring
payload.user (after extracting userId and roles) and before invoking NPS.vote —
if userId is falsy throw the same 'invalid user' Error; then proceed to call
NPS.vote and Banner.dismiss as before (use the existing userId, npsId, score,
comment, and roles variables).
---
Nitpick comments:
In `@apps/meteor/ee/server/apps/communication/uikit.ts`:
- Around line 143-144: Remove the inline explanatory comments that sit directly
above the response sends; specifically delete the comment lines that reference
the "Using ?? to always send something..." and similar comments around the
res.send(result ?? {}) calls so the implementation contains only the expression
calls (occurrences to update: the res.send(result ?? {}) calls in this file at
the three spots where those comments appear). Keep the res.send(result ?? {})
behavior unchanged—only remove the comment lines.
In `@apps/meteor/server/modules/core-apps/cloudAnnouncements.module.ts`:
- Around line 149-252: The getInteraction function uses unsafe "as any" casts
for view objects in the 'viewClosed' and 'viewSubmit' branches which bypass type
safety; instead define a narrow intermediate interface matching the internal
payload view shape (or extend UiKit types) and map/convert the payload.view to
that interface before assigning to the returned UiKit.UserInteraction object
(i.e., in getInteraction, replace view as any with a typed local variable like
internalView: InternalView and use only the explicitly mapped fields such as
viewId/view and any allowed properties), ensuring you update return objects in
the viewClosed and viewSubmit branches to use the typed value rather than any.
In `@apps/meteor/server/modules/core-apps/mention.module.ts`:
- Line 103: The code currently force-casts `user` to `IUser & { username: string
}` which is unsafe because `IUser.username` is optional; instead, add a runtime
guard that checks `user` exists and `typeof user.username === 'string'` before
casting or passing to `processWebhookMessage`, and if the check fails
return/throw an appropriate error; update the call site that uses the casted
value (the `user as IUser & { username: string }` occurrence and the
`processWebhookMessage` invocation) to only pass the user after the guard so the
type matches runtime guarantees.
In `@apps/meteor/server/services/uikit-core-app/service.ts`:
- Around line 33-60: The null-check guards after calling getAppModule in
blockAction, viewClosed, and viewSubmit are dead code because getAppModule
already throws on a missing module; remove those if (!service) checks and simply
call and return the optional handlers directly (e.g., return
service.blockAction?.(payload), return service.viewClosed?.(payload), return
service.viewSubmit?.(payload)); if you prefer to keep guards for clarity, make
their return behavior consistent across all three methods (use explicit return
undefined rather than a bare return). Ensure references to getAppModule,
blockAction, viewClosed, and viewSubmit are updated accordingly.
In `@packages/core-services/src/types/IUiKitCoreApp.ts`:
- Around line 6-28: Extract the duplicated visitor shape into a shared type
(e.g., UiKitVisitor) and replace the inline visitor union in
UiKitCoreAppBlockActionPayload with this new type; update the other occurrence
in apps/meteor/ee/server/apps/communication/uikit.ts to import and use
UiKitVisitor as well so both places reference the single definition (ensure
fields: id, username, name?, department?, updatedAt?, token, phone?,
visitorEmails?, livechatData?, status? are preserved and exported from
core-services or core-typings).
| const user = payload.user!; | ||
| const room = payload.room!; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Non-null assertions on payload.user and payload.room bypass the stricter typing this PR introduces.
Since the payload types now explicitly model user and room as possibly undefined, asserting them with ! silently opts out. An early guard would be more consistent with the PR's goal and safer at runtime.
Suggested fix
- const user = payload.user!;
- const room = payload.room!;
+ if (!payload.user || !payload.room) {
+ throw new Error('Mention bot - Invalid payload: missing user or room');
+ }
+
+ const user = payload.user;
+ const room = payload.room;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const user = payload.user!; | |
| const room = payload.room!; | |
| if (!payload.user || !payload.room) { | |
| throw new Error('Mention bot - Invalid payload: missing user or room'); | |
| } | |
| const user = payload.user; | |
| const room = payload.room; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/modules/core-apps/mention.module.ts` around lines 33 - 34,
Replace the non-null assertions on payload.user and payload.room by adding an
early guard that checks whether payload.user and payload.room are defined before
proceeding; inside the function where user and room are assigned (currently
using const user = payload.user! and const room = payload.room!), return early
(or throw a controlled error/log and return) if either is undefined, then safely
assign const user = payload.user and const room = payload.room for the rest of
mention.module.ts logic so you preserve the stricter typing and avoid runtime
crashes.
tassoevan
force-pushed
the
refactor/uikit-coreapp-types
branch
from
ac58f2f to
592ee1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/ee/server/apps/communication/uikit.ts (1)
143-146: Remove duplicate inline comments — violates the "Avoid code comments" guideline.The same comment
// Using ?? to always send something in the response, even if the app had no result.appears verbatim in all three cases. The?? {}operator is self-explanatory in context; the comment adds no value and is repeated three times.As per coding guidelines: "Avoid code comments in the implementation."
Also applies to: 161-165, 185-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/apps/communication/uikit.ts` around lines 143 - 146, Remove the redundant inline comment that precedes the res.send(result ?? {}) calls (the duplicate “// Using ?? to always send something in the response, even if the app had no result.”) in this file; the expression res.send(result ?? {}) is self-explanatory, so delete that comment in all occurrences around the res.send(result ?? {}) statements (including the other two identical instances) to comply with the "Avoid code comments" guideline.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/ee/server/apps/communication/uikit.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/ee/server/apps/communication/uikit.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/ee/server/apps/communication/uikit.ts (3)
209-212: LGTM — explicitResponse<>type onrouteHandleris a meaningful improvement.Pinning the response type to
Response<OperationResult<'POST', '/apps/ui.interaction/:id'> | { error: unknown }>gives TypeScript the surface to catch mismatchedres.send(...)payloads across the entire handler. This is the core goal of the PR and the signature is correctly aligned with the type used inUiKitUserInteractionRequest.
113-119: Implicit chaining between tworouter.post('/:id', ...)registrations is fragile.The anonymous handler (line 113) is registered at module-evaluation time;
AppUIKitInteractionApi.routeHandleris registered inside the constructor (line 206). The anonymous handler callsnext()to fall through torouteHandlerfor non-core apps. This works only while:
AppUIKitInteractionApiis always instantiated before any request arrives.AppUIKitInteractionApiis never instantiated more than once — each additional instantiation appends another copy ofrouteHandlerto the route chain, causing a double-res.send("Cannot set headers after they are sent") for every subsequent non-core-app request.However, no instantiations of
AppUIKitInteractionApiwere found in the codebase. If the class is unused or instantiated through dynamic mechanisms, this concern may not apply in practice. Verify whether this class is actually instantiated and whether the implicit chaining is a real problem.Consider asserting singleton instantiation or moving the anonymous handler into the constructor (after
routeHandler) to make the ordering and lifecycle explicit.
149-165: The review comment is incorrect. The type definitions explicitly excludevisitorfromUiKitCoreAppViewSubmitPayloadandUiKitCoreAppViewClosedPayload. OnlyUiKitCoreAppBlockActionPayloadincludesvisitor(line 13 ofpackages/core-services/src/types/IUiKitCoreApp.ts). The code inuikit.tscorrectly follows these type definitions:blockActionextracts and passesvisitorwhileviewSubmitandviewCloseddo not, as designed. All module implementations (banner, nps, cloudAnnouncements) only reference theuserfield, nevervisitor, confirming this is intentional.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/ee/server/apps/communication/uikit.ts`:
- Around line 116-119: Move the await UiKitCoreApp.isRegistered(appId) call
inside the existing try/catch that begins at line 121 so any rejection is
caught; specifically, call UiKitCoreApp.isRegistered(appId) within the try
block, then if it returns false call return next(), and if it throws catch the
error and pass it to Express by calling next(err) (or next() with appropriate
handling) to avoid an unhandled rejection causing the request to hang.
- Around line 191-193: The switch's default currently throws an Error causing a
500; change it to return an HTTP 400 response for unsupported action types
instead. In the default branch of the switch in
apps/meteor/ee/server/apps/communication/uikit.ts (the branch that now does
throw new Error('Type not supported')), replace the throw with logic that sends
a 400 response (status 400 and a clear body/message like "Type not supported")
so the handler behaves like routeHandler's 400 for actionButton and treats
unknown types as client errors.
---
Nitpick comments:
In `@apps/meteor/ee/server/apps/communication/uikit.ts`:
- Around line 143-146: Remove the redundant inline comment that precedes the
res.send(result ?? {}) calls (the duplicate “// Using ?? to always send
something in the response, even if the app had no result.”) in this file; the
expression res.send(result ?? {}) is self-explanatory, so delete that comment in
all occurrences around the res.send(result ?? {}) statements (including the
other two identical instances) to comply with the "Avoid code comments"
guideline.
1 participant
Proposed changes (including videos or screenshots)
It strictens types for user/server interactions with UiKit blocks.
Issue(s)
Steps to test or reproduce
Further comments
It was a weakness discovered while trying to convert some JavaScript modules under
@rocket.chat/livechat.Task: ARCH-1965
Summary by CodeRabbit