fix(whatsnew): re-load entries when app language changes#526
Conversation
WalkthroughWhatsNewViewModel is refactored to observe app-language changes and dynamically reload What's New history and state, rather than preloading once at startup. Coroutine flow patterns are added to subscribe to language changes, and loading logic is extracted into reusable helper functions. What's New documentation files in 13 languages are updated to document the two new fixes: backend-busy toast behavior and dynamic language switching without app restart. ChangesWhat's New Dynamic Language Switching
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt`:
- Around line 49-50: In the catch blocks that currently use "catch (t:
Throwable)" (the ones that log via logger.e(t) { "Failed to observe app-language
for what's-new reloads" } and the other two similar catches inside
WhatsNewViewModel), check for CancellationException and rethrow it before
logging/handling: add "if (t is CancellationException) throw t" at the top of
each catch, and add the import kotlinx.coroutines.CancellationException; this
preserves coroutine cancellation semantics while keeping the existing error
logging for non-cancellation failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4bd239dd-6c53-4c17-87cf-059d381c56ea
📒 Files selected for processing (14)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ar/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/bn/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/es/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/fr/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/hi/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/it/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ja/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ko/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ru/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/tr/16.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/16.json
| } catch (t: Throwable) { | ||
| logger.e(t) { "Failed to evaluate what's-new state" } | ||
| logger.e(t) { "Failed to observe app-language for what's-new reloads" } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kotlin coroutines, should CancellationException be rethrown when using catch(Throwable) in ViewModel coroutine blocks?
💡 Result:
Yes—when you catch a broad Throwable in code running in viewModelScope (or any structured coroutine), you should not “consume” CancellationException. Either (a) don’t catch it in the first place by catching specific exceptions, or (b) if you must catch Throwable, then rethrow when it’s CancellationException (directly or via ensureActive()), so coroutine cancellation can complete correctly. Rationale (grounded in docs): 1) Android’s coroutines best-practices explicitly says: “don’t consume exceptions of type CancellationException (don’t catch them, or always rethrow them if caught)” [1]. 2) Kotlin coroutines treat cancellation as exceptional control flow: coroutines use CancellationException for cancellation, and “these exceptions are ignored by all handlers” [2]. If you catch them and don’t rethrow/propagate, you prevent the coroutine from terminating as cancellation expects. 3) Kotlin documentation recommends using a try/catch to handle exceptions within the coroutine body, and notes that CancellationException is part of the cancellation mechanism (and is ignored by handlers) [3][2]. Practical pattern when using catch(Throwable): - Prefer catching specific exceptions (e.g., IOException) instead of Throwable in viewModelScope blocks. - If you must catch Throwable, do: - if e is CancellationException -> rethrow e - otherwise handle/log the error A common equivalent is coroutineContext.ensureActive() at the top of the catch, which will rethrow cancellation if the coroutine is actually cancelled. So the direct answer to your question is: yes, CancellationException should effectively be rethrown (not swallowed) when using catch(Throwable) in ViewModel coroutine blocks, to preserve correct cooperative cancellation [1][2].
Citations:
- 1: https://developer.android.com/kotlin/coroutines/coroutines-best-practices
- 2: https://kotlinlang.org/docs/exception-handling.html
- 3: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-coroutine-exception-handler
🏁 Script executed:
# Check if the file exists and read the relevant sections
fd -t f "WhatsNewViewModel.kt" composeApp/src/commonMain/kotlin/
# Get file size
wc -l composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt
# Read the file around the specified lines (40-75 to see context)
sed -n '40,75p' composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt | cat -nRepository: OpenHub-Store/GitHub-Store
Length of output: 1655
🏁 Script executed:
# Read the beginning of the file to check package name and imports
head -40 composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt | cat -nRepository: OpenHub-Store/GitHub-Store
Length of output: 2109
🏁 Script executed:
# Verify the actual line numbers - show lines 49-50, 60-61, 68-69 with full context
sed -n '45,75p' composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewViewModel.kt | cat -nRepository: OpenHub-Store/GitHub-Store
Length of output: 1269
Rethrow CancellationException in broad Throwable catches to preserve cancellation semantics.
Lines 49-50, 60-61, and 68-69 use catch (t: Throwable), which swallows CancellationException. This breaks ViewModel cancellation and can emit misleading error logs when the ViewModel is cleared. Add if (t is CancellationException) throw t in each block and import CancellationException from kotlinx.coroutines.
Proposed fix
+import kotlinx.coroutines.CancellationException
@@
- } catch (t: Throwable) {
+ } catch (t: Throwable) {
+ if (t is CancellationException) throw t
logger.e(t) { "Failed to observe app-language for what's-new reloads" }
}
@@
- } catch (t: Throwable) {
+ } catch (t: Throwable) {
+ if (t is CancellationException) throw t
logger.e(t) { "Failed to load what's-new history" }
}
@@
- } catch (t: Throwable) {
+ } catch (t: Throwable) {
+ if (t is CancellationException) throw t
logger.e(t) { "Failed to evaluate what's-new state" }
}🤖 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 49 - 50, In the catch blocks that currently use "catch (t:
Throwable)" (the ones that log via logger.e(t) { "Failed to observe app-language
for what's-new reloads" } and the other two similar catches inside
WhatsNewViewModel), check for CancellationException and rethrow it before
logging/handling: add "if (t is CancellationException) throw t" at the top of
each catch, and add the import kotlinx.coroutines.CancellationException; this
preserves coroutine cancellation semantics while keeping the existing error
logging for non-cancellation failures.
1 participant
Bug
Switching the app language in Tweaks left the What's-new sheet and the Profile → What's new history showing the previous language's text.
Root cause
`WhatsNewViewModel.init` loaded entries once into `_historyEntries` and `_pendingEntry` StateFlows and never re-loaded. Even though the underlying `WhatsNewLoader` resolves the locale per call (so a fresh load would pick up the new language), the cached VM state served the previous language's strings until the VM was destroyed.
On Android the symptom is permanent until process death — the Activity recreates on language change but the ViewModelStore (and therefore the VM instance) survives `recreate()`. On Desktop language changes already require a restart, so the bug doesn't surface there in practice; this PR's behaviour is a no-op on the cold-start path it already took.
Fix
`WhatsNewViewModel` now collects `tweaksRepository.getAppLanguage().distinctUntilChanged()` and re-runs both `loadAll()` (history) and `evaluate()` (pending sheet) on every emission. `evaluate()` keeps its existing `lastSeen >= current` short-circuit so language changes after the user dismisses the sheet don't re-pop it.
Test plan
Summary by CodeRabbit
Bug Fixes
Improvements