feat: Add Deezer API integration for artist artwork#624
Conversation
- Add DeezerApiService and DeezerModels for Deezer API - Add ArtistImageRepository with LRU memory cache and DB caching - Add imageUrl column to ArtistEntity with database migration v10->v11 - Update Artist model to include imageUrl field - Update ArtistListItem to display artist artwork with AsyncImage - Update ArtistDetailScreen header to show artist image - Add artist image fetching in PlayerViewModel and ArtistDetailViewModel - Fix database migration with fallbackToDestructiveMigration(dropAllTables=true)
There was a problem hiding this comment.
Pull request overview
This PR adds Deezer API integration to fetch and display artist artwork throughout the application. The implementation includes a caching layer (LRU memory cache + Room database persistence), database schema changes, and UI updates to display artist images in both list and detail views.
Key Changes
- Implements a new
ArtistImageRepositorywith dual-layer caching (memory + database) for artist images from Deezer API - Adds database migration (v10→v11) to support storing artist image URLs in the artists table
- Updates UI components to display artist artwork with fallback to default icons when images are unavailable
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
ArtistImageRepository.kt |
New repository implementing image fetching from Deezer API with LRU memory cache and database persistence |
DeezerApiService.kt |
New Retrofit interface for Deezer artist search API |
DeezerModels.kt |
Data models for Deezer API responses with multiple image size options |
AppModule.kt |
Adds Deezer Retrofit instance and ArtistImageRepository dependency injection; includes database migration setup |
PixelPlayDatabase.kt |
Adds MIGRATION_10_11 to add image_url column to artists table |
MusicDao.kt |
Adds queries to fetch and update artist image URLs |
ArtistEntity.kt |
Adds imageUrl field and updates conversion functions |
LibraryModels.kt |
Adds imageUrl field to Artist data class |
PlayerViewModel.kt |
Adds background fetching of artist images when artists are loaded |
ArtistDetailViewModel.kt |
Fetches artist image when detail view is opened if not already cached |
LibraryScreen.kt |
Updates ArtistListItem to display artist images with circular clipping and fallback icon |
ArtistDetailScreen.kt |
Updates header to display artist artwork with gradient overlay for text readability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PixelPlayDatabase.MIGRATION_10_11 | ||
| ) | ||
| .fallbackToDestructiveMigration() | ||
| .fallbackToDestructiveMigration(dropAllTables = true) |
There was a problem hiding this comment.
The parameter 'dropAllTables = true' in fallbackToDestructiveMigration will drop all tables if any migration fails, causing complete data loss. This is dangerous for a production app as users will lose all their playlists, favorites, and cached data. Consider either removing the parameter (defaults to false for incremental migrations) or adding proper migrations for all version gaps to prevent fallback scenarios.
| .fallbackToDestructiveMigration(dropAllTables = true) | |
| .fallbackToDestructiveMigration() |
| PixelPlayDatabase.MIGRATION_4_5, | ||
| PixelPlayDatabase.MIGRATION_6_7, |
There was a problem hiding this comment.
Missing migrations for versions 5→6, 7→8, and 8→9. The migration list jumps from MIGRATION_6_7 directly to MIGRATION_9_10. Users upgrading from versions 7, 8, or 9 will trigger a destructive migration (due to the dropAllTables setting), losing all their data. All migration paths should be provided to prevent data loss.
| PixelPlayDatabase.MIGRATION_4_5, | |
| PixelPlayDatabase.MIGRATION_6_7, | |
| PixelPlayDatabase.MIGRATION_4_5, | |
| PixelPlayDatabase.MIGRATION_5_6, | |
| PixelPlayDatabase.MIGRATION_6_7, | |
| PixelPlayDatabase.MIGRATION_7_8, | |
| PixelPlayDatabase.MIGRATION_8_9, |
| package com.theveloper.pixelplay.data.repository | ||
|
|
||
| import android.util.Log | ||
| import android.util.LruCache | ||
| import com.theveloper.pixelplay.data.database.MusicDao | ||
| import com.theveloper.pixelplay.data.network.deezer.DeezerApiService | ||
| import kotlinx.coroutines.Dispatchers | ||
| import kotlinx.coroutines.sync.Mutex | ||
| import kotlinx.coroutines.sync.withLock | ||
| import kotlinx.coroutines.withContext | ||
| import javax.inject.Inject | ||
| import javax.inject.Singleton | ||
|
|
||
| /** | ||
| * Repository for fetching and caching artist images from Deezer API. | ||
| * Uses both in-memory LRU cache and Room database for persistent storage. | ||
| */ | ||
| @Singleton | ||
| class ArtistImageRepository @Inject constructor( | ||
| private val deezerApiService: DeezerApiService, | ||
| private val musicDao: MusicDao | ||
| ) { | ||
| companion object { | ||
| private const val TAG = "ArtistImageRepository" | ||
| private const val CACHE_SIZE = 100 // Number of artist images to cache in memory | ||
| } | ||
|
|
||
| // In-memory LRU cache for quick access | ||
| private val memoryCache = LruCache<String, String>(CACHE_SIZE) | ||
|
|
||
| // Mutex to prevent duplicate API calls for the same artist | ||
| private val fetchMutex = Mutex() | ||
| private val pendingFetches = mutableSetOf<String>() | ||
|
|
||
| /** | ||
| * Get artist image URL, fetching from Deezer if not cached. | ||
| * @param artistName Name of the artist | ||
| * @param artistId Room database ID of the artist (for caching) | ||
| * @return Image URL or null if not found | ||
| */ | ||
| suspend fun getArtistImageUrl(artistName: String, artistId: Long): String? { | ||
| if (artistName.isBlank()) return null | ||
|
|
||
| val normalizedName = artistName.trim().lowercase() | ||
|
|
||
| // Check memory cache first | ||
| memoryCache.get(normalizedName)?.let { cachedUrl -> | ||
| return cachedUrl | ||
| } | ||
|
|
||
| // Check database cache | ||
| val dbCachedUrl = withContext(Dispatchers.IO) { | ||
| musicDao.getArtistImageUrl(artistId) | ||
| } | ||
| if (!dbCachedUrl.isNullOrEmpty()) { | ||
| memoryCache.put(normalizedName, dbCachedUrl) | ||
| return dbCachedUrl | ||
| } | ||
|
|
||
| // Fetch from Deezer API | ||
| return fetchAndCacheArtistImage(artistName, artistId, normalizedName) | ||
| } | ||
|
|
||
| /** | ||
| * Prefetch artist images for a list of artists in background. | ||
| * Useful for batch loading when displaying artist lists. | ||
| */ | ||
| suspend fun prefetchArtistImages(artists: List<Pair<Long, String>>) { | ||
| withContext(Dispatchers.IO) { | ||
| artists.forEach { (artistId, artistName) -> | ||
| try { | ||
| getArtistImageUrl(artistName, artistId) | ||
| } catch (e: Exception) { | ||
| Log.w(TAG, "Failed to prefetch image for $artistName: ${e.message}") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private suspend fun fetchAndCacheArtistImage( | ||
| artistName: String, | ||
| artistId: Long, | ||
| normalizedName: String | ||
| ): String? { | ||
| // Prevent duplicate fetches for the same artist | ||
| fetchMutex.withLock { | ||
| if (pendingFetches.contains(normalizedName)) { | ||
| return null // Already fetching | ||
| } | ||
| pendingFetches.add(normalizedName) | ||
| } | ||
|
|
||
| return try { | ||
| withContext(Dispatchers.IO) { | ||
| val response = deezerApiService.searchArtist(artistName, limit = 1) | ||
| val deezerArtist = response.data.firstOrNull() | ||
|
|
||
| if (deezerArtist != null) { | ||
| // Use picture_medium for list views, picture_big for detail views | ||
| // We store the medium size as default, UI can request bigger sizes if needed | ||
| val imageUrl = deezerArtist.pictureMedium | ||
| ?: deezerArtist.pictureBig | ||
| ?: deezerArtist.picture | ||
|
|
||
| if (!imageUrl.isNullOrEmpty()) { | ||
| // Cache in memory | ||
| memoryCache.put(normalizedName, imageUrl) | ||
|
|
||
| // Cache in database | ||
| musicDao.updateArtistImageUrl(artistId, imageUrl) | ||
|
|
||
| Log.d(TAG, "Fetched and cached image for $artistName: $imageUrl") | ||
| imageUrl | ||
| } else { | ||
| null | ||
| } | ||
| } else { | ||
| Log.d(TAG, "No Deezer artist found for: $artistName") | ||
| null | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Error fetching artist image for $artistName: ${e.message}") | ||
| null | ||
| } finally { | ||
| fetchMutex.withLock { | ||
| pendingFetches.remove(normalizedName) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Clear all cached images. Useful for debugging or forced refresh. | ||
| */ | ||
| fun clearCache() { | ||
| memoryCache.evictAll() | ||
| } | ||
| } |
There was a problem hiding this comment.
The new ArtistImageRepository lacks test coverage. Since the repository has existing tests (e.g., MusicRepositoryImplTest.kt), the ArtistImageRepository should also have unit tests covering cache behavior, API failure scenarios, concurrent request handling, and database persistence.
| /** | ||
| * Clear all cached images. Useful for debugging or forced refresh. | ||
| */ | ||
| fun clearCache() { | ||
| memoryCache.evictAll() | ||
| } |
There was a problem hiding this comment.
The clearCache method only clears the in-memory cache but doesn't clear the database cache. This inconsistency could lead to confusion where calling clearCache() still shows cached images from the database. Consider either updating the documentation to clarify this behavior or adding a parameter to optionally clear the database cache as well.
| * @return Search response containing list of matching artists | ||
| */ | ||
| @GET("search/artist") | ||
| suspend fun searchArtist( | ||
| @Query("q") query: String, | ||
| @Query("limit") limit: Int = 1 |
There was a problem hiding this comment.
No API key or authentication mechanism is configured for the Deezer API. While the search endpoint might work without authentication, it may have strict rate limits for unauthenticated requests. Consider adding proper API authentication if available, or implementing exponential backoff and rate limiting to handle potential 429 (Too Many Requests) responses.
| * @return Search response containing list of matching artists | |
| */ | |
| @GET("search/artist") | |
| suspend fun searchArtist( | |
| @Query("q") query: String, | |
| @Query("limit") limit: Int = 1 | |
| * @param accessToken Optional Deezer API access token for authenticated requests | |
| * @return Search response containing list of matching artists | |
| */ | |
| @GET("search/artist") | |
| suspend fun searchArtist( | |
| @Query("q") query: String, | |
| @Query("limit") limit: Int = 1, | |
| @Query("access_token") accessToken: String? = null |
| AsyncImage( | ||
| model = ImageRequest.Builder(LocalContext.current) | ||
| .data(artist.imageUrl) | ||
| .crossfade(true) | ||
| .build(), | ||
| contentDescription = artist.name, | ||
| contentScale = ContentScale.Crop, | ||
| modifier = Modifier.fillMaxSize() | ||
| ) |
There was a problem hiding this comment.
Missing error handling for image loading failures. When an image URL fails to load (network error, invalid URL, etc.), AsyncImage will show nothing. Consider adding an error placeholder or fallback to show the MusicIconPattern when image loading fails.
| // Fetch artist image from Deezer if not already cached | ||
| newState.artist?.let { artist -> | ||
| if (artist.imageUrl.isNullOrEmpty()) { | ||
| launch { | ||
| try { | ||
| val imageUrl = artistImageRepository.getArtistImageUrl(artist.name, artist.id) | ||
| if (!imageUrl.isNullOrEmpty()) { | ||
| _uiState.update { state -> | ||
| state.copy(artist = state.artist?.copy(imageUrl = imageUrl)) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.w("ArtistDebug", "Failed to fetch artist image: ${e.message}") | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Launching a coroutine inside the collect block can trigger multiple concurrent image fetch attempts if the Flow emits multiple times. Since getArtistById returns a Flow, it can emit multiple updates (e.g., when the database is updated). Each emission will check if imageUrl is empty and launch a new fetch, potentially causing duplicate API calls. Consider tracking whether an image fetch is already in progress for this artist to prevent redundant fetches.
|
|
||
| for (artist in artistsWithoutImages) { | ||
| try { | ||
| val imageUrl = artistImageRepository.getArtistImageUrl(artist.name, artist.id) | ||
| if (!imageUrl.isNullOrEmpty()) { | ||
| // Update artist in the UI state | ||
| _playerUiState.update { currentState -> | ||
| val updatedArtists = currentState.artists.map { a -> | ||
| if (a.id == artist.id) a.copy(imageUrl = imageUrl) else a | ||
| } | ||
| currentState.copy(artists = updatedArtists.toImmutableList()) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.w("PlayerViewModel", "Failed to fetch image for ${artist.name}: ${e.message}") | ||
| } | ||
| } |
There was a problem hiding this comment.
UI state updates inside a loop can cause performance issues. Each update to _playerUiState triggers recomposition and creates a new immutable list by mapping over all artists. With many artists, this becomes inefficient. Consider collecting all updated artists first, then updating the UI state once after the loop completes.
| for (artist in artistsWithoutImages) { | |
| try { | |
| val imageUrl = artistImageRepository.getArtistImageUrl(artist.name, artist.id) | |
| if (!imageUrl.isNullOrEmpty()) { | |
| // Update artist in the UI state | |
| _playerUiState.update { currentState -> | |
| val updatedArtists = currentState.artists.map { a -> | |
| if (a.id == artist.id) a.copy(imageUrl = imageUrl) else a | |
| } | |
| currentState.copy(artists = updatedArtists.toImmutableList()) | |
| } | |
| } | |
| } catch (e: Exception) { | |
| Log.w("PlayerViewModel", "Failed to fetch image for ${artist.name}: ${e.message}") | |
| } | |
| } | |
| // Work on a mutable copy and apply all updates in a single UI state change | |
| val updatedArtistsList = artistsList.toMutableList() | |
| var hasUpdates = false | |
| for (artist in artistsWithoutImages) { | |
| try { | |
| val imageUrl = artistImageRepository.getArtistImageUrl(artist.name, artist.id) | |
| if (!imageUrl.isNullOrEmpty()) { | |
| val index = updatedArtistsList.indexOfFirst { it.id == artist.id } | |
| if (index != -1) { | |
| updatedArtistsList[index] = updatedArtistsList[index].copy(imageUrl = imageUrl) | |
| hasUpdates = true | |
| } | |
| } | |
| } catch (e: Exception) { | |
| Log.w("PlayerViewModel", "Failed to fetch image for ${artist.name}: ${e.message}") | |
| } | |
| } | |
| if (hasUpdates) { | |
| _playerUiState.update { currentState -> | |
| currentState.copy(artists = updatedArtistsList.toImmutableList()) | |
| } | |
| } |
| // Prevent duplicate fetches for the same artist | ||
| fetchMutex.withLock { | ||
| if (pendingFetches.contains(normalizedName)) { | ||
| return null // Already fetching | ||
| } | ||
| pendingFetches.add(normalizedName) | ||
| } |
There was a problem hiding this comment.
The mutex-based duplicate prevention may not work as intended. When multiple requests for the same artist arrive simultaneously, the first one adds the artist to pendingFetches and proceeds, but subsequent requests immediately return null without waiting. This means the second request won't benefit from the first request's result, potentially causing UI inconsistencies. Consider using a deferred result approach where subsequent requests wait for the pending fetch to complete.
| } catch (e: Exception) { | ||
| Log.e(TAG, "Error fetching artist image for $artistName: ${e.message}") | ||
| null |
There was a problem hiding this comment.
When an API error occurs, the error is logged but no empty/null result is cached in the database. This means every subsequent request for the same artist will retry the API call, potentially causing repeated failed network requests. Consider caching a sentinel value or timestamp to prevent repeated API calls for artists that aren't found or cause errors.
Summary
Deezer API entegrasyonu ile sanatçı fotoğraflarını çekip gösterme özelliği eklendi.
Changes
DeezerApiServiceveDeezerModelsoluşturulduArtistImageRepositoryile LRU memory cache + Room database cacheArtistEntity'yeimage_urlkolonu eklendi (v10→v11)Artistdata class'aimageUrleklendiArtistListItem(Library → Artists tab) artwork gösteriyorArtistDetailScreenheader'da büyük artwork gösteriyorTesting