feat: enhance translation UI and caching logic#333
Merged
rainxchzed merged 5 commits intomainfrom Mar 16, 2026
Merged
Conversation
- Redesign `TranslationControls` with a pill-shaped layout, `AnimatedContent` transitions, and specific visual states for idle, translating, success, and error. - Improve `LanguagePicker` by adding a device language shortcut, search field styling, and better list item visuals. - Implement more robust translation caching in `TranslationRepositoryImpl` with a time-to-live (TTL) of 30 minutes and a more stable cache key logic. - Add `AnimatedContent` to "About" and "What's New" sections for smoother transitions when switching between original and translated text. - Update `DetailsViewModel` to manage translation jobs effectively, ensuring previous requests are cancelled when a new translation or release selection is triggered. - Add new string resources for translation error handling and language selection.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughReworks translation handling and UI: adds three new string resources, replaces a string-key cache with a typed CacheKey → CachedTranslation cache with TTL and size limits, adds cancellable translation Jobs in the ViewModel, passes deviceLanguageCode into LanguagePicker, and refactors translation UI to state-driven AnimatedContent. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant VM as DetailsViewModel
participant Repo as TranslationRepositoryImpl
participant Cache as Cache (CacheKey)
participant API as Translation API
User->>VM: Request translation (About / WhatsNew)
VM->>VM: Cancel existing translation Job (if any)
VM->>Repo: translate(text, targetLang, sourceLang)
Repo->>Cache: lookup(CacheKey)
alt hit and not expired
Cache-->>Repo: CachedTranslation
Repo-->>VM: TranslationResult
else miss or expired
Repo->>API: fetch translation
API-->>Repo: response
Repo->>Cache: store CachedTranslation (with timestamp)
Repo-->>VM: TranslationResult
end
VM->>VM: Update TranslationState -> SHOWING_TRANSLATION
VM-->>User: UI updates via AnimatedContent (fade)
alt User changes selection / cancels
User->>VM: Cancel / select new content
VM->>VM: aboutTranslationJob?.cancel() / whatsNewTranslationJob?.cancel()
VM->>VM: Reset TranslationState
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt (1)
1360-1365:⚠️ Potential issue | 🟠 MajorClear the previous translation payload before starting a new request.
Starting from
getCurrentState().copy(...)preserves the lasttranslatedTextandisShowingTranslation. If the user changes language after one successful translation and the new request fails, the old translation stays on screen andTranslationControlsnever reaches its error state becausetranslatedTextis still non-null.🐛 Proposed fix
updateState( getCurrentState().copy( isTranslating = true, + translatedText = null, + isShowingTranslation = false, + targetLanguageDisplayName = null, + detectedSourceLanguage = null, error = null, targetLanguageCode = targetLanguageCode, ), )Based on learnings: Use
TranslationStatemodel to track the translation UI state including loading, error, and success states.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt` around lines 1360 - 1365, The current updateState call in DetailsViewModel (via getCurrentState().copy(...)) preserves previous translatedText and isShowingTranslation, so a failed new translation can leave old text visible; modify the copy inside the block that sets isTranslating = true to also clear the previous translation payload by setting translatedText = null and isShowingTranslation = false (and keep error = null, targetLanguageCode unchanged) so the TranslationState reflects a fresh loading state before the new request.
🧹 Nitpick comments (1)
feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/LanguagePicker.kt (1)
64-75: Hide the shortcut language from the default list.When
searchQueryis blank,filteredLanguagesstill containsdeviceLanguage, so the same option renders twice: once as the shortcut card and once as a regular row. Filtering it out in the blank-query branch keeps the shortcut intentional and avoids double selected states.♻️ Proposed cleanup
- val filteredLanguages = - remember(searchQuery) { + val filteredLanguages = + remember(searchQuery, deviceLanguage?.code) { val all = SupportedLanguages.all if (searchQuery.isBlank()) { - all + deviceLanguage?.let { shortcut -> + all.filterNot { it.code == shortcut.code } + } ?: all } else { all.filter { it.displayName.contains(searchQuery, ignoreCase = true) || it.code.contains(searchQuery, ignoreCase = true) } } }Based on learnings: Implement
LanguagePickercomposable to allow selecting readme translation target language.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/LanguagePicker.kt` around lines 64 - 75, filteredLanguages includes deviceLanguage when searchQuery is blank, causing the shortcut card to appear twice; update the blank-query branch in the LanguagePicker logic to exclude the shortcut by filtering SupportedLanguages.all to remove deviceLanguage (e.g., use all.filter { it != deviceLanguage } or compare by code/identifier) so the shortcut remains only in the shortcut card and not in the default list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/TranslationRepositoryImpl.kt`:
- Around line 84-86: clearCache() currently calls cache.clear() without
synchronization while translate() and the access-ordered LinkedHashMap (cache)
use cacheMutex; with accessOrder=true even reads mutate the map, so wrap the
clear in the same lock. Update clearCache() to acquire cacheMutex (the same
Mutex used by translate()/cache accesses) before calling cache.clear(), or
refactor both paths to use a single non-suspending lock (e.g., a ReentrantLock)
and use that consistently for cache, ensuring all references to cache,
translate(), and clearCache() use the same locking primitive.
- Around line 41-45: The cache key built in buildCacheKey (used under cacheMutex
with cache and isExpired) must uniquely identify the translation request:
include sourceLanguage alongside targetLanguage and either the full text or a
fixed-size hash (e.g., SHA256) of the full text instead of the current
truncation/prefix-suffix/length scheme used elsewhere (lines ~201-204); update
buildCacheKey and any other cache-key construction sites to incorporate
sourceLanguage and replace the truncate/length strategy with a deterministic
hash to avoid aliasing and stale detectedSourceLanguage/translatedText returns.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/TranslationControls.kt`:
- Around line 193-198: The conditional in TranslationControls that sets the chip
text is inverted: swap the branches so the chip shows the action (not the
current state); change the logic using isShowingTranslation so that when
isShowingTranslation is true the text is
stringResource(Res.string.show_original), and when false it is displayName ?:
stringResource(Res.string.translate) (refer to the text assignment in
TranslationControls where displayName, isShowingTranslation, stringResource,
Res.string.translate and Res.string.show_original are used).
---
Outside diff comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt`:
- Around line 1360-1365: The current updateState call in DetailsViewModel (via
getCurrentState().copy(...)) preserves previous translatedText and
isShowingTranslation, so a failed new translation can leave old text visible;
modify the copy inside the block that sets isTranslating = true to also clear
the previous translation payload by setting translatedText = null and
isShowingTranslation = false (and keep error = null, targetLanguageCode
unchanged) so the TranslationState reflects a fresh loading state before the new
request.
---
Nitpick comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/LanguagePicker.kt`:
- Around line 64-75: filteredLanguages includes deviceLanguage when searchQuery
is blank, causing the shortcut card to appear twice; update the blank-query
branch in the LanguagePicker logic to exclude the shortcut by filtering
SupportedLanguages.all to remove deviceLanguage (e.g., use all.filter { it !=
deviceLanguage } or compare by code/identifier) so the shortcut remains only in
the shortcut card and not in the default list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fed5bbdf-bcb1-4abe-b4ee-f7dc88fa53bc
📒 Files selected for processing (9)
core/presentation/src/commonMain/composeResources/values/strings.xmlfeature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/TranslationRepositoryImpl.ktfeature/details/domain/src/commonMain/kotlin/zed/rainxch/details/domain/repository/TranslationRepository.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/LanguagePicker.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/TranslationControls.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/About.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/sections/WhatsNew.kt
.../data/src/commonMain/kotlin/zed/rainxch/details/data/repository/TranslationRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
.../data/src/commonMain/kotlin/zed/rainxch/details/data/repository/TranslationRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
...ion/src/commonMain/kotlin/zed/rainxch/details/presentation/components/TranslationControls.kt
Show resolved
Hide resolved
- Update `TranslationRepositoryImpl` to use a data class for `CacheKey` instead of a manual string builder, incorporating the source language into the key. - Remove `clearCache` method from the `TranslationRepository` interface and its implementation. - Refine translation button text logic to correctly toggle between "show original" and the target language display name. - Reformat `TranslationControls` and `TranslationState` for better code consistency and readability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/TranslationRepositoryImpl.kt (1)
22-24: InjectHttpClientfrom DI for consistency and proper lifecycle management.
TranslationRepositoryImplis already singleton-scoped, so resource accumulation is not a concern. However, align with the established pattern used byDetailsRepositoryImpland other repositories by injectinghttpClientfrom the DI container rather than creating it locally. This improves testability and ensures consistent lifecycle management across the codebase.Proposed direction
class TranslationRepositoryImpl( private val localizationManager: LocalizationManager, + private val httpClient: HttpClient, ) : TranslationRepository { - private val httpClient: HttpClient = createPlatformHttpClient(ProxyConfig.None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/TranslationRepositoryImpl.kt` around lines 22 - 24, TranslationRepositoryImpl currently constructs its own HttpClient via createPlatformHttpClient(ProxyConfig.None) instead of receiving one from DI; change the constructor of TranslationRepositoryImpl to accept an HttpClient parameter and use that instance instead of calling createPlatformHttpClient, mirroring the pattern used by DetailsRepositoryImpl; update any DI module/provider to bind and supply the shared HttpClient to TranslationRepositoryImpl and remove the local createPlatformHttpClient usage to ensure consistent lifecycle and testability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/TranslationControls.kt`:
- Around line 111-114: In TranslationControls (the composable in
TranslationControls.kt) the leading decorative Icon calls (e.g.,
Icon(imageVector = Icons.Default.GTranslate, ...) and the other leading icon
around line 237) duplicate the spoken label of adjacent Text, so change their
contentDescription values to null (contentDescription = null) to mark them
decorative; leave accessible labels on the surrounding Text controls unchanged
so screen readers announce the label once.
- Around line 280-287: The error state in TranslationState.controlState
currently requires translatedText == null, but DetailsViewModel only sets error
on failure and leaves translatedText stale, so ERROR never shows; fix by
clearing the previous translation when a translation fails: in the
failure-handling branch inside DetailsViewModel where you set error (the
translation retry/failure handler), also set translatedText = null (and any
related showing flags like isShowingTranslation = false if present) so
TranslationControlState.ERROR becomes reachable; alternatively remove the
translatedText == null check from TranslationState.controlState if you prefer
showing errors regardless of stale text.
---
Nitpick comments:
In
`@feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/TranslationRepositoryImpl.kt`:
- Around line 22-24: TranslationRepositoryImpl currently constructs its own
HttpClient via createPlatformHttpClient(ProxyConfig.None) instead of receiving
one from DI; change the constructor of TranslationRepositoryImpl to accept an
HttpClient parameter and use that instance instead of calling
createPlatformHttpClient, mirroring the pattern used by DetailsRepositoryImpl;
update any DI module/provider to bind and supply the shared HttpClient to
TranslationRepositoryImpl and remove the local createPlatformHttpClient usage to
ensure consistent lifecycle and testability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a97b42a2-0ead-480b-808e-61e55308bbe4
📒 Files selected for processing (2)
feature/details/data/src/commonMain/kotlin/zed/rainxch/details/data/repository/TranslationRepositoryImpl.ktfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/TranslationControls.kt
Comment on lines
+111
to
+114
| Icon( | ||
| imageVector = Icons.Default.GTranslate, | ||
| contentDescription = stringResource(Res.string.translate), | ||
| tint = MaterialTheme.colorScheme.onSurfaceVariant, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/TranslationControls.kt"Repository: OpenHub-Store/GitHub-Store
Length of output: 12451
Set contentDescription = null for decorative-leading icons adjacent to Text.
Icons at lines 113 and 237 duplicate the same spoken label already provided by adjacent Text components, causing repetitive TalkBack announcements. For decorative-leading icons in labeled controls, set contentDescription = null.
♿ Suggested fix
Icon(
imageVector = Icons.Default.GTranslate,
- contentDescription = stringResource(Res.string.translate),
+ contentDescription = null,
tint = MaterialTheme.colorScheme.onSurfaceVariant,
modifier = Modifier.size(16.dp),
)
@@
Icon(
imageVector = Icons.Default.Refresh,
- contentDescription = stringResource(Res.string.translation_error_retry),
+ contentDescription = null,
tint = MaterialTheme.colorScheme.onErrorContainer,
modifier = Modifier.size(14.dp),
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/TranslationControls.kt`
around lines 111 - 114, In TranslationControls (the composable in
TranslationControls.kt) the leading decorative Icon calls (e.g.,
Icon(imageVector = Icons.Default.GTranslate, ...) and the other leading icon
around line 237) duplicate the spoken label of adjacent Text, so change their
contentDescription values to null (contentDescription = null) to mark them
decorative; leave accessible labels on the surrounding Text controls unchanged
so screen readers announce the label once.
Comment on lines
+280
to
+287
| private val TranslationState.controlState: TranslationControlState | ||
| get() = | ||
| when { | ||
| isTranslating -> TranslationControlState.TRANSLATING | ||
| error != null && translatedText == null -> TranslationControlState.ERROR | ||
| isShowingTranslation && translatedText != null -> TranslationControlState.SHOWING_TRANSLATION | ||
| !isShowingTranslation && translatedText != null -> TranslationControlState.SHOWING_ORIGINAL | ||
| else -> TranslationControlState.IDLE |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how translation state fields are mutated across state model + viewmodel.
fd 'TranslationState.kt|DetailsViewModel.kt' feature/details | while read -r file; do
echo "===== $file ====="
rg -n -C3 '\b(isTranslating|isShowingTranslation|translatedText|error)\b' "$file"
doneRepository: OpenHub-Store/GitHub-Store
Length of output: 8488
🏁 Script executed:
cat -n feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/TranslationControls.kt | sed -n '270,300p'Repository: OpenHub-Store/GitHub-Store
Length of output: 851
🏁 Script executed:
rg -n 'isShowingTranslation|translatedText' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt -A2 -B2 | head -100Repository: OpenHub-Store/GitHub-Store
Length of output: 953
🏁 Script executed:
rg -n 'getCurrentState\(\)\.copy' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 167
🏁 Script executed:
sed -n '1360,1370p' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 452
🏁 Script executed:
sed -n '1394,1402p' feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 407
Clear stale translation text when error occurs.
The error state condition at line 284 requires translatedText == null to display, but translation failures do not clear the previous translation text (line 1397 in DetailsViewModel only sets error, not translatedText). When a retry fails after a successful translation, the stale text remains, so the ERROR state is unreachable and the error message is hidden behind the prior translation display.
Either:
- Clear
translatedText = nullwhen translation fails (line 1397) - Remove the
translatedText == nullrequirement from the error condition (line 284) so errors display regardless of stale data
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/components/TranslationControls.kt`
around lines 280 - 287, The error state in TranslationState.controlState
currently requires translatedText == null, but DetailsViewModel only sets error
on failure and leaves translatedText stale, so ERROR never shows; fix by
clearing the previous translation when a translation fails: in the
failure-handling branch inside DetailsViewModel where you set error (the
translation retry/failure handler), also set translatedText = null (and any
related showing flags like isShowingTranslation = false if present) so
TranslationControlState.ERROR becomes reachable; alternatively remove the
translatedText == null check from TranslationState.controlState if you prefer
showing errors regardless of stale text.
- Add `org.gradle.parallel=true` to `gradle.properties` to allow Gradle to execute tasks in parallel for decoupled projects.
- Create `.claude/memory/feedback_coding_boundaries.md` to establish guidelines for AI interaction. - Specify that core implementation tasks—such as architecture, state management, and bug fixes—should be handled by the user. - Define the AI's role as providing reviews, suggestions, and boilerplate code to ensure the user maintains technical ownership and skill proficiency.
- Add `animateContentSize()` to the `About` section to ensure smooth transitions during content updates - Refactor `WhatsNew` section by extracting `ExpandableMarkdownContent` into a separate Composable function for better maintainability - Ensure the `WhatsNew` markdown content properly utilizes `animateContentSize()` and `liquefiable` state - Improve layout consistency in `WhatsNew` by wrapping the expandable content in a dedicated `item` block within the list view
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TranslationControlswith a pill-shaped layout,AnimatedContenttransitions, and specific visual states for idle, translating, success, and error.LanguagePickerby adding a device language shortcut, search field styling, and better list item visuals.TranslationRepositoryImplwith a time-to-live (TTL) of 30 minutes and a more stable cache key logic.AnimatedContentto "About" and "What's New" sections for smoother transitions when switching between original and translated text.DetailsViewModelto manage translation jobs effectively, ensuring previous requests are cancelled when a new translation or release selection is triggered.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores