feat: Add OpenAPI Support to commands.get API #36953
Conversation
…uired command parameter, using Ajv's default error handling
|
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: bb7646b 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 |
WalkthroughRefactors GET /v1/commands.get to a typed, AJV-validated route with OpenAPI-capable chained definitions, adds shared schema-based validation and standardized error responses, updates corresponding public typings via ExtractRoutesFromAPI, adjusts an E2E test message expectation, removes the previous static endpoint typing, and adds a changeset bump. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API v1 (commands.get)
participant AJV as Validator (AJV)
participant SC as slashCommands Registry
Client->>API: GET /v1/commands.get?command=...
API->>AJV: Validate query { command }
alt Invalid query
AJV-->>API: Validation error
API-->>Client: 400 { success:false, error:"must have required property 'command'" }
else Valid query
API->>SC: Lookup command (toLowerCase)
alt Command not found
SC-->>API: undefined
API-->>Client: 400 { success:false, error:"The required \"command\" param is invalid" }
else Command found
SC-->>API: { clientOnly, command, description, params, providesPreview }
API-->>Client: 200 { success:true, command:{ ... } }
end
end
opt Unauthorized
API-->>Client: 401 { success:false, error: "unauthorized" }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.changeset/wet-roses-call.md (1)
6-6: Clarify scope and cross-reference the tracking issueNit: s/"endpoints"/"endpoint"/ if only commands.get is migrated here, and add "Refs #34983" for traceability.
Apply this diff:
-Add OpenAPI support for the Rocket.Chat commands.get 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 the Rocket.Chat commands.get API endpoint 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. Refs #34983.apps/meteor/app/api/server/v1/commands.ts (3)
65-65: Type the query paramsCast to
CommandsGetParamsto align with the validated schema and improve editor tooling.Apply this diff:
- const params = this.queryParams; + const params = this.queryParams as CommandsGetParams;
67-69: Redundant runtime check; Ajv already enforces this
query: isCommandsGetParamsrejects missing/non-stringcommandbeforeaction()runs. Drop this branch to avoid conflicting messages with schema errors.Apply this diff:
- if (typeof params.command !== 'string') { - return API.v1.failure('The query param "command" must be provided.'); - }
29-31: Naming nit: singular endpoint constIt holds a single GET route; singular reads clearer and matches usage in the exported type alias.
Apply this diff:
-const commandsEndpoints = API.v1.get( +const commandsGetEndpoint = API.v1.get(and
-export type CommandsEndpoints = ExtractRoutesFromAPI<typeof commandsEndpoints>; +export type CommandsEndpoints = ExtractRoutesFromAPI<typeof commandsGetEndpoint>;Also applies to: 385-386
apps/meteor/tests/end-to-end/api/commands.ts (1)
25-25: Avoid brittle equality on Ajv error textAjv message wording can change across versions. Assert key fragment instead.
Apply this diff:
- expect(res.body.error).to.be.equal(`must have required property 'command'`); + expect(res.body.error).to.include(`required property 'command'`);
📜 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 (4)
.changeset/wet-roses-call.md(1 hunks)apps/meteor/app/api/server/v1/commands.ts(2 hunks)apps/meteor/tests/end-to-end/api/commands.ts(1 hunks)packages/rest-typings/src/v1/commands.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rest-typings/src/v1/commands.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
PR: RocketChat/Rocket.Chat#0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/end-to-end/api/commands.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/commands.ts (2)
packages/rest-typings/src/v1/Ajv.ts (3)
ajv(23-23)validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)packages/rest-typings/src/index.ts (1)
Endpoints(52-100)
| 200: ajv.compile<{ | ||
| command: Pick<SlashCommand, 'clientOnly' | 'command' | 'description' | 'params' | 'providesPreview'>; | ||
| }>({ | ||
| type: 'object', | ||
| properties: { | ||
| command: { | ||
| type: 'object', | ||
| properties: { | ||
| clientOnly: { type: 'boolean' }, | ||
| command: { type: 'string' }, | ||
| description: { type: 'string' }, | ||
| params: { type: 'string' }, | ||
| providesPreview: { type: 'boolean' }, | ||
| }, | ||
| required: ['command', 'providesPreview'], | ||
| }, | ||
| success: { | ||
| type: 'boolean', | ||
| enum: [true], | ||
| }, | ||
| }, | ||
| required: ['command', 'success'], | ||
| additionalProperties: false, | ||
| }), | ||
| }, |
There was a problem hiding this comment.
200-response validator type omits success: true; tighten types and nested schema
The Ajv generic for 200 currently excludes success, while the schema requires it. This weakens extracted typings. Also consider freezing the nested command shape for cleaner OpenAPI docs.
Apply this diff:
- 200: ajv.compile<{
- command: Pick<SlashCommand, 'clientOnly' | 'command' | 'description' | 'params' | 'providesPreview'>;
- }>({
+ 200: ajv.compile<{
+ command: Pick<SlashCommand, 'clientOnly' | 'command' | 'description' | 'params' | 'providesPreview'>;
+ success: true;
+ }>({
type: 'object',
properties: {
command: {
type: 'object',
properties: {
clientOnly: { type: 'boolean' },
command: { type: 'string' },
description: { type: 'string' },
params: { type: 'string' },
providesPreview: { type: 'boolean' },
},
required: ['command', 'providesPreview'],
+ additionalProperties: false,
},
success: {
type: 'boolean',
enum: [true],
},
},
required: ['command', 'success'],
additionalProperties: false,
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 200: ajv.compile<{ | |
| command: Pick<SlashCommand, 'clientOnly' | 'command' | 'description' | 'params' | 'providesPreview'>; | |
| }>({ | |
| type: 'object', | |
| properties: { | |
| command: { | |
| type: 'object', | |
| properties: { | |
| clientOnly: { type: 'boolean' }, | |
| command: { type: 'string' }, | |
| description: { type: 'string' }, | |
| params: { type: 'string' }, | |
| providesPreview: { type: 'boolean' }, | |
| }, | |
| required: ['command', 'providesPreview'], | |
| }, | |
| success: { | |
| type: 'boolean', | |
| enum: [true], | |
| }, | |
| }, | |
| required: ['command', 'success'], | |
| additionalProperties: false, | |
| }), | |
| }, | |
| 200: ajv.compile<{ | |
| command: Pick<SlashCommand, 'clientOnly' | 'command' | 'description' | 'params' | 'providesPreview'>; | |
| success: true; | |
| }>({ | |
| type: 'object', | |
| properties: { | |
| command: { | |
| type: 'object', | |
| properties: { | |
| clientOnly: { type: 'boolean' }, | |
| command: { type: 'string' }, | |
| description: { type: 'string' }, | |
| params: { type: 'string' }, | |
| providesPreview: { type: 'boolean' }, | |
| }, | |
| required: ['command', 'providesPreview'], | |
| additionalProperties: false, | |
| }, | |
| success: { | |
| type: 'boolean', | |
| enum: [true], | |
| }, | |
| }, | |
| required: ['command', 'success'], | |
| additionalProperties: false, | |
| }), |
🤖 Prompt for AI Agents
In apps/meteor/app/api/server/v1/commands.ts around lines 37 to 61, the Ajv
generic for the 200 response omits the required success property and the nested
command type is not frozen; update the generic to include success: true (e.g.,
include success: true in the generic type) so the TypeScript type matches the
schema, and tighten/freeze the nested command shape (make the command property
readonly/explicitly typed or use a frozen/const-style typed structure) so the
nested command shape is exact for OpenAPI generation; ensure required fields
align between the generic and the schema and keep additionalProperties: false.
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