fix: resolve multiple scrobbling bugs causing lost scrobbles (#107)

sozercan · web-flow · commit 8abefe54a612 · 2026-02-23T20:29:12.000-08:00
diff --git a/Core/Services/Scrobbling/LastFMService.swift b/Core/Services/Scrobbling/LastFMService.swift
@@ -325,25 +325,29 @@ final class LastFMService: ScrobbleServiceProtocol {
 
     // swiftformat:disable modifierOrder
     nonisolated private func checkForErrors(_ response: [String: Any]) throws {
-        guard let errorCode = response["error"] as? Int else {
-            return // No error
+        // Handle Last.fm integer error codes
+        if let errorCode = response["error"] as? Int {
+            let message = response["message"] as? String ?? "Unknown error"
+
+            switch errorCode {
+            case 4:
+                throw ScrobbleError.sessionExpired
+            case 9:
+                throw ScrobbleError.invalidCredentials
+            case 11, 16:
+                throw ScrobbleError.serviceUnavailable
+            case 17:
+                throw ScrobbleError.serviceUnavailable
+            case 29:
+                throw ScrobbleError.rateLimited(retryAfter: nil)
+            default:
+                throw ScrobbleError.invalidResponse("Last.fm error \(errorCode): \(message)")
+            }
         }
 
-        let message = response["message"] as? String ?? "Unknown error"
-
-        switch errorCode {
-        case 4:
-            throw ScrobbleError.sessionExpired
-        case 9:
-            throw ScrobbleError.invalidCredentials
-        case 11, 16:
-            throw ScrobbleError.serviceUnavailable
-        case 17:
-            throw ScrobbleError.serviceUnavailable
-        case 29:
-            throw ScrobbleError.rateLimited(retryAfter: nil)
-        default:
-            throw ScrobbleError.invalidResponse("Last.fm error \(errorCode): \(message)")
+        // Handle string errors from the Cloudflare Worker proxy
+        if let errorMessage = response["error"] as? String {
+            throw ScrobbleError.invalidResponse("Worker error: \(errorMessage)")
         }
     }
 
@@ -356,8 +360,8 @@ final class LastFMService: ScrobbleServiceProtocol {
     ) -> [ScrobbleResult] {
         // Last.fm returns scrobbles.scrobble (single object or array)
         guard let scrobblesWrapper = response["scrobbles"] as? [String: Any] else {
-            // If no scrobbles key, assume all accepted
-            return tracks.map { ScrobbleResult(track: $0, accepted: true) }
+            // No scrobbles key — response is malformed; mark all as rejected for retry
+            return tracks.map { ScrobbleResult(track: $0, accepted: false, errorMessage: "Malformed response: missing scrobbles key") }
         }
 
         // Normalize to array
@@ -374,8 +378,11 @@ final class LastFMService: ScrobbleServiceProtocol {
         for (index, track) in tracks.enumerated() {
             if index < scrobbleEntries.count {
                 let entry = scrobbleEntries[index]
-                let ignoredMessage = (entry["ignoredMessage"] as? [String: Any])?["#text"] as? String
-                let accepted = ignoredMessage == nil || ignoredMessage?.isEmpty == true
+                let ignoredObj = entry["ignoredMessage"] as? [String: Any]
+                let ignoredCode = ignoredObj?["code"] as? String
+                let ignoredText = ignoredObj?["#text"] as? String
+                // Code "0" means accepted; any other code means rejected
+                let accepted = ignoredCode == nil || ignoredCode == "0"
 
                 let correctedArtistFlag = (entry["artist"] as? [String: Any])?["corrected"] as? String
                 let correctedTrackFlag = (entry["track"] as? [String: Any])?["corrected"] as? String
@@ -391,7 +398,7 @@ final class LastFMService: ScrobbleServiceProtocol {
                     accepted: accepted,
                     correctedArtist: correctedArtist,
                     correctedTrack: correctedTrack,
-                    errorMessage: accepted ? nil : ignoredMessage
+                    errorMessage: accepted ? nil : (ignoredText?.isEmpty == false ? ignoredText : "Ignored (code \(ignoredCode ?? "unknown"))")
                 ))
             } else {
                 results.append(ScrobbleResult(track: track, accepted: true))
diff --git a/Core/Services/Scrobbling/ScrobblingCoordinator.swift b/Core/Services/Scrobbling/ScrobblingCoordinator.swift
@@ -23,6 +23,15 @@ final class ScrobblingCoordinator {
     /// The video ID of the track currently being tracked.
     private var currentTrackVideoId: String?
 
+    /// Title of the track currently being tracked (for change detection when videoId is stale).
+    private var currentTrackTitle: String?
+
+    /// Artist of the track currently being tracked (for change detection when videoId is stale).
+    private var currentTrackArtist: String?
+
+    /// Snapshot of the tracked Song at the time tracking started (for finalization).
+    private var trackedSong: Song?
+
     /// When the current track started playing (for scrobble timestamp).
     private var trackStartTime: Date?
 
@@ -154,7 +163,12 @@ final class ScrobblingCoordinator {
 
         // Track change detection
         if let track = currentTrack {
-            if track.videoId != self.currentTrackVideoId {
+            let videoIdChanged = track.videoId != self.currentTrackVideoId
+            // Also detect by title/artist for natural transitions where videoId is stale
+            let metadataChanged = self.currentTrackVideoId != nil
+                && (track.title != self.currentTrackTitle || track.artistsDisplay != self.currentTrackArtist)
+
+            if videoIdChanged || metadataChanged {
                 // Track changed — finalize previous, start tracking new
                 self.finalizeCurrentTrack()
                 self.startTrackingNewTrack(track)
@@ -194,6 +208,9 @@ final class ScrobblingCoordinator {
 
     private func startTrackingNewTrack(_ track: Song) {
         self.currentTrackVideoId = track.videoId
+        self.currentTrackTitle = track.title
+        self.currentTrackArtist = track.artistsDisplay
+        self.trackedSong = track
         self.trackStartTime = Date()
         self.accumulatedPlayTime = 0
         self.lastProgress = self.playerService.progress
@@ -207,10 +224,21 @@ final class ScrobblingCoordinator {
         // Nothing to finalize if no track was being tracked
         guard self.currentTrackVideoId != nil else { return }
 
+        // Final threshold check before discarding accumulated play time
+        if !self.hasScrobbled, let song = self.trackedSong {
+            let duration = song.duration ?? self.playerService.duration
+            if duration > 0 {
+                self.checkScrobbleThreshold(track: song, duration: duration)
+            }
+        }
+
         self.logger.debug("Finalized track (accumulated: \(String(format: "%.1f", self.accumulatedPlayTime))s, scrobbled: \(self.hasScrobbled))")
 
         // Reset tracking state
         self.currentTrackVideoId = nil
+        self.currentTrackTitle = nil
+        self.currentTrackArtist = nil
+        self.trackedSong = nil
         self.trackStartTime = nil
         self.accumulatedPlayTime = 0
         self.lastProgress = 0
diff --git a/Tests/KasetTests/LastFMServiceTests.swift b/Tests/KasetTests/LastFMServiceTests.swift
@@ -239,7 +239,7 @@ struct LastFMServiceTests {
         #expect(results[0].correctedTrack == nil)
     }
 
-    @Test("parseScrobbleResponse detects rejected scrobbles via ignoredMessage")
+    @Test("parseScrobbleResponse detects rejected scrobbles via ignoredMessage code")
     func parseScrobbleResponseRejected() {
         let track = ScrobbleTrack(title: "Song", artist: "Artist")
 
@@ -259,6 +259,62 @@ struct LastFMServiceTests {
         #expect(results[0].errorMessage == "Track was ignored")
     }
 
+    @Test("parseScrobbleResponse accepts when ignoredMessage code is 0")
+    func parseScrobbleResponseAcceptedWithCode0() {
+        let track = ScrobbleTrack(title: "Song", artist: "Artist")
+
+        let response: [String: Any] = [
+            "scrobbles": [
+                "scrobble": [
+                    "artist": ["corrected": "0", "#text": "Artist"],
+                    "track": ["corrected": "0", "#text": "Song"],
+                    "ignoredMessage": ["#text": "", "code": "0"],
+                ] as [String: Any],
+            ] as [String: Any],
+        ]
+
+        let results = LastFMService.parseScrobbleResponse(response, tracks: [track])
+
+        #expect(results[0].accepted)
+        #expect(results[0].errorMessage == nil)
+    }
+
+    @Test("parseScrobbleResponse rejects when ignoredMessage code is non-zero with empty text")
+    func parseScrobbleResponseRejectedEmptyText() {
+        let track = ScrobbleTrack(title: "Song", artist: "Artist")
+
+        let response: [String: Any] = [
+            "scrobbles": [
+                "scrobble": [
+                    "artist": ["corrected": "0", "#text": "Artist"],
+                    "track": ["corrected": "0", "#text": "Song"],
+                    "ignoredMessage": ["#text": "", "code": "2"],
+                ] as [String: Any],
+            ] as [String: Any],
+        ]
+
+        let results = LastFMService.parseScrobbleResponse(response, tracks: [track])
+
+        #expect(!results[0].accepted)
+        #expect(results[0].errorMessage?.contains("code 2") == true)
+    }
+
+    @Test("parseScrobbleResponse marks all rejected when scrobbles key is missing")
+    func parseScrobbleResponseMissingScrobblesKey() {
+        let track = ScrobbleTrack(title: "Song", artist: "Artist")
+
+        // Malformed response (e.g., worker error that passed checkForErrors)
+        let response: [String: Any] = [
+            "status": "ok",
+        ]
+
+        let results = LastFMService.parseScrobbleResponse(response, tracks: [track])
+
+        #expect(results.count == 1)
+        #expect(!results[0].accepted)
+        #expect(results[0].errorMessage?.contains("Malformed") == true)
+    }
+
     @Test("parseScrobbleResponse handles batch with mixed results")
     func parseScrobbleResponseBatch() {
         let tracks = [
@@ -272,12 +328,12 @@ struct LastFMServiceTests {
                     [
                         "artist": ["corrected": "0", "#text": "Artist"],
                         "track": ["corrected": "0", "#text": "Accepted Song"],
-                        "ignoredMessage": ["#text": ""],
+                        "ignoredMessage": ["#text": "", "code": "0"],
                     ] as [String: Any],
                     [
                         "artist": ["corrected": "0", "#text": "Artist"],
                         "track": ["corrected": "0", "#text": "Rejected Song"],
-                        "ignoredMessage": ["#text": "Artist was ignored"],
+                        "ignoredMessage": ["#text": "Artist was ignored", "code": "1"],
                     ] as [String: Any],
                 ] as [[String: Any]],
             ] as [String: Any],
diff --git a/Tests/KasetTests/ScrobblingCoordinatorTests.swift b/Tests/KasetTests/ScrobblingCoordinatorTests.swift
@@ -401,4 +401,59 @@ struct ScrobblingCoordinatorTests {
         let isReplay = hasScrobbled && newProgress < lastProgress - threshold
         #expect(!isReplay, "Backward jump before scrobbling should not trigger replay (could be a seek)")
     }
+
+    // MARK: - Fix: Title/artist-based track change detection
+
+    @Test("Title change triggers track change detection even with same videoId")
+    func titleChangeDetectedWithSameVideoId() {
+        // When videoId is stale (reused from previous track), title/artist change
+        // should still be detected as a track change
+        let currentVideoId = "video123"
+        let currentTitle = "Song A"
+        let currentArtist = "Artist A"
+
+        let newVideoId = "video123" // Same stale videoId
+        let newTitle = "Song B"
+        let newArtist = "Artist B"
+
+        let videoIdChanged = newVideoId != currentVideoId
+        let metadataChanged = (newTitle != currentTitle || newArtist != currentArtist)
+
+        #expect(!videoIdChanged, "videoId should be the same (stale)")
+        #expect(metadataChanged, "Title/artist change should be detected")
+        #expect(videoIdChanged || metadataChanged, "Track change should be detected via metadata fallback")
+    }
+
+    @Test("Same title and artist does not trigger false track change")
+    func sameMetadataNoFalseChange() {
+        let currentVideoId = "video123"
+        let currentTitle = "Song A"
+        let currentArtist = "Artist A"
+
+        let newVideoId = "video123"
+        let newTitle = "Song A"
+        let newArtist = "Artist A"
+
+        let videoIdChanged = newVideoId != currentVideoId
+        let metadataChanged = (newTitle != currentTitle || newArtist != currentArtist)
+
+        #expect(!videoIdChanged)
+        #expect(!metadataChanged)
+        #expect(!(videoIdChanged || metadataChanged), "No change should be detected")
+    }
+
+    // MARK: - Fix: Final threshold check on finalize
+
+    @Test("Finalize performs threshold check before discarding play time")
+    func finalizeThresholdCheck() {
+        // If accumulated play time is at threshold when track changes,
+        // the finalize should trigger the scrobble before resetting state
+        let duration = 200.0
+        let percentThreshold = 0.5
+        let minSeconds: TimeInterval = 240
+        let accumulated: TimeInterval = 100.0 // Exactly 50%
+
+        let thresholdMet = accumulated >= duration * percentThreshold || accumulated >= minSeconds
+        #expect(thresholdMet, "Threshold should be met at exactly 50% of duration")
+    }
 }
