feat(apps): Implement deleteNotifyUser to remove ephemeral mess…#38413
feat(apps): Implement deleteNotifyUser to remove ephemeral mess…#38413yush-1018 wants to merge 5 commits intoRocketChat:developfrom
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)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
WalkthroughAdds delete-notify support across the Apps Engine: new Notifier API method, bridge abstractions, server bridge implementations to broadcast ephemeral deletion payloads ( Changes
Sequence DiagramsequenceDiagram
participant App as App
participant Notifier as Notifier
participant Bridge as MessageBridge
participant ServerBridge as AppMessageBridge
participant Client as Client Stream Handler
participant Messages as Messages State
App->>Notifier: deleteNotifyUser(user, message)
Notifier->>Notifier: ensure message.u (app user) exists
Notifier->>Bridge: doDeleteNotifyUser(user, message, appId)
Bridge->>Bridge: check write permissions
Bridge->>ServerBridge: deleteNotifyUser(user, message, appId)
ServerBridge->>ServerBridge: convert app message -> platform message
ServerBridge->>ServerBridge: attach `_deleted: true`
ServerBridge->>Client: broadcast ephemeral `notify-user` message
Client->>Client: receive stream (msg._deleted)
Client->>Messages: Messages.state.delete(msg._id)
Messages->>Messages: remove message from local state
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 |
![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
🤖 Fix all issues with AI agents
In `@apps/meteor/app/apps/server/bridges/messages.ts`:
- Around line 85-98: In deleteNotifyUser, ensure the converted app message has
an identifier before broadcasting deletion: after calling
this.orch.getConverters()?.get('messages').convertAppMessage(message) check that
msg and msg._id are present (and msg.rid if needed) and bail out (or log) if
missing so you don't send a notify.ephemeralMessage delete that clients cannot
match; update the guard around convertAppMessage to verify msg._id and avoid
broadcasting when it's absent.
🧹 Nitpick comments (1)
apps/meteor/client/startup/incomingMessages.ts (1)
13-16: Use a discriminated union for delete payload typing.
IMessage & { _deleted?: boolean }implies a full message even when the delete payload likely only carries_id. A union keeps the delete case minimal and narrows types correctly.Suggested refactor
import type { IMessage } from '@rocket.chat/core-typings'; import { Meteor } from 'meteor/meteor'; +type NotifyUserMessage = (IMessage & { _deleted?: false }) | { _id: IMessage['_id']; _deleted: true }; + import { sdk } from '../../app/utils/client/lib/SDKClient'; import { onLoggedIn } from '../lib/loggedIn'; import { getUserId } from '../lib/user'; import { Messages } from '../stores'; @@ - return sdk.stream('notify-user', [`${getUserId()}/message`], (msg: IMessage & { _deleted?: boolean }) => { + return sdk.stream('notify-user', [`${getUserId()}/message`], (msg: NotifyUserMessage) => { if (msg._deleted) { return Messages.state.delete(msg._id); }
| protected async deleteNotifyUser(user: IAppsUser, message: IAppsMessage, appId: string): Promise<void> { | ||
| this.orch.debugLog(`The App ${appId} is deleting a notification message.`); | ||
|
|
||
| const msg = await this.orch.getConverters()?.get('messages').convertAppMessage(message); | ||
|
|
||
| if (!msg) { | ||
| return; | ||
| } | ||
|
|
||
| void api.broadcast('notify.ephemeralMessage', user.id, msg.rid, { | ||
| ...msg, | ||
| _deleted: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Validate a message id before broadcasting deletion.
If Line 88’s conversion yields a message without _id, the client can’t match and remove the notification, turning the delete into a silent no-op. Consider guarding for a missing id.
🛠️ Proposed guard
const msg = await this.orch.getConverters()?.get('messages').convertAppMessage(message);
if (!msg) {
return;
}
+
+ if (!msg._id) {
+ this.orch.debugLog(`The App ${appId} attempted to delete a notification without a message id.`);
+ return;
+ }
void api.broadcast('notify.ephemeralMessage', user.id, msg.rid, {
...msg,
_deleted: true,
});🤖 Prompt for AI Agents
In `@apps/meteor/app/apps/server/bridges/messages.ts` around lines 85 - 98, In
deleteNotifyUser, ensure the converted app message has an identifier before
broadcasting deletion: after calling
this.orch.getConverters()?.get('messages').convertAppMessage(message) check that
msg and msg._id are present (and msg.rid if needed) and bail out (or log) if
missing so you don't send a notify.ephemeralMessage delete that clients cannot
match; update the guard around convertAppMessage to verify msg._id and avoid
broadcasting when it's absent.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
d-gubert
changed the title
|
Hey @yush-1018 , can you provide an example app that shows how to use this new API? |
|
Hey @d-gubert, import { export class DeleteNotifyUserApp extends App { } class TestDeleteCommand implements ISlashCommand { } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38413 +/- ##
===========================================
+ Coverage 70.58% 70.60% +0.01%
===========================================
Files 3180 3180
Lines 111934 111933 -1
Branches 20245 20221 -24
===========================================
+ Hits 79012 79030 +18
+ Misses 30868 30848 -20
- Partials 2054 2055 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ExampleApp.ts (4)
39-45: Prefix unused parameters with_to signal intentional non-use.
read,http, andpersisare required by theISlashCommandinterface but are never used in the body. Prefixing with_satisfies TypeScript'snoUnusedParametersrule and signals intent.♻️ Proposed refactor
public async executor( context: SlashCommandContext, - read: IRead, + _read: IRead, modify: IModify, - http: IHttp, - persis: IPersistence + _http: IHttp, + _persis: IPersistence ): Promise<void> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ExampleApp.ts` around lines 39 - 45, The executor method declared for the ISlashCommand implementation has unused parameters (read, http, persis); update the method signature in the executor function to prefix those parameters with an underscore (e.g., _read, _http, _persis) so TypeScript's noUnusedParameters rule is satisfied and intent is clear, leaving the rest of the method body and other parameter names (context, modify) unchanged.
50-70: Remove inline comments from the implementation.Comments on lines 50, 57–60, 63, 66, and 69 violate the coding guidelines ("Avoid code comments in the implementation"). The key design constraint (manual ID assignment for ephemeral messages) belongs in the PR description, README, or a JSDoc block on the class/method — not as inline comments.
As per coding guidelines: "Avoid code comments in the implementation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ExampleApp.ts` around lines 50 - 70, Remove the inline comments surrounding the ephemeral-message flow in this snippet (the comment blocks around construction, the "VERY IMPORTANT" paragraph, and numbered steps) and keep the implementation intact (message assignment and setRoom/setText/getMessage usage). Move the rationale about manual ID assignment for ephemeral messages into a JSDoc on the enclosing class or method (or the PR/README) so the requirement is documented, and leave the explicit manual ID assignment (message.id = 'test-delete-id-' + Date.now()) along with the notifyUser and deleteNotifyUser calls (notifier.getMessageBuilder, setRoom, setText, notifyUser, deleteNotifyUser) unchanged.
20-22: Redundant passthrough constructor can be removed.The constructor does nothing but forward arguments to
super; TypeScript will emit the same behavior automatically without it.♻️ Proposed refactor
export class DeleteNotifyUserApp extends App { - constructor(info: IAppInfo, logger: ILogger, accessors: IAppAccessors) { - super(info, logger, accessors); - } - public async extendConfiguration(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ExampleApp.ts` around lines 20 - 22, The constructor in class ExampleApp is a redundant passthrough that only calls super(info, logger, accessors); remove the explicit constructor method from ExampleApp (the symbols to change: the constructor declaration in ExampleApp) so the base class constructor will be invoked automatically; before removing, verify there is no custom initialization or parameter handling in the constructor body and, if needed, move any such logic elsewhere (none present here), then run a TypeScript build to confirm no regressions.
67-67:setTimeoutblocks the executor, but the 3-second delay is acceptable for example code.The slash command executor blocks the Deno subprocess request slot for the full 3-second delay. While the Apps Engine runtime has a configurable timeout (default 30 seconds, controlled via
APPS_ENGINE_RUNTIME_TIMEOUT), this approach still has limitations:
- It holds the executor slot during the delay, preventing reuse for other requests.
- The delay won't survive an app restart between the notify and delete calls.
For production apps, the
ISchedulerAPI (viamodify.getScheduler()) is the idiomatic pattern for deferred work—it's non-blocking and restart-safe. For example code, the currentsetTimeoutapproach works, but consider demonstrating the scheduler pattern for better guidance to developers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ExampleApp.ts` at line 67, The current await new Promise(...) (setTimeout) blocks the executor; replace it with the Apps Engine scheduler by calling modify.getScheduler() (IScheduler) and scheduling a delayed task (e.g., scheduler.scheduleOnce or scheduler.createJob/schedule) to perform the later delete/cleanup instead of awaiting setTimeout; capture whatever context you need (message id, channel id, etc.) in the scheduled job payload so the scheduled handler can run the existing notify and delete logic, and remove the blocking await in the slash command executor.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ExampleApp.tsapps/meteor/app/apps/server/bridges/messages.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/apps/server/bridges/messages.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:
ExampleApp.ts
🧠 Learnings (1)
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
ExampleApp.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). (1)
- GitHub Check: cubic · AI code reviewer
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ExampleApp.ts`:
- Line 61: The current assignment message.id = 'test-delete-id-' + Date.now()
can collide under concurrent invocations; update the assignment inside the
/test-delete-notify handler (where message.id is set) to use a cryptographically
unique ID such as crypto.randomUUID() or a composite key like
`${userId}-${Date.now()}` so each ephemeral message client-side is unique and
cannot be stale-targeted by concurrent requests.
---
Nitpick comments:
In `@ExampleApp.ts`:
- Around line 39-45: The executor method declared for the ISlashCommand
implementation has unused parameters (read, http, persis); update the method
signature in the executor function to prefix those parameters with an underscore
(e.g., _read, _http, _persis) so TypeScript's noUnusedParameters rule is
satisfied and intent is clear, leaving the rest of the method body and other
parameter names (context, modify) unchanged.
- Around line 50-70: Remove the inline comments surrounding the
ephemeral-message flow in this snippet (the comment blocks around construction,
the "VERY IMPORTANT" paragraph, and numbered steps) and keep the implementation
intact (message assignment and setRoom/setText/getMessage usage). Move the
rationale about manual ID assignment for ephemeral messages into a JSDoc on the
enclosing class or method (or the PR/README) so the requirement is documented,
and leave the explicit manual ID assignment (message.id = 'test-delete-id-' +
Date.now()) along with the notifyUser and deleteNotifyUser calls
(notifier.getMessageBuilder, setRoom, setText, notifyUser, deleteNotifyUser)
unchanged.
- Around line 20-22: The constructor in class ExampleApp is a redundant
passthrough that only calls super(info, logger, accessors); remove the explicit
constructor method from ExampleApp (the symbols to change: the constructor
declaration in ExampleApp) so the base class constructor will be invoked
automatically; before removing, verify there is no custom initialization or
parameter handling in the constructor body and, if needed, move any such logic
elsewhere (none present here), then run a TypeScript build to confirm no
regressions.
- Line 67: The current await new Promise(...) (setTimeout) blocks the executor;
replace it with the Apps Engine scheduler by calling modify.getScheduler()
(IScheduler) and scheduling a delayed task (e.g., scheduler.scheduleOnce or
scheduler.createJob/schedule) to perform the later delete/cleanup instead of
awaiting setTimeout; capture whatever context you need (message id, channel id,
etc.) in the scheduled job payload so the scheduled handler can run the existing
notify and delete logic, and remove the blocking await in the slash command
executor.
|
Hi @d-gubert, |
3 participants
This PR enables Rocket.Chat Apps to delete notification (ephemeral) messages. Since these messages are not saved in the database, I implemented a deleteNotifyUser method that triggers a client-side event. When the client receives the message with a _deleted: true flag, it automatically removes it from the UI.
Fixes: #35801
Summary by CodeRabbit