chore: Adds deprecation warning on livechat:addMonitor with new endpoint to replace it
#37061
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 4049425 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a deprecation warning for the Meteor method Changes
Sequence DiagramsequenceDiagram
participant Client
participant REST_API as REST API
participant Validator
participant Handler
note right of REST_API `#DDEBF7`: New endpoint /livechat/monitors.create
Client->>REST_API: POST /livechat/monitors.create\n{ username: "alice" }
REST_API->>Validator: isPOSTLivechatMonitorCreateRequest(payload)
alt valid
Validator-->>REST_API: ok
REST_API->>Handler: authorize & license check\nmanage-livechat-monitors, enterprise
alt authorized
Handler->>Handler: LivechatEnterprise.addMonitor("alice")
Handler-->>REST_API: { _id, username, roles }
REST_API-->>Client: 200 OK\n{ success: true, body: { _id, username, roles } }
else not authorized
REST_API-->>Client: 403 Forbidden
end
else invalid
Validator-->>REST_API: invalid
REST_API-->>Client: 400 Bad Request\n{ success: false, error: "..." }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37061 +/- ##
===========================================
- Coverage 68.83% 68.80% -0.04%
===========================================
Files 3361 3361
Lines 114340 114308 -32
Branches 20619 20640 +21
===========================================
- Hits 78706 78648 -58
- Misses 33536 33566 +30
+ Partials 2098 2094 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.changeset/many-walls-impress.md (1)
6-6: Clarify the endpoint path notation.The changeset describes the replacement endpoint as
livechat/monitors.save, but the actual REST endpoint path is/v1/livechat/monitors.save(including the version prefix). Consider using the full path for clarity in user-facing documentation.apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts (1)
63-95: LGTM! Consider enhancing error details in failure response.The new REST endpoint is well-structured with proper authentication, validation, and error handling. However, the generic failure message at line 92 might make debugging difficult.
Consider logging the error details for debugging:
} catch (error: unknown) { if (error instanceof Meteor.Error) { return API.v1.failure(error.reason); } + console.error('Error adding monitor:', error); return API.v1.failure('error-adding-monitor'); }apps/meteor/tests/end-to-end/api/livechat/22-monitors.ts (1)
113-145: Consider migrating monitor removal tests to REST endpoint.The monitor creation tests have been migrated to the REST endpoint, but the removal tests (lines 113-145) still use the Meteor method
livechat:removeMonitor. For consistency and to complete the REST migration, consider adding a REST endpoint for monitor removal and updating these tests accordingly.If a REST endpoint for removal already exists or is planned, verify its usage here. Otherwise, this inconsistency should be tracked for future work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
.changeset/many-walls-impress.md(1 hunks)apps/meteor/client/omnichannel/monitors/MonitorsTable.tsx(2 hunks)apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts(2 hunks)apps/meteor/ee/app/livechat-enterprise/server/methods/addMonitor.ts(2 hunks)apps/meteor/tests/data/livechat/units.ts(1 hunks)apps/meteor/tests/e2e/utils/omnichannel/monitors.ts(1 hunks)apps/meteor/tests/end-to-end/api/livechat/22-monitors.ts(1 hunks)packages/rest-typings/src/v1/omnichannel.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/utils/omnichannel/monitors.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/utils/omnichannel/monitors.ts
🧬 Code graph analysis (6)
apps/meteor/tests/end-to-end/api/livechat/22-monitors.ts (1)
apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)
apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts (5)
packages/rest-typings/src/v1/omnichannel.ts (2)
POSTLivechatMonitorsSaveSuccessResponse(546-546)isPOSTLivechatMonitorSaveRequest(531-531)packages/rest-typings/src/v1/Ajv.ts (3)
validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)validateForbiddenErrorResponse(92-92)apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
LivechatEnterprise(13-183)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)packages/rest-typings/src/index.ts (1)
Endpoints(51-98)
apps/meteor/client/omnichannel/monitors/MonitorsTable.tsx (2)
apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
addMonitor(14-30)packages/ui-contexts/src/index.ts (1)
useEndpoint(32-32)
apps/meteor/tests/data/livechat/units.ts (2)
apps/meteor/tests/e2e/utils/omnichannel/monitors.ts (1)
createMonitor(8-22)apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)
packages/rest-typings/src/v1/omnichannel.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(186-253)
apps/meteor/tests/e2e/utils/omnichannel/monitors.ts (2)
apps/meteor/tests/data/livechat/units.ts (1)
createMonitor(7-21)apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
removeMonitor(32-51)
🔇 Additional comments (5)
apps/meteor/ee/app/livechat-enterprise/server/methods/addMonitor.ts (1)
7-7: LGTM!The deprecation logging is correctly implemented and positioned before the permission check, ensuring all invocations are logged regardless of authorization outcome. The specified version and replacement endpoint align with the REST migration strategy.
Also applies to: 19-19
apps/meteor/tests/data/livechat/units.ts (1)
7-21: LGTM!The migration from the Meteor method call to the REST endpoint is correctly implemented. The simplified response handling (directly using
res.bodyinstead of parsing JSON-RPC envelope) aligns with REST conventions and the updated return type accurately reflects the new endpoint's response shape.apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts (1)
97-102: LGTM!The type extraction and module augmentation correctly expose the new endpoint types to consumers, enabling type-safe usage across the codebase.
apps/meteor/tests/end-to-end/api/livechat/22-monitors.ts (1)
83-111: LGTM!The test updates correctly migrate from RPC-style Meteor method calls to REST endpoint usage. The assertions now properly validate HTTP status codes and REST response shapes, including appropriate error handling for invalid inputs.
packages/rest-typings/src/v1/omnichannel.ts (1)
516-531: LGTM!The request type and validator are correctly defined with appropriate AJV schema validation. The required username field and schema structure align with the endpoint requirements.
apps/meteor/client/views/omnichannel/monitors/MonitorsTable.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/i18n/src/locales/en.i18n.json (1)
6246-6246: Avoid duplicate strings for the same error; confirm intended key and usagesYou added error-adding-monitor, but a similar key already exists: Failed_to_add_monitor (Line 2087). This risks divergence. Prefer reusing one key across flows (or deprecate the legacy one) to keep i18n consistent.
- Confirm which key the new REST path uses and whether the old Meteor path still references the other.
- If the REST path can reuse the existing label, drop the new key; otherwise plan a cleanup to migrate consumers to kebab-case and remove the legacy key in a follow-up.
- Ensure other locales add this key if keeping it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/i18n/src/locales/en.i18n.json(1 hunks)
⏰ 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
lucas-a-pelegrino
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
added
the
stat: ready to merge
tassoevan
left a comment
tassoevan
left a comment
There was a problem hiding this comment.
Aren't *.save endpoints supposed to update or insert objects? Since the original method uses add as verb I'm not sure if it just inserts a new monitor or it's idempotent.
We use the |
lucas-a-pelegrino
dismissed stale reviews from tassoevan and aleksandernsilva
via
497677f
lucas-a-pelegrino
requested review from
KevLehman,
aleksandernsilva and
tassoevan
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/25 21:17 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 7 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Tue, 25 Nov 2025 21:17:28 GMT |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts(2 hunks)packages/rest-typings/src/v1/omnichannel.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rest-typings/src/v1/omnichannel.ts
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.673Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
🧬 Code graph analysis (1)
apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts (5)
packages/rest-typings/src/v1/omnichannel.ts (2)
POSTLivechatMonitorsCreateSuccessResponse(546-548)isPOSTLivechatMonitorCreateRequest(531-531)packages/rest-typings/src/v1/Ajv.ts (3)
validateBadRequestErrorResponse(47-47)validateUnauthorizedErrorResponse(70-70)validateForbiddenErrorResponse(93-93)apps/meteor/ee/app/livechat-enterprise/server/lib/LivechatEnterprise.ts (1)
LivechatEnterprise(14-186)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)packages/rest-typings/src/index.ts (1)
Endpoints(51-98)
⏰ 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 comments (3)
apps/meteor/ee/app/livechat-enterprise/server/api/monitors.ts (3)
2-8: LGTM! Clean imports for the new endpoint.All imported validators and types are properly used in the endpoint definition.
Also applies to: 12-12, 14-14
63-76: Endpoint configuration looks solid.The endpoint properly enforces authentication, permissions (
manage-livechat-monitors), license checks, and request/response validation. The use of.createin the route name clearly indicates this is a create-only operation.
86-91: Type augmentation correctly implemented.The pattern of extracting routes and augmenting the
Endpointsinterface ensures type-safe access to the new endpoint across the codebase.
Proposed changes (including videos or screenshots)
This PR adds a deprecation warning for
livechat:addMonitormeteor method, as well as a new endpoint to replace it;livechat/monitors.createIssue(s)
CTZ-74
CTZ-1403
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Deprecations
Bug Fixes
Chores
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.