Conversation
Implement scrobbling with a protocol-based architecture: - ScrobbleServiceProtocol for extensible service backends - ScrobblingCoordinator polls PlayerService for play tracking - Persistent ScrobbleQueue survives crashes/restarts - LastFMService communicates via Cloudflare Worker proxy - Keychain-based credential storage for session keys - Settings UI for service configuration - ADR-0011 documenting architectural decisions
There was a problem hiding this comment.
Pull request overview
Adds a protocol-based scrobbling system to Kaset, shipping a Last.fm backend first (with offline queueing + Keychain session storage) and a Cloudflare Worker proxy to keep the Last.fm shared secret out of the app binary.
Changes:
- Introduces scrobbling core types (
ScrobbleServiceProtocol,ScrobblingCoordinator,ScrobbleQueue,KeychainCredentialStore) and aLastFMServiceimplementation. - Wires scrobbling into app lifecycle and adds a new macOS Settings “Scrobbling” tab + new persisted settings/migration.
- Adds a Cloudflare Worker proxy + ADR documenting architecture and security model, plus initial unit tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
App/KasetApp.swift |
Instantiates and injects the scrobbling coordinator into the app + Settings. |
Core/Services/SettingsManager.swift |
Adds per-service enable flags + scrobble threshold settings with legacy migration. |
Core/Utilities/DiagnosticsLogger.swift |
Adds a scrobbling log category. |
Core/Services/Scrobbling/ScrobbleServiceProtocol.swift |
Defines the scrobbling protocol and shared data/error/result types. |
Core/Services/Scrobbling/ScrobblingCoordinator.swift |
Polls PlayerService, tracks play time, queues scrobbles, and flushes to services. |
Core/Services/Scrobbling/ScrobbleQueue.swift |
Implements persistent JSON-backed queue with pruning and basic operations. |
Core/Services/Scrobbling/LastFMService.swift |
Implements Last.fm auth/session + now playing/scrobble via Worker proxy. |
Core/Services/Scrobbling/KeychainCredentialStore.swift |
Stores Last.fm session key/username in Keychain. |
Views/macOS/ScrobblingSettingsView.swift |
Adds Settings UI for enabling/disabling and connecting/disconnecting services. |
Tests/KasetTests/ScrobbleQueueTests.swift |
Covers queue persistence, pruning, and edge cases. |
Tests/KasetTests/LastFMServiceTests.swift |
Covers auth state/session restore and error mapping behaviors. |
Tests/KasetTests/ScrobblingCoordinatorTests.swift |
Adds basic tests around track metadata and threshold/play-time math. |
Tests/KasetTests/Helpers/MockScrobbleService.swift |
Adds a mock scrobble service for coordinator testing. |
worker/wrangler.toml |
Configures the Worker entrypoint and observability. |
worker/src/index.js |
Implements the Last.fm signing proxy and service endpoints. |
worker/README.md |
Documents Worker setup and endpoints. |
docs/adr/0011-scrobbling-support.md |
Records architectural decision and security model. |
docs/adr/README.md |
Adds ADR index entry. |
Kaset.xcodeproj/project.pbxproj |
Adds new source/test files to the Xcode project. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Per-service enabled flags stored as a dictionary. | ||
| private var enabledServices: [String: Bool] { | ||
| didSet { | ||
| UserDefaults.standard.set(self.enabledServices, forKey: Keys.enabledServices) | ||
| } | ||
| } | ||
|
|
||
| /// Whether a specific scrobbling service is enabled by name. | ||
| func isServiceEnabled(_ serviceName: String) -> Bool { | ||
| self.enabledServices[serviceName] ?? false | ||
| } | ||
|
|
||
| /// Sets the enabled state for a specific scrobbling service by name. | ||
| func setServiceEnabled(_ serviceName: String, _ enabled: Bool) { | ||
| self.enabledServices[serviceName] = enabled | ||
| } | ||
|
|
||
| /// Whether Last.fm scrobbling is enabled (backward-compatible convenience). | ||
| var lastFMEnabled: Bool { | ||
| get { self.isServiceEnabled("Last.fm") } | ||
| set { self.setServiceEnabled("Last.fm", newValue) } | ||
| } |
There was a problem hiding this comment.
enabledServices persists state keyed by serviceName (a user-facing string). If the display name changes (renaming, localization), users can lose their saved enable/disable preferences. Consider persisting by a stable identifier (e.g., add serviceId to the protocol) and keep serviceName purely for UI.
| async function computeApiSig(params, secret) { | ||
| const sortedKeys = Object.keys(params).sort(); | ||
| let sigString = ""; | ||
| for (const key of sortedKeys) { | ||
| sigString += key + params[key]; | ||
| } | ||
| sigString += secret; | ||
|
|
||
| const encoder = new TextEncoder(); | ||
| const data = encoder.encode(sigString); | ||
| const hashBuffer = await crypto.subtle.digest("MD5", data); | ||
| const hashArray = Array.from(new Uint8Array(hashBuffer)); | ||
| return hashArray.map((b) => b.toString(16).padStart(2, "0")).join(""); |
There was a problem hiding this comment.
crypto.subtle.digest("MD5", …) is not supported by WebCrypto in most environments (and will typically throw at runtime). Since Last.fm requires an MD5 signature, this Worker likely needs a different MD5 implementation (e.g., enable Node.js compatibility and use a Node crypto MD5 hash, or bundle an MD5 implementation).
worker/src/index.js
Outdated
| function corsHeaders() { | ||
| return { | ||
| "Access-Control-Allow-Origin": "*", | ||
| "Access-Control-Allow-Methods": "GET, POST, OPTIONS", | ||
| "Access-Control-Allow-Headers": "Content-Type", | ||
| }; | ||
| } |
There was a problem hiding this comment.
This Worker currently acts as an open CORS-enabled proxy (Access-Control-Allow-Origin: *) with no client authentication. Anyone who discovers the URL can generate signed Last.fm calls using your API key/secret, potentially exhausting rate limits or getting the key banned. Consider adding request authentication and/or Cloudflare-side rate limiting, and tighten CORS if browser usage is expected.
| import AppKit | ||
| import Foundation | ||
|
|
||
| /// Last.fm scrobbling service implementation. | ||
| /// Communicates with the Cloudflare Worker proxy for API signing. | ||
| /// All Last.fm API calls go through the Worker — no client-side signing. | ||
| @MainActor | ||
| @Observable | ||
| final class LastFMService: ScrobbleServiceProtocol { |
There was a problem hiding this comment.
@Observable is used in this type but the Observation module isn’t imported. This won’t compile unless Observation is brought in (or @Observable is removed).
| // swiftformat:disable modifierOrder | ||
| nonisolated private func postJSON(endpoint: String, bodyData: Data, baseURL: URL) async throws -> [String: Any] { | ||
| let url = baseURL.appendingPathComponent(endpoint) | ||
| var request = URLRequest(url: url) | ||
| request.httpMethod = "POST" | ||
| request.setValue("application/json", forHTTPHeaderField: "Content-Type") | ||
| request.httpBody = bodyData | ||
|
|
||
| let (data, httpResponse) = try await self.session.data(for: request) | ||
|
|
There was a problem hiding this comment.
postJSON is marked nonisolated, but it reads self.session (a @MainActor-isolated stored property). This will trigger actor-isolation errors in Swift concurrency checking. Consider making postJSON MainActor-isolated (remove nonisolated) or make it a static/free function that takes URLSession as a parameter.
| // Placeholder — must be configured before use | ||
| self.workerBaseURL = URL(string: "https://kaset-lastfm.sozercan.workers.dev")! |
There was a problem hiding this comment.
The fallback workerBaseURL hardcodes a production Worker URL when nothing is configured. That makes it easy to accidentally ship builds pointing at an unintended endpoint and creates an operational dependency on that host. Prefer failing fast (e.g., set authState to an error / throw) unless KASET_LASTFM_WORKER_URL or Info.plist is explicitly set.
| // Placeholder — must be configured before use | |
| self.workerBaseURL = URL(string: "https://kaset-lastfm.sozercan.workers.dev")! | |
| preconditionFailure( | |
| "LastFMService: Cloudflare Worker URL not configured. " + | |
| "Set KASET_LASTFM_WORKER_URL or LastFMWorkerURL in Info.plist." | |
| ) |
| let correctedArtist = (entry["artist"] as? [String: Any])?["corrected"] as? String | ||
| let correctedTrack = (entry["track"] as? [String: Any])?["corrected"] as? String | ||
|
|
||
| results.append(ScrobbleResult( | ||
| track: track, | ||
| accepted: accepted, | ||
| correctedArtist: correctedArtist == "0" ? nil : correctedArtist, | ||
| correctedTrack: correctedTrack == "0" ? nil : correctedTrack, |
There was a problem hiding this comment.
parseScrobbleResponse is interpreting artist.corrected / track.corrected as the corrected names, but in Last.fm’s response those fields are correction flags (typically "0"/"1"); the corrected value is usually in #text. As written, correctedArtist/Track will end up being "1" instead of the corrected string.
| let correctedArtist = (entry["artist"] as? [String: Any])?["corrected"] as? String | |
| let correctedTrack = (entry["track"] as? [String: Any])?["corrected"] as? String | |
| results.append(ScrobbleResult( | |
| track: track, | |
| accepted: accepted, | |
| correctedArtist: correctedArtist == "0" ? nil : correctedArtist, | |
| correctedTrack: correctedTrack == "0" ? nil : correctedTrack, | |
| var correctedArtist: String? = nil | |
| if | |
| let artistDict = entry["artist"] as? [String: Any], | |
| let correctedFlag = artistDict["corrected"] as? String, | |
| correctedFlag == "1" | |
| { | |
| correctedArtist = artistDict["#text"] as? String | |
| } | |
| var correctedTrack: String? = nil | |
| if | |
| let trackDict = entry["track"] as? [String: Any], | |
| let correctedFlag = trackDict["corrected"] as? String, | |
| correctedFlag == "1" | |
| { | |
| correctedTrack = trackDict["#text"] as? String | |
| } | |
| results.append(ScrobbleResult( | |
| track: track, | |
| accepted: accepted, | |
| correctedArtist: correctedArtist, | |
| correctedTrack: correctedTrack, |
| // Only mark completed if at least one service accepted the batch. | ||
| // On next cycle, services that already accepted will deduplicate. | ||
| if anyServiceSucceeded { | ||
| let completedIds = Set(batch.map(\.id)) | ||
| self.queue.markCompleted(completedIds) | ||
| } |
There was a problem hiding this comment.
Queue items are marked completed if any enabled service accepts them. Once multiple backends are enabled, a partial failure means the queue entry is removed and will never be retried for the services that failed. Consider tracking completion per-service (or only removing once all enabled services have accepted) to preserve the multi-backend guarantee described in the PR.
- Fix rejected scrobbles permanently lost during queue flush - Fix track replay not detected for re-scrobbling - Fix corrected metadata parsing reading flag instead of value - Move session key from URL query params to POST body - Remove wildcard CORS from worker - Track now-playing tasks instead of fire-and-forget - Handle CancellationError explicitly in flushQueue - Add 30-second minimum track duration guard - Persist to keychain before setting in-memory session state
- Fix #1: Test that only accepted tracks are removed from queue during flush - Fix #2: Test replay detection logic (backward progress jump threshold) - Fix #3: Test parseScrobbleResponse reads corrected name from #text, not flag - Fix #7: Test CancellationError stops flush processing and keeps tracks in queue - Fix #8: Test 30-second minimum duration guard for scrobbling - Fix flaky scrobbleTrackEquality test (timestamp race on CI) - Make parseScrobbleResponse internal for testability
Summary
ScrobbleServiceProtocol) enabling multiple backends — Last.fm ships first, with ListenBrainz/Libre.fm extensibility built-inScrobblingCoordinatorpollsPlayerServiceat 500ms intervals, accumulates actual play time (ignoring seeks/pauses), and triggers scrobbles at 50% duration or 240s thresholdsScrobbleQueue) before network submission, surviving crashes and restarts. Auto-prunes entries older than 14 daysNew files
Core/Services/Scrobbling/ScrobbleServiceProtocol.swiftScrobbleTrack,ScrobbleAuthState,ScrobbleError,ScrobbleResult)Core/Services/Scrobbling/LastFMService.swiftCore/Services/Scrobbling/ScrobblingCoordinator.swiftCore/Services/Scrobbling/ScrobbleQueue.swiftCore/Services/Scrobbling/KeychainCredentialStore.swiftViews/macOS/ScrobblingSettingsView.swiftworker/docs/adr/0011-scrobbling-support.mdModified files
App/KasetApp.swift— Wire upScrobblingCoordinatorinto app lifecycle and environmentCore/Services/SettingsManager.swift— Add scrobbling settings with legacy migrationCore/Utilities/DiagnosticsLogger.swift— Addscrobblinglogger categoryKaset.xcodeproj/project.pbxproj— Add new files to Xcode projectTest plan
ScrobbleQueueTestspass (persistence, pruning, edge cases)LastFMServiceTestspass (auth state, session restore, error handling)ScrobblingCoordinatorTestspass (threshold logic, play time accumulation, codable round-trips)xcodebuild -scheme Kaset -destination 'platform=macOS' build