fix: implement AirPlay support via WebKit's native picker #83
Conversation
Replace non-functional AVRoutePickerView with WebKit's native webkitShowPlaybackTargetPicker() which actually routes WebView audio. Changes: - Add showAirPlayPicker() to SingletonPlayerWebView+PlaybackControls - Add AirPlay state tracking via webkitcurrentplaybacktargetiswirelesschanged - Replace AVRoutePickerView with custom button in PlayerBar - Add AirPlay-related properties to PlayerService - Add ADR-0010 documenting the implementation and known limitations Known limitations (documented in ADR): - AirPlay disconnects on track change (WebKit limitation) - Picker appears top-left due to hidden video element Closes #42 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add error logging to showAirPlayPicker() using DiagnosticsLogger.airplay - Remove unused checkAirPlayStatus() method - Add visual feedback when AirPlay connected (blue icon, animated transition) - Update accessibility label based on connection state
There was a problem hiding this comment.
Pull request overview
This PR fixes non-functional AirPlay support by replacing AVRoutePickerView with WebKit's native picker API. The issue (#42) reported that the in-app AirPlay button showed devices but didn't actually route audio to them.
Changes:
- Replaces AVRoutePickerView with JavaScript injection calling
webkitShowPlaybackTargetPicker()on the video element - Adds AirPlay state tracking via
webkitcurrentplaybacktargetiswirelesschangedevent listener in the observer script - Adds ADR-0010 documenting the implementation approach and known limitations (AirPlay disconnects on track changes)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/adr/0010-airplay-fix.md | Documents the architectural decision, root cause analysis, implementation steps, and known limitations |
| Views/macOS/SingletonPlayerWebView+PlaybackControls.swift | Adds showAirPlayPicker() and checkAirPlayStatus() methods for WebKit AirPlay API access |
| Views/macOS/SingletonPlayerWebView+ObserverScript.swift | Adds JavaScript event listener for AirPlay state changes and sends status updates to Swift |
| Views/macOS/PlayerBar.swift | Replaces AVRoutePickerView with custom Button that calls showAirPlayPicker() |
| Views/macOS/MiniPlayerWebView.swift | Updates message handler to process AIRPLAY_STATUS messages and route to PlayerService |
| Core/Utilities/DiagnosticsLogger.swift | Adds dedicated AirPlay logger for future debugging |
| Core/Utilities/AccessibilityIdentifiers.swift | Adds accessibility identifier for the AirPlay button |
| Core/Services/Player/PlayerService.swift | Adds showAirPlayPicker() method and AirPlay state properties (isAirPlayConnected, airPlayWasRequested) |
Comments suppressed due to low confidence (2)
Views/macOS/MiniPlayerWebView.swift:416
- The blank line added here appears unnecessary and inconsistent with the surrounding code style. The closing brace at line 417 already provides clear visual separation.
}
Views/macOS/SingletonPlayerWebView+PlaybackControls.swift:193
- The checkAirPlayStatus method is defined but never called in the codebase. If this method is not currently needed, consider removing it to keep the codebase clean. Otherwise, add a comment explaining why it's being kept for future use.
webView.evaluateJavaScript(script) { result, error in
if let error {
DiagnosticsLogger.airplay.error("showAirPlayPicker error: \(error.localizedDescription)")
} else if let status = result as? String {
switch status {
case "no-video":
DiagnosticsLogger.airplay.warning("showAirPlayPicker: no video element available")
case "unsupported":
DiagnosticsLogger.airplay.warning("showAirPlayPicker: webkitShowPlaybackTargetPicker not supported")
default:
DiagnosticsLogger.airplay.debug("showAirPlayPicker: \(status)")
}
}
}
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Updates the AirPlay connection status from the WebView. | ||
| func updateAirPlayStatus(isConnected: Bool, wasRequested: Bool = false) { | ||
| self.isAirPlayConnected = isConnected | ||
| if wasRequested { | ||
| self.airPlayWasRequested = true | ||
| } | ||
| } |
There was a problem hiding this comment.
The updateAirPlayStatus method lacks logging for status changes, which is inconsistent with similar update methods in this file. For example, updateVideoAvailability() logs changes at line 453. Consider adding logging to track when AirPlay connections are established or lost, using either DiagnosticsLogger.player or the newly added DiagnosticsLogger.airplay.
| /// Whether AirPlay is currently connected (playing to a wireless target). | ||
| private(set) var isAirPlayConnected: Bool = false | ||
|
|
||
| /// Whether the user has requested AirPlay this session (for persistence across track changes). | ||
| private(set) var airPlayWasRequested: Bool = false | ||
|
|
There was a problem hiding this comment.
The properties isAirPlayConnected and airPlayWasRequested are defined but never read anywhere in the codebase. If these properties are intended for future use or debugging, consider adding comments explaining their purpose. Otherwise, consider whether they're needed at all, or if they should be used to provide visual feedback in the UI (e.g., showing different icons when AirPlay is connected).
| /// Whether AirPlay is currently connected (playing to a wireless target). | |
| private(set) var isAirPlayConnected: Bool = false | |
| /// Whether the user has requested AirPlay this session (for persistence across track changes). | |
| private(set) var airPlayWasRequested: Bool = false |
| /// Show the AirPlay picker for selecting audio output devices. | ||
| func showAirPlayPicker() { | ||
| self.airPlayWasRequested = true | ||
| SingletonPlayerWebView.shared.showAirPlayPicker() | ||
| } |
There was a problem hiding this comment.
Consider adding test coverage for the new AirPlay functionality. The PlayerServiceTests file shows comprehensive testing of other PlayerService methods. Tests could verify that showAirPlayPicker() sets airPlayWasRequested to true, and that updateAirPlayStatus() correctly updates the isAirPlayConnected property.
| static let scripting = Logger(subsystem: "com.sertacozercan.Kaset", category: "Scripting") | ||
|
|
||
| /// Logger for AirPlay-related events. | ||
| static let airplay = Logger(subsystem: "com.sertacozercan.Kaset", category: "AirPlay") |
There was a problem hiding this comment.
The new AirPlay logger is defined but never used in the codebase. Consider adding logging to the AirPlay-related methods for consistency with other parts of the codebase. For example, log when AirPlay status changes in updateAirPlayStatus() method in PlayerService.swift, similar to how updateVideoAvailability() logs status changes at line 453 of PlayerService.swift.
| static let airplay = Logger(subsystem: "com.sertacozercan.Kaset", category: "AirPlay") | |
| static let airplay = Logger(subsystem: "com.sertacozercan.Kaset", category: "AirPlay") | |
| /// Logs a change in AirPlay status, for use by AirPlay-related services. | |
| /// | |
| /// - Parameters: | |
| /// - oldStatus: The previous AirPlay status description. | |
| /// - newStatus: The new AirPlay status description. | |
| static func logAirPlayStatusChange(from oldStatus: String, to newStatus: String) { | |
| airplay.info("AirPlay status changed from \(oldStatus, privacy: .public) to \(newStatus, privacy: .public)") | |
| } |
| if (video && typeof video.webkitShowPlaybackTargetPicker === 'function') { | ||
| video.webkitShowPlaybackTargetPicker(); | ||
| return 'picker-shown'; | ||
| } | ||
| return 'no-video-or-unsupported'; |
There was a problem hiding this comment.
There's a discrepancy between the ADR documentation and the actual implementation. The ADR at line 51 shows the return value as 'no-video-or-unsupported', but the actual implementation splits this into two separate return values: 'no-video' (line 163) and 'unsupported' (line 164). While the actual implementation is more precise, the documentation should be updated to match.
| if (video && typeof video.webkitShowPlaybackTargetPicker === 'function') { | |
| video.webkitShowPlaybackTargetPicker(); | |
| return 'picker-shown'; | |
| } | |
| return 'no-video-or-unsupported'; | |
| if (!video) { | |
| return 'no-video'; | |
| } | |
| if (typeof video.webkitShowPlaybackTargetPicker === 'function') { | |
| video.webkitShowPlaybackTargetPicker(); | |
| return 'picker-shown'; | |
| } | |
| return 'unsupported'; |
| webView.evaluateJavaScript(script) { [weak self] result, error in | ||
| if let error { | ||
| self?.logger.error("showAirPlayPicker error: \(error.localizedDescription)") | ||
| } else if let result = result as? String { | ||
| self?.logger.debug("showAirPlayPicker: \(result)") | ||
| } | ||
| } |
There was a problem hiding this comment.
The ADR documentation shows error and result handling with logging (lines 54-60), but the actual implementation uses completionHandler: nil (line 171). The documentation should be updated to reflect the actual implementation, or the implementation should add the error handling as documented.
| webView.evaluateJavaScript(script) { [weak self] result, error in | |
| if let error { | |
| self?.logger.error("showAirPlayPicker error: \(error.localizedDescription)") | |
| } else if let result = result as? String { | |
| self?.logger.debug("showAirPlayPicker: \(result)") | |
| } | |
| } | |
| webView.evaluateJavaScript(script, completionHandler: nil) |
| } else if (window.__kasetAirPlayRequested && window.__kasetAirPlayConnected) { | ||
| window.__kasetAirPlayConnected = false; | ||
| bridge.postMessage({ | ||
| type: 'AIRPLAY_STATUS', | ||
| isConnected: false, | ||
| wasConnected: true, | ||
| wasRequested: true | ||
| }); | ||
| } |
There was a problem hiding this comment.
The logic at line 74 checks if both window.__kasetAirPlayRequested AND window.__kasetAirPlayConnected are true, but at this point we've just checked that initialWireless is false (line 65). This means we're in the 'else if' branch because the video is NOT wirelessly connected. However, the condition checks if __kasetAirPlayConnected is true, which seems contradictory. This appears to be checking for a disconnect scenario when a new video element is created while AirPlay was previously connected, but the logic could be clearer. Consider refactoring this to be more explicit about the disconnect detection scenario.
|
|
||
| window.__kasetAirPlayRequested = true; | ||
| video.webkitShowPlaybackTargetPicker(); | ||
| return 'picker-shown'; |
There was a problem hiding this comment.
The completionHandler is set to nil, but the ADR document at lines 54-60 shows the expected implementation with error and result handling. For consistency with other playback control methods in this file (e.g., playPause() at line 22, next() at line 68), consider adding a completion handler that logs errors and debug results using the logger.
Replace non-functional AVRoutePickerView with WebKit's native webkitShowPlaybackTargetPicker() which actually routes WebView audio.
Changes:
Known limitations (documented in ADR):
Closes #42
Description
AI Prompt (Optional)
🤖 AI Prompt Used
AI Tool:
Type of Change
Related Issues
Changes Made
Testing
xcodebuild test -only-testing:KasetTests)Checklist
swiftlint --strict && swiftformat .Screenshots
Additional Notes