Conversation
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive video mode feature to Kaset, allowing users to watch music videos in a floating window while using other apps. The implementation extracts the video element from YouTube Music's web interface using JavaScript/CSS injection and displays it in a resizable, corner-snapping window.
Key Changes
- New video window system: Introduces
VideoWindowControllerandVideoPlayerWindowfor managing a floating video player - WebView architecture refactoring: Splits
SingletonPlayerWebViewinto multiple focused extensions for playback controls, video mode, and observer scripts - Video availability detection: Adds JavaScript logic to detect when tracks have music videos available and surfaces this in the UI
- Cookie management optimization: Implements debouncing for cookie backup operations to reduce I/O overhead
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
docs/video.md |
Comprehensive documentation of video mode architecture, CSS injection strategy, and known issues |
docs/playback.md |
Updated to reference new video mode documentation |
Views/macOS/VideoPlayerWindow.swift |
SwiftUI view for the floating video window with frame change handling |
Views/macOS/SingletonPlayerWebView+VideoMode.swift |
CSS injection logic for extracting and displaying video element |
Views/macOS/SingletonPlayerWebView+PlaybackControls.swift |
Extracted playback control methods into dedicated extension |
Views/macOS/SingletonPlayerWebView+ObserverScript.swift |
Extracted observer script with video availability detection |
Views/macOS/MiniPlayerWebView.swift |
Refactored to use new extension architecture and Auto Layout constraints |
Views/macOS/PlayerBar.swift |
Added video button that appears when current track has video |
Views/macOS/MainWindow.swift |
Video window lifecycle management via onChange handler |
App/VideoWindowController.swift |
Manages video window state, corner snapping, and cleanup logic |
App/KasetApp.swift |
Added debug logging (should be removed) |
Core/Services/Player/PlayerService.swift |
Video state management and grace period logic |
Core/Services/WebKit/WebKitManager.swift |
Cookie change debouncing to reduce I/O |
Core/Models/Song.swift |
Added optional hasVideo property |
Core/Utilities/AccessibilityIdentifiers.swift |
Added video button identifier |
Tests/KasetTests/VideoSupportTests.swift |
Basic tests for video state management |
TEMP_SWIFTPM_MIGRATION_PLAN.md |
Unrelated SwiftPM migration plan (should be in separate PR) |
Kaset.xcodeproj/project.pbxproj |
Project file updates for new source files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| })(); | ||
| """ | ||
|
|
||
| webView.evaluateJavaScript(script, completionHandler: nil) |
There was a problem hiding this comment.
Multiple calls to evaluateJavaScript use completionHandler: nil, which silently ignores JavaScript execution errors. For critical operations like video mode injection and cleanup, error handling should be implemented to detect and log failures. Consider adding completion handlers that log errors to aid in debugging issues.
Views/macOS/MiniPlayerWebView.swift
Outdated
| // Use autoresizing to match container size | ||
| webView.translatesAutoresizingMaskIntoConstraints = false | ||
| NSLayoutConstraint.activate([ | ||
| webView.leadingAnchor.constraint(equalTo: container.leadingAnchor), | ||
| webView.trailingAnchor.constraint(equalTo: container.trailingAnchor), | ||
| webView.topAnchor.constraint(equalTo: container.topAnchor), | ||
| webView.bottomAnchor.constraint(equalTo: container.bottomAnchor), | ||
| ]) |
There was a problem hiding this comment.
The constraints added here use translatesAutoresizingMaskIntoConstraints = false, but the comment on line 289 says "Don't inject CSS here - updateDisplayMode() handles it after layout completes". This is inconsistent with the earlier approach in waitForValidBoundsAndInject() which uses autoresizingMask (lines 62-63 in SingletonPlayerWebView+VideoMode.swift). Using a mix of autoresizing masks and Auto Layout constraints for the same view can cause layout issues. Consider using one approach consistently.
| // Use autoresizing to match container size | |
| webView.translatesAutoresizingMaskIntoConstraints = false | |
| NSLayoutConstraint.activate([ | |
| webView.leadingAnchor.constraint(equalTo: container.leadingAnchor), | |
| webView.trailingAnchor.constraint(equalTo: container.trailingAnchor), | |
| webView.topAnchor.constraint(equalTo: container.topAnchor), | |
| webView.bottomAnchor.constraint(equalTo: container.bottomAnchor), | |
| ]) | |
| // Use autoresizing to match container size (consistent with other singleton usages) | |
| webView.translatesAutoresizingMaskIntoConstraints = true | |
| webView.frame = container.bounds | |
| webView.autoresizingMask = [.widthSizable, .heightSizable] |
| return { success: true, width: width, height: height }; | ||
| })(); | ||
| """ | ||
| webView.evaluateJavaScript(resizeScript, completionHandler: nil) |
There was a problem hiding this comment.
The resize script errors are ignored with completionHandler: nil. When the window is resized, if the CSS update fails, the video will appear stretched or incorrectly sized. Add error handling to detect resize failures.
| webView.evaluateJavaScript(resizeScript, completionHandler: nil) | |
| webView.evaluateJavaScript(resizeScript) { result, error in | |
| if let error = error { | |
| NSLog("[Kaset] Failed to refresh video mode CSS: \(error.localizedDescription)") | |
| return | |
| } | |
| guard | |
| let dict = result as? [String: Any], | |
| let success = dict["success"] as? Bool | |
| else { | |
| // If the script didn't return the expected object, log for diagnostics. | |
| if let result = result { | |
| NSLog("[Kaset] Unexpected result from video mode resize script: \(result)") | |
| } | |
| return | |
| } | |
| if success == false { | |
| NSLog("[Kaset] Video mode CSS resize script reported failure: \(dict)") | |
| } | |
| } |
| // Video button - only shown when track has video | ||
| if self.playerService.currentTrackHasVideo { | ||
| Button { | ||
| HapticService.toggle() | ||
| DiagnosticsLogger.player.debug( | ||
| "Video button clicked, toggling showVideo from \(self.playerService.showVideo)") | ||
| withAnimation(AppAnimation.standard) { | ||
| player.showVideo.toggle() | ||
| } | ||
| } label: { | ||
| Image(systemName: self.playerService.showVideo ? "tv.fill" : "tv") | ||
| .font(.system(size: 15, weight: .medium)) | ||
| .foregroundStyle(self.playerService.showVideo ? .red : .primary.opacity(0.85)) | ||
| .contentTransition(.symbolEffect(.replace)) | ||
| } | ||
| .buttonStyle(.pressable) | ||
| .keyboardShortcut("v", modifiers: [.command, .shift]) | ||
| .accessibilityIdentifier(AccessibilityID.PlayerBar.videoButton) | ||
| .accessibilityLabel("Video") | ||
| .accessibilityValue(self.playerService.showVideo ? "Playing" : "Off") | ||
| } |
There was a problem hiding this comment.
The video button doesn't have the .glassEffectID() modifier like the other buttons in this section (lyrics and queue buttons). This inconsistency in styling may cause visual differences in the UI. Consider adding .glassEffectID("video", in: self.playerNamespace) for consistency.
Views/macOS/MiniPlayerWebView.swift
Outdated
| // Only close video window on track change if we're NOT in video mode | ||
| // When video mode is active, trackChanged detection is unreliable because | ||
| // our video container can obscure the DOM elements being queried | ||
| if self.playerService.showVideo, !self.playerService.isVideoGracePeriodActive { | ||
| // Check if this is a real track change by comparing video IDs | ||
| // rather than relying on title/artist which can be unreliable | ||
| DiagnosticsLogger.player.info( | ||
| "trackChanged to '\(title)' while video shown - closing video window") | ||
| self.playerService.showVideo = false | ||
| } |
There was a problem hiding this comment.
The comment on line 388 mentions "comparing video IDs rather than relying on title/artist", but the actual implementation on line 387 only checks showVideo and isVideoGracePeriodActive without any video ID comparison. The comment is misleading and doesn't match the implementation. Either update the implementation to actually compare video IDs (recommended for more reliable track change detection) or update the comment to accurately describe what the code does.
Views/macOS/MiniPlayerWebView.swift
Outdated
| return video ? 'applied' : 'no-video-yet'; | ||
| })(); | ||
| """ | ||
| webView.evaluateJavaScript(applyVolumeScript, completionHandler: nil) |
There was a problem hiding this comment.
Volume script execution errors are silently ignored with completionHandler: nil. Since correct volume setting is critical for user experience (as evidenced by extensive volume enforcement logic throughout the codebase), errors here should be logged to help diagnose volume-related issues.
| webView.evaluateJavaScript(applyVolumeScript, completionHandler: nil) | |
| webView.evaluateJavaScript(applyVolumeScript) { result, error in | |
| if let error = error { | |
| DiagnosticsLogger.player.error( | |
| "Failed to apply saved volume \(savedVolume) in Singleton WebView: \(error.localizedDescription)" | |
| ) | |
| return | |
| } | |
| if let resultString = result as? String { | |
| DiagnosticsLogger.player.debug( | |
| "Apply volume script result: \(resultString) for saved volume \(savedVolume)" | |
| ) | |
| } | |
| } |
Views/macOS/VideoPlayerWindow.swift
Outdated
| @objc private func reinjectCSS() { | ||
| // Legacy method - now handled directly in frameDidChange | ||
| } | ||
|
|
There was a problem hiding this comment.
The reinjectCSS() method is marked as legacy but is still defined in the class. This dead code should be removed to avoid confusion and reduce maintenance burden.
| @objc private func reinjectCSS() { | |
| // Legacy method - now handled directly in frameDidChange | |
| } |
| })(); | ||
| """ | ||
|
|
||
| webView.evaluateJavaScript(script, completionHandler: nil) |
There was a problem hiding this comment.
The cleanup script errors are ignored with completionHandler: nil. If restoration of the video element to its original location fails, it could cause issues when the user reopens the video window. Add error handling to detect and log restoration failures.
| import Foundation | ||
| import Testing | ||
| @testable import Kaset | ||
|
|
||
| /// Tests for Video Support functionality. | ||
| @Suite("Video Support", .serialized, .tags(.service)) | ||
| @MainActor | ||
| struct VideoSupportTests { | ||
| var playerService: PlayerService | ||
|
|
||
| init() { | ||
| UserDefaults.standard.removeObject(forKey: "playerVolume") | ||
| UserDefaults.standard.removeObject(forKey: "playerVolumeBeforeMute") | ||
| self.playerService = PlayerService() | ||
| } | ||
|
|
||
| // MARK: - Initial State Tests | ||
|
|
||
| @Test("currentTrackHasVideo initially false") | ||
| func currentTrackHasVideoInitiallyFalse() { | ||
| #expect(self.playerService.currentTrackHasVideo == false) | ||
| } | ||
|
|
||
| @Test("showVideo initially false") | ||
| func showVideoInitiallyFalse() { | ||
| #expect(self.playerService.showVideo == false) | ||
| } | ||
|
|
||
| // MARK: - Video Availability Tests | ||
|
|
||
| @Test("updateVideoAvailability sets hasVideo correctly") | ||
| func updateVideoAvailabilitySetsHasVideo() { | ||
| #expect(self.playerService.currentTrackHasVideo == false) | ||
|
|
||
| self.playerService.updateVideoAvailability(hasVideo: true) | ||
| #expect(self.playerService.currentTrackHasVideo == true) | ||
|
|
||
| self.playerService.updateVideoAvailability(hasVideo: false) | ||
| #expect(self.playerService.currentTrackHasVideo == false) | ||
| } | ||
|
|
||
| // MARK: - Video Window Behavior Tests | ||
|
|
||
| @Test("showVideo stays open even when hasVideo becomes false") | ||
| func showVideoStaysOpenWhenHasVideoChanges() { | ||
| // The video window should not auto-close based on hasVideo detection | ||
| // because detection is unreliable when video mode CSS is active. | ||
| // Only trackChanged should close the video window. | ||
| self.playerService.updateVideoAvailability(hasVideo: true) | ||
| self.playerService.showVideo = true | ||
| #expect(self.playerService.showVideo == true) | ||
|
|
||
| // hasVideo becomes false (unreliable detection during video mode) | ||
| self.playerService.updateVideoAvailability(hasVideo: false) | ||
| #expect(self.playerService.showVideo == true, "Video window should NOT auto-close based on hasVideo") | ||
| } | ||
|
|
||
| @Test("showVideo can be enabled even when hasVideo is false") | ||
| func showVideoCanBeEnabledWhenNoVideo() { | ||
| // We allow enabling showVideo even without hasVideo because: | ||
| // 1. hasVideo detection might lag behind | ||
| // 2. User explicitly requested video mode | ||
| #expect(self.playerService.currentTrackHasVideo == false) | ||
| self.playerService.showVideo = true | ||
| #expect(self.playerService.showVideo == true, "showVideo should be allowed even if hasVideo is false") | ||
| } | ||
|
|
||
| @Test("showVideo stays open when changing to another video track") | ||
| func showVideoStaysOpenForVideoTrack() { | ||
| // Enable video with a video-capable track | ||
| self.playerService.updateVideoAvailability(hasVideo: true) | ||
| self.playerService.showVideo = true | ||
| #expect(self.playerService.showVideo == true) | ||
|
|
||
| // Track changes but still has video | ||
| self.playerService.updateVideoAvailability(hasVideo: true) | ||
| #expect(self.playerService.showVideo == true, "Video window should stay open") | ||
| } | ||
|
|
||
| // MARK: - Model Tests | ||
|
|
||
| @Test("Song.hasVideo property exists and defaults to nil") | ||
| func songHasVideoPropertyExists() { | ||
| let song = Song( | ||
| id: "test", | ||
| title: "Test Song", | ||
| artists: [], | ||
| album: nil, | ||
| duration: 180, | ||
| thumbnailURL: nil, | ||
| videoId: "test-video" | ||
| ) | ||
| #expect(song.hasVideo == nil) | ||
| } | ||
|
|
||
| @Test("Song.hasVideo can be set explicitly") | ||
| func songHasVideoCanBeSet() { | ||
| let songWithVideo = Song( | ||
| id: "test", | ||
| title: "Test Song", | ||
| artists: [], | ||
| album: nil, | ||
| duration: 180, | ||
| thumbnailURL: nil, | ||
| videoId: "test-video", | ||
| hasVideo: true | ||
| ) | ||
| #expect(songWithVideo.hasVideo == true) | ||
|
|
||
| let songWithoutVideo = Song( | ||
| id: "test2", | ||
| title: "Test Song 2", | ||
| artists: [], | ||
| album: nil, | ||
| duration: 180, | ||
| thumbnailURL: nil, | ||
| videoId: "test-video-2", | ||
| hasVideo: false | ||
| ) | ||
| #expect(songWithoutVideo.hasVideo == false) | ||
| } | ||
|
|
||
| // MARK: - Display Mode Tests | ||
|
|
||
| @Test("SingletonPlayerWebView DisplayMode enum has all cases") | ||
| func displayModeEnumHasAllCases() { | ||
| let hidden = SingletonPlayerWebView.DisplayMode.hidden | ||
| let miniPlayer = SingletonPlayerWebView.DisplayMode.miniPlayer | ||
| let video = SingletonPlayerWebView.DisplayMode.video | ||
|
|
||
| // Just verify the enum cases exist | ||
| #expect(hidden == .hidden) | ||
| #expect(miniPlayer == .miniPlayer) | ||
| #expect(video == .video) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test coverage for the video feature is limited to basic state management and model tests. Critical functionality like video window lifecycle (opening/closing), WebView display mode transitions, CSS injection/removal, corner snapping, and grace period handling are not tested. Consider adding tests for these behaviors, especially the complex state machine around window opening/closing and the isClosing flag logic.
| /// Waits for the WebView's superview to have valid (non-zero) bounds, then injects CSS. | ||
| func waitForValidBoundsAndInject(attempts: Int = 0) { | ||
| guard let webView else { return } | ||
|
|
||
| let maxAttempts = 20 // 2 seconds max (20 * 100ms) | ||
| if let superview = webView.superview, superview.bounds.width > 0, superview.bounds.height > 0 { | ||
| webView.frame = superview.bounds | ||
| webView.autoresizingMask = [.width, .height] | ||
| self.injectVideoModeCSS() | ||
| } else if attempts < maxAttempts { | ||
| Task { @MainActor [weak self] in | ||
| try? await Task.sleep(for: .milliseconds(100)) | ||
| self?.waitForValidBoundsAndInject(attempts: attempts + 1) | ||
| } | ||
| } else { | ||
| // Fall back to current bounds anyway | ||
| if let superview = webView.superview { | ||
| webView.frame = superview.bounds | ||
| webView.autoresizingMask = [.width, .height] | ||
| } | ||
| self.injectVideoModeCSS() | ||
| } | ||
| } |
There was a problem hiding this comment.
The waitForValidBoundsAndInject function creates a recursive async task chain that could potentially leak if the WebView is deallocated during the wait period. While [weak self] is used (line 65), the error from Task.sleep is silently ignored with try?. If the task is cancelled (e.g., when the video window closes), the recursion should stop cleanly, but there's no logging to verify this happens. Consider adding logging when the wait times out or is cancelled to help diagnose layout timing issues.
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // MARK: - VideoWindowController | ||
|
|
||
| /// Manages the floating video window. |
There was a problem hiding this comment.
The VideoWindowController class uses VideoPlayerWindow which is marked with @available(macOS 26.0, *), but the controller itself doesn't have this availability annotation. This will cause a compilation error when building for earlier macOS versions.
The show(playerService:webKitManager:) method instantiates VideoPlayerWindow() on line 57, which requires macOS 26.0 or later. The entire class should be marked with @available(macOS 26.0, *) to ensure it's only used on compatible systems.
| /// Manages the floating video window. | |
| /// Manages the floating video window. | |
| @available(macOS 26.0, *) |
| // MARK: - VideoWebViewContainer | ||
|
|
||
| /// NSViewRepresentable container for the video WebView. | ||
| struct VideoWebViewContainer: NSViewRepresentable { |
There was a problem hiding this comment.
The VideoWebViewContainer struct doesn't have an @available(macOS 26.0, *) annotation, but it's only used within VideoPlayerWindow which does have this annotation. While this works due to the parent's annotation, adding the explicit availability annotation to VideoWebViewContainer and VideoContainerView would improve code clarity and prevent potential misuse outside the intended context.
|
|
||
| // MARK: - VideoContainerView | ||
|
|
||
| /// Custom NSView that observes frame changes and re-injects CSS. |
There was a problem hiding this comment.
The VideoContainerView class doesn't have an @available(macOS 26.0, *) annotation, but it's only used within the video feature that requires macOS 26.0. Adding this annotation would make the version requirement explicit and prevent accidental use in incompatible contexts.
| /// Custom NSView that observes frame changes and re-injects CSS. | |
| /// Custom NSView that observes frame changes and re-injects CSS. | |
| @available(macOS 26.0, *) |
| } catch { | ||
| // Task was cancelled (new cookie change came in), skip backup | ||
| return |
There was a problem hiding this comment.
The debounce task in cookiesDidChange catches all errors with a generic catch block that only handles cancellation. If Task.sleep throws a different error (unlikely but possible), the backup would be silently skipped. Consider checking specifically for CancellationError or documenting that only cancellation errors are expected.
| } catch { | |
| // Task was cancelled (new cookie change came in), skip backup | |
| return | |
| } catch is CancellationError { | |
| // Task was cancelled (new cookie change came in), skip backup | |
| return | |
| } catch { | |
| os_log("Unexpected error during cookie debounce delay: %{public}@", | |
| type: .error, | |
| String(describing: error)) |
| var coordinator: Coordinator? | ||
| let logger = DiagnosticsLogger.player |
There was a problem hiding this comment.
The logger property is changed from private to internal visibility but the coordinator property is also changed from private to internal. This increases the visibility of internal implementation details. Consider if these properties need to be exposed or if a more targeted approach (like specific accessor methods) would be better for maintainability.
| var coordinator: Coordinator? | |
| let logger = DiagnosticsLogger.player | |
| private var coordinator: Coordinator? | |
| private let logger = DiagnosticsLogger.player |
| @@ -279,7 +294,6 @@ final class SingletonPlayerWebView { | |||
|
|
|||
| let previousVideoId = self.currentVideoId | |||
| guard videoId != previousVideoId else { | |||
There was a problem hiding this comment.
The removed log message "Video {videoId} already loaded, skipping" provided useful debugging information. Consider keeping it at debug level or adding a comment explaining why early return happens without logging.
| guard videoId != previousVideoId else { | |
| guard videoId != previousVideoId else { | |
| self.logger.debug("Video \(videoId) already loaded, skipping") |
Signed-off-by: Sertac Ozercan <sozercan@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
fixes #31
Type of Change
Related Issues
Changes Made
Testing
xcodebuild test -only-testing:KasetTests)Checklist
swiftlint --strict && swiftformat .Screenshots
Additional Notes