chore: Add OpenAPI support for the Rocket.Chat autotranslate translateMessage API#38978
Conversation
…eMessage 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
|
Looks like this PR is ready to merge! 🎉 |
WalkthroughThe changes migrate the autotranslate.translateMessage endpoint from shared rest-typings definitions to app-local definitions with OpenAPI schema support and AJV validation. The endpoint return type changes from void to IMessage | undefined, and the route registration is refactored to use body and response schema validation through a chained API definition pattern. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 |
🦋 Changeset detectedLatest commit: a0642ff 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 |
Add OpenAPI support for the Rocket.Chat autotranslate translateMessage 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38978 +/- ##
========================================
Coverage 70.64% 70.65%
========================================
Files 3189 3189
Lines 112716 112716
Branches 20413 20440 +27
========================================
+ Hits 79632 79639 +7
+ Misses 31040 31031 -9
- Partials 2044 2046 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
left a comment
ggazzo
left a comment
There was a problem hiding this comment.
For the old prs I was accepting to avoid more ci runs, but this isn't a fix, so it's a minor, not a patch.
github-project-automation
bot
moved this to Backlog
in rest api conversion to new endpoints
Add OpenAPI support for the translateMessage API endpoints by migrating to modern syntax and utilizing shared AJV schemas for validation.
| let translatedMessage; | ||
|
|
||
| if (message && room) { | ||
| await TranslationProviderRegistry.translateMessage(message, room, targetLanguage); | ||
| translatedMessage = await TranslationProviderRegistry.translateMessage(message, room, targetLanguage); | ||
| } | ||
|
|
||
| if (!translatedMessage) { | ||
| return; | ||
| } | ||
|
|
||
| return translatedMessage; |
There was a problem hiding this comment.
the bug is the function wasnt return the Message
|
github-project-automation
bot
moved this from Backlog
to Done
in rest api conversion to new endpoints
|
my bad i closed it by mistake |
… Promise<IMessage | undefined>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
@ggazzo 👍 |
![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/autotranslate/server/functions/translateMessage.ts (1)
14-25: Simplify:message &&is redundant, andletcan becomeconst.
messageis already guaranteed truthy by theif (!message?.rid)guard above (line 10), so themessage &&in the condition is dead code. Also, thelet translatedMessagewith a conditional assignment can be flattened to an early return +const, making the control flow clearer.♻️ Proposed refactor
const room = await Rooms.findOneById(message?.rid); - let translatedMessage; - if (message && room) { - translatedMessage = await TranslationProviderRegistry.translateMessage(message, room, targetLanguage); + if (!room) { + return; } - if (!translatedMessage) { + const translatedMessage = await TranslationProviderRegistry.translateMessage(message, room, targetLanguage); + + if (!translatedMessage) { return; } return translatedMessage;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/autotranslate/server/functions/translateMessage.ts` around lines 14 - 25, The conditional `message &&` is redundant because `message` is already checked earlier; replace the conditional assignment pattern by early-return and use a const for immutability: call Rooms.findOneById(message?.rid) into a const room, if no room return early, then call TranslationProviderRegistry.translateMessage(message, room, targetLanguage) into a const translatedMessage and return it (remove the `let translatedMessage` and the extra `if (message && ...)` check).apps/meteor/app/api/server/v1/autotranslate.ts (1)
124-126: RedundantmessageIdnull-guard — AJV already enforces it.The schema at line 92 marks
messageIdasrequired, and the AJV body validator (isAutotranslateTranslateMessageParamsPOST) is applied before the handler runs. By the time execution reaches line 124,messageIdis guaranteed to be a non-empty string, making this check dead code.♻️ Proposed cleanup
- if (!messageId) { - return API.v1.failure('The bodyParam "messageId" is required.'); - } - const message = await Messages.findOneById(messageId); + const message = await Messages.findOneById(messageId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/autotranslate.ts` around lines 124 - 126, Remove the redundant runtime null-check for messageId: the AJV schema validator isAutotranslateTranslateMessageParamsPOST already guarantees messageId is present (and non-empty) per the schema, so delete the if (!messageId) { return API.v1.failure(...); } block around the handler in autotranslate.ts; rely on the validated request body (messageId variable) and keep the remaining logic intact (no other changes to variable names or flow in the handler).
🤖 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/autotranslate.ts`:
- Around line 87-90: The schema property targetLanguage currently has nullable:
true but the TypeScript request type declares targetLanguage?: string (string |
undefined), causing a runtime mismatch if a client sends null; fix by making the
schema and TS type consistent: either remove nullable: true from the schema so
targetLanguage is omitted when not present (preferred to match targetLanguage?:
string), or update the TypeScript declaration (where targetLanguage is defined)
to allow null (string | null | undefined); update whichever you choose and
ensure usages of this.bodyParams.targetLanguage in autotranslate handlers handle
the chosen union.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/autotranslate.ts`:
- Around line 124-126: Remove the redundant runtime null-check for messageId:
the AJV schema validator isAutotranslateTranslateMessageParamsPOST already
guarantees messageId is present (and non-empty) per the schema, so delete the if
(!messageId) { return API.v1.failure(...); } block around the handler in
autotranslate.ts; rely on the validated request body (messageId variable) and
keep the remaining logic intact (no other changes to variable names or flow in
the handler).
In `@apps/meteor/app/autotranslate/server/functions/translateMessage.ts`:
- Around line 14-25: The conditional `message &&` is redundant because `message`
is already checked earlier; replace the conditional assignment pattern by
early-return and use a const for immutability: call
Rooms.findOneById(message?.rid) into a const room, if no room return early, then
call TranslationProviderRegistry.translateMessage(message, room, targetLanguage)
into a const translatedMessage and return it (remove the `let translatedMessage`
and the extra `if (message && ...)` check).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.changeset/hungry-monkeys-hang.mdapps/meteor/app/api/server/v1/autotranslate.tsapps/meteor/app/autotranslate/server/functions/translateMessage.tsapps/meteor/app/autotranslate/server/methods/translateMessage.tspackages/rest-typings/src/index.tspackages/rest-typings/src/v1/autoTranslate.tspackages/rest-typings/src/v1/autotranslate/AutotranslateTranslateMessageParamsPOST.ts
💤 Files with no reviewable changes (2)
- packages/rest-typings/src/index.ts
- packages/rest-typings/src/v1/autotranslate/AutotranslateTranslateMessageParamsPOST.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (3/4)
🧰 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:
packages/rest-typings/src/v1/autoTranslate.tsapps/meteor/app/api/server/v1/autotranslate.tsapps/meteor/app/autotranslate/server/functions/translateMessage.tsapps/meteor/app/autotranslate/server/methods/translateMessage.ts
🧠 Learnings (4)
📓 Common learnings
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.
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-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/autotranslate.ts
📚 Learning: 2025-11-19T12:32:29.696Z
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.
Applied to files:
.changeset/hungry-monkeys-hang.md
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
.changeset/hungry-monkeys-hang.md
🧬 Code graph analysis (1)
apps/meteor/app/autotranslate/server/functions/translateMessage.ts (1)
apps/meteor/app/autotranslate/server/autotranslate.ts (1)
TranslationProviderRegistry(30-122)
🔇 Additional comments (4)
packages/rest-typings/src/v1/autoTranslate.ts (1)
1-13: LGTM – clean removal of the translateMessage entry, consistent with the module-augmentation approach adopted inautotranslate.ts..changeset/hungry-monkeys-hang.md (1)
1-6: LGTM –minorbump for both affected packages is correct.apps/meteor/app/autotranslate/server/methods/translateMessage.ts (1)
10-10: LGTM – return type aligns correctly withtranslateMessage's updated signature.apps/meteor/app/api/server/v1/autotranslate.ts (1)
106-114: The response validator at lines 106-114 uses{ $ref: '#/components/schemas/IMessage' }, but this reference is not validated at runtime. Response validators in this codebase serve for OpenAPI schema documentation and type extraction only, not actual runtime validation. No module load failure occurs because the$refis never resolved by ajv at runtime.Likely an incorrect or invalid review comment.
| targetLanguage: { | ||
| type: 'string', | ||
| nullable: true, | ||
| }, |
There was a problem hiding this comment.
nullable: true introduces a null–undefined type mismatch.
AJV does support the nullable OpenAPI specification keyword, so the schema itself is valid. However, the TypeScript type on line 78 declares targetLanguage?: string (i.e., string | undefined), not string | null | undefined. If a client explicitly sends "targetLanguage": null, AJV will accept it, but the compiled validator's TypeScript signature will lie to downstream code (this.bodyParams.targetLanguage typed as string | undefined, but actually null at runtime).
Either align the TypeScript type:
- targetLanguage?: string;
+ targetLanguage?: string | null;or drop nullable: true and omit the field from the request body to express "no target language" (which is the more common REST convention and matches how the field is used as string | undefined).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/autotranslate.ts` around lines 87 - 90, The
schema property targetLanguage currently has nullable: true but the TypeScript
request type declares targetLanguage?: string (string | undefined), causing a
runtime mismatch if a client sends null; fix by making the schema and TS type
consistent: either remove nullable: true from the schema so targetLanguage is
omitted when not present (preferred to match targetLanguage?: string), or update
the TypeScript declaration (where targetLanguage is defined) to allow null
(string | null | undefined); update whichever you choose and ensure usages of
this.bodyParams.targetLanguage in autotranslate handlers handle the chosen
union.
There was a problem hiding this comment.
change the schema isnt it out of the scoop
There was a problem hiding this comment.
@ahmed-n-abdeltwab, this isn't out of scope—the schema and type definitions are being added in this PR (lines 76–92), so ensuring they're consistent now prevents runtime type bugs before they ship.
The mismatch is straightforward: the TypeScript type says targetLanguage?: string (undefined when absent), but nullable: true in the schema accepts null. If a client sends "targetLanguage": null, AJV passes it through but TypeScript thinks it's string | undefined.
The simplest fix is to remove nullable: true from the schema (line 90), so the field is omitted when not needed—matching both the TypeScript type and common REST conventions. That keeps the PR's scope (OpenAPI migration) while avoiding the type-safety hole.
If you prefer to handle this later or differently, just let me know.
🧠 Learnings used
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.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
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.
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.
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
|
/jira ARCH-1935 |
3 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:
Looking forward to your feedback! 🚀
Summary by CodeRabbit
New Features
Bug Fixes
Chores
COMM-144
Task: ARCH-1994