feat: Add OpenAPI Support to custom-user-status.list API #36916
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: 82dbc07 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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 Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughConverts the custom-user-status.list route to a typed API.v1.get endpoint with AJV request/response schemas, exports its endpoint types, adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant API as API.v1 (custom-user-status.list)
participant V as AJV Validator
participant S as action()
participant DB as CustomUserStatus Model
rect rgb(236, 246, 255)
note over API,V: Request validation (CustomUserStatusListSchema)
C->>API: GET /v1/custom-user-status.list?name&_id&offset&count
API->>V: validate(params)
V-->>API: valid / invalid
alt invalid
API-->>C: 400 Bad Request (validated)
else valid
API->>S: action(params)
S->>S: compute pagination (getPaginationItems)
S->>DB: findPaginated(filter, sort, skip, limit)
DB-->>S: { statuses, total }
S-->>API: { statuses, count, offset, total, success }
API-->>C: 200 OK (validated)
end
end
rect rgb(255, 244, 244)
note over API: Auth failure
API-->>C: 401 Unauthorized (validated)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36916 +/- ##
===========================================
+ Coverage 66.42% 66.54% +0.12%
===========================================
Files 3341 3344 +3
Lines 114284 114626 +342
Branches 21072 21149 +77
===========================================
+ Hits 75913 76279 +366
+ Misses 35686 35658 -28
- Partials 2685 2689 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the custom-user-status.list API endpoint to the new OpenAPI pattern, adding AJV-based JSON schema validation and improving API documentation with Swagger UI integration.
- Refactored the endpoint to use the new chained route definition syntax with enhanced validation
- Moved schema definitions from rest-typings to the actual endpoint implementation
- Added comprehensive response validation and error handling
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/rest-typings/src/v1/customUserStatus.ts | Removed schema definitions and validation logic that was moved to the endpoint implementation |
| packages/core-typings/src/Ajv.ts | Added IUserStatus type to the schemas array for OpenAPI support |
| apps/meteor/app/api/server/v1/custom-user-status.ts | Migrated to new OpenAPI pattern with comprehensive schema validation and type definitions |
| .changeset/sweet-terms-relax.md | Added changeset documentation for the OpenAPI migration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
cardoso
left a comment
cardoso
left a comment
There was a problem hiding this comment.
Sounds -> statuses . Finally a useful catch by copilot 😁
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/rest-typings/src/v1/customUserStatus.ts (1)
12-17: Contract mismatch — alignnamebetween server and typingsServer route "custom-user-status.update" enforces name: String (apps/meteor/app/api/server/v1/custom-user-status.ts) while rest-typings mark it optional (packages/rest-typings/src/v1/customUserStatus.ts). Align them.
Preferred fix (allow partial updates): change the server validation to make name optional:
check(this.bodyParams, {
_id: String,
name: Match.Maybe(String),
statusType: Match.Maybe(String),
});Alternate: make typings require name (remove the
?) if updates must always include name.apps/meteor/app/api/server/v1/custom-user-status.ts (1)
175-185: Makenameoptional on update to match public typings.
Typings marknameoptional; server currently requires it. Allow partial updates.- check(this.bodyParams, { - _id: String, - name: String, - statusType: Match.Maybe(String), - }); + check(this.bodyParams, { + _id: String, + name: Match.Maybe(String), + statusType: Match.Maybe(String), + }); @@ - const userStatusData = { - _id: this.bodyParams._id, - name: this.bodyParams.name, - statusType: this.bodyParams.statusType, - }; + const userStatusData: { _id: string; name?: string; statusType?: string } = { + _id: this.bodyParams._id, + ...(this.bodyParams.name !== undefined ? { name: this.bodyParams.name } : {}), + ...(this.bodyParams.statusType !== undefined ? { statusType: this.bodyParams.statusType } : {}), + };
♻️ Duplicate comments (1)
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
71-81: Replace “sounds” with “custom user statuses” in descriptions.
This was noted previously; apply the wording fix.- count: { - type: 'number', - description: 'The number of sounds returned in this response.', - }, + count: { type: 'number', description: 'The number of custom user statuses returned in this response.' }, offset: { type: 'number', - description: 'The number of sounds that were skipped in this response.', + description: 'The number of custom user statuses that were skipped in this response.', }, total: { type: 'number', - description: 'The total number of sounds that match the query.', + description: 'The total number of custom user statuses that match the query.', },
🧹 Nitpick comments (3)
.changeset/sweet-terms-relax.md (1)
7-7: Polish the changeset summary for clarity and brevity.
Reads a bit long and misses a period. Suggest tightening and explicitly naming the endpoint path.-Add OpenAPI support for the Rocket.Chat custom-user-status.list API endpoints by migrating to a modern chained route definition syntax and utilizing shared AJV schemas for validation to enhance API documentation and ensure type safety through response validation +Add OpenAPI support for /v1/custom-user-status.list by migrating to chained route definitions and shared AJV schemas, improving docs and ensuring request/response type safety.apps/meteor/app/api/server/v1/custom-user-status.ts (2)
20-47: Query param coercion: ensure Ajv is configured or widen schema.
count/offsetarrive as strings via query params. Ifajvis not usingcoerceTypes: true, validation will fail. Either confirm coercion is enabled in the shared ajv instance or accept string-or-number here.- count: { - type: 'number', - nullable: true, - }, - offset: { - type: 'number', - nullable: true, - }, + count: { oneOf: [{ type: 'number' }, { type: 'string' }], nullable: true }, + offset: { oneOf: [{ type: 'number' }, { type: 'string' }], nullable: true },
158-161: Nit: fix error message grammar.- return API.v1.failure('The "customUserStatusId" params is required!'); + return API.v1.failure('The "customUserStatusId" parameter is required!');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/sweet-terms-relax.md(1 hunks)apps/meteor/app/api/server/v1/custom-user-status.ts(2 hunks)packages/core-typings/src/Ajv.ts(1 hunks)packages/rest-typings/src/v1/customUserStatus.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/custom-user-status.ts (3)
packages/rest-typings/src/v1/Ajv.ts (3)
ajv(23-23)validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(71-75)packages/rest-typings/src/index.ts (1)
Endpoints(52-100)
🔇 Additional comments (2)
apps/meteor/app/api/server/v1/custom-user-status.ts (2)
209-214: Type export pattern looks good.
TheExtractRoutesFromAPI-based augmentation cleanly adds the list endpoint to public typings at build-time.
101-106: Incorrect — parseJsonQuery already sanitizes $-operatorsparseJsonQuery (apps/meteor/app/api/server/helpers/parseJsonQuery.ts) calls clean(...) from apps/meteor/app/api/server/lib/cleanQuery.ts which removes keys starting with '$' unless they’re in the allow-list (defaults include ['$or','$and','$regex']) and validates the query, so merging ...query here does not allow arbitrary operator injection.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/app/api/server/v1/custom-user-status.ts (1)
70-81: Fixed “sounds” -> “custom user statuses” descriptions.Accuracy issue from earlier review is resolved.
🧹 Nitpick comments (3)
apps/meteor/app/api/server/v1/custom-user-status.ts (3)
17-47: Use integer and non-negative constraints for pagination params.count/offset are integral and should be >= 0. Tighten validation.
count: { - type: 'number', + type: 'integer', + minimum: 0, nullable: true, }, offset: { - type: 'number', + type: 'integer', + minimum: 0, nullable: true, },
57-91: 200-schema alignment with ICustomUserStatus looks good; add minItems.Add minItems: 0 to statuses array for completeness.
statuses: { type: 'array', + minItems: 0, items: { $ref: '#/components/schemas/ICustomUserStatus', }, },
209-214: Avoid type-name collision with rest-typings’ CustomUserStatusEndpoints.This file exports CustomUserStatusEndpoints, which likely duplicates the name from rest-typings and can confuse imports/IDE tooling. Prefer a distinct local name.
-export type CustomUserStatusEndpoints = ExtractRoutesFromAPI<typeof customUserStatusEndpoints>; +export type CustomUserStatusListEndpoints = ExtractRoutesFromAPI<typeof customUserStatusEndpoints>; declare module '@rocket.chat/rest-typings' { // eslint-disable-next-line @typescript-eslint/naming-convention, @typescript-eslint/no-empty-interface - interface Endpoints extends CustomUserStatusEndpoints {} + interface Endpoints extends CustomUserStatusListEndpoints {} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/api/server/v1/custom-user-status.ts(2 hunks)packages/core-typings/src/Ajv.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core-typings/src/Ajv.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/custom-user-status.ts (4)
packages/rest-typings/src/v1/Ajv.ts (3)
ajv(23-23)validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)packages/models/src/index.ts (1)
CustomUserStatus(150-150)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(71-75)packages/rest-typings/src/index.ts (1)
Endpoints(52-100)
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/app/api/server/v1/custom-user-status.ts (5)
1-4: Resolved type mismatch and imports look correct.Good switch to ICustomUserStatus and proper AJV/typing utilities; this addresses prior “IUserStatus”/schema refs issues.
Also applies to: 11-11
107-114: LGTM on findPaginated usage and concurrent fetch.Correct default sort and concurrent cursor/total retrieval.
115-121: Response shaping matches the declared 200 schema.count/offset/total/success all align.
49-56: AJV coerceTypes is enabled for the custom-user-status validator — no change required.packages/rest-typings/src/v1/customUserStatus.ts constructs Ajv({ coerceTypes: true }) and compiles the validator used by the endpoint, so query params like ?count=50 (string) will be coerced to numbers. (useDefaults is not enabled on that Ajv instance.)
95-106: No change required — parseJsonQuery returns a cleaned object and pagination enforces limits.
- parseJsonQuery initializes query = {} and runs clean(...), which returns an object ({} fallback for non-objects), so spreading query is safe. (apps/meteor/app/api/server/helpers/parseJsonQuery.ts, apps/meteor/app/api/server/lib/cleanQuery.ts)
- getPaginationItems clamps count to API_Upper_Count_Limit (fallback 100) and uses API_Default_Count (fallback 50); infinite count is only allowed when API_Allow_Infinite_Count=true. (apps/meteor/app/api/server/helpers/getPaginationItems.ts)
2 participants
Description:
This PR integrates OpenAPI support into the
Rocket.Chat API, migrate ofRocket.Chat APIendpoints to the new OpenAPI pattern. The update includes improved API documentation, enhanced type safety, and response validation using AJV.Key Changes:
Issue Reference:
Relates to #34983, part of the ongoing OpenAPI integration effort.
Testing:
Verified that the API response schemas are correctly documented in Swagger UI.
All tests passed without any breaking changes
Endpoints:
Looking forward to your feedback! 🚀
Summary by CodeRabbit