refactor: update API routes to use AJV for validation and improve res…#39332
Open
ggazzo wants to merge 1 commit into
Open
refactor: update API routes to use AJV for validation and improve res…#39332ggazzo wants to merge 1 commit into
ggazzo wants to merge 1 commit into
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 |
|
Contributor
WalkthroughRefactors multiple v1 REST routes to AJV-validated per-route schemas and direct API.v1.get/post handlers; adds commands.list/run/preview, expands custom-user-status create/update/delete, and updates mailer, push, stats routes and related tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labelstype: chore 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39332 +/- ##
===========================================
+ Coverage 70.83% 70.87% +0.04%
===========================================
Files 3207 3207
Lines 113353 113353
Branches 20510 20499 -11
===========================================
+ Hits 80298 80344 +46
+ Misses 31005 30959 -46
Partials 2050 2050
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/v1/stats.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/stats.ts:77">
P1: `statistics.telemetry` body validation is too permissive: `params` items are not required to include `eventName`, but the handler assumes it exists and calls `telemetryEvent.call`, which can throw at runtime for invalid input.</violation>
</file>
<file name="apps/meteor/app/api/server/v1/mailer.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/mailer.ts:35">
P1: The new 200 response schema requires `{ success: true }`, but this handler returns `API.v1.success(result)` where `result` is `void`, causing schema mismatch.</violation>
</file>
<file name="apps/meteor/tests/end-to-end/api/commands.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/commands.ts:142">
P2: This assertion makes the “params is not a string” test validate missing `roomId` instead, creating duplicate coverage and losing the intended test case.</violation>
<violation number="2" location="apps/meteor/tests/end-to-end/api/commands.ts:174">
P2: This expectation turns the “tmid is not a string” test into another “invalid thread” test, duplicating nearby coverage and obscuring the case being validated.</violation>
</file>
<file name="apps/meteor/app/api/server/v1/custom-user-status.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/custom-user-status.ts:247">
P1: Defaulting optional `statusType` to an empty string in update requests can unintentionally clear existing status types when the field is omitted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| properties: { | ||
| params: { | ||
| type: 'array', | ||
| items: { type: 'object' }, |
Contributor
There was a problem hiding this comment.
P1: statistics.telemetry body validation is too permissive: params items are not required to include eventName, but the handler assumes it exists and calls telemetryEvent.call, which can throw at runtime for invalid input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/stats.ts, line 77:
<comment>`statistics.telemetry` body validation is too permissive: `params` items are not required to include `eventName`, but the handler assumes it exists and calls `telemetryEvent.call`, which can throw at runtime for invalid input.</comment>
<file context>
@@ -1,62 +1,103 @@
+ properties: {
+ params: {
+ type: 'array',
+ items: { type: 'object' },
+ },
+ },
</file context>
Suggested change
| items: { type: 'object' }, | |
| items: { | |
| type: 'object', | |
| properties: { | |
| eventName: { type: 'string' }, | |
| }, | |
| required: ['eventName'], | |
| additionalProperties: true, | |
| }, |
| const { from, subject, body, dryrun, query } = this.bodyParams; | ||
|
|
||
| const result = await sendMail({ from, subject, body, dryrun: Boolean(dryrun), query }); | ||
| const result = await sendMail({ from, subject, body, dryrun: Boolean(dryrun), query }); |
Contributor
There was a problem hiding this comment.
P1: The new 200 response schema requires { success: true }, but this handler returns API.v1.success(result) where result is void, causing schema mismatch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/mailer.ts, line 35:
<comment>The new 200 response schema requires `{ success: true }`, but this handler returns `API.v1.success(result)` where `result` is `void`, causing schema mismatch.</comment>
<file context>
@@ -1,41 +1,66 @@
+ const { from, subject, body, dryrun, query } = this.bodyParams;
- const result = await sendMail({ from, subject, body, dryrun: Boolean(dryrun), query });
+ const result = await sendMail({ from, subject, body, dryrun: Boolean(dryrun), query });
- return API.v1.success(result);
</file context>
| .expect((res) => { | ||
| expect(res.body).to.have.property('success', false); | ||
| expect(res.body.error).to.be.equal('The tmid parameter when provided must be a string.'); | ||
| expect(res.body.error).to.be.equal('Invalid thread.'); |
Contributor
There was a problem hiding this comment.
P2: This expectation turns the “tmid is not a string” test into another “invalid thread” test, duplicating nearby coverage and obscuring the case being validated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/commands.ts, line 174:
<comment>This expectation turns the “tmid is not a string” test into another “invalid thread” test, duplicating nearby coverage and obscuring the case being validated.</comment>
<file context>
@@ -171,7 +171,7 @@ describe('[Commands]', () => {
.expect((res) => {
expect(res.body).to.have.property('success', false);
- expect(res.body.error).to.be.equal('The tmid parameter when provided must be a string.');
+ expect(res.body.error).to.be.equal('Invalid thread.');
})
.end(done);
</file context>
| .expect((res) => { | ||
| expect(res.body).to.have.property('success', false); | ||
| expect(res.body.error).to.be.equal('The parameters for the command must be a single string.'); | ||
| expect(res.body.error).to.be.equal("must have required property 'roomId'"); |
Contributor
There was a problem hiding this comment.
P2: This assertion makes the “params is not a string” test validate missing roomId instead, creating duplicate coverage and losing the intended test case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/commands.ts, line 142:
<comment>This assertion makes the “params is not a string” test validate missing `roomId` instead, creating duplicate coverage and losing the intended test case.</comment>
<file context>
@@ -139,7 +139,7 @@ describe('[Commands]', () => {
.expect((res) => {
expect(res.body).to.have.property('success', false);
- expect(res.body.error).to.be.equal('The parameters for the command must be a single string.');
+ expect(res.body.error).to.be.equal("must have required property 'roomId'");
})
.end(done);
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/commands.ts (1)
459-459: Prefer removing inline deprecation comment from implementation code.Line 459 introduces
/*@deprecated*/; this should be tracked via type/docs/changelog instead of code comments in runtime implementation.As per coding guidelines "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/commands.ts` at line 459, Remove the inline runtime comment "/* `@deprecated` */" from the implementation; instead mark the API as deprecated via type/system-level JSDoc or the public docs/changelog and update the exported symbol's type declaration or add `@deprecated` in its definition (so the deprecation is tracked in types and documentation rather than runtime code).
🤖 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/commands.ts`:
- Around line 520-526: In processQueryOptionsOnResult the loop currently
iterates numeric indices from Object.keys(options.fields) instead of field
names, causing projection logic to skip; change the loop to iterate actual field
names (e.g., for (const field of Object.keys(options.fields)) or for (const
field in options.fields) with a hasOwnProperty check) and then use
options.fields[field as F] to decide pushing into fieldsToRemove and fieldsToGet
so include/exclude projection works correctly.
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 72-99: The request body schema statisticsTelemetryBodySchema is
too permissive and the code uses params as never to bypass types when calling
telemetryEvent.call, so tighten validation by making params an array of
discriminated-union event objects: require an eventName literal (enum of
TelemetryEvents) and per-event property schemas that match TelemetryMap entries
(e.g., slashCommandsStats -> { command: string }, updateCounter -> { settingsId:
string }); replace the blanket cast in the action handler by validating each
event against the appropriate AJV sub-schema (or precompiled validators) and
only then call telemetryEvent.call(eventName as TelemetryEvents, validatedData)
so the runtime shape matches the compile-time TelemetryMap contract (adjust
statisticsTelemetryBodySchema, add per-event schemas/validators, and remove
params as never).
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/commands.ts`:
- Line 459: Remove the inline runtime comment "/* `@deprecated` */" from the
implementation; instead mark the API as deprecated via type/system-level JSDoc
or the public docs/changelog and update the exported symbol's type declaration
or add `@deprecated` in its definition (so the deprecation is tracked in types and
documentation rather than runtime code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68ef7007-f0cd-4293-8764-6f6daffa9a4c
📒 Files selected for processing (6)
apps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/tests/end-to-end/api/commands.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/tests/end-to-end/api/commands.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/stats.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-03-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/tests/end-to-end/api/commands.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/commands.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/commands.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/tests/end-to-end/api/commands.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/tests/end-to-end/api/commands.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.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/tests/end-to-end/api/commands.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/tests/end-to-end/api/commands.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/tests/end-to-end/api/commands.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/stats.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/tests/end-to-end/api/commands.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/stats.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/custom-user-status.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/stats.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/stats.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/custom-user-status.tsapps/meteor/app/api/server/v1/commands.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/custom-user-status.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/stats.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/push.ts
📚 Learning: 2026-03-02T16:21:44.002Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39174
File: apps/meteor/server/services/push/tokenManagement/registerPushToken.ts:47-49
Timestamp: 2026-03-02T16:21:44.002Z
Learning: In the Rocket.Chat push token system (apps/meteor/server/services/push/tokenManagement/registerPushToken.ts), the main (non-VoIP) token is responsible for keeping authToken, appName, and userId updated. VoIP tokens are supplementary—when a device already has a non-VoIP token registered, VoIP token updates only modify the voipToken field, delegating metadata management to the main token registration flow.
Applied to files:
apps/meteor/app/api/server/v1/push.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/app/api/server/v1/push.ts
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/app/api/server/v1/commands.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:
apps/meteor/app/api/server/v1/commands.ts
🔇 Additional comments (6)
apps/meteor/app/api/server/v1/push.ts (1)
209-287:push.get/push.infomigration looks clean and behavior-preserving.The AJV route config, auth requirements, and success-path handlers are consistent with the previous endpoint behavior.
apps/meteor/app/api/server/v1/stats.ts (1)
18-66:statisticsandstatistics.listroute migration is solid.Auth, response wiring, and action flow are preserved while moving to explicit route definitions.
apps/meteor/app/api/server/v1/mailer.ts (1)
13-66: Mailer endpoint migration is consistent and low-risk.The move to per-route
body/responseschemas preserves existing behavior and keeps permission/rate-limit constraints intact.apps/meteor/app/api/server/v1/commands.ts (1)
126-457: The AJV-backed route migration for commands endpoints is well-structured.Request/response schema wiring and handler transitions are consistent across list/run/preview paths.
apps/meteor/tests/end-to-end/api/commands.ts (1)
127-174: Assertion updates are consistent with the new validation/forbidden responses.These expectation changes look aligned with the AJV-based request validation and explicit forbidden payload behavior.
Also applies to: 480-480
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
50-268: Custom user status route migration looks consistent and complete.Body validation, response schemas, and action handlers are wired cleanly across list/create/delete/update.
Comment on lines
+520
to
+526
| for (const field in Object.keys(options.fields)) { | ||
| if (options.fields.hasOwnProperty(field as F)) { | ||
| if (options.fields[field as F] === 0) { | ||
| fieldsToRemove.push(field as F); | ||
| } else if (options.fields[field as F] === 1) { | ||
| fieldsToGet.push(field as F); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix field-projection iteration in processQueryOptionsOnResult.
Line 520 iterates indexes of Object.keys(...) ("0", "1", ...) instead of actual field names, so include/exclude field projection logic is effectively skipped.
🔧 Suggested fix
- for (const field in Object.keys(options.fields)) {
- if (options.fields.hasOwnProperty(field as F)) {
+ for (const field of Object.keys(options.fields) as F[]) {
+ if (Object.prototype.hasOwnProperty.call(options.fields, field)) {
if (options.fields[field as F] === 0) {
fieldsToRemove.push(field as F);
} else if (options.fields[field as F] === 1) {
fieldsToGet.push(field as F);
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/commands.ts` around lines 520 - 526, In
processQueryOptionsOnResult the loop currently iterates numeric indices from
Object.keys(options.fields) instead of field names, causing projection logic to
skip; change the loop to iterate actual field names (e.g., for (const field of
Object.keys(options.fields)) or for (const field in options.fields) with a
hasOwnProperty check) and then use options.fields[field as F] to decide pushing
into fieldsToRemove and fieldsToGet so include/exclude projection works
correctly.
Comment on lines
+72
to
+99
| const statisticsTelemetryBodySchema = ajv.compile<StatisticsTelemetryBody>({ | ||
| type: 'object', | ||
| properties: { | ||
| params: { | ||
| type: 'array', | ||
| items: { type: 'object' }, | ||
| }, | ||
| }, | ||
| additionalProperties: true, | ||
| }); | ||
|
|
||
| API.v1.post( | ||
| 'statistics.telemetry', | ||
| { authRequired: true }, | ||
| { | ||
| post() { | ||
| const events = this.bodyParams; | ||
| authRequired: true, | ||
| body: statisticsTelemetryBodySchema, | ||
| response: { | ||
| 200: successResponseSchema, | ||
| 401: validateUnauthorizedErrorResponse, | ||
| }, | ||
| }, | ||
| function action() { | ||
| const events = this.bodyParams; | ||
|
|
||
| events?.params?.forEach((event) => { | ||
| const { eventName, ...params } = event; | ||
| void telemetryEvent.call(eventName, params); | ||
| }); | ||
| events?.params?.forEach((event: { eventName: string; [key: string]: unknown }) => { | ||
| const { eventName, ...params } = event; | ||
| void telemetryEvent.call(eventName as TelemetryEvents, params as never); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for ITelemetryEvent interface and the call method signature
find . -type f -name "*.ts" | xargs grep -l "ITelemetryEvent" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 216
🏁 Script executed:
# Find the call method definition in ITelemetryEvent
rg -n "call.*TelemetryEvents" --type=ts -B2 -A5 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 1792
🏁 Script executed:
# Check the actual stats.ts file to verify the code snippet
cat -n apps/meteor/app/api/server/v1/stats.ts | sed -n '70,105p'Repository: RocketChat/Rocket.Chat
Length of output: 1022
🏁 Script executed:
# Search for TelemetryMap definition
rg -n "TelemetryMap" --type=ts -B2 -A5 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 4552
🏁 Script executed:
# Check telemetryEvent initialization and its type
rg -n "telemetryEvent.*=" --type=ts apps/meteor/app/api/server/v1/stats.ts -B2 -A2Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the StatisticsTelemetryBody type definition
rg -n "StatisticsTelemetryBody" --type=ts -B5 -A10 apps/meteor/app/api/server/v1/stats.tsRepository: RocketChat/Rocket.Chat
Length of output: 458
🏁 Script executed:
# Check the complete ITelemetryEvent and its types
cat -n packages/core-services/src/types/ITelemetryEvent.tsRepository: RocketChat/Rocket.Chat
Length of output: 693
🏁 Script executed:
# Verify the Param type definition
cat -n packages/rest-typings/src/v1/statistics.tsRepository: RocketChat/Rocket.Chat
Length of output: 2412
🏁 Script executed:
# Check if there's any runtime validation after schema validation
rg -n "bodyParams" --type=ts -B5 -A10 apps/meteor/app/api/server/v1/stats.ts | head -50Repository: RocketChat/Rocket.Chat
Length of output: 501
Telemetry payload lacks proper event-type validation; schema and type-casting bypass the call<T extends TelemetryEvents>(eventName: T, data: TelemetryMap[T]) contract.
The schema (lines 72–81) validates only that params is an array of generic objects—it does not enforce the discriminated union structure defined in the Param type (rest-typings/src/v1/statistics.ts). Combined with the params as never cast at line 98, invalid payloads can bypass validation and reach telemetry event handlers.
Enforce the schema to require eventName as a literal string matching known event names and validate each event object matches the corresponding TelemetryMap[T] shape (e.g., slashCommandsStats requires { command: string }, updateCounter requires { settingsId: string }).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/stats.ts` around lines 72 - 99, The request
body schema statisticsTelemetryBodySchema is too permissive and the code uses
params as never to bypass types when calling telemetryEvent.call, so tighten
validation by making params an array of discriminated-union event objects:
require an eventName literal (enum of TelemetryEvents) and per-event property
schemas that match TelemetryMap entries (e.g., slashCommandsStats -> { command:
string }, updateCounter -> { settingsId: string }); replace the blanket cast in
the action handler by validating each event against the appropriate AJV
sub-schema (or precompiled validators) and only then call
telemetryEvent.call(eventName as TelemetryEvents, validatedData) so the runtime
shape matches the compile-time TelemetryMap contract (adjust
statisticsTelemetryBodySchema, add per-event schemas/validators, and remove
params as never).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/meteor/app/api/server/v1/commands.ts (1)
519-526:⚠️ Potential issue | 🟡 MinorField projection loop iterates indexes, not field names.
for (const field in Object.keys(options.fields))iterates key-array indexes, so projection checks won’t target actual field names.🔧 Suggested fix
- for (const field in Object.keys(options.fields)) { - if (options.fields.hasOwnProperty(field as F)) { + for (const field of Object.keys(options.fields) as F[]) { + if (Object.prototype.hasOwnProperty.call(options.fields, field)) { if (options.fields[field as F] === 0) { fieldsToRemove.push(field as F); } else if (options.fields[field as F] === 1) { fieldsToGet.push(field as F); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/commands.ts` around lines 519 - 526, The loop over projection fields is iterating array indexes because it uses "for (const field in Object.keys(options.fields))"; change it to iterate actual field names (e.g., use a for...of over Object.keys(options.fields) or a for...in directly over options.fields) so the checks against options.fields[field] target real field names, and adjust/remove the hasOwnProperty check accordingly; update the logic that populates fieldsToRemove and fieldsToGet to use the corrected field variable (references: options.fields, fieldsToRemove, fieldsToGet).apps/meteor/app/api/server/v1/stats.ts (1)
74-101:⚠️ Potential issue | 🟠 MajorTelemetry request validation is still too permissive for typed dispatch.
paramsitems are accepted as generic objects, then force-cast intotelemetryEvent.call(...). This bypasses theTelemetryEvents/payload contract and can pass malformed events to handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/stats.ts` around lines 74 - 101, The telemetry endpoint currently accepts generic objects and force-casts them into telemetryEvent.call, letting malformed events through; update statisticsTelemetryBodySchema to require each params item to be an object with a required string eventName and a params/payload object (or union/discriminator if you have per-event schemas) and validate eventName against the TelemetryEvents enum before calling handlers; in the API.v1.post action (function action) explicitly check typeof eventName === 'string' and that eventName is a known TelemetryEvents value (e.g., via Object.values(TelemetryEvents).includes(eventName)) and only then invoke telemetryEvent.call(eventName as TelemetryEvents, params), otherwise log/skip and return a validation error.
🧹 Nitpick comments (3)
apps/meteor/app/api/server/v1/custom-user-status.ts (2)
185-201: Prefer fetching created/updated status by returned_idinstead ofname.
insertOrUpdateUserStatusalready returns the target id; querying bynameadds an avoidable round-trip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 185 - 201, The handler action() currently calls insertOrUpdateUserStatus(this.userId, userStatusData) but then re-queries the DB using CustomUserStatus.findOneByName(userStatusData.name); instead, capture the id returned from insertOrUpdateUserStatus, validate it, and fetch the created/updated record by that id (e.g., CustomUserStatus.findOneById(returnedId) or equivalent) to avoid the extra name-based query; if the returned id is missing or the lookup by id fails, throw the same Meteor.Error as before.
65-72: MovecustomUserStatusIdemptiness validation into schema.Add
minLength: 1tocustomUserStatusIdand drop the runtime guard so invalid payloads fail uniformly at validation time.Also applies to: 221-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 65 - 72, Update the JSON Schema used by customUserStatusDeleteBodySchema to enforce non-empty IDs by adding "minLength: 1" to the customUserStatusId property so empty strings are rejected at validation time; then remove the redundant runtime guard that checks for emptiness where the schema is used (ensure the runtime code relies on ajv.validate/compile result and returns a validation error instead of a manual empty-string check). Also apply the same minLength addition to the corresponding schema used around the other occurrence referenced (the schema defined at the later block) so both payloads uniformly fail validation for empty customUserStatusId values.apps/meteor/app/api/server/v1/commands.ts (1)
459-459: Drop inline implementation comment to match repository style rule.Keep deprecation signaling via naming/export path or docs instead of inline code comments in implementation files.
As per coding guidelines: Avoid code comments in the implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/commands.ts` at line 459, Remove the inline comment "/* `@deprecated` */" from the implementation and instead mark deprecation via the public API (rename/export path) or add a proper JSDoc `@deprecated` on the exported symbol (e.g., the command export or function/class surrounding that comment) and update README/docs; ensure no inline implementation comments remain.
🤖 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/custom-user-status.ts`:
- Around line 74-89: The schema wrongly requires name: update
customUserStatusUpdateBodySchema to only require _id (remove 'name' from
required) and allow name to be optional (keep name: { type: 'string' } with no
required entry); then update the update handler (the
/v1/custom-user-status.update handler code that reads the body around the
existing lines 244–248) to handle missing name by either preserving the current
name when only statusType is provided or by validating that at least one of name
or statusType is present before performing the update—implement the handler to
merge provided fields into the existing CustomUserStatus record (only set fields
that exist on the request) rather than assuming name is always present.
In `@apps/meteor/app/api/server/v1/mailer.ts`:
- Around line 19-30: The response schemas for both mailer routes are missing a
400 entry for body-validation failures; update each route's response object (the
same block shown around the existing 200/401/403 entries in
apps/meteor/app/api/server/v1/mailer.ts) to include a 400 response schema for
validation errors — either by adding a compiled AJV schema matching your
validation-error shape (e.g., ajv.compile({ type: 'object', properties: { error:
{ type: 'string' }, details: { type: 'array', items: { type: 'object' } } },
required: ['error'], additionalProperties: true })) or by referencing the shared
validator used elsewhere (e.g., validateBadRequestResponse); ensure you add this
400 entry to both response blocks (the one at ~19-30 and the second at ~47-57)
so body-validation failures are declared.
In `@apps/meteor/app/api/server/v1/push.ts`:
- Around line 221-232: The response mapping for the push.get route is missing a
400 entry for query validation failures; update the response object inside
push.get (the block that currently defines 200 and 401) to include a 400 mapping
that validates the shape of a bad-request/validation error (use ajv.compile with
the same error schema pattern used elsewhere in the codebase for validation
errors, e.g., an object containing an errors array/message and success: false or
reuse an existing helper like validateBadRequestResponse if present) so that
query schema validation failures are correctly declared.
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 90-93: The response map for the statistics.telemetry route is
missing a 400 entry to reflect the body validator; update the response object
(the same block that currently lists successResponseSchema and
validateUnauthorizedErrorResponse) to include a 400 mapping to the bad-request
validation schema (e.g. validateBadRequestResponse or your project's equivalent)
so validation failures return the correct documented response for
statistics.telemetry.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/commands.ts`:
- Around line 519-526: The loop over projection fields is iterating array
indexes because it uses "for (const field in Object.keys(options.fields))";
change it to iterate actual field names (e.g., use a for...of over
Object.keys(options.fields) or a for...in directly over options.fields) so the
checks against options.fields[field] target real field names, and adjust/remove
the hasOwnProperty check accordingly; update the logic that populates
fieldsToRemove and fieldsToGet to use the corrected field variable (references:
options.fields, fieldsToRemove, fieldsToGet).
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 74-101: The telemetry endpoint currently accepts generic objects
and force-casts them into telemetryEvent.call, letting malformed events through;
update statisticsTelemetryBodySchema to require each params item to be an object
with a required string eventName and a params/payload object (or
union/discriminator if you have per-event schemas) and validate eventName
against the TelemetryEvents enum before calling handlers; in the API.v1.post
action (function action) explicitly check typeof eventName === 'string' and that
eventName is a known TelemetryEvents value (e.g., via
Object.values(TelemetryEvents).includes(eventName)) and only then invoke
telemetryEvent.call(eventName as TelemetryEvents, params), otherwise log/skip
and return a validation error.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/commands.ts`:
- Line 459: Remove the inline comment "/* `@deprecated` */" from the
implementation and instead mark deprecation via the public API (rename/export
path) or add a proper JSDoc `@deprecated` on the exported symbol (e.g., the
command export or function/class surrounding that comment) and update
README/docs; ensure no inline implementation comments remain.
In `@apps/meteor/app/api/server/v1/custom-user-status.ts`:
- Around line 185-201: The handler action() currently calls
insertOrUpdateUserStatus(this.userId, userStatusData) but then re-queries the DB
using CustomUserStatus.findOneByName(userStatusData.name); instead, capture the
id returned from insertOrUpdateUserStatus, validate it, and fetch the
created/updated record by that id (e.g.,
CustomUserStatus.findOneById(returnedId) or equivalent) to avoid the extra
name-based query; if the returned id is missing or the lookup by id fails, throw
the same Meteor.Error as before.
- Around line 65-72: Update the JSON Schema used by
customUserStatusDeleteBodySchema to enforce non-empty IDs by adding "minLength:
1" to the customUserStatusId property so empty strings are rejected at
validation time; then remove the redundant runtime guard that checks for
emptiness where the schema is used (ensure the runtime code relies on
ajv.validate/compile result and returns a validation error instead of a manual
empty-string check). Also apply the same minLength addition to the corresponding
schema used around the other occurrence referenced (the schema defined at the
later block) so both payloads uniformly fail validation for empty
customUserStatusId values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 026b2070-fa6b-4c2c-9dc8-c2c94326a795
📒 Files selected for processing (6)
apps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/tests/end-to-end/api/commands.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/tests/end-to-end/api/commands.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/api/server/v1/stats.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.ts
🧠 Learnings (15)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
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:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 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/stats.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/api/server/v1/stats.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.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/stats.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.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/stats.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.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/api/server/v1/stats.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/mailer.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/stats.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.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/api/server/v1/stats.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.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/api/server/v1/stats.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/mailer.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/custom-user-status.tsapps/meteor/app/api/server/v1/commands.ts
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/app/api/server/v1/commands.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/commands.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:
apps/meteor/app/api/server/v1/commands.ts
📚 Learning: 2026-03-02T16:21:44.002Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39174
File: apps/meteor/server/services/push/tokenManagement/registerPushToken.ts:47-49
Timestamp: 2026-03-02T16:21:44.002Z
Learning: In the Rocket.Chat push token system (apps/meteor/server/services/push/tokenManagement/registerPushToken.ts), the main (non-VoIP) token is responsible for keeping authToken, appName, and userId updated. VoIP tokens are supplementary—when a device already has a non-VoIP token registered, VoIP token updates only modify the voipToken field, delegating metadata management to the main token registration flow.
Applied to files:
apps/meteor/app/api/server/v1/push.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/app/api/server/v1/push.ts
🔇 Additional comments (5)
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
101-173: List endpoint migration is clean and behavior-preserving.Using explicit query/response schemas plus parallel
statuses/totalretrieval keeps this refactor tight and readable.apps/meteor/app/api/server/v1/stats.ts (1)
18-68:statisticsandstatistics.listroute refactor looks good.The move to explicit
gethandlers with centralized success schema keeps the endpoint surface consistent.apps/meteor/app/api/server/v1/push.ts (1)
209-287:push.get/push.infomigration to explicit route schemas is solid.Auth, validation wiring, and success payloads are clear and consistent with the new pattern.
apps/meteor/app/api/server/v1/mailer.ts (1)
13-66: Route refactor is clean and keeps business logic intact.The
API.v1.post(..., action)conversion with explicit body validators is consistent and easy to follow.apps/meteor/app/api/server/v1/commands.ts (1)
126-457: Commands endpoint restructuring is coherent and well-scoped.The per-route schemas and explicit 400/401/403 mappings are consistently applied across get/list/run/preview flows.
Comment on lines
+74
to
+89
| type CustomUserStatusUpdateBody = { | ||
| _id: string; | ||
| name: string; | ||
| statusType?: string; | ||
| }; | ||
|
|
||
| const customUserStatusUpdateBodySchema = ajv.compile<CustomUserStatusUpdateBody>({ | ||
| type: 'object', | ||
| properties: { | ||
| _id: { type: 'string' }, | ||
| name: { type: 'string' }, | ||
| statusType: { type: 'string', nullable: true }, | ||
| }, | ||
| required: ['_id', 'name'], | ||
| additionalProperties: false, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find rest-typings definition for custom-user-status endpoints
find . -path "*/rest-typings/*" -name "*custom*user*status*" -o -path "*/rest-typings/src/v1*" -name "*.ts" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1291
🏁 Script executed:
# Search for customUserStatus in rest-typings
rg "customUserStatus" --type ts -A 5 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 7978
🏁 Script executed:
# Read the custom-user-status.ts file to understand full context
cat -n apps/meteor/app/api/server/v1/custom-user-status.tsRepository: RocketChat/Rocket.Chat
Length of output: 9361
🏁 Script executed:
# Check insertOrUpdateUserStatus to understand if it handles partial updates
rg "insertOrUpdateUserStatus" -A 20 apps/meteor/app/user-status/server/methods/ | head -60Repository: RocketChat/Rocket.Chat
Length of output: 6428
🏁 Script executed:
# Check if there are tests showing the expected behavior for statusType-only updates
rg "custom-user-status.update" -B 3 -A 10 --type ts | grep -A 10 "statusType"Repository: RocketChat/Rocket.Chat
Length of output: 751
Update endpoint schema requires name but contract defines it as optional, breaking the API contract.
The rest-typings definition (packages/rest-typings/src/v1/customUserStatus.ts) specifies /v1/custom-user-status.update with name?: string (optional), but the schema at lines 74–89 includes name in the required array. This rejects requests that only update statusType, contradicting the published contract.
Fix required
type CustomUserStatusUpdateBody = {
_id: string;
- name: string;
+ name?: string;
statusType?: string;
};
const customUserStatusUpdateBodySchema = ajv.compile<CustomUserStatusUpdateBody>({
type: 'object',
properties: {
_id: { type: 'string' },
name: { type: 'string' },
statusType: { type: 'string', nullable: true },
},
- required: ['_id', 'name'],
+ required: ['_id'],
additionalProperties: false,
});Also update the handler (lines 244–248) to handle cases where name is not provided—either by preserving the existing name or validating that at least one field is provided for the update.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 74 - 89,
The schema wrongly requires name: update customUserStatusUpdateBodySchema to
only require _id (remove 'name' from required) and allow name to be optional
(keep name: { type: 'string' } with no required entry); then update the update
handler (the /v1/custom-user-status.update handler code that reads the body
around the existing lines 244–248) to handle missing name by either preserving
the current name when only statusType is provided or by validating that at least
one of name or statusType is present before performing the update—implement the
handler to merge provided fields into the existing CustomUserStatus record (only
set fields that exist on the request) rather than assuming name is always
present.
Comment on lines
+19
to
+30
| response: { | ||
| 200: ajv.compile({ | ||
| type: 'object', | ||
| properties: { | ||
| success: { type: 'boolean', enum: [true] }, | ||
| }, | ||
| required: ['success'], | ||
| additionalProperties: true, | ||
| }), | ||
| 401: validateUnauthorizedErrorResponse, | ||
| 403: validateForbiddenErrorResponse, | ||
| }, |
Contributor
There was a problem hiding this comment.
Include 400 responses for body-validation failures on both mailer routes.
Both endpoints define body validators, but response schemas currently do not declare 400.
Also applies to: 47-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/mailer.ts` around lines 19 - 30, The response
schemas for both mailer routes are missing a 400 entry for body-validation
failures; update each route's response object (the same block shown around the
existing 200/401/403 entries in apps/meteor/app/api/server/v1/mailer.ts) to
include a 400 response schema for validation errors — either by adding a
compiled AJV schema matching your validation-error shape (e.g., ajv.compile({
type: 'object', properties: { error: { type: 'string' }, details: { type:
'array', items: { type: 'object' } } }, required: ['error'],
additionalProperties: true })) or by referencing the shared validator used
elsewhere (e.g., validateBadRequestResponse); ensure you add this 400 entry to
both response blocks (the one at ~19-30 and the second at ~47-57) so
body-validation failures are declared.
Comment on lines
+221
to
+232
| response: { | ||
| 200: ajv.compile({ | ||
| type: 'object', | ||
| properties: { | ||
| data: {}, | ||
| success: { type: 'boolean', enum: [true] }, | ||
| }, | ||
| required: ['data', 'success'], | ||
| additionalProperties: false, | ||
| }), | ||
| ); | ||
| 401: validateUnauthorizedErrorResponse, | ||
| }, |
Contributor
There was a problem hiding this comment.
Add 400 response mapping for push.get query validation failures.
push.get has a query schema, but its declared responses currently omit 400.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/push.ts` around lines 221 - 232, The response
mapping for the push.get route is missing a 400 entry for query validation
failures; update the response object inside push.get (the block that currently
defines 200 and 401) to include a 400 mapping that validates the shape of a
bad-request/validation error (use ajv.compile with the same error schema pattern
used elsewhere in the codebase for validation errors, e.g., an object containing
an errors array/message and success: false or reuse an existing helper like
validateBadRequestResponse if present) so that query schema validation failures
are correctly declared.
Comment on lines
+90
to
+93
| response: { | ||
| 200: successResponseSchema, | ||
| 401: validateUnauthorizedErrorResponse, | ||
| }, |
Contributor
There was a problem hiding this comment.
Declare 400 for statistics.telemetry to match validator behavior.
This route has a body validator, but the response map omits a 400 entry for invalid payloads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/stats.ts` around lines 90 - 93, The response
map for the statistics.telemetry route is missing a 400 entry to reflect the
body validator; update the response object (the same block that currently lists
successResponseSchema and validateUnauthorizedErrorResponse) to include a 400
mapping to the bad-request validation schema (e.g. validateBadRequestResponse or
your project's equivalent) so validation failures return the correct documented
response for statistics.telemetry.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
185-194:⚠️ Potential issue | 🟠 MajorCreate should fetch by returned
_id, not byname.
insertOrUpdateUserStatusreturns the inserted_idfor create calls. Looking up bynameafter creation is ambiguous and can return the wrong record under concurrent same-name writes.Proposed fix
- await insertOrUpdateUserStatus(this.userId, userStatusData); - - const customUserStatus = await CustomUserStatus.findOneByName(userStatusData.name); + const createdId = await insertOrUpdateUserStatus(this.userId, userStatusData); + if (typeof createdId !== 'string') { + throw new Meteor.Error('error-creating-custom-user-status', 'Error creating custom user status'); + } + + const customUserStatus = await CustomUserStatus.findOneById(createdId); if (!customUserStatus) { throw new Meteor.Error('error-creating-custom-user-status', 'Error creating custom user status'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 185 - 194, The create flow in action() calls insertOrUpdateUserStatus but then fetches the record with CustomUserStatus.findOneByName(userStatusData.name), which is racy; update action() to capture the _id returned by insertOrUpdateUserStatus (e.g., const id = await insertOrUpdateUserStatus(this.userId, userStatusData)) and then fetch the created record by that id (e.g., use CustomUserStatus.findOneById or equivalent with the returned id); also handle the case the returned id is falsy by throwing or returning an error before attempting the fetch.apps/meteor/tests/end-to-end/apps/slash-command-test-simple.ts (1)
19-32:⚠️ Potential issue | 🔴 CriticalTest assertion cannot be reached with
command: nulldue to AJV schema validation.The endpoint uses
commandsRunBodySchemawhich requirescommand: { type: 'string' }withoutnullable: true. AJV schema validation occurs before the handler code executes, so sendingcommand: nullwill be rejected by the schema validator, not by the handler logic. The expected error message "The command provided does not exist (or is disabled)." is only produced at line 286-288 in the handler, which is unreachable for null values. Either change the test to send a valid string command that doesn't exist, or adjust the expected error message to match what AJV validation actually returns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/apps/slash-command-test-simple.ts` around lines 19 - 32, The test sends command: null which fails AJV schema validation (commandsRunBodySchema requires a string) before the handler logic that returns "The command provided does not exist (or is disabled.)" runs; update the test in apps/meteor/tests/end-to-end/apps/slash-command-test-simple.ts for the 'should return an error when no command is provided' case to either send a syntactically valid but nonexistent command (e.g., command: 'nonexistent_cmd') so the handler branch at lines 286-288 is reached, or keep command: null but change the expected assertion to match the AJV validation error response returned by the commands.run endpoint.
♻️ Duplicate comments (5)
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
74-89:⚠️ Potential issue | 🟠 MajorUpdate schema/handler currently break partial update semantics.
custom-user-status.updatestill requiresname, and the handler defaults omittedstatusTypeto'', which can clear persisted values on partial updates. This conflicts with the endpoint contract (name?: string) and changes behavior for update calls that only send a subset of fields.Based on learnings: "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."Proposed fix
type CustomUserStatusUpdateBody = { _id: string; - name: string; + name?: string; statusType?: string; }; const customUserStatusUpdateBodySchema = ajv.compile<CustomUserStatusUpdateBody>({ type: 'object', properties: { _id: { type: 'string' }, name: { type: 'string' }, statusType: { type: 'string', nullable: true }, }, - required: ['_id', 'name'], + required: ['_id'], additionalProperties: false, }); async function action() { - const userStatusData = { - _id: this.bodyParams._id, - name: this.bodyParams.name, - statusType: this.bodyParams.statusType ?? '', - }; - - const customUserStatusToUpdate = await CustomUserStatus.findOneById(userStatusData._id); + const customUserStatusToUpdate = await CustomUserStatus.findOneById(this.bodyParams._id); if (!customUserStatusToUpdate) { - return API.v1.failure(`No custom user status found with the id of "${userStatusData._id}".`); + return API.v1.failure(`No custom user status found with the id of "${this.bodyParams._id}".`); } + const userStatusData = { + _id: customUserStatusToUpdate._id, + name: this.bodyParams.name ?? customUserStatusToUpdate.name, + statusType: this.bodyParams.statusType ?? customUserStatusToUpdate.statusType ?? '', + previousName: customUserStatusToUpdate.name, + previousStatusType: customUserStatusToUpdate.statusType ?? '', + }; + await insertOrUpdateUserStatus(this.userId, userStatusData);Also applies to: 243-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 74 - 89, The update schema and handler break partial updates: change CustomUserStatusUpdateBody and customUserStatusUpdateBodySchema so fields intended to be optional for partial updates (notably name and statusType) are optional/nullable in the AJV schema (remove name from required and keep statusType optional) to match the endpoint contract, and update the custom-user-status.update handler so it does not default an omitted statusType to '' or otherwise overwrite fields when they are not provided (only apply changes for properties present in the request body) to preserve existing partial-update behavior.apps/meteor/app/api/server/v1/push.ts (1)
213-232:⚠️ Potential issue | 🟡 MinorAdd a
400response mapping forpush.getquery-validation failures.Line 213 introduces a query schema, but the response block at Line 221 only declares
200and401.Suggested patch
response: { @@ }), + 400: validateBadRequestErrorResponse, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/push.ts` around lines 213 - 232, The push.get route declares a query schema but lacks a 400 response for validation failures; update the response map inside the same object (the route handling where query: ajv.compile<{ id: string }> is defined) to include a 400 entry pointing to the project’s bad-request validator (e.g., add "400: validateBadRequestResponse") so validation errors return the correct 400 schema alongside the existing 200 and 401 entries.apps/meteor/app/api/server/v1/mailer.ts (1)
1-7:⚠️ Potential issue | 🟡 MinorDeclare
400responses for both body-validated mailer endpoints.Line 17 and Line 45 add
bodyvalidation, but the response maps at Line 19 and Line 47 do not declare400for validation failures.Suggested patch
import { ajv, isMailerProps, isMailerUnsubscribeProps, + validateBadRequestErrorResponse, validateForbiddenErrorResponse, validateUnauthorizedErrorResponse, } from '@rocket.chat/rest-typings'; @@ response: { @@ }), + 400: validateBadRequestErrorResponse, }, @@ response: { @@ }), + 400: validateBadRequestErrorResponse, },Also applies to: 19-30, 47-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/mailer.ts` around lines 1 - 7, The endpoints that validate request bodies using isMailerProps and isMailerUnsubscribeProps are missing a 400 response in their OpenAPI/response maps; update the response maps for the mailer endpoints that reference isMailerProps and isMailerUnsubscribeProps to include a 400 validation-failure response alongside the existing unauthorized/forbidden responses (use the same validate* helpers/pattern as the other error responses so validation failures are documented). Ensure both endpoints' response objects include a "400" entry for body validation errors.apps/meteor/app/api/server/v1/stats.ts (2)
70-101:⚠️ Potential issue | 🟠 MajorConstrain telemetry event payloads and remove unsafe cast bypass.
Line 79 accepts generic objects, and Line 100 bypasses
ITelemetryEvent.call<T extends TelemetryEvents>(...)viaas TelemetryEvents+as never. Invalid payloads can pass AJV and fail at runtime (event not found) instead of being rejected as bad request.Use this read-only verification to confirm the mismatch (expected: strict telemetry union in typings, generic schema in route):
#!/bin/bash set -euo pipefail sed -n '70,105p' apps/meteor/app/api/server/v1/stats.ts | nl -ba sed -n '1,40p' packages/core-services/src/types/ITelemetryEvent.ts | nl -ba sed -n '1,50p' packages/rest-typings/src/v1/statistics.ts | nl -baSuggested patch
-import type { TelemetryEvents } from '@rocket.chat/core-services'; import { ajv, validateBadRequestErrorResponse, validateUnauthorizedErrorResponse } from '@rocket.chat/rest-typings'; -type StatisticsTelemetryBody = { - params?: Array<{ eventName: string; [key: string]: unknown }>; -}; +type StatisticsTelemetryEvent = + | { eventName: 'slashCommandsStats'; command: string; timestamp?: number } + | { eventName: 'updateCounter'; settingsId: string; timestamp?: number }; + +type StatisticsTelemetryBody = { + params?: StatisticsTelemetryEvent[]; +}; const statisticsTelemetryBodySchema = ajv.compile<StatisticsTelemetryBody>({ type: 'object', properties: { params: { type: 'array', - items: { type: 'object' }, + items: { + oneOf: [ + { + type: 'object', + properties: { + eventName: { type: 'string', const: 'slashCommandsStats' }, + command: { type: 'string' }, + timestamp: { type: 'number', nullable: true }, + }, + required: ['eventName', 'command'], + additionalProperties: false, + }, + { + type: 'object', + properties: { + eventName: { type: 'string', const: 'updateCounter' }, + settingsId: { type: 'string' }, + timestamp: { type: 'number', nullable: true }, + }, + required: ['eventName', 'settingsId'], + additionalProperties: false, + }, + ], + }, }, }, additionalProperties: true, }); - events?.params?.forEach((event: { eventName: string; [key: string]: unknown }) => { - const { eventName, ...params } = event; - void telemetryEvent.call(eventName as TelemetryEvents, params as never); - }); + events?.params?.forEach((event) => { + if (event.eventName === 'slashCommandsStats') { + void telemetryEvent.call(event.eventName, { command: event.command }); + return; + } + void telemetryEvent.call(event.eventName, { settingsId: event.settingsId }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/stats.ts` around lines 70 - 101, The route currently accepts arbitrary objects and bypasses typing with casts; update StatisticsTelemetryBody/statisticsTelemetryBodySchema to enforce a discriminated union keyed by eventName (use an enum of TelemetryEvents or import the typed telemetry payload schemas from rest-typings) so AJV rejects unknown/invalid payloads, remove the unsafe "as TelemetryEvents" / "as never" casts in the API.v1.post action, and instead validate eventName at runtime (reject with 400 if not in TelemetryEvents) and call telemetryEvent.call with a properly-typed payload (or dispatch per eventName to ensure correct types) so invalid events fail validation rather than runtime "event not found" errors.
90-93:⚠️ Potential issue | 🟡 MinorAdd 400 response mapping for telemetry body validation errors.
Line 89 adds a body validator, but Line 90-93 only declares 200/401 responses.
Suggested patch
response: { + 400: validateBadRequestErrorResponse, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/stats.ts` around lines 90 - 93, The route's response object in apps/meteor/app/api/server/v1/stats.ts currently only lists successResponseSchema and validateUnauthorizedErrorResponse but is missing a 400 mapping for the body validator added above; update the same "response" object to include a 400 entry that references the validation error schema used by the telemetry body validator (i.e., add a 400: <validation-error-schema> alongside successResponseSchema and validateUnauthorizedErrorResponse) so validation failures return the correct 400 response shape.
🧹 Nitpick comments (3)
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
223-225: Remove redundant missing-param guard in delete handler.
customUserStatusDeleteBodySchemaalready requirescustomUserStatusId, so this branch is unreachable after AJV validation.Proposed cleanup
- const { customUserStatusId } = this.bodyParams; - if (!customUserStatusId) { - return API.v1.failure('The "customUserStatusId" params is required!'); - } + const { customUserStatusId } = this.bodyParams;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/custom-user-status.ts` around lines 223 - 225, Remove the redundant guard that checks for customUserStatusId in the delete handler inside apps/meteor/app/api/server/v1/custom-user-status.ts: since customUserStatusDeleteBodySchema already requires customUserStatusId via AJV, delete the if (!customUserStatusId) { return API.v1.failure(...) } branch (and any associated unreachable returns) in the delete handler so the handler relies on validated input from customUserStatusDeleteBodySchema.apps/meteor/app/api/server/v1/commands.ts (2)
233-243: Mutation side-effect creates implicit dependency on evaluation order.
processQueryOptionsOnResultmutates the input array viasplice(lines 508-513). Thecount: commands.lengthon line 241 relies on this mutation happening first. While this works due to JavaScript's left-to-right property evaluation, storing the result explicitly would be clearer:♻️ Optional refactor for clarity
- return API.v1.success({ - commands: processQueryOptionsOnResult(commands, { - sort: sort || { name: 1 }, - skip: offset, - limit: count, - }), - appsLoaded: true, - offset, - count: commands.length, - total: totalCount, - }); + const paginatedCommands = processQueryOptionsOnResult(commands, { + sort: sort || { name: 1 }, + skip: offset, + limit: count, + }); + + return API.v1.success({ + commands: paginatedCommands, + appsLoaded: true, + offset, + count: paginatedCommands.length, + total: totalCount, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/commands.ts` around lines 233 - 243, processQueryOptionsOnResult mutates the commands array and the current code relies on that side-effect via left-to-right property evaluation; to make it explicit and safe, call processQueryOptionsOnResult(...) once, store its return in a local variable (e.g., processedCommands), then pass processedCommands into API.v1.success for the commands field and use processedCommands.length for count; reference the existing symbols processQueryOptionsOnResult, commands, sort, offset, count, and API.v1.success when making the change.
269-283: Consider removing redundant type checks.These manual
typeofchecks are now redundant since the AJV schema at lines 64-76 already validates these types. However, given the refactoring scope, this could be deferred to a follow-up PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/commands.ts` around lines 269 - 283, Remove the redundant runtime typeof checks for body.command, body.params, body.roomId, and body.tmid in the command handler and rely on the existing AJV schema validation instead; locate the block performing the checks (the if statements that call API.v1.failure for command, params, roomId, and tmid) and delete those four conditional blocks (or replace them with a single assertion that request body has passed schema validation) so validation is centralized via the schema rather than duplicated in the handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/meteor/app/api/server/v1/custom-user-status.ts`:
- Around line 185-194: The create flow in action() calls
insertOrUpdateUserStatus but then fetches the record with
CustomUserStatus.findOneByName(userStatusData.name), which is racy; update
action() to capture the _id returned by insertOrUpdateUserStatus (e.g., const id
= await insertOrUpdateUserStatus(this.userId, userStatusData)) and then fetch
the created record by that id (e.g., use CustomUserStatus.findOneById or
equivalent with the returned id); also handle the case the returned id is falsy
by throwing or returning an error before attempting the fetch.
In `@apps/meteor/tests/end-to-end/apps/slash-command-test-simple.ts`:
- Around line 19-32: The test sends command: null which fails AJV schema
validation (commandsRunBodySchema requires a string) before the handler logic
that returns "The command provided does not exist (or is disabled.)" runs;
update the test in
apps/meteor/tests/end-to-end/apps/slash-command-test-simple.ts for the 'should
return an error when no command is provided' case to either send a syntactically
valid but nonexistent command (e.g., command: 'nonexistent_cmd') so the handler
branch at lines 286-288 is reached, or keep command: null but change the
expected assertion to match the AJV validation error response returned by the
commands.run endpoint.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/custom-user-status.ts`:
- Around line 74-89: The update schema and handler break partial updates: change
CustomUserStatusUpdateBody and customUserStatusUpdateBodySchema so fields
intended to be optional for partial updates (notably name and statusType) are
optional/nullable in the AJV schema (remove name from required and keep
statusType optional) to match the endpoint contract, and update the
custom-user-status.update handler so it does not default an omitted statusType
to '' or otherwise overwrite fields when they are not provided (only apply
changes for properties present in the request body) to preserve existing
partial-update behavior.
In `@apps/meteor/app/api/server/v1/mailer.ts`:
- Around line 1-7: The endpoints that validate request bodies using
isMailerProps and isMailerUnsubscribeProps are missing a 400 response in their
OpenAPI/response maps; update the response maps for the mailer endpoints that
reference isMailerProps and isMailerUnsubscribeProps to include a 400
validation-failure response alongside the existing unauthorized/forbidden
responses (use the same validate* helpers/pattern as the other error responses
so validation failures are documented). Ensure both endpoints' response objects
include a "400" entry for body validation errors.
In `@apps/meteor/app/api/server/v1/push.ts`:
- Around line 213-232: The push.get route declares a query schema but lacks a
400 response for validation failures; update the response map inside the same
object (the route handling where query: ajv.compile<{ id: string }> is defined)
to include a 400 entry pointing to the project’s bad-request validator (e.g.,
add "400: validateBadRequestResponse") so validation errors return the correct
400 schema alongside the existing 200 and 401 entries.
In `@apps/meteor/app/api/server/v1/stats.ts`:
- Around line 70-101: The route currently accepts arbitrary objects and bypasses
typing with casts; update StatisticsTelemetryBody/statisticsTelemetryBodySchema
to enforce a discriminated union keyed by eventName (use an enum of
TelemetryEvents or import the typed telemetry payload schemas from rest-typings)
so AJV rejects unknown/invalid payloads, remove the unsafe "as TelemetryEvents"
/ "as never" casts in the API.v1.post action, and instead validate eventName at
runtime (reject with 400 if not in TelemetryEvents) and call telemetryEvent.call
with a properly-typed payload (or dispatch per eventName to ensure correct
types) so invalid events fail validation rather than runtime "event not found"
errors.
- Around line 90-93: The route's response object in
apps/meteor/app/api/server/v1/stats.ts currently only lists
successResponseSchema and validateUnauthorizedErrorResponse but is missing a 400
mapping for the body validator added above; update the same "response" object to
include a 400 entry that references the validation error schema used by the
telemetry body validator (i.e., add a 400: <validation-error-schema> alongside
successResponseSchema and validateUnauthorizedErrorResponse) so validation
failures return the correct 400 response shape.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/commands.ts`:
- Around line 233-243: processQueryOptionsOnResult mutates the commands array
and the current code relies on that side-effect via left-to-right property
evaluation; to make it explicit and safe, call processQueryOptionsOnResult(...)
once, store its return in a local variable (e.g., processedCommands), then pass
processedCommands into API.v1.success for the commands field and use
processedCommands.length for count; reference the existing symbols
processQueryOptionsOnResult, commands, sort, offset, count, and API.v1.success
when making the change.
- Around line 269-283: Remove the redundant runtime typeof checks for
body.command, body.params, body.roomId, and body.tmid in the command handler and
rely on the existing AJV schema validation instead; locate the block performing
the checks (the if statements that call API.v1.failure for command, params,
roomId, and tmid) and delete those four conditional blocks (or replace them with
a single assertion that request body has passed schema validation) so validation
is centralized via the schema rather than duplicated in the handler.
In `@apps/meteor/app/api/server/v1/custom-user-status.ts`:
- Around line 223-225: Remove the redundant guard that checks for
customUserStatusId in the delete handler inside
apps/meteor/app/api/server/v1/custom-user-status.ts: since
customUserStatusDeleteBodySchema already requires customUserStatusId via AJV,
delete the if (!customUserStatusId) { return API.v1.failure(...) } branch (and
any associated unreachable returns) in the delete handler so the handler relies
on validated input from customUserStatusDeleteBodySchema.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6a192cd4-6de1-4f8e-89c7-604ee6223a7d
📒 Files selected for processing (7)
apps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/push.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/tests/end-to-end/api/commands.tsapps/meteor/tests/end-to-end/apps/slash-command-test-simple.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/tests/end-to-end/api/commands.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/tests/end-to-end/apps/slash-command-test-simple.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.ts
🧠 Learnings (24)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
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:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/slash-command-test-simple.ts
📚 Learning: 2025-12-02T22:23:49.593Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37654
File: apps/meteor/client/hooks/useAppSlashCommands.ts:32-38
Timestamp: 2025-12-02T22:23:49.593Z
Learning: In apps/meteor/client/hooks/useAppSlashCommands.ts, the `data?.forEach((command) => slashCommands.add(command))` call during render is intentional. The query is configured with `structuralSharing: false` to prevent React Query from keeping stable data references, and `slashCommands.add` is idempotent, so executing on every render is acceptable and ensures the command registry stays current.
Applied to files:
apps/meteor/tests/end-to-end/apps/slash-command-test-simple.tsapps/meteor/app/api/server/v1/commands.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/apps/slash-command-test-simple.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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/slash-command-test-simple.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/apps/slash-command-test-simple.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/tests/end-to-end/apps/slash-command-test-simple.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/tests/end-to-end/apps/slash-command-test-simple.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 : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/apps/slash-command-test-simple.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/tests/end-to-end/apps/slash-command-test-simple.tsapps/meteor/app/api/server/v1/push.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
apps/meteor/tests/end-to-end/apps/slash-command-test-simple.ts
📚 Learning: 2026-02-04T12:08:56.950Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38374
File: apps/meteor/tests/end-to-end/apps/app-logs-nested-requests.ts:26-37
Timestamp: 2026-02-04T12:08:56.950Z
Learning: In the E2E tests under apps/meteor/tests/end-to-end/apps, prefer using a fixed sleep (e.g., 1s) instead of implementing polling or retry logic when waiting for asynchronous operations to complete. Tests should fail deterministically if the expected result isn't available after the sleep.
Applied to files:
apps/meteor/tests/end-to-end/apps/slash-command-test-simple.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/tests/end-to-end/apps/slash-command-test-simple.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.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/tests/end-to-end/apps/slash-command-test-simple.tsapps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.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/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/app/api/server/v1/commands.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/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.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/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/stats.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/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/app/api/server/v1/push.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/api/server/v1/custom-user-status.tsapps/meteor/app/api/server/v1/commands.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/custom-user-status.tsapps/meteor/app/api/server/v1/commands.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/custom-user-status.tsapps/meteor/app/api/server/v1/mailer.tsapps/meteor/app/api/server/v1/stats.tsapps/meteor/app/api/server/v1/commands.tsapps/meteor/app/api/server/v1/push.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:
apps/meteor/app/api/server/v1/commands.ts
📚 Learning: 2026-03-02T16:21:44.002Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39174
File: apps/meteor/server/services/push/tokenManagement/registerPushToken.ts:47-49
Timestamp: 2026-03-02T16:21:44.002Z
Learning: In the Rocket.Chat push token system (apps/meteor/server/services/push/tokenManagement/registerPushToken.ts), the main (non-VoIP) token is responsible for keeping authToken, appName, and userId updated. VoIP tokens are supplementary—when a device already has a non-VoIP token registered, VoIP token updates only modify the voipToken field, delegating metadata management to the main token registration flow.
Applied to files:
apps/meteor/app/api/server/v1/push.ts
🔇 Additional comments (8)
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
101-172: List route schema/payload alignment looks solid.The 200 schema now explicitly matches the returned paginated payload (
statuses,count,offset,total,success), improving contract clarity.apps/meteor/app/api/server/v1/mailer.ts (1)
32-38:API.v1.postaction migration looks behavior-preserving.Both handlers keep the original flow and return shape while moving to typed route actions.
Also applies to: 59-65
apps/meteor/app/api/server/v1/push.ts (1)
234-260: Typed-route migration forpush.get/push.infohandlers looks solid.The action logic and declared success payload shapes are aligned in these segments.
Also applies to: 262-287
apps/meteor/app/api/server/v1/commands.ts (4)
1-19: LGTM!Import additions are well-organized and align with the new AJV validation approach and response schema definitions.
34-124: LGTM!Schema definitions are well-structured with proper required fields and nullable annotations for optional parameters. The
unknowntype forpreviewItem.valueappropriately handles varied preview item types.
520-528: Bug:for...in Object.keys(...)iterates array indexes, not field names.This issue was flagged in a previous review. The loop iterates numeric indexes (
"0","1", ...) instead of actual field names fromoptions.fields, causing the field projection logic to be effectively skipped.
316-457: LGTM!The preview endpoints follow consistent patterns with proper permission checks via
canAccessRoomIdAsync, thread validation, and forbidden response handling. The same optional refactor for removing redundant typeof checks applies here as noted forcommands.run.apps/meteor/app/api/server/v1/stats.ts (1)
18-68: GET route migration is clean and behavior-preserving.
authRequiredandAPI.v1.success(...)usage stay consistent for bothstatisticsandstatistics.list.
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.
…ponse schemas
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements
Tests