feat(ai): add built-in AI assistant with mentions, context awareness,…#39807
Open
RickyVishwakarma wants to merge 12 commits intoRocketChat:developfrom
Open
feat(ai): add built-in AI assistant with mentions, context awareness,…#39807RickyVishwakarma wants to merge 12 commits intoRocketChat:developfrom
RickyVishwakarma wants to merge 12 commits intoRocketChat:developfrom
Conversation
… and rate limiting
|
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 |
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a server-side AI integration: OpenAI-backed service with a Meteor method ('ai.generate'), an afterSaveMessage hook that triggers on Changes
Sequence DiagramsequenceDiagram
participant User
participant Chat as Chat System
participant Hook as Message Hook
participant Cache as RateLimitMap
participant Messages as Messages DB
participant AIService as AI Service
participant OpenAI as OpenAI API
User->>Chat: Send message with "@ai"
Chat->>Hook: afterSaveMessage callback
Hook->>Cache: Check user cooldown
alt Cooldown active
Hook-->>Chat: Skip processing
else Cooldown not active
Cache->>Cache: Record timestamp
Hook->>Messages: Fetch last 10 messages for room
Hook->>Hook: Build combined prompt
Hook->>Messages: Insert temp "🤖 Thinking..." as ai.bot
Hook->>AIService: generateAIResponse(prompt)
AIService->>OpenAI: POST /v1/chat/completions (gpt-4o-mini)
OpenAI-->>AIService: Completion response
AIService-->>Hook: Return generated text
Hook->>Messages: Update temp message with AI reply
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
added
the
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
11 issues found across 6 files
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/ai/server/hooks.ts">
<violation number="1" location="apps/meteor/app/ai/server/hooks.ts:8">
P2: AI mention cooldown is implemented with an unbounded per-process Map, causing memory growth over time and non-shared rate-limit enforcement across instances.</violation>
<violation number="2" location="apps/meteor/app/ai/server/hooks.ts:73">
P2: AI mention hook ignores the `AI_Enable` feature toggle, so it still posts bot messages and triggers AI calls when AI is disabled.</violation>
<violation number="3" location="apps/meteor/app/ai/server/hooks.ts:88">
P2: If AI generation fails, the persisted temporary "🤖 Thinking..." message is never updated or removed, leaving stale bot messages in the room.</violation>
<violation number="4" location="apps/meteor/app/ai/server/hooks.ts:93">
P2: Directly updating `Messages` for AI reply bypasses the standard message update pipeline and can skip edit metadata, message processing, and side effects.</violation>
</file>
<file name="apps/meteor/app/ai/server/aiService.ts">
<violation number="1" location="apps/meteor/app/ai/server/aiService.ts:5">
P2: OpenAI API key is cached at module load, so runtime updates to `AI_OpenAI_Key` are ignored until restart.</violation>
<violation number="2" location="apps/meteor/app/ai/server/aiService.ts:14">
P2: AI requests are sent even when the OpenAI API key is unset, resulting in invalid Authorization headers and avoidable external failures instead of a clear fail-fast error.</violation>
<violation number="3" location="apps/meteor/app/ai/server/aiService.ts:27">
P2: Missing HTTP status handling can mask upstream API failures as a normal `'No response'` result.</violation>
</file>
<file name="apps/meteor/app/ai/server/settings.ts">
<violation number="1" location="apps/meteor/app/ai/server/settings.ts:12">
P1: OpenAI API key is registered as a plain string instead of a secret/password setting, so it does not follow established sensitive-setting handling patterns.</violation>
</file>
<file name="apps/meteor/app/ai/server/methods.ts">
<violation number="1" location="apps/meteor/app/ai/server/methods.ts:7">
P2: Unauthorized branch uses a non-standard Meteor error code (`'Not authorized'`) instead of the project’s canonical `error-not-authorized`, causing inconsistent client error handling.</violation>
<violation number="2" location="apps/meteor/app/ai/server/methods.ts:10">
P2: `ai.generate` accepts client input without runtime validation/size limits and forwards it directly to the AI backend.</violation>
<violation number="3" location="apps/meteor/app/ai/server/methods.ts:10">
P1: `ai.generate` lacks feature-toggle and rate-limit enforcement at the method boundary, allowing direct authenticated RPC calls to bypass expected AI usage controls.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // ========================= | ||
| // ✏️ Update message | ||
| // ========================= | ||
| await Messages.update( |
Contributor
There was a problem hiding this comment.
P2: Directly updating Messages for AI reply bypasses the standard message update pipeline and can skip edit metadata, message processing, and side effects.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/ai/server/hooks.ts, line 93:
<comment>Directly updating `Messages` for AI reply bypasses the standard message update pipeline and can skip edit metadata, message processing, and side effects.</comment>
<file context>
@@ -0,0 +1,109 @@
+ // =========================
+ // ✏️ Update message
+ // =========================
+ await Messages.update(
+ { _id: tempMessage._id },
+ {
</file context>
| import { settings } from '../../../settings/server'; | ||
|
|
||
| // 🔒 Rate limiter (per user) | ||
| const userLastRequest = new Map<string, number>(); |
Contributor
There was a problem hiding this comment.
P2: AI mention cooldown is implemented with an unbounded per-process Map, causing memory growth over time and non-shared rate-limit enforcement across instances.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/ai/server/hooks.ts, line 8:
<comment>AI mention cooldown is implemented with an unbounded per-process Map, causing memory growth over time and non-shared rate-limit enforcement across instances.</comment>
<file context>
@@ -0,0 +1,109 @@
+import { settings } from '../../../settings/server';
+
+// 🔒 Rate limiter (per user)
+const userLastRequest = new Map<string, number>();
+const COOLDOWN = 5000; // 5 sec
+
</file context>
![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: 8
🧹 Nitpick comments (2)
apps/meteor/app/ai/server/hooks.ts (2)
7-9: Remove inline emoji comments per coding guidelines.Multiple lines contain descriptive comments with emojis (e.g.,
// 🔒 Rate limiter,// ❌ Ignore bot itself). As per coding guidelines for.tsfiles: "Avoid code comments in the implementation."Also applies to: 15-16, 20-21, 25-26, 32-33, 42-43, 49-51, 70-72, 85-87, 90-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/ai/server/hooks.ts` around lines 7 - 9, Remove all inline emoji comments across the file (e.g., "// 🔒 Rate limiter", "// ❌ Ignore bot itself") to comply with the .ts coding guideline; locate comment occurrences near declarations like userLastRequest and COOLDOWN and inside functions in hooks.ts and delete those comment lines (leave functional code intact), ensuring any necessary explanatory text is moved to external documentation or top-of-file non-implementation documentation if needed.
7-9: In-memory rate limiter won't work in multi-instance deployments.The
Mapis local to each process. In clustered/Kubernetes deployments, users can bypass the cooldown by hitting different instances. Consider using Redis or a shared cache for production-grade rate limiting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/ai/server/hooks.ts` around lines 7 - 9, The current in-memory rate limiter using userLastRequest and COOLDOWN in hooks.ts won't work across multiple instances; replace the Map with a shared store (e.g., a Redis client) and implement per-user cooldowns using a Redis key per user (e.g., "rate:user:{userId}") with SET/GET or SET with NX and PX to atomically enforce the cooldown; inject or import the Redis client where userLastRequest is referenced, use the COOLDOWN value to set the key TTL (milliseconds) and check existence before allowing a request, and remove all local Map usages so the limiter works across clustered/Kubernetes deployments.
🤖 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/app/ai/server/aiService.ts`:
- Line 5: The module currently captures OPENAI_API_KEY once at load via const
OPENAI_API_KEY = settings.get('AI_OpenAI_Key') which becomes stale if an admin
updates the setting; update the code to read the API key dynamically
instead—either subscribe with settings.watch() like in slackbridge.ts or simply
call settings.get('AI_OpenAI_Key') inside the generateAIResponse function (and
any other functions that use OPENAI_API_KEY) so the latest value is used at
invocation; replace usages of the module-level OPENAI_API_KEY constant with the
dynamically retrieved value or remove the constant entirely.
- Around line 10-27: The fetch call in aiService.ts that posts to OpenAI (using
OPENAI_API_KEY, model 'gpt-4o-mini' and prompt) needs a request timeout and HTTP
error handling to avoid hangs and misleading results: use the native
AbortController to set a configurable timeout (e.g., 10s), pass
controller.signal into fetch, clear the timeout on completion, check response.ok
before calling response.json(), and if not ok throw or return a descriptive
error including response.status and await response.text() (or attempt to parse
error JSON) so callers don’t get misleading data; also catch JSON parse errors
around await response.json() and surface a clear error instead of returning 'No
response'.
In `@apps/meteor/app/ai/server/hooks.ts`:
- Around line 11-30: The afterSaveMessage callback currently ignores bot/system
messages and checks mentions but doesn't respect the AI_Enable toggle; use the
existing settings import to short-circuit when the feature is off by adding an
early return at the top of the 'afterSaveMessage' handler (e.g., if
(!settings.get('AI_Enable')) return message;), ensuring the settings import is
used and the feature can be disabled by admins; keep this check before any
processing like the ai.bot or message.msg checks so the handler exits quickly
when disabled.
- Around line 32-40: The rate-limiting block uses message.u._id without ensuring
message.u exists, risking a crash; update the check in the hook handling (the
code around the COOLDOWN, now, last, userLastRequest.get and
userLastRequest.set) to first verify message.u and message.u._id are defined
(e.g., if (!message.u || !message.u._id) return message or skip rate-limiting)
before calling userLastRequest.get/set, so all accesses to message.u._id are
guarded.
- Around line 93-100: The DB update using Messages.update({ _id: tempMessage._id
}, { $set: { msg: aiReply } }) won't push a real-time change to clients; after
performing the update, call notifyOnMessageChange(...) to notify connected
clients (pass either the updated message id tempMessage._id or the updated
message object as expected by notifyOnMessageChange). Ensure
notifyOnMessageChange is imported in hooks.ts and invoke it immediately after
Messages.update so clients see the AI reply.
In `@apps/meteor/app/ai/server/index.ts`:
- Line 4: The file contains a redundant self-import statement "import
'../../app/ai/server';" in index.ts which creates a circular import; remove that
import line from apps/meteor/app/ai/server/index.ts (or simply delete the import
of '../../app/ai/server') and verify there are no necessary side-effects relied
on by index.ts—if any initialization is required, call the actual exported
initializer function (e.g., server initialization function) instead of importing
the same module back into itself.
In `@apps/meteor/app/ai/server/methods.ts`:
- Around line 4-11: The 'ai.generate' method lacks input validation, rate
limiting, and a feature-toggle check; update the method to call check(prompt,
String) before using prompt, enforce RateLimiter.limitMethod('ai.generate', ...)
with appropriate rate/interval matching other methods, and verify the admin
toggle (e.g., Settings.get('AI_Enable') or equivalent) and throw a Meteor.Error
when the setting is disabled; ensure generateAIResponse is only invoked after
these checks so unauthorized/invalid/disabled requests are rejected and
expensive AI calls are protected.
In `@apps/meteor/app/ai/server/settings.ts`:
- Around line 3-8: Remove the inline comment near settings.add('AI_Enable', ...)
and enforce the setting: ensure code that triggers AI behavior checks
settings.get('AI_Enable') — add an early return in the AI webhook/handler in
hooks.ts so the hook exits immediately when disabled, and add a guard in the
public method(s) in methods.ts to throw an appropriate error if
settings.get('AI_Enable') is false; reference the existing settings key
'AI_Enable' when adding these checks.
---
Nitpick comments:
In `@apps/meteor/app/ai/server/hooks.ts`:
- Around line 7-9: Remove all inline emoji comments across the file (e.g., "//
🔒 Rate limiter", "// ❌ Ignore bot itself") to comply with the .ts coding
guideline; locate comment occurrences near declarations like userLastRequest and
COOLDOWN and inside functions in hooks.ts and delete those comment lines (leave
functional code intact), ensuring any necessary explanatory text is moved to
external documentation or top-of-file non-implementation documentation if
needed.
- Around line 7-9: The current in-memory rate limiter using userLastRequest and
COOLDOWN in hooks.ts won't work across multiple instances; replace the Map with
a shared store (e.g., a Redis client) and implement per-user cooldowns using a
Redis key per user (e.g., "rate:user:{userId}") with SET/GET or SET with NX and
PX to atomically enforce the cooldown; inject or import the Redis client where
userLastRequest is referenced, use the COOLDOWN value to set the key TTL
(milliseconds) and check existence before allowing a request, and remove all
local Map usages so the limiter works across clustered/Kubernetes deployments.
🪄 Autofix (Beta)
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
Run ID: efd6805d-306b-4c4a-84ef-5be6975a628a
📒 Files selected for processing (6)
apps/meteor/app/ai/lib/types.tsapps/meteor/app/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.tsapps/meteor/app/ai/server/index.tsapps/meteor/app/ai/server/methods.tsapps/meteor/app/ai/server/settings.ts
📜 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)
- GitHub Check: cubic · AI code reviewer
🧰 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/ai/server/index.tsapps/meteor/app/ai/server/methods.tsapps/meteor/app/ai/server/settings.tsapps/meteor/app/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 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/ai/server/index.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/ai/server/index.tsapps/meteor/app/ai/server/methods.tsapps/meteor/app/ai/server/settings.tsapps/meteor/app/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.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/ai/server/index.tsapps/meteor/app/ai/server/methods.tsapps/meteor/app/ai/server/settings.tsapps/meteor/app/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.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/ai/server/settings.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/app/ai/server/settings.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/ai/server/aiService.ts
📚 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/ai/server/hooks.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/app/ai/server/hooks.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/app/ai/server/hooks.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/ai/server/hooks.ts
Comment on lines
+4
to
+11
| Meteor.methods({ | ||
| async 'ai.generate'(prompt: string) { | ||
| if (!this.userId) { | ||
| throw new Meteor.Error('Not authorized'); | ||
| } | ||
|
|
||
| return await generateAIResponse(prompt); | ||
| }, |
Contributor
There was a problem hiding this comment.
Add input validation, rate limiting, and feature toggle check.
This method deviates from established Rocket.Chat patterns (see followMessage.ts and setEmail.ts):
- Missing
check(prompt, String)for input validation - No
RateLimiter.limitMethod()— AI calls are expensive and abuse-prone - No check for
AI_Enablesetting — the feature cannot be disabled by admins
Proposed fix
import { Meteor } from 'meteor/meteor';
+import { check } from 'meteor/check';
import { generateAIResponse } from './aiService';
+import { settings } from '../../../settings/server';
+import { RateLimiter } from '../../../lib/server';
Meteor.methods({
async 'ai.generate'(prompt: string) {
+ check(prompt, String);
+
+ if (!settings.get('AI_Enable')) {
+ throw new Meteor.Error('error-ai-disabled', 'AI feature is disabled', { method: 'ai.generate' });
+ }
+
if (!this.userId) {
- throw new Meteor.Error('Not authorized');
+ throw new Meteor.Error('error-not-authorized', 'Not authorized', { method: 'ai.generate' });
}
return await generateAIResponse(prompt);
},
});
+
+RateLimiter.limitMethod('ai.generate', 5, 60000, {
+ userId() {
+ return true;
+ },
+});📝 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.
Suggested change
| Meteor.methods({ | |
| async 'ai.generate'(prompt: string) { | |
| if (!this.userId) { | |
| throw new Meteor.Error('Not authorized'); | |
| } | |
| return await generateAIResponse(prompt); | |
| }, | |
| import { Meteor } from 'meteor/meteor'; | |
| import { check } from 'meteor/check'; | |
| import { generateAIResponse } from './aiService'; | |
| import { settings } from '../../../settings/server'; | |
| import { RateLimiter } from '../../../lib/server'; | |
| Meteor.methods({ | |
| async 'ai.generate'(prompt: string) { | |
| check(prompt, String); | |
| if (!settings.get('AI_Enable')) { | |
| throw new Meteor.Error('error-ai-disabled', 'AI feature is disabled', { method: 'ai.generate' }); | |
| } | |
| if (!this.userId) { | |
| throw new Meteor.Error('error-not-authorized', 'Not authorized', { method: 'ai.generate' }); | |
| } | |
| return await generateAIResponse(prompt); | |
| }, | |
| }); | |
| RateLimiter.limitMethod('ai.generate', 5, 60000, { | |
| userId() { | |
| return true; | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/ai/server/methods.ts` around lines 4 - 11, The 'ai.generate'
method lacks input validation, rate limiting, and a feature-toggle check; update
the method to call check(prompt, String) before using prompt, enforce
RateLimiter.limitMethod('ai.generate', ...) with appropriate rate/interval
matching other methods, and verify the admin toggle (e.g.,
Settings.get('AI_Enable') or equivalent) and throw a Meteor.Error when the
setting is disabled; ensure generateAIResponse is only invoked after these
checks so unauthorized/invalid/disabled requests are rejected and expensive AI
calls are protected.
Comment on lines
+3
to
+8
| // Enable/Disable AI | ||
| settings.add('AI_Enable', true, { | ||
| type: 'boolean', | ||
| public: true, | ||
| group: 'AI', | ||
| }); |
Contributor
There was a problem hiding this comment.
Remove inline comment and note that AI_Enable is not enforced.
The comment violates coding guidelines. More critically, while AI_Enable is registered here, neither aiService.ts, methods.ts, nor hooks.ts check this setting before executing AI logic — the toggle has no effect.
Proposed fix for this file
import { settings } from '../../../settings/server';
-// Enable/Disable AI
settings.add('AI_Enable', true, {
type: 'boolean',
public: true,
group: 'AI',
});The AI_Enable check should be added to hooks.ts (early return if disabled) and methods.ts (throw error if disabled). See the review comments on those files.
📝 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.
Suggested change
| // Enable/Disable AI | |
| settings.add('AI_Enable', true, { | |
| type: 'boolean', | |
| public: true, | |
| group: 'AI', | |
| }); | |
| import { settings } from '../../../settings/server'; | |
| settings.add('AI_Enable', true, { | |
| type: 'boolean', | |
| public: true, | |
| group: 'AI', | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/ai/server/settings.ts` around lines 3 - 8, Remove the inline
comment near settings.add('AI_Enable', ...) and enforce the setting: ensure code
that triggers AI behavior checks settings.get('AI_Enable') — add an early return
in the AI webhook/handler in hooks.ts so the hook exits immediately when
disabled, and add a guard in the public method(s) in methods.ts to throw an
appropriate error if settings.get('AI_Enable') is false; reference the existing
settings key 'AI_Enable' when adding these checks.
Contributor
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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/ai/server/settings.ts">
<violation number="1" location="apps/meteor/app/ai/server/settings.ts:4">
P1: The `AI_Enable` setting was accidentally replaced, causing duplicate `AI_OpenAI_Key` registration and removing the boolean toggle consumed by AI runtime guards.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Author
|
fix(ai): address review issues (security, validation, error handling) |
Author
|
fix(ai): restore AI_Enable setting and correct configuration |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/meteor/app/ai/server/hooks.ts (2)
96-103:⚠️ Potential issue | 🟠 MajorMessage update won't push real-time updates to clients.
Messages.update()modifies the database but doesn't trigger Rocket.Chat's real-time notification system. Clients won't see the "Thinking..." message change to the AI reply without callingnotifyOnMessageChange.,
Proposed fix
+import { notifyOnMessageChange } from '../../../lib/server/lib/notifyListener'; // ... after Messages.update: await Messages.update( { _id: tempMessage._id }, { $set: { msg: aiReply, }, } ); + + await notifyOnMessageChange({ id: tempMessage._id }); return message;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/ai/server/hooks.ts` around lines 96 - 103, The code updates the DB via Messages.update but doesn't notify clients; after updating the temp message (using tempMessage._id) include a call to Rocket.Chat's real-time notifier (notifyOnMessageChange) so clients see the "Thinking..." message replaced by aiReply; specifically, after Messages.update(...) for tempMessage._id, fetch or construct the updated message (with msg: aiReply) and call notifyOnMessageChange (or the app's equivalent notifier) passing the message id and updated fields so the UI receives the real-time update.
35-43:⚠️ Potential issue | 🟡 MinorPotential null dereference on
message.u._id.Lines 37 and 43 access
message.u._iddirectly, but the guard at line 16 only checksmessage.u?.username. Ifmessage.uexists but_idis undefined, or ifmessage.uis undefined (and username check returnsundefined === 'ai.bot'which is false), subsequent access will throw.,
Proposed fix
if (!isMentioningAI) return message; + if (!message.u?._id) { + return message; + } + // ✅ Rate limiting const now = Date.now(); - const last = userLastRequest.get(message.u._id) || 0; + const last = userLastRequest.get(message.u._id) ?? 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/ai/server/hooks.ts` around lines 35 - 43, The rate-limiting block assumes message.u._id exists and can throw; modify the logic around userLastRequest/COOLDOWN to safely handle missing user IDs: obtain the user id with optional chaining (e.g., const userId = message.u?._id) and if userId is falsy return message early, then use userId when reading/writing userLastRequest and when computing last/now. Ensure all references to message.u._id in this block are replaced with the safe userId variable.
🧹 Nitpick comments (1)
apps/meteor/app/ai/server/hooks.ts (1)
7-9: In-memory rate limiter will grow unbounded over time.The
userLastRequestMap is never cleaned up. On a busy server with many users, this will accumulate entries indefinitely since old timestamps are never removed. Consider periodic cleanup or using a bounded cache.Proposed fix with cleanup
const userLastRequest = new Map<string, number>(); const COOLDOWN = 5000; // 5 sec +const CLEANUP_INTERVAL = 60000; // 1 min + +// Periodic cleanup of stale entries +setInterval(() => { + const cutoff = Date.now() - COOLDOWN; + for (const [userId, timestamp] of userLastRequest) { + if (timestamp < cutoff) { + userLastRequest.delete(userId); + } + } +}, CLEANUP_INTERVAL);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/ai/server/hooks.ts` around lines 7 - 9, The in-memory Map userLastRequest used for per-user rate limiting grows unbounded; change it to a bounded/expiry cache by either replacing userLastRequest with an LRU/TTL cache (e.g., an lru-cache instance) or add periodic cleanup that scans userLastRequest and deletes entries whose timestamp is older than COOLDOWN (or a slightly larger TTL) using a setInterval; ensure cleanup is started when the module initializes and cleared on shutdown so old entries do not accumulate and the rate limiter remains memory-bounded.
🤖 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/app/ai/server/hooks.ts`:
- Around line 76-86: The sendMessage call is using the wrong argument order and
omits the room: change the call to match sendMessage(user, message, room,
options) by passing the user object first (the ai bot user), then the message
object (the '🤖 Thinking...' payload), then the room object from the surrounding
scope, and keep the options (e.g., { _id: 'ai-bot' }) as the fourth argument;
update the tempMessage invocation accordingly (refer to sendMessage and
tempMessage variables in this file).
---
Duplicate comments:
In `@apps/meteor/app/ai/server/hooks.ts`:
- Around line 96-103: The code updates the DB via Messages.update but doesn't
notify clients; after updating the temp message (using tempMessage._id) include
a call to Rocket.Chat's real-time notifier (notifyOnMessageChange) so clients
see the "Thinking..." message replaced by aiReply; specifically, after
Messages.update(...) for tempMessage._id, fetch or construct the updated message
(with msg: aiReply) and call notifyOnMessageChange (or the app's equivalent
notifier) passing the message id and updated fields so the UI receives the
real-time update.
- Around line 35-43: The rate-limiting block assumes message.u._id exists and
can throw; modify the logic around userLastRequest/COOLDOWN to safely handle
missing user IDs: obtain the user id with optional chaining (e.g., const userId
= message.u?._id) and if userId is falsy return message early, then use userId
when reading/writing userLastRequest and when computing last/now. Ensure all
references to message.u._id in this block are replaced with the safe userId
variable.
---
Nitpick comments:
In `@apps/meteor/app/ai/server/hooks.ts`:
- Around line 7-9: The in-memory Map userLastRequest used for per-user rate
limiting grows unbounded; change it to a bounded/expiry cache by either
replacing userLastRequest with an LRU/TTL cache (e.g., an lru-cache instance) or
add periodic cleanup that scans userLastRequest and deletes entries whose
timestamp is older than COOLDOWN (or a slightly larger TTL) using a setInterval;
ensure cleanup is started when the module initializes and cleared on shutdown so
old entries do not accumulate and the rate limiter remains memory-bounded.
🪄 Autofix (Beta)
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
Run ID: c0f9e2b0-897c-44d6-871b-51f33818f546
📒 Files selected for processing (5)
apps/meteor/app/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.tsapps/meteor/app/ai/server/index.tsapps/meteor/app/ai/server/methods.tsapps/meteor/app/ai/server/settings.ts
✅ Files skipped from review due to trivial changes (1)
- apps/meteor/app/ai/server/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/ai/server/methods.ts
- apps/meteor/app/ai/server/settings.ts
📜 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)
- GitHub Check: cubic · AI code reviewer
🧰 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/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.ts
🧠 Learnings (16)
📚 Learning: 2026-03-16T22:56:54.500Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39677
File: packages/models/src/helpers/omnichannel/agentStatus.ts:10-29
Timestamp: 2026-03-16T22:56:54.500Z
Learning: In `packages/models/src/helpers/omnichannel/agentStatus.ts` (PR `#39677`), the `queryStatusAgentOnline` function intentionally omits the `$or` offline-status guard for non-bot agents when `isLivechatEnabledWhenAgentIdle === true`. This is by design: the setting `Livechat_enabled_when_agent_idle` (`accept_chats_when_agent_idle`) means agents should receive chats even when idle/offline, so the offline filter must be removed in that path. Bots are always status-agnostic and are always included regardless of their online/offline status. Do not flag this as a bug.
Applied to files:
apps/meteor/app/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/app/ai/server/aiService.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/app/ai/server/aiService.ts
📚 Learning: 2026-03-16T11:50:18.066Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:18.066Z
Learning: In `apps/meteor/tests/end-to-end/apps/` (and Livechat E2E tests broadly), calling `createAgent()` and `makeAgentAvailable()` immediately after `updateSetting('Livechat_enabled', true)` — without any intermediate sleep — is an established, non-flaky pattern used across 10+ tests in the codebase. Do not flag this sequence as a potential race condition or suggest adding a delay between them.
Applied to files:
apps/meteor/app/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.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/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.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/ai/server/aiService.tsapps/meteor/app/ai/server/hooks.ts
📚 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/ai/server/hooks.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/ai/server/hooks.ts
📚 Learning: 2026-03-20T13:51:20.280Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:20.280Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-03-09T21:20:12.687Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39386
File: apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts:12-15
Timestamp: 2026-03-09T21:20:12.687Z
Learning: In `apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts`, the early return `if (data.voipToken) return null` (Lines 13-15) is intentionally correct. VoIP token updates always include an `_id`, so they are handled by the `_id` lookup block above (Lines 5-9) and never reach this guard. The guard is only a safety net for edge cases where `_id` is absent or no document was found, preventing an incorrect `token + appName` fallback match for VoIP-only payloads.
Applied to files:
apps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/ai/server/hooks.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/app/ai/server/hooks.ts
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
apps/meteor/app/ai/server/hooks.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/ai/server/hooks.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/ai/server/hooks.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/ai/server/hooks.ts
🔇 Additional comments (1)
apps/meteor/app/ai/server/aiService.ts (1)
3-58: Implementation looks solid with proper timeout and error handling.The dynamic API key retrieval, AbortController timeout, and HTTP error handling are all correctly implemented. The abort error is properly distinguished from other errors.
One minor note: the inline comments (e.g.,
// ✅ Get API key dynamically) could be removed per coding guidelines, as the code is self-documenting.
Author
|
correct sendMessage usage and improve hook stability |
coderabbitai
bot
added
the
type: feature
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/ai/server/settings.ts (1)
10-15: Remove inline comment on the API key setting.The comment
// 🔐 OpenAI API Key (secure)should be removed per the coding guideline to avoid code comments in the implementation. Thetype: 'password'andpublic: falseconfiguration are appropriate for securing the API key.Proposed fix
-// 🔐 OpenAI API Key (secure) settings.add('AI_OpenAI_Key', '', { type: 'password', public: false, group: 'AI', });As per coding guidelines:
**/*.{ts,tsx,js}: Avoid code comments in the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/ai/server/settings.ts` around lines 10 - 15, Remove the inline comment before the API key setting by deleting the line containing "// 🔐 OpenAI API Key (secure)"; keep the settings.add call for 'AI_OpenAI_Key' intact (preserving type: 'password', public: false, group: 'AI') so the configuration remains secure but without the implementation comment.
🤖 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/ai/server/settings.ts`:
- Around line 10-15: Remove the inline comment before the API key setting by
deleting the line containing "// 🔐 OpenAI API Key (secure)"; keep the
settings.add call for 'AI_OpenAI_Key' intact (preserving type: 'password',
public: false, group: 'AI') so the configuration remains secure but without the
implementation comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ecb52c1-267d-45a6-b581-20ec67378a90
📒 Files selected for processing (2)
apps/meteor/app/ai/server/hooks.tsapps/meteor/app/ai/server/settings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/ai/server/hooks.ts
📜 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)
- GitHub Check: cubic · AI code reviewer
🧰 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/ai/server/settings.ts
🧠 Learnings (14)
📚 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/ai/server/settings.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/app/ai/server/settings.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/app/ai/server/settings.ts
📚 Learning: 2025-10-07T15:08:37.419Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36942
File: apps/meteor/client/lib/e2ee/pbkdf2.ts:13-45
Timestamp: 2025-10-07T15:08:37.419Z
Learning: In apps/meteor/client/lib/e2ee/pbkdf2.ts, PBKDF2 iteration count validation is not enforced because the iterations parameter is outside the user's control and is system-managed.
Applied to files:
apps/meteor/app/ai/server/settings.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/ai/server/settings.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
apps/meteor/app/ai/server/settings.ts
📚 Learning: 2026-03-16T22:56:54.500Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39677
File: packages/models/src/helpers/omnichannel/agentStatus.ts:10-29
Timestamp: 2026-03-16T22:56:54.500Z
Learning: In `packages/models/src/helpers/omnichannel/agentStatus.ts` (PR `#39677`), the `queryStatusAgentOnline` function intentionally omits the `$or` offline-status guard for non-bot agents when `isLivechatEnabledWhenAgentIdle === true`. This is by design: the setting `Livechat_enabled_when_agent_idle` (`accept_chats_when_agent_idle`) means agents should receive chats even when idle/offline, so the offline filter must be removed in that path. Bots are always status-agnostic and are always included regardless of their online/offline status. Do not flag this as a bug.
Applied to files:
apps/meteor/app/ai/server/settings.ts
📚 Learning: 2026-03-20T13:51:20.280Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:20.280Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/ai/server/settings.ts
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/ai/server/settings.ts
📚 Learning: 2026-03-04T14:16:49.202Z
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.
Applied to files:
apps/meteor/app/ai/server/settings.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/ai/server/settings.ts
📚 Learning: 2026-03-16T11:50:18.066Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:18.066Z
Learning: In `apps/meteor/tests/end-to-end/apps/` (and Livechat E2E tests broadly), calling `createAgent()` and `makeAgentAvailable()` immediately after `updateSetting('Livechat_enabled', true)` — without any intermediate sleep — is an established, non-flaky pattern used across 10+ tests in the codebase. Do not flag this sequence as a potential race condition or suggest adding a delay between them.
Applied to files:
apps/meteor/app/ai/server/settings.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/ai/server/settings.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/ai/server/settings.ts
🔇 Additional comments (1)
apps/meteor/app/ai/server/settings.ts (1)
3-3: Remove inline comment per coding guidelines.The comment
// ✅ Enable/Disable AIshould be removed as it violates the guideline to avoid code comments in the implementation. This was flagged in a previous review.
coderabbitai
bot
removed
the
type: feature
Author
|
Hi maintainers 👋 All review comments have been addressed:
QA:
Ready for final review 🙌 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/meteor/app/ai/server/hooks.ts (1)
8-8: Drop the inline implementation commentary.The emoji section markers add noise in a hot server path and this repo explicitly avoids implementation comments in TS/JS files. As per coding guidelines, "Avoid code comments in the implementation."
Also applies to: 12-12, 23-23, 28-28, 33-33, 38-38, 45-45, 56-56, 60-62, 81-83, 96-98, 105-107, 117-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/ai/server/hooks.ts` at line 8, Remove the inline emoji section comments (e.g. "// 🔒 Rate limiter" and the other emoji-marked comment lines at the noted locations such as the ones containing emoji markers on lines referenced in the review) from hooks.ts so the hot server path contains no implementation comments; simply delete those comment lines (do not add new inline comments in the functions or export blocks) and leave the surrounding code and exported symbols untouched.
🤖 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/app/ai/server/hooks.ts`:
- Around line 63-66: The history query and reply path must respect thread
context: when handling a threaded trigger (message.tmid is set) propagate
message.tmid into the reply creation and scope the Messages.find query to the
thread by including tmid: message.tmid (instead of the room-wide query using
only rid), and when creating the placeholder reply include the same tmid so the
AI response stays in-thread; apply the same change to the other similar block
(lines around the Messages.find at 84-90) so both history lookups and reply
creations use message.tmid when present.
- Around line 13-17: Unify mention detection by using a single anchored,
case-insensitive regex that lists allowed aliases (e.g.
/^@(?:ai|ai\.bot|ai-bot)$/i) and use that same matcher for both detection and
cleanup; update the code paths that reference botUser.username, the structured
mention check, and the fallback includes/replace logic (functions/variables:
botUser, and the mention parsing/cleanup code around the current mention
handling) so you only detect exact alias matches and strip only the matched
alias portion rather than doing substring matches like includes('@ai') or
replace(/@ai/gi, '').
- Around line 8-10: Replace the module-local Map-based rate limiter
(userLastRequest and COOLDOWN) with a shared TTL-backed store (e.g., Redis or
your existing cache abstraction) so cooldowns are honored across processes and
entries auto-expire; update the check/set logic used around the hooks that
reference userLastRequest (the same places in lines ~45-54) to query the shared
store for a per-user key, atomically set the key with a 5s TTL when allowing a
request (or use SETNX/EXPIRE or a cache.put-with-ttl), and base throttle
decisions on the presence/expiry of that key instead of storing timestamps in
the Map. Ensure the new store key uses a unique prefix (e.g.,
"rate:ai:authorId") and handle store errors gracefully where userLastRequest was
previously referenced.
- Around line 19-21: The afterSaveMessage callback currently awaits
generateAIResponse inside callbacks.add('afterSaveMessage', async (message, {
room }) => { ... }), which blocks callbacks.run (see callbacksBase.ts) and makes
message saves slow; change this so the callback posts the placeholder
synchronously and does NOT await the external OpenAI round-trip—instead schedule
the real work to run asynchronously (use Meteor.defer / setImmediate / your
app's job queue) and pass the message id/room to that background task which will
call generateAIResponse() and update the placeholder when done; apply the same
change to the other handler(s) referenced around lines 95-119 so no OpenAI
network call is awaited inside the afterSaveMessage callback chain.
---
Nitpick comments:
In `@apps/meteor/app/ai/server/hooks.ts`:
- Line 8: Remove the inline emoji section comments (e.g. "// 🔒 Rate limiter"
and the other emoji-marked comment lines at the noted locations such as the ones
containing emoji markers on lines referenced in the review) from hooks.ts so the
hot server path contains no implementation comments; simply delete those comment
lines (do not add new inline comments in the functions or export blocks) and
leave the surrounding code and exported symbols untouched.
🪄 Autofix (Beta)
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
Run ID: 588f904f-69a3-456c-ae24-6f5581dc85de
📒 Files selected for processing (1)
apps/meteor/app/ai/server/hooks.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/ai/server/hooks.ts
🧠 Learnings (22)
📚 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/ai/server/hooks.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/ai/server/hooks.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/app/ai/server/hooks.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/ai/server/hooks.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/ai/server/hooks.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/app/ai/server/hooks.ts
📚 Learning: 2026-03-16T22:56:54.500Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39677
File: packages/models/src/helpers/omnichannel/agentStatus.ts:10-29
Timestamp: 2026-03-16T22:56:54.500Z
Learning: In `packages/models/src/helpers/omnichannel/agentStatus.ts` (PR `#39677`), the `queryStatusAgentOnline` function intentionally omits the `$or` offline-status guard for non-bot agents when `isLivechatEnabledWhenAgentIdle === true`. This is by design: the setting `Livechat_enabled_when_agent_idle` (`accept_chats_when_agent_idle`) means agents should receive chats even when idle/offline, so the offline filter must be removed in that path. Bots are always status-agnostic and are always included regardless of their online/offline status. Do not flag this as a bug.
Applied to files:
apps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-02-24T19:36:55.089Z
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').
Applied to files:
apps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-03-20T13:51:20.280Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:20.280Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-03-16T11:50:18.066Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:18.066Z
Learning: In `apps/meteor/tests/end-to-end/apps/` (and Livechat E2E tests broadly), calling `createAgent()` and `makeAgentAvailable()` immediately after `updateSetting('Livechat_enabled', true)` — without any intermediate sleep — is an established, non-flaky pattern used across 10+ tests in the codebase. Do not flag this sequence as a potential race condition or suggest adding a delay between them.
Applied to files:
apps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-03-09T21:20:12.687Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39386
File: apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts:12-15
Timestamp: 2026-03-09T21:20:12.687Z
Learning: In `apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts`, the early return `if (data.voipToken) return null` (Lines 13-15) is intentionally correct. VoIP token updates always include an `_id`, so they are handled by the `_id` lookup block above (Lines 5-9) and never reach this guard. The guard is only a safety net for edge cases where `_id` is absent or no document was found, preventing an incorrect `token + appName` fallback match for VoIP-only payloads.
Applied to files:
apps/meteor/app/ai/server/hooks.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/ai/server/hooks.ts
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
apps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/ai/server/hooks.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/ai/server/hooks.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/ai/server/hooks.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/ai/server/hooks.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/ai/server/hooks.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/app/ai/server/hooks.ts
📚 Learning: 2026-03-11T18:15:53.272Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/app/api/server/v1/rooms.ts:294-296
Timestamp: 2026-03-11T18:15:53.272Z
Learning: In Rocket.Chat's `rooms.mediaConfirm/:rid/:fileId` endpoint (apps/meteor/app/api/server/v1/rooms.ts), updating `file.name` from `bodyParams.fileName` without updating `file.path` is intentionally safe. The file path is keyed by `_id` (not by filename), so the stored path remains valid regardless of a rename. `file.name` only affects the display name in the message attachment; do not flag this as a path-divergence issue.
Applied to files:
apps/meteor/app/ai/server/hooks.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/ai/server/hooks.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/ai/server/hooks.ts
Comment on lines
+8
to
+10
| // 🔒 Rate limiter | ||
| const userLastRequest = new Map<string, number>(); | ||
| const COOLDOWN = 5000; |
Contributor
There was a problem hiding this comment.
Use a shared TTL-backed rate limit store here.
userLastRequest only throttles within the current process and never evicts entries. On multi-node deployments the 5-second cooldown is bypassable by hopping app instances, and the map keeps growing with every distinct author. This needs a shared store/cache entry with expiry instead of a module-local Map.
Also applies to: 45-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/ai/server/hooks.ts` around lines 8 - 10, Replace the
module-local Map-based rate limiter (userLastRequest and COOLDOWN) with a shared
TTL-backed store (e.g., Redis or your existing cache abstraction) so cooldowns
are honored across processes and entries auto-expire; update the check/set logic
used around the hooks that reference userLastRequest (the same places in lines
~45-54) to query the shared store for a per-user key, atomically set the key
with a 5s TTL when allowing a request (or use SETNX/EXPIRE or a
cache.put-with-ttl), and base throttle decisions on the presence/expiry of that
key instead of storing timestamps in the Map. Ensure the new store key uses a
unique prefix (e.g., "rate:ai:authorId") and handle store errors gracefully
where userLastRequest was previously referenced.
Comment on lines
+19
to
+21
| callbacks.add( | ||
| 'afterSaveMessage', | ||
| async (message, { room }) => { |
Contributor
There was a problem hiding this comment.
Move the OpenAI round-trip off the synchronous afterSaveMessage path.
apps/meteor/app/lib/server/lib/afterSaveMessage.ts awaits callbacks.run('afterSaveMessage', ...), and the runner in apps/meteor/server/lib/callbacks/callbacksBase.ts executes callbacks sequentially. Awaiting generateAIResponse() here turns provider latency/timeouts into slow message sends and accumulated pending work. Queue this after the placeholder is posted instead of blocking the save callback.
Also applies to: 95-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/ai/server/hooks.ts` around lines 19 - 21, The
afterSaveMessage callback currently awaits generateAIResponse inside
callbacks.add('afterSaveMessage', async (message, { room }) => { ... }), which
blocks callbacks.run (see callbacksBase.ts) and makes message saves slow; change
this so the callback posts the placeholder synchronously and does NOT await the
external OpenAI round-trip—instead schedule the real work to run asynchronously
(use Meteor.defer / setImmediate / your app's job queue) and pass the message
id/room to that background task which will call generateAIResponse() and update
the placeholder when done; apply the same change to the other handler(s)
referenced around lines 95-119 so no OpenAI network call is awaited inside the
afterSaveMessage callback chain.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/ai/server/hooks.ts">
<violation number="1" location="apps/meteor/app/ai/server/hooks.ts:61">
P2: Mention detection supports `ai.bot`, but prompt cleaning strips only `@ai`, so `@ai.bot` can be sent to the model and even trigger needless calls when it is the only content.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Author
|
Good catch 👍 Updated prompt cleaning to handle both @ai and @ai.bot consistently using a unified regex. This prevents leftover tokens and avoids triggering AI when only the mention is present. |
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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/ai/server/hooks.ts">
<violation number="1" location="apps/meteor/app/ai/server/hooks.ts:62">
P2: Cooldown is recorded before prompt validation, so mention-only messages can consume rate limit without triggering an AI request.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Author
|
Good catch 👍 Moved rate limiting after prompt validation so mention-only messages no longer consume cooldown. Also ensured consistent mention handling and thread-aware responses. |
Author
|
Is anyone check?? |
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.
🚀 Built-in AI Assistant for Rocket.Chat
✨ Features
🧠 Why this feature?
Modern chat platforms like Slack and Teams provide native AI assistants. This feature brings similar capabilities to Rocket.Chat while keeping it customizable and self-hostable.
⚙️ Configuration
📌 Future Improvements
Summary by CodeRabbit
New Features
Bug Fixes / Limits