feat: Updates DeepL auto translate provider to get supported languages from API #37250
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: cd8d707 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 |
WalkthroughThe PR updates the DeepL AutoTranslate integration to fetch supported languages dynamically from the DeepL API at runtime instead of using hardcoded values, with automatic conversion to BCP-47 format and per-target caching. Changes
Sequence DiagramsequenceDiagram
participant Translation as Translation Request
participant AutoTrans as DeeplAutoTranslate
participant Cache as Language Cache
participant API as DeepL API
Translation->>AutoTrans: requestTranslation(target)
AutoTrans->>Cache: check cached languages?
alt Cache miss
AutoTrans->>API: fetch languages (Authorization header)
API-->>AutoTrans: language list (JSON)
AutoTrans->>AutoTrans: transform to BCP-47 format<br/>(Intl.Locale)
AutoTrans->>Cache: store formatted languages
else Cache hit
Cache-->>AutoTrans: return cached languages
end
AutoTrans->>AutoTrans: normalize target language
AutoTrans-->>Translation: proceed with translation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The change introduces dynamic API fetching with error handling and language transformation logic that replaces static values. While focused to a single file, it involves new runtime dependencies, API integration, and BCP-47 format conversion that warrant careful review of correctness and edge cases. Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-17T12:36:01.020ZApplied to files:
⏰ 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)
🔇 Additional comments (3)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
.changeset/wild-impalas-sip.md(1 hunks)apps/meteor/app/autotranslate/server/autotranslate.ts(2 hunks)apps/meteor/app/autotranslate/server/bcp47Mapping.ts(1 hunks)apps/meteor/app/autotranslate/server/deeplTranslate.ts(1 hunks)apps/meteor/package.json(1 hunks)packages/core-typings/src/IAutoTranslate.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/app/autotranslate/server/autotranslate.ts (1)
apps/meteor/app/autotranslate/server/bcp47Mapping.ts (1)
bcp47Mapping(1-201)
apps/meteor/app/autotranslate/server/deeplTranslate.ts (1)
packages/core-typings/src/IAutoTranslate.ts (1)
ISupportedLanguage(18-22)
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37250 +/- ##
===========================================
- Coverage 67.60% 67.57% -0.04%
===========================================
Files 3338 3338
Lines 113721 113712 -9
Branches 20660 20736 +76
===========================================
- Hits 76879 76837 -42
- Misses 34158 34196 +38
+ Partials 2684 2679 -5
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.
I'm wondering if we really need this mapping, I'm thinking just the results from the provider is enough for our need
I would love your thoughts here @tassoevan
In our discussion, someone mentioned that in few cases, the direct values from the api didn't work. So, as a safeguard I created a mapping with default subregions. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/autotranslate/server/deeplTranslate.ts (1)
90-91: Update outdated comment.The comment states "Deepl does not provide an endpoint yet to retrieve the supported languages," but this implementation now fetches languages dynamically from the DeepL API.
Apply this diff to update the comment:
/** * Returns supported languages for translation by the active service provider. - * Deepl does not provide an endpoint yet to retrieve the supported languages. - * So each supported languages are explicitly maintained. + * Fetches supported target languages dynamically from the DeepL API. * @private implements super abstract method. * @param {string} target * @returns {object} code : value pair */
🧹 Nitpick comments (1)
apps/meteor/app/autotranslate/server/deeplTranslate.ts (1)
119-121: Remove redundant cache check.The cache is already checked at lines 101-103 before the fetch. This second check after the fetch (lines 119-121) appears redundant unless there's a concern about concurrent requests, which is not evident in the current implementation.
Consider removing the redundant check:
result = (await request.json()) as typeof result; - if (this.supportedLanguages[target]) { - return this.supportedLanguages[target]; - } this.supportedLanguages[target || 'en'] = result.map(({ language, ...other }) => ({ ...other, language: new Intl.Locale(language).toString(), })); return this.supportedLanguages[target || 'en'];
📜 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 (1)
apps/meteor/app/autotranslate/server/deeplTranslate.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/autotranslate/server/deeplTranslate.ts (1)
packages/core-typings/src/IAutoTranslate.ts (1)
ISupportedLanguage(18-22)
⏰ 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: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/autotranslate/server/deeplTranslate.ts (2)
37-37: LGTM!The addition of a dedicated field for the supported languages endpoint URL is a clean approach to handle the free vs pro API endpoint distinction.
55-55: LGTM!The endpoint URL initialization correctly follows the existing pattern of distinguishing between free and pro API keys.
Also applies to: 59-59
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/app/autotranslate/server/deeplTranslate.ts (1)
121-125: Fix the language field mapping to preserve original codes and properly generate BCP-47.This implementation has two critical issues:
Overwrites the original language code: The mapping replaces the
languagefield with the result ofnew Intl.Locale(language).toString(). According to theISupportedLanguageinterface, thelanguagefield should contain the original ISO 639-1 code (e.g., "en", "pt"), andbcp47should be a separate optional field.Incorrect BCP-47 generation:
new Intl.Locale(language).toString()only canonicalizes input—it does NOT add regional subtags. For example,new Intl.Locale("en").toString()returns"en", not"en-US". The PR objectives and comments indicate a mapping file with default subregions should be used for proper BCP-47 codes.Based on learnings
You need to:
- Preserve the original
languagefield from the API response- Use the BCP-47 mapping helper (mentioned in AI summary but not visible in this file) to generate proper BCP-47 codes with regional subtags
Apply this structural fix (you'll need to import and use the actual BCP-47 mapping helper):
-this.supportedLanguages[target || 'en'] = result.map(({ language, ...other }) => ({ - ...other, - language: new Intl.Locale(language).toString(), -})); +this.supportedLanguages[target || 'en'] = result.map(({ language, ...other }) => ({ + ...other, + language, + bcp47: /* Use proper BCP-47 mapping helper here, e.g., getBcp47ForLanguage(language) */, +}));
🧹 Nitpick comments (2)
apps/meteor/app/autotranslate/server/deeplTranslate.ts (2)
112-114: Consider logging the error before throwing.The error is thrown without logging details. Per past feedback, DeepL API error responses are safe to log and can aid debugging.
Consider this enhancement:
if (!request.ok) { + const errorText = await request.text(); + SystemLogger.error({ msg: 'Failed to fetch supported languages from DeepL', status: request.status, error: errorText }); - throw new Error('Failed to fetch supported languages'); + throw new Error(`Failed to fetch supported languages: ${request.status}`); }Based on learnings
118-120: Redundant cache check after fetch.The cache is already checked at lines 101-103 before the fetch. This second check appears redundant unless it's intended to handle race conditions—but the current implementation isn't thread-safe anyway.
Consider removing the duplicate check:
result = (await request.json()) as typeof result; -if (this.supportedLanguages[target]) { - return this.supportedLanguages[target]; -} this.supportedLanguages[target || 'en'] = result.map(({ language, ...other }) => ({
📜 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 (1)
apps/meteor/app/autotranslate/server/deeplTranslate.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T12:36:00.993Z
Learnt from: tassoevan
PR: RocketChat/Rocket.Chat#37250
File: apps/meteor/app/autotranslate/server/deeplTranslate.ts:122-126
Timestamp: 2025-10-17T12:36:00.993Z
Learning: In JavaScript, `new Intl.Locale(language).toString()` returns a BCP-47 compliant identifier but only canonicalizes the input—it does not add regional subtags. For example, `new Intl.Locale("en").toString()` returns `"en"`, not `"en-US"`. To map base language codes to full BCP-47 codes with regional subtags (e.g., "en" → "en-US"), use explicit mapping or a dedicated helper function.
Applied to files:
apps/meteor/app/autotranslate/server/deeplTranslate.ts
🧬 Code graph analysis (1)
apps/meteor/app/autotranslate/server/deeplTranslate.ts (1)
packages/core-typings/src/IAutoTranslate.ts (1)
ISupportedLanguage(18-22)
⏰ 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). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/app/autotranslate/server/deeplTranslate.ts (2)
37-37: LGTM!The private field declaration follows good encapsulation practices and is properly typed.
55-60: LGTM!The endpoint URL initialization correctly distinguishes between free and paid API keys and sets the appropriate DeepL API endpoint for language queries.
scuciatto
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
…om DeepL API Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com>
abhinavkrin
force-pushed
the
feat/auto-translate-supported-languages-bcp47
branch
from
05ae870 to
a5b7e7a
Compare
dionisio-bot
bot
removed
the
stat: ready to merge
abhinavkrin
changed the title
dougfabris
changed the title
dougfabris
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
dougfabris
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
kodiakhq
bot
deleted the
feat/auto-translate-supported-languages-bcp47
branch
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
CORE-1319
Summary by CodeRabbit