chore(message-parser): remove orphaned i18next pluralization TODO from Reaction component#39944
Open
sharanyamahajan wants to merge 8 commits intoRocketChat:developfrom
Open
chore(message-parser): remove orphaned i18next pluralization TODO from Reaction component#39944sharanyamahajan wants to merge 8 commits intoRocketChat:developfrom
sharanyamahajan wants to merge 8 commits intoRocketChat:developfrom
Conversation
Added a new line to app.json to improve configuration consistency. Changes- 1. Updated app.json 2. Added the required configuration line Notes-This is a minor configuration update and does not affect application functionality.
chore: update app.json configuration
Removed unnecessary line breaks in the UserCard component to improve code readability and maintain formatting consistency. No functional changes were introduced.
…m Reaction component
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
WalkthroughAdds Docker/docker-compose and VS Code container debug support; introduces runtime message moderation reasoning and an escapeRegExp utility; refactors server-side message search parsing; adjusts federation member event handling; and removes minor client-side noise (blank lines and a stale TODO). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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: 10
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/events/member.ts (1)
316-316: KeeproomIdin the leave-path error for diagnostics.Line 316 now omits
roomId, which makes federation incident triage harder in logs.🔍 Small observability tweak
- throw new Error(`Room not found while leaving user ${userId}`); + throw new Error(`Room not found while leaving user ${userId} from room ${roomId}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/packages/federation-matrix/src/events/member.ts` at line 316, The thrown error currently omits roomId which hurts diagnostics; update the Error thrown at the leave path so it includes both userId and roomId (e.g., change the message created by the throw new Error(...) in the member leave logic to include roomId alongside userId) so logs contain both identifiers for easier federation incident triage.apps/meteor/server/lib/parseMessageSearchQuery.ts (1)
243-244: Keep the export typed.Widening
optionstoanydrops the safety the constructor already has and makes this API easier to misuse. I'd reuse a shared options type here unless this export intentionally crosses an untyped boundary.Suggested refactor
+type ParseMessageSearchQueryOptions = { + user?: IUser; + offset?: number; + limit?: number; + forceRegex?: boolean; +}; + class MessageSearchQueryParser { ... - constructor({ + constructor({ user, offset = 0, limit = 20, forceRegex = false, - }: { - user?: IUser; - offset?: number; - limit?: number; - forceRegex?: boolean; - }) { + }: ParseMessageSearchQueryOptions) { ... } } -export function parseMessageSearchQuery(text: string, options: any) { +export function parseMessageSearchQuery(text: string, options: ParseMessageSearchQueryOptions) { const parser = new MessageSearchQueryParser(options);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/parseMessageSearchQuery.ts` around lines 243 - 244, parseMessageSearchQuery currently types its options parameter as any, which weakens type safety; change the signature of parseMessageSearchQuery to accept the proper shared options type used by MessageSearchQueryParser (reuse the existing Options/MessageSearchOptions interface or the exact type used in MessageSearchQueryParser's constructor) and update any callers if necessary so the exported function preserves the parser's strong typing instead of widening to any.
🤖 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/lib/server/lib/sendNotificationsOnMessage.ts`:
- Around line 301-304: Remove the raw console.log in
sendNotificationsOnMessage.ts that prints "Moderation reasoning" (including
userId and reasons) and replace it with a structured server logger call (e.g.,
processLogger.debug or the app's central logger) inside the same function so
logs are consistent and not noisy; ensure you log only non-sensitive identifiers
(use message.u?._id ?? 'unknown') and include reasons as a structured field
rather than a freeform console output.
- Around line 282-300: The code is mutating message via (message as
any).moderationReasons and using console.log; remove the any-cast and the
mutation and instead either (A) add a properly typed moderationReasons?:
ModerationReason[] to the IMessage interface and assign to
message.moderationReasons (no any cast) wherever IMessage is
constructed/validated, or (B) keep IMessage unchanged and create a separate
Map<string, ModerationReason[]> (e.g., moderationReasonsMap) keyed by
message._id and store reasons there; update any consumers to read from the new
typed field or map; also remove the console.log and replace with proper logging
if needed (use existing logger), and ensure the local const reasons:
ModerationReason[] is strongly typed.
In `@apps/meteor/server/lib/parseMessageSearchQuery.ts`:
- Around line 208-214: The parser now always sets this.query.$text in
consumeMessageText which makes the existing forceRegex flag a no-op; modify
consumeMessageText to first check this.forceRegex and, if true, skip setting
this.query.$text and this.options.projection (so regex-mode callers retain their
original behavior), otherwise proceed to trim and set $text and projection as
existing; reference consumeMessageText, this.forceRegex, this.query.$text, and
this.options.projection when making the change.
- Line 58: Remove inline status comments like "✅ FIXED" from the implementation
in parseMessageSearchQuery (and any similar comments in this file such as the
one near the code around the parseMessageSearchQuery function or other helper
functions), leaving no TODO/status annotations in production code; simply delete
those comment lines so the implementation contains only meaningful comments or
none at all per guidelines.
- Around line 202-203: The regex used to strip sort tokens
(/(?:order|sort):(asc|desc)/g) can match prefixes like "sort:ascending" and
leave "ending" behind; update the regex in parseMessageSearchQuery (the
replacement that sets this.options.sort) to require a token boundary after the
direction—e.g., use a trailing word-boundary or lookahead so only complete "asc"
or "desc" are matched (for example change to require (asc|desc)\b or
(asc|desc)(?=\s|$)), keeping the existing replacement that sets
this.options.sort.
In `@compose.yaml`:
- Around line 1-10: The compose fragment for service "rocketchat" is incomplete
and will fail because it lacks required backing services and env vars; update
the compose to add a MongoDB service (e.g., "mongo") and link it via environment
variables on the "rocketchat" service: set MONGO_URL (and MONGO_OPLOG_URL if
needed), ROOT_URL, and any necessary PORT/NODE_ENV values, add depends_on: -
mongo, and mount persistent volumes for both MongoDB and Rocket.Chat data;
ensure the "rocketchat" service keeps its existing build/context/dockerfile and
ports mapping while adding volumes and the database wiring so the stack is
runnable.
In `@Dockerfile`:
- Line 10: The Dockerfile's CMD currently uses "node index.js" which will fail
because there is no root index.js; update the Dockerfile (and
compose.debug.yaml) to either add a build stage that builds the Meteor app from
apps/meteor/ (e.g., run meteor build or npm install/build in apps/meteor and
produce a runnable bundle) and set CMD to start the built Meteor bundle, or
change CMD to run the Meteor app directly from apps/meteor/ (e.g., start script
in package.json in apps/meteor/) — locate the CMD instruction and any references
in compose.debug.yaml and replace the hardcoded node index.js with the correct
build+start flow or the meteor app start command so the container starts the
actual Meteor application.
- Around line 4-5: The Dockerfile currently runs RUN npm install --production
--silent (and COPY package.json/package-lock.json) which conflicts with the
repo's declared packageManager (yarn@4) and will break workspace resolution;
replace the npm-based steps by copying .yarnrc.yml and Yarn files (and
package.json/package.json.lock if used), then run yarn install --immutable
--production --silent (or yarn workspaces focus --all --production for
workspaces) to produce a reproducible install; also the container CMD/ENTRYPOINT
currently runs node index.js which does not exist — change it to invoke the
package start script via yarn (e.g., CMD ["yarn","start"]) or point to the
correct build entry (for example the real built file under dist or the script
name in package.json) so the container starts the actual app.
In `@ee/packages/federation-matrix/src/events/member.ts`:
- Line 246: Remove the inline implementation comment "// ✅ FIX: Safe avatar
handling" from ee/packages/federation-matrix/src/events/member.ts so runtime
TypeScript/JavaScript code contains no implementation comments; locate the
comment near the member event handling code (e.g., within the member event
handler or related function in this file) and delete that single comment line to
comply with the repository's TS/JS comment guidelines.
- Around line 247-253: Move the avatar reset logic so it only runs for
remote-origin joins and uses the Upload lifecycle methods instead of
Users.resetAvatar: inside the block that checks 'avatar_url' in content, guard
the reset with a check that joiningUser.avatarOrigin === 'remote' (or the same
remote-origin guard used elsewhere) and replace the
Users.resetAvatar(joiningUser._id) call with the same Upload-based avatar
removal used by the existing avatar flow (invoke the Upload method/class used
elsewhere in this file to remove/clear the user's avatar). Ensure this code runs
after the remote-origin guard so local-origin joins do not trigger the reset.
---
Nitpick comments:
In `@apps/meteor/server/lib/parseMessageSearchQuery.ts`:
- Around line 243-244: parseMessageSearchQuery currently types its options
parameter as any, which weakens type safety; change the signature of
parseMessageSearchQuery to accept the proper shared options type used by
MessageSearchQueryParser (reuse the existing Options/MessageSearchOptions
interface or the exact type used in MessageSearchQueryParser's constructor) and
update any callers if necessary so the exported function preserves the parser's
strong typing instead of widening to any.
In `@ee/packages/federation-matrix/src/events/member.ts`:
- Line 316: The thrown error currently omits roomId which hurts diagnostics;
update the Error thrown at the leave path so it includes both userId and roomId
(e.g., change the message created by the throw new Error(...) in the member
leave logic to include roomId alongside userId) so logs contain both identifiers
for easier federation incident triage.
🪄 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: b421c739-5fee-48b9-bd4f-dc9cd1ac010c
📒 Files selected for processing (12)
.dockerignore.vscode/launch.json.vscode/tasks.jsonDockerfileapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/app/utils/server/escapeRegExp.tsapps/meteor/client/components/UserCard/UserCard.tsxapps/meteor/client/components/message/content/reactions/Reaction.tsxapps/meteor/server/lib/parseMessageSearchQuery.tscompose.debug.yamlcompose.yamlee/packages/federation-matrix/src/events/member.ts
💤 Files with no reviewable changes (2)
- apps/meteor/client/components/UserCard/UserCard.tsx
- apps/meteor/client/components/message/content/reactions/Reaction.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/utils/server/escapeRegExp.tsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.tsee/packages/federation-matrix/src/events/member.ts
🧠 Learnings (23)
📓 Common learnings
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.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
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.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
📚 Learning: 2026-03-23T19:33:43.067Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 39818
File: apps/meteor/client/hooks/usePruneWarningMessage.ts:45-56
Timestamp: 2026-03-23T19:33:43.067Z
Learning: In Rocket.Chat, cron expressions used for the retention policy (e.g., `RetentionPolicy_Advanced_Precision_Cron`) follow the `cron` npm package convention, which uses **0-indexed months** (0 = January, 11 = December) rather than POSIX-style 1–12. In `apps/meteor/client/hooks/usePruneWarningMessage.ts`, the local `sendAt` logic intentionally mirrors this behavior—do **not** adjust `date.getMonth()` with `+1` or similar changes, because it will break tests and introduce inconsistency with the backend.
Applied to files:
.dockerignore
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/utils/server/escapeRegExp.tsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.tsee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/utils/server/escapeRegExp.tsapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.tsee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.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/app/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.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/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/server/lib/parseMessageSearchQuery.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/lib/parseMessageSearchQuery.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/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
ee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
ee/packages/federation-matrix/src/events/member.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.
Applied to files:
ee/packages/federation-matrix/src/events/member.ts
🪛 Trivy (0.69.3)
Dockerfile
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 5-6: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y build-essential git'
Rule: DS-0029
(IaC/Dockerfile)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🔇 Additional comments (6)
apps/meteor/app/utils/server/escapeRegExp.ts (1)
1-3: Looks correct and production-ready.This uses the standard, correct metacharacter escaping pattern and the function signature is appropriately typed.
ee/packages/federation-matrix/src/events/member.ts (2)
241-241: No actionable change (whitespace-only).
134-144: Good cleanup with behavior-preserving simplifications.The refactors in room creation, join-rule lookup, invite room naming, and subscription short-circuiting are cleaner and keep intent clear.
Also applies to: 154-167, 184-206, 305-305
compose.debug.yaml (1)
1-12: Same runtime completeness concern ascompose.yaml.No additional issue beyond the already-raised startup/dependency gap.
.vscode/launch.json (1)
4-8: Debug configuration wiring looks consistent.
preLaunchTaskand platform settings align with the added Docker debug task.apps/meteor/server/lib/parseMessageSearchQuery.ts (1)
47-50: Nice hardening on user-supplied regex terms.Escaping each token and anchoring the alternation avoids regex metacharacter injection and unintended partial matches across the username and attachment filters.
Also applies to: 65-68, 112-113, 126-127, 140-140
Comment on lines
+282
to
+300
| const reasons: any[] = []; | ||
|
|
||
| if (message.msg && message.msg.length > 200) { | ||
| reasons.push({ | ||
| type: 'long_message', | ||
| message: 'Message unusually long (possible spam)', | ||
| }); | ||
| } | ||
|
|
||
| if (/(.)\1{5,}/.test(message.msg || '')) { | ||
| reasons.push({ | ||
| type: 'repetition', | ||
| message: 'Repeated characters detected', | ||
| }); | ||
| } | ||
|
|
||
| if (reasons.length > 0) { | ||
| (message as any).moderationReasons = reasons; | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether IMessage officially includes moderationReasons
ast-grep --pattern $'interface IMessage {\n $$$\n moderationReasons: $_\n $$$\n}'
# Locate all usages/assumptions of moderationReasons in TS sources
rg -n --type=ts -C3 '\bmoderationReasons\b'Repository: RocketChat/Rocket.Chat
Length of output: 681
Remove the any type bypass and undeclared moderationReasons mutation; add proper typing or use a separate tracking object.
Lines 299 and 282: The code mutates message via (message as any).moderationReasons to inject a field not declared in the IMessage interface. This bypasses type checking and creates a hidden contract for shared notification objects. Additionally, the console.log at line 301 should not be in production code—remove or replace with proper debug logging.
Define moderationReasons in the IMessage type contract, or use a separate Map<string, any[]> to track moderation metadata outside the message object.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts` around lines
282 - 300, The code is mutating message via (message as any).moderationReasons
and using console.log; remove the any-cast and the mutation and instead either
(A) add a properly typed moderationReasons?: ModerationReason[] to the IMessage
interface and assign to message.moderationReasons (no any cast) wherever
IMessage is constructed/validated, or (B) keep IMessage unchanged and create a
separate Map<string, ModerationReason[]> (e.g., moderationReasonsMap) keyed by
message._id and store reasons there; update any consumers to read from the new
typed field or map; also remove the console.log and replace with proper logging
if needed (use existing logger), and ensure the local const reasons:
ModerationReason[] is strongly typed.
Comment on lines
+301
to
+304
| console.log('Moderation reasoning:', { | ||
| userId: message.u?._id ?? 'unknown', | ||
| reasons, | ||
| }); |
Contributor
There was a problem hiding this comment.
Remove raw console.log from the server notification path.
Line 301 logs user-related data in a hot path and can create noisy, unstructured production logs.
Suggested minimal change
- console.log('Moderation reasoning:', {
- userId: message.u?._id ?? 'unknown',
- reasons,
- });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log('Moderation reasoning:', { | |
| userId: message.u?._id ?? 'unknown', | |
| reasons, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts` around lines
301 - 304, Remove the raw console.log in sendNotificationsOnMessage.ts that
prints "Moderation reasoning" (including userId and reasons) and replace it with
a structured server logger call (e.g., processLogger.debug or the app's central
logger) inside the same function so logs are consistent and not noisy; ensure
you log only non-sensitive identifiers (use message.u?._id ?? 'unknown') and
include reasons as a structured field rather than a freeform console output.
| }); | ||
| } | ||
|
|
||
| // ✅ FIXED |
Contributor
There was a problem hiding this comment.
Please drop the inline “fixed” status comments.
These don't add durable context and will go stale quickly. As per coding guidelines, "Avoid code comments in the implementation."
Also applies to: 165-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/lib/parseMessageSearchQuery.ts` at line 58, Remove inline
status comments like "✅ FIXED" from the implementation in
parseMessageSearchQuery (and any similar comments in this file such as the one
near the code around the parseMessageSearchQuery function or other helper
functions), leaving no TODO/status annotations in production code; simply delete
those comment lines so the implementation contains only meaningful comments or
none at all per guidelines.
Comment on lines
+202
to
+203
| return text.replace(/(?:order|sort):(asc|desc)/g, (_: string, direction: string) => { | ||
| this.options.sort = { ts: direction === 'asc' ? 1 : -1 }; |
Contributor
There was a problem hiding this comment.
Anchor the sort token.
/(?:order|sort):(asc|desc)/g still matches the sort:asc prefix in sort:ascending, so the parser strips only part of the token and leaves ending behind for the text search. Add a trailing boundary/end check.
Suggested fix
- return text.replace(/(?:order|sort):(asc|desc)/g, (_: string, direction: string) => {
+ return text.replace(/(?:order|sort):(asc|desc)\b/g, (_: string, direction: string) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/lib/parseMessageSearchQuery.ts` around lines 202 - 203,
The regex used to strip sort tokens (/(?:order|sort):(asc|desc)/g) can match
prefixes like "sort:ascending" and leave "ending" behind; update the regex in
parseMessageSearchQuery (the replacement that sets this.options.sort) to require
a token boundary after the direction—e.g., use a trailing word-boundary or
lookahead so only complete "asc" or "desc" are matched (for example change to
require (asc|desc)\b or (asc|desc)(?=\s|$)), keeping the existing replacement
that sets this.options.sort.
Comment on lines
208
to
214
| private consumeMessageText(text: string) { | ||
| text = text.trim().replace(/\s\s/g, ' '); | ||
|
|
||
| if (text === '') { | ||
| return text; | ||
| } | ||
| text = text.trim(); | ||
| if (!text) return text; | ||
|
|
||
| if (/^\/.+\/[imxs]*$/.test(text)) { | ||
| const r = text.split('/'); | ||
| this.query.msg = { | ||
| $regex: r[1], | ||
| $options: r[2], | ||
| }; | ||
| } else if (this.forceRegex) { | ||
| this.query.msg = { | ||
| $regex: text, | ||
| $options: 'i', | ||
| }; | ||
| } else { | ||
| this.query.$text = { | ||
| $search: text, | ||
| }; | ||
| this.options.projection = { | ||
| score: { | ||
| $meta: 'textScore', | ||
| }, | ||
| }; | ||
| } | ||
| this.query.$text = { $search: text }; | ||
| this.options.projection = { score: { $meta: 'textScore' } }; | ||
|
|
Contributor
There was a problem hiding this comment.
forceRegex is a no-op after this change.
This branch always emits $text, but the parser still accepts and stores forceRegex. That silently changes the contract for callers that request regex-mode matching. Please either preserve the flag here or remove it in a separate breaking change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/server/lib/parseMessageSearchQuery.ts` around lines 208 - 214,
The parser now always sets this.query.$text in consumeMessageText which makes
the existing forceRegex flag a no-op; modify consumeMessageText to first check
this.forceRegex and, if true, skip setting this.query.$text and
this.options.projection (so regex-mode callers retain their original behavior),
otherwise proceed to trim and set $text and projection as existing; reference
consumeMessageText, this.forceRegex, this.query.$text, and
this.options.projection when making the change.
Comment on lines
+1
to
+10
| services: | ||
| rocketchat: | ||
| image: rocketchat | ||
| build: | ||
| context: . | ||
| dockerfile: ./Dockerfile | ||
| environment: | ||
| NODE_ENV: production | ||
| ports: | ||
| - 3000:3000 |
Contributor
There was a problem hiding this comment.
Compose service is not runnable as a standalone Rocket.Chat environment.
This definition only builds and maps port 3000; it omits required backing services/configuration (e.g., database wiring), so startup is likely to fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compose.yaml` around lines 1 - 10, The compose fragment for service
"rocketchat" is incomplete and will fail because it lacks required backing
services and env vars; update the compose to add a MongoDB service (e.g.,
"mongo") and link it via environment variables on the "rocketchat" service: set
MONGO_URL (and MONGO_OPLOG_URL if needed), ROOT_URL, and any necessary
PORT/NODE_ENV values, add depends_on: - mongo, and mount persistent volumes for
both MongoDB and Rocket.Chat data; ensure the "rocketchat" service keeps its
existing build/context/dockerfile and ports mapping while adding volumes and the
database wiring so the stack is runnable.
Comment on lines
+4
to
+5
| COPY ["package.json", "package-lock.json*", "npm-shrinkwrap.json*", "./"] | ||
| RUN npm install --production --silent && mv node_modules ../ |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== packageManager declaration =="
rg -n '"packageManager"\s*:' package.json
echo "== yarn config present =="
fd -a '^\.yarnrc\.yml$'
echo "== Dockerfile install command =="
rg -n 'npm install|yarn install|corepack' DockerfileRepository: RocketChat/Rocket.Chat
Length of output: 386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .yarnrc.yml existence (with hidden) =="
fd -H '^\.yarnrc\.yml$' || echo "Not found at repo root"
echo "== Full Dockerfile content =="
cat -n Dockerfile
echo "== index.js at root =="
ls -la index.js 2>/dev/null || echo "Not found"
echo "== Workspace packages (top-level) =="
ls -la packages/ 2>/dev/null | head -20 || echo "No packages dir"
echo "== Monorepo root structure =="
ls -la | grep -E '^d|\.json$|\.yml$' | head -20Repository: RocketChat/Rocket.Chat
Length of output: 3009
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json main field =="
rg -A2 '"main"' package.json | head -5
echo "== Search for server/app entry points in packages =="
find packages -name "main.ts" -o -name "main.js" -o -name "index.ts" -o -name "server.ts" 2>/dev/null | head -10
echo "== Other Dockerfiles in repo =="
fd -e Dockerfile
echo "== docker-compose references =="
rg -n 'build:|image:' docker-compose*.yml | head -10
echo "== .yarnrc.yml content (to confirm yarn v4 setup) =="
cat -n .yarnrc.ymlRepository: RocketChat/Rocket.Chat
Length of output: 635
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Build scripts in root package.json =="
rg -A5 '"scripts"' package.json | head -20
echo "== Check if index.js is built artifact (dist/build folders) =="
fd -t f 'index.js' --max-depth 2 2>/dev/null | head -10
echo "== apps/* structure - any server/api apps? =="
ls -1 apps/ 2>/dev/null | head -15
echo "== Check docker-compose.yml for build context =="
grep -A10 'build:' docker-compose*.yml | head -30
echo "== Git history - when was this Dockerfile last changed? =="
git log --oneline -5 -- Dockerfile 2>/dev/null || echo "Not in git"Repository: RocketChat/Rocket.Chat
Length of output: 2171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== References to root Dockerfile =="
rg -i 'dockerfile|docker build' --type md --type yml --type json . | grep -v 'apps/\|ee/' | head -20
echo "== Is root Dockerfile mentioned in docs? =="
find docs -name '*.md' 2>/dev/null -exec grep -l -i 'dockerfile\|docker build' {} \;
echo "== CI workflow files - do they use root Dockerfile? =="
rg 'Dockerfile' .github/workflows/ | grep -v 'apps/\|ee/'
echo "== Check README for docker instructions =="
head -100 README.md | grep -A5 -i docker || echo "Not in first 100 lines"Repository: RocketChat/Rocket.Chat
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Simple grep for Dockerfile references =="
rg 'Dockerfile' --no-heading | grep -v '^apps/\|^ee/' | head -20
echo "== Check if Dockerfile is in .gitignore =="
[ -f .gitignore ] && grep -i dockerfile .gitignore || echo "Not in .gitignore"
echo "== Git status for Dockerfile (is it modified in a PR?) =="
git status Dockerfile 2>/dev/null || echo "File status unknown"
echo "== Check .docker subdirectories =="
find . -type d -name '.docker' 2>/dev/null
echo "== Is there a root-level Docker build documented anywhere? =="
rg 'FROM node:lts' --type-listRepository: RocketChat/Rocket.Chat
Length of output: 2810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== compose.yaml service definition =="
grep -A15 'services:' compose.yaml | head -25
echo "== compose.debug.yaml service definition =="
grep -A15 'services:' compose.debug.yaml | head -25
echo "== Check if index.js might be in dist after build =="
find . -type f -name 'index.js' -path '*/dist/*' 2>/dev/null | head -5
echo "== Turbo build output - does it create root index.js? =="
rg 'index.js' turbo.json 2>/dev/null || echo "Not in turbo.json"Repository: RocketChat/Rocket.Chat
Length of output: 710
Use Yarn v4 and fix missing entrypoint in Dockerfile.
Lines 4-5 use npm despite this repo declaring "packageManager": "yarn@4.12.0" with .yarnrc.yml. This breaks workspace resolution and reproducibility in the container.
Additionally, line 10 runs node index.js, but that file does not exist in the repo. The container will fail at startup.
Proposed fix
-COPY ["package.json", "package-lock.json*", "npm-shrinkwrap.json*", "./"]
-RUN npm install --production --silent && mv node_modules ../
+COPY ["package.json", "yarn.lock", ".yarnrc.yml", "./"]
+COPY .yarn ./.yarn
+RUN corepack enable && yarn install --immutable --mode=skip-build📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| COPY ["package.json", "package-lock.json*", "npm-shrinkwrap.json*", "./"] | |
| RUN npm install --production --silent && mv node_modules ../ | |
| COPY ["package.json", "yarn.lock", ".yarnrc.yml", "./"] | |
| COPY .yarn ./.yarn | |
| RUN corepack enable && yarn install --immutable --mode=skip-build |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 4 - 5, The Dockerfile currently runs RUN npm install
--production --silent (and COPY package.json/package-lock.json) which conflicts
with the repo's declared packageManager (yarn@4) and will break workspace
resolution; replace the npm-based steps by copying .yarnrc.yml and Yarn files
(and package.json/package.json.lock if used), then run yarn install --immutable
--production --silent (or yarn workspaces focus --all --production for
workspaces) to produce a reproducible install; also the container CMD/ENTRYPOINT
currently runs node index.js which does not exist — change it to invoke the
package start script via yarn (e.g., CMD ["yarn","start"]) or point to the
correct build entry (for example the real built file under dist or the script
name in package.json) so the container starts the actual app.
| EXPOSE 3000 | ||
| RUN chown -R node /usr/src/app | ||
| USER node | ||
| CMD ["node", "index.js"] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root index.js existence check =="
fd -a '^index\.js$' .
echo "== possible startup scripts in package.json =="
rg -n '"scripts"\s*:|"start"\s*:|"dev"\s*:|"build"\s*:' package.json -A20Repository: RocketChat/Rocket.Chat
Length of output: 2796
🏁 Script executed:
#!/bin/bash
# Get full Dockerfile context to understand the image purpose and any build steps
fd -a 'Dockerfile' . | head -20Repository: RocketChat/Rocket.Chat
Length of output: 556
🏁 Script executed:
# Read the Dockerfile to see the full context around line 10
cat -n Dockerfile 2>/dev/null || echo "Dockerfile not found in root"Repository: RocketChat/Rocket.Chat
Length of output: 423
🏁 Script executed:
# Search for Meteor-specific startup or build configurations
rg -i 'meteor|start|entry' package.json -B2 -A5 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 1013
🏁 Script executed:
# Check if there are build artifacts or dist directories that might contain index.js
fd -a 'index\.js$' . -type f | grep -E '(dist|build|\.meteor|bundle)' | head -20Repository: RocketChat/Rocket.Chat
Length of output: 236
🏁 Script executed:
# Read one of the service Dockerfiles to see the correct pattern
head -20 /home/jailuser/git/ee/apps/account-service/DockerfileRepository: RocketChat/Rocket.Chat
Length of output: 754
🏁 Script executed:
# Search for any references to index.js as an entry point in build or CI configs
rg 'index\.js' --max-count=10 -B3 -A3Repository: RocketChat/Rocket.Chat
Length of output: 30061
🏁 Script executed:
# Check if there's a meteor build configuration or output directory
find . -maxdepth 3 -name '.meteor' -type d 2>/dev/null || echo "No .meteor directories in first 3 levels"Repository: RocketChat/Rocket.Chat
Length of output: 87
🏁 Script executed:
# Look for build output or dist directories that might contain index.js
ls -la apps/meteor/ 2>/dev/null | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1339
The Dockerfile entry point will fail at container startup—no root index.js exists in this monorepo.
Line 10 hardcodes node index.js, but the repository contains no root-level index.js file. The main application is a Meteor app in apps/meteor/ (confirmed by .meteor/ directory), and the Dockerfile copies the entire monorepo without a corresponding build step or proper entry point for the Meteor application. Additionally, compose.debug.yaml contains the same problematic command. The Dockerfile needs either a build stage that compiles/bundles the Meteor app or a corrected entry point that references the actual application location.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` at line 10, The Dockerfile's CMD currently uses "node index.js"
which will fail because there is no root index.js; update the Dockerfile (and
compose.debug.yaml) to either add a build stage that builds the Meteor app from
apps/meteor/ (e.g., run meteor build or npm install/build in apps/meteor and
produce a runnable bundle) and set CMD to start the built Meteor bundle, or
change CMD to run the Meteor app directly from apps/meteor/ (e.g., start script
in package.json in apps/meteor/) — locate the CMD instruction and any references
in compose.debug.yaml and replace the hardcoded node index.js with the correct
build+start flow or the meteor app start command so the container starts the
actual Meteor application.
| throw new Error(`Failed to get or create joining user: ${userId}`); | ||
| } | ||
|
|
||
| // ✅ FIX: Safe avatar handling |
Contributor
There was a problem hiding this comment.
Remove implementation comment from runtime code.
Please drop this inline comment (and emoji) to match repo guidance for TS/JS implementation code.
As per coding guidelines, "**/*.{ts,tsx,js}: Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ee/packages/federation-matrix/src/events/member.ts` at line 246, Remove the
inline implementation comment "// ✅ FIX: Safe avatar handling" from
ee/packages/federation-matrix/src/events/member.ts so runtime
TypeScript/JavaScript code contains no implementation comments; locate the
comment near the member event handling code (e.g., within the member event
handler or related function in this file) and delete that single comment line to
comply with the repository's TS/JS comment guidelines.
sharanyamahajan
force-pushed
the
chore/remove-i18next-todo
branch
from
4252952 to
cc2a576
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
Dockerfile (1)
4-5:⚠️ Potential issue | 🔴 CriticalAlready-reported container bootstrap mismatch still applies (Yarn/Corepack + entrypoint).
This duplicates earlier feedback: install flow conflicts with the repo’s Yarn v4 setup, and
CMD ["node", "index.js"]is still expected to fail if that file is not present in the container runtime path.Based on learnings: "always use yarn install --immutable with the Yarn version pinned in package.json via Corepack."
Also applies to: 10-10
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 4 - 5, The Dockerfile is using npm-based copy/install and moves node_modules which conflicts with the repo's Yarn v4/Corepack setup and may leave CMD ["node","index.js"] pointing to a missing file; replace the RUN line that uses npm with a Yarn v4 install flow: ensure the image uses Corepack/Yarn, copy the repository lockfile used by Yarn (yarn.lock) and package.json, and run yarn install --immutable (with the packageManager pinned in package.json) so installs are deterministic and compatible with Yarn v4; also remove the mv node_modules hack and ensure the container CMD/entrypoint references an existing runtime path or update CMD to the correct start script (refer to the COPY, RUN and CMD lines in the diff).apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts (1)
282-304:⚠️ Potential issue | 🟠 MajorKeep moderation reasons out of the shared
messageobject and raw logs.
any[]plus(message as any).moderationReasons = reasonsturns this into an untyped side effect onIMessage, and the samemessageinstance is later reused across notification builders. That creates a hidden contract and risks leaking server-only moderation metadata into downstream payloads. Theconsole.loghere also adds unstructured user-linked logging in a hot path. Either extend the message type deliberately or keep this metadata in a separate typed structure, and use the existing server logger if this signal really needs to be emitted.This verifies whether
moderationReasonsis actually part ofIMessageor only injected ad hoc here; expected result is noIMessagedeclaration for it and a local mutation/logging site in this file.#!/bin/bash set -e echo "Checking whether IMessage declares moderationReasons..." ast-grep --pattern $'interface IMessage {\n $$$\n moderationReasons: $_\n $$$\n}' || true echo echo "Finding moderationReasons declarations/usages..." rg -n -C3 --type=ts --type=tsx '\bmoderationReasons\b' echo echo "Inspecting the any-cast mutation and raw logging in sendNotificationsOnMessage..." rg -n -C2 --type=ts '\(message as any\)|console\.log' apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts` around lines 282 - 304, Do not mutate the shared IMessage instance or emit raw console logs: remove the ad-hoc (message as any).moderationReasons = reasons and the console.log call in sendNotificationsOnMessage; instead capture the reasons array into a local, typed structure (e.g., const moderationReasons: ModerationReason[] = reasons) and pass that structure explicitly to any downstream notification builders that need it, or store it in a private map keyed by message._id if needed; use the existing server logger (e.g., serverLogger.info/debug) to emit a structured log entry with userId and moderationReasons rather than console.log, and ensure no IMessage interface changes are made unless you add a deliberate, reviewed field named moderationReasons to IMessage.
🧹 Nitpick comments (1)
apps/meteor/server/lib/parseMessageSearchQuery.ts (1)
243-245: Avoidanytype for theoptionsparameter.The
anytype loses type safety and IDE support. Reuse the existing constructor parameter type for consistency and better validation.♻️ Proposed fix to add proper typing
+type ParseMessageSearchQueryOptions = { + user?: IUser; + offset?: number; + limit?: number; + forceRegex?: boolean; +}; + -export function parseMessageSearchQuery(text: string, options: any) { +export function parseMessageSearchQuery(text: string, options: ParseMessageSearchQueryOptions = {}) { const parser = new MessageSearchQueryParser(options); return parser.parse(text); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/parseMessageSearchQuery.ts` around lines 243 - 245, Replace the use of the unsafe any for the options parameter in parseMessageSearchQuery by reusing the constructor parameter type of MessageSearchQueryParser; change the signature from (text: string, options: any) to use ConstructorParameters<typeof MessageSearchQueryParser>[0] (or the named options type if one exists) so the parameter matches MessageSearchQueryParser's constructor type and restores type safety when constructing new MessageSearchQueryParser(options).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts`:
- Around line 282-304: Do not mutate the shared IMessage instance or emit raw
console logs: remove the ad-hoc (message as any).moderationReasons = reasons and
the console.log call in sendNotificationsOnMessage; instead capture the reasons
array into a local, typed structure (e.g., const moderationReasons:
ModerationReason[] = reasons) and pass that structure explicitly to any
downstream notification builders that need it, or store it in a private map
keyed by message._id if needed; use the existing server logger (e.g.,
serverLogger.info/debug) to emit a structured log entry with userId and
moderationReasons rather than console.log, and ensure no IMessage interface
changes are made unless you add a deliberate, reviewed field named
moderationReasons to IMessage.
In `@Dockerfile`:
- Around line 4-5: The Dockerfile is using npm-based copy/install and moves
node_modules which conflicts with the repo's Yarn v4/Corepack setup and may
leave CMD ["node","index.js"] pointing to a missing file; replace the RUN line
that uses npm with a Yarn v4 install flow: ensure the image uses Corepack/Yarn,
copy the repository lockfile used by Yarn (yarn.lock) and package.json, and run
yarn install --immutable (with the packageManager pinned in package.json) so
installs are deterministic and compatible with Yarn v4; also remove the mv
node_modules hack and ensure the container CMD/entrypoint references an existing
runtime path or update CMD to the correct start script (refer to the COPY, RUN
and CMD lines in the diff).
---
Nitpick comments:
In `@apps/meteor/server/lib/parseMessageSearchQuery.ts`:
- Around line 243-245: Replace the use of the unsafe any for the options
parameter in parseMessageSearchQuery by reusing the constructor parameter type
of MessageSearchQueryParser; change the signature from (text: string, options:
any) to use ConstructorParameters<typeof MessageSearchQueryParser>[0] (or the
named options type if one exists) so the parameter matches
MessageSearchQueryParser's constructor type and restores type safety when
constructing new MessageSearchQueryParser(options).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e201da8f-4c9c-492f-bf4c-9d4931444b6a
📒 Files selected for processing (12)
.dockerignore.vscode/launch.json.vscode/tasks.jsonDockerfileapps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/app/utils/server/escapeRegExp.tsapps/meteor/client/components/UserCard/UserCard.tsxapps/meteor/client/components/message/content/reactions/Reaction.tsxapps/meteor/server/lib/parseMessageSearchQuery.tscompose.debug.yamlcompose.yamlee/packages/federation-matrix/src/events/member.ts
💤 Files with no reviewable changes (2)
- apps/meteor/client/components/message/content/reactions/Reaction.tsx
- apps/meteor/client/components/UserCard/UserCard.tsx
✅ Files skipped from review due to trivial changes (6)
- .vscode/launch.json
- apps/meteor/app/utils/server/escapeRegExp.ts
- .dockerignore
- compose.yaml
- compose.debug.yaml
- .vscode/tasks.json
🚧 Files skipped from review as they are similar to previous changes (1)
- ee/packages/federation-matrix/src/events/member.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.ts
🧠 Learnings (31)
📓 Common learnings
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.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
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.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.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/app/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.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/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.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/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-03-11T16:46:55.955Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:55.955Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.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/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/lib/server/lib/sendNotificationsOnMessage.tsapps/meteor/server/lib/parseMessageSearchQuery.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/server/lib/parseMessageSearchQuery.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/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
apps/meteor/server/lib/parseMessageSearchQuery.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/server/lib/parseMessageSearchQuery.tsDockerfile
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/server/lib/parseMessageSearchQuery.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/lib/parseMessageSearchQuery.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/lib/parseMessageSearchQuery.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
Dockerfile
📚 Learning: 2026-03-17T16:08:37.572Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 39690
File: packages/ui-voip/package.json:11-11
Timestamp: 2026-03-17T16:08:37.572Z
Learning: In `packages/ui-voip/package.json` (RocketChat/Rocket.Chat), the team deliberately chose to use `rm -rf dist` directly in the `"build"` script instead of `rimraf`, as they decided against introducing the `rimraf` dependency for this package. Do not flag `rm -rf dist` in the ui-voip build script as a cross-platform issue requiring rimraf.
Applied to files:
Dockerfile
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
Dockerfile
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
Dockerfile
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
Dockerfile
📚 Learning: 2026-02-26T19:26:01.702Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:26:01.702Z
Learning: The RocketChat/Rocket.Chat project does not use Biome for linting, despite the presence of a biome.json file in the repository. Lint-related suggestions should not reference Biome rules.
Applied to files:
Dockerfile
🪛 Trivy (0.69.3)
Dockerfile
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[error] 5-6: 'apt-get' missing '--no-install-recommends'
'--no-install-recommends' flag is missed: 'apt-get update && apt-get install -y build-essential git'
Rule: DS-0029
(IaC/Dockerfile)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🔇 Additional comments (5)
Dockerfile (1)
8-9: Good security baseline: runtime drops root privileges.
chown+USER nodeaddresses the non-root runtime requirement.apps/meteor/server/lib/parseMessageSearchQuery.ts (4)
58-58: Remove the inline status comments.These "✅ FIXED" annotations don't provide durable context and will go stale. As per coding guidelines, avoid code comments in the implementation.
Also applies to line 165.
201-206: Anchor the sort token with a word boundary.The regex
/(?:order|sort):(asc|desc)/gstill matches thesort:ascprefix insort:ascending, leavingendingbehind in the search text.- return text.replace(/(?:order|sort):(asc|desc)/g, (_: string, direction: string) => { + return text.replace(/(?:order|sort):(asc|desc)\b/g, (_: string, direction: string) => {
208-216:forceRegexflag is stored but never used.The constructor accepts and stores
forceRegex(line 34), butconsumeMessageTextalways emits$textsearch. This silently breaks theMessage_AlwaysSearchRegExpsetting that callers pass.Either restore the regex-mode branch when
this.forceRegexis true, or explicitly remove the parameter and update callers in a separate breaking-change PR.
47-52: Good security hardening withescapeRegExp.Escaping usernames before building regex patterns prevents regex injection. The
^...$anchoring ensures exact username matching. Same improvement inconsumeMentionis also well done.
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
Removed an orphaned TODO comment related to i18next pluralization from the Reaction component.
The comment is no longer relevant as pluralization is already handled by the component from @rocket.chat/fuselage.
This change improves code clarity and removes outdated technical debt markers.
Issue(s)
Closes #39925
Steps to test
Further comments
This is a cleanup-only change with no impact on functionality.
Summary by CodeRabbit
New Features
Bug Fixes