feat: add confirmation dialog when using @all or @here mentions - #41750
Open
kunalUdhani wants to merge 2 commits into
Open
feat: add confirmation dialog when using @all or @here mentions#41750kunalUdhani wants to merge 2 commits into
kunalUdhani wants to merge 2 commits into
Conversation
- Add new admin settings: - Message_ConfirmGroupMentions (boolean, default: true) - Message_ConfirmGroupMentions_MinMembers (int, default: 10) - Create processGroupMentionConfirmation flow that intercepts @all/@here mentions and shows a warning modal before sending - Add i18n keys for modal text and setting labels - Hook into the sendMessage flow after processTooLongMessage
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: b66134e The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-03-16T21:50:37.589ZApplied to files:
🔇 Additional comments (1)
WalkthroughThe message-send flow now checks ChangesGroup Mention Confirmation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant sendMessage
participant processGroupMentionConfirmation
participant ChatAPI
participant ConfirmationModal
sendMessage->>processGroupMentionConfirmation: message text
processGroupMentionConfirmation->>ChatAPI: load room member count
processGroupMentionConfirmation->>ConfirmationModal: display warning
ConfirmationModal-->>processGroupMentionConfirmation: confirm or cancel
processGroupMentionConfirmation-->>sendMessage: abort decision
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.ts (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the inline implementation comments.
The added comments restate behavior that the surrounding code already shows. Remove them to follow the TypeScript implementation guidance.
As per coding guidelines: “Avoid code comments in the implementation.”
Also applies to: 43-43
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.ts` around lines 29 - 30, Remove the inline implementation comments around the room usersCount usage in processGroupMentionConfirmation, including the corresponding comment at the other referenced location, while leaving the surrounding logic unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/meteor/client/lib/chats/flows/sendMessage.ts`:
- Around line 30-33: Update sendMessage and its process flow around
processGroupMentionConfirmation so the cancellation result is propagated to
sendMessage. When confirmation returns true and process exits without sending,
ensure sendMessage skips afterSendMessageCallback; retain the callback for
successfully sent messages.
In `@packages/i18n/src/locales/en.i18n.json`:
- Line 3681: Remove the duplicate Send_anyway entry from the English locale
object, retaining the existing declaration elsewhere in the same file so
consumers continue using the established translation.
---
Nitpick comments:
In `@apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.ts`:
- Around line 29-30: Remove the inline implementation comments around the room
usersCount usage in processGroupMentionConfirmation, including the corresponding
comment at the other referenced location, while leaving the surrounding logic
unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f1b34289-7267-4780-b625-ad644677a9c4
📒 Files selected for processing (4)
apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.tsapps/meteor/client/lib/chats/flows/sendMessage.tsapps/meteor/server/settings/message.tspackages/i18n/src/locales/en.i18n.json
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/server/settings/message.tsapps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.tsapps/meteor/client/lib/chats/flows/sendMessage.ts
apps/meteor/**
📄 CodeRabbit inference engine (CLAUDE.md)
The main Rocket.Chat Meteor application resides in
apps/meteor/; place its application code there rather than in other monorepo areas.
Files:
apps/meteor/server/settings/message.tsapps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.tsapps/meteor/client/lib/chats/flows/sendMessage.ts
packages/**
📄 CodeRabbit inference engine (CLAUDE.md)
Shared libraries belong in
packages/, while other services belong inapps/andee/.
Files:
packages/i18n/src/locales/en.i18n.json
🧠 Learnings (7)
📚 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/server/settings/message.tsapps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.tsapps/meteor/client/lib/chats/flows/sendMessage.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/server/settings/message.tsapps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.tsapps/meteor/client/lib/chats/flows/sendMessage.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/server/settings/message.tsapps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.tsapps/meteor/client/lib/chats/flows/sendMessage.ts
📚 Learning: 2026-08-05T22:02:59.828Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 41707
File: apps/meteor/server/hooks/messages/processThreads.ts:66-68
Timestamp: 2026-08-05T22:02:59.828Z
Learning: In Rocket.Chat Meteor server code, `callbacks.runAsync` returns its input item rather than the asynchronous callback promise. Callers of `afterReadMessages` must invoke `callbacks.runAsync` without awaiting it, keeping read-receipt I/O off the message-send path; this includes `apps/meteor/server/hooks/messages/processThreads.ts`.
Applied to files:
apps/meteor/server/settings/message.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/lib/chats/flows/processGroupMentionConfirmation.tsapps/meteor/client/lib/chats/flows/sendMessage.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.tsapps/meteor/client/lib/chats/flows/sendMessage.ts
📚 Learning: 2026-08-10T13:36:55.243Z
Learnt from: abhinavkrin
Repo: RocketChat/Rocket.Chat PR: 41736
File: packages/i18n/src/locales/th-TH.i18n.json:398-398
Timestamp: 2026-08-10T13:36:55.243Z
Learning: During the coordinated i18n interpolation migration for `Channel_already_exist`, do not flag locale files that still use `%s` until the English base translation and all affected locales are converted together to `{{channelName}}`. Partial locale-only conversions fail the i18n `extra-placeholders` check; the coordinated conversion is owned by the related migration change.
Applied to files:
packages/i18n/src/locales/en.i18n.json
🪛 Biome (2.5.6)
packages/i18n/src/locales/en.i18n.json
[error] 3681-3681: The key Send_anyway was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🔇 Additional comments (1)
apps/meteor/server/settings/message.ts (1)
168-180: LGTM!
Comment on lines
+30
to
+33
| if (await processGroupMentionConfirmation(chat, message)) { | ||
| chat.composer?.setText(message.msg); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect callback behavior and existing send-flow coverage.
rg -n -C 6 '\bafterSendMessageCallback\b' apps/meteor --glob '*.{ts,tsx}'
fd -t f . apps/meteor | rg '(sendMessage|processGroupMentionConfirmation).*test|test.*(sendMessage|processGroupMentionConfirmation)'Repository: RocketChat/Rocket.Chat
Length of output: 4240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sendMessage.ts ---'
cat -n apps/meteor/client/lib/chats/flows/sendMessage.ts
printf '%s\n' '--- processGroupMentionConfirmation.ts ---'
cat -n apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.ts
printf '%s\n' '--- afterSendMessageCallback.ts ---'
cat -n apps/meteor/client/lib/chats/flows/afterSendMessageCallback.ts
printf '%s\n' '--- all sendMessage flow references ---'
rg -n -C 4 'processGroupMentionConfirmation|sendMessage\(|afterSendMessageCallback' apps/meteor/client apps/meteor/tests --glob '*.{ts,tsx}' | head -n 300
printf '%s\n' '--- candidate test files ---'
fd -t f . apps/meteor | rg -i '(sendmessage|processgroupmentionconfirmation|aftersendmessagecallback).*(test|spec)|((test|spec).*(sendmessage|processgroupmentionconfirmation|aftersendmessagecallback))'Repository: RocketChat/Rocket.Chat
Length of output: 36945
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- afterSaveMessage registrations and consumers ---'
rg -n -C 8 "afterSaveMessage|clientCallbacks\.run" . --glob '*.{ts,tsx,js}' | head -n 500
printf '%s\n' '--- callback API definitions ---'
rg -n -C 10 "afterSaveMessage" packages apps --glob '*.{ts,tsx,js}' | head -n 500
printf '%s\n' '--- sendMessage callers and return-value handling ---'
rg -n -C 8 "sendMessage\(" apps/meteor/client --glob '*.{ts,tsx}' | head -n 500
printf '%s\n' '--- process helper return contracts ---'
rg -n -C 5 "processSetReaction|processTooLongMessage|processSlashCommand|processMessageUploads|processMessageEditing" apps/meteor/client/lib/chats/flows --glob '*.{ts,tsx}'Repository: RocketChat/Rocket.Chat
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- client afterSaveMessage registrations ---'
rg -n -C 10 "clientCallbacks\.add\(\s*['\"]afterSaveMessage|['\"]afterSaveMessage['\"]" apps/meteor/client packages --glob '*.{ts,tsx}' \
| rg -v 'server|afterSendMessageCallback|clientCallbacks\.spec' \
| head -n 250
printf '%s\n' '--- direct sendMessage imports and flow wiring ---'
rg -n -C 8 "from ['\"].*flows/sendMessage|flows\.sendMessage|sendMessage:" apps/meteor/client --glob '*.{ts,tsx}' | head -n 300
printf '%s\n' '--- early-return process helpers ---'
for file in \
apps/meteor/client/lib/chats/flows/processSetReaction.ts \
apps/meteor/client/lib/chats/flows/processTooLongMessage.ts \
apps/meteor/client/lib/chats/flows/processSlashCommand.ts \
apps/meteor/client/lib/chats/flows/processMessageUploads.ts \
apps/meteor/client/lib/chats/flows/processMessageEditing.ts
do
if test -f "$file"; then
printf '%s\n' "--- $file ---"
cat -n "$file"
fi
done
printf '%s\n' '--- read-only control-flow verifier ---'
python3 - <<'PY'
from pathlib import Path
source = Path("apps/meteor/client/lib/chats/flows/sendMessage.ts").read_text()
required = [
"await process(chat, message, previewUrls, isSlashCommandAllowed);",
"await afterSendMessageCallback(message, message.rid);",
"if (await processGroupMentionConfirmation(chat, message)) {",
"chat.composer?.setText(message.msg);",
"return;",
]
missing = [item for item in required if item not in source]
if missing:
raise SystemExit(f"missing expected source fragments: {missing}")
process_call = source.index("await process(chat, message, previewUrls, isSlashCommandAllowed);")
callback_call = source.index("await afterSendMessageCallback(message, message.rid);")
if not process_call < callback_call:
raise SystemExit("unexpected call order")
branch = source.index("if (await processGroupMentionConfirmation(chat, message))")
branch_end = source.index("\n\t}", branch)
if source.index("return;", branch, branch_end) == -1:
raise SystemExit("cancellation branch does not return from process")
print("process() returns before sendMessage() invokes afterSendMessageCallback()")
print("sendMessage() has no cancellation-result check between these calls")
PYRepository: RocketChat/Rocket.Chat
Length of output: 24646
Propagate group-mention cancellation to sendMessage.
When processGroupMentionConfirmation returns true, process returns without sending the message. sendMessage still runs afterSendMessageCallback, which records a “Message / Send” analytics event for the canceled message. Propagate the cancellation result and skip after-send callbacks when cancellation occurs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/meteor/client/lib/chats/flows/sendMessage.ts` around lines 30 - 33,
Update sendMessage and its process flow around processGroupMentionConfirmation
so the cancellation result is propagated to sendMessage. When confirmation
returns true and process exits without sending, ensure sendMessage skips
afterSendMessageCallback; retain the callback for successfully sent messages.
| "Message_ConfirmGroupMentions_MinMembers_Description": "Only show the confirmation dialog if the room has at least this many members. Set to 0 to always confirm.", | ||
| "Group_mention_confirm_title": "Notify all members?", | ||
| "Group_mention_confirm_description": "This message will notify all {{count}} members of this channel. Are you sure you want to proceed?", | ||
| "Send_anyway": "Send anyway", |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicate Send_anyway key.
Line 3681 declares a key that already exists in this locale file. Delete this declaration and use the existing translation. Duplicate object keys can fail locale validation and make one value unreachable.
🧰 Tools
🪛 Biome (2.5.6)
[error] 3681-3681: The key Send_anyway was already declared.
(lint/suspicious/noDuplicateObjectKeys)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/i18n/src/locales/en.i18n.json` at line 3681, Remove the duplicate
Send_anyway entry from the English locale object, retaining the existing
declaration elsewhere in the same file so consumers continue using the
established translation.
Source: Linters/SAST tools
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/i18n/src/locales/en.i18n.json">
<violation number="1" location="packages/i18n/src/locales/en.i18n.json:3681">
P3: `Send_anyway` is added here, but it already exists with the same value at line 5040, creating a duplicate key in en.i18n.json. JSON parsers keep only the last occurrence, so this added line is redundant and should be removed (and the new flow can rely on the pre-existing key).</violation>
</file>
<file name="apps/meteor/client/lib/chats/flows/sendMessage.ts">
<violation number="1" location="apps/meteor/client/lib/chats/flows/sendMessage.ts:30">
P2: When processGroupMentionConfirmation resolves to true (user cancelled), process() returns without sending the message, but this cancellation isn't propagated to the caller in sendMessage.ts. As a result, afterSendMessageCallback still runs and records a 'Message / Send' analytics event for a message that was never actually sent. Consider returning a signal (e.g. boolean) from process() so sendMessage can skip the after-send callbacks when the group mention is cancelled.</violation>
<violation number="2" location="apps/meteor/client/lib/chats/flows/sendMessage.ts:31">
P2: On Cancel, `chat.composer?.setText(message.msg)` restores the *composed* message, not what the user typed. `text` is already `.trim()`ed and passed through `closeUnclosedCodeBlock` (sendMessage.ts ~93-94) before `composeMessage` builds `message.msg` via `prependReplies(text, quotedMessages)`, so cancelling can show auto-appended closing code-block backticks, drop leading/trailing whitespace, and — because `chat.composer?.dismissAllQuotedMessages()` already ran before `process()` — the quoted message UI is gone while the quoted content is baked into the restored text as plain inline text. Recommend restoring the original pre-compose text (or aborting before dismissAllQuotedMessages) rather than `message.msg`.</violation>
</file>
<file name="apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.ts">
<violation number="1" location="apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.ts:8">
P2: The `@(all|here)` check will falsely trigger the confirmation dialog for text that is not a group mention. Because there is no boundary required before the `@`, strings like an email address (`foo@all.com`), a URL path (`https://x.com/@all`), or a word containing `@here` (`cat@here!`) all match, so users get an unrelated 'Notify all members?' prompt. The existing mention parser in the codebase (`MentionsParser`) only treats `@all`/`@here` as a mention when preceded by the start of the text, whitespace, or `>`. Consider using the same convention, e.g. `/(^|\s|>)@(all|here)\b/`, so the confirmation is shown only for actual group mentions.</violation>
<violation number="2" location="apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.ts:36">
P2: When an administrator sets `Message_MaxAll` below the room size, this confirmation is shown even though the server rejects the group mention; users must dismiss an inaccurate warning and then receive a failed send. Skipping the prompt when the local member count exceeds the configured public maximum would keep the dialog consistent with server behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
|
|
||
| if (await processGroupMentionConfirmation(chat, message)) { | ||
| chat.composer?.setText(message.msg); |
Contributor
There was a problem hiding this comment.
P2: On Cancel, chat.composer?.setText(message.msg) restores the composed message, not what the user typed. text is already .trim()ed and passed through closeUnclosedCodeBlock (sendMessage.ts ~93-94) before composeMessage builds message.msg via prependReplies(text, quotedMessages), so cancelling can show auto-appended closing code-block backticks, drop leading/trailing whitespace, and — because chat.composer?.dismissAllQuotedMessages() already ran before process() — the quoted message UI is gone while the quoted content is baked into the restored text as plain inline text. Recommend restoring the original pre-compose text (or aborting before dismissAllQuotedMessages) rather than message.msg.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/chats/flows/sendMessage.ts, line 31:
<comment>On Cancel, `chat.composer?.setText(message.msg)` restores the *composed* message, not what the user typed. `text` is already `.trim()`ed and passed through `closeUnclosedCodeBlock` (sendMessage.ts ~93-94) before `composeMessage` builds `message.msg` via `prependReplies(text, quotedMessages)`, so cancelling can show auto-appended closing code-block backticks, drop leading/trailing whitespace, and — because `chat.composer?.dismissAllQuotedMessages()` already ran before `process()` — the quoted message UI is gone while the quoted content is baked into the restored text as plain inline text. Recommend restoring the original pre-compose text (or aborting before dismissAllQuotedMessages) rather than `message.msg`.</comment>
<file context>
@@ -26,6 +27,11 @@ const process = async (chat: ChatAPI, message: IMessage, previewUrls?: string[],
}
+ if (await processGroupMentionConfirmation(chat, message)) {
+ chat.composer?.setText(message.msg);
+ return;
+ }
</file context>
| return false; | ||
| } | ||
|
|
||
| const memberCount: number = room.usersCount ?? 0; |
Contributor
There was a problem hiding this comment.
P2: When an administrator sets Message_MaxAll below the room size, this confirmation is shown even though the server rejects the group mention; users must dismiss an inaccurate warning and then receive a failed send. Skipping the prompt when the local member count exceeds the configured public maximum would keep the dialog consistent with server behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.ts, line 36:
<comment>When an administrator sets `Message_MaxAll` below the room size, this confirmation is shown even though the server rejects the group mention; users must dismiss an inaccurate warning and then receive a failed send. Skipping the prompt when the local member count exceeds the configured public maximum would keep the dialog consistent with server behavior.</comment>
<file context>
@@ -0,0 +1,71 @@
+ return false;
+ }
+
+ const memberCount: number = room.usersCount ?? 0;
+ const minMembers: number = settings.peek<number>('Message_ConfirmGroupMentions_MinMembers') ?? 0;
+
</file context>
| import { settings } from '../../settings'; | ||
| import type { ChatAPI } from '../ChatAPI'; | ||
|
|
||
| const GROUP_MENTION_REGEX = /@(all|here)\b/; |
Contributor
There was a problem hiding this comment.
P2: The @(all|here) check will falsely trigger the confirmation dialog for text that is not a group mention. Because there is no boundary required before the @, strings like an email address (foo@all.com), a URL path (https://x.com/@all), or a word containing @here (cat@here!) all match, so users get an unrelated 'Notify all members?' prompt. The existing mention parser in the codebase (MentionsParser) only treats @all/@here as a mention when preceded by the start of the text, whitespace, or >. Consider using the same convention, e.g. /(^|\s|>)@(all|here)\b/, so the confirmation is shown only for actual group mentions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/chats/flows/processGroupMentionConfirmation.ts, line 8:
<comment>The `@(all|here)` check will falsely trigger the confirmation dialog for text that is not a group mention. Because there is no boundary required before the `@`, strings like an email address (`foo@all.com`), a URL path (`https://x.com/@all`), or a word containing `@here` (`cat@here!`) all match, so users get an unrelated 'Notify all members?' prompt. The existing mention parser in the codebase (`MentionsParser`) only treats `@all`/`@here` as a mention when preceded by the start of the text, whitespace, or `>`. Consider using the same convention, e.g. `/(^|\s|>)@(all|here)\b/`, so the confirmation is shown only for actual group mentions.</comment>
<file context>
@@ -0,0 +1,71 @@
+import { settings } from '../../settings';
+import type { ChatAPI } from '../ChatAPI';
+
+const GROUP_MENTION_REGEX = /@(all|here)\b/;
+
+/**
</file context>
| return; | ||
| } | ||
|
|
||
| if (await processGroupMentionConfirmation(chat, message)) { |
Contributor
There was a problem hiding this comment.
P2: When processGroupMentionConfirmation resolves to true (user cancelled), process() returns without sending the message, but this cancellation isn't propagated to the caller in sendMessage.ts. As a result, afterSendMessageCallback still runs and records a 'Message / Send' analytics event for a message that was never actually sent. Consider returning a signal (e.g. boolean) from process() so sendMessage can skip the after-send callbacks when the group mention is cancelled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/chats/flows/sendMessage.ts, line 30:
<comment>When processGroupMentionConfirmation resolves to true (user cancelled), process() returns without sending the message, but this cancellation isn't propagated to the caller in sendMessage.ts. As a result, afterSendMessageCallback still runs and records a 'Message / Send' analytics event for a message that was never actually sent. Consider returning a signal (e.g. boolean) from process() so sendMessage can skip the after-send callbacks when the group mention is cancelled.</comment>
<file context>
@@ -26,6 +27,11 @@ const process = async (chat: ChatAPI, message: IMessage, previewUrls?: string[],
return;
}
+ if (await processGroupMentionConfirmation(chat, message)) {
+ chat.composer?.setText(message.msg);
+ return;
</file context>
| "Message_ConfirmGroupMentions_MinMembers_Description": "Only show the confirmation dialog if the room has at least this many members. Set to 0 to always confirm.", | ||
| "Group_mention_confirm_title": "Notify all members?", | ||
| "Group_mention_confirm_description": "This message will notify all {{count}} members of this channel. Are you sure you want to proceed?", | ||
| "Send_anyway": "Send anyway", |
Contributor
There was a problem hiding this comment.
P3: Send_anyway is added here, but it already exists with the same value at line 5040, creating a duplicate key in en.i18n.json. JSON parsers keep only the last occurrence, so this added line is redundant and should be removed (and the new flow can rely on the pre-existing key).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/i18n/src/locales/en.i18n.json, line 3681:
<comment>`Send_anyway` is added here, but it already exists with the same value at line 5040, creating a duplicate key in en.i18n.json. JSON parsers keep only the last occurrence, so this added line is redundant and should be removed (and the new flow can rely on the pre-existing key).</comment>
<file context>
@@ -3672,6 +3672,13 @@
+ "Message_ConfirmGroupMentions_MinMembers_Description": "Only show the confirmation dialog if the room has at least this many members. Set to 0 to always confirm.",
+ "Group_mention_confirm_title": "Notify all members?",
+ "Group_mention_confirm_description": "This message will notify all {{count}} members of this channel. Are you sure you want to proceed?",
+ "Send_anyway": "Send anyway",
"Message_not_sent_try_again": "Message not sent. \nPlease try again",
"Message_QuoteChainLimit": "Maximum Number of Chained Quotes",
</file context>
Author
|
Here is a preview of the modal in action: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements a confirmation dialog when a user types
@allor@herein a message, prompting them before notifying all channel members.Closes #909
Changes
New admin settings (Admin -> Message):
Message_ConfirmGroupMentions, boolean, default:true) -- enables/disables the featureMessage_ConfirmGroupMentions_MinMembers, int, default:10) -- only shows the dialog if the room has at least this many membersNew flow:
processGroupMentionConfirmation@all/@herein the message textroom.usersCountfrom local state (no extra API call)GenericModalasking the user to confirmIntegration
sendMessage.tsafterprocessTooLongMessage, before slash commandsScreenshots
Modal appearing when sending @ALL in a 6-member channel:
Type of change
Summary by CodeRabbit
@allor@herein larger rooms.