Skip to content

0.16.0#88

Merged
briankeane merged 7 commits into
mainfrom
develop
Apr 22, 2026
Merged

0.16.0#88
briankeane merged 7 commits into
mainfrom
develop

Conversation

@briankeane

Copy link
Copy Markdown
Collaborator

This pull request refactors the example app's playback UI and logic to remove the streaming/download player mode toggle, consolidating playback to use only PlayolaStationPlayer. It also cleans up related state management, simplifies the station picker, and improves code readability and maintainability. Additionally, a detailed markdown document is added to explain the previously used streaming approach.

Player Mode Removal and Simplification:

  • Removed the PlayerMode enum and all logic related to toggling between streaming and download playback modes in ContentView.swift. The app now uses only PlayolaStationPlayer for playback, simplifying state management and UI. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]

  • Refactored playback control logic (playOrPause, playWithOffset) to operate exclusively on PlayolaStationPlayer, removing all branching on player mode and related helper methods. [1] [2]

UI and Code Improvements:

  • Simplified the station picker (StationPickerView) by removing dependencies on player mode and the streaming player, and improved station selection logic and image display. [1] [2] [3] [4]

  • Added helper functions for button color, icon, and offset based on player state, making the main view code cleaner and more maintainable.

  • Added clarifying comments and improved code readability throughout, including more descriptive variable names and UI explanations. [1] [2] [3] [4] [5] [6] [7]

Documentation:

  • Added STREAMING_APPROACH.md, a comprehensive document describing the AVPlayer-based streaming approach from version 0.15.0, including architecture, playback flow, fade handling, and the rationale for reverting to the download-based approach.

Minor Codebase Cleanup:

  • Removed an unused closure property from ListeningSessionReporter that was no longer needed after the refactor.

briankeane and others added 7 commits March 20, 2026 22:09
Incorporate changes from New Streaming Player
play(stationId:) now clears existing spin players before starting fresh,
preventing stale AVPlayers from being reused after audio interruptions.
Removed misleading interruption state saves from headphone disconnect
handler and dead isSuspended property. Added scheduleFetcher injection
for testability and 8 new tests covering all interruption codepaths.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Revert all source code to the stable 0.14.0 state, removing the
streaming player port (0.15.0) which introduced issues. Add
STREAMING_APPROACH.md documenting the AVPlayer-based streaming
architecture for future reference.
Revert to 0.14.0 and document streaming approach
* Fix app hang by awaiting audio session before engine start

The Sentry hang (2000ms+) was caused by AVAudioEngine.start() being
called before the audio session was configured. configureAudioSession()
fired a Task (fire-and-forget) then engine.start() ran immediately,
causing the engine to stall on the main thread.

Add ensureAudioSessionConfigured() async method to PlayolaMainMixer
and await it in handleSuccessfulDownload before playback begins. Also
remove the Thread.sleep retry loop from start() which was masking the
unconfigured session issue while blocking the main thread.

* Handle audio session config failure and add precondition asserts

Replace try? with proper error handling in handleSuccessfulDownload so
a failed audio session config bails out instead of proceeding to
engine.start() with an unconfigured session.

Add assertions in playNow and prepareAudioEngine to catch direct
callers that skip ensureAudioSessionConfigured().

* Add .context to .gitignore
@greptile-apps

greptile-apps Bot commented Apr 7, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a 2000ms+ main-thread hang by properly awaiting audio session configuration before starting AVAudioEngine. The core fix is the new ensureAudioSessionConfigured() async method in PlayolaMainMixer that replaces the fire-and-forget Task approach, and is explicitly await-ed in handleSuccessfulDownload before any engine operation. The retry loop in start() that was masking the root cause is correctly removed. The example app is simultaneously simplified to drop the streaming/download toggle and use only PlayolaStationPlayer.

Confidence Score: 5/5

Safe to merge — the fix is well-targeted and all remaining findings are P2 style suggestions.

The root-cause fix (awaiting audio session config before engine start) is correct and properly threaded through the call chain. The retry-loop removal is justified. Both P2 comments are acknowledged in the code and do not introduce regressions relative to the previous state. No blocking issues found.

SpinPlayer.swift — the assert + fire-and-forget pattern in playNow and prepareAudioEngine is a minor clarity issue worth a follow-up cleanup.

Vulnerabilities

No security concerns identified.

Important Files Changed

Filename Overview
Sources/PlayolaPlayer/Player/PlayolaMainMixer.swift Adds ensureAudioSessionConfigured() async method; removes the retry-loop from start(). Both changes are well-reasoned and clean.
Sources/PlayolaPlayer/Player/SpinPlayer.swift Core fix (awaiting ensureAudioSessionConfigured() before engine ops) is correct; the assert + fire-and-forget pattern in playNow and prepareAudioEngine may give misleading false-safety in Release builds.
PlayolaPlayerExample/PlayolaPlayerExample/ContentView.swift Clean simplification removing the streaming/download mode toggle; playback logic consolidated to PlayolaStationPlayer only.
Sources/PlayolaPlayer/Player/ListeningSessionReporter.swift Minor cleanup — unused closure property removed; no functional changes.
STREAMING_APPROACH.md New document archiving the AVPlayer-based streaming approach and the rationale for reverting to download-based playback.

Sequence Diagram

sequenceDiagram
    participant SP as SpinPlayer
    participant MM as PlayolaMainMixer
    participant AS as AudioSessionManager
    participant AE as AVAudioEngine

    SP->>SP: handleSuccessfulDownload()
    SP->>MM: ensureAudioSessionConfigured() [await]
    MM->>AS: configureForPlayback() [await]
    MM->>AS: activate() [await]
    AS-->>MM: configured ✓
    MM-->>SP: returns (or throws)
    alt spin.playbackTiming == .playing
        SP->>SP: playNow(from:)
        SP->>MM: assert(isConfigured)
        SP->>AE: engine.start()
    else spin.playbackTiming == .future
        SP->>SP: schedulePlay(at:)
        SP->>SP: prepareAudioEngine()
        SP->>MM: assert(isConfigured)
        SP->>AE: engine.start()
    end
Loading

Reviews (1): Last reviewed commit: "Fix app hang by awaiting audio session b..." | Re-trigger Greptile

Comment on lines 364 to 371
self.playbackStartOffset = from
// Make sure audio session is configured before playback
// Callers must await ensureAudioSessionConfigured() before calling playNow.
// The fire-and-forget fallback avoids a crash but may still race.
assert(
playolaMainMixer.audioSessionManager.isConfigured,
"Audio session must be configured before calling playNow — call ensureAudioSessionConfigured() first"
)
playolaMainMixer.configureAudioSession()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Fire-and-forget after assert may give false confidence in Release

The assert catches the unconfigured session in Debug builds. But in Release builds (assert disabled), calling configureAudioSession() (fire-and-forget) and then engine.start() immediately after preserves the same race condition this PR set out to fix — the Task spawned by configureAudioSession() may not finish before engine.start() runs.

Since ensureAudioSessionConfigured() is now properly awaited in handleSuccessfulDownload before reaching this call site, the fire-and-forget fallback here is effectively dead code on the happy path (configureAudioSession early-returns when already configured). Consider removing it and relying solely on the assert to alert developers of a missed ensureAudioSessionConfigured() call, or convert it to a preconditionFailure to crash in both Debug and Release when the invariant is violated.

Comment on lines +868 to 874
// Callers must await ensureAudioSessionConfigured() before calling schedulePlay.
assert(
playolaMainMixer.audioSessionManager.isConfigured,
"Audio session must be configured before calling schedulePlay — call ensureAudioSessionConfigured() first"
)
playolaMainMixer.configureAudioSession()
try engine.start()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Same assert + fire-and-forget pattern in prepareAudioEngine

Mirrors the concern in playNow: the configureAudioSession() call after the assert is a no-op in the normal path (session already configured) and a racing fallback in Release builds where the assert is elided. Consider removing the configureAudioSession() call here as well, since ensureAudioSessionConfigured() is awaited earlier in the call chain.

@briankeane
briankeane merged commit eaa9487 into main Apr 22, 2026
5 checks passed
briankeane added a commit that referenced this pull request May 24, 2026
The 0.16.0 release (PR #88) was squash-merged into main, which broke
git's view of shared history. Conflicts arose in PlayolaMainMixer.swift
and SpinPlayer.swift because develop has since evolved those files
(PR #89 made PlayolaMainMixer.start async, and the recent P2 fix added
off-main engine start to handleSuccessfulDownload).

Resolved by taking develop's version in both files — develop has
everything main has plus the newer fixes. Also restored the P1 baseUrl
fix in PlayolaStationPlayer.swift that the auto-merge silently reverted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant