chore: Replace non-null assertions with proper error handling in RealAppsEngineUIHost#38755
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 |
|
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (7)📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2026-01-17T01:51:47.764ZApplied to files:
📚 Learning: 2025-09-19T15:15:04.642ZApplied to files:
📚 Learning: 2025-10-06T20:32:23.658ZApplied to files:
📚 Learning: 2026-02-10T16:32:42.586ZApplied to files:
⏰ 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)
🔇 Additional comments (4)
WalkthroughRemoved non-null assertions in RealAppsEngineUIHost, replaced them with explicit runtime checks and guarded user retrieval; members without usernames are filtered, avatar URL logic returns empty string when missing, and slugifiedName falls back to room id when name is absent. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/client/apps/RealAppsEngineUIHost.ts (1)
65-80: Good error handling — minor nit on redundant fallback.The explicit guards and descriptive errors are a clear improvement. One small observation:
this.getUserAvatarUrl(username)already returns''when the avatar URL is missing (lines 23-25), so the|| ''on line 78 is redundant. Not blocking, but removing it would be slightly cleaner.Proposed diff
return { id: _id, username, - avatarUrl: this.getUserAvatarUrl(username) || '', + avatarUrl: this.getUserAvatarUrl(username), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/apps/RealAppsEngineUIHost.ts` around lines 65 - 80, In getClientUserInfo(), remove the redundant fallback "|| ''" when calling this.getUserAvatarUrl(username) because getUserAvatarUrl already returns an empty string when no avatar exists; update the return object in getClientUserInfo() to set avatarUrl to this.getUserAvatarUrl(username) directly (no additional fallback) so the code is cleaner and avoids unnecessary duplication.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/apps/RealAppsEngineUIHost.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/client/apps/RealAppsEngineUIHost.ts
🧠 Learnings (5)
📚 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/client/apps/RealAppsEngineUIHost.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/client/apps/RealAppsEngineUIHost.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/client/apps/RealAppsEngineUIHost.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/client/apps/RealAppsEngineUIHost.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/apps/RealAppsEngineUIHost.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/apps/RealAppsEngineUIHost.ts (3)
20-32: LGTM!Clean replacement of the non-null assertion with an explicit guard and empty-string fallback. The existing URL-prefix logic is correctly preserved.
58-62: LGTM!Falling back to
idwhen the room name is missing is a sensible default for DMs or unnamed rooms.
45-53:⚠️ Potential issue | 🔴 CriticalAdd missing import for
IUser.The type predicate on line 46 references
IUser, which is not imported in this file. The type predicate syntaxmember is IUser & { username: string }requiresIUserto be in lexical scope. Add:import type { IUser } from '@rocket.chat/core-typings';The API endpoint
/v1/groups.membersis typed to returnmembers: IUser[]inpackages/rest-typings/src/v1/groups/groups.ts, and all other client-side files in this codebase follow the pattern of explicitly importingIUserfrom@rocket.chat/core-typingswhen using it.⛔ Skipped due to learnings
Learnt from: Dnouv Repo: RocketChat/Rocket.Chat PR: 37057 File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27 Timestamp: 2025-09-25T09:59:26.461Z Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).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.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.
🤖 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/client/apps/RealAppsEngineUIHost.ts`:
- Around line 65-80: In getClientUserInfo(), remove the redundant fallback "||
''" when calling this.getUserAvatarUrl(username) because getUserAvatarUrl
already returns an empty string when no avatar exists; update the return object
in getClientUserInfo() to set avatarUrl to this.getUserAvatarUrl(username)
directly (no additional fallback) so the code is cleaner and avoids unnecessary
duplication.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/client/apps/RealAppsEngineUIHost.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/client/apps/RealAppsEngineUIHost.ts
🧠 Learnings (5)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/client/apps/RealAppsEngineUIHost.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/client/apps/RealAppsEngineUIHost.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/client/apps/RealAppsEngineUIHost.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/client/apps/RealAppsEngineUIHost.ts
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/apps/RealAppsEngineUIHost.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). (2)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/client/apps/RealAppsEngineUIHost.ts (3)
20-32: Clean null-safety improvement for avatar URL resolution.Returning an empty string for a missing avatar URL is a sensible fallback that aligns with the interface contract.
65-73: Good explicit guards with descriptive error messages.Throwing meaningful errors ("User is not logged in", "Username is not available") instead of relying on non-null assertions is a clear improvement for debuggability.
58-62: Solid fallback forslugifiedName.Using
slugifiedName ?? idhandles the case where a room name is missing gracefully.
🤖 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/client/apps/RealAppsEngineUIHost.ts`:
- Around line 45-53: The code uses the IUser type in the type predicate inside
RealAppsEngineUIHost (the cachedMembers assignment/filter that uses "member is
IUser & { username: string }") but IUser is not imported; add a type-only import
"import type { IUser } from '@rocket.chat/core-typings';" near the other imports
so the predicate compiles and matches the REST /v1/groups.members IUser shape.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38755 +/- ##
===========================================
- Coverage 70.78% 70.48% -0.30%
===========================================
Files 3159 3176 +17
Lines 109364 111147 +1783
Branches 19671 20038 +367
===========================================
+ Hits 77415 78346 +931
- Misses 29920 30757 +837
- Partials 2029 2044 +15
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1 participant
Description
This PR addresses the
FIXMEcomment in client/apps/RealAppsEngineUIHost.ts by replacing unsafe non-null assertions (!) with proper null checks and error handling.Related Issue
Closes #38754
Changes Made
!with a check forundefined.cachedMembersto ensure only members with a validusernameare processed.slugifiedName(usesidifnameis missing).username.User is not logged in,Username is not available) instead of allowing property access on potentialundefinedvalues.// FIXMEcomment.Verification
npx tsc --noEmitchecks passed (verified type safety).eslintchecks passed (verified code style).Checklist
Summary by CodeRabbit