fix: Expose an authoritative track refresh - #169
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
This PR addresses a race where consumers of the public Player.events stream can receive .tracksChanged / .mediaChanged before Player’s mirrored audioTracks / videoTracks / subtitleTracks arrays have been refreshed, by exposing an authoritative refresh operation as public API.
Changes:
- Promotes
Player.refreshTracks()from internal topublicand adds API documentation describing the ordering caveat. - Adds a new test that verifies
refreshTracks()is callable viaimport SwiftVLCand leaves track arrays empty when no media is loaded.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/SwiftVLCTests/Player/PlayerPublicTrackRefreshTests.swift | Adds a public-API visibility test for Player.refreshTracks() without media. |
| Sources/SwiftVLC/Player/Player.swift | Makes refreshTracks() public and documents when/why public event consumers should call it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// The track arrays are asynchronously mirrored snapshots. Receiving the payload-free | ||
| /// ``PlayerEvent/tracksChanged`` or ``PlayerEvent/mediaChanged`` event does not guarantee | ||
| /// they are updated; consumers requiring an immediate native read should call this | ||
| /// method after either event. This also invalidates selected-track observations. |
| import SwiftVLC | ||
| import Testing | ||
|
|
||
| @Suite(.tags(.mainActor)) | ||
| @MainActor struct PlayerPublicTrackRefreshTests { | ||
| @Test | ||
| func `refreshTracks without media leaves public track lists empty`() { | ||
| let player = Player() | ||
|
|
||
| player.refreshTracks() | ||
|
|
||
| #expect(player.audioTracks.isEmpty) | ||
| #expect(player.videoTracks.isEmpty) | ||
| #expect(player.subtitleTracks.isEmpty) | ||
| } | ||
| } |
|
Thank you @mvanhorn 👏🏼 |
|
Appreciate the quick merge, @omaralbeik. Glad the authoritative track refresh fits the API shape you wanted. |
EventBridgeoffers the internal sourced event before the public event, but the independent async consumers can resume in either order. A public consumer can therefore receive.tracksChangedor.mediaChangedbeforePlayer's main-actor consumer has refreshedaudioTracks,videoTracks, andsubtitleTracks. Those event cases carry no track payload, and the existing nativerefreshTracks()operation is not public, so clients currently have no authoritative way to close that race. The issue is open, unassigned, maintainer-authored, and has no prior or competing pull request in the supplied evidence.Validation
refreshTracks()method and verify all three track arrays remain empty without failure.import SwiftVLC, compile and invokerefreshTracks()to guard the public API visibility required by external event consumers..tracksChangedand.mediaChangedhandler behavior: both continue to call the same refresh operation, fetching audio, video, and subtitle tracks and invalidating selected-track observations.refreshTracks()on the player actor provides the fresh-read path.Summary
Promote the existing main-actor
Player.refreshTracks()operation to public API instead of changingPlayerEventassociated values, which would force source changes across every event switch. Document that the observable track arrays are asynchronously mirrored snapshots and that raw-event consumers should callrefreshTracks()after.tracksChangedor.mediaChangedwhen they require an immediate native read. Keep the existing implementation and internal event-handler call sites as the single behavior path so public and automatic refreshes fetch all three track types and invalidate selected-track observations identically.Closes #72