refactor: improve TypeScript safety in deleteMessage by removing unsafe user casting#39608
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 info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent 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)
WalkthroughAdded an explicit user existence check in deleteMessage and introduced a sanitized Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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. 📝 Coding Plan
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 |
There was a problem hiding this comment.
1 issue found across 1 file
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/lib/server/functions/deleteMessage.ts">
<violation number="1" location="apps/meteor/app/lib/server/functions/deleteMessage.ts:23">
P2: New guard requires `user.username` and `user.name` to be truthy, which is a behavioral change beyond the refactor and can reject users whose records lack those optional fields. This can break message deletion for legacy/service users that do not have both fields populated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/app/lib/server/functions/deleteMessage.ts (2)
86-91: Same indentation issue as above.🧹 Suggested formatting fix
- - await Messages.setAsDeletedByIdAndUser(message._id, { -_id: user._id, -username: user.username, -name: user.name, -}); + await Messages.setAsDeletedByIdAndUser(message._id, { + _id: user._id, + username: user.username, + name: user.name, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/functions/deleteMessage.ts` around lines 86 - 91, Indentation for the call to Messages.setAsDeletedByIdAndUser is inconsistent; reformat the block so the await and call are aligned and the object argument is indented correctly—locate the call to Messages.setAsDeletedByIdAndUser (using message._id and the user object with user._id, user.username, user.name) and adjust spacing so the await, function name, opening paren, and the object literal are consistently indented with the surrounding code style.
62-67: Inconsistent indentation in object literal.The object properties appear to be at the wrong indentation level. Consider aligning them consistently within the function call.
🧹 Suggested formatting fix
- - await Messages.cloneAndSaveAsHistoryById(message._id, { -_id: user._id, -username: user.username, -name: user.name, -}); + await Messages.cloneAndSaveAsHistoryById(message._id, { + _id: user._id, + username: user.username, + name: user.name, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/functions/deleteMessage.ts` around lines 62 - 67, The object literal passed to Messages.cloneAndSaveAsHistoryById has inconsistent indentation for its properties; reformat the call so the object keys (_id, username, name) are aligned consistently inside the function invocation (e.g., each property on its own line indented to the same level under the opening brace), keeping the call signature Messages.cloneAndSaveAsHistoryById(message._id, { ... }) and retaining the same property names user._id, user.username, user.name.
🤖 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/functions/deleteMessage.ts`:
- Around line 86-91: Indentation for the call to
Messages.setAsDeletedByIdAndUser is inconsistent; reformat the block so the
await and call are aligned and the object argument is indented correctly—locate
the call to Messages.setAsDeletedByIdAndUser (using message._id and the user
object with user._id, user.username, user.name) and adjust spacing so the await,
function name, opening paren, and the object literal are consistently indented
with the surrounding code style.
- Around line 62-67: The object literal passed to
Messages.cloneAndSaveAsHistoryById has inconsistent indentation for its
properties; reformat the call so the object keys (_id, username, name) are
aligned consistently inside the function invocation (e.g., each property on its
own line indented to the same level under the opening brace), keeping the call
signature Messages.cloneAndSaveAsHistoryById(message._id, { ... }) and retaining
the same property names user._id, user.username, user.name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5356475e-16dd-4b09-ba34-b5501854264e
📒 Files selected for processing (1)
apps/meteor/app/lib/server/functions/deleteMessage.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/functions/deleteMessage.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
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.
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/functions/deleteMessage.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/server/functions/deleteMessage.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/deleteMessage.ts
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/lib/server/functions/deleteMessage.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/lib/server/functions/deleteMessage.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/lib/server/functions/deleteMessage.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/lib/server/functions/deleteMessage.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/lib/server/functions/deleteMessage.ts
📚 Learning: 2026-03-11T16:46:55.955Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:55.955Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.
Applied to files:
apps/meteor/app/lib/server/functions/deleteMessage.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/app/lib/server/functions/deleteMessage.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/functions/deleteMessage.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/functions/deleteMessage.ts
🔇 Additional comments (1)
apps/meteor/app/lib/server/functions/deleteMessage.ts (1)
22-25:⚠️ Potential issue | 🔴 CriticalValidation is bypassed for direct callers of
deleteMessage, and thenamefield requirement will cause runtime errors.
deleteMessageis exported and called directly by multiple modules without the validation indeleteMessageValidatingPermission. Specifically:
- The SlackAdapter (line 981) queries the user with
{ projection: { username: 1 } }, excludingname, then passes it todeleteMessage- The Apps bridge converter uses
removeEmpty()which strips undefined fields, so user objects may lacknameAdditionally, the
IMessage['u']type definition marks only_idandusernameas required;nameis optional. However, lines 89 and 66 attempt to accessuser.namewithout checking if it exists. For the SlackAdapter caller,user.namewill be undefined, and passing this tosetAsDeletedByIdAndUserwill insertname: undefinedinto the message history.Move validation into
deleteMessageitself or enforce required user fields at the type level to prevent callers from bypassing validation.⛔ Skipped due to learnings
Learnt from: ahmed-n-abdeltwab Repo: RocketChat/Rocket.Chat PR: 39230 File: apps/meteor/app/api/server/v1/chat.ts:214-222 Timestamp: 2026-03-03T11:11:48.541Z Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.Learnt from: juliajforesti Repo: RocketChat/Rocket.Chat PR: 39545 File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61 Timestamp: 2026-03-11T22:04:20.529Z Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.Learnt from: ahmed-n-abdeltwab Repo: RocketChat/Rocket.Chat PR: 39340 File: apps/meteor/app/api/server/v1/im.ts:1349-1398 Timestamp: 2026-03-12T10:26:26.697Z Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.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.Learnt from: tassoevan Repo: RocketChat/Rocket.Chat PR: 39304 File: packages/ui-contexts/src/ActionManagerContext.ts:26-26 Timestamp: 2026-03-04T14:16:49.202Z Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.Learnt from: sampaiodiego Repo: RocketChat/Rocket.Chat PR: 39003 File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45 Timestamp: 2026-02-24T19:16:35.307Z Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.Learnt from: ricardogarim Repo: RocketChat/Rocket.Chat PR: 39535 File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249 Timestamp: 2026-03-11T16:46:55.955Z Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.Learnt from: gabriellsh Repo: RocketChat/Rocket.Chat PR: 37419 File: apps/meteor/server/services/media-call/service.ts:141-141 Timestamp: 2025-11-19T18:20:37.116Z Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.Learnt from: juliajforesti Repo: RocketChat/Rocket.Chat PR: 38493 File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82 Timestamp: 2026-02-24T19:36:55.089Z Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').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.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.
Description
This PR improves TypeScript safety in the
deleteMessagefunction by removing unsafe type casting for theuserobject.Previously, the code used the following type assertion:
user as Required<Pick<IUser, '_id' | 'username' | 'name'>>This forced TypeScript to assume that
usernameandnamewere always defined on theuserobject. However, in theIUsertype definition these fields are optional, which can lead to potential type safety issues.Changes Made
userobject contains the required fields (_id,username, andname).This ensures that TypeScript can correctly infer the types without relying on forced casting.
Why This Change Is Needed
Using type assertions like
as Required<...>bypasses TypeScript’s safety checks and may hide potential issues. By validating the fields and passing a properly typed object, the code becomes safer and easier to maintain.Impact
This change does not alter runtime behavior but improves TypeScript correctness and maintainability.
Summary by CodeRabbit