refactor: drop runBlocking { getString } from common repos and parsers#1051
Open
anikettuli wants to merge 2 commits into
Open
refactor: drop runBlocking { getString } from common repos and parsers#1051anikettuli wants to merge 2 commits into
anikettuli wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors multiple commonMain call sites to remove runBlocking { getString(...) } around Compose resources, pushing localization lookups into suspend/composable contexts, and fixes locale-dependent grouping caused by a localized MetaTrailer.type fallback.
Changes:
- Replaces many
runBlocking { getString(...) }usages by making call chainssuspend, moving lookups into composables viastringResource(...), or caching string fallbacks within suspend functions. - Fixes
MetaTrailer.typedefaulting to a localized string by making the fallback locale-independent ("") and applying the localized label at render/grouping time. - Removes stale localized fields from
MetaScreenSectionItemand updates tab labeling to derive titles fromStringResourceat render time.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| composeApp/src/commonTest/kotlin/com/nuvio/app/features/details/MetaDetailsParserTest.kt | Updates tests to call the now-suspend MetaDetailsParser.parse. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/trakt/TraktCommentsRepository.kt | Removes blocking resource lookup by caching a fallback username string in a suspend context. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/trakt/TraktAuthRepository.kt | Makes connect flow suspend and replaces blocking localization helper with getString. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/tmdb/TmdbMetadataService.kt | Removes runBlocking lookups, converts helpers to suspend, and fixes trailer type fallback/grouping. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/streams/StreamModels.kt | Replaces localized default stream label with a literal fallback. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/settings/TraktSettingsPage.kt | Wraps connect actions in rememberCoroutineScope().launch to call suspend repository code. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/settings/MetaScreenSettingsPage.kt | Exposes MetaScreenSectionKey.titleRes for reuse in details UI. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/profiles/ProfileRepository.kt | Removes blocking localization helper by making a verification helper suspend and calling getString. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/home/HomeCatalogSettingsRepository.kt | Converts sync/build helpers to suspend to allow non-blocking getString usage. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/details/MetaScreenSettingsRepository.kt | Removes localized string fields from section items and drops blocking localizedString helper. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/details/MetaDetailsScreen.kt | Uses stringResource(key.titleRes) when rendering tab labels. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/details/MetaDetailsParser.kt | Makes parsing suspend and removes blocking string lookups while parsing videos/trailers/streams. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/details/components/DetailSeriesContent.kt | Converts helper label/badge builders to composables using stringResource. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/details/components/DetailMetaInfo.kt | Fixes TalkBack/localization for the audience-score rating icon without blocking calls. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/collection/FolderDetailRepository.kt | Makes initialization suspend and removes blocking string lookups for tab/error labels. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/collection/CollectionRepository.kt | Makes JSON validation suspend and removes blocking localized error creation. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/collection/CollectionManagementScreen.kt | Calls suspend JSON validation from a UI coroutine scope. |
| composeApp/src/commonMain/kotlin/com/nuvio/app/features/addons/AddonModels.kt | Replaces localized fallback addon label with a literal fallback in a getter. |
71bea9a to
e86a5fd
Compare
4dd7521 to
614ab1f
Compare
Replaces 23 of 41 runBlocking { getString } sites in commonMain across
the meta-details, collection, Trakt, profile, and TMDB code paths.
Wrapping suspend getString in runBlocking blocks the calling thread on
every property read or recomposition and deadlocks on Kotlin/JS.
Three patterns: propagate suspend up call chains that were already
fully suspend; mark non-Composable helpers @composable + use
stringResource; and where the call sat in a data-class property
getter, use a literal English fallback (fires only on degenerate
addon manifests with no name and no URL-derived slug).
Also fixes MetaTrailer.type defaulting to a localized "Trailer"
string when missing — UI already handles blank type with a localized
fallback at render time, so the eager localization mismatched
grouping/sort keys across locales.
The 18 remaining sites are in androidMain platform-bridge callbacks
(OkHttp / ExoPlayer / NotificationManager) and are tracked as a
follow-up.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
StreamItem.streamLabel now returns name.orEmpty() so the data class
stays locale-neutral. The four composable display sites
(StreamsScreen, PlayerSourcesPanel, PlayerEpisodesPanel,
PlayerControls) localize the blank case with
stringResource(Res.string.stream_default_name). DownloadsRepository's
fallbackTitle path is unchanged because buildFileName already does
.ifBlank { "download" }; DownloadItem.streamTitle is only re-read in
PlayerScreen which already has its own .ifBlank fallback.
Also drops the redundant outer runBlocking wrapper on the
assertFailsWith test in MetaDetailsParserTest — only the inner
runBlocking is needed since assertFailsWith's lambda is non-suspend.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
e86a5fd to
29cb596
Compare
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
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.
Summary
Replaces a cross-cutting threading anti-pattern across the meta-details, collection, Trakt, profile, and TMDB code paths, and fixes one localization bug surfaced while auditing the same call sites.
runBlocking { getString(...) }call sites incommonMain. The remaining 18 are inandroidMainplatform-bridge callbacks (OkHttp / ExoPlayer / NotificationManager) and are tracked as a follow-up.MetaTrailer.typefallback that broke trailer category grouping/sorting in non-English locales.PR type
Why
runBlocking { getString }removal.getStringfrom compose-resources issuspendfor a reason — wrapping it inrunBlockingblocks the calling thread on every property read or recomposition. Several offenders were inside data-class property getters (StreamItem.streamLabel,ManagedAddon.displayTitle) which are read by Compose on every recomposition, and inside repository helpers backing tab labels and validation errors. On Kotlin/JS it would deadlock outright.The fix uses three patterns. Where the enclosing function was already
suspend(or had a fully-suspend caller chain),runBlockingis dropped and the enclosing function markedsuspend; composableonClickhandlers were wrapped incoroutineScope.launch { ... }. Touched in this pass:MetaDetailsParser.parse,CollectionRepository.validateJson,FolderDetailRepository.initialize,TraktAuthRepository.onConnectRequested,ProfileRepository.verifyPinLocally,HomeCatalogSettingsRepository.{syncCatalogs, syncCollections, buildCollectionDefinitions},TmdbMetadataService.{buildPeople, toMetaTrailer}.Where the call sat in a non-Composable helper that was only ever called from composables (
Int.label,MetaVideo.episodeBadgeinDetailSeriesContent.kt), the helper was marked@Composableand switched tostringResource(...). Where the call sat in a data-class property getter that cannot be madesuspend, the fallback was replaced with a literal English string; those fallbacks only fire in degenerate edge cases (addon manifest with no name AND no URL-derived slug). Deadtitle/descriptionfields onMetaScreenSectionItemwere removed in the same pass — the only consumer was readingkey.titleResdirectly already, so the fields were holding stale strings produced by the veryrunBlockingcall this PR removes.MetaScreenSectionKey.titleReswas elevated fromprivatetointernalto share between settings page and the meta details screen.MetaTrailer.typefallback.MetaTrailer.typeis a category enum (Trailer/Teaser/Clip) used as a grouping/sort key inTmdbMetadataServiceand as a category label inDetailTrailersSection. It was defaulting to the localizedRes.string.generic_trailerstring when missing, which meant a trailer with notypefrom a French-locale device grouped under "Bande-annonce" while the same dataset on an English device grouped under "Trailer". The fix is to default to empty string;DetailTrailersSection.kt:60already doestrailer.type.ifBlank { localizedFallback }at render time, so the user-visible behavior is unchanged but the underlying data is now locale-independent.Policy check
Testing
./gradlew :composeApp:compileFullDebugKotlinAndroidpasses clean../gradlew :composeApp:testFullDebugUnitTestpasses; existingMetaDetailsParserTestwas wrapped inrunBlockingsinceMetaDetailsParser.parseis nowsuspend.Manual (please verify during review):
coroutineScope.launch, no behavior change).type: blank types fall into the same group as the localized "Trailer" category rather than producing a separate empty-name group.Screenshots / Video (UI changes only)
No new UI surface.
Breaking changes
None. All changes are internal to
commonMain; no public KMP API was removed. Several internal repository functions becamesuspendand their callers were updated in the same PR.Linked issues
None linked. Surfaced during a code audit of
runBlockingusage.(Supersedes #1050, which auto-closed when the branch was renamed — cross-fork PRs don't follow GitHub's branch-rename redirect.)