Fix critical/high/medium code-review findings

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.

Author: lostf1sh

app/schemas/com.lostf1sh.pixelplayeross.data.database.PixelPlayerDatabase/46.json

@@ -17,6 +17,8 @@
     <uses-permission android:name="android.permission.FOREGROUND_SERVICE" />
     <uses-permission android:name="android.permission.POST_NOTIFICATIONS" />
     <uses-permission android:name="android.permission.FOREGROUND_SERVICE_MEDIA_PLAYBACK" />
+    <!-- Long full library rescans run as a dataSync foreground service so they are not killed (F138). -->
+    <uses-permission android:name="android.permission.FOREGROUND_SERVICE_DATA_SYNC" />
     <uses-permission
         android:name="android.permission.WRITE_SETTINGS"
         tools:ignore="ProtectedPermissions" />
@@ -122,6 +124,12 @@
             </intent-filter>
         </service>
 
+        <!-- Allow WorkManager's foreground service to run as dataSync for long library syncs (F138). -->
+        <service
+            android:name="androidx.work.impl.foreground.SystemForegroundService"
+            android:foregroundServiceType="dataSync"
+            tools:node="merge" />
+
         <receiver
             android:name=".ui.glancewidget.PixelPlayerGlanceWidgetReceiver"
             android:exported="false">

app/src/main/AndroidManifest.xml

@@ -64,6 +64,8 @@ class PixelPlayerApplication : Application(), ImageLoaderFactory, Configuration.
     // ADD THE COMPANION OBJECT
     companion object {
         const val NOTIFICATION_CHANNEL_ID = "pixelplayer_music_channel"
+        // Low-importance channel for the background library-sync foreground notification (F138).
+        const val SYNC_NOTIFICATION_CHANNEL_ID = "pixelplayer_sync_channel"
     }
 
     private val appLifecycleObserver = object : DefaultLifecycleObserver {
@@ -94,8 +96,14 @@ class PixelPlayerApplication : Application(), ImageLoaderFactory, Configuration.
                 "PixelPlayerOSS Music Playback",
                 NotificationManager.IMPORTANCE_LOW
             )
+            val syncChannel = NotificationChannel(
+                SYNC_NOTIFICATION_CHANNEL_ID,
+                getString(R.string.sync_notification_channel_name),
+                NotificationManager.IMPORTANCE_LOW
+            )
             val notificationManager = getSystemService(NotificationManager::class.java)
             notificationManager.createNotificationChannel(channel)
+            notificationManager.createNotificationChannel(syncChannel)
         }
 
         ProcessLifecycleOwner.get().lifecycle.addObserver(appLifecycleObserver)

app/src/main/java/com/lostf1sh/pixelplayeross/PixelPlayerApplication.kt

@@ -2,8 +2,6 @@ package com.lostf1sh.pixelplayeross.data.database
 
 import androidx.room.Embedded
 import androidx.room.Entity
-import androidx.room.Index
-import androidx.room.PrimaryKey
 
 // For simplicity, we store colors as hexadecimal Strings.
 // Stores the color values for ONE scheme (either light or dark)
@@ -60,12 +58,12 @@ data class StoredColorSchemeValues(
 
 @Entity(
     tableName = "album_art_themes",
-    indices = [
-        Index(value = ["albumArtUriString", "paletteStyle"])
-    ]
+    // Composite primary key so each (album art, palette style) variant is cached as its own row.
+    // Previously the PK was albumArtUriString alone, which collapsed all styles onto one row (F71).
+    primaryKeys = ["albumArtUriString", "paletteStyle"]
 )
 data class AlbumArtThemeEntity(
-    @PrimaryKey val albumArtUriString: String,
+    val albumArtUriString: String,
     val paletteStyle: String,
     @Embedded(prefix = "light_") val lightThemeValues: StoredColorSchemeValues,
     @Embedded(prefix = "dark_") val darkThemeValues: StoredColorSchemeValues

app/src/main/java/com/lostf1sh/pixelplayeross/data/database/AlbumArtThemeEntity.kt

@@ -36,6 +36,14 @@ interface LocalPlaylistDao {
     @Query("DELETE FROM playlists WHERE id = :playlistId")
     suspend fun deletePlaylist(playlistId: String)
 
+    // playlist_songs has no FK/cascade, so deleting a playlist must also clear its song rows in the
+    // same transaction or they are orphaned forever (F73).
+    @Transaction
+    suspend fun deletePlaylistWithSongs(playlistId: String) {
+        clearPlaylistSongs(playlistId)
+        deletePlaylist(playlistId)
+    }
+
     @Insert(onConflict = OnConflictStrategy.REPLACE)
     suspend fun upsertPlaylistSongs(entities: List<PlaylistSongEntity>)
 

app/src/main/java/com/lostf1sh/pixelplayeross/data/database/LocalPlaylistDao.kt

@@ -1497,7 +1497,10 @@ interface MusicDao {
     @Query("DELETE FROM albums WHERE NOT EXISTS (SELECT 1 FROM songs WHERE songs.album_id = albums.id)")
     suspend fun deleteOrphanedAlbums()
 
-    @Query("DELETE FROM artists WHERE NOT EXISTS (SELECT 1 FROM song_artist_cross_ref WHERE song_artist_cross_ref.artist_id = artists.id)")
+    // Guard against deleting an artist still referenced by songs.artist_id: that column is NOT NULL but its
+    // foreign key is declared ON DELETE SET NULL, so deleting a referenced artist would abort the transaction
+    // with a NOT NULL violation. Excluding still-referenced artists keeps cleanup safe (F72).
+    @Query("DELETE FROM artists WHERE NOT EXISTS (SELECT 1 FROM song_artist_cross_ref WHERE song_artist_cross_ref.artist_id = artists.id) AND NOT EXISTS (SELECT 1 FROM songs WHERE songs.artist_id = artists.id)")
     suspend fun deleteOrphanedArtists()
 
     // --- Favorite Operations ---

app/src/main/java/com/lostf1sh/pixelplayeross/data/database/MusicDao.kt

@@ -25,7 +25,7 @@ import androidx.sqlite.db.SupportSQLiteDatabase
         JellyfinSongEntity::class,
         JellyfinPlaylistEntity::class
     ],
-    version = 45,
+    version = 46,
     exportSchema = true
 )
 abstract class PixelPlayerDatabase : RoomDatabase() {
@@ -555,6 +555,52 @@ abstract class PixelPlayerDatabase : RoomDatabase() {
             }
         }
 
+        val MIGRATION_45_46 = object : Migration(45, 46) {
+            override fun migrate(db: SupportSQLiteDatabase) {
+                // album_art_themes' primary key changes from (albumArtUriString) to
+                // (albumArtUriString, paletteStyle) so every palette-style variant is cached as its
+                // own row instead of overwriting the single row for that art (F71). SQLite cannot
+                // ALTER a primary key, and the table is a regenerable palette cache, so drop and
+                // recreate it with the new schema; entries are recomputed on demand.
+                recreateAlbumArtThemesTableWithCompositeKey(db)
+            }
+        }
+
+        private fun recreateAlbumArtThemesTableWithCompositeKey(db: SupportSQLiteDatabase) {
+            db.execSQL("DROP TABLE IF EXISTS album_art_themes")
+
+            val colorColumns = listOf(
+                "primary", "onPrimary", "primaryContainer", "onPrimaryContainer",
+                "secondary", "onSecondary", "secondaryContainer", "onSecondaryContainer",
+                "tertiary", "onTertiary", "tertiaryContainer", "onTertiaryContainer",
+                "background", "onBackground", "surface", "onSurface",
+                "surfaceVariant", "onSurfaceVariant", "error", "onError",
+                "outline", "errorContainer", "onErrorContainer",
+                "inversePrimary", "inverseSurface", "inverseOnSurface",
+                "surfaceTint", "outlineVariant", "scrim",
+                "surfaceBright", "surfaceDim",
+                "surfaceContainer", "surfaceContainerHigh", "surfaceContainerHighest", "surfaceContainerLow", "surfaceContainerLowest",
+                "primaryFixed", "primaryFixedDim", "onPrimaryFixed", "onPrimaryFixedVariant",
+                "secondaryFixed", "secondaryFixedDim", "onSecondaryFixed", "onSecondaryFixedVariant",
+                "tertiaryFixed", "tertiaryFixedDim", "onTertiaryFixed", "onTertiaryFixedVariant"
+            )
+
+            val themePrefixes = listOf("light_", "dark_")
+            val columnDefinitions = StringBuilder()
+            columnDefinitions.append("albumArtUriString TEXT NOT NULL, ")
+            columnDefinitions.append("paletteStyle TEXT NOT NULL, ")
+            themePrefixes.forEach { prefix ->
+                colorColumns.forEach { column ->
+                    columnDefinitions.append("${prefix}${column} TEXT NOT NULL, ")
+                }
+            }
+            val columnsSql = columnDefinitions.toString().trimEnd(',', ' ')
+
+            db.execSQL(
+                "CREATE TABLE IF NOT EXISTS album_art_themes ($columnsSql, PRIMARY KEY(albumArtUriString, paletteStyle))"
+            )
+        }
+
         private fun ensureSongsTableHasDateAdded(db: SupportSQLiteDatabase) {
             if (!tableExists(db, "songs")) {
                 recreateSongsTable(db)

app/src/main/java/com/lostf1sh/pixelplayeross/data/database/PixelPlayerDatabase.kt

@@ -21,6 +21,22 @@ interface TransitionDao {
     @Upsert
     suspend fun setRules(rules: List<TransitionRuleEntity>)
 
+    /**
+     * Upserts a single rule, deduping the per-playlist default rule (both track ids null) by hand.
+     * The unique index on (playlistId, fromTrackId, toTrackId) treats SQL NULLs as distinct, so a plain
+     * @Upsert keeps inserting duplicate default rules instead of replacing the existing one. Deleting any
+     * existing default first also cleans up duplicates left by the previous behavior (F76).
+     */
+    @Transaction
+    suspend fun upsertRule(rule: TransitionRuleEntity) {
+        if (rule.fromTrackId == null && rule.toTrackId == null) {
+            deletePlaylistDefaultRule(rule.playlistId)
+            setRule(rule.copy(id = 0))
+        } else {
+            setRule(rule)
+        }
+    }
+
     /**
      * Gets the default transition rule for a given playlist.
      * A default rule is one where fromTrackId and toTrackId are both null.

app/src/main/java/com/lostf1sh/pixelplayeross/data/database/TransitionDao.kt

@@ -54,6 +54,8 @@ class JellyfinRepository @Inject constructor(
         private const val KEY_USERNAME = "username"
         private const val KEY_ACCESS_TOKEN = "access_token"
         private const val KEY_USER_ID = "user_id"
+        private const val KEY_LAST_FULL_SYNC = "last_full_sync"
+        private const val SYNC_THRESHOLD_MS = 24 * 60 * 60 * 1000L // 24 hours
 
         private const val JELLYFIN_SONG_ID_OFFSET = 12_000_000_000_000L
         private const val JELLYFIN_ALBUM_ID_OFFSET = 13_000_000_000_000L
@@ -83,6 +85,17 @@ class JellyfinRepository @Inject constructor(
     private val _isLoggedInFlow = MutableStateFlow(false)
     val isLoggedInFlow: StateFlow<Boolean> = _isLoggedInFlow.asStateFlow()
 
+    /** Wall-clock millis of the last successful full library+playlist sync (0 if never). */
+    private var lastFullSyncTime: Long
+        get() = prefs.getLong(KEY_LAST_FULL_SYNC, 0L)
+        set(value) {
+            prefs.edit().putLong(KEY_LAST_FULL_SYNC, value).apply()
+        }
+
+    /** True when the cached library has not been fully synced within the staleness threshold (F111). */
+    fun isLibrarySyncStale(): Boolean =
+        System.currentTimeMillis() - lastFullSyncTime > SYNC_THRESHOLD_MS
+
     init {
         initFromSavedCredentials()
     }
@@ -381,6 +394,9 @@ class JellyfinRepository @Inject constructor(
                 Timber.e(e, "$TAG: Failed to sync unified library")
             }
 
+            // Mark a successful full sync so the dashboard/home don't re-sync on every open (F111).
+            lastFullSyncTime = System.currentTimeMillis()
+
             Result.success(
                 BulkSyncResult(
                     playlistCount = playlistResult.size,

app/src/main/java/com/lostf1sh/pixelplayeross/data/jellyfin/JellyfinRepository.kt

@@ -529,12 +529,16 @@ class SongMetadataEditor(
                 Timber.tag(TAG).e("METADATA_EDIT: Writer failed on temp file ${tempFile.absolutePath}")
                 return false
             }
-            // Stream edited bytes back to the original path (truncate + overwrite).
-            tempFile.inputStream().use { input ->
-                FileOutputStream(originalFile, false).use { out -> input.copyTo(out) }
+            // Durably stage the edited bytes next to the original, then atomically swap
+            // them in. The original is never truncated until the new bytes are on disk,
+            // so an interrupted/failed copy-back leaves the original intact.
+            val replaceOk = safelyReplaceFileContents(originalFile, tempFile)
+            if (!replaceOk) {
+                Timber.tag(TAG).e("METADATA_EDIT: Failed to atomically replace $originalPath")
+                return false
             }
             Timber.tag(TAG).d(
-                "METADATA_EDIT: Restored ${tempFile.length()} edited bytes back to $originalPath"
+                "METADATA_EDIT: Restored ${originalFile.length()} edited bytes back to $originalPath"
             )
             true
         } catch (e: Exception) {
@@ -547,6 +551,76 @@ class SongMetadataEditor(
         }
     }
 
+    /**
+     * Durably replaces [target]'s contents with the bytes from [source] without ever leaving the
+     * target truncated or empty on failure.
+     *
+     * Strategy: copy [source]'s bytes into a sibling staging file in the SAME directory as [target]
+     * (so an atomic rename can stay on one filesystem), fsync the staging file, then atomically
+     * rename it over [target]. The original is never opened for truncation; if any step throws the
+     * original is left intact and the staging file is cleaned up, so an interrupted/failed write
+     * (disk full, transient I/O, process kill) cannot corrupt or zero out the user's media file.
+     *
+     * Falls back to a guarded copy-into-place only if a same-directory staging file cannot be created
+     * (e.g. read-only parent), and even then the original is overwritten only after the staged bytes
+     * are durably written.
+     */
+    private fun safelyReplaceFileContents(target: File, source: File): Boolean {
+        val parent = target.absoluteFile.parentFile
+        val staging = if (parent != null) {
+            File(parent, ".${target.name}.metaedit_${System.nanoTime()}.tmp")
+        } else {
+            null
+        }
+
+        if (staging != null) {
+            return try {
+                source.inputStream().use { input ->
+                    FileOutputStream(staging).use { out ->
+                        input.copyTo(out)
+                        out.fd.sync()
+                    }
+                }
+                val renamed = staging.renameTo(target)
+                if (renamed) {
+                    true
+                } else {
+                    // renameTo can fail across odd filesystems; fall back to a verified copy-in-place
+                    // using the already-durable staging file as the source of truth.
+                    Timber.tag(TAG).w(
+                        "METADATA_EDIT: Atomic rename to ${target.absolutePath} failed; falling back to copy-in-place"
+                    )
+                    copyIntoPlaceDurably(target, staging)
+                }
+            } catch (e: Exception) {
+                Timber.tag(TAG).e(e, "METADATA_EDIT: Safe replace failed for ${target.absolutePath}")
+                false
+            } finally {
+                if (staging.exists() && !staging.delete()) {
+                    Timber.tag(TAG).w("METADATA_EDIT: Could not delete staging file ${staging.absolutePath}")
+                }
+            }
+        }
+
+        // No writable parent for staging: copy directly but still fsync so bytes are durable.
+        return copyIntoPlaceDurably(target, source)
+    }
+
+    private fun copyIntoPlaceDurably(target: File, source: File): Boolean {
+        return try {
+            source.inputStream().use { input ->
+                FileOutputStream(target, false).use { output ->
+                    input.copyTo(output)
+                    output.fd.sync()
+                }
+            }
+            true
+        } catch (e: Exception) {
+            Timber.tag(TAG).e(e, "METADATA_EDIT: Copy-in-place failed for ${target.absolutePath}")
+            false
+        }
+    }
+
     /**
      * FLAC files with high sample rates (>96kHz) or bit depths (>24bit) can cause issues with TagLib.
      * This function detects such files and logs warnings.
@@ -1010,12 +1084,13 @@ class SongMetadataEditor(
             }
             Timber.tag(TAG)
                 .e("VORBISJAVA: Temp file size: ${tempFile.length()} bytes, original: ${audioFile.length()} bytes")
-            
-            tempFile.inputStream().use { input ->
-                FileOutputStream(audioFile, false).use { output ->
-                    input.copyTo(output)
-                    output.fd.sync()
-                }
+
+            // Atomically swap the edited bytes in via a same-directory staging file + fsync + rename.
+            // The original is never truncated before the new bytes are durable, so an interrupted
+            // copy-back cannot leave the .opus file empty/corrupted.
+            if (!safelyReplaceFileContents(audioFile, tempFile)) {
+                Timber.tag(TAG).e("VORBISJAVA: Failed to atomically replace ${audioFile.path}")
+                return false
             }
 
             Timber.tag(TAG).e("VORBISJAVA: SUCCESS - Updated file metadata: ${audioFile.path}")