Incorporate changes from New Streaming Player#83
Conversation
Introduces StreamingStationPlayer as an alternative to PlayolaStationPlayer that streams audio via AVPlayer instead of downloading entire files first. This significantly reduces startup latency for long audio files. New files: - FadeScheduleBuilder: pure fade schedule generation (shared utility) - AVPlayerProviding: protocol wrapping AVPlayer for testability - StreamingSpinPlayer: AVPlayer-based single spin playback - StreamingStationPlayer: orchestrator with schedule polling - ScheduleService: extracted shared schedule-fetching logic Modified: - ListeningSessionReporter: added publisher-based init for both players 37 new tests across 3 test suites, all passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add player mode picker (Streaming vs Download) to ContentView - Stop current player before switching stations - Hard-code default station ID for testing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace 3-member tuple return with PlayerTestContext struct - Use `where` clause instead of `if` inside `for` loop Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- P1: AVPlayerWrapper.waitForReady now keeps observing on .unknown status instead of leaking the continuation; throws on @unknown default - P1: StreamingSpinPlayer.playNow guards state after await seek to prevent clear() race from leaving player in .playing with dangling timers - P1: StreamingStationPlayer adds deinit to remove NotificationCenter observers, preventing retain cycle / memory leak - P2: stopCurrentPlayer() is now async; callers await it before starting new playback to prevent stop/play races Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- P1: StreamingStationPlayer transitions to .idle when last spin finishes and spinPlayers is empty - P1: Change @ObservedObject to @StateObject for streamingPlayer in ContentView so SwiftUI owns its lifecycle - P2: FadeScheduleBuilder skips overlapping steps to ensure monotonic time ordering when fades are closer than fadeDurationMS - Add test for overlapping fade edge case Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
After awaiting player.load(), check that stationId is still set before proceeding to playback. If stop() ran during the suspension, clean up the player instead of overwriting the .idle state. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add AVPlayer-based streaming player
Greptile SummaryThis PR introduces the new Key changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as ContentView
participant SSP as StreamingStationPlayer
participant SpinP as StreamingSpinPlayer
participant AVW as AVPlayerWrapper
participant LSR as ListeningSessionReporter
participant API as Playola API
UI->>SSP: play(stationId:)
SSP->>API: getSchedule(stationId:)
API-->>SSP: Schedule (spins)
SSP->>SpinP: load(spin)
SpinP->>AVW: loadURL(url)
AVW->>AVW: waitForReady (KVO on AVPlayerItem.status)
AVW-->>SpinP: .success / .failure
SpinP-->>SSP: Result
SSP->>SpinP: playNow(from: elapsedSeconds)
SpinP->>AVW: seek + play()
SpinP-->>SSP: delegate.startedPlaying(spin)
SSP->>SSP: state = .playing(spin)
SSP-->>UI: @Published state update
Note over SSP: $stationId publishes new value
SSP->>LSR: stationIdPublisher emits stationId
LSR->>API: POST /v1/listeningSessions
API-->>LSR: 200 OK
LSR->>LSR: startPeriodicNotifications (every 10s)
loop Every 20s (schedule polling)
SSP->>API: getSchedule(stationId:)
API-->>SSP: Updated schedule
SSP->>SpinP: loadAndPlaySpin (future spins)
end
UI->>SSP: stop()
SSP->>SpinP: stop() → clear()
SSP->>SSP: stationId = nil
SSP-->>LSR: stationIdPublisher emits nil
LSR->>API: POST /v1/listeningSessions/end
LSR->>LSR: stopPeriodicNotifications
Last reviewed commit: "Merge pull request #..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Void, Error>) in | ||
| self.statusObservation = item.observe(\.status, options: [.new]) { | ||
| [weak self] observedItem, _ in | ||
| guard let self else { return } | ||
|
|
||
| Task { @MainActor in | ||
| switch observedItem.status { | ||
| case .readyToPlay: | ||
| self.statusObservation?.invalidate() | ||
| self.statusObservation = nil | ||
| continuation.resume() | ||
| case .failed: | ||
| self.statusObservation?.invalidate() | ||
| self.statusObservation = nil | ||
| let error = | ||
| observedItem.error | ||
| ?? StationPlayerError.playbackError("AVPlayerItem failed to load") | ||
| continuation.resume(throwing: error) | ||
| case .unknown: | ||
| // Not yet determined — keep observing. | ||
| return | ||
| @unknown default: | ||
| self.statusObservation?.invalidate() | ||
| self.statusObservation = nil | ||
| continuation.resume( | ||
| throwing: StationPlayerError.playbackError( | ||
| "AVPlayerItem entered unexpected status")) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
self is nil
The KVO callback captures self weakly and returns early via guard let self else { return } if the AVPlayerWrapper has been deallocated. In that case, the CheckedContinuation is never resumed — leaving the awaiting loadURL caller suspended indefinitely (or until its enclosing Task is cancelled).
While in practice AVPlayerWrapper is kept alive by the local player variable inside StreamingSpinPlayer.load(_:) for the duration of the await, the pattern is fragile. If clear() is ever called before the observation fires, the local player variable is the only remaining reference, and any intervening refactoring that changes ownership could silently introduce a hang.
A safer approach is to use strong (unowned) capture of the continuation, or store it in an Optional that is nilled after first use:
private func waitForReady(item: AVPlayerItem) async throws {
try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Void, Error>) in
self.statusObservation = item.observe(\.status, options: [.new]) {
[weak self] observedItem, _ in
// Invalidate synchronously to prevent potential double-fire
self?.statusObservation?.invalidate()
self?.statusObservation = nil
switch observedItem.status {
case .readyToPlay:
continuation.resume()
case .failed:
let error = observedItem.error
?? StationPlayerError.playbackError("AVPlayerItem failed to load")
continuation.resume(throwing: error)
case .unknown:
return
@unknown default:
continuation.resume(
throwing: StationPlayerError.playbackError(
"AVPlayerItem entered unexpected status"))
}
}
}
}This also removes the Task { @MainActor in } wrapper from the callback, eliminating the (admittedly small) window where a second KVO event could enqueue a second Task that also attempts to resume an already-consumed continuation.
| @@ -401,7 +481,6 @@ struct StationPickerView: View { | |||
| }, | |||
There was a problem hiding this comment.
dismiss() fires before playback completes — errors are silently dropped
dismiss() is called synchronously outside the Task block, so the sheet is dismissed immediately when the user taps a station — before streamingPlayer.play() or PlayolaStationPlayer.shared.play() even begins executing. If either play() call throws (e.g. due to a network error), the error is silently consumed by the print(...) and the user receives no feedback.
If surfacing playback errors to the user is desirable, consider moving dismiss() inside the Task (before the do/catch) so the sheet stays visible until playback actually starts:
Task {
do {
let stationId = station.playolaID ?? station.id
selectedStationId = stationId
switch playerMode {
case .streaming:
streamingPlayer.stop()
try await streamingPlayer.play(stationId: stationId)
case .download:
await PlayolaStationPlayer.shared.stop()
try await PlayolaStationPlayer.shared.play(stationId: stationId)
}
dismiss()
} catch {
// TODO: surface error to user
print("Failed to start playback: \(error)")
}
}| func stopCurrentPlayer() async { | ||
| await downloadPlayer.stop() | ||
| streamingPlayer.stop() | ||
| } |
There was a problem hiding this comment.
stopCurrentPlayer() always stops both players, not just the active one
Regardless of the current playerMode, both downloadPlayer.stop() and streamingPlayer.stop() are always called. This is harmless but creates unnecessary calls on the inactive player each time mode is switched or the stop button is tapped. Stopping only the player that corresponds to the current mode would be cleaner:
func stopCurrentPlayer() async {
switch playerMode {
case .streaming:
streamingPlayer.stop()
case .download:
await downloadPlayer.stop()
}
}If the intent is specifically to stop both players when switching modes (e.g., via onChange(of: playerMode)), that call site could explicitly stop both, making the invariant explicit.
| public func configure( | ||
| authProvider: PlayolaAuthenticationProvider, | ||
| baseURL: URL = URL(string: "https://admin-api.playola.fm")! | ||
| ) { | ||
| self.authProvider = authProvider | ||
| self.baseUrl = baseURL | ||
| self.listeningSessionReporter = ListeningSessionReporter( | ||
| stationIdPublisher: $stationId.eraseToAnyPublisher(), | ||
| stationIdGetter: { [weak self] in self?.stationId }, | ||
| authProvider: authProvider, | ||
| baseURL: baseURL | ||
| ) | ||
| } |
There was a problem hiding this comment.
configure() must be called before play() for listening sessions to work — no enforcement
ListeningSessionReporter is created in configure() and subscribes to $stationId changes via publisher.sink. If play(stationId:) is called before configure(), stationId is already set when the reporter is created, and the sink receives no initial value — so the first listening session is never reported.
A doc comment or runtime precondition would help callers avoid this silent failure. For example:
/// Configure with authentication provider.
///
/// - Important: Must be called before `play(stationId:)` to ensure
/// listening sessions are reported correctly.
public func configure(
authProvider: PlayolaAuthenticationProvider,
baseURL: URL = URL(string: "https://admin-api.playola.fm")!
) {Alternatively, the reporter could use $stationId.prepend(stationId) so it captures the current value at subscription time and handles both orderings.
1 participant
This pull request introduces significant improvements to the PlayolaPlayer example app, primarily by adding a toggle between streaming and download playback modes, refactoring the main view logic to support both modes, and enhancing the flexibility and testability of the listening session reporting infrastructure. Additional code cleanup and UI improvements are also included.
Major feature: Dual playback mode support
PlayerModeenum and UI toggle to switch between streaming (AVPlayer) and download (AVAudioEngine) playback modes inContentView.swift, with logic updates throughout the view to support both player types. [1] [2] [3] [4]playOrPause,playWithOffset, etc.) and UI feedback (button color, icon, album art animation) to reflect the selected playback mode and player state. [1] [2] [3] [4] [5]Station selection and playback integration
StationPickerViewto support both playback modes, allowing users to start playback in the selected mode directly from the station picker. [1] [2]Listening session infrastructure improvements
ListeningSessionReporterto decouple from a concretePlayolaStationPlayerby introducing astationIdGetterclosure and an alternate initializer that accepts a publisher and getter, improving testability and flexibility. [1] [2] [3] [4]Code cleanup and UI polish
ContentView.swiftfor clarity and maintainability.Documentation update
CLAUDE.md.