Skip to content

0.17.1#94

Merged
briankeane merged 3 commits into
mainfrom
develop
Jun 4, 2026
Merged

0.17.1#94
briankeane merged 3 commits into
mainfrom
develop

Conversation

@briankeane

Copy link
Copy Markdown
Collaborator

No description provided.

Exposes a public, opt-in logging hook so a consuming app can observe the
library's JSON API request/response traffic for remote debugging. The
library emits raw events only — it does not redact, persist, or format.

- PlayolaNetworkLogEvent: Sendable event describing one request/response
- PlayolaNetworkLogger.handler: thread-safe (OSAllocatedUnfairLock) hook,
  nil by default (no logging, unchanged behavior/perf)
- PlayolaNetworkLoggingSession: transparent URLSessionProtocol wrapper that
  times each call and emits on success and thrown error without altering
  return values or rethrown errors

Wired into the schedule fetch (PlayolaStationPlayer) and the listening-session
reporter (ListeningSessionReporter) by default. Audio downloads
(FileDownloaderAsync) are intentionally excluded. No breaking API changes.
Addresses PR review (P2): the wrapper crosses actor boundaries
(@mainactor PlayolaStationPlayer awaits data(for:)), so declare explicit
Sendable conformance. The sound fix constrains the wrapped base to
'any URLSessionProtocol & Sendable' (URLSession already conforms) rather
than papering over a non-Sendable stored property. Test doubles passed to
the wrapper are marked Sendable accordingly.
…seam

Add network-logging seam for JSON API traffic
@briankeane
briankeane merged commit ecd9b19 into main Jun 4, 2026
5 checks passed
@greptile-apps

greptile-apps Bot commented Jun 4, 2026

Copy link
Copy Markdown

Greptile Summary

This PR introduces a transparent network-logging seam for the library's JSON API traffic. Three new public types are added to PlayolaCorePlayolaNetworkLogEvent, PlayolaNetworkLogger, and PlayolaNetworkLoggingSession — and both PlayolaStationPlayer and ListeningSessionReporter are updated to route their HTTP calls through the new wrapper by default.

  • PlayolaNetworkLogger.handler is a thread-safe (OSAllocatedUnfairLock) optional closure the consuming app sets at startup; nil (the default) disables all logging with negligible overhead.
  • PlayolaNetworkLoggingSession is a Sendable struct that wraps any URLSessionProtocol, times each call, and emits a PlayolaNetworkLogEvent on both success and error — returning/rethrowing results unchanged.
  • The CHANGELOG lists the change under ## 0.18.0 while the PR title says 0.17.1; these need to align before the git tag is pushed so consumers see consistent release notes.

Confidence Score: 4/5

The networking and logging changes are transparent pass-throughs with correct thread safety; the only actionable item before merging is reconciling the version number between the PR title and the CHANGELOG entry.

The logging wrapper is well-implemented and the existing test suite is thorough. The version discrepancy between the PR title (0.17.1) and the CHANGELOG (0.18.0) is a real release-process issue — whichever string the git tag is cut from will mismatch the other, giving consumers a confusing release. Everything else is clean.

CHANGELOG.md — version number must be aligned with the PR title (or vice-versa) before tagging.

Important Files Changed

Filename Overview
CHANGELOG.md New changelog file, but the version entry 0.18.0 conflicts with the PR title 0.17.1.
Sources/PlayolaCore/PlayolaNetworkLogger.swift New public logging infrastructure: PlayolaNetworkLogEvent value type and PlayolaNetworkLogger enum with thread-safe OSAllocatedUnfairLock-backed handler. Well-structured and Sendable-clean.
Sources/PlayolaCore/Protocols/PlayolaNetworkLoggingSession.swift New transparent URLSessionProtocol wrapper that times each call and emits log events. Correctly passes through both data and errors unchanged; guard on nil handler is efficient.
Sources/PlayolaPlayer/Player/ListeningSessionReporter.swift Default urlSession parameter updated to wrap tls12Session in PlayolaNetworkLoggingSession; change is applied to both the production and DEBUG initializers.
Sources/PlayolaPlayer/Player/PlayolaStationPlayer.swift Schedule fetch now routes through PlayolaNetworkLoggingSession; a new instance is created per call (struct, so lightweight), but the session cannot be overridden via injection unlike ListeningSessionReporter.
Tests/PlayolaPlayerTests/PlayolaNetworkLoggingSessionTests.swift Good test coverage: success path, error pass-through, handler-nil no-op, and transparent data pass-through. QueueDateProvider enables exact durationMS assertions.
Tests/PlayolaPlayerTests/Test Utilities/MockUrlSession.swift Made final and @unchecked Sendable to satisfy the new any URLSessionProtocol & Sendable requirement; mutable state is unprotected but the test suite is .serialized.

Sequence Diagram

sequenceDiagram
    participant App
    participant PlayolaStationPlayer
    participant ListeningSessionReporter
    participant PlayolaNetworkLoggingSession
    participant tls12Session as URLSession (tls12)
    participant PlayolaNetworkLogger

    Note over App: Set handler at startup
    App->>PlayolaNetworkLogger: "handler = closure"

    Note over PlayolaStationPlayer: Schedule fetch
    PlayolaStationPlayer->>PlayolaNetworkLoggingSession: data(for: URLRequest(url:))
    PlayolaNetworkLoggingSession->>tls12Session: data(for: request)
    tls12Session-->>PlayolaNetworkLoggingSession: (Data, URLResponse) or throws
    PlayolaNetworkLoggingSession->>PlayolaNetworkLogger: handler?(PlayolaNetworkLogEvent)
    PlayolaNetworkLogger-->>App: event (method, url, status, duration, headers, body)
    PlayolaNetworkLoggingSession-->>PlayolaStationPlayer: (Data, URLResponse) unchanged

    Note over ListeningSessionReporter: Listening session report
    ListeningSessionReporter->>PlayolaNetworkLoggingSession: data(for: request)
    PlayolaNetworkLoggingSession->>tls12Session: data(for: request)
    tls12Session-->>PlayolaNetworkLoggingSession: (Data, URLResponse) or throws
    PlayolaNetworkLoggingSession->>PlayolaNetworkLogger: handler?(PlayolaNetworkLogEvent)
    PlayolaNetworkLogger-->>App: event
    PlayolaNetworkLoggingSession-->>ListeningSessionReporter: (Data, URLResponse) unchanged
Loading

Reviews (1): Last reviewed commit: "Merge pull request #93 from playola-radi..." | Re-trigger Greptile

Comment thread CHANGELOG.md
which Swift Package Manager consumers pin to.

## 0.18.0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 CHANGELOG version conflicts with PR title

The changelog entry is ## 0.18.0 but the PR is titled 0.17.1. Since this change introduces three new public types (PlayolaNetworkLogEvent, PlayolaNetworkLogger, PlayolaNetworkLoggingSession) it qualifies as a minor version bump under SemVer, making 0.18.0 the semantically correct version — but the PR title and any git tag should match. One of the two needs to be updated before the tag is pushed, otherwise consumers pinning to the version string will get mismatched release notes.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +405 to +406
let loggingSession = PlayolaNetworkLoggingSession(wrapping: tls12Session)
let (data, response) = try await loggingSession.data(for: URLRequest(url: url))

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 Logging session created per call, not injectable

A fresh PlayolaNetworkLoggingSession is constructed on every schedule fetch. PlayolaNetworkLoggingSession is a lightweight struct so this is not a performance concern, but it means there is no way to swap the session out in tests the way ListeningSessionReporter allows via its urlSession parameter. If testability for this specific request path ever matters, consider storing an injectable urlSession property on PlayolaStationPlayer and assigning PlayolaNetworkLoggingSession(wrapping: tls12Session) as its default, mirroring the pattern already used in ListeningSessionReporter.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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