fix(whatsnew): defeat locale race by threading tag explicitly to loader#531
Merged
rainxchzed merged 1 commit intomainfrom May 6, 2026
Merged
fix(whatsnew): defeat locale race by threading tag explicitly to loader#531rainxchzed merged 1 commit intomainfrom
rainxchzed merged 1 commit intomainfrom
Conversation
…e with MainActivity setActiveLanguageTag
Contributor
WalkthroughThe PR adds language-tag-aware loading to the What's New feature across the domain interface, implementation, and UI layer. The interface gains optional languageTag parameters, the implementation builds language-specific file paths, and the ViewModel observes language changes to reload What's New entries in the current language. ChangesLanguage-Aware What's New Loading
Sequence DiagramsequenceDiagram
participant VM as WhatsNewViewModel
participant LM as LocalizationManager
participant Loader as WhatsNewLoaderImpl
participant FS as File System
LM->>VM: 🔔 App language changed
VM->>VM: Update lastLanguageTag
VM->>Loader: loadAll(languageTag)
Loader->>Loader: candidatePaths(versionCode, languageTag)
Loader->>FS: 📂 Check language-specific files
FS-->>Loader: Load entry
Loader-->>VM: List<WhatsNewEntry>
VM->>VM: Update history state
VM->>Loader: forVersionCode(current, languageTag)
Loader->>Loader: loadOrNull with language paths
Loader->>FS: 📂 Check language-specific file
FS-->>Loader: Entry or null
Loader-->>VM: WhatsNewEntry?
VM->>VM: Evaluate pending state
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt (1)
80-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
evaluate()doesn't clear_pendingEntryon the early-return path during a language swap.Scenario: user is on the upgrade flow (
lastSeen < current) and the sheet is currently shown (_pendingEntryset with old-language content). Language changes →reloadPending→evaluate(newTag). If the new language resolves toentry == null(e.g. translation missing and no fallback hit) or!entry.showAsSheet, the function advanceslastSeenWhatsNewVersionCodeand returns without clearing_pendingEntry. The stale-language sheet stays visible, and becauselastSeenwas just bumped, nothing will re-appear after dismissal/relaunch.🛡️ Proposed fix
val entry = whatsNewLoader.forVersionCode(current, languageTag) if (entry == null || !entry.showAsSheet) { + _pendingEntry.value = null tweaksRepository.setLastSeenWhatsNewVersionCode(current) return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt` around lines 80 - 93, evaluate() can leave a stale _pendingEntry when a language swap causes entry == null or !entry.showAsSheet but you still advance the last-seen version; update evaluate (the suspend function) so that before calling tweaksRepository.setLastSeenWhatsNewVersionCode(current) in those early-return branches you also clear _pendingEntry.value (or set it to null) to ensure the sheet is dismissed; touch the branch that checks entry == null || !entry.showAsSheet and call _pendingEntry.value = null (or equivalent) immediately before invoking tweaksRepository.setLastSeenWhatsNewVersionCode(current) so the UI no longer shows stale-language content after a language change.
🧹 Nitpick comments (1)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt (1)
46-59: ⚡ Quick winCatching
ThrowableswallowsCancellationExceptionon VM teardown.When
viewModelScopeis cancelled, the suspendingcollectraisesCancellationException, which will be caught and logged as an error. This produces noisy log lines on every VM teardown and breaks structured-cancellation propagation. The same pattern appears inreloadHistory(line 67),evaluate(line 75),markSeen(line 101), andforceShowLatest(line 117).Rethrow
CancellationExceptionto preserve cancellation semantics:Suggested fix
+import kotlinx.coroutines.CancellationException @@ - } catch (t: Throwable) { - logger.e(t) { "Failed to observe app-language for what's-new reloads" } - } + } catch (t: CancellationException) { + throw t + } catch (t: Throwable) { + logger.e(t) { "Failed to observe app-language for what's-new reloads" } + }Apply the same pattern to the other four catch blocks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt` around lines 46 - 59, The catch blocks that currently catch Throwable (e.g., in the viewModelScope.launch observer block around tweaksRepository.getAppLanguage().collect and the other methods reloadHistory, evaluate, markSeen, forceShowLatest) should preserve coroutine cancellation: change each catch to rethrow CancellationException (or any throwable that is CancellationException) and only log non-cancellation failures; e.g., in the catch(t: Throwable) for the collect observer, if (t is CancellationException) throw t else logger.e(t) { "...message..." }; apply the identical pattern to reloadHistory, evaluate, markSeen, and forceShowLatest so cancellation propagates correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt`:
- Around line 80-93: evaluate() can leave a stale _pendingEntry when a language
swap causes entry == null or !entry.showAsSheet but you still advance the
last-seen version; update evaluate (the suspend function) so that before calling
tweaksRepository.setLastSeenWhatsNewVersionCode(current) in those early-return
branches you also clear _pendingEntry.value (or set it to null) to ensure the
sheet is dismissed; touch the branch that checks entry == null ||
!entry.showAsSheet and call _pendingEntry.value = null (or equivalent)
immediately before invoking
tweaksRepository.setLastSeenWhatsNewVersionCode(current) so the UI no longer
shows stale-language content after a language change.
---
Nitpick comments:
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt`:
- Around line 46-59: The catch blocks that currently catch Throwable (e.g., in
the viewModelScope.launch observer block around
tweaksRepository.getAppLanguage().collect and the other methods reloadHistory,
evaluate, markSeen, forceShowLatest) should preserve coroutine cancellation:
change each catch to rethrow CancellationException (or any throwable that is
CancellationException) and only log non-cancellation failures; e.g., in the
catch(t: Throwable) for the collect observer, if (t is CancellationException)
throw t else logger.e(t) { "...message..." }; apply the identical pattern to
reloadHistory, evaluate, markSeen, and forceShowLatest so cancellation
propagates correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07ea1f3f-aa43-4db0-81d3-b59ed5802722
📒 Files selected for processing (3)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewLoaderImpl.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/WhatsNewLoader.kt
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.
Bug
After #526 the what's-new sheet/history still showed stale-language content when switching app language. Reporter confirmed it's reproducible on the merged build.
Root cause
`WhatsNewLoaderImpl` resolved the locale via `localizationManager.getCurrentLanguageCode()` → `Locale.getDefault()`. That's a mutable global updated by `MainActivity.setActiveLanguageTag()`.
Both the WhatsNewVM and MainActivity subscribe to the same `tweaksRepository.getAppLanguage()` flow. No ordering guarantee between subscribers. When the VM's collector fired first, the loader read `Locale.getDefault()` before MainActivity had a chance to call `setActiveLanguageTag()` → loader resolved the previous locale → stale content cached. `recreate()` afterwards preserved the ViewModelStore + the stale cache.
Fix
Thread the BCP-47 tag explicitly from the flow value into `WhatsNewLoader`:
Net result: the loader's resolved locale comes from the same flow value that's about to update the global `Locale.getDefault()`, so the order between subscribers no longer matters.
Test plan
Summary by CodeRabbit