feat: Adding scheduled messages#40310
Open
Polinarr60 wants to merge 8 commits intoRocketChat:developfrom
Open
Conversation
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: 762aba9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 43 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
|
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 end-to-end scheduled messages: client UI to schedule/view/cancel, REST endpoints, server cron to dispatch and retry scheduled messages, model and typing updates to mark/filter scheduled messages, and English i18n entries. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Client as Client (UI)
participant API as Server API
participant DB as Database
participant Cron as Cron Job
rect rgba(100, 150, 255, 0.5)
Note over User,Client: Schedule a message
User->>Client: Enter text & choose schedule
Client->>Client: Validate future datetime
Client->>API: POST /v1/chat.scheduleMessage
API->>API: Validate & authorize
API->>DB: Insert message with scheduled=true, scheduledAt
DB-->>API: Acknowledge
API-->>Client: Return created message
Client->>User: Show success toast
end
rect rgba(100, 200, 100, 0.5)
Note over Cron,DB: Cron processes scheduled messages
Cron->>DB: Query scheduled=true and scheduledAt <= now
DB-->>Cron: Return candidates
loop per message
Cron->>DB: Attempt atomic claim (set scheduled=false)
DB-->>Cron: Claim result
Cron->>API: Load sender & check canSendMessage
API-->>Cron: Authorization result
Cron->>API: sendMessage for delivery
API->>DB: Persist sent message
Cron->>DB: Delete scheduled record (if send succeeded)
end
end
rect rgba(255, 150, 100, 0.5)
Note over User,Client: View & cancel scheduled messages
User->>Client: Open "View scheduled messages"
Client->>API: GET /v1/chat.getScheduledMessages
API->>DB: Query scheduled messages for room (paginated)
DB-->>API: Return results
API-->>Client: Return messages + counts
Client->>User: Display list
User->>Client: Request cancel
Client->>API: POST /v1/chat.cancelScheduledMessage
API->>DB: Delete scheduled message
DB-->>API: Deleted
API-->>Client: Return success
Client->>User: Show success toast & refresh
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/models/src/models/Messages.ts (1)
84-106:⚠️ Potential issue | 🔴 CriticalSeveral "visible" query methods still surface scheduled (unsent) messages — drift vs the methods you updated.
You added
scheduled: { $ne: true }to six find variants, but multiple sibling methods that participate in the same "visible" semantics were not updated. This causes user-visible inconsistencies and at least one count/find pagination drift:
countVisibleByRoomIdBetweenTimestampsNotContainingTypes(Lines 980–1014) — itsfindVisibleByRoomIdBetweenTimestampsNotContainingTypessibling now excludes scheduled messages, but this counter does not. Anything paginating withcountthen iterating withfindwill see an off-by-N total.findVisibleByMentionAndRoomId/findPaginatedVisibleByMentionAndRoomId(Lines 84–106) — a user mentioned inside someone else's scheduled draft will see the mention in their list before the message is actually sent. Privacy/UX leak.findVisibleByIds(Lines 772–781) — direct id lookups still return scheduled messages.findVisibleThreadByThreadId(Lines 783–793) — scheduled replies leak into thread views.getLastVisibleUserMessageSentByRoomId(Lines 1058–1073) — can return a future-dated scheduled message as "last visible," which then drives edit-last-message and similar flows.countVisibleByRoomIdBetweenTimestampsInclusive(Line 681) — same count drift class as above.findForUpdates(Line 860),countPinned/findPaginatedPinnedByRoom/countStarred/findStarredByUserAtRoom— lower risk but still drift from the new "visible excludes scheduled" invariant.Recommend extracting the visibility predicate (
{ _hidden: { $ne: true }, scheduled: { $ne: true } }) into a helper and applying it consistently across every "visible*" / "countVisible*" method, especially the count↔find pairs.Also applies to: 772-793, 980-1014, 1058-1073
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/Messages.ts` around lines 84 - 106, The visible-query methods are leaking scheduled (unsent) messages; update findVisibleByMentionAndRoomId and findPaginatedVisibleByMentionAndRoomId to include scheduled: { $ne: true } in their query, and factor the common visibility predicate into a helper (e.g., getVisibleMessageFilter or VISIBLE_MESSAGE_FILTER) that returns { _hidden: { $ne: true }, scheduled: { $ne: true } } so you can reuse it; then apply that helper consistently to all "visible*" and "countVisible*" methods called out in the review (findVisibleByIds, findVisibleThreadByThreadId, getLastVisibleUserMessageSentByRoomId, countVisibleByRoomIdBetweenTimestampsNotContainingTypes, countVisibleByRoomIdBetweenTimestampsInclusive, findForUpdates, countPinned/findPaginatedPinnedByRoom, countStarred/findStarredByUserAtRoom) to eliminate count/find drift and mention/thread leaks.apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
502-515:⚠️ Potential issue | 🟠 MajorSchedule menu should be disabled or hidden when uploading, processing, or recording — not just gated by
canSend.The scheduleMenu is correctly gated by
canSendat the parent level (line 502), so it won't render when the user lacks send permission. However, it lacks gating forisRecording,isUploading, andisProcessingUploadsstates. The Send button disables on all three, but the schedule menu remains clickable while uploads or recording are in progress. Either pass these states touseScheduleMessageButtonso the hook can disable itself (via adisabledprop onGenericMenu), or conditionally render the menu only when!isRecording && !isUploading && !isProcessingUploads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx` around lines 502 - 515, The scheduleMenu is rendered when canSend is true but is not disabled during isRecording, isUploading, or isProcessingUploads; update either the call to useScheduleMessageButton (pass a disabled prop derived from isRecording || isUploading || isProcessingUploads) so the hook sets GenericMenu's disabled state, or wrap scheduleMenu with an additional conditional render so it only renders when canSend && !isRecording && !isUploading && !isProcessingUploads; locate usages of scheduleMenu and the hook useScheduleMessageButton in MessageBox.tsx and ensure the GenericMenu produced by the hook respects the disabled flag.
🧹 Nitpick comments (7)
packages/models/src/models/Messages.ts (1)
46-82: Add an index to support the scheduled-messages cron.
apps/meteor/server/cron/scheduledMessages.tspolls every minute withMessages.find({ scheduled: true, scheduledAt: { $lte: now } }). None of the indexes declared here coverscheduledorscheduledAt, so this becomes a recurring full-collection scan on the largest collection in the system.📈 Suggested index
{ key: { 'federation.eventId': 1 }, sparse: true }, { key: { t: 1 }, sparse: true }, + { key: { scheduledAt: 1 }, sparse: true, partialFilterExpression: { scheduled: true } }, ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/Messages.ts` around lines 46 - 82, The scheduled-messages cron query (Messages.find({ scheduled: true, scheduledAt: { $lte: now } })) is not supported by any existing index; update the Messages modelIndexes() (in the Messages class) to add an index on scheduled and scheduledAt — e.g. a compound index { scheduled: 1, scheduledAt: 1 } with a partialFilterExpression { scheduled: true } (or sparse:true) so the cron query can use the index and avoid full collection scans.apps/meteor/server/startup/cron.ts (1)
22-22: Operational: ensure scheduled-message poll is supported by an index.Per the relevant snippet,
scheduledMessagesCron()runs every minute and executesMessages.find({ scheduled: true, scheduledAt: { $lte: now } }).MessagesRaw.modelIndexes()(seepackages/models/src/models/Messages.ts) has no index coveringscheduled/scheduledAt, so this becomes a recurring full-collection scan as the messages collection grows. Add a sparse partial index, e.g.:{ key: { scheduledAt: 1 }, sparse: true, partialFilterExpression: { scheduled: true } },Also worth confirming the 1-minute granularity is acceptable for the UX (users picking "send at 14:30:30" will see delivery up to ~60s late).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/startup/cron.ts` at line 22, The scheduled-messages cron (scheduledMessagesCron) performs Messages.find({ scheduled: true, scheduledAt: { $lte: now } }) every minute but there is no index for scheduled/scheduledAt in the Messages model, causing full collection scans; update the model index list in MessagesRaw.modelIndexes (in the Messages model) to add a partial/sparse index on scheduledAt that only covers scheduled messages (e.g., an index with key { scheduledAt: 1 } and partialFilterExpression { scheduled: true } or sparse: true) so the query can use an index; also confirm that the current 1-minute cron granularity is acceptable for UX (possible up-to-60s delivery delay).apps/meteor/app/api/server/index.ts (1)
13-14: Consider consolidating with./v1/chat.The new endpoints are still part of the
chat.*namespace; co-locating their registration in./v1/chat(as siblings of the existing chat endpoints) would avoid a separate side-effect-only module and keep route discovery centralized. Not a blocker if you prefer to keep scheduling concerns isolated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/index.ts` around lines 13 - 14, The two separate side-effect-only imports (./v1/chat.scheduleMessage and ./v1/chat) register endpoints in the same chat.* namespace; move the route/endpoint registration logic from ./v1/chat.scheduleMessage into the existing ./v1/chat module so all chat.* routes are declared as siblings in one file, then remove the standalone import of ./v1/chat.scheduleMessage from apps/meteor/app/api/server/index.ts; update any export/initialization in ./v1/chat as needed to ensure scheduleMessage registration runs on import.apps/meteor/client/views/room/composer/messageBox/hooks/useScheduleMessageButton.tsx (1)
11-11: Add an explicit return type to the hook.The hook returns
{ scheduleMenu: JSX.Element }but the return type is inferred. Add an explicit return type for clarity and to keep the public surface stable across edits.♻️ Suggested signature
-export const useScheduleMessageButton = (roomId: string, tmid?: string) => { +export const useScheduleMessageButton = (roomId: string, tmid?: string): { scheduleMenu: ReactElement } => {(also
import type { ReactElement } from 'react';)As per coding guidelines: "Write concise, technical TypeScript/JavaScript with accurate typing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/composer/messageBox/hooks/useScheduleMessageButton.tsx` at line 11, The hook useScheduleMessageButton currently relies on an inferred return type; update its signature to an explicit return type of { scheduleMenu: ReactElement } (or JSX.Element) and add an import type { ReactElement } from 'react'; ensure the function declaration exports: export const useScheduleMessageButton = (roomId: string, tmid?: string): { scheduleMenu: ReactElement } => { ... } so the public surface remains stable.apps/meteor/app/api/server/v1/chat.scheduleMessage.ts (1)
7-9: Operational hardening: bound and rate-limit scheduling.Three concerns to consider before shipping:
- Cap the number of pending scheduled messages per user (and globally per room) to avoid abuse / cron starvation.
- Cap how far in the future
scheduledAtmay be (e.g., 1 year). Right now any future timestamp is accepted, which can keep "ghost" messages in storage indefinitely.- Add this endpoint to the standard rate-limiter (most chat.* endpoints set
rateLimiterOptions).These can be applied via the existing
rateLimiterOptionsconfig plus a guarded count viaMessages.countDocuments({ scheduled: true, 'u._id': this.userId }).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/chat.scheduleMessage.ts` around lines 7 - 9, Add operational guards to the API.v1 chat schedule endpoint: attach appropriate rateLimiterOptions when registering the route (consistent with other chat.* endpoints) and enforce limits before saving a scheduled message by (1) checking per-user pending scheduled count via Messages.countDocuments({ scheduled: true, 'u._id': this.userId }) and rejecting when it exceeds the per-user cap, (2) checking per-room pending scheduled count via Messages.countDocuments({ scheduled: true, rid: roomId }) and rejecting when it exceeds the per-room cap, and (3) validating scheduledAt to reject timestamps beyond a maximum allowed horizon (e.g., now + 1 year); return explicit errors for each violation. Ensure these checks run in the same handler that registers under API.v1 so they prevent creation and are covered by the rate limiter.packages/rest-typings/src/v1/chat.ts (1)
1019-1046: Add AJV validators alongside the new endpoint typings.Every other endpoint in this module (e.g.,
isChatPostMessageProps,isChatGetThreadsListProps) ships anajv.compile/ajvQuery.compile-derived validator. The three new scheduled endpoints are declared inline only, with no exportedis*Propsguard, which breaks the established pattern and prevents reuse by callers that need runtime validation. DefineChatScheduleMessage,ChatGetScheduledMessages,ChatCancelScheduledMessagetypes + JSON schemas and export the correspondingajv.compilevalidators (or move them to thechat/scheduled.tsmodule the AI summary mentions and re-export from here).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rest-typings/src/v1/chat.ts` around lines 1019 - 1046, The new inline endpoints lack runtime AJV validators; add concrete types ChatScheduleMessage, ChatGetScheduledMessages, ChatCancelScheduledMessage and corresponding JSON schemas, then export AJV-compiled guards (e.g., isChatScheduleMessageProps, isChatGetScheduledMessagesProps, isChatCancelScheduledMessageProps) using ajv.compile/ajvQuery.compile consistent with existing validators; either define these types/schemas and exported validators in this file or implement them in chat/scheduled.ts and re-export the compiled validators here so callers can perform runtime validation the same way as other endpoints like isChatPostMessageProps.apps/meteor/client/views/room/composer/messageBox/MessageBoxScheduledMessagesModal.tsx (1)
23-41: PreferuseCallbackand removeanyfrom the response mapping.Two small but concrete improvements:
useMemo(() => async () => { ... })returns the function as the memoized value — that is the exact role ofuseCallback. Switch touseCallbackfor idiomatic React and to match the rest of this file.(msg: any) => …and theas anycasts hide that REST responses arrive as JSON (string dates), which is exactly the conversion happening on lines 30–31. Type the response shape explicitly so theDate | stringunion is enforced rather than silenced.♻️ Suggested change
- const loadScheduledMessages = useMemo( - () => async () => { - try { - setLoading(true); - const result = await getScheduledMessages({ roomId }); - const messages = result.messages.map((msg: any) => ({ - ...msg, - ts: typeof msg.ts === 'string' ? new Date(msg.ts) : msg.ts, - scheduledAt: typeof msg.scheduledAt === 'string' ? new Date(msg.scheduledAt) : msg.scheduledAt, - })); - setScheduledMessages(messages); - } catch (error: any) { - dispatchToastMessage({ type: 'error', message: error.message }); - } finally { - setLoading(false); - } - }, - [roomId, getScheduledMessages, dispatchToastMessage], - ); + const loadScheduledMessages = useCallback(async () => { + try { + setLoading(true); + const result = await getScheduledMessages({ roomId }); + const messages = result.messages.map((msg) => ({ + ...msg, + ts: typeof msg.ts === 'string' ? new Date(msg.ts) : msg.ts, + scheduledAt: + typeof msg.scheduledAt === 'string' ? new Date(msg.scheduledAt) : msg.scheduledAt, + })); + setScheduledMessages(messages); + } catch (error) { + dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : t('Error') }); + } finally { + setLoading(false); + } + }, [roomId, getScheduledMessages, dispatchToastMessage, t]);Apply the same
error instanceof Error ? error.message : t('Error')pattern inhandleCancel(line 53) so users don't see a literalundefinedtoast when the SDK throws a non-Errorvalue.As per coding guidelines: "Write concise, technical TypeScript/JavaScript with accurate typing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/views/room/composer/messageBox/MessageBoxScheduledMessagesModal.tsx` around lines 23 - 41, Replace the useMemo wrapper with useCallback for the async loader (rename/keep loadScheduledMessages) and strictly type the REST response instead of using any: define an interface for the scheduled message payload (e.g. fields including ts: string | Date and scheduledAt: string | Date), call getScheduledMessages(): Promise<{ messages: ScheduledMessagePayload[] }>, map each msg to convert string dates to Date and pass to setScheduledMessages; also update handleCancel to use error instanceof Error ? error.message : t('Error') when dispatching the toast so non-Error throws don't produce "undefined".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/rare-llamas-behave.md:
- Around line 1-72: The changeset file .changeset/rare-llamas-behave.md
currently lists ~40 packages with no actual code changes; update it to include
only packages that had real modifications by removing entries for packages with
zero file changes (e.g., `@rocket.chat/favicon`, `@rocket.chat/peggy-loader`,
`@rocket.chat/jest-presets`, `@rocket.chat/eslint-config`,
`@rocket.chat/password-policies`, `@rocket.chat/cas-validate`, `@rocket.chat/tracing`,
`@rocket.chat/logger`, `@rocket.chat/jwt`, `@rocket.chat/cron`, etc.), leaving only
the true-changed packages such as `@rocket.chat/meteor`, `@rocket.chat/apps-engine`,
`@rocket.chat/rest-typings`, `@rocket.chat/core-typings`,
`@rocket.chat/model-typings`, `@rocket.chat/models`, `@rocket.chat/i18n` and the other
~20–25 packages you confirmed; ensure the final YAML list in
.changeset/rare-llamas-behave.md matches the actual diff (remove extraneous
package lines and save the file).
In `@apps/meteor/app/api/server/v1/chat.scheduleMessage.ts`:
- Around line 79-83: The query params (this.queryParams) are strings so the
check calls using Match.Maybe(Number) will fail; convert/normalize count and
offset to numbers before validating (e.g., parseInt/Number with fallback
defaults) or reuse the existing pagination helper getPaginationItems to parse
and validate them; update the destructuring in chat.scheduleMessage.ts to accept
raw strings, coerce count and offset to numeric values, then run check against
Number (or validate the parsed values) and proceed using the parsed numeric
count/offset.
- Around line 136-153: The cancel flow is non-atomic: replace the separate
Messages.findOneById + checks + Messages.deleteOne with a single conditional
atomic delete on Messages that includes _id, 'u._id' equal to this.userId, and
scheduled: true (e.g., deleteOne({ _id: messageId, 'u._id': this.userId,
scheduled: true })), then inspect the delete result/deletedCount and throw the
appropriate Meteor.Error ('error-message-not-found' / 'error-not-allowed' /
'error-not-scheduled') if nothing was deleted; keep the same error
types/messages and method names (chat.cancelScheduledMessage) so callers/crons
can rely on them, and also ensure the scheduledMessages cron uses a similar
atomic claim pattern as noted.
- Line 1: Remove the file-level "// `@ts-nocheck`" from chat.scheduleMessage.ts
and add proper TypeScript types and AJV-validated runtime checks: replace any
uses of loose types like messageData: any and untyped
this.bodyParams/this.queryParams with the corresponding is*Props validators from
packages/rest-typings (see v1/chat.ts) to validate and type the request shape,
then type the Messages.insertOne call with the correct
MessageInsert/MessageDocument type so the insert argument is fully typed and
validated before insertion; ensure you import the appropriate isXProps
validators and apply them to parse/validate request data prior to using
messageData or calling Messages.insertOne.
- Around line 32-43: Replace the read-only permission check with a write/send
permission check: call canSendMessageAsync(roomId, this.userId) instead of
canAccessRoomIdAsync and throw the same Meteor.Error('error-not-allowed', ...)
when it fails; remove the now-duplicate Users.findOneById lookup that the
comment mentions; additionally validate the message body is non-empty after
trimming and enforce the Message_MaxAllowedSize setting before scheduling so the
server does not accept oversized or empty messages; keep the existing
date-parsing/validation (scheduledDate, scheduledAt) and error types
(error-invalid-date, error-past-date) intact.
In `@apps/meteor/client/lib/chats/flows/scheduleMessage.ts`:
- Line 26: Replace the misleading success toast that uses t('Message_sent') when
a message is scheduled: update both dispatchToastMessage call sites in
scheduleMessage.ts (the dispatchToastMessage(...) in the scheduling flow) and
useScheduleMessageButton.tsx (the dispatchToastMessage(...) at line ~38) to use
a scheduling-specific i18n key such as t('Message_scheduled'), and add the
corresponding "Message_scheduled" entries to all locale files so translations
are available; ensure the toast type remains 'success' and the message string
uses the new key consistently in both locations.
- Around line 7-31: The exported function scheduleMessage is dead code; either
remove it or refactor callers to use it for consistency. Option A: delete the
scheduleMessage export and its file-level export if no other references exist
(remove scheduleMessage from
apps/meteor/client/lib/chats/flows/scheduleMessage.ts). Option B: update
useScheduleMessageButton to call the scheduleMessage flow instead of directly
using useEndpoint('POST', '/v1/chat.scheduleMessage')—import scheduleMessage
into useScheduleMessageButton, adapt its payload to match scheduleMessage({ rid,
msg, scheduledAt, tmid }) and handle success/error the same way as sendMessage
is used in ChatMessages.ts. Ensure only one consistent pattern remains across
sendMessage, scheduleMessage, and useScheduleMessageButton.
In
`@apps/meteor/client/views/room/composer/messageBox/MessageBoxScheduledMessagesModal.tsx`:
- Around line 99-108: The Cancel button is icon-only and relies on title which
isn't a reliable accessible name; in the MessageBoxScheduledMessagesModal
component update the Button (the one rendering <Icon name='trash' /> and calling
handleCancel(message._id)) to include an explicit aria-label using the
translated string (aria-label={t('Cancel')}) so screen readers announce the
action.
In
`@apps/meteor/client/views/room/composer/messageBox/MessageBoxScheduleModal.tsx`:
- Around line 15-48: The code mixes UTC strings from toISOString() with local
time inputs causing wrong defaults and min validation; fix by using local-based
formatting and local Date construction: replace defaultDate/defaultTime and
currentDate/currentTime (now and tomorrow) generation with helper local
formatters (e.g., formatLocalDate(now) -> "YYYY-MM-DD" and formatLocalTime(now)
-> "HH:MM") so defaultDate/defaultTime, currentDate/currentTime and minDate use
local calendar values, compute tomorrow by adding 1 day on the local Date,
compute minTime as current local time when date === currentDate, and in
handleSchedule replace new Date(`${date}T${time}`) with a local constructor (new
Date(year, month-1, day, hour, minute)) parsed from the date/time strings so
scheduledAt is built in local timezone; update references to defaultDate,
defaultTime, currentDate, currentTime, minDate, minTime and the scheduledAt
creation in handleSchedule accordingly.
In `@apps/meteor/server/cron/scheduledMessages.ts`:
- Around line 46-50: The catch block around sendMessage currently deletes the
scheduledMessage on any error and swallows the exception; change the catch in
the send flow (where scheduledMessage, messageDeleted, sendMessage and
Messages.deleteOne are referenced) to (1) log the error with structured context
(include scheduledMessage._id, scheduledMessage.rid, scheduledMessage.userId or
authorId) using the project's logger, (2) do not delete the record on transient
failures — instead update the scheduledMessage with a failure marker (e.g., set
scheduledFailedAt to new Date() and optionally increment a retryCount) via
Messages.updateOne({ _id: scheduledMessage._id }, { $set: { scheduledFailedAt:
... } }), and (3) rethrow or surface the error so it isn't silently swallowed
(or at minimum emit/record a metric/alert) so operational teams can investigate.
Ensure messageDeleted still controls permanent deletion only when appropriate.
- Around line 10-13: The current Messages.find({ scheduled: true, scheduledAt: {
$lte: now } }).toArray() is racy and unbounded; replace the read-then-delete
pattern with an atomic per-message claim and bounded batch: issue a limited
query (e.g., limit N) and for each candidate use an atomic findOneAndUpdate on
Messages with the same filter plus a not-yet-claimed predicate (e.g., { _id: id,
scheduled: true, scheduledAt: { $lte: now }, claimed: { $ne: true } }) setting
claimed=true and claimedAt=now (or a worker id) — only proceed to call
sendMessage for documents returned by that findOneAndUpdate; remove the
standalone deleteOne and the messageDeleted flag usage, and iterate in bounded
batches until no more documents are claimable to avoid OOM and duplicate sends.
In `@packages/rest-typings/src/v1/chat.ts`:
- Around line 1019-1033: The TypeScript typing for the
'/v1/chat.scheduleMessage' POST params advertises fields the server's check() in
chat.scheduleMessage does not accept; either remove
alias/avatar/emoji/attachments/customFields from the POST params or extend the
server-side check() to validate and accept those fields so they aren't rejected
at runtime—also replace attachments?: any[] with attachments?:
MessageAttachment[] and customFields?: IMessage['customFields'] to use accurate
types; update the POST params definition (the '/v1/chat.scheduleMessage' POST
params object) and/or the server-side check() in chat.scheduleMessage
accordingly so types and runtime validation match.
In `@packages/rest-typings/src/v1/chat/scheduled.ts`:
- Around line 26-31: The schema is using "nullable: true" for properties (tmid,
alias, avatar, emoji, attachments, customFields, and the ones noted around lines
49-50) which allows null at runtime but TypeScript types are optional (T |
undefined); either remove "nullable: true" from these property schemas to match
the TS types, or update the TS declarations to allow null (e.g. field?: T |
null) for each listed property; locate the property entries in the scheduled
message schema (the object containing tmid, alias, avatar, emoji, attachments,
customFields) and apply the chosen change consistently.
- Around line 16-17: Replace the loose any types for attachments and
customFields with the project's established types: change attachments?: any[] to
attachments?: MessageAttachment[] and change customFields?: Record<string, any>
to customFields?: IMessage['customFields'] (or the exact customFields type
exported by IMessage), and add the necessary imports for MessageAttachment and
IMessage from the core-typings module so the new types are consistent with the
main chat.ts and IMessage definitions.
---
Outside diff comments:
In `@apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx`:
- Around line 502-515: The scheduleMenu is rendered when canSend is true but is
not disabled during isRecording, isUploading, or isProcessingUploads; update
either the call to useScheduleMessageButton (pass a disabled prop derived from
isRecording || isUploading || isProcessingUploads) so the hook sets
GenericMenu's disabled state, or wrap scheduleMenu with an additional
conditional render so it only renders when canSend && !isRecording &&
!isUploading && !isProcessingUploads; locate usages of scheduleMenu and the hook
useScheduleMessageButton in MessageBox.tsx and ensure the GenericMenu produced
by the hook respects the disabled flag.
In `@packages/models/src/models/Messages.ts`:
- Around line 84-106: The visible-query methods are leaking scheduled (unsent)
messages; update findVisibleByMentionAndRoomId and
findPaginatedVisibleByMentionAndRoomId to include scheduled: { $ne: true } in
their query, and factor the common visibility predicate into a helper (e.g.,
getVisibleMessageFilter or VISIBLE_MESSAGE_FILTER) that returns { _hidden: {
$ne: true }, scheduled: { $ne: true } } so you can reuse it; then apply that
helper consistently to all "visible*" and "countVisible*" methods called out in
the review (findVisibleByIds, findVisibleThreadByThreadId,
getLastVisibleUserMessageSentByRoomId,
countVisibleByRoomIdBetweenTimestampsNotContainingTypes,
countVisibleByRoomIdBetweenTimestampsInclusive, findForUpdates,
countPinned/findPaginatedPinnedByRoom, countStarred/findStarredByUserAtRoom) to
eliminate count/find drift and mention/thread leaks.
---
Nitpick comments:
In `@apps/meteor/app/api/server/index.ts`:
- Around line 13-14: The two separate side-effect-only imports
(./v1/chat.scheduleMessage and ./v1/chat) register endpoints in the same chat.*
namespace; move the route/endpoint registration logic from
./v1/chat.scheduleMessage into the existing ./v1/chat module so all chat.*
routes are declared as siblings in one file, then remove the standalone import
of ./v1/chat.scheduleMessage from apps/meteor/app/api/server/index.ts; update
any export/initialization in ./v1/chat as needed to ensure scheduleMessage
registration runs on import.
In `@apps/meteor/app/api/server/v1/chat.scheduleMessage.ts`:
- Around line 7-9: Add operational guards to the API.v1 chat schedule endpoint:
attach appropriate rateLimiterOptions when registering the route (consistent
with other chat.* endpoints) and enforce limits before saving a scheduled
message by (1) checking per-user pending scheduled count via
Messages.countDocuments({ scheduled: true, 'u._id': this.userId }) and rejecting
when it exceeds the per-user cap, (2) checking per-room pending scheduled count
via Messages.countDocuments({ scheduled: true, rid: roomId }) and rejecting when
it exceeds the per-room cap, and (3) validating scheduledAt to reject timestamps
beyond a maximum allowed horizon (e.g., now + 1 year); return explicit errors
for each violation. Ensure these checks run in the same handler that registers
under API.v1 so they prevent creation and are covered by the rate limiter.
In
`@apps/meteor/client/views/room/composer/messageBox/hooks/useScheduleMessageButton.tsx`:
- Line 11: The hook useScheduleMessageButton currently relies on an inferred
return type; update its signature to an explicit return type of { scheduleMenu:
ReactElement } (or JSX.Element) and add an import type { ReactElement } from
'react'; ensure the function declaration exports: export const
useScheduleMessageButton = (roomId: string, tmid?: string): { scheduleMenu:
ReactElement } => { ... } so the public surface remains stable.
In
`@apps/meteor/client/views/room/composer/messageBox/MessageBoxScheduledMessagesModal.tsx`:
- Around line 23-41: Replace the useMemo wrapper with useCallback for the async
loader (rename/keep loadScheduledMessages) and strictly type the REST response
instead of using any: define an interface for the scheduled message payload
(e.g. fields including ts: string | Date and scheduledAt: string | Date), call
getScheduledMessages(): Promise<{ messages: ScheduledMessagePayload[] }>, map
each msg to convert string dates to Date and pass to setScheduledMessages; also
update handleCancel to use error instanceof Error ? error.message : t('Error')
when dispatching the toast so non-Error throws don't produce "undefined".
In `@apps/meteor/server/startup/cron.ts`:
- Line 22: The scheduled-messages cron (scheduledMessagesCron) performs
Messages.find({ scheduled: true, scheduledAt: { $lte: now } }) every minute but
there is no index for scheduled/scheduledAt in the Messages model, causing full
collection scans; update the model index list in MessagesRaw.modelIndexes (in
the Messages model) to add a partial/sparse index on scheduledAt that only
covers scheduled messages (e.g., an index with key { scheduledAt: 1 } and
partialFilterExpression { scheduled: true } or sparse: true) so the query can
use an index; also confirm that the current 1-minute cron granularity is
acceptable for UX (possible up-to-60s delivery delay).
In `@packages/models/src/models/Messages.ts`:
- Around line 46-82: The scheduled-messages cron query (Messages.find({
scheduled: true, scheduledAt: { $lte: now } })) is not supported by any existing
index; update the Messages modelIndexes() (in the Messages class) to add an
index on scheduled and scheduledAt — e.g. a compound index { scheduled: 1,
scheduledAt: 1 } with a partialFilterExpression { scheduled: true } (or
sparse:true) so the cron query can use the index and avoid full collection
scans.
In `@packages/rest-typings/src/v1/chat.ts`:
- Around line 1019-1046: The new inline endpoints lack runtime AJV validators;
add concrete types ChatScheduleMessage, ChatGetScheduledMessages,
ChatCancelScheduledMessage and corresponding JSON schemas, then export
AJV-compiled guards (e.g., isChatScheduleMessageProps,
isChatGetScheduledMessagesProps, isChatCancelScheduledMessageProps) using
ajv.compile/ajvQuery.compile consistent with existing validators; either define
these types/schemas and exported validators in this file or implement them in
chat/scheduled.ts and re-export the compiled validators here so callers can
perform runtime validation the same way as other endpoints like
isChatPostMessageProps.
🪄 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: d0f58417-c1d7-4b0c-8ae0-84f9e93d6419
📒 Files selected for processing (18)
.changeset/rare-llamas-behave.mdapps/meteor/app/api/server/index.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.tsapps/meteor/client/lib/chats/flows/scheduleMessage.tsapps/meteor/client/views/room/composer/messageBox/MessageBox.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/MessageBoxActionsToolbar.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxActionsToolbar/hooks/useViewScheduledMessagesAction.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxScheduleMenu.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxScheduleModal.tsxapps/meteor/client/views/room/composer/messageBox/MessageBoxScheduledMessagesModal.tsxapps/meteor/client/views/room/composer/messageBox/hooks/useScheduleMessageButton.tsxapps/meteor/server/cron/scheduledMessages.tsapps/meteor/server/startup/cron.tspackages/core-typings/src/IMessage/IMessage.tspackages/i18n/src/locales/en.i18n.jsonpackages/models/src/models/Messages.tspackages/rest-typings/src/v1/chat.tspackages/rest-typings/src/v1/chat/scheduled.ts
coderabbitai
Bot
removed
the
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/meteor/app/api/server/v1/chat.scheduleMessage.ts (2)
30-73:⚠️ Potential issue | 🟡 MinorServer-side
messagevalidation (non-empty +Message_MaxAllowedSize) is missing.The endpoint inserts straight into
MessagesviaMessages.insertOne(messageData as IMessage), bypassing thevalidateMessage/length checks that the normalsendMessageflow runs. A client can therefore schedule an empty (or whitespace-only)msg, or a message exceedingMessage_MaxAllowedSize, which will then be delivered by the cron a minute later and cause errors at delivery (or pollute the room).Validate before insertion:
message.trim().length > 0message.length <= settings.get<number>('Message_MaxAllowedSize')Throw a clear
Meteor.Error('error-invalid-message', ...)on failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/chat.scheduleMessage.ts` around lines 30 - 73, The scheduled message handler currently inserts user content directly via Messages.insertOne (see messageData / Messages.insertOne) without validating content; before building/inserting messageData in chat.scheduleMessage, validate that message.trim().length > 0 and that message.length <= settings.get<number>('Message_MaxAllowedSize'), and if either check fails throw new Meteor.Error('error-invalid-message', 'Invalid message', { method: 'chat.scheduleMessage' }); perform these checks immediately after extracting message/scheduledAt and before calling canSendMessageAsync or any DB insert so invalid messages are rejected server-side.
13-35: 🛠️ Refactor suggestion | 🟠 MajorReplace
this: anyandmeteor/checkwith route-level AJV body/query schemas.All three handlers (lines 29, 106, 165) declare
this: anyand rely onmeteor/checkfor input validation, but the route options only define response schemas — nobody/queryschemas. This:
- Violates the coding guideline on accurate typing (the parameters of every endpoint are effectively
any).- Diverges from the established pattern of other migrated endpoints in
apps/meteor/app/api/server/v1/, which validatebodyandqueryvia AJV (is*Props-style validators).- Causes the redundant runtime guard at line 110 (
check(roomId, String)after the value was already destructured and partially used bygetPaginationItems).A corresponding
is*Propsset should live inpackages/rest-typings/src/v1/chat/scheduled.ts(the file already appears in this PR's import surface) so the route can be wired withbody: isChatScheduleMessagePropsetc., and themeteor/checkcalls can be dropped.As per coding guidelines: "Write concise, technical TypeScript/JavaScript with accurate typing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/chat.scheduleMessage.ts` around lines 13 - 35, The route's action uses an untyped this: any and runtime validation via meteor/check (check(this.bodyParams, ...)) instead of a route-level AJV schema; replace the runtime checks by adding an AJV body schema (e.g., body: isChatScheduleMessageProps) to the route options, remove the meteor/check calls inside the action, and update the action signature to a properly typed handler (remove this: any) so request params are typed; add the isChatScheduleMessageProps validator in packages/rest-typings/src/v1/chat/scheduled.ts (or use the existing one if present) and wire it into the route options for chat.scheduleMessage (and the other two handlers at lines noted) to match the is*Props pattern used across v1 endpoints.
🧹 Nitpick comments (3)
apps/meteor/server/cron/scheduledMessages.ts (2)
48-58: Type the dispatch payload instead ofany.
const message: any = { ... }defeats type-checking on the call tosendMessage. Use the matchingPartial<IMessage>(or the parameter type accepted bysendMessage) so accidental field renames inIMessage(e.g.,tmid,attachments) get caught at compile time. Same for the inner field-presence checks — they can be expressed via a typed builder.As per coding guidelines: "Write concise, technical TypeScript/JavaScript with accurate typing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/cron/scheduledMessages.ts` around lines 48 - 58, The current message object is typed as any which bypasses compile-time checks; change the declaration to the correct type (e.g., Partial<IMessage> or the exact parameter type expected by sendMessage) and build the object using that type so missing/renamed fields (tmid, attachments, alias, avatar, emoji, etc.) are caught by the compiler; update the construction around the message variable and the call to sendMessage(user, message, room, {}) to use this typed payload and prefer assigning optional properties via typed conditionals or a typed builder to avoid using && spreads with an any type.
4-6: Simplify theSystemLoggerimport path for consistency.The current import
'../../server/lib/logger/system'unnecessarily traverses through the root directory. Other files inapps/meteor/server/cron/import their sibling server modules using the shorter form:userDataDownloads.tsuses'../lib/dataExport'andoembed.tsuses'../methods/OEmbedCacheCleanup'. Align this import to'../lib/logger/system'for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/cron/scheduledMessages.ts` around lines 4 - 6, Update the SystemLogger import in scheduledMessages.ts to use the shorter sibling-relative path; replace the current import from '../../server/lib/logger/system' with '../lib/logger/system' so the file imports SystemLogger consistently with other cron modules (refer to the SystemLogger symbol in scheduledMessages.ts).apps/meteor/app/api/server/v1/chat.scheduleMessage.ts (1)
118-130: Drop the redundantNumber(offset)/Number(count)casts.
getPaginationItemsalready parses and returns{ offset: number; count: number }. Wrapping them inNumber(...)again is dead conversion noise and slightly obscures the types. Pass them directly:♻️ Suggested change
- { - sort: { scheduledAt: 1 }, - skip: Number(offset), - limit: Number(count), - }, + { + sort: { scheduledAt: 1 }, + skip: offset, + limit: count, + },Also consider replacing the two-query pattern (
find().toArray()+countDocuments) withMessages.findPaginated(...).toArray()and thetotalfrom the same call, mirroring other listing endpoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/chat.scheduleMessage.ts` around lines 118 - 130, The code redundantly wraps offset and count in Number(...) when getPaginationItems already returns numbers; update the Messages.find call to pass offset and count directly (use offset and count variables instead of Number(offset)/Number(count)) and optionally replace the two-query pattern (find(...).toArray() + countDocuments) with Messages.findPaginated(...) so you can call Messages.findPaginated({ rid: roomId, 'u._id': this.userId, scheduled: true, scheduledAt: { $exists: true } }, { sort: { scheduledAt: 1 }, skip: offset, limit: count }).toArray() and obtain the total from the same paginated call to mirror other listing endpoints.
🤖 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/api/server/v1/chat.scheduleMessage.ts`:
- Around line 59-73: When building messageData and before calling
Messages.insertOne, validate any provided tmid by fetching the parent message
(e.g., via Messages.findOne({_id: tmid})) and ensure the parent exists and
parent.rid === roomId; if not, throw/reject the request. This keeps the
same-room thread invariant enforced here (matching what sendMessage enforces at
delivery), so update the scheduling handler that constructs messageData (and the
call site around Messages.insertOne) to perform that lookup and validation and
abort insertion on mismatch.
- Around line 53-57: The try/catch around canSendMessageAsync in
chat.scheduleMessage (and the similar block in chat.getScheduledMessages) is
collapsing distinct errors into a generic Meteor.Error('error-not-allowed');
remove the catch so canSendMessageAsync's original errors propagate, or if you
must catch, rethrow the original error (throw error) instead of replacing it
with 'error-not-allowed', ensuring codes like 'error-invalid-room',
'room_is_archived', 'room_is_blocked', 'You_have_been_muted' and read-only
errors are preserved; update the blocks referencing canSendMessageAsync and any
Meteor.Error rethrows in scheduleMessage and getScheduledMessages accordingly.
In `@apps/meteor/server/cron/scheduledMessages.ts`:
- Around line 58-73: The catch-all reset is causing duplicate sends and infinite
retries; instead, change the workflow in scheduledMessages.ts so that you either
remove the DB row before calling sendMessage (rely on the existing atomic claim
around lines 20-32 and pass the document by value), or if you keep
deleteAfterSend, do NOT flip scheduled back to true when Messages.deleteOne
fails after a successful send—log the delete failure and leave scheduled:false
so it won't be re-delivered; additionally add a retry cap by introducing fields
like scheduledRetryCount and scheduledFailedAt on the message document,
increment scheduledRetryCount on send failures (use the existing catch path that
currently calls Messages.updateOne) and stop retrying / surface the failure once
scheduledRetryCount >= N (or set scheduledFailedAt) so malformed or persistent
errors become dead-lettered instead of being retried forever.
- Around line 40-46: The catch block around canSendMessageAsync currently
deletes the scheduled message silently; instead, log the failure with
messageId/roomId/userId using the project logger (e.g., processLogger.error or
Logger) and persist failure metadata on the scheduled message rather than
deleting it: update Messages (Messages.updateOne({ _id: scheduledMessage._id },
{ $set: { scheduledFailedAt: new Date(), failureReason: error?.message ||
String(error) } })) so the UI can surface the problem; only perform
Messages.deleteOne for explicit non-recoverable errors (e.g., user not found)
after checking error type/message. Ensure you reference canSendMessageAsync,
scheduledMessage, Messages.deleteOne/updateOne, scheduledFailedAt, and
failureReason when making the change.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/chat.scheduleMessage.ts`:
- Around line 30-73: The scheduled message handler currently inserts user
content directly via Messages.insertOne (see messageData / Messages.insertOne)
without validating content; before building/inserting messageData in
chat.scheduleMessage, validate that message.trim().length > 0 and that
message.length <= settings.get<number>('Message_MaxAllowedSize'), and if either
check fails throw new Meteor.Error('error-invalid-message', 'Invalid message', {
method: 'chat.scheduleMessage' }); perform these checks immediately after
extracting message/scheduledAt and before calling canSendMessageAsync or any DB
insert so invalid messages are rejected server-side.
- Around line 13-35: The route's action uses an untyped this: any and runtime
validation via meteor/check (check(this.bodyParams, ...)) instead of a
route-level AJV schema; replace the runtime checks by adding an AJV body schema
(e.g., body: isChatScheduleMessageProps) to the route options, remove the
meteor/check calls inside the action, and update the action signature to a
properly typed handler (remove this: any) so request params are typed; add the
isChatScheduleMessageProps validator in
packages/rest-typings/src/v1/chat/scheduled.ts (or use the existing one if
present) and wire it into the route options for chat.scheduleMessage (and the
other two handlers at lines noted) to match the is*Props pattern used across v1
endpoints.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/chat.scheduleMessage.ts`:
- Around line 118-130: The code redundantly wraps offset and count in
Number(...) when getPaginationItems already returns numbers; update the
Messages.find call to pass offset and count directly (use offset and count
variables instead of Number(offset)/Number(count)) and optionally replace the
two-query pattern (find(...).toArray() + countDocuments) with
Messages.findPaginated(...) so you can call Messages.findPaginated({ rid:
roomId, 'u._id': this.userId, scheduled: true, scheduledAt: { $exists: true } },
{ sort: { scheduledAt: 1 }, skip: offset, limit: count }).toArray() and obtain
the total from the same paginated call to mirror other listing endpoints.
In `@apps/meteor/server/cron/scheduledMessages.ts`:
- Around line 48-58: The current message object is typed as any which bypasses
compile-time checks; change the declaration to the correct type (e.g.,
Partial<IMessage> or the exact parameter type expected by sendMessage) and build
the object using that type so missing/renamed fields (tmid, attachments, alias,
avatar, emoji, etc.) are caught by the compiler; update the construction around
the message variable and the call to sendMessage(user, message, room, {}) to use
this typed payload and prefer assigning optional properties via typed
conditionals or a typed builder to avoid using && spreads with an any type.
- Around line 4-6: Update the SystemLogger import in scheduledMessages.ts to use
the shorter sibling-relative path; replace the current import from
'../../server/lib/logger/system' with '../lib/logger/system' so the file imports
SystemLogger consistently with other cron modules (refer to the SystemLogger
symbol in scheduledMessages.ts).
🪄 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: e19e5d9a-e649-4dd6-b4e1-3a34beeac9ae
📒 Files selected for processing (5)
.changeset/rare-llamas-behave.mdapps/meteor/app/api/server/v1/chat.scheduleMessage.tsapps/meteor/server/cron/scheduledMessages.tspackages/rest-typings/src/v1/chat.tspackages/rest-typings/src/v1/chat/scheduled.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/rare-llamas-behave.md
- packages/rest-typings/src/v1/chat/scheduled.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rest-typings/src/v1/chat.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/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.ts
🧠 Learnings (33)
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-03-23T19:33:46.159Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 39818
File: apps/meteor/client/hooks/usePruneWarningMessage.ts:45-56
Timestamp: 2026-03-23T19:33:46.159Z
Learning: In RocketChat/Rocket.Chat, cron expressions used by the retention policy (e.g., `RetentionPolicy_Advanced_Precision_Cron`) follow the convention of the `cron` npm package (outdated version), which uses **0-indexed months** (0 = January, 11 = December), not the standard 1-12 used in POSIX cron. The local `sendAt` function in `apps/meteor/client/hooks/usePruneWarningMessage.ts` intentionally replicates this 0-indexed behavior. Do NOT suggest adding `+1` to `date.getMonth()` in this file, as it would break tests and create inconsistency with the backend.
Applied to files:
apps/meteor/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.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/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.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/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.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/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-04-17T23:32:07.223Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/messages.ts:348-352
Timestamp: 2026-04-17T23:32:07.223Z
Learning: In `apps/meteor/app/apps/server/converters/messages.ts`, the `timestamp` handler inside `_convertAttachmentsToApp` uses a non-null assertion (`attachment.ts!`) intentionally. The `ts` property on `MessageAttachment` is optional only to accommodate MessageAttachment creation-time scenarios; by the time `_convertAttachmentsToApp` is called, the message has already been stored and the attachment is guaranteed to have a `ts` value. Do not flag this non-null assertion as unsafe during code review.
Applied to files:
apps/meteor/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.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/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.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 apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T18:10:55.887Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:55.887Z
Learning: In RocketChat/Rocket.Chat, the route handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892) contains a `catch` block that intercepts all route-handler errors and converts them into proper HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`). As a result, `next()` in Hono middleware (such as `metricsMiddleware` in `apps/meteor/app/api/server/middlewares/metrics.ts`) will never throw. Do not flag missing try/finally guards around `await next()` calls in this middleware for gauge/counter decrement safety, as the execution flow is never disrupted by route-level errors.
Applied to files:
apps/meteor/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.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/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-04-23T18:14:24.851Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/lib/server/lib/debug.js:87-91
Timestamp: 2026-04-23T18:14:24.851Z
Learning: In `apps/meteor/app/lib/server/lib/debug.js`, the `wrapMethods` function wraps Meteor DDP method handlers inside a `tracerActiveSpan` callback. The `end()` (summary timer) and `endHistogram()` (histogram timer) calls are placed after `await originalHandler.apply(this, originalArgs)` without a try/finally guard. This is intentional and safe: errors thrown by Meteor method handlers are caught by the route-handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892-893), which converts them into proper response objects, so execution flow is never disrupted and the timer calls are always reached. Do not flag the missing try/finally as a bug for these timer calls in `debug.js`.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.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/cron/scheduledMessages.tsapps/meteor/app/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/v1/chat.scheduleMessage.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/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/app/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-02-24T19:09:01.522Z
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:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-03-20T13:52:29.575Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:29.575Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/api/server/v1/chat.scheduleMessage.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/api/server/v1/chat.scheduleMessage.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/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-04-20T17:11:59.452Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40225
File: apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts:55-71
Timestamp: 2026-04-20T17:11:59.452Z
Learning: In `apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts`, the concern about an empty `?appId=` query param bypassing the truthy check and overriding the path `appId` in the `makeAppLogsQuery` spread is not relevant. The AJV query schema (`isAppLogsProps`) validates and rejects invalid/empty `appId` values before the action handler is reached, making the in-handler guard sufficient as-is. Do not flag this pattern as a vulnerability in future reviews of this file.
Applied to files:
apps/meteor/app/api/server/v1/chat.scheduleMessage.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/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/api/server/v1/chat.scheduleMessage.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/api/server/v1/chat.scheduleMessage.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/api/server/v1/chat.scheduleMessage.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/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/app/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/api/server/v1/chat.scheduleMessage.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/chat.scheduleMessage.ts
coderabbitai
Bot
added
the
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/i18n/src/locales/en.i18n.json (2)
4848-4856: Consolidate duplicated translation keys that differ only by case.
Schedule_Message/Schedule_messageandView_scheduled_messages/View_Scheduled_Messagescarry the same meaning and will drift over time. Prefer one canonical key per message and update call sites accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/src/locales/en.i18n.json` around lines 4848 - 4856, Consolidate duplicated translation keys that differ only by case by choosing a single canonical key for each message (e.g., pick either "Schedule_Message" or "Schedule_message" and either "View_scheduled_messages" or "View_Scheduled_Messages"), remove the duplicates from en.i18n.json, and update all call sites that reference the removed keys to use the canonical key (search for usages of "Schedule_Message", "Schedule_message", "View_scheduled_messages", and "View_Scheduled_Messages" and replace them accordingly); ensure you keep the existing string values unchanged and run tests or a translation usage grep to confirm no references remain to the removed keys.
4858-4860: Prefer scoped keys over global generic status keys.Keys like
sent,cancelled, andfailedare too broad in this global namespace and are easy to collide with unrelated features. Consider namespaced keys such asScheduled_message_status_sent, etc.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/i18n/src/locales/en.i18n.json` around lines 4858 - 4860, The global keys "sent", "cancelled", and "failed" in en.i18n.json are too generic—rename them to scoped keys (e.g., "Scheduled_message_status_sent", "Scheduled_message_status_cancelled", "Scheduled_message_status_failed") and update all callers to use the new keys; search for any i18n lookups or components referencing "sent", "cancelled", or "failed" and replace those references (or add fallbacks) to the new scoped keys so existing UI text stays correct.apps/meteor/server/cron/scheduledMessages.ts (1)
57-70: AddscheduledRetryCounttoIMessage; improvemessageobject typing.The
as anycast on line 70 masks a missing type field.scheduledRetryCountis persisted to the database (line 97:$inc: { scheduledRetryCount: 1 }) but is not declared onIMessage, even though related fields likescheduledandscheduledAtare. AddscheduledRetryCount?: numberto theIMessageinterface.Similarly, line 57's
message: anydefeats type checking on the dispatch payload. SincesendMessagealready acceptsany, this is lower priority, but importing the dispatch type or typing it asPartial<IMessage> & Pick<IMessage, 'msg' | 'rid'>would tighten coverage.Per coding guidelines: "Write concise, technical TypeScript/JavaScript with accurate typing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/cron/scheduledMessages.ts` around lines 57 - 70, The IMessage interface is missing the optional field scheduledRetryCount; add scheduledRetryCount?: number to IMessage so the DB $inc and reads are typed, and replace the ad-hoc cast ((scheduledMessage as any).scheduledRetryCount || 0) with a typed access (scheduledMessage.scheduledRetryCount ?? 0). Also tighten the local message variable typing: change message: any to a safer type such as Partial<IMessage> & Pick<IMessage, 'msg' | 'rid'> (or import the dispatch type used by sendMessage) so the payload construction (msg, rid, tmid, alias, avatar, emoji, attachments) is type-checked against IMessage while keeping sendMessage compatibility.
🤖 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/server/cron/scheduledMessages.ts`:
- Around line 105-109: The function scheduledMessagesCron currently calls await
processScheduledMessages() immediately at startup which causes an unprotected,
synchronous catch-up pass; remove the eager startup invocation so the function
only registers the cron job via cronJobs.add('Process Scheduled Messages', '* *
* * *', async () => processScheduledMessages()) and rely solely on the scheduled
tick (i.e., delete the await processScheduledMessages() line and return the
cronJobs.add result directly).
- Around line 93-99: The retry logic currently re-sets scheduled:true and
increments scheduledRetryCount in Messages.updateOne for scheduledMessage._id
but leaves scheduledAt in the past, causing immediate reprocessing and burning
through maxRetries; update the database update to also push scheduledAt forward
using an exponential backoff based on the current scheduledRetryCount (e.g., now
+ 2^scheduledRetryCount minutes) so each retry is delayed, and continue
incrementing scheduledRetryCount (refer to Messages.updateOne,
scheduledMessage._id, scheduled, scheduledRetryCount, scheduledAt, maxRetries).
- Around line 43-55: The recoverable-error check in the catch block is broken
because it reads error.error (only present on Meteor.Error) and looks for a
non-existent 'error-room-not-found' code; update the logic in
scheduledMessages.ts (inside the catch handling for scheduledMessage) to derive
the error code from either error.error or error.message (e.g., const code =
error?.error ?? error?.message) and then check code against the recoverable
list, and replace 'error-room-not-found' with the correct 'error-invalid-room'
string so that archived/blocked/muted/invalid-room errors trigger the
Messages.updateOne retry branch instead of deleting the message.
---
Nitpick comments:
In `@apps/meteor/server/cron/scheduledMessages.ts`:
- Around line 57-70: The IMessage interface is missing the optional field
scheduledRetryCount; add scheduledRetryCount?: number to IMessage so the DB $inc
and reads are typed, and replace the ad-hoc cast ((scheduledMessage as
any).scheduledRetryCount || 0) with a typed access
(scheduledMessage.scheduledRetryCount ?? 0). Also tighten the local message
variable typing: change message: any to a safer type such as Partial<IMessage> &
Pick<IMessage, 'msg' | 'rid'> (or import the dispatch type used by sendMessage)
so the payload construction (msg, rid, tmid, alias, avatar, emoji, attachments)
is type-checked against IMessage while keeping sendMessage compatibility.
In `@packages/i18n/src/locales/en.i18n.json`:
- Around line 4848-4856: Consolidate duplicated translation keys that differ
only by case by choosing a single canonical key for each message (e.g., pick
either "Schedule_Message" or "Schedule_message" and either
"View_scheduled_messages" or "View_Scheduled_Messages"), remove the duplicates
from en.i18n.json, and update all call sites that reference the removed keys to
use the canonical key (search for usages of "Schedule_Message",
"Schedule_message", "View_scheduled_messages", and "View_Scheduled_Messages" and
replace them accordingly); ensure you keep the existing string values unchanged
and run tests or a translation usage grep to confirm no references remain to the
removed keys.
- Around line 4858-4860: The global keys "sent", "cancelled", and "failed" in
en.i18n.json are too generic—rename them to scoped keys (e.g.,
"Scheduled_message_status_sent", "Scheduled_message_status_cancelled",
"Scheduled_message_status_failed") and update all callers to use the new keys;
search for any i18n lookups or components referencing "sent", "cancelled", or
"failed" and replace those references (or add fallbacks) to the new scoped keys
so existing UI text stays correct.
🪄 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: 58101305-4f02-40eb-aa27-77c65666534e
📒 Files selected for processing (4)
.changeset/tasty-hoops-try.mdapps/meteor/app/api/server/v1/chat.scheduleMessage.tsapps/meteor/server/cron/scheduledMessages.tspackages/i18n/src/locales/en.i18n.json
✅ Files skipped from review due to trivial changes (1)
- .changeset/tasty-hoops-try.md
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/api/server/v1/chat.scheduleMessage.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/server/cron/scheduledMessages.ts
🧠 Learnings (20)
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json
📚 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:
packages/i18n/src/locales/en.i18n.json
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-03-23T19:33:46.159Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 39818
File: apps/meteor/client/hooks/usePruneWarningMessage.ts:45-56
Timestamp: 2026-03-23T19:33:46.159Z
Learning: In RocketChat/Rocket.Chat, cron expressions used by the retention policy (e.g., `RetentionPolicy_Advanced_Precision_Cron`) follow the convention of the `cron` npm package (outdated version), which uses **0-indexed months** (0 = January, 11 = December), not the standard 1-12 used in POSIX cron. The local `sendAt` function in `apps/meteor/client/hooks/usePruneWarningMessage.ts` intentionally replicates this 0-indexed behavior. Do NOT suggest adding `+1` to `date.getMonth()` in this file, as it would break tests and create inconsistency with the backend.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-17T23:32:07.223Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/messages.ts:348-352
Timestamp: 2026-04-17T23:32:07.223Z
Learning: In `apps/meteor/app/apps/server/converters/messages.ts`, the `timestamp` handler inside `_convertAttachmentsToApp` uses a non-null assertion (`attachment.ts!`) intentionally. The `ts` property on `MessageAttachment` is optional only to accommodate MessageAttachment creation-time scenarios; by the time `_convertAttachmentsToApp` is called, the message has already been stored and the attachment is guaranteed to have a `ts` value. Do not flag this non-null assertion as unsafe during code review.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T18:10:55.887Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:55.887Z
Learning: In RocketChat/Rocket.Chat, the route handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892) contains a `catch` block that intercepts all route-handler errors and converts them into proper HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`). As a result, `next()` in Hono middleware (such as `metricsMiddleware` in `apps/meteor/app/api/server/middlewares/metrics.ts`) will never throw. Do not flag missing try/finally guards around `await next()` calls in this middleware for gauge/counter decrement safety, as the execution flow is never disrupted by route-level errors.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T18:14:24.851Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/lib/server/lib/debug.js:87-91
Timestamp: 2026-04-23T18:14:24.851Z
Learning: In `apps/meteor/app/lib/server/lib/debug.js`, the `wrapMethods` function wraps Meteor DDP method handlers inside a `tracerActiveSpan` callback. The `end()` (summary timer) and `endHistogram()` (histogram timer) calls are placed after `await originalHandler.apply(this, originalArgs)` without a try/finally guard. This is intentional and safe: errors thrown by Meteor method handlers are caught by the route-handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892-893), which converts them into proper response objects, so execution flow is never disrupted and the timer calls are always reached. Do not flag the missing try/finally as a bug for these timer calls in `debug.js`.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T13:34:53.287Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40269
File: packages/apps-engine/src/server/accessors/Notifier.ts:19-19
Timestamp: 2026-04-23T13:34:53.287Z
Learning: In `packages/apps-engine/src/server/accessors/Notifier.ts`, the `as IUser` cast on `await this.userBridge.doGetAppUser(this.appId)` in both `notifyUser` (line ~19) and `notifyRoom` (line ~29) is intentional — the app user is guaranteed to exist at that point in the execution flow. Do not flag this as an unsafe cast. Any improvement to add a defensive guard is deferred to a future PR.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/cron/scheduledMessages.ts
🔇 Additional comments (1)
apps/meteor/server/cron/scheduledMessages.ts (1)
67-68: Duplicate delivery ifMessages.deleteOnefails after a successfulsendMessage.If
sendMessagesucceeds and the subsequentdeleteOnethrows (transient Mongo failure, step-down, etc.), control falls into the outer catch at line 69 which re-setsscheduled: trueand incrementsscheduledRetryCount. SincescheduledAtis unchanged, the next tick re-claims the same document and dispatches the message a second time. This was raised on a previous commit and is still present — consider deleting (or marking dispatched) beforesendMessage, or guarding the outer catch from running after a successful send.
coderabbitai
Bot
removed
the
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/server/cron/scheduledMessages.ts`:
- Around line 102-104: The backoff uses the stale variable now (captured before
the per-message loop) when computing newScheduledAt, so delays can be too short
or in the past; update the calculation in the catch block to use the current
time at scheduling (e.g., Date.now() or new Date()) together with retryCount to
compute newScheduledAt, and remove the inline comment about exponential backoff;
locate and change the code that computes newScheduledAt (references: now,
retryCount, newScheduledAt in scheduledMessages.ts) to use the current timestamp
instead of now.
- Around line 75-77: Remove the pre-send deletion that calls
Messages.deleteOne({ _id: scheduledMessage._id }) before sendMessage; instead,
rely on the earlier atomic claim that set scheduled: false to prevent re-pick,
call sendMessage(user, message, room, {}), and only after a successful send
remove the scheduled document (i.e., move the deleteOne to after sendMessage
succeeds); also drop the inline comment about deleting before send. This ensures
transient sendMessage failures trigger the existing catch-path logic (which uses
Messages.updateOne / Messages.deleteOne) and prevents silent loss of the drafted
message.
- Around line 43-63: The recoverable branch in scheduledMessages.ts currently
only sets scheduled:true causing infinite immediate retries; update that branch
(where Messages.updateOne is called for recoverable errors) to mirror the outer
catch behavior: log a warning via SystemLogger.warn including err, messageId,
roomId, userId; compute and set a new scheduledAt using exponential backoff
based on scheduledRetryCount (increment scheduledRetryCount in the same update);
if the retry count exceeds the cap, dead-letter the message with
Messages.deleteOne (and log that action) instead of rescheduling. Ensure you
update the same document fields (scheduledAt and scheduledRetryCount) atomically
in the Messages.updateOne call and include contextual logging similar to the
non-recoverable path.
🪄 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: 3340a1e2-2a8d-4693-a3ef-14d010548c77
📒 Files selected for processing (1)
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
🧠 Learnings (20)
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-03-23T19:33:46.159Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 39818
File: apps/meteor/client/hooks/usePruneWarningMessage.ts:45-56
Timestamp: 2026-03-23T19:33:46.159Z
Learning: In RocketChat/Rocket.Chat, cron expressions used by the retention policy (e.g., `RetentionPolicy_Advanced_Precision_Cron`) follow the convention of the `cron` npm package (outdated version), which uses **0-indexed months** (0 = January, 11 = December), not the standard 1-12 used in POSIX cron. The local `sendAt` function in `apps/meteor/client/hooks/usePruneWarningMessage.ts` intentionally replicates this 0-indexed behavior. Do NOT suggest adding `+1` to `date.getMonth()` in this file, as it would break tests and create inconsistency with the backend.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-17T23:32:07.223Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/messages.ts:348-352
Timestamp: 2026-04-17T23:32:07.223Z
Learning: In `apps/meteor/app/apps/server/converters/messages.ts`, the `timestamp` handler inside `_convertAttachmentsToApp` uses a non-null assertion (`attachment.ts!`) intentionally. The `ts` property on `MessageAttachment` is optional only to accommodate MessageAttachment creation-time scenarios; by the time `_convertAttachmentsToApp` is called, the message has already been stored and the attachment is guaranteed to have a `ts` value. Do not flag this non-null assertion as unsafe during code review.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T18:10:55.887Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:55.887Z
Learning: In RocketChat/Rocket.Chat, the route handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892) contains a `catch` block that intercepts all route-handler errors and converts them into proper HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`). As a result, `next()` in Hono middleware (such as `metricsMiddleware` in `apps/meteor/app/api/server/middlewares/metrics.ts`) will never throw. Do not flag missing try/finally guards around `await next()` calls in this middleware for gauge/counter decrement safety, as the execution flow is never disrupted by route-level errors.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T18:14:24.851Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/lib/server/lib/debug.js:87-91
Timestamp: 2026-04-23T18:14:24.851Z
Learning: In `apps/meteor/app/lib/server/lib/debug.js`, the `wrapMethods` function wraps Meteor DDP method handlers inside a `tracerActiveSpan` callback. The `end()` (summary timer) and `endHistogram()` (histogram timer) calls are placed after `await originalHandler.apply(this, originalArgs)` without a try/finally guard. This is intentional and safe: errors thrown by Meteor method handlers are caught by the route-handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892-893), which converts them into proper response objects, so execution flow is never disrupted and the timer calls are always reached. Do not flag the missing try/finally as a bug for these timer calls in `debug.js`.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T13:34:53.287Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40269
File: packages/apps-engine/src/server/accessors/Notifier.ts:19-19
Timestamp: 2026-04-23T13:34:53.287Z
Learning: In `packages/apps-engine/src/server/accessors/Notifier.ts`, the `as IUser` cast on `await this.userBridge.doGetAppUser(this.appId)` in both `notifyUser` (line ~19) and `notifyRoom` (line ~29) is intentional — the app user is guaranteed to exist at that point in the execution flow. Do not flag this as an unsafe cast. Any improvement to add a defensive guard is deferred to a future PR.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/cron/scheduledMessages.ts
coderabbitai
Bot
added
the
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/server/cron/scheduledMessages.ts (2)
51-51: Drop theas anycasts andmessage: any— extend the model type instead.
(scheduledMessage as any).scheduledRetryCount(lines 51, 106) andconst message: any(line 92) defeat TypeScript on the very fields this PR introduces. Since the PR is addingscheduled,scheduledAt, andscheduledRetryCountto the message model already, declare them as optional onIMessage(or aScheduledMessageextension) so the cron reads them with proper typing, and type the outbound payload asPartial<IMessage>(or a dedicatedSendMessagePayload).♻️ Sketch
- const retryCount = ((scheduledMessage as any).scheduledRetryCount || 0) + 1; + const retryCount = (scheduledMessage.scheduledRetryCount ?? 0) + 1;- const message: any = { + const message: Partial<IMessage> = { msg: scheduledMessage.msg, rid: scheduledMessage.rid, ... };As per coding guidelines (
**/*.{ts,tsx,js}: "Write concise, technical TypeScript/JavaScript with accurate typing").Also applies to: 92-92, 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/cron/scheduledMessages.ts` at line 51, The cron file is using unsafe any casts for scheduled fields—replace them by extending the message model: add optional properties scheduled?: boolean, scheduledAt?: number|string, and scheduledRetryCount?: number to IMessage (or create a ScheduledMessage interface extending IMessage) so scheduledMessage can be properly typed and you can remove (scheduledMessage as any). Also change the outbound payload variable currently declared as message: any to a typed payload such as Partial<IMessage> or a dedicated SendMessagePayload type, and update usages in functions like the retry logic and send path (references: scheduledMessage, scheduledRetryCount, scheduledAt, scheduled) to use the new typed interface instead of any.
50-78: Duplicated retry/backoff/dead-letter logic — extract a helper.The recoverable-auth branch (lines 50–78) and the
sendMessagefailure branch (lines 105–143) implement essentially identical logic: bumpscheduledRetryCount, dead-letter once>= maxRetries(with the sameSystemLogger.errorshape), otherwise compute2^retryCountminutes of backoff andupdateOnethe same three fields. Extracting a smallrescheduleOrDropFailedMessage(scheduledMessage, error, logMsg)(or two thin wrappers) would remove ~40 lines of near-identical code, ensure both paths stay in sync (e.g., next time you tweakmaxRetriesor backoff cap), and make the per-message control flow easier to follow.Also worth considering: cap the backoff (e.g.,
Math.min(2 ** retryCount, 30)minutes) soretryCount=4doesn't push the next attempt 16 minutes out only to be dead-lettered on the 5th anyway.Also applies to: 105-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/cron/scheduledMessages.ts` around lines 50 - 78, Extract the duplicated retry/dead-letter/backoff logic into a helper (e.g., rescheduleOrDropFailedMessage(scheduledMessage, error, logMsg)) and call it from both the recoverable-auth branch and the sendMessage failure branch; the helper should read and increment scheduledRetryCount on the passed scheduledMessage, compare it to a single maxRetries constant, call SystemLogger.error with the same shape (msg, err, messageId, roomId, userId, retryCount) and delete the message via Messages.deleteOne when max reached, otherwise compute a capped backoff (e.g., backoffMins = Math.min(2 ** retryCount, 30)), set scheduledAt, scheduledRetryCount and scheduled via Messages.updateOne, and return or throw as appropriate so the callers remain simple and consistent.
🤖 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/server/cron/scheduledMessages.ts`:
- Around line 34-41: The user-not-found branch after Users.findOneById currently
flips the message back to scheduled: true and leaves scheduledAt in the past,
causing perpetual retries; update this branch in scheduledMessages.ts to treat a
missing user as non-recoverable by applying the same dead-letter or
retry/backoff logic used in canSendMessageAsync/sendMessage: either mark the
message as dead-lettered when retries >= maxRetries (set appropriate
status/fields and log via SystemLogger.error including context), or increment
the retry counter and set scheduledAt with exponential backoff (same
algorithm/fields used elsewhere) and persist via Messages.updateOne for
consistency; reference scheduledMessage._id,
scheduledMessage.scheduledAt/retries, Messages.updateOne, Users.findOneById,
maxRetries, canSendMessageAsync, sendMessage and SystemLogger.error when
locating and implementing the fix.
---
Nitpick comments:
In `@apps/meteor/server/cron/scheduledMessages.ts`:
- Line 51: The cron file is using unsafe any casts for scheduled fields—replace
them by extending the message model: add optional properties scheduled?:
boolean, scheduledAt?: number|string, and scheduledRetryCount?: number to
IMessage (or create a ScheduledMessage interface extending IMessage) so
scheduledMessage can be properly typed and you can remove (scheduledMessage as
any). Also change the outbound payload variable currently declared as message:
any to a typed payload such as Partial<IMessage> or a dedicated
SendMessagePayload type, and update usages in functions like the retry logic and
send path (references: scheduledMessage, scheduledRetryCount, scheduledAt,
scheduled) to use the new typed interface instead of any.
- Around line 50-78: Extract the duplicated retry/dead-letter/backoff logic into
a helper (e.g., rescheduleOrDropFailedMessage(scheduledMessage, error, logMsg))
and call it from both the recoverable-auth branch and the sendMessage failure
branch; the helper should read and increment scheduledRetryCount on the passed
scheduledMessage, compare it to a single maxRetries constant, call
SystemLogger.error with the same shape (msg, err, messageId, roomId, userId,
retryCount) and delete the message via Messages.deleteOne when max reached,
otherwise compute a capped backoff (e.g., backoffMins = Math.min(2 **
retryCount, 30)), set scheduledAt, scheduledRetryCount and scheduled via
Messages.updateOne, and return or throw as appropriate so the callers remain
simple and consistent.
🪄 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: 20c6e844-ca43-419c-aaf7-850d19bf2c8c
📒 Files selected for processing (1)
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
🧠 Learnings (20)
📚 Learning: 2026-03-23T19:33:46.159Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 39818
File: apps/meteor/client/hooks/usePruneWarningMessage.ts:45-56
Timestamp: 2026-03-23T19:33:46.159Z
Learning: In RocketChat/Rocket.Chat, cron expressions used by the retention policy (e.g., `RetentionPolicy_Advanced_Precision_Cron`) follow the convention of the `cron` npm package (outdated version), which uses **0-indexed months** (0 = January, 11 = December), not the standard 1-12 used in POSIX cron. The local `sendAt` function in `apps/meteor/client/hooks/usePruneWarningMessage.ts` intentionally replicates this 0-indexed behavior. Do NOT suggest adding `+1` to `date.getMonth()` in this file, as it would break tests and create inconsistency with the backend.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-17T23:32:07.223Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/messages.ts:348-352
Timestamp: 2026-04-17T23:32:07.223Z
Learning: In `apps/meteor/app/apps/server/converters/messages.ts`, the `timestamp` handler inside `_convertAttachmentsToApp` uses a non-null assertion (`attachment.ts!`) intentionally. The `ts` property on `MessageAttachment` is optional only to accommodate MessageAttachment creation-time scenarios; by the time `_convertAttachmentsToApp` is called, the message has already been stored and the attachment is guaranteed to have a `ts` value. Do not flag this non-null assertion as unsafe during code review.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T18:10:55.887Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:55.887Z
Learning: In RocketChat/Rocket.Chat, the route handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892) contains a `catch` block that intercepts all route-handler errors and converts them into proper HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`). As a result, `next()` in Hono middleware (such as `metricsMiddleware` in `apps/meteor/app/api/server/middlewares/metrics.ts`) will never throw. Do not flag missing try/finally guards around `await next()` calls in this middleware for gauge/counter decrement safety, as the execution flow is never disrupted by route-level errors.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T18:14:24.851Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/lib/server/lib/debug.js:87-91
Timestamp: 2026-04-23T18:14:24.851Z
Learning: In `apps/meteor/app/lib/server/lib/debug.js`, the `wrapMethods` function wraps Meteor DDP method handlers inside a `tracerActiveSpan` callback. The `end()` (summary timer) and `endHistogram()` (histogram timer) calls are placed after `await originalHandler.apply(this, originalArgs)` without a try/finally guard. This is intentional and safe: errors thrown by Meteor method handlers are caught by the route-handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892-893), which converts them into proper response objects, so execution flow is never disrupted and the timer calls are always reached. Do not flag the missing try/finally as a bug for these timer calls in `debug.js`.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-23T13:34:53.287Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40269
File: packages/apps-engine/src/server/accessors/Notifier.ts:19-19
Timestamp: 2026-04-23T13:34:53.287Z
Learning: In `packages/apps-engine/src/server/accessors/Notifier.ts`, the `as IUser` cast on `await this.userBridge.doGetAppUser(this.appId)` in both `notifyUser` (line ~19) and `notifyRoom` (line ~29) is intentional — the app user is guaranteed to exist at that point in the execution flow. Do not flag this as an unsafe cast. Any improvement to add a defensive guard is deferred to a future PR.
Applied to files:
apps/meteor/server/cron/scheduledMessages.ts
📚 Learning: 2026-04-20T17:11:59.452Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40225
File: apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts:55-71
Timestamp: 2026-04-20T17:11:59.452Z
Learning: In `apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts`, the concern about an empty `?appId=` query param bypassing the truthy check and overriding the path `appId` in the `makeAppLogsQuery` spread is not relevant. The AJV query schema (`isAppLogsProps`) validates and rejects invalid/empty `appId` values before the action handler is reached, making the in-handler guard sufficient as-is. Do not flag this pattern as a vulnerability in future reviews of this file.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.ts
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
apps/meteor/server/cron/scheduledMessages.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/server/cron/scheduledMessages.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/cron/scheduledMessages.ts
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
1 participant
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.
Proposed changes (including videos or screenshots)
Added a complete message scheduling feature, enabling users to schedule messages to be sent at a specific date and time.
VIDEO: https://drive.google.com/file/d/1VPe8fiTi2k1KOQu4tJi4f7pGP1mW3C5x/view?usp=sharing
Issue(s)
Cancel #40309
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Documentation