chore: migrate livechat/config, livechat/webhook.test, and livechat/integrations.settings to OpenAPI chained API pattern with AJV validation#39506
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: 8ae4f7d 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 |
|
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:
WalkthroughReplaced ad-hoc API.v1.addRoute usage for three Livechat endpoints with structured API.v1.get/API.v1.post endpoint definitions, added AJV-validated request/response schemas, explicit auth/permission metadata, typed endpoint constants and ExtractRoutesFromAPI type aliases, and updated/removed corresponding public REST typings. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Livechat API Server
participant Webhook as ExternalWebhook
Client->>API: POST /v1/livechat/webhook.test (auth + payload)
API->>API: validate auth & permissions
API->>API: build sample payload (visitor, agent, messages, customFields)
alt webhook URL missing
API-->>Client: 200 { success: false, error: "Webhook_URL_not_set" }
else webhook URL present
API->>Webhook: HTTP POST (payload, headers, options)
Webhook-->>API: HTTP response (status, body)
alt status 200
API-->>Client: 200 { success: true }
else
API-->>Client: 200 { success: false, error: "invalid-webhook-response" }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
NAME-ASHWANIYADAV
force-pushed
the
chore/migrate-livechat-omnichannel-openapi
branch
from
b3e9ea9 to
a13c1bc
Compare
|
Hey @ggazzo @ahmed-n-abdeltwab, migrated livechat/config, livechat/webhook.test and livechat/integrations.settings to the new OpenAPI chained pattern as part of RocketChat/Rocket.Chat-Open-API#150 Would appreciate a review when you get the time! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/livechat/server/api/v1/webhooks.ts (1)
98-106: Consider preserving the HTTP status code in the error for better diagnostics.When
request.status !== 200, the thrown error loses the actual status code before being caught and re-wrapped. This makes debugging webhook issues harder since the logs only show "error-invalid-webhook-response" without the original status.That said, given the migration-focused scope of this PR, this improvement could be deferred to a follow-up. Based on learnings: "maintainers prefer to avoid any logic changes; style-only cleanups may be deferred to follow-ups to keep scope tight."
💡 Optional improvement for better diagnostics
if (request.status === 200) { return API.v1.success(); } - throw new Error('Invalid status code'); + throw new Error(`error-invalid-webhook-response: received status ${request.status}`); } catch (error) { logger.error({ msg: 'Error testing webhook', err: error }); throw new Error('error-invalid-webhook-response'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/livechat/server/api/v1/webhooks.ts` around lines 98 - 106, The code throws a generic Error('Invalid status code') and later rethrows a generic Error('error-invalid-webhook-response'), losing the original HTTP status; update the webhook test logic so the thrown error includes the actual response status (e.g., include request.status or a message like `Invalid status code: ${request.status}`) before it is caught, and when rethrowing preserve or wrap that information so logger.error({ msg: 'Error testing webhook', err }) receives the original status; modify the error creation around the request.status check (the block using request.status, API.v1.success(), and logger.error) to attach the status code to the error message or a property.
🤖 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/livechat/server/api/v1/webhooks.ts`:
- Around line 98-106: The code throws a generic Error('Invalid status code') and
later rethrows a generic Error('error-invalid-webhook-response'), losing the
original HTTP status; update the webhook test logic so the thrown error includes
the actual response status (e.g., include request.status or a message like
`Invalid status code: ${request.status}`) before it is caught, and when
rethrowing preserve or wrap that information so logger.error({ msg: 'Error
testing webhook', err }) receives the original status; modify the error creation
around the request.status check (the block using request.status,
API.v1.success(), and logger.error) to attach the status code to the error
message or a property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb93ac30-daf5-4777-803d-a04071529b76
📒 Files selected for processing (4)
apps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.tspackages/rest-typings/src/v1/omnichannel.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rest-typings/src/v1/omnichannel.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.ts
🧠 Learnings (12)
📓 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: 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:44.506Z
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.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
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.
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.
📚 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/api/v1/webhooks.tsapps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.ts
📚 Learning: 2026-03-10T08:13:44.506Z
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:44.506Z
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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.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/livechat/server/api/v1/webhooks.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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/livechat/server/api/v1/config.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/livechat/server/api/v1/config.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/livechat/server/api/v1/config.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/livechat/server/api/v1/config.ts
🔇 Additional comments (4)
apps/meteor/app/livechat/server/api/v1/config.ts (1)
13-68: LGTM!The chained endpoint pattern is correctly implemented. The
livechat/configendpoint appropriately uses a permissive schema for the dynamic config object, whilelivechat/config/routingcorrectly reuses the predefinedGETLivechatConfigRoutingvalidator. The auth/permission configuration and module augmentation follow the established migration conventions.apps/meteor/app/livechat/imports/server/rest/integrations.ts (1)
8-31: LGTM!The endpoint migration correctly configures authentication, permissions, and response validation. The permissive
{ type: 'object' }for array items is a pragmatic choice given thatISettingis a complex union type—this matches the approach used elsewhere in the codebase for runtime validation of variable response shapes.apps/meteor/app/livechat/server/api/v1/webhooks.ts (2)
12-27: LGTM!The endpoint migration correctly configures authentication (
authRequired: true), permissions (view-livechat-webhooks), and response validation. Theajv.compile<void>pattern for the 200 response follows the established convention for migration PRs.
110-115: LGTM!The type extraction and module augmentation follow the established pattern used across all migrated endpoints in this PR.
NAME-ASHWANIYADAV
force-pushed
the
chore/migrate-livechat-omnichannel-openapi
branch
from
a13c1bc to
44a1e69
Compare
|
Hey @ahmed-n-abdeltwab, addressed both your comments! Moved GETLivechatConfigParams out of rest-typings and defined it directly in config.ts |
NAME-ASHWANIYADAV
force-pushed
the
chore/migrate-livechat-omnichannel-openapi
branch
from
44a1e69 to
492692b
Compare
|
@ahmed-n-abdeltwab i have Updated the settings array items to use $ref: '#/components/schemas/ISetting' instead of the generic type: 'object' |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/livechat/imports/server/rest/integrations.ts`:
- Line 2: Add a 403 Forbidden response entry to the endpoint's contract
responses map for the route that declares permissionsRequired:
['view-livechat-manager']; update the responses object next to the existing 200
and 401 entries to include a 403 key with a schema matching the error shape
(reuse the same error schema as 401/validateUnauthorizedErrorResponse or create
a validateForbiddenErrorResponse validator and register it with ajv), and ensure
the route's contract validation includes that 403 response so permission-denied
cases are documented.
In `@apps/meteor/app/livechat/server/api/v1/config.ts`:
- Around line 47-64: The response schema references IOmnichannelRoom and
ILivechatAgent but those types aren't registered with typia/AJV, so the $ref
links remain unresolved; open the typia schema generation call
(typia.json.schemas<...>() in packages/core-typings/src/Ajv.ts) and add
IOmnichannelRoom and ILivechatAgent to the generic type list so typia includes
them in the generated components/schemas registry, rebuild/regen the schemas,
and confirm ajv.compile in config.ts can resolve
'#/components/schemas/IOmnichannelRoom' and
'#/components/schemas/ILivechatAgent'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8bbd2281-5f61-430d-9739-ebdbb231244e
📒 Files selected for processing (4)
apps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.tspackages/rest-typings/src/v1/omnichannel.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.tspackages/rest-typings/src/v1/omnichannel.ts
🧠 Learnings (13)
📓 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: 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:44.506Z
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.
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.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
📚 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/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.tspackages/rest-typings/src/v1/omnichannel.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/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.ts
📚 Learning: 2026-03-10T08:13:44.506Z
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:44.506Z
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/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.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/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.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/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.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/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.tspackages/rest-typings/src/v1/omnichannel.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/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.tspackages/rest-typings/src/v1/omnichannel.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/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/livechat/server/api/v1/config.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/app/livechat/server/api/v1/config.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/livechat/server/api/v1/webhooks.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/livechat/server/api/v1/webhooks.ts
🔇 Additional comments (11)
apps/meteor/app/livechat/server/api/v1/webhooks.ts (6)
1-10: LGTM!Imports correctly include the necessary types and utilities for the OpenAPI migration pattern.
12-27: LGTM!The endpoint configuration correctly follows the OpenAPI chained pattern with proper auth requirements, permissions, and response schema validation.
28-73: LGTM!Sample data structure is appropriate for webhook connectivity testing purposes.
92-107: LGTM!Error handling correctly logs failures and converts them to the i18n error key
error-invalid-webhook-response. The debug logging provides appropriate visibility into webhook testing operations.
109-115: LGTM!Type extraction and module augmentation correctly follow the established OpenAPI migration pattern for this repository.
74-84: No action needed. The request automatically sets theContent-Type: application/jsonheader during serialization.The
serverFetchwrapper (viaparseRequestOptionsinpackages/server-fetch/src/parsers.ts) detects object bodies and automatically:
- Stringifies the object with
JSON.stringify()- Adds the
Content-Type: application/jsonheaderThis same pattern is used throughout the codebase, including the existing webhook sender in
apps/meteor/app/livechat/server/lib/webhooks.ts, which has been working correctly in production.> Likely an incorrect or invalid review comment.apps/meteor/app/livechat/server/api/v1/config.ts (1)
41-106: Migration structure looks correct.The chained endpoint pattern using
API.v1.get()with query validation, response schemas, and the module augmentation follows the established OpenAPI migration conventions in this codebase. The endpoint logic remains unchanged, which aligns with the migration scope. Based on learnings: "In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes."packages/rest-typings/src/v1/omnichannel.ts (2)
2826-2833: Formatting improvement looks good.The explicit two-branch structure with the clarifying comment for
sendToVisitor: falseimproves readability without changing runtime behavior.
4574-5182: Endpoint removals align with the migration.The removal of
GET /v1/livechat/config,GET /v1/livechat/integrations.settings, andPOST /v1/livechat/webhook.testfromOmnichannelEndpointsis correct since these endpoints are now defined via module augmentation in their respective files (config.ts,integrations.ts,webhooks.ts). The unifiedEndpointsinterface will still expose them to consumers likeddp-clientand UI components.apps/meteor/app/livechat/imports/server/rest/integrations.ts (2)
16-30: The migrated schema and handler stay behavior-preserving.
findIntegrationSettings()still supplies{ settings }, andAPI.v1.success()adds the standardsuccess: truewrapper, so this keeps the migration scoped to the OpenAPI surface without changing endpoint logic.Based on learnings, maintainers prefer to avoid any logic changes in OpenAPI migration PRs and defer scope-expanding cleanups to follow-ups.
34-39: Good use of extracted route typing here.Deriving
LivechatIntegrationsEndpointsfromExtractRoutesFromAPI<typeof livechatIntegrationsEndpoints>keeps the public REST typing coupled to the implementation instead of another manual omnichannel entry.
NAME-ASHWANIYADAV
force-pushed
the
chore/migrate-livechat-omnichannel-openapi
branch
from
492692b to
fd73ca7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/app/livechat/server/api/v1/webhooks.ts (1)
81-81: Remove the new inlineSECURITYcomment.Please keep that rationale in the PR/docs instead of the implementation so this migration stays aligned with the repo’s TS/JS guideline.
As per coding guidelines "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/livechat/server/api/v1/webhooks.ts` at line 81, Remove the inline SECURITY comment "// SECURITY: Webhooks can only be configured by users with enough privileges. It's ok to disable this check here." from apps/meteor/app/livechat/server/api/v1/webhooks.ts and instead document the rationale in the PR description or external docs; do not add implementation comments in the file and ensure no other functional changes are made to the surrounding code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/livechat/server/api/v1/config.ts`:
- Around line 47-50: The 200-response AJV type/schema only includes config, room
and agent but the actual response includes a guest field; update the ajv.compile
generic and schema object to include guest (e.g., extend the return type from {
config: { [k: string]: string | boolean } & { room?: IOmnichannelRoom; agent?:
ILivechatAgent } } to also include guest?: IGuest or the correct guest shape) so
ExtractRoutesFromAPI emits the complete client-facing type; modify the
ajv.compile call and the response contract to explicitly declare guest alongside
room and agent (keeping additionalProperties: true).
In `@apps/meteor/app/livechat/server/api/v1/webhooks.ts`:
- Around line 93-97: The debug logs currently emit the full webhookUrl and full
response body which may leak sensitive query params or remote content; update
the logging around the fetch in the webhook handler to avoid logging webhookUrl
and response text directly—use the URL host (e.g., new URL(webhookUrl).host) and
the HTTP status code (response.status on the fetch Response) instead, and remove
or redact logger.debug calls that include webhookUrl or raw response content
(references: webhookUrl, fetch(..., options), request/response, logger.debug).
---
Nitpick comments:
In `@apps/meteor/app/livechat/server/api/v1/webhooks.ts`:
- Line 81: Remove the inline SECURITY comment "// SECURITY: Webhooks can only be
configured by users with enough privileges. It's ok to disable this check here."
from apps/meteor/app/livechat/server/api/v1/webhooks.ts and instead document the
rationale in the PR description or external docs; do not add implementation
comments in the file and ensure no other functional changes are made to the
surrounding code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41d4045f-77f0-4247-a4c7-84ea4389123a
📒 Files selected for processing (5)
apps/meteor/app/livechat/imports/server/rest/integrations.tsapps/meteor/app/livechat/server/api/v1/config.tsapps/meteor/app/livechat/server/api/v1/webhooks.tspackages/core-typings/src/Ajv.tspackages/rest-typings/src/v1/omnichannel.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/livechat/imports/server/rest/integrations.ts
- packages/rest-typings/src/v1/omnichannel.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/server/api/v1/config.tspackages/core-typings/src/Ajv.ts
🧠 Learnings (14)
📓 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: 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:44.506Z
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.
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.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
📚 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/api/v1/webhooks.tsapps/meteor/app/livechat/server/api/v1/config.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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/server/api/v1/config.ts
📚 Learning: 2026-03-10T08:13:44.506Z
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:44.506Z
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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/server/api/v1/config.tspackages/core-typings/src/Ajv.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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/server/api/v1/config.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/app/livechat/server/api/v1/webhooks.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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/server/api/v1/config.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/livechat/server/api/v1/webhooks.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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/server/api/v1/config.tspackages/core-typings/src/Ajv.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/livechat/server/api/v1/webhooks.tsapps/meteor/app/livechat/server/api/v1/config.tspackages/core-typings/src/Ajv.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/livechat/server/api/v1/config.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/livechat/server/api/v1/config.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/app/livechat/server/api/v1/config.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/livechat/server/api/v1/config.ts
🔇 Additional comments (1)
packages/core-typings/src/Ajv.ts (1)
7-10: Registers the new livechat$reftargets correctly.Adding
IOmnichannelRoomandILivechatAgenthere keeps the migrated response schemas resolvable through#/components/schemas/....Also applies to: 18-18
NAME-ASHWANIYADAV
force-pushed
the
chore/migrate-livechat-omnichannel-openapi
branch
from
fd73ca7 to
c4d69f4
Compare
|
Hey @ahmed-n-abdeltwab, all your comments have been addressed! Also fixed CodeRabbit's suggestions (added guest field to response schema, fixed debug log security, and registered ILivechatVisitor, IOmnichannelRoom, ILivechatAgent in the typia schema registry). Would appreciate a re review when you get the time! 🙏 |
|
@ahmed-n-abdeltwab sir what is the the status of this pr is their anything else i need to do ?? |
NAME-ASHWANIYADAV
force-pushed
the
chore/migrate-livechat-omnichannel-openapi
branch
from
c4d69f4 to
1db126d
Compare
ahmed-n-abdeltwab
left a comment
ahmed-n-abdeltwab
left a comment
There was a problem hiding this comment.
maybe also you need to testapi locally and provide gif record for swagger ui testing these APIs
ok i will provides gif records asap |
NAME-ASHWANIYADAV
force-pushed
the
chore/migrate-livechat-omnichannel-openapi
branch
from
a38a1ea to
02b71a0
Compare
Swagger UI Testing Results 🧪Tested all 3 migrated endpoints locally via Swagger UI at ✅ GET /api/v1/livechat/config
✅ GET /api/v1/livechat/integrations.settings
---
✅ POST /api/v1/livechat/webhook.test
---
All 3 endpoints are working correctly via Swagger UI. OpenAPI spec is properly generated with response schemas (200, 401, 403). @ahmed-n-abdeltwab @ggazzo Could you please review and merge when you get a chance? 🙏 Thanks! |
NAME-ASHWANIYADAV
force-pushed
the
chore/migrate-livechat-omnichannel-openapi
branch
from
49a7aa6 to
73b247f
Compare
There was a problem hiding this comment.
You are still missing some steps that I mentioned in the chat group. Take a look here: https://open.rocket.chat/channel/idea-Replace-old-REST-API-definitions-over-the-new-API?msg=em7y5dW3uQkEKvCqE
Hint: There seems to be an issue with the AJV JSON schema. Please read my last comment and double-check your changes. You can also review this PR as an example #36916
|
ok i will update it |
|
@ahmed-n-abdeltwab, I found the root cause of the AJV $ref issue — the livechat files load before api/server/ajv.ts |
|
Please follow the project steps and then test the API using |
|
@ahmed-n-abdeltwab The livechat endpoint files in apps/meteor/app/livechat/ load before api/server/ajv.ts registers the schemas, so I'm getting MissingRefError with $ref. Should I just register the schemas inline before ajv.compile(), or is there a better way to handle endpoints that live outside api/server/v1/? |
|
@ahmed-n-abdeltwab — Whenever you see this message, I would appreciate a reply before I proceed with implementing any solution. |
|
You should take a look at the action throwing the error; it reveals a lot about the problem and why the migration is failing. I suggest you investigate that error first and fix it. It is likely that you forgot to add props or included something that shouldn't be there. Since we made the action strict, it must exactly match the old register endpoint from rest-typing. for the AJV that's worry you, I think your problem will be solved if you add the missing interface to the typia schema generator Additionally, you’ll likely encounter issues with sdklegacy when using livechat/config. You should fix that after resolving the action error |
|
@ahmed-n-abdeltwab Thanks for the detailed review and guidance — i went through everything carefully from pr #150 description to group chats and through the comments on this pr and tackled each point one by one. |
Unfortunately, I don't have permission to update the branch myself, but if you add the ISetting to Typia, it will work. Additionally, I never said Swagger UI is our indicator for a completed migration. It's an achievement we received after migration. We run testapi and also make sure the action doesn't throw error that's what's tell if the API migrated correctly or not |
|
ok i will implement the changes in typia and will run testapi command and update here |
… in integrations.ts
|
@ahmed-n-abdeltwab |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/livechat/imports/server/rest/integrations.ts">
<violation number="1" location="apps/meteor/app/livechat/imports/server/rest/integrations.ts:12">
P1: Module-level AJV schema registration for `ISetting` can conflict with global AJV bootstrap registration, causing duplicate schema-key startup failures.</violation>
</file>
<file name="apps/meteor/app/livechat/server/api/v1/config.ts">
<violation number="1" location="apps/meteor/app/livechat/server/api/v1/config.ts:98">
P1: `GET livechat/config` dropped `config.guest` from the response, breaking an existing endpoint contract used by tests/consumers.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@NAME-ASHWANIYADAV I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/livechat/server/api/v1/config.ts">
<violation number="1" location="apps/meteor/app/livechat/server/api/v1/config.ts:57">
P2: `livechat/config` response typing is too narrow (`string | boolean` index signature) compared to runtime/schema (`additionalProperties: true` + object/array/number values), causing incorrect exported REST typings.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@ahmed-n-abdeltwab @ggazzo @cardoso I've addressed all feedback and the recent AI bot suggestions. The ISetting omnichannel.ts |
2 participants
Changes
livechat/configGET endpoint fromAPI.v1.addRouteto chained.get()pattern, chained with existinglivechat/config/routinglivechat/webhook.testPOST endpoint fromAPI.v1.addRouteto chained.post()patternlivechat/integrations.settingsGET endpoint fromAPI.v1.addRouteto chained.get()patternrest-typings/omnichannel.ts(now auto-inferred viaExtractRoutesFromAPI)Part of
Part of the ongoing REST API migration effort: RocketChat/Rocket.Chat-Open-API#150
Summary by CodeRabbit
New Features
Bug Fixes
Refactor