Conversation
There was a problem hiding this comment.
Pull request overview
Expands the app’s translation capabilities by adding multiple dedicated translation providers (DeepL, Google Cloud Translate v2, LibreTranslate) alongside the existing AI and Google Translate (Web) options, including settings UI and serialization support.
Changes:
- Added new
AppSettings.TranslateConfig.Providervariants (DeepL / GoogleCloud / LibreTranslate) and updated provider selection + persistence/serialization (incl. legacy Google provider decoding). - Refactored translation execution behind a
TranslationEngineabstraction and implemented provider-specific engines. - Updated Android/iOS/Desktop settings UI + string resources, and added unit tests for cache keys, DeepL locale mapping, and serialization round-trips.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datastore/model/AppSettings.kt | Adds new translation provider types and updates default/serial names for compatibility. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/translation/TranslationProvider.kt | Introduces TranslationEngine abstraction and implements DeepL/Google Cloud/LibreTranslate engines. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/translation/TranslationProviderCacheKey.kt | Updates provider cache keys to differentiate providers. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/settings/AiConfigPresenter.kt | Exposes new provider options and provider-specific settings fields/setters; normalizes LibreTranslate base URL. |
| app/src/main/java/dev/dimension/flare/ui/screen/settings/AiConfigScreen.kt | Adds UI to configure new providers (API keys/base URL) and provider label mapping. |
| app/src/main/res/values/strings.xml | Adds new provider-related strings (labels, fields, descriptions). |
| app/src/main/res/values-zh-rCN/strings.xml | Chinese (Simplified) translations for new provider-related strings. |
| iosApp/flare/UI/Screen/AiConfigScreen.swift | Adds provider-specific settings UI/editing flows for new providers. |
| iosApp/flare/Localizable.xcstrings | Adds new iOS localization keys for new provider UI strings. |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/settings/SettingsScreen.kt | Adds desktop settings UI for new providers and provider label mapping. |
| desktopApp/src/main/composeResources/values/strings.xml | Desktop string resources for new providers/settings. |
| desktopApp/src/main/composeResources/values-zh-rCN/strings.xml | Desktop Chinese (Simplified) strings for new providers/settings. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/datastore/model/TranslateConfigSerializationTest.kt | Tests legacy decode + round-trip serialization for new providers. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/translation/DeepLTargetLanguageTest.kt | Tests locale normalization/mapping for DeepL target language codes. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/translation/TranslationProviderCacheKeyTest.kt | Tests cache key differentiation/normalization across providers. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/database/cache/mapper/MicroblogTest.kt | Updates tests to use renamed Google provider (GoogleWeb). |
| app/src/main/java/dev/dimension/flare/ui/screen/home/HomeScreen.kt | Adjusts top-level route set computation (includes settings route). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AppSettings.TranslateConfig.Provider.GoogleWeb -> "google-web" | ||
| is AppSettings.TranslateConfig.Provider.DeepL -> "deepl:${if (usePro) "pro" else "free"}" | ||
| is AppSettings.TranslateConfig.Provider.GoogleCloud -> "google-cloud:v2" | ||
| is AppSettings.TranslateConfig.Provider.LibreTranslate -> "libretranslate:${baseUrl.trimEnd('/').lowercase()}" |
There was a problem hiding this comment.
LibreTranslate cache key lowercases the entire baseUrl. That can create collisions (URL paths can be case-sensitive) and it can also vary by device locale on JVM. Consider removing .lowercase() or normalizing only scheme/host in a locale-independent way (e.g., parse the URL and canonicalize host + trim trailing slash).
| is AppSettings.TranslateConfig.Provider.LibreTranslate -> "libretranslate:${baseUrl.trimEnd('/').lowercase()}" | |
| is AppSettings.TranslateConfig.Provider.LibreTranslate -> "libretranslate:${baseUrl.trimEnd('/')}" |
| when (this) { | ||
| AppSettings.TranslateConfig.Provider.AI -> "ai" | ||
| AppSettings.TranslateConfig.Provider.Google -> "google" | ||
| AppSettings.TranslateConfig.Provider.GoogleWeb -> "google-web" |
There was a problem hiding this comment.
Changing the Google provider cache key from the previous value to google-web will invalidate existing translation cache entries across upgrades (same provider behavior, different key). If backward-compatible caching is desired, keep the legacy key for GoogleWeb or provide an alias/migration.
| AppSettings.TranslateConfig.Provider.GoogleWeb -> "google-web" | |
| AppSettings.TranslateConfig.Provider.GoogleWeb -> "google" |
| private val httpClient: HttpClient by lazy { | ||
| ktorClient() |
There was a problem hiding this comment.
DeepLTranslationEngine is instantiated per request via engine(...), so this instance-scoped HttpClient by lazy { ktorClient() } will typically create a new HttpClient for each translation call. HttpClient creation is expensive and can leak resources if not closed; prefer reusing a shared client (e.g., singleton/object client, injected client, or a companion-level lazy client).
| private val httpClient: HttpClient by lazy { | |
| ktorClient() | |
| companion object { | |
| private val httpClient: HttpClient by lazy { | |
| ktorClient() | |
| } |
| private val httpClient: HttpClient by lazy { | ||
| ktorClient() |
There was a problem hiding this comment.
Same concern as above: GoogleCloudTranslationEngine is created per call, so its instance-scoped lazy HttpClient will usually allocate a fresh client each time. Please reuse a shared HttpClient rather than constructing per translation invocation.
| private val httpClient: HttpClient by lazy { | |
| ktorClient() | |
| private val httpClient: HttpClient | |
| get() = sharedHttpClient | |
| private companion object { | |
| val sharedHttpClient: HttpClient by lazy { | |
| ktorClient() | |
| } |
| private val httpClient: HttpClient by lazy { | ||
| ktorClient() |
There was a problem hiding this comment.
Same concern as above: LibreTranslateEngine is created per call, so this instance-scoped lazy HttpClient will typically allocate a new client per translation invocation. Consider sharing/injecting a single HttpClient for translation providers.
| private val httpClient: HttpClient by lazy { | |
| ktorClient() | |
| private companion object { | |
| val httpClient: HttpClient by lazy { | |
| ktorClient() | |
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d53b560370
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| is AppSettings.TranslateConfig.Provider.DeepL -> DeepLTranslationEngine(provider) | ||
| is AppSettings.TranslateConfig.Provider.GoogleCloud -> GoogleCloudTranslationEngine(provider) | ||
| is AppSettings.TranslateConfig.Provider.LibreTranslate -> LibreTranslateEngine(provider) |
There was a problem hiding this comment.
Reuse translation engines to avoid per-call HttpClient churn
engine() constructs a new DeepLTranslationEngine/GoogleCloudTranslationEngine/LibreTranslateEngine on every translation call. Each of those engines owns a lazy ktorClient(), and ktorClient() creates a fresh HttpClient; because these short-lived engine instances are never explicitly closed, repeated translations (especially batch pre-translation) keep allocating clients/sockets/threads unnecessarily. This introduces avoidable resource pressure and latency for every non-GoogleWeb provider request.
Useful? React with 👍 / 👎.
No description provided.