chore: Add OpenAPI Support to emoji-custom.create API#36523
chore: Add OpenAPI Support to emoji-custom.create API#36523ggazzo merged 13 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: fe3f8f2 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 |
|
hey @cardoso the POST method lacks support for handling multipart/form-data payloads |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36523 +/- ##
===========================================
- Coverage 70.65% 70.62% -0.04%
===========================================
Files 3189 3189
Lines 112715 112715
Branches 20438 20452 +14
===========================================
- Hits 79642 79604 -38
- Misses 31028 31063 +35
- Partials 2045 2048 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@ahmed-n-abdeltwab can you fix that in this PR? |
ahmed-n-abdeltwab
force-pushed
the
feat/openapi-emoji-custom-create
branch
from
121790a to
9eb457f
Compare
WalkthroughAdds OpenAPI/AJV-backed handling for the emoji-custom.create route using chained route syntax, exposes the HTTP Request on handler Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API_Router as "API Router"
participant Handler as "emoji-custom.create Handler"
participant Validator as "Validator (AJV)"
participant Emoji_Service as "Emoji Service"
participant Storage_DB as "Storage/DB"
Client->>API_Router: POST /v1/emoji-custom.create (payload)
API_Router->>Handler: Invoke (handler.this.request available)
Handler->>Validator: Validate request & response schema
alt Valid
Handler->>Emoji_Service: insertOrUpdateEmoji(metadata)
Emoji_Service->>Storage_DB: Upsert metadata
Storage_DB-->>Emoji_Service: OK
Handler->>Emoji_Service: uploadEmojiCustomWithBuffer(file buffer)
Emoji_Service->>Storage_DB: Store asset
Storage_DB-->>Emoji_Service: OK
Emoji_Service-->>Handler: Result
Handler-->>API_Router: 200 { structured response }
API_Router-->>Client: 200
else Invalid or Error
Handler-->>API_Router: 400 { failure payload }
API_Router-->>Client: 400
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (2)
apps/meteor/app/api/server/v1/emoji-custom.ts (2)
158-171: Consider including error details in the failure response.The catch block logs the error but returns a generic
API.v1.failure()without any error message. This makes debugging difficult for API consumers. Consider passing error details to the failure response.Apply this diff to include error context:
} catch (e) { SystemLogger.error(e); - return API.v1.failure(); + return API.v1.failure(e instanceof Error ? e.message : 'Failed to create emoji'); }
252-254: Consider the purpose of the EmojiCustomEndpoints alias.The
EmojiCustomEndpointstype is an alias toEmojiCustomCreateEndpoints, which only includes the create endpoint. This naming might be misleading since it suggests it covers all emoji-custom endpoints when it only covers one.If the intent is to eventually include all emoji-custom endpoints in this type, consider:
- Using a more specific name like
EmojiCustomCreateEndpoint(singular), or- Documenting the planned expansion, or
- Directly exporting
EmojiCustomCreateEndpointsif the alias doesn't add clarity.
📜 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/weak-terms-shave.md(1 hunks)apps/meteor/app/api/server/definition.ts(1 hunks)apps/meteor/app/api/server/v1/emoji-custom.ts(4 hunks)packages/rest-typings/src/v1/emojiCustom.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/rest-typings/src/v1/emojiCustom.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/emoji-custom.ts (5)
packages/rest-typings/src/v1/Ajv.ts (1)
ajv(23-23)apps/meteor/app/api/server/lib/getUploadFormData.ts (1)
getUploadFormData(55-195)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)packages/rest-typings/src/v1/emojiCustom.ts (1)
EmojiCustomEndpoints(53-73)packages/rest-typings/src/index.ts (1)
Endpoints(51-98)
🔇 Additional comments (6)
.changeset/weak-terms-shave.md (1)
1-7: LGTM!The changeset correctly documents the OpenAPI support addition as patch-level changes for both affected packages. The description accurately summarizes the enhancement.
apps/meteor/app/api/server/v1/emoji-custom.ts (4)
4-4: LGTM!The import additions for
ajvandisEmojiCustomListare appropriate for the OpenAPI schema validation implementation.
14-14: LGTM!The
ExtractRoutesFromAPIimport is correctly used for deriving route types from the endpoint specification.
138-146: Verify multipart/form-data Content-Type handling.Per the PR comments, there's a concern about POST method support for multipart/form-data payloads. While
getUploadFormDatahandles multipart parsing, verify that the OpenAPI specification documents the expectedContent-Type: multipart/form-dataheader and that no middleware blocks it.Consider adding explicit Content-Type validation or documentation in the OpenAPI schema to address the maintainer's concern about multipart/form-data handling.
256-259: LGTM!The module augmentation correctly extends the
@rocket.chat/rest-typingsEndpoints interface. The eslint-disable comments are appropriate for this declaration merging pattern.apps/meteor/app/api/server/definition.ts (1)
304-304: LGTM!Adding the
request: Requestfield toTypedThisaligns it with the existingActionThisinterface and enables typed access to the HTTP request in OpenAPI-style handlers. This change directly supports the emoji-custom.create endpoint implementation.
ggazzo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/app/api/server/v1/emoji-custom.ts (1)
111-135: 400 response schema is still missing themessageproperty.With
additionalProperties: false, anyAPI.v1.failure()response that includes amessagefield will fail AJV validation. Addmessage: { type: 'string' }to the 400 schema properties.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/emoji-custom.ts` around lines 111 - 135, The 400 response schema in emoji-custom.ts (the ajv.compile object under response: 400) disallows extra fields and is missing the message property, causing API.v1.failure() responses with a message to fail validation; update the 400 schema properties to include message: { type: 'string' } so the AJV validation accepts responses that include a message field while keeping additionalProperties: false intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/emoji-custom.ts`:
- Around line 158-171: The catch in the emoji upload handler swallows error
context (around insertOrUpdateEmoji and uploadEmojiCustomWithBuffer) by calling
API.v1.failure() with no details; change it to propagate the original error
information instead—either rethrow the caught error (throw err) so the framework
can enrich the response, or call API.v1.failure({ message: err.message ||
err.reason, errorType: err.error || 'error' }) so clients receive
message/errorType; keep SystemLogger.error({ err }) but do not suppress the
error payload from API responses.
---
Duplicate comments:
In `@apps/meteor/app/api/server/v1/emoji-custom.ts`:
- Around line 111-135: The 400 response schema in emoji-custom.ts (the
ajv.compile object under response: 400) disallows extra fields and is missing
the message property, causing API.v1.failure() responses with a message to fail
validation; update the 400 schema properties to include message: { type:
'string' } so the AJV validation accepts responses that include a message field
while keeping additionalProperties: false intact.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/api/server/definition.tsapps/meteor/app/api/server/v1/emoji-custom.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/api/server/definition.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/api/server/v1/emoji-custom.ts
🧠 Learnings (3)
📓 Common learnings
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:06.802Z
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.
📚 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/api/server/v1/emoji-custom.ts
📚 Learning: 2026-02-23T17:53:06.802Z
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:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/emoji-custom.ts
🔇 Additional comments (2)
apps/meteor/app/api/server/v1/emoji-custom.ts (2)
4-4: LGTM! Imports are clean and aligned with the new AJV schema and type-extraction patterns introduced.Also applies to: 14-14
251-259: Module augmentation pattern is well-structured.The type extraction from the route definition and merging into
@rocket.chat/rest-typingsEndpointsvia module augmentation is a clean approach for co-locating endpoint types with their implementation.
| try { | ||
| const emojiData = await insertOrUpdateEmoji(this.userId, { | ||
| ...fields, | ||
| newFile: true, | ||
| aliases: fields.aliases || '', | ||
| name: fields.name, | ||
| extension: fields.extension, | ||
| }); | ||
|
|
||
| await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, mimetype, emojiData); | ||
| } catch (err) { | ||
| SystemLogger.error({ err }); | ||
| return API.v1.failure(); | ||
| } | ||
| await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, mimetype, emojiData); | ||
| } catch (err) { | ||
| SystemLogger.error({ err }); | ||
| return API.v1.failure(); | ||
| } |
There was a problem hiding this comment.
catch block swallows error context — API consumers receive no diagnostic information.
API.v1.failure() at line 170 returns { success: false } with no message, error type, or details. If insertOrUpdateEmoji throws a meaningful Meteor.Error (e.g., duplicate emoji name), the client gets a bare failure with no way to understand what went wrong.
Compare with the emoji-custom.update endpoint (lines 226–230) that lets errors propagate to the framework, which enriches the response with error/errorType/message fields. The emoji-custom.delete endpoint (line 242) also forwards the message.
Consider forwarding the error message:
Proposed fix
} catch (err) {
SystemLogger.error({ err });
- return API.v1.failure();
+ if (err instanceof Meteor.Error) {
+ return API.v1.failure(err.message);
+ }
+ return API.v1.failure('Failed to create emoji');
}Note: if you forward error messages, the 400 response schema must also include message: { type: 'string' } in its properties (as flagged separately).
📝 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.
| try { | |
| const emojiData = await insertOrUpdateEmoji(this.userId, { | |
| ...fields, | |
| newFile: true, | |
| aliases: fields.aliases || '', | |
| name: fields.name, | |
| extension: fields.extension, | |
| }); | |
| await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, mimetype, emojiData); | |
| } catch (err) { | |
| SystemLogger.error({ err }); | |
| return API.v1.failure(); | |
| } | |
| await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, mimetype, emojiData); | |
| } catch (err) { | |
| SystemLogger.error({ err }); | |
| return API.v1.failure(); | |
| } | |
| try { | |
| const emojiData = await insertOrUpdateEmoji(this.userId, { | |
| ...fields, | |
| newFile: true, | |
| aliases: fields.aliases || '', | |
| name: fields.name, | |
| extension: fields.extension, | |
| }); | |
| await uploadEmojiCustomWithBuffer(this.userId, fileBuffer, mimetype, emojiData); | |
| } catch (err) { | |
| SystemLogger.error({ err }); | |
| if (err instanceof Meteor.Error) { | |
| return API.v1.failure(err.message); | |
| } | |
| return API.v1.failure('Failed to create emoji'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/emoji-custom.ts` around lines 158 - 171, The
catch in the emoji upload handler swallows error context (around
insertOrUpdateEmoji and uploadEmojiCustomWithBuffer) by calling API.v1.failure()
with no details; change it to propagate the original error information
instead—either rethrow the caught error (throw err) so the framework can enrich
the response, or call API.v1.failure({ message: err.message || err.reason,
errorType: err.error || 'error' }) so clients receive message/errorType; keep
SystemLogger.error({ err }) but do not suppress the error payload from API
responses.
dionisio-bot
bot
added
the
stat: ready to merge
|
/jira ARCH-1935 |
Co-authored-by: Matheus Cardoso <[email protected]> Co-authored-by: Guilherme Gazzo <[email protected]>
4 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:
Endpoints: (In progress)
Looking forward to your feedback! 🚀
Summary by CodeRabbit
Task: ARCH-1990
[COMM-144]