feat: watched movies collection - #14
Conversation
Konsist architecture test enforces that all DatabaseDataSource interface functions must have the suspend modifier. Apply it to getWatchedMovies() in the interface and both implementations (Room, Noop), and update the repository test to use coEvery/coVerify accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@claude Please review this PR |
9 similar comments
|
@claude Please review this PR |
|
@claude Please review this PR |
|
@claude Please review this PR |
|
@claude Please review this PR |
|
@claude Please review this PR |
|
@claude Please review this PR |
|
@claude Please review this PR |
|
@claude Please review this PR |
|
@claude Please review this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| RateMovieUseCaseInput(event.movieId, event.rating), | ||
| ).collect { resultState -> | ||
| resultState | ||
| .onSuccess { |
There was a problem hiding this comment.
🔴 Critical — Stuck UI state on null data race condition
If viewState.value.data is null (e.g., the movie hasn't loaded yet due to a race between the two concurrent coroutines launched in the Init event), return@onSuccess exits silently. At this point isRatingInProgress was set to true before the launch and never gets reset. The UI will be permanently stuck with the rating controls disabled and no feedback to the user.
| .onSuccess { | |
| val movie = viewState.value.data | |
| if (movie == null) { | |
| setState { copy(isRatingInProgress = false) } | |
| setEffect { MovieDetailsSideEffect.ShowSnackbar("Something went wrong. Please try again.") } | |
| return@onSuccess | |
| } |
| publicRating = movie.voteAverage, | ||
| releaseDate = movie.releaseYear, | ||
| userRating = event.rating.toInt(), | ||
| ), |
There was a problem hiding this comment.
🟠 Major — Nested .collect inside a suspending onSuccess callback
Calling saveWatchedMovieUseCase.invoke(...).collect { ... } synchronously inside the rateMovieUseCase collect block creates nested Flow collection. This blocks the outer collector while the inner one is running and makes the control flow very hard to follow and test.
The cleaner and idiomatic approach is to chain flows using operators so the control flow stays flat:
is MovieDetailsEvent.RateMovie -> {
setState { copy(isRatingInProgress = true) }
viewModelScope.launch {
rateMovieUseCase(RateMovieUseCaseInput(event.movieId, event.rating))
.flatMapLatest { rateResult ->
when {
rateResult is ResultState.Success -> {
val movie = viewState.value.data
?: return@flatMapLatest flowOf(ResultState.Error(ErrorModel.Unknown()))
saveWatchedMovieUseCase(SaveWatchedMovieInput(...))
}
else -> flowOf(rateResult.map { })
}
}
.collect { ... }
}
}
| userRating = event.rating.toInt(), | ||
| isRatingInProgress = false, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Minor — Hardcoded string literal in ViewModel
ViewModels should not own UI strings. They have no Context and strings can't be localised when hardcoded here. This applies to all three literal messages ("Rating saved", "Something went wrong. Please try again.").
Prefer one of these approaches:
- Typed side effect — replace the
message: Stringfield inShowSnackbarwith a sealed type (e.g.RatingSaved,RatingError) and let the Composable resolve each case to the correctstringResource. UiTextwrapper — a common pattern that wraps either aStringResor a plainString, keeping ViewModel logic and UI strings decoupled.
| override fun handleEvents(event: WatchedMovieListEvent) { | ||
| when (event) { | ||
| is WatchedMovieListEvent.GetWatchedMovies -> { | ||
| viewModelScope.launch { |
There was a problem hiding this comment.
🟠 Major — Accumulating coroutines on repeated GetWatchedMovies events
getWatchedMoviesUseCase returns a Room-backed Flow that never completes. Every time GetWatchedMovies is dispatched — which happens on every ON_RESUME of the NavBackStackEntry, including every tab switch — a new, long-lived coroutine is launched and left running alongside the previous ones. After N tab switches, N coroutines all collect the same flow and race to update viewState.
Load the data once in init, or cancel the previous job before launching a new one:
private var loadJob: Job? = null
is WatchedMovieListEvent.GetWatchedMovies -> {
loadJob?.cancel()
loadJob = viewModelScope.launch {
getWatchedMoviesUseCase(input = Unit).collect { ... }
}
}Alternatively, kick off collection in init {} and remove the event entirely — the Watched list is always reactive to DB changes and doesn't need a manual refresh trigger.
| copy( | ||
| isLoading = false, | ||
| errorMessage = null, | ||
| data = it.map { mapper.fromDomainToUi(it) } |
There was a problem hiding this comment.
🟡 Minor — Shadowed it parameter
it.map { mapper.fromDomainToUi(it) } uses it for two different things: the outer it is the List<WatchedMovie> received from onSuccess, and the inner it is each WatchedMovie element. Kotlin allows this, but it's a readability hazard and easy to misread as "map each element using itself".
| data = it.map { mapper.fromDomainToUi(it) } | |
| data = it.map { movie -> mapper.fromDomainToUi(movie) } |
| .onError { error -> | ||
| setState { | ||
| copy( | ||
| isLoading = false, |
There was a problem hiding this comment.
🟡 Minor — Hardcoded fallback string in ViewModel
Same concern as in MovieDetailsViewModel: hardcoded English strings in a ViewModel bypass localisation and couple business logic to UI text. Expose a typed error state or a StringRes-based wrapper instead.
| isLoading = false, | |
| errorMessage = error.message ?: "An error occurred", |
→ Consider replacing errorMessage: String? in WatchedMovieListUiState with an errorRes: Int? (a @StringRes resource ID) so the string is only resolved in the Composable.
| posterUrl = posterUrl, | ||
| overview = overview, | ||
| publicRating = publicRating, | ||
| releaseDate = releaseDate, |
There was a problem hiding this comment.
🟠 Major — System.currentTimeMillis() is not injectable — breaks unit tests
System.currentTimeMillis() cannot be controlled in unit tests. The MoviesRepositoryImplTest for saveWatchedMovie therefore cannot assert on ratedAt, nor can it verify ordering logic that depends on the timestamp. This is already a gap in the existing tests.
Inject a Clock (or a simple () -> Long lambda) into the repository:
internal class MoviesRepositoryImpl(
private val databaseDataSource: DatabaseDataSource,
private val networkDataSource: NetworkDataSource,
private val mappers: Mappers,
private val clock: () -> Long = System::currentTimeMillis, // default keeps prod behaviour
)Then use clock() instead of System.currentTimeMillis(). Tests can pass { FIXED_TIMESTAMP }.
| databaseDataSource.insertWatchedMovie(dbModel) | ||
| emit(ResultState.Success(Unit)) | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 Major — flow { innerFlow.collect { emit(...) } } anti-pattern breaks cancellation
Using .collect inside a flow {} builder is flagged by the Kotlin coroutines team as an anti-pattern. If the outer flow is cancelled while the inner .collect is running, the inner collect is NOT automatically cancelled — cancellation has to propagate through a cooperative check at the next emit call, which may never come for a Room Flow that's waiting for DB changes.
The correct idiom is emitAll, which properly links cancellation:
| override fun getWatchedMovies(): Flow<ResultState<List<WatchedMovie>>> = flow { | |
| emitAll( | |
| databaseDataSource.getWatchedMovies().map { dbModels -> | |
| ResultState.Success(dbModels.map(mappers.watchedMovieDomainMapper::fromDbToDomain)) | |
| } | |
| ) | |
| } |
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 Major — LifecycleEventEffect(ON_RESUME) triggers repeated data loads across tab switches
LifecycleEventEffect uses addObserver under the hood. When the composable re-enters composition while the NavBackStackEntry is already RESUMED, Android's LifecycleRegistry synthetically dispatches ON_RESUME to the newly-added observer. This means every tab switch (Discover → Watched → Discover → Watched …) fires GetWatchedMovies again.
Paired with the accumulation issue noted in WatchedMovieListViewModel, this causes a new long-lived coroutine to be started on each switch. The data will still appear correct (all coroutines emit the same Room data), but the duplicated subscriptions waste resources and can cause state flickering if Room emits updates at the wrong moment.
Preferred fix: Remove this LifecycleEventEffect and instead start the data load once from WatchedMovieListViewModel.init {}:
init {
handleEvents(WatchedMovieListEvent.GetWatchedMovies)
}The Room Flow is reactive — it will automatically push fresh data when the DB changes (e.g., after a new rating is saved). No manual refresh is needed on every resume.
| movie: WatchedMovieUiModel, | ||
| onClick: () -> Unit, | ||
| modifier: Modifier = Modifier, | ||
| ) { |
There was a problem hiding this comment.
🟡 Minor — Use stringResource() instead of LocalContext.current.getString()
LocalContext.current.getString(...) works but bypasses Compose's idiomatic API. stringResource() is the preferred approach inside composables — it reads from the composition-local context, works correctly with Compose Preview, and is more readable.
| ) { | |
| val userRatingDescription = stringResource(R.string.content_description_user_rating, movie.userRating) |
You can also remove the val context = LocalContext.current at line 110 if this is its only usage.
| onMovieClicked = onMovieClicked, | ||
| ) | ||
| } | ||
| if (selectedTabIndex == 1) { |
There was a problem hiding this comment.
🟢 Suggestion — Consider HorizontalPager for native swipe-between-tabs UX
The current if/else conditional rendering tears down each screen completely when switching tabs — all local Compose state is lost and the screen is recomposed from scratch. This also means LifecycleEventEffect in WatchedMovieListScreen fires on every tab visit (see that comment), and swipe gestures between tabs are not supported.
HorizontalPager keeps both pages in the composition (controlled by beyondBoundsPageCount) and supports the standard swipe gesture that Material 3 users expect from a TabRow:
val pagerState = rememberPagerState { tabTitles.size }
val scope = rememberCoroutineScope()
TabRow(selectedTabIndex = pagerState.currentPage) {
tabTitles.forEachIndexed { index, title ->
Tab(
selected = pagerState.currentPage == index,
onClick = { scope.launch { pagerState.animateScrollToPage(index) } },
text = { Text(title) },
)
}
}
HorizontalPager(state = pagerState) { page ->
when (page) {
0 -> MovieListScreen(...)
1 -> WatchedMovieListScreen(...)
}
}Note: the PR description mentions HorizontalPager was the intended approach — this appears to have been swapped out before merge.
| onMovieClicked(sideEffect.movieId) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟢 Suggestion — Add @Preview for WatchedMovieListScreen and WatchedMovieRow
No @Preview composables were added for any of the new screens in this PR (WatchedMovieListScreen, WatchedMovieRow, MovieCatalogTabbedScreen). The project already has previews for existing screens — keeping that convention makes it easier to iterate on UI without running the full app.
@Preview(showBackground = true)
@Composable
private fun WatchedMovieRowPreview() {
AppTheme {
WatchedMovieRow(
movie = WatchedMovieUiModel(
movieId = 1,
,
posterPath = null,
voteAverage = 8.6,
userRating = 9,
releaseYear = "2014",
),
onClick = {},
)
}
}
Code Review — feat: watched movies collectionOverall this is a solid feature implementation. The layering is clean, the MVI pattern is applied consistently, the Room migration is correct, and the test coverage is comprehensive. A few issues need attention before merge. Finding Summary
🔴 Critical
🟠 Major
🟡 Minor
🟢 Suggestions
What looks good ✅
🤖 Generated with Claude Code |
Bug fixes:
- Reset isRatingInProgress = false when viewState.data is null in
MovieDetailsViewModel to prevent permanently stuck UI state
- Fix accumulating coroutines on WatchedMovieListViewModel by moving
data load to init {} instead of triggering on every ON_RESUME event;
remove GetWatchedMovies event and LifecycleEventEffect from screen
- Fix flow { collect } anti-pattern in getWatchedMovies() by replacing
with flow { emitAll(...) } for correct cancellation propagation
- Inject clock: () -> Long into MoviesRepositoryImpl to make
System.currentTimeMillis() testable
Code quality:
- Extract saveRating() helper in MovieDetailsViewModel to flatten
nested .collect inside rateMovieUseCase success callback
- Replace ShowSnackbar(message: String) side effect with typed
RatingSaved / RatingError data objects; resolve strings from
string resources in the screen instead of hardcoding in ViewModel
- Replace WatchedMovieListUiState.errorMessage: String? with
@stringres errorRes: Int? to keep string resolution in the UI layer
- Rename shadowed 'it' lambda parameter to 'movie' in
WatchedMovieListViewModel for readability
- Replace LocalContext.current.getString() with stringResource()
in WatchedMovieRow composable
UX improvement:
- Replace if/else tab switching in MovieCatalogTabbedScreen with
HorizontalPager to support swipe gestures and retain scroll position
Compose:
- Add @Preview composables for WatchedMovieRow, WatchedMovieListScreen
(loading / empty / with-data states) and MovieCatalogTabbedScreen
Tests: update MovieDetailsViewModelTest and WatchedMovieListViewModelTest
to reflect renamed side effects, removed event, and renamed state field
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@claude Please review this PR |
| fun getWatchedMovieEntities(): Flow<List<WatchedMovieEntity>> | ||
|
|
||
| @Query("SELECT * FROM watched_movies WHERE movieId = :movieId") | ||
| fun getWatchedMovieEntity(movieId: Int): WatchedMovieEntity? |
There was a problem hiding this comment.
🟠 Major — Data / Threading
getWatchedMovieEntity() is neither suspend nor returns Flow, making it a synchronous blocking DAO query. Room 2.2+ requires these to be marked @WorkerThread and will throw a IllegalStateException at runtime if called on the main thread. Even though RoomDataSource.getWatchedMovie() is suspend, the coroutine context inside flow { } builder isn't guaranteed unless flowOn is explicitly applied.
| fun getWatchedMovieEntity(movieId: Int): WatchedMovieEntity? | |
| @Query("SELECT * FROM watched_movies WHERE movieId = :movieId") | |
| suspend fun getWatchedMovieEntity(movieId: Int): WatchedMovieEntity? |
This aligns with the sibling insertWatchedMovie pattern and removes any threading ambiguity.
| import com.pantelisstampoulis.androidtemplateproject.database.model.WatchedMovieDbModel | ||
| import com.pantelisstampoulis.androidtemplateproject.database.model.WatchedMovieEntity | ||
|
|
||
| class WatchedMovieDbMapper { |
There was a problem hiding this comment.
🟡 Minor — Convention (CLAUDE.md)
Two violations in one declaration:
-
Missing
internal: Per CLAUDE.md, allDatabaseDataSourceimpl classes and their collaborators in:core:database:roommust beinternal.WatchedMovieDbMapperis public and leaks into the module's public surface. -
Missing mapper interface: CLAUDE.md states "All mappers implement typed interfaces from
:architecture:mapper".WatchedMovieDbMapperdoesn't implementApiToDbMapper<WatchedMovieDbModel, WatchedMovieEntity>orDbToApiMapper. Compare withWatchedMovieDomainMapperwhich correctly implementsDbToDomainMapper.
| class WatchedMovieDbMapper { | |
| internal class WatchedMovieDbMapper : ApiToDbMapper<WatchedMovieDbModel, WatchedMovieEntity> { |
Then rename toDb → fromApiToDb and keep mapFromDb as a convenience (or add a reverse interface).
| setState { copy(userRating = rating, isRatingInProgress = false) } | ||
| setEffect { MovieDetailsSideEffect.RatingSaved } | ||
| } | ||
| .onError { |
There was a problem hiding this comment.
🟠 Major — Data Integrity Bug
movie.releaseYear is the UI-formatted year string (e.g. "2014") produced by MovieUiMapper via movie.releaseDate.take(4). You're storing a truncated year where the DB schema expects a full release date string ("2014-03-07").
The data works coincidentally — WatchedMovieUiMapper.releaseYear will still show "2014" by taking the first 4 chars of "2014" — but the stored value is semantically incorrect. Any future feature that needs the actual date (e.g. sorting by exact release date, display formatting) will silently get a stale/invalid value.
Fix: MovieUiModel doesn't carry the original releaseDate. The cleanest solution is to keep the domain Movie object alongside data: MovieUiModel? in MovieDetailsUiState, then read movie.releaseDate from the domain object in saveRating. Alternatively, add a releaseDate: String? field to MovieUiModel (in addition to releaseYear).
| } | ||
|
|
||
| override fun getWatchedMovies(): Flow<ResultState<List<WatchedMovie>>> = flow { | ||
| emitAll( |
There was a problem hiding this comment.
🟠 Major — Design / Error Modelling
ResultState.Error(ErrorModel.NotFound()) is used to signal "movie has not been rated yet" — a completely normal, non-exceptional state. This forces every caller to silently swallow the error:
.onError {
// NotFound = not rated yet, leave userRating as null
}Using an error state for control flow is semantically wrong and fragile: a future change that adds real error cases here (e.g. DB corruption, decryption failure) would be indistinguishable from "not found" unless the caller explicitly inspects ErrorModel subtype.
Suggested fix: Change the return type to Flow<ResultState<WatchedMovie?>> and emit ResultState.Success(null) when no row is found:
override fun getWatchedMovie(movieId: Int): Flow<ResultState<WatchedMovie?>> = flow {
val dbModel = databaseDataSource.getWatchedMovie(movieId)
emit(ResultState.Success(dbModel?.let { mappers.watchedMovieDomainMapper.fromDbToDomain(it) }))
}Update MoviesRepository, GetWatchedMovieUseCase, and MovieDetailsViewModel accordingly — null becomes the clear "not yet rated" sentinel.
1 participant
Summary
Implements the Watched Movies feature as specified in
SPEC.mdand planned inPLAN.md.Users can rate any movie (1–10 stars) from the Movie Details screen. Rated movies are persisted locally and surfaced in a new dedicated Watched tab on the main screen, showing the user's personal rating alongside the public TMDB rating. Ratings are immutable once saved.
Key Changes by Layer
Database (
core/database)WatchedMovieDbModel(API model) andWatchedMovieEntity(Room entity)WatchedMovieDaowithinsertWatchedMovie,getWatchedMovieEntities, andgetWatchedMovieEntityqueriesDatabaseDataSourceinterface withinsertWatchedMovie,getWatchedMovies, andgetWatchedMovieRoomDataSourceandNoopDatabaseDataSourceWatchedMovieDbMapper(ApiToDb+DbToDomaincontracts)watched_moviestable)getWatchedMovies()madesuspendto satisfy Konsist architecture ruleDomain (
core/domain,core/model)WatchedMoviedomain modelMoviesRepositoryinterface withgetWatchedMovies,getWatchedMovie, andsaveWatchedMovieMoviesRepositoryImpl(:core:data)WatchedMovieDomainMapper(DbToDomain)GetWatchedMoviesUseCase,GetWatchedMovieUseCase,SaveWatchedMovieUseCaseUI / Feature (
feature:movie-catalog)WatchedMovieUiModelandWatchedMovieUiMapper(DomainToUi)MovieCatalogTabbedScreen(Discover + Watched tabs viaHorizontalPager)WatchedMovieListScreenwith: movie list, compact user-rating badge (⭐ N), empty state, and offline-capable loading from local DBWatchedMovieListViewModel(MVI:WatchedMovieListEvent,WatchedMovieListSideEffect)MovieDetailsScreento show the star rating row (interactive for unrated, read-only for rated), with a snackbar on save success/failureMovieDetailsViewModelto orchestrateGetWatchedMovieUseCase(check rated state on load) andSaveWatchedMovieUseCase(persist rating on user action)WatchedMovieListViewModelKoin factory and connect all routesTesting
Unit tests added
GetWatchedMoviesUseCaseImplTestFlow<ResultState.Success>with the full list of watched movies from the repositoryGetWatchedMovieUseCaseImplTestSuccess(movie)for a known ID andSuccess(null)when not foundSaveWatchedMovieUseCaseImplTestSuccess(Unit); emitsErroron repository failureMoviesRepositoryImplTest(new cases)getWatchedMoviesmaps DB models correctly;getWatchedMoviereturns domain model or null;saveWatchedMoviecalls data source and emits successWatchedMovieListViewModelTestLoading, transitions toSuccesswith mapped UI models; empty list shows empty stateMovieDetailsViewModelTest(extended)RateMovieevent triggers save use case; snackbar side-effects fired on success/failureAll tests use Mockative (
coEvery/coVerifyfor suspend functions,every/verifyfor Flow-returning non-suspend functions) and Turbine for Flow assertions, consistent with project conventions.Reference
SPEC.mdPLAN.mdChecklist
🤖 Generated with Claude Code