chore: improve prometheus metrics#39857
Open
d-gubert wants to merge 18 commits into
Open
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPrometheus metrics were expanded across the codebase: new *_seconds Histograms, *_total Counters, response-size Histogram, active-requests Gauge, and an event-loop utilization Gauge. The REST metrics middleware signature changed to accept explicit instruments and was reattached in routers; many call sites now emit the new metrics or move increments to post-success paths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Router as API Router
participant Metrics as MetricsMiddleware
participant Handler as Route Handler
participant Registry as MetricsRegistry
participant Tracer as TracerSpan
participant Logger as LoggerMiddleware
Client->>Router: HTTP request
Router->>Metrics: invoke middleware
Metrics->>Registry: inc rocketchatRestApiActiveRequests (method) rgba(0,128,0,0.5)
Metrics->>Handler: call next()
Handler-->>Metrics: response available
Metrics->>Registry: observe endpointTimeSummary & endpointTimeHistogram (status, method, entrypoint) rgba(0,0,255,0.5)
Metrics->>Registry: observe responseSizeHistogram (content-length) rgba(255,165,0,0.5)
Metrics->>Registry: dec rocketchatRestApiActiveRequests (method) rgba(0,128,0,0.5)
Metrics->>Tracer: pass context
Tracer->>Logger: pass context
Logger->>Client: send response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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 |
|
d-gubert
force-pushed
the
improve-metric-naming
branch
from
371c277 to
67ae5c8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39857 +/- ##
===========================================
+ Coverage 69.60% 69.62% +0.01%
===========================================
Files 3317 3317
Lines 121868 121906 +38
Branches 21821 21811 -10
===========================================
+ Hits 84831 84875 +44
+ Misses 33702 33692 -10
- Partials 3335 3339 +4
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.
1 issue found across 22 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/middlewares/metrics.ts">
<violation number="1" location="apps/meteor/app/api/server/middlewares/metrics.ts:30">
P2: Ensure `activeRequestsGauge.dec()` runs in a `finally` block so failed requests do not leak in-flight gauge counts.
(Based on your team's feedback about concurrency-related behavioral changes.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/lib/server/lib/debug.js (1)
79-92:⚠️ Potential issue | 🟡 MinorStop method timers in a
finallyblock so failures are measured too.If
originalHandlerthrows, both timers are skipped. That underreports failed method latency.Proposed fix
return tracerActiveSpan( `Method ${name}`, { attributes: { method: name, userId: this.userId, }, }, async () => { - const result = await originalHandler.apply(this, originalArgs); - end(); - endHistogram(); - return result; + try { + return await originalHandler.apply(this, originalArgs); + } finally { + end(); + endHistogram(); + } }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/lib/debug.js` around lines 79 - 92, The timers for the traced method are only stopped on successful completion, so move the end() and endHistogram() calls into a finally block around the await of originalHandler to ensure they run on both success and error; inside the tracerActiveSpan callback that currently awaits originalHandler.apply(this, originalArgs), wrap that await in try { return result } finally { end(); endHistogram(); } (preserve rethrow behavior) so tracerActiveSpan, the Method ${name} span, originalHandler, originalArgs, and this.userId remain unchanged but timers always stop.
🧹 Nitpick comments (3)
apps/meteor/app/api/server/middlewares/metrics.spec.ts (1)
28-41: Consider asserting the new metric hooks (histogram,responseSize,activeRequests).The new dependencies are mocked and injected, but not explicitly asserted. Adding call assertions will lock the middleware’s expanded contract in tests.
Also applies to: 91-105, 152-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/middlewares/metrics.spec.ts` around lines 28 - 41, Add assertions in the tests to verify the new metric hooks are invoked: after exercising metricsMiddleware (the instance created via metricsMiddleware(...) in the spec), assert histogram.startTimer (or the returned timer) was called/used, responseSizeHistogram.observe was called with expected sizes, and activeRequestsGauge.inc/dec were called to reflect request lifecycle; reference the mocked objects named histogram, responseSizeHistogram, and activeRequestsGauge so the test locks the middleware contract and fails if those hooks stop being used.apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.ts (1)
51-56: Add assertions for the new_totalwebhook counters.You added the new mocks, but current expectations still mostly validate legacy counters. Adding explicit assertions for
totalLivechatWebhooksSuccessTotal/totalLivechatWebhooksFailuresTotalwill prevent silent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.ts` around lines 51 - 56, Tests currently only assert legacy counters but you added new mocks totalLivechatWebhooksSuccessTotal and totalLivechatWebhooksFailuresTotal; update the assertions in webhooks.spec.ts to explicitly assert that metrics.totalLivechatWebhooksSuccessTotal.inc and metrics.totalLivechatWebhooksFailuresTotal.inc are called the expected number of times (or with expected args) in the same test cases that assert totalLivechatWebhooksSuccess.inc / totalLivechatWebhooksFailures.inc; locate the expectations around the webhook handling tests and add matching sinon assertions for these two new spies so the new `_total` counters are validated too.apps/meteor/app/metrics/server/lib/metrics.ts (1)
88-93: Consider removing the0bucket boundary.The first bucket
0in the response size histogram captures responses with exactly 0 bytes. Since the middleware already guards withif (contentLength > 0), this bucket will always be empty. Consider starting at100instead.♻️ Suggested change
rocketchatRestApiResponseSizeBytes: new client.Histogram({ name: 'rocketchat_rest_api_response_size_bytes', help: 'histogram of rocketchat rest api response sizes in bytes', labelNames: ['method', 'entrypoint', 'status'], - buckets: [0, 100, 1000, 10000, 100000, 1000000, 10000000, 100000000, 1000000000], + buckets: [100, 1000, 10000, 100000, 1000000, 10000000, 100000000, 1000000000], }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/metrics/server/lib/metrics.ts` around lines 88 - 93, The histogram for response sizes (rocketchatRestApiResponseSizeBytes created via client.Histogram) includes a 0 bucket that will never be used because the middleware only records metrics when contentLength > 0; update the buckets array to remove the 0 boundary (e.g., start at 100) so buckets better reflect observed data and reduce empty buckets in the prometheus histogram configuration.
🤖 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/middlewares/metrics.ts`:
- Around line 25-57: The activeRequestsGauge is incremented before awaiting
next() but only decremented after it, so if await next() throws the gauge will
drift; update the middleware (the async (c, next) => { ... } handler) to wrap
await next() in a try/finally where activeRequestsGauge.dec(methodLabel) is
called in the finally block (keep methodLabel creation and the rest of the
post-next metrics logic unchanged) so the gauge is always decremented even when
next() throws.
In `@apps/meteor/app/lib/server/functions/notifications/email.js`:
- Around line 187-193: The payload-creation step is incrementing
metrics.notificationsSentTotal and then sendEmailFromData increments it again,
causing double-counting; remove the metrics.notificationsSentTotal.inc call from
the function that builds/returns the email payload (the earlier block that
returns `email`) and keep a single increment in sendEmailFromData (i.e., only
call metrics.notificationsSentTotal.inc inside sendEmailFromData), or
alternatively guard the payload increment so it runs only for non-queued
immediate sends; ensure the only symbol left incrementing the total in the
queued flow is metrics.notificationsSentTotal.inc in sendEmailFromData.
---
Outside diff comments:
In `@apps/meteor/app/lib/server/lib/debug.js`:
- Around line 79-92: The timers for the traced method are only stopped on
successful completion, so move the end() and endHistogram() calls into a finally
block around the await of originalHandler to ensure they run on both success and
error; inside the tracerActiveSpan callback that currently awaits
originalHandler.apply(this, originalArgs), wrap that await in try { return
result } finally { end(); endHistogram(); } (preserve rethrow behavior) so
tracerActiveSpan, the Method ${name} span, originalHandler, originalArgs, and
this.userId remain unchanged but timers always stop.
---
Nitpick comments:
In `@apps/meteor/app/api/server/middlewares/metrics.spec.ts`:
- Around line 28-41: Add assertions in the tests to verify the new metric hooks
are invoked: after exercising metricsMiddleware (the instance created via
metricsMiddleware(...) in the spec), assert histogram.startTimer (or the
returned timer) was called/used, responseSizeHistogram.observe was called with
expected sizes, and activeRequestsGauge.inc/dec were called to reflect request
lifecycle; reference the mocked objects named histogram, responseSizeHistogram,
and activeRequestsGauge so the test locks the middleware contract and fails if
those hooks stop being used.
In `@apps/meteor/app/metrics/server/lib/metrics.ts`:
- Around line 88-93: The histogram for response sizes
(rocketchatRestApiResponseSizeBytes created via client.Histogram) includes a 0
bucket that will never be used because the middleware only records metrics when
contentLength > 0; update the buckets array to remove the 0 boundary (e.g.,
start at 100) so buckets better reflect observed data and reduce empty buckets
in the prometheus histogram configuration.
In `@apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.ts`:
- Around line 51-56: Tests currently only assert legacy counters but you added
new mocks totalLivechatWebhooksSuccessTotal and
totalLivechatWebhooksFailuresTotal; update the assertions in webhooks.spec.ts to
explicitly assert that metrics.totalLivechatWebhooksSuccessTotal.inc and
metrics.totalLivechatWebhooksFailuresTotal.inc are called the expected number of
times (or with expected args) in the same test cases that assert
totalLivechatWebhooksSuccess.inc / totalLivechatWebhooksFailures.inc; locate the
expectations around the webhook handling tests and add matching sinon assertions
for these two new spies so the new `_total` counters are validated too.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 380b8058-9c84-4d8f-99c0-41cd5baa3ac6
📒 Files selected for processing (22)
apps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/lib/server/startup/rateLimiter.jsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/metrics/server/lib/collectMetrics.tsapps/meteor/app/metrics/server/lib/metrics.tsapps/meteor/app/push-notifications/server/lib/PushNotification.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/server/services/meteor/service.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/server/startup/callbacks.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/tests/unit/server/services/omnichannel/queue.tests.tsapps/meteor/app/push-notifications/server/lib/PushNotification.tsapps/meteor/app/metrics/server/lib/collectMetrics.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/startup/rateLimiter.jsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/server/services/meteor/service.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/api.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/server/startup/callbacks.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/metrics/server/lib/metrics.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/api/server/middlewares/metrics.spec.ts
🧠 Learnings (36)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: 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-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/methods/sendMessage.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/methods/sendMessage.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/tests/unit/server/services/omnichannel/queue.tests.tsapps/meteor/app/push-notifications/server/lib/PushNotification.tsapps/meteor/app/metrics/server/lib/collectMetrics.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/server/services/meteor/service.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/api.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/server/startup/callbacks.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/metrics/server/lib/metrics.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/tests/unit/server/services/omnichannel/queue.tests.tsapps/meteor/app/push-notifications/server/lib/PushNotification.tsapps/meteor/app/metrics/server/lib/collectMetrics.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/server/services/meteor/service.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/api.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/server/startup/callbacks.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/metrics/server/lib/metrics.ts
📚 Learning: 2025-11-17T14:30:36.271Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 37491
File: packages/desktop-api/src/index.ts:17-27
Timestamp: 2025-11-17T14:30:36.271Z
Learning: In the Rocket.Chat desktop API (`packages/desktop-api/src/index.ts`), the `CustomNotificationOptions` type has an optional `id` field by design. Custom notifications dispatched without an ID cannot be closed programmatically using `closeCustomNotification`, and this is intentional.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/desktop.ts
📚 Learning: 2025-11-17T22:38:48.631Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37505
File: packages/i18n/src/locales/en.i18n.json:3765-3765
Timestamp: 2025-11-17T22:38:48.631Z
Learning: Rocket.Chat i18n copy: Keep sentence case for the value of "Notification_Desktop_show_voice_calls" in packages/i18n/src/locales/en.i18n.json (“Show desktop notifications for voice calls”) per design directive; do not change to Title Case even if nearby labels differ.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/desktop.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/server/services/meteor/service.tsapps/meteor/server/services/video-conference/service.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/unit/server/services/omnichannel/queue.tests.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/tests/unit/server/services/omnichannel/queue.tests.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-03-02T16:31:41.304Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39250
File: apps/meteor/tests/end-to-end/api/livechat/07-queue.ts:1084-1094
Timestamp: 2026-03-02T16:31:41.304Z
Learning: In E2E API tests at apps/meteor/tests/end-to-end/api/livechat/, using sleep(1000) after updateSetting() or updateEESetting() calls in test setup hooks is acceptable and intentional to allow omnichannel settings to propagate their side effects.
Applied to files:
apps/meteor/tests/unit/server/services/omnichannel/queue.tests.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.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 : Implement proper wait strategies for dynamic content in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/unit/server/services/omnichannel/queue.tests.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/unit/server/services/omnichannel/queue.tests.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/api/server/middlewares/metrics.spec.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/omnichannel/queue.tests.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/api/server/middlewares/metrics.spec.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/unit/server/services/omnichannel/queue.tests.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-03-06T18:09:17.867Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/elements/Timestamp/DateTimeFormats.spec.tsx:20-23
Timestamp: 2026-03-06T18:09:17.867Z
Learning: In the RocketChat/Rocket.Chat gazzodown package (`packages/gazzodown`), tests are intended to run under the UTC timezone, but as of PR `#39397` this is NOT yet explicitly enforced in `jest.config.ts` or the `package.json` test scripts (which just run `jest` without `TZ=UTC`). To make timezone-sensitive snapshot tests reliable across all environments, `TZ=UTC` should be added to the test scripts in `package.json` or to `jest.config.ts` via `testEnvironmentOptions.timezone`. Without explicit UTC enforcement, snapshot tests involving date-fns formatted output or `toLocaleString()` will fail for contributors in non-UTC timezones.
Applied to files:
apps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/app/push-notifications/server/lib/PushNotification.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/api/server/api.tsapps/meteor/server/services/video-conference/service.ts
📚 Learning: 2026-03-16T22:56:54.500Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39677
File: packages/models/src/helpers/omnichannel/agentStatus.ts:10-29
Timestamp: 2026-03-16T22:56:54.500Z
Learning: In `packages/models/src/helpers/omnichannel/agentStatus.ts` (PR `#39677`), the `queryStatusAgentOnline` function intentionally omits the `$or` offline-status guard for non-bot agents when `isLivechatEnabledWhenAgentIdle === true`. This is by design: the setting `Livechat_enabled_when_agent_idle` (`accept_chats_when_agent_idle`) means agents should receive chats even when idle/offline, so the offline filter must be removed in that path. Bots are always status-agnostic and are always included regardless of their online/offline status. Do not flag this as a bug.
Applied to files:
apps/meteor/server/services/omnichannel/queue.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/server/services/video-conference/service.ts
📚 Learning: 2026-03-03T11:11:48.541Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:48.541Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/integrations/server/api/api.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/api/server/api.ts
📚 Learning: 2026-03-20T13:52:29.575Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:29.575Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/integrations/server/api/api.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/metrics/server/lib/metrics.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/integrations/server/api/api.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/integrations/server/api/api.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/api/server/api.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/ee/server/apps/communication/rest.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 `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/api/server/middlewares/metrics.spec.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/unit/app/livechat/server/lib/webhooks.spec.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/unit/app/livechat/server/lib/webhooks.spec.ts
📚 Learning: 2026-03-11T16:46:55.955Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:55.955Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.
Applied to files:
apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/app/api/server/middlewares/metrics.spec.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 : Group related tests in the same file
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.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/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-03-16T11:57:17.987Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:43-45
Timestamp: 2026-03-16T11:57:17.987Z
Learning: In `apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts`, `externalIds` lookups are namespaced per `app.id`. Because `cleanupApps()` is called before each test run and a fresh app is installed (giving a new unique `app.id`), fixed `userId` strings like `'nonexistent-id'` in null-path tests are safe and cannot collide with stale visitor records from previous runs. Do not flag these as needing unique/random values.
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.ts
🔇 Additional comments (18)
apps/meteor/app/lib/server/lib/processDirectEmail.ts (1)
113-113: Consistent_totalinstrumentation addition.Line 113 correctly mirrors the existing message counter path for this flow.
apps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts (1)
63-63: Test mock updated correctly for new histogram metric.Line 63 keeps the unit test metrics stub aligned with the queue instrumentation.
apps/meteor/app/push-notifications/server/lib/PushNotification.ts (1)
107-107: Mobile push counter parity is maintained.Line 107 correctly adds the
_totalcompanion counter before dispatch.apps/meteor/app/metrics/server/lib/collectMetrics.ts (1)
95-95:/metricsscrape counter now has_totalvariant.Line 95 is a correct and safe addition for Prometheus naming compliance.
apps/meteor/server/services/video-conference/service.ts (1)
678-678: Direct videoconf push path now tracks_totalas expected.Line 678 is aligned with the PR’s counter naming migration.
apps/meteor/app/lib/server/functions/notifications/desktop.ts (1)
70-70: Desktop notification_totalmetric wiring looks good.Line 70 correctly mirrors the existing desktop notification increment.
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
109-109:sendMessagenow exports the_totalcompanion counter as intended.Line 109 is aligned with the counter naming migration.
apps/meteor/server/services/omnichannel/queue.ts (1)
272-274: LGTM: queue wait time is now captured consistently for summary and histogram.Using one
waitTimeSecondsvalue for both observations avoids drift between metric series.apps/meteor/app/lib/server/startup/rateLimiter.js (1)
129-138: LGTM: label reuse + dual counter increments are clean and consistent.This keeps the legacy and
_totalseries synchronized with identical labels.apps/meteor/server/services/meteor/service.ts (1)
143-145: LGTM: shared elapsed-time calculation improves metric consistency.Recording both units from the same
elapsedMssource is the right approach.apps/meteor/app/integrations/server/api/api.ts (1)
430-442: LGTM: middleware migration is complete and correctly wired.The updated metrics bindings and chain order look consistent with the new middleware contract.
apps/meteor/ee/server/apps/communication/rest.ts (1)
75-85: LGTM: metrics middleware options were migrated correctly.All required metric instruments are passed with the expected bindings.
apps/meteor/server/startup/callbacks.ts (1)
10-37: LGTM!The dual-metric tracking implementation correctly captures timing for both summaries and histograms. The stop closures properly call both
stopTimer()andstopHistogram()without arguments, matching the() => voidreturn type contract defined incallbacksBase.ts.apps/meteor/app/api/server/api.ts (1)
105-121: LGTM!The middleware configuration correctly passes all required metric objects (
endpointTimeSummary,endpointTimeHistogram,responseSizeHistogram,activeRequestsGauge) and the reordering places metrics collection first in the chain for accurate request timing.apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts (1)
43-43: LGTM!The
deprecationsTotalcounter increments consistently mirror the existingdeprecationscounter calls with matching label dimensions.Also applies to: 67-67, 97-97, 120-120, 130-130, 157-157
apps/meteor/app/livechat/server/lib/webhooks.ts (1)
35-56: LGTM!The
*Totalcounter increments correctly mirror the existing webhook success/failure counters on all outcome paths.apps/meteor/app/metrics/server/lib/metrics.ts (2)
7-8: LGTM!Good bucket configurations:
latencyBucketscovers sub-10ms to 10s range appropriate for API latency, andqueueWaitBucketscovers 1s to 1hr for queue processing times.
331-339: LGTM!The Event Loop Utilization gauge correctly implements differential measurement by storing the baseline and calculating utilization since the last scrape. The
collect()callback pattern ensures fresh data on each Prometheus scrape.
d-gubert
force-pushed
the
improve-metric-naming
branch
from
67ae5c8 to
aa9ac55
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/ee/server/apps/communication/rest.ts (1)
74-87: Align middleware order with the new observability pattern.At Line 74, logger is still mounted before metrics/tracer. To keep behavior consistent with the stated metrics-first rollout, mount as
metrics -> tracer -> logger.Proposed reorder
this.api.router - .use(loggerMiddleware(logger)) .use( metricsMiddleware({ basePathRegex: new RegExp(/^\/api\/apps\//), api: this.api, settings, endpointTimeSummary: metrics.rocketchatRestApi, endpointTimeHistogram: metrics.rocketchatRestApiSeconds, responseSizeHistogram: metrics.rocketchatRestApiResponseSizeBytes, activeRequestsGauge: metrics.rocketchatRestApiActiveRequests, }), ) - .use(tracerSpanMiddleware); + .use(tracerSpanMiddleware) + .use(loggerMiddleware(logger));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/apps/communication/rest.ts` around lines 74 - 87, Reorder the middleware registration so metricsMiddleware is mounted first, then tracerSpanMiddleware, and finally loggerMiddleware(logger); specifically update the chain that currently calls .use(loggerMiddleware(logger)).use(metricsMiddleware(...)).use(tracerSpanMiddleware) to instead call .use(metricsMiddleware(...)).use(tracerSpanMiddleware).use(loggerMiddleware(logger)), keeping the same metrics config (endpointTimeSummary: metrics.rocketchatRestApi, endpointTimeHistogram: metrics.rocketchatRestApiSeconds, responseSizeHistogram: metrics.rocketchatRestApiResponseSizeBytes, activeRequestsGauge: metrics.rocketchatRestApiActiveRequests) and using the same api/settings objects so behavior is preserved while aligning with the metrics-first observability pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/ee/server/apps/communication/rest.ts`:
- Around line 74-87: Reorder the middleware registration so metricsMiddleware is
mounted first, then tracerSpanMiddleware, and finally loggerMiddleware(logger);
specifically update the chain that currently calls
.use(loggerMiddleware(logger)).use(metricsMiddleware(...)).use(tracerSpanMiddleware)
to instead call
.use(metricsMiddleware(...)).use(tracerSpanMiddleware).use(loggerMiddleware(logger)),
keeping the same metrics config (endpointTimeSummary: metrics.rocketchatRestApi,
endpointTimeHistogram: metrics.rocketchatRestApiSeconds, responseSizeHistogram:
metrics.rocketchatRestApiResponseSizeBytes, activeRequestsGauge:
metrics.rocketchatRestApiActiveRequests) and using the same api/settings objects
so behavior is preserved while aligning with the metrics-first observability
pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fde42f6-eb68-4c42-a6a8-c123e479ea3d
📒 Files selected for processing (22)
apps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/lib/server/startup/rateLimiter.jsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/metrics/server/lib/collectMetrics.tsapps/meteor/app/metrics/server/lib/metrics.tsapps/meteor/app/push-notifications/server/lib/PushNotification.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/server/services/meteor/service.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/server/startup/callbacks.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts
✅ Files skipped from review due to trivial changes (7)
- apps/meteor/app/lib/server/lib/processDirectEmail.ts
- apps/meteor/app/lib/server/methods/sendMessage.ts
- apps/meteor/app/lib/server/functions/notifications/email.js
- apps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts
- apps/meteor/app/lib/server/startup/rateLimiter.js
- apps/meteor/server/startup/callbacks.ts
- apps/meteor/app/api/server/api.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/meteor/app/push-notifications/server/lib/PushNotification.ts
- apps/meteor/server/services/video-conference/service.ts
- apps/meteor/app/lib/server/functions/notifications/desktop.ts
- apps/meteor/app/lib/server/lib/debug.js
- apps/meteor/app/integrations/server/api/api.ts
- apps/meteor/app/livechat/server/lib/webhooks.ts
- apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
- apps/meteor/app/api/server/middlewares/metrics.ts
- apps/meteor/server/services/omnichannel/queue.ts
- apps/meteor/app/api/server/middlewares/metrics.spec.ts
- apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.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/metrics/server/lib/collectMetrics.tsapps/meteor/server/services/meteor/service.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/metrics/server/lib/metrics.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: 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-20T13:52:29.575Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:29.575Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/metrics/server/lib/collectMetrics.tsapps/meteor/ee/server/apps/communication/rest.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/metrics/server/lib/collectMetrics.tsapps/meteor/server/services/meteor/service.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/metrics/server/lib/metrics.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/metrics/server/lib/collectMetrics.tsapps/meteor/server/services/meteor/service.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/metrics/server/lib/metrics.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/server/services/meteor/service.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/ee/server/apps/communication/rest.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/ee/server/apps/communication/rest.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/ee/server/apps/communication/rest.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/ee/server/apps/communication/rest.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/ee/server/apps/communication/rest.ts
🔇 Additional comments (4)
apps/meteor/app/metrics/server/lib/collectMetrics.ts (1)
94-96: Good dual-write for counter migration.Incrementing both legacy and
_totalcounters in the same success path is a clean compatibility step.apps/meteor/server/services/meteor/service.ts (1)
143-145: Nice unit-safe dual instrumentation.Computing
elapsedMsonce and emitting both ms-summary and seconds-histogram keeps values consistent and avoids duplicate time reads.apps/meteor/app/metrics/server/lib/metrics.ts (2)
7-327: Strong metrics expansion and naming alignment.The
_totalcounter additions and new*_seconds/REST histograms are consistent and well-structured for Prometheus aggregation.
331-339: ELU gauge implementation looks correct.Using a rolling ELU baseline in
collect()is a solid approach for scrape-interval utilization reporting.
d-gubert
force-pushed
the
improve-metric-naming
branch
from
aa9ac55 to
0ccd717
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/app/api/server/middlewares/metrics.ts (1)
29-34:⚠️ Potential issue | 🟠 MajorKeep the active request gauge balanced on thrown requests.
If
await next()throws, Line 34 is skipped androcketchat_rest_api_active_requestsremains incremented for that request.Proposed fix
const methodLabel = { method: c.req.method.toLowerCase() }; activeRequestsGauge.inc(methodLabel); - await next(); - - activeRequestsGauge.dec(methodLabel); + try { + await next(); + } finally { + activeRequestsGauge.dec(methodLabel); + } const { method, path, routePath } = c.req;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/middlewares/metrics.ts` around lines 29 - 34, The activeRequestsGauge is incremented before awaiting next() but not decremented if await next() throws; wrap the await next() call in a try/finally so activeRequestsGauge.dec(methodLabel) is always executed (use the existing methodLabel and activeRequestsGauge identifiers), ensuring the gauge is decremented in the finally block and rethrow any caught error if you capture it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/lib/server/lib/debug.js`:
- Around line 87-91: The current async wrapper around originalHandler calls
end() and endHistogram() only on success, so exceptions skip recording; change
the wrapper to call end() and endHistogram() inside a finally block after
awaiting originalHandler.apply(this, originalArgs) so timers are always stopped,
and ensure the original error is re-thrown (i.e., await in try, stop timers in
finally, then return result or rethrow as appropriate) for the functions
referencing originalHandler, originalArgs, end(), and endHistogram().
---
Duplicate comments:
In `@apps/meteor/app/api/server/middlewares/metrics.ts`:
- Around line 29-34: The activeRequestsGauge is incremented before awaiting
next() but not decremented if await next() throws; wrap the await next() call in
a try/finally so activeRequestsGauge.dec(methodLabel) is always executed (use
the existing methodLabel and activeRequestsGauge identifiers), ensuring the
gauge is decremented in the finally block and rethrow any caught error if you
capture it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1a9f3a3-154a-406a-aa01-3616190f0eaf
📒 Files selected for processing (15)
apps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/lib/server/startup/rateLimiter.jsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/metrics/server/lib/collectMetrics.tsapps/meteor/app/metrics/server/lib/metrics.tsapps/meteor/app/push-notifications/server/lib/PushNotification.ts
✅ Files skipped from review due to trivial changes (1)
- apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/meteor/app/lib/server/methods/sendMessage.ts
- apps/meteor/app/push-notifications/server/lib/PushNotification.ts
- apps/meteor/app/lib/server/functions/notifications/desktop.ts
- apps/meteor/app/livechat/server/lib/webhooks.ts
- apps/meteor/app/api/server/middlewares/metrics.spec.ts
- apps/meteor/app/metrics/server/lib/metrics.ts
- apps/meteor/app/lib/server/lib/processDirectEmail.ts
- apps/meteor/app/metrics/server/lib/collectMetrics.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/api.tsapps/meteor/app/lib/server/startup/rateLimiter.jsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/debug.js
🧠 Learnings (22)
📓 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: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/api/server/api.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-20T13:52:29.575Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:29.575Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/api/server/api.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/api/server/middlewares/metrics.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/api.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/api/server/middlewares/metrics.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/api.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/integrations/server/api/api.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/integrations/server/api/api.tsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/integrations/server/api/api.ts
📚 Learning: 2026-04-20T17:11:59.452Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40225
File: apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts:55-71
Timestamp: 2026-04-20T17:11:59.452Z
Learning: In `apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts`, the concern about an empty `?appId=` query param bypassing the truthy check and overriding the path `appId` in the `makeAppLogsQuery` spread is not relevant. The AJV query schema (`isAppLogsProps`) validates and rejects invalid/empty `appId` values before the action handler is reached, making the in-handler guard sufficient as-is. Do not flag this pattern as a vulnerability in future reviews of this file.
Applied to files:
apps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/app/integrations/server/api/api.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/integrations/server/api/api.ts
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-04-23T13:34:51.420Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40269
File: packages/apps-engine/src/server/accessors/Notifier.ts:19-19
Timestamp: 2026-04-23T13:34:51.420Z
Learning: In `packages/apps-engine/src/server/accessors/Notifier.ts`, the `as IUser` cast on `await this.userBridge.doGetAppUser(this.appId)` in both `notifyUser` (line ~19) and `notifyRoom` (line ~29) is intentional — the app user is guaranteed to exist at that point in the execution flow. Do not flag this as an unsafe cast. Any improvement to add a defensive guard is deferred to a future PR.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-02-23T17:53:18.785Z
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.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-04-10T21:17:22.932Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40096
File: apps/meteor/ee/server/apps/lib/redactor.ts:3-17
Timestamp: 2026-04-10T21:17:22.932Z
Learning: In RocketChat/Rocket.Chat, `X-User-Id` / `x-user-id` headers must NOT be added to redaction paths in apps log redaction (e.g., `apps/meteor/ee/server/apps/lib/redactor.ts`). The maintainer (d-gubert) has confirmed that X-User-Id is an identifier, not a credential — its presence in logs is useful for diagnostics, and `X-Auth-Token` is the only header that constitutes a real secret. Do not suggest redacting X-User-Id in future reviews of this area.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/lib/server/lib/debug.js
🔇 Additional comments (6)
apps/meteor/app/lib/server/functions/notifications/email.js (1)
181-189: Previously raised double-counting ofnotificationsSentTotalin the queued email flow was acknowledged as intentional by the author to preserve parity with the existingnotificationsSentbehavior. No new concerns on these changes.apps/meteor/app/lib/server/startup/rateLimiter.js (1)
129-138: LGTM!Label extraction is clean and the label keys match the
ddpRateLimitExceededTotaldeclaration inapps/meteor/app/metrics/server/lib/metrics.ts(limit_name, user_id, client_address, type, name, connection_id). Emitting both the legacy counter and the new_totalcounter preserves backward compatibility during the migration.apps/meteor/app/integrations/server/api/api.ts (1)
429-442: LGTM — integrations metrics wiring matches the new middleware contract.The four metric instruments are passed explicitly, and the metrics → tracer → logger order is consistent with the updated observability flow.
apps/meteor/app/api/server/api.ts (1)
105-116: LGTM — REST API metrics are wired consistently.The middleware now receives the required summary, latency histogram, response-size histogram, and active-request gauge before tracing runs.
apps/meteor/app/lib/server/lib/debug.js (2)
60-67: LGTM: shared method labels keep the summary and histogram aligned.The label set matches
meteorMethodsSecondsand avoids duplicating the same label object construction for both timers.
121-128: LGTM: subscription histogram is wired with the expected label.The new timer uses
{ subscription: name }, matching themeteorSubscriptionsSecondsmetric definition, and is stopped alongside the existing subscription timer.
d-gubert
force-pushed
the
improve-metric-naming
branch
from
0ccd717 to
90a9a80
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/api/server/middlewares/metrics.spec.ts (1)
28-70: Consider asserting the new instruments are invoked.The new
histogram,responseSizeHistogram, andactiveRequestsGaugemocks are injected but never asserted against. Adding a couple of expectations (e.g.activeRequestsGauge.inc/deccalled with{ method: 'get' },histogram.startTimercalled once) would lock down the new wiring inmetricsMiddlewareand catch regressions if any of the new instruments gets dropped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/middlewares/metrics.spec.ts` around lines 28 - 70, Add assertions that verify the new instruments injected into metricsMiddleware are actually used: assert histogram.startTimer was called once (expect(histogram.startTimer).toHaveBeenCalledTimes(1)), assert the returned timer mock (mockEndTimer) was called (already present) and also assert activeRequestsGauge.inc and activeRequestsGauge.dec were invoked with the request labels (e.g., { method: 'get' }), and assert responseSizeHistogram.observe was called at least once to record the response size; reference the test-local variables histogram, responseSizeHistogram, activeRequestsGauge, summary and mockEndTimer when adding these expectations.
🤖 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/ee/server/apps/communication/rest.ts`:
- Around line 73-86: Middleware ordering in
apps/meteor/ee/server/apps/communication/rest.ts is inconsistent with other
entry points; reorder the .use chain on this.api.router so it matches the other
routers (metrics → tracer → logger). Specifically, replace the current sequence
that calls loggerMiddleware(logger) before metricsMiddleware and
tracerSpanMiddleware so that metricsMiddleware(...) is applied first (keeping
its options like basePathRegex, api, settings, and metrics fields intact), then
tracerSpanMiddleware, and finally loggerMiddleware(logger) at the end.
---
Nitpick comments:
In `@apps/meteor/app/api/server/middlewares/metrics.spec.ts`:
- Around line 28-70: Add assertions that verify the new instruments injected
into metricsMiddleware are actually used: assert histogram.startTimer was called
once (expect(histogram.startTimer).toHaveBeenCalledTimes(1)), assert the
returned timer mock (mockEndTimer) was called (already present) and also assert
activeRequestsGauge.inc and activeRequestsGauge.dec were invoked with the
request labels (e.g., { method: 'get' }), and assert
responseSizeHistogram.observe was called at least once to record the response
size; reference the test-local variables histogram, responseSizeHistogram,
activeRequestsGauge, summary and mockEndTimer when adding these expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00fc1723-293e-46d1-bf9d-1b692752a250
📒 Files selected for processing (22)
apps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/lib/server/startup/rateLimiter.jsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/metrics/server/lib/collectMetrics.tsapps/meteor/app/metrics/server/lib/metrics.tsapps/meteor/app/push-notifications/server/lib/PushNotification.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/server/services/meteor/service.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/server/startup/callbacks.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts
✅ Files skipped from review due to trivial changes (2)
- apps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts
- apps/meteor/server/startup/callbacks.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/meteor/app/metrics/server/lib/collectMetrics.ts
- apps/meteor/app/lib/server/lib/processDirectEmail.ts
- apps/meteor/app/lib/server/functions/notifications/desktop.ts
- apps/meteor/app/push-notifications/server/lib/PushNotification.ts
- apps/meteor/app/lib/server/methods/sendMessage.ts
- apps/meteor/app/livechat/server/lib/webhooks.ts
- apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
- apps/meteor/server/services/meteor/service.ts
- apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.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 (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/lib/server/startup/rateLimiter.jsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/app/api/server/api.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/metrics/server/lib/metrics.tsapps/meteor/app/integrations/server/api/api.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/app/api/server/middlewares/metrics.spec.ts
🧠 Learnings (40)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: 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-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/server/services/video-conference/service.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-04-23T13:34:51.420Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40269
File: packages/apps-engine/src/server/accessors/Notifier.ts:19-19
Timestamp: 2026-04-23T13:34:51.420Z
Learning: In `packages/apps-engine/src/server/accessors/Notifier.ts`, the `as IUser` cast on `await this.userBridge.doGetAppUser(this.appId)` in both `notifyUser` (line ~19) and `notifyRoom` (line ~29) is intentional — the app user is guaranteed to exist at that point in the execution flow. Do not flag this as an unsafe cast. Any improvement to add a defensive guard is deferred to a future PR.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/integrations/server/api/api.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/server/services/video-conference/service.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/integrations/server/api/api.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-02-23T17:53:18.785Z
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.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-04-10T21:17:22.932Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40096
File: apps/meteor/ee/server/apps/lib/redactor.ts:3-17
Timestamp: 2026-04-10T21:17:22.932Z
Learning: In RocketChat/Rocket.Chat, `X-User-Id` / `x-user-id` headers must NOT be added to redaction paths in apps log redaction (e.g., `apps/meteor/ee/server/apps/lib/redactor.ts`). The maintainer (d-gubert) has confirmed that X-User-Id is an identifier, not a credential — its presence in logs is useful for diagnostics, and `X-Auth-Token` is the only header that constitutes a real secret. Do not suggest redacting X-User-Id in future reviews of this area.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-20T13:52:29.575Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:29.575Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
Applied to files:
apps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/integrations/server/api/api.ts
📚 Learning: 2026-04-23T18:10:53.335Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:53.335Z
Learning: In RocketChat/Rocket.Chat, the route handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892) contains a `catch` block that intercepts all route-handler errors and converts them into proper HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`). As a result, `next()` in Hono middleware (such as `metricsMiddleware` in `apps/meteor/app/api/server/middlewares/metrics.ts`) will never throw. Do not flag missing try/finally guards around `await next()` calls in this middleware for gauge/counter decrement safety, as the execution flow is never disrupted by route-level errors.
Applied to files:
apps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/api/server/api.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/integrations/server/api/api.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/ee/server/apps/communication/rest.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/ee/server/apps/communication/rest.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/integrations/server/api/api.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/ee/server/apps/communication/rest.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/metrics/server/lib/metrics.tsapps/meteor/app/integrations/server/api/api.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/ee/server/apps/communication/rest.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/metrics/server/lib/metrics.tsapps/meteor/app/integrations/server/api/api.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/server/services/video-conference/service.ts
📚 Learning: 2026-04-23T18:14:22.005Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/lib/server/lib/debug.js:87-91
Timestamp: 2026-04-23T18:14:22.005Z
Learning: In `apps/meteor/app/lib/server/lib/debug.js`, the `wrapMethods` function wraps Meteor DDP method handlers inside a `tracerActiveSpan` callback. The `end()` (summary timer) and `endHistogram()` (histogram timer) calls are placed after `await originalHandler.apply(this, originalArgs)` without a try/finally guard. This is intentional and safe: errors thrown by Meteor method handlers are caught by the route-handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892-893), which converts them into proper response objects, so execution flow is never disrupted and the timer calls are always reached. Do not flag the missing try/finally as a bug for these timer calls in `debug.js`.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/integrations/server/api/api.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-04-20T17:11:59.452Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40225
File: apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts:55-71
Timestamp: 2026-04-20T17:11:59.452Z
Learning: In `apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts`, the concern about an empty `?appId=` query param bypassing the truthy check and overriding the path `appId` in the `makeAppLogsQuery` spread is not relevant. The AJV query schema (`isAppLogsProps`) validates and rejects invalid/empty `appId` values before the action handler is reached, making the in-handler guard sufficient as-is. Do not flag this pattern as a vulnerability in future reviews of this file.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-16T11:50:18.066Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:18.066Z
Learning: In `apps/meteor/tests/end-to-end/apps/` (and Livechat E2E tests broadly), calling `createAgent()` and `makeAgentAvailable()` immediately after `updateSetting('Livechat_enabled', true)` — without any intermediate sleep — is an established, non-flaky pattern used across 10+ tests in the codebase. Do not flag this sequence as a potential race condition or suggest adding a delay between them.
Applied to files:
apps/meteor/app/lib/server/lib/debug.js
📚 Learning: 2026-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.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/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/integrations/server/api/api.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/app/lib/server/lib/debug.js
📚 Learning: 2026-04-23T18:10:53.335Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:53.335Z
Learning: In Rocket.Chat’s Hono middleware under apps/meteor/app/api/server/middlewares/*.ts, do not raise a review finding that “missing try/finally around `await next()` may break gauge/counter decrement safety” when the middleware relies on `await next()` not throwing. Rocket.Chat’s route handler wrapper in apps/meteor/app/api/server/ApiClass.ts catches and converts route-handler errors into HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`), so route-level errors will not propagate as thrown exceptions. Only flag missing try/finally if you find evidence of exceptions escaping that wrapper (e.g., direct throws inside the middleware itself or errors not handled by the wrapper).
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.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/app/api/server/middlewares/metrics.spec.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/app/api/server/middlewares/metrics.spec.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 `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.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 : Group related tests in the same file
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.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/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/integrations/server/api/api.ts
🔇 Additional comments (2)
apps/meteor/app/integrations/server/api/api.ts (1)
429-442: LGTM.Middleware chain is reordered to metrics → tracer → logger as intended, and
metricsMiddlewareis wired with the new explicit instruments consistently with other API entry points.apps/meteor/app/api/server/api.ts (1)
105-116: LGTM.
metricsMiddlewareis correctly updated to the new explicit instrument shape, and the ordering places metrics before tracer/remoteAddress/cors/logger as described in the PR objective.
d-gubert
force-pushed
the
improve-metric-naming
branch
2 times, most recently
from
8a600c6 to
3efb8c4
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/middlewares/metrics.spec.ts (1)
28-30: Consider asserting the newly injected instruments are exercised.The tests now inject
histogram,responseSizeHistogram, andactiveRequestsGaugemocks but never assert they are called. Adding assertions (e.g.,histogram.startTimerwas called,activeRequestsGauge.inc/decwere each called once with{ method: 'get' }, andresponseSizeHistogram.observewas called when a body is returned) would catch regressions in the new metrics logic that are otherwise invisible to this suite.Also applies to: 69-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/middlewares/metrics.spec.ts` around lines 28 - 30, Add assertions to the metrics spec to ensure injected instruments are exercised: assert histogram.startTimer was called and that its returned timer function was invoked, assert activeRequestsGauge.inc and activeRequestsGauge.dec were each called once with the expected labels (e.g., { method: 'get' }), and assert responseSizeHistogram.observe was called when a response body is sent; apply the same extra assertions for the other test case(s) around the existing mocks at the other location (lines 69-70) so regressions in startTimer/inc/dec/observe are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/api/server/middlewares/metrics.spec.ts`:
- Around line 28-30: Add assertions to the metrics spec to ensure injected
instruments are exercised: assert histogram.startTimer was called and that its
returned timer function was invoked, assert activeRequestsGauge.inc and
activeRequestsGauge.dec were each called once with the expected labels (e.g., {
method: 'get' }), and assert responseSizeHistogram.observe was called when a
response body is sent; apply the same extra assertions for the other test
case(s) around the existing mocks at the other location (lines 69-70) so
regressions in startTimer/inc/dec/observe are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e56cf6f7-ff36-4029-9f3f-32f5b7d9a3cc
📒 Files selected for processing (22)
apps/meteor/app/api/server/api.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/functions/notifications/desktop.tsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/lib/deprecationWarningLogger.tsapps/meteor/app/lib/server/lib/processDirectEmail.tsapps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/lib/server/startup/rateLimiter.jsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/metrics/server/lib/collectMetrics.tsapps/meteor/app/metrics/server/lib/metrics.tsapps/meteor/app/push-notifications/server/lib/PushNotification.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/server/services/meteor/service.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/server/services/video-conference/service.tsapps/meteor/server/startup/callbacks.tsapps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.tsapps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts
✅ Files skipped from review due to trivial changes (5)
- apps/meteor/app/metrics/server/lib/collectMetrics.ts
- apps/meteor/tests/unit/app/livechat/server/lib/webhooks.spec.ts
- apps/meteor/tests/unit/server/services/omnichannel/queue.tests.ts
- apps/meteor/app/lib/server/lib/deprecationWarningLogger.ts
- apps/meteor/app/metrics/server/lib/metrics.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/meteor/app/lib/server/functions/notifications/desktop.ts
- apps/meteor/app/push-notifications/server/lib/PushNotification.ts
- apps/meteor/app/lib/server/lib/processDirectEmail.ts
- apps/meteor/server/services/video-conference/service.ts
- apps/meteor/server/services/omnichannel/queue.ts
- apps/meteor/app/lib/server/startup/rateLimiter.js
- apps/meteor/server/startup/callbacks.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). (2)
- GitHub Check: 🚀 Notify external services - draft
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/api/server/api.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/server/services/meteor/service.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/app/api/server/middlewares/metrics.spec.ts
🧠 Learnings (43)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: 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-02-24T19:16:35.307Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 39003
File: apps/meteor/client/lib/chats/flows/sendMessage.ts:39-45
Timestamp: 2026-02-24T19:16:35.307Z
Learning: In apps/meteor/client/lib/chats/flows/sendMessage.ts, when sdk.call('sendMessage', ...) throws an error, the message is intentionally left with temp: true (not deleted or cleaned up) to support a future retry UI feature. This allows users to retry sending failed messages rather than losing them.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.ts
📚 Learning: 2026-04-17T23:32:07.223Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/messages.ts:348-352
Timestamp: 2026-04-17T23:32:07.223Z
Learning: In `apps/meteor/app/apps/server/converters/messages.ts`, the `timestamp` handler inside `_convertAttachmentsToApp` uses a non-null assertion (`attachment.ts!`) intentionally. The `ts` property on `MessageAttachment` is optional only to accommodate MessageAttachment creation-time scenarios; by the time `_convertAttachmentsToApp` is called, the message has already been stored and the attachment is guaranteed to have a `ts` value. Do not flag this non-null assertion as unsafe during code review.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/server/services/meteor/service.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/server/services/meteor/service.tsapps/meteor/app/lib/server/functions/notifications/email.js
📚 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/lib/server/methods/sendMessage.tsapps/meteor/app/livechat/server/lib/webhooks.ts
📚 Learning: 2026-04-23T18:10:53.335Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:53.335Z
Learning: In RocketChat/Rocket.Chat, the route handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892) contains a `catch` block that intercepts all route-handler errors and converts them into proper HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`). As a result, `next()` in Hono middleware (such as `metricsMiddleware` in `apps/meteor/app/api/server/middlewares/metrics.ts`) will never throw. Do not flag missing try/finally guards around `await next()` calls in this middleware for gauge/counter decrement safety, as the execution flow is never disrupted by route-level errors.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/api/server/api.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/lib/debug.js
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/server/services/meteor/service.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/api/server/api.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/server/services/meteor/service.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/lib/server/methods/sendMessage.tsapps/meteor/app/api/server/api.tsapps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/server/services/meteor/service.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-20T13:52:29.575Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/api/server/v1/stats.ts:98-117
Timestamp: 2026-03-20T13:52:29.575Z
Learning: In `apps/meteor/app/api/server/v1/stats.ts`, the `statistics.telemetry` POST endpoint intentionally has no `body` AJV schema in its route options. The proper request body shape (a `params` array of telemetry event objects) has not been formally defined yet, so body validation is deferred to a follow-up. Do not flag the missing body schema for this endpoint during OpenAPI migration reviews.
Applied to files:
apps/meteor/app/api/server/api.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-04-23T18:14:22.005Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/lib/server/lib/debug.js:87-91
Timestamp: 2026-04-23T18:14:22.005Z
Learning: In `apps/meteor/app/lib/server/lib/debug.js`, the `wrapMethods` function wraps Meteor DDP method handlers inside a `tracerActiveSpan` callback. The `end()` (summary timer) and `endHistogram()` (histogram timer) calls are placed after `await originalHandler.apply(this, originalArgs)` without a try/finally guard. This is intentional and safe: errors thrown by Meteor method handlers are caught by the route-handler action wrapper in `apps/meteor/app/api/server/ApiClass.ts` (around line 892-893), which converts them into proper response objects, so execution flow is never disrupted and the timer calls are always reached. Do not flag the missing try/finally as a bug for these timer calls in `debug.js`.
Applied to files:
apps/meteor/app/api/server/api.tsapps/meteor/server/services/meteor/service.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-11T16:46:55.955Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:55.955Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.
Applied to files:
apps/meteor/app/livechat/server/lib/webhooks.ts
📚 Learning: 2026-03-14T14:58:58.834Z
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Applied to files:
apps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/integrations/server/api/api.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-16T22:56:54.500Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39677
File: packages/models/src/helpers/omnichannel/agentStatus.ts:10-29
Timestamp: 2026-03-16T22:56:54.500Z
Learning: In `packages/models/src/helpers/omnichannel/agentStatus.ts` (PR `#39677`), the `queryStatusAgentOnline` function intentionally omits the `$or` offline-status guard for non-bot agents when `isLivechatEnabledWhenAgentIdle === true`. This is by design: the setting `Livechat_enabled_when_agent_idle` (`accept_chats_when_agent_idle`) means agents should receive chats even when idle/offline, so the offline filter must be removed in that path. Bots are always status-agnostic and are always included regardless of their online/offline status. Do not flag this as a bug.
Applied to files:
apps/meteor/app/livechat/server/lib/webhooks.ts
📚 Learning: 2026-02-20T09:04:55.725Z
Learnt from: Shreyas2004wagh
Repo: RocketChat/Rocket.Chat PR: 38681
File: apps/meteor/server/modules/streamer/streamer.module.ts:307-313
Timestamp: 2026-02-20T09:04:55.725Z
Learning: In apps/meteor/server/modules/streamer/streamer.module.ts, the catch block in sendToManySubscriptions intentionally uses SystemLogger.debug (not error or warn) for per-subscription delivery failures to keep logs less noisy, as this was a deliberate design choice reviewed and approved by KevLehman.
Applied to files:
apps/meteor/app/livechat/server/lib/webhooks.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-04-14T21:10:36.233Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36292
File: apps/meteor/client/hooks/useHasValidLocationHash.ts:7-12
Timestamp: 2026-04-14T21:10:36.233Z
Learning: In the RocketChat/Rocket.Chat repository, adding JSDoc-style comments to React hooks (e.g., files under apps/meteor/client/hooks/) is considered a good pattern and should not be flagged as a violation of the "avoid code comments in the implementation" guideline. The guideline against code comments does not apply to JSDoc documentation on exported hooks.
Applied to files:
apps/meteor/app/livechat/server/lib/webhooks.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/server/services/meteor/service.tsapps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-04-20T17:11:59.452Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40225
File: apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts:55-71
Timestamp: 2026-04-20T17:11:59.452Z
Learning: In `apps/meteor/ee/server/apps/communication/endpoints/appLogsHandler.ts`, the concern about an empty `?appId=` query param bypassing the truthy check and overriding the path `appId` in the `makeAppLogsQuery` spread is not relevant. The AJV query schema (`isAppLogsProps`) validates and rejects invalid/empty `appId` values before the action handler is reached, making the in-handler guard sufficient as-is. Do not flag this pattern as a vulnerability in future reviews of this file.
Applied to files:
apps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.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/ee/server/apps/communication/rest.tsapps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/lib/server/lib/debug.js
📚 Learning: 2026-03-16T11:50:18.066Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39657
File: apps/meteor/tests/end-to-end/apps/app-resolve-visitor.ts:18-21
Timestamp: 2026-03-16T11:50:18.066Z
Learning: In `apps/meteor/tests/end-to-end/apps/` (and Livechat E2E tests broadly), calling `createAgent()` and `makeAgentAvailable()` immediately after `updateSetting('Livechat_enabled', true)` — without any intermediate sleep — is an established, non-flaky pattern used across 10+ tests in the codebase. Do not flag this sequence as a potential race condition or suggest adding a delay between them.
Applied to files:
apps/meteor/ee/server/apps/communication/rest.tsapps/meteor/app/lib/server/lib/debug.js
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/ee/server/apps/communication/rest.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.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/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-02-23T17:53:18.785Z
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.
Applied to files:
apps/meteor/app/lib/server/lib/debug.jsapps/meteor/app/lib/server/functions/notifications/email.jsapps/meteor/app/api/server/middlewares/metrics.ts
📚 Learning: 2026-04-23T13:34:51.420Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40269
File: packages/apps-engine/src/server/accessors/Notifier.ts:19-19
Timestamp: 2026-04-23T13:34:51.420Z
Learning: In `packages/apps-engine/src/server/accessors/Notifier.ts`, the `as IUser` cast on `await this.userBridge.doGetAppUser(this.appId)` in both `notifyUser` (line ~19) and `notifyRoom` (line ~29) is intentional — the app user is guaranteed to exist at that point in the execution flow. Do not flag this as an unsafe cast. Any improvement to add a defensive guard is deferred to a future PR.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-04-10T21:17:22.932Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 40096
File: apps/meteor/ee/server/apps/lib/redactor.ts:3-17
Timestamp: 2026-04-10T21:17:22.932Z
Learning: In RocketChat/Rocket.Chat, `X-User-Id` / `x-user-id` headers must NOT be added to redaction paths in apps log redaction (e.g., `apps/meteor/ee/server/apps/lib/redactor.ts`). The maintainer (d-gubert) has confirmed that X-User-Id is an identifier, not a credential — its presence in logs is useful for diagnostics, and `X-Auth-Token` is the only header that constitutes a real secret. Do not suggest redacting X-User-Id in future reviews of this area.
Applied to files:
apps/meteor/app/lib/server/functions/notifications/email.js
📚 Learning: 2026-04-23T18:10:53.335Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39857
File: apps/meteor/app/api/server/middlewares/metrics.ts:25-57
Timestamp: 2026-04-23T18:10:53.335Z
Learning: In Rocket.Chat’s Hono middleware under apps/meteor/app/api/server/middlewares/*.ts, do not raise a review finding that “missing try/finally around `await next()` may break gauge/counter decrement safety” when the middleware relies on `await next()` not throwing. Rocket.Chat’s route handler wrapper in apps/meteor/app/api/server/ApiClass.ts catches and converts route-handler errors into HTTP API response objects (e.g., `api.failure`, `api.unauthorized`, `api.tooManyRequests`), so route-level errors will not propagate as thrown exceptions. Only flag missing try/finally if you find evidence of exceptions escaping that wrapper (e.g., direct throws inside the middleware itself or errors not handled by the wrapper).
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.tsapps/meteor/app/api/server/middlewares/metrics.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/app/api/server/middlewares/metrics.spec.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/app/api/server/middlewares/metrics.spec.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 `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-02-24T19:22:48.358Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/omnichannel/omnichannel-send-pdf-transcript.spec.ts:66-67
Timestamp: 2026-02-24T19:22:48.358Z
Learning: In Playwright end-to-end tests (e.g., under apps/meteor/tests/e2e/...), prefer locating elements by translated text (getByText) and ARIA roles (getByRole) over data-qa attributes. If translation values change, update the corresponding test locators accordingly. Never use data-qa locators. This guideline applies to all Playwright e2e test specs in the repository and helps keep tests robust to UI text changes and accessible semantics.
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2026-03-06T18:10:15.268Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39397
File: packages/gazzodown/src/code/CodeBlock.spec.tsx:47-68
Timestamp: 2026-03-06T18:10:15.268Z
Learning: In tests (especially those using testing-library/dom/jsdom) for Rocket.Chat components, the HTML <code> element has an implicit ARIA role of 'code'. Therefore, screen.getByRole('code') or screen.findByRole('code') will locate <code> elements even without a role attribute. Do not flag findByRole('code') as invalid in reviews; prefer using the implicit role instead of adding role="code" unless necessary for accessibility.
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.spec.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/api/server/middlewares/metrics.ts
🔇 Additional comments (12)
apps/meteor/server/services/meteor/service.ts (1)
143-145: LGTM!Clean refactor —
elapsedMscomputed once and both the existing summary (ms) and new histogram (seconds) observations are consistent with the unit conversion pattern used elsewhere formessageRoundtripTime/messageRoundtripTimeSeconds.apps/meteor/app/livechat/server/lib/webhooks.ts (1)
37-37: LGTM!The new
_total-suffixed counter increments are correctly paired with each existing success/failure site, preserving the original behavior while adding OpenMetrics-compliant counters.Also applies to: 50-50, 55-55
apps/meteor/app/lib/server/functions/notifications/email.js (1)
184-189: LGTM!Increments now happen only after
Mailer.sendresolves, and both the legacynotificationsSentand the newnotificationsSentTotalcounters are updated in a single place — consistent with the pattern indesktop.tsandPushNotification.ts.apps/meteor/app/lib/server/methods/sendMessage.ts (1)
108-113: LGTM!Metrics are now incremented only after
sendMessageresolves successfully, and the result is correctly captured and returned. No double-counting withprocessDirectEmail.ts, since the innersendMessagefunction itself does not touch these counters — onlyexecuteSendMessageandprocessDirectEmaildo, and each is a distinct entry point.apps/meteor/app/lib/server/lib/debug.js (3)
60-67: Histogram labels align with metric declaration.
meteorMethodLabelsprovidesmethod,has_connection,has_userwhich matchesrocketchat_meteor_methods_secondslabel names declared inapps/meteor/app/metrics/server/lib/metrics.ts. Sharing the labels object across both timers keeps summary and histogram consistent.
87-92: Timer stop placement is intentional.
endHistogram()placed alongsideend()after the awaited handler without a try/finally is consistent with pre-existing summary timer behavior; Meteor method errors are intercepted at the route-handler action wrapper inApiClass.ts, so execution flow reaches these calls. Based on learnings fromapps/meteor/app/lib/server/lib/debug.js:87-91(PR#39857).
121-129: Subscription histogram wiring LGTM.
meteorSubscriptionsSecondsuses{ subscription: name }, matching its declared labelNames. Stopping both timers inside the overriddenready()preserves the existing semantics.apps/meteor/app/api/server/api.ts (1)
105-116: LGTM — instruments wired consistently with the new middleware signature, and the metrics → tracer → logger chain matches the other API entry points.apps/meteor/app/integrations/server/api/api.ts (1)
429-442: LGTM —/hooks/metrics wiring and ordering mirrors the primary REST router.apps/meteor/ee/server/apps/communication/rest.ts (1)
74-87: Middleware ordering now aligned. Apps REST router matches the metrics → tracer → logger pattern used by the other entry points.apps/meteor/app/api/server/middlewares/metrics.ts (2)
25-57: LGTM on the instrument wiring.Label shapes (
{status, method, entrypoint}for histograms,{method}for the gauge, plusversion/user_agentspread for the summary) match thelabelNamesdeclared inapps/meteor/app/metrics/server/lib/metrics.ts. As discussed previously, the gauge decrement without try/finally is safe becauseApiClasswraps route errors into response objects.
54-57: No issues. The response-size tracking viacontent-lengthheader is reliable in this codebase. Compression middleware is not enabled (thecompression()middleware is commented out in metrics.ts), and all REST API responses are returned as buffered payloads viac.json()orc.body()rather than chunked/streamed responses, so Hono will consistently set thecontent-lengthheader. Therocketchat_rest_api_response_size_byteshistogram will accurately track response sizes without systematic under-reporting.> Likely an incorrect or invalid review comment.
Member
|
would this change require a change to our default dashboards? |
Contributor
|
@d-gubert I can't wait to see this merged. From an SRE perspective, moving latency instrumentation to histograms is a big reliability win. Like you said unlike summaries, histograms are aggregatable across pods, so we can compute accurate service-level p95/p99 and build consistent SLOs/alerts for a multi-instance Rocket.Chat deployments. @sampaiodiego this looks like a safe change operationally: it keeps the existing metrics and adds new _seconds histogram counterparts (plus fixes naming like _total), so it won’t break current dashboards—just unlocks better ones. The added REST response size, active in-flight requests, and event loop utilization metrics also close key observability gaps for capacity and incident triage. |
sampaiodiego
previously approved these changes
May 6, 2026
d-gubert
force-pushed
the
improve-metric-naming
branch
from
b33e44c to
996bf3f
Compare
…counters)" Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
…stogram Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
…chain Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
…leware Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
…ctiveRequestsGauge mocks Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
…stogram in metricsMiddleware Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Co-authored-by: d-gubert <1810309+d-gubert@users.noreply.github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
d-gubert
force-pushed
the
improve-metric-naming
branch
from
996bf3f to
5bfca44
Compare
sampaiodiego
approved these changes
May 8, 2026
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
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.
Motivation
The existing Prometheus metrics in Rocket.Chat has several issues:
_totalsuffix required by the OpenMetrics/Prometheus naming convention.Changes
New metrics:
_seconds) for all existing latency Summaries:rocketchat_meteor_methods_seconds,rocketchat_callbacks_seconds,rocketchat_hooks_seconds,rocketchat_rest_api_seconds,rocketchat_meteor_subscriptions_seconds,rocketchat_apps_bridge_methods_seconds,rocketchat_queue_wait_duration_seconds_totalsuffixed duplicates for counters that lacked them:rocketchat_deprecations_total,rocketchat_metrics_requests_total,rocketchat_messages_sent_total,rocketchat_notifications_sent_total,rocketchat_ddp_rate_limit_exceeded_total,rocketchat_livechat_webhooks_success_total,rocketchat_livechat_webhooks_failures_totalrocketchat_rest_api_response_size_bytes– histogram of REST API response payload sizes in bytesrocketchat_rest_api_active_requests– gauge tracking the number of concurrent in-flight REST API requestsnodejs_event_loop_utilization_ratio- gauge tracking the ratio of the event loop utilization reported by NodeJSMetrics middleware improvements:
summary/histogramparams toendpointTimeSummary/endpointTimeHistogramto clearly indicate what is being measuredendpointTimeHistograma required parameteruser_agent,version) from the histogram to reduce cardinality while keeping them on the summarymetrics → tracer → loggerin all three API entry pointsThis PR was made with Copilot generated code
Summary by CodeRabbit