Fix critical/high/medium code-review findings#4
Closed
lostf1sh wants to merge 4 commits into
Closed
Conversation
Resolve 93 of 94 verified critical/high/medium findings from the multi-agent code review. Every finding was re-verified against the current code before being fixed. Database & data integrity: - Add MIGRATION_45_46 (schema v46): album_art_themes gets a composite primary key (albumArtUriString, paletteStyle) so each palette style is cached as its own row instead of overwriting one row. Migration output verified to match the generated v46 schema. - Clear playlist_songs when a playlist is deleted (deletePlaylistWithSongs) instead of orphaning rows. - Guard deleteOrphanedArtists against artists still referenced by the NOT NULL songs.artist_id (its FK is SET_NULL), avoiding a transaction abort. - Dedupe the per-playlist default transition rule in the DAO (the unique index treats NULL track ids as distinct), no schema change required. Concurrency & reliability: - Drop main-thread runBlocking Coil artwork fetches in MediaItemBuilder (suspend conversion across MediaItemBuilder / TrustedMediaItemsResolution / MusicService). - Make StateHolder scope release identity-safe so a sibling ViewModel's onCleared cannot null a still-alive PlayerViewModel's scope. - Run long FULL/REBUILD library syncs as a dataSync foreground service so they are not killed in the background. - Jellyfin library re-sync is now gated on a 24h staleness window instead of running on every dashboard/home open. Security & UX: - Surface a warning when Navidrome credentials fall back to unencrypted storage. - Add the missing package declaration to DelimiterConfigScreen. - Move hardcoded user-facing strings to resources; model the Jellyfin sync banner as typed state; one-shot save event for transition settings; remember album color-scheme flows; remove a dead duplicate enum. Verified: :app:compileDebugKotlin, :app:testDebugUnitTest and :app:lintDebug all pass. Deferred: F99 (cloud song/album/artist IDs derived from 32-bit String.hashCode) is not auto-applied. Re-deriving IDs requires a favorites/playlist reconciliation migration and on-device testing; applying it blindly would risk silently orphaning cloud favorites and playlists.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds foreground data-sync permission/service and worker retry/foreground promotion; adjusts Room entities, DAOs, and schema snapshots; hardens DataStore and backup import/export; refines repository cold-start and paging keys; localizes and stabilizes many Compose components and lifecycle cleanup; prewarms and palettes widget artwork with size-aware caching; makes media-file writes atomic; updates utilities, tests, and resources. ChangesComprehensive application updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/com/lostf1sh/pixelplayeross/data/repository/MusicRepositoryImpl.kt (1)
121-155:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical concurrency race in cold-start directory filter resolution.
The
@VolatileflagdirFilterResolvedandcachedDirFilter.valueread inresolvedDirFilter()have a race condition:
onEach { dirFilterResolved = true }(line 140) performs a volatile write.stateInupdatescachedDirFilter.valueimmediately after (synchronized write).- Another thread can observe
dirFilterResolved == truevia the volatile read but still see the stale defaultCachedDirFilter()fromcachedDirFilter.valuebecause there is no happens-before relationship between the volatile write inonEachand theStateFlowupdate instateIn.Impact: During the narrow window between steps 1 and 2 of the first emission,
resolvedDirFilter()can returnCachedDirFilter(applyFilter = false), bypassing directory filtering and leaking songs from blocked directories. This defeats the privacy/directory-filtering boundary that this code is intended to enforce.🔒 Proposed fix using CompletableDeferred for proper synchronization
Replace the
@Volatileflag with aCompletableDeferredto ensure the first real value is ready before returning the cached value:- /** - * Becomes true once [cachedDirFilter] has produced its first real value (i.e. the - * DataStore-backed directory preferences have emitted and the filter has been computed). - * Until then [cachedDirFilter] still holds the seeded default (applyFilter = false), which - * would incorrectly bypass the directory filter for users who have blocked directories. - */ - `@Volatile` private var dirFilterResolved = false + /** + * Completed once [cachedDirFilter] has produced its first real value (i.e. the + * DataStore-backed directory preferences have emitted and the filter has been computed). + * Until then [cachedDirFilter] still holds the seeded default (applyFilter = false), which + * would incorrectly bypass the directory filter for users who have blocked directories. + */ + private val dirFilterReady = CompletableDeferred<Unit>() private val cachedDirFilter: StateFlow<CachedDirFilter> = combine( userPreferencesRepository.allowedDirectoriesFlow, userPreferencesRepository.blockedDirectoriesFlow ) { allowed, blocked -> val (dirs, apply) = DirectoryFilterUtils.computeAllowedParentDirs( allowedDirs = allowed, blockedDirs = blocked, getAllParentDirs = { musicDao.getDistinctParentDirectories() }, normalizePath = ::normalizePath ) CachedDirFilter(dirs, apply) - }.onEach { dirFilterResolved = true } + }.onEach { dirFilterReady.complete(Unit) } .stateIn(repositoryScope, SharingStarted.Eagerly, CachedDirFilter()) /** * Returns a resolved directory filter for one-shot reads. If [cachedDirFilter] has not yet * produced its first real value (cold-start window), computes a fresh filter from the * preference flows instead of returning the seeded default — otherwise blocked-directory * songs could leak into queues/library on the first access after process start. */ private suspend fun resolvedDirFilter(): CachedDirFilter { - if (dirFilterResolved) return cachedDirFilter.value - val allowedDirs = userPreferencesRepository.allowedDirectoriesFlow.first() - val blockedDirs = userPreferencesRepository.blockedDirectoriesFlow.first() - val (allowedParentDirs, applyFilter) = computeAllowedDirs(allowedDirs, blockedDirs) - return CachedDirFilter(allowedParentDirs, applyFilter) + dirFilterReady.await() + return cachedDirFilter.value }This ensures that
resolvedDirFilter()will suspend until the first real emission completes, eliminating the race while preserving the caching benefit.🤖 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 `@app/src/main/java/com/lostf1sh/pixelplayeross/data/repository/MusicRepositoryImpl.kt` around lines 121 - 155, The volatile flag dirFilterResolved is racy with cachedDirFilter.value; replace it with a CompletableDeferred<CachedDirFilter> (e.g. firstDirFilterReady) that you complete from the onEach block when the first real CachedDirFilter is emitted, and in resolvedDirFilter() suspend on firstDirFilterReady.await() (or await with timeout if needed) before returning cachedDirFilter.value; update usages of dirFilterResolved and its writes/reads to complete/await the CompletableDeferred so callers cannot observe the volatile flag true while cachedDirFilter still holds the seeded default.app/src/main/java/com/lostf1sh/pixelplayeross/data/preferences/UserPreferencesRepository.kt (1)
1920-1953:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject overflowed numeric coercions during restore.
Long.toInt()can wrap, and the currentDouble/Long -> Int/Longcoercions also silently truncate out-of-range values. A malformed backup can therefore restore negative or nonsensical settings instead of being skipped. Add range/integrality checks before writing narrowed numeric types.Suggested fix
when (effectiveType) { "int" -> { - val value = entry.intValue - ?: entry.doubleValue?.toInt() - ?: entry.longValue?.toInt() - ?: return@forEach + val value = entry.intValue + ?: entry.longValue + ?.takeIf { it in Int.MIN_VALUE.toLong()..Int.MAX_VALUE.toLong() } + ?.toInt() + ?: entry.doubleValue + ?.takeIf { + it.isFinite() && + it % 1.0 == 0.0 && + it in Int.MIN_VALUE.toDouble()..Int.MAX_VALUE.toDouble() + } + ?.toInt() + ?: return@forEach preferences[intPreferencesKey(entry.key)] = value } "long" -> { - val value = entry.longValue - ?: entry.doubleValue?.toLong() - ?: entry.intValue?.toLong() - ?: return@forEach + val value = entry.longValue + ?: entry.intValue?.toLong() + ?: entry.doubleValue + ?.takeIf { + it.isFinite() && + it % 1.0 == 0.0 && + it in Long.MIN_VALUE.toDouble()..Long.MAX_VALUE.toDouble() + } + ?.toLong() + ?: return@forEach preferences[longPreferencesKey(entry.key)] = value }🤖 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 `@app/src/main/java/com/lostf1sh/pixelplayeross/data/preferences/UserPreferencesRepository.kt` around lines 1920 - 1953, When restoring numeric prefs in UserPreferencesRepository (the block using entry.intValue/entry.doubleValue/entry.longValue and writing to intPreferencesKey/longPreferencesKey/androidx.datastore.preferences.core.floatPreferencesKey/androidx.datastore.preferences.core.doublePreferencesKey), reject any narrowing coercions that would overflow or lose integrality: before converting Double->Int/Long or Long->Int, check the source value is finite, within the target type range (Int.MIN_VALUE..Int.MAX_VALUE or Long.MIN_VALUE..Long.MAX_VALUE) and—for Double sources—has no fractional part when targeting integer types; if a check fails, skip that entry (return@forEach) instead of performing toInt()/toLong() truncation. Also apply similar range/finite checks when narrowing Double->Float (ensure value is finite and within Float.MIN_VALUE..Float.MAX_VALUE or use Float.isFinite) and when using Long.toInt() ensure the long is within Int range before assignment.
🧹 Nitpick comments (1)
app/src/main/java/com/lostf1sh/pixelplayeross/presentation/navigation/Screen.kt (1)
29-29:genreIdroute encoding is consistent with the current decoding, so the “+becomes mismatch” concern isn’t supported.
Screen.ktencodesgenreIdwithjava.net.URLEncoder.encode(...), and the read path decodes it withjava.net.URLDecoder.decode(...)(in bothGenreDetailScreenandGenreDetailViewModelviasavedStateHandle). This makes+round-trip back to spaces, so the specific"Hip Hop" -> "Hip+Hop"mismatch claim doesn’t match the implemented flow.
Optional cleanup: align path-segment semantics by switching toUri.encode/Uri.decodeand ensure decoding happens only once.Suggested fix
-import java.net.URLEncoder +import android.net.Uri @@ - fun createRoute(genreId: String) = "genre_detail/" + URLEncoder.encode(genreId, "UTF-8") + fun createRoute(genreId: String) = "genre_detail/${Uri.encode(genreId)}"🤖 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 `@app/src/main/java/com/lostf1sh/pixelplayeross/presentation/navigation/Screen.kt` at line 29, The current createRoute function (createRoute in Screen.kt) uses java.net.URLEncoder.encode while the decode happens via URLDecoder in GenreDetailScreen and GenreDetailViewModel through savedStateHandle — this works but is inconsistent with Android URI semantics; replace URLEncoder.encode(...) with android.net.Uri.encode(...) in createRoute and migrate corresponding decode calls to android.net.Uri.decode(...) (or remove duplicate decoding so decoding happens exactly once in the savedStateHandle/GenreDetailScreen path) to keep encoding/decoding consistent and avoid double-decoding issues.
🤖 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
`@app/src/main/java/com/lostf1sh/pixelplayeross/data/jellyfin/JellyfinRepository.kt`:
- Around line 397-398: lastFullSyncTime is being set unconditionally even when
partial syncs fail; change JellyfinRepository so that lastFullSyncTime is only
updated after a fully successful run of syncUnifiedLibrarySongsFromJellyfin(),
syncLibrarySongs(), and any playlist syncs: detect failures (exceptions or error
results) from syncUnifiedLibrarySongsFromJellyfin() and syncLibrarySongs() and
only assign lastFullSyncTime = System.currentTimeMillis() when all these
operations complete without errors (or return success), otherwise leave the
timestamp unchanged and propagate/log the failure so the stale-gate will retry.
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/data/media/SongMetadataEditor.kt`:
- Around line 584-606: The current fallback after staging.renameTo(target) uses
copyIntoPlaceDurably(target, staging), which reopens target with
FileOutputStream(target, false) and can truncate the original media file
mid-copy; instead, change the fallback in the safe-replace path (the block
handling renamed = staging.renameTo(target) in SongMetadataEditor.kt) to fail
the edit rather than attempt an in-place overwrite: log a clear warning/error
and return false when renameTo fails, and remove/avoid calling
copyIntoPlaceDurably in this branch (apply the same change to the other
identical fallback around the lines 609–616), so we never truncate the original
media file when rename cannot be used.
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/data/paging/MediaStorePagingSource.kt`:
- Around line 36-37: The refresh key reconstruction is using
state.config.pageSize which can mismatch the actual loaded page size; update the
logic in getRefreshKey (the code that reads anchorPage?.prevKey/nextKey) to use
the actual loaded page size from the anchor page (e.g., anchorPage?.data?.size)
when computing the offset, falling back to state.config.initialLoadSize or
state.config.pageSize if the anchor page size is unavailable, so the computed
prev/next keys align with the real loaded page bounds.
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/data/preferences/UserPreferencesRepository.kt`:
- Around line 1828-1833: The redaction is asymmetric: code currently filters out
secret/URL-like keys during export (using isLikelySecretKey) but clearExisting
still deletes those live keys, and the import code accepts them from a backup;
to fix, change the clearExisting logic in UserPreferencesRepository (the method
that wipes existing prefs before import) to preserve any keys where
isLikelySecretKey(keyName) returns true instead of deleting them, and ensure the
import/restore path also skips any incoming entries with
isLikelySecretKey(keyName) (same check used in the export mapNotNull). Update
both the clearExisting implementation and the import loop that writes prefs so
secret keys are neither removed nor written from backups.
In `@app/src/main/java/com/lostf1sh/pixelplayeross/data/worker/SyncWorker.kt`:
- Around line 158-184: The delete-detection uses
fetchMediaStoreIds(directoryResolver) before the runtime minSongDurationMs is
refreshed, so short tracks configured below the default 10000ms can be
misclassified as deleted; fix by loading the preference-backed threshold into
minSongDurationMs before the deletedIds computation (or modify
fetchMediaStoreIds to accept an explicit threshold parameter and pass the
resolved value) so that deletedIds (computed from
musicDao.getAllMediaStoreSongIds(), mediaStoreIds and SyncMode.REBUILD handling)
uses the correct duration cutoff.
- Around line 781-788: The incremental-sync change check treats a missing
MediaStore album artist as a change signal; update the condition that compares
existing.albumArtist and raw.albumArtist to ignore the cursor value when the
cursor didn't supply one (i.e., when raw.albumArtist == null) the same way the
genre guard does. Locate the boolean expression using existing.albumArtist and
raw.albumArtist in SyncWorker.kt and change it to only consider the cursor value
as a signal when raw.albumArtist != null (for example, require raw.albumArtist
== null || existing.albumArtist == raw.albumArtist) so songs with an existing
tag-parsed albumArtist aren't spuriously reprocessed or overwritten.
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/presentation/jellyfin/dashboard/JellyfinDashboardViewModel.kt`:
- Around line 54-55: The Failed data class currently surfaces raw
Throwable.message to users (data class Failed(val message: String?) :
JellyfinSyncBanner) which can leak backend/OkHttp/server details; change the
code paths that construct Failed to map throwable types/messages to a small set
of user-facing reasons (e.g., "Network error", "Authentication failed", "Server
unavailable", "Unknown error") and pass those sanitized strings into Failed,
while logging the original throwable (stack trace and message) to the app
logger; update all places that create Failed from exceptions (the sync routines
that call Failed(...), referenced in the file around the usages you saw) to
perform this mapping before constructing Failed and keep
JellyfinSyncStatus.Error unchanged.
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/PlaylistViewModel.kt`:
- Around line 1113-1117: The resolver.update call that finalizes the MediaStore
item must be checked: after calling resolver.update(itemUri, finalize, null,
null) in PlaylistViewModel, validate the returned updateCount is 1 and if not,
throw an exception (e.g., IOException) so the existing catch/cleanup logic runs
and the pending row is removed; reference the local variables resolver,
finalize, itemUri and playlist.name when adding the error message to aid
debugging.
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/SongRemovalStateHolder.kt`:
- Line 70: The deletion currently force-casts Song.id with song.id.toLong() in
SongRemovalStateHolder, which can throw for non-numeric IDs; update the flow to
avoid hard-casting by (a) extending the repository contract (e.g., add
musicRepository.deleteByIdentifier(String id) or
musicRepository.deleteByCanonicalId(String id)) and call that with song.id for
non-numeric identifiers, or (b) if numeric deletion is required, first attempt a
safe parse (e.g., tryParse/NumberFormatException guard) and only call
musicRepository.deleteById(parsedLong) when parsing succeeds; update the
SongRemovalStateHolder call site to use the new repository method or the guarded
parse so no runtime exception is possible.
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/PixelPlayerGlanceWidget.kt`:
- Around line 122-128: The loop currently prewarms album art for every item in
playerInfo.queue causing unnecessary work; restrict prewarming to only the
thumbnails the widget can render (four for the ExtraLarge layout). Update the
loop that calls decodeIntoCacheIfAbsent to iterate only the first four queue
items (or use ExtraLargeWidgetLayout.THUMBNAIL_COUNT if that constant exists)
while keeping the same queueTargetPx calculation and behavior.
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/WidgetUtils.kt`:
- Around line 30-42: The getKey function currently only considers content hash,
causing different size variants of the same artwork to share a cache entry and
potentially serve smaller cached bitmaps to larger widgets. Modify the getKey
function to accept render dimensions (width and height parameters) in addition
to the byteArray, and include these dimensions in the returned cache key string
by appending them to the hex-encoded SHA-256 digest. Also identify any
URI-backed key generation functions (likely other overloaded versions or related
caching methods) that generate keys similarly and apply the same
dimension-inclusion logic to them.
In `@app/src/main/java/com/lostf1sh/pixelplayeross/utils/Extensions.kt`:
- Around line 47-55: MOJIBAKE_PUNCTUATION_REGEX currently only checks for "â"
followed by Windows-1252 special chars, missing ISO-8859-1 C1 control sequence
mojibake like �/�; update the Regex used by MOJIBAKE_PUNCTUATION_REGEX to
include the Unicode range U+0080..U+00BF (e.g. \u0080-\u00BF) in the character
class so that String.hasMojibakeSignature() will detect those ISO-8859-1
mojibake sequences as well.
---
Outside diff comments:
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/data/preferences/UserPreferencesRepository.kt`:
- Around line 1920-1953: When restoring numeric prefs in
UserPreferencesRepository (the block using
entry.intValue/entry.doubleValue/entry.longValue and writing to
intPreferencesKey/longPreferencesKey/androidx.datastore.preferences.core.floatPreferencesKey/androidx.datastore.preferences.core.doublePreferencesKey),
reject any narrowing coercions that would overflow or lose integrality: before
converting Double->Int/Long or Long->Int, check the source value is finite,
within the target type range (Int.MIN_VALUE..Int.MAX_VALUE or
Long.MIN_VALUE..Long.MAX_VALUE) and—for Double sources—has no fractional part
when targeting integer types; if a check fails, skip that entry (return@forEach)
instead of performing toInt()/toLong() truncation. Also apply similar
range/finite checks when narrowing Double->Float (ensure value is finite and
within Float.MIN_VALUE..Float.MAX_VALUE or use Float.isFinite) and when using
Long.toInt() ensure the long is within Int range before assignment.
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/data/repository/MusicRepositoryImpl.kt`:
- Around line 121-155: The volatile flag dirFilterResolved is racy with
cachedDirFilter.value; replace it with a CompletableDeferred<CachedDirFilter>
(e.g. firstDirFilterReady) that you complete from the onEach block when the
first real CachedDirFilter is emitted, and in resolvedDirFilter() suspend on
firstDirFilterReady.await() (or await with timeout if needed) before returning
cachedDirFilter.value; update usages of dirFilterResolved and its writes/reads
to complete/await the CompletableDeferred so callers cannot observe the volatile
flag true while cachedDirFilter still holds the seeded default.
---
Nitpick comments:
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/presentation/navigation/Screen.kt`:
- Line 29: The current createRoute function (createRoute in Screen.kt) uses
java.net.URLEncoder.encode while the decode happens via URLDecoder in
GenreDetailScreen and GenreDetailViewModel through savedStateHandle — this works
but is inconsistent with Android URI semantics; replace URLEncoder.encode(...)
with android.net.Uri.encode(...) in createRoute and migrate corresponding decode
calls to android.net.Uri.decode(...) (or remove duplicate decoding so decoding
happens exactly once in the savedStateHandle/GenreDetailScreen path) to keep
encoding/decoding consistent and avoid double-decoding issues.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 45890821-5937-4903-b52d-0ccf8ac875f9
📒 Files selected for processing (85)
app/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/46.jsonapp/src/main/AndroidManifest.xmlapp/src/main/java/com/lostf1sh/pixelplayeross/PixelPlayerApplication.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/database/AlbumArtThemeEntity.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/database/LocalPlaylistDao.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/database/MusicDao.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/database/PixelPlayerDatabase.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/database/TransitionDao.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/jellyfin/JellyfinRepository.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/media/SongMetadataEditor.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/navidrome/NavidromeRepository.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/paging/MediaStorePagingSource.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/preferences/PlaylistPreferencesRepository.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/preferences/UserPreferencesRepository.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/repository/MusicRepositoryImpl.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/repository/TransitionRepositoryImpl.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/service/TrustedMediaItemsResolution.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/stats/PlaybackStatsRepository.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/worker/NavidromeSyncWorker.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/worker/SyncManager.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/worker/SyncWorker.ktapp/src/main/java/com/lostf1sh/pixelplayeross/di/AppModule.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/EditSongSheet.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/ExpressiveOfflineState.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/ExpressiveScrollBar.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/FileExplorerBottomSheet.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/LyricsSheet.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/PlaylistBottomSheet.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/ReorderPresetsSheet.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/ReorderTabsSheet.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/SongInfoBottomSheet.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/UnifiedPlayerSheetShared.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/external/ExternalPlayerOverlay.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/scoped/MiniPlayerDismissGestureHandler.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/scoped/SheetActionHandlers.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/jellyfin/dashboard/JellyfinDashboardScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/jellyfin/dashboard/JellyfinDashboardViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/library/LibraryTabId.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/navidrome/dashboard/NavidromeDashboardScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/navidrome/dashboard/NavidromeDashboardViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/navigation/AppNavigation.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/navigation/Screen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/AlbumDetailScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/ArtistDetailScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/CreatePlaylistScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/DailyMixScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/DelimiterConfigScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/EditTransitionScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/GenreDetailScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/HomeScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/LibraryMediaTabs.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/LibraryScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/PlaylistDetailScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/RecentlyPlayedScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/SearchScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/screens/SettingsScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/ConnectivityStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/DailyMixStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/EqualizerViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/GenreDetailViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/LibraryStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/ListeningStatsTracker.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/LyricsStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/MashupViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/PlaybackStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/PlayerViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/PlaylistViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/SearchStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/SettingsViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/SleepTimerStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/SongRemovalStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/ThemeStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/TransitionViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/PixelPlayerGlanceWidget.ktapp/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/WidgetUtils.ktapp/src/main/java/com/lostf1sh/pixelplayeross/utils/AudioDecoder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/utils/AudioFileProvider.ktapp/src/main/java/com/lostf1sh/pixelplayeross/utils/Extensions.ktapp/src/main/java/com/lostf1sh/pixelplayeross/utils/LyricsUtils.ktapp/src/main/java/com/lostf1sh/pixelplayeross/utils/MediaItemBuilder.ktapp/src/main/res/values/plurals.xmlapp/src/main/res/values/strings.xmlapp/src/main/res/values/strings_presentation_batch_g.xmlapp/src/test/java/com/lostf1sh/pixelplayeross/data/service/TrustedMediaItemsResolutionTest.ktapp/src/test/java/com/lostf1sh/pixelplayeross/presentation/library/LibraryTabIdTest.kt
💤 Files with no reviewable changes (4)
- app/src/test/java/com/lostf1sh/pixelplayeross/presentation/library/LibraryTabIdTest.kt
- app/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/ReorderTabsSheet.kt
- app/src/main/java/com/lostf1sh/pixelplayeross/presentation/library/LibraryTabId.kt
- app/src/main/java/com/lostf1sh/pixelplayeross/presentation/components/LyricsSheet.kt
Comment on lines
+158
to
184
| // --- DELETION DETECTION PHASE --- | ||
| // Detect deleted songs efficiently using ID comparison. We do this for | ||
| // INCREMENTAL and FULL modes. REBUILD clears everything anyway. | ||
| // | ||
| // NOTE: detection only — the actual removal is deferred and folded into the | ||
| // same @Transaction as the upsert below (incrementalSyncMusicData accepts a | ||
| // deletedSongIds parameter). Deleting here in a separate transaction risked a | ||
| // failure between the committed delete and the upsert, leaving rows deleted but | ||
| // their replacements unwritten until the next successful sync. | ||
| val deletedIds: List<Long> = | ||
| if (syncMode != SyncMode.REBUILD) { | ||
| // Only compare MediaStore-backed songs; cloud sources are excluded. | ||
| val localSongIds = musicDao.getAllMediaStoreSongIds().toHashSet() | ||
| val mediaStoreIds = fetchMediaStoreIds(directoryResolver) | ||
|
|
||
| // Identify IDs that are in local DB but not in MediaStore | ||
| val ids = (localSongIds - mediaStoreIds).toList() | ||
| if (ids.isNotEmpty()) { | ||
| Timber.tag(TAG) | ||
| .i("Found ${ids.size} deleted songs. Removal deferred to upsert transaction...") | ||
| } else { | ||
| Timber.tag(TAG).d("No deleted songs found.") | ||
| } | ||
| ids | ||
| } else { | ||
| Timber.tag(TAG).d("No deleted songs found.") | ||
| emptyList() | ||
| } |
There was a problem hiding this comment.
Load the duration filter before computing deletedIds.
This block calls fetchMediaStoreIds(directoryResolver) before Line 148 refreshes minSongDurationMs from preferences, so the delete phase always uses the default 10000 ms cutoff. Any library configured below 10 seconds can have valid short tracks misclassified as deleted and removed on the next sync.
Move the min-duration read above the deletion phase, or pass the resolved threshold into fetchMediaStoreIds() instead of relying on the field 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 `@app/src/main/java/com/lostf1sh/pixelplayeross/data/worker/SyncWorker.kt`
around lines 158 - 184, The delete-detection uses
fetchMediaStoreIds(directoryResolver) before the runtime minSongDurationMs is
refreshed, so short tracks configured below the default 10000ms can be
misclassified as deleted; fix by loading the preference-backed threshold into
minSongDurationMs before the deletedIds computation (or modify
fetchMediaStoreIds to accept an explicit threshold parameter and pass the
resolved value) so that deletedIds (computed from
musicDao.getAllMediaStoreSongIds(), mediaStoreIds and SyncMode.REBUILD handling)
uses the correct duration cutoff.
The only prior GitHub release (v0.7.0-foss, versionCode 8) is a throwaway with no real installed base; the first real distribution is the upcoming F-Droid build. So there is no device whose database needs the inherited v3..v46 upgrade path. - Reset @database version 46 -> 1 and remove all 43 MIGRATION_x_y objects and their migration-only helpers (recreate*/getTableColumns/addColumnIfMissing/ ensure*/rebuildSongsSearchIndex, etc.). - Keep the fresh-install machinery: createRuntimeArtifactsCallback still installs the songs_fts virtual table and the favorite/FTS sync triggers in onCreate (Room does not manage these from the entity list). - AppModule: drop addMigrations(...); add fallbackToDestructiveMigrationOnDowngrade so installing over the throwaway higher-versioned build wipes-and-recreates instead of crashing. Full destructive fallback stays debug-only. - Regenerate the exported schema as 1.json; remove stale 43/44/45/46.json. - Remove the now-obsolete PixelPlayerDatabaseMigrationTest. All entity/DAO-level fixes from the previous commit are preserved and now ship as the v1 baseline (e.g. album_art_themes composite primary key, transition_rules unique index, deleteOrphanedArtists guard). New upgrade migrations will be added normally from version 2 onward. Verified: :app:compileDebugKotlin, :app:testDebugUnitTest, :app:lintDebug and :app:compileDebugAndroidTestKotlin all pass.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/src/main/java/com/lostf1sh/pixelplayeross/di/AppModule.kt (1)
352-356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact user identifiers from logged URLs too.
HEADERSlogging still prints the full request URL, and the current redaction list misses Navidrome'suusername param and Jellyfin'sUserId. Those values will still end up in logcat for every debug request.Suggested fix
- redactQueryParams("t", "s", "api_key") + redactQueryParams("t", "s", "api_key", "u", "UserId")🤖 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 `@app/src/main/java/com/lostf1sh/pixelplayeross/di/AppModule.kt` around lines 352 - 356, The request logging redaction currently calls redactQueryParams("t", "s", "api_key") in AppModule.kt but misses Navidrome's username param and Jellyfin's user identifier; update the redactQueryParams call (the one invoked where HTTP logging is configured) to include "u" and "UserId" so those query parameters are also redacted from HEADERS-level logs.app/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/1.json (1)
640-665:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
songs.artist_idwith its FK action before freezing v1.This baseline still declares
artist_idasNOT NULLwhile the FK usesON DELETE SET NULL. Any direct delete fromartistswill fail instead of applying the declared action. Since this snapshot is now version 1, please fix the root contract now by either makingartist_idnullable or changing the FK action to match the real invariant (RESTRICT/CASCADE).Also applies to: 968-974
🤖 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 `@app/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/1.json` around lines 640 - 665, The schema declares artist_id as NOT NULL but the FK uses ON DELETE SET NULL; update the contract so they match by making artist_id nullable: change the createSql definition to remove the NOT NULL constraint for `artist_id` (so the FOREIGN KEY `artist_id` can be set NULL on delete) and update the corresponding fields entry (fieldPath "artistId", columnName "artist_id") to set notNull: false; apply the same fix to the other occurrence referenced (the second createSql/fields block) so both snapshots are consistent with ON DELETE SET NULL.
🤖 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 `@app/src/main/java/com/lostf1sh/pixelplayeross/di/AppModule.kt`:
- Around line 127-131: The Room builder currently calls
.fallbackToDestructiveMigrationOnDowngrade(dropAllTables = true) unconditionally
which will wipe user data on downgrade; change this to only enable destructive
downgrade in non-release builds (e.g. guard the call behind BuildConfig.DEBUG or
an internal build flag) so release APKs never invoke
fallbackToDestructiveMigrationOnDowngrade. Locate the Room database builder in
AppModule.kt and wrap or conditionally add the
.fallbackToDestructiveMigrationOnDowngrade(...) invocation based on the
debug/internal build check (leave forward migrations unchanged).
---
Outside diff comments:
In
`@app/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/1.json`:
- Around line 640-665: The schema declares artist_id as NOT NULL but the FK uses
ON DELETE SET NULL; update the contract so they match by making artist_id
nullable: change the createSql definition to remove the NOT NULL constraint for
`artist_id` (so the FOREIGN KEY `artist_id` can be set NULL on delete) and
update the corresponding fields entry (fieldPath "artistId", columnName
"artist_id") to set notNull: false; apply the same fix to the other occurrence
referenced (the second createSql/fields block) so both snapshots are consistent
with ON DELETE SET NULL.
In `@app/src/main/java/com/lostf1sh/pixelplayeross/di/AppModule.kt`:
- Around line 352-356: The request logging redaction currently calls
redactQueryParams("t", "s", "api_key") in AppModule.kt but misses Navidrome's
username param and Jellyfin's user identifier; update the redactQueryParams call
(the one invoked where HTTP logging is configured) to include "u" and "UserId"
so those query parameters are also redacted from HEADERS-level logs.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 732a85e6-93e1-4300-8b78-718fc9267520
📒 Files selected for processing (6)
app/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/1.jsonapp/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/43.jsonapp/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/45.jsonapp/src/androidTest/java/com/lostf1sh/pixelplayeross/data/database/PixelPlayerDatabaseMigrationTest.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/database/PixelPlayerDatabase.ktapp/src/main/java/com/lostf1sh/pixelplayeross/di/AppModule.kt
💤 Files with no reviewable changes (3)
- app/src/androidTest/java/com/lostf1sh/pixelplayeross/data/database/PixelPlayerDatabaseMigrationTest.kt
- app/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/43.json
- app/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/45.json
Comment on lines
+127
to
+131
| // Schema starts at version 1 for the first public (F-Droid) release, so there are no | ||
| // upgrade migrations yet. If a throwaway pre-release build with a higher DB version is | ||
| // present, wipe-and-recreate on downgrade instead of crashing (no real data exists yet). | ||
| // Forward migrations are added normally from version 2 onward. | ||
| .fallbackToDestructiveMigrationOnDowngrade(dropAllTables = true) |
There was a problem hiding this comment.
Don't ship destructive downgrade fallback in release.
This is unconditional today, so any rollback from a future schema version to this APK will silently wipe playlists, favorites, and stats instead of refusing to open the newer DB. If this is only meant to bridge internal pre-release installs, gate it behind debug/internal builds rather than keeping it in the release builder path.
🤖 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 `@app/src/main/java/com/lostf1sh/pixelplayeross/di/AppModule.kt` around lines
127 - 131, The Room builder currently calls
.fallbackToDestructiveMigrationOnDowngrade(dropAllTables = true) unconditionally
which will wipe user data on downgrade; change this to only enable destructive
downgrade in non-release builds (e.g. guard the call behind BuildConfig.DEBUG or
an internal build flag) so release APKs never invoke
fallbackToDestructiveMigrationOnDowngrade. Locate the Room database builder in
AppModule.kt and wrap or conditionally add the
.fallbackToDestructiveMigrationOnDowngrade(...) invocation based on the
debug/internal build check (leave forward migrations unchanged).
Skip 2 (one already fixed, one nitpick); fix 13: Jellyfin sync-stale gating and error sanitization, durable metadata replace, paging refresh key, backup secret-key redaction symmetry + numeric narrowing guards, albumArtist sync signal, playlist export finalize check, song-removal id parse, widget art prewarm + dimensioned cache key, mojibake regex C1 range, and a dir-filter resolution race.
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)
app/src/main/java/com/lostf1sh/pixelplayeross/data/jellyfin/JellyfinRepository.kt (1)
373-381:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate playlist-fetch failure instead of returning a success summary.
At Line 379, this branch returns
Result.success(...)even whensyncPlaylists()failed, so the ViewModel follows the success path and shows a synced banner for a failed run.💡 Proposed minimal fix
- val playlistResult = syncPlaylists().getOrElse { + val playlistResult = syncPlaylists().getOrElse { error -> try { syncUnifiedLibrarySongsFromJellyfin() } catch (e: Exception) { Timber.e(e, "$TAG: Failed to sync unified library after playlist fetch failure") } - return@withContext Result.success( - BulkSyncResult(playlistCount = 0, syncedSongCount = syncedSongCount, failedPlaylistCount = 0) - ) + return@withContext Result.failure(error) }🤖 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 `@app/src/main/java/com/lostf1sh/pixelplayeross/data/jellyfin/JellyfinRepository.kt` around lines 373 - 381, The current getOrElse branch swallows syncPlaylists() failures and returns Result.success(...), causing the ViewModel to treat a failed playlist fetch as success; change the error path so that when syncPlaylists() fails you propagate the failure (return a Result.failure with the original exception or rethrow) instead of returning Result.success. Update the block that now calls syncUnifiedLibrarySongsFromJellyfin() and returns BulkSyncResult to instead return Result.failure(it) (or wrap the caught Exception) so callers see the failure; reference syncPlaylists(), syncUnifiedLibrarySongsFromJellyfin(), and the BulkSyncResult-returning withContext block to locate and modify the code.
♻️ Duplicate comments (1)
app/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/WidgetComponents.kt (1)
76-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winURI-backed cache keys must include size dimensions.
The URI cache key still uses
"uri:$rawUri"without incorporating the requested size, so different widget sizes will share the same cached bitmap. If an 80dp widget populates the cache first, a 120dp widget will hit the cache and reuse the smaller bitmap instead of decoding a sharper version at its target size. This is the same collision issue that was fixed for byte-array keys on lines 40-43, but it remains unresolved for URI-backed artwork.🔧 Proposed fix to include size in URI cache keys
} ?: albumArtUri?.let { rawUri -> - val cacheKey = "uri:$rawUri" + val cacheKey = "uri:$rawUri@${(size.value * context.resources.displayMetrics.density).toInt().coerceAtLeast(1)}" var bitmap = AlbumArtBitmapCache.getBitmap(cacheKey) if (bitmap == null) { bitmap = decodeAlbumArtFromUri( context = context, rawUri = rawUri, requestedSize = size ) bitmap?.let { AlbumArtBitmapCache.putBitmap(cacheKey, it) } } bitmap?.let { ImageProvider(it) } }🤖 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 `@app/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/WidgetComponents.kt` around lines 76 - 88, The URI-backed cache key is missing size dimensions so different requested sizes collide; update the cacheKey construction where albumArtUri is handled (the block using rawUri, cacheKey, AlbumArtBitmapCache, decodeAlbumArtFromUri and ImageProvider) to incorporate the requested size (the size variable passed into decodeAlbumArtFromUri) into the key so keys are unique per URI+size (e.g., include width/height or size.value in the key) before calling AlbumArtBitmapCache.getBitmap/putBitmap.
🤖 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
`@app/src/main/java/com/lostf1sh/pixelplayeross/data/jellyfin/JellyfinRepository.kt`:
- Around line 373-381: The current getOrElse branch swallows syncPlaylists()
failures and returns Result.success(...), causing the ViewModel to treat a
failed playlist fetch as success; change the error path so that when
syncPlaylists() fails you propagate the failure (return a Result.failure with
the original exception or rethrow) instead of returning Result.success. Update
the block that now calls syncUnifiedLibrarySongsFromJellyfin() and returns
BulkSyncResult to instead return Result.failure(it) (or wrap the caught
Exception) so callers see the failure; reference syncPlaylists(),
syncUnifiedLibrarySongsFromJellyfin(), and the BulkSyncResult-returning
withContext block to locate and modify the code.
---
Duplicate comments:
In
`@app/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/WidgetComponents.kt`:
- Around line 76-88: The URI-backed cache key is missing size dimensions so
different requested sizes collide; update the cacheKey construction where
albumArtUri is handled (the block using rawUri, cacheKey, AlbumArtBitmapCache,
decodeAlbumArtFromUri and ImageProvider) to incorporate the requested size (the
size variable passed into decodeAlbumArtFromUri) into the key so keys are unique
per URI+size (e.g., include width/height or size.value in the key) before
calling AlbumArtBitmapCache.getBitmap/putBitmap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9efd6b8-c798-467c-a69d-ac67c5ea1a06
📒 Files selected for processing (15)
app/src/main/java/com/lostf1sh/pixelplayeross/data/jellyfin/JellyfinRepository.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/media/SongMetadataEditor.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/paging/MediaStorePagingSource.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/preferences/UserPreferencesRepository.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/repository/MusicRepositoryImpl.ktapp/src/main/java/com/lostf1sh/pixelplayeross/data/worker/SyncWorker.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/jellyfin/dashboard/JellyfinDashboardScreen.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/jellyfin/dashboard/JellyfinDashboardViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/PlaylistViewModel.ktapp/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/SongRemovalStateHolder.ktapp/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/PixelPlayerGlanceWidget.ktapp/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/WidgetComponents.ktapp/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/WidgetUtils.ktapp/src/main/java/com/lostf1sh/pixelplayeross/utils/Extensions.ktapp/src/main/res/values/strings.xml
✅ Files skipped from review due to trivial changes (1)
- app/src/main/res/values/strings.xml
🚧 Files skipped from review as they are similar to previous changes (7)
- app/src/main/java/com/lostf1sh/pixelplayeross/utils/Extensions.kt
- app/src/main/java/com/lostf1sh/pixelplayeross/presentation/jellyfin/dashboard/JellyfinDashboardScreen.kt
- app/src/main/java/com/lostf1sh/pixelplayeross/presentation/viewmodel/PlaylistViewModel.kt
- app/src/main/java/com/lostf1sh/pixelplayeross/data/worker/SyncWorker.kt
- app/src/main/java/com/lostf1sh/pixelplayeross/data/repository/MusicRepositoryImpl.kt
- app/src/main/java/com/lostf1sh/pixelplayeross/data/preferences/UserPreferencesRepository.kt
- app/src/main/java/com/lostf1sh/pixelplayeross/ui/glancewidget/PixelPlayerGlanceWidget.kt
Two follow-up code-review fixes: - JellyfinRepository.syncAllPlaylistsAndSongs: return Result.failure on syncPlaylists() failure instead of masking it as Result.success, so the dashboard shows an error (and the stale-gate retries) rather than a green banner. - WidgetComponents.AlbumArtImage: include the requested size in the URI cache key so the same artwork at different widget sizes (Bar 44dp / Control 80dp / Grid variable) no longer collides. These widgets don't prewarm, so no shared-key coordination is affected.
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.
Summary
Two parts:
Part 1 — Findings
Database & data integrity
album_art_themesgets a composite PK(albumArtUriString, paletteStyle)so each palette style is cached separately (now baked into the v1 baseline).playlist_songson playlist delete (deletePlaylistWithSongs).deleteOrphanedArtistsagainst artists still referenced by theNOT NULLsongs.artist_id.Concurrency & reliability
runBlockingCoil artwork fetches fromMediaItemBuilder(suspend conversion).PlayerViewModel's scope.FULL/REBUILDlibrary syncs as adataSyncforeground service.Security & UX
DelimiterConfigScreen.rememberalbum color-scheme flows; remove a dead duplicate enum.Part 2 — Migration squash (v1 baseline)
The only prior GitHub release (
v0.7.0-foss, versionCode 8) is a throwaway with no real installed base; the first real distribution is the upcoming F-Droid build. So no device needs the inherited v3..v46 upgrade path.@Databaseversion 46 → 1; all 43MIGRATION_x_yobjects and migration-only helpers removed.createRuntimeArtifactsCallbackstill createssongs_fts+ the favorite/FTS sync triggers inonCreate.AppModule:addMigrations(...)dropped;fallbackToDestructiveMigrationOnDowngrade(dropAllTables = true)added so installing over the throwaway higher-versioned build wipes-and-recreates instead of crashing. Full destructive fallback stays debug-only.1.json; stale43/44/45/46.jsonremoved; obsoletePixelPlayerDatabaseMigrationTestremoved.All entity/DAO-level fixes ship as the v1 baseline. New upgrade migrations will be added normally from version 2 onward.
Verification
:app:compileDebugKotlin:app:testDebugUnitTest:app:lintDebug:app:compileDebugAndroidTestKotlinDeferred — F99
Cloud song/album/artist IDs derive from 32-bit
String.hashCode(). Re-deriving them changes the IDs that existing favorites (Room) and playlists (DataStore) reference; without a tested reconciliation migration it would risk silently orphaning that data. Left for a deliberately-designed migration (lower stakes now that v1 is a clean baseline, but still a data-shape change worth doing carefully).Summary by CodeRabbit
New Features
Bug Fixes
Improvements