Share web-player scripts; add music video fullscreen + quality picker - #316
Open
sozercan wants to merge 9 commits into
Open
Share web-player scripts; add music video fullscreen + quality picker#316sozercan wants to merge 9 commits into
sozercan wants to merge 9 commits into
Conversation
Consolidate the origin-neutral JavaScript and WebView boilerplate that the music (SingletonPlayerWebView) and regular-YouTube (YouTubeWatchWebView) playback paths had duplicated into a single WebPlayerScripts source of truth: volume bootstrap, generic <video> play/pause/seek, WKWebView reparenting, and content-process crash recovery. Both paths compose from it; observers, setVolume, and the video-extraction scripts stay parallel by design (ADR-0023, extending ADR-0020). Borrow two video features onto the music side, both additive and leaving the proven audio path untouched: - Native fullscreen for the floating video window (.fullScreenPrimary + toggleFullscreen, with hover/double-click/^⌘F affordances). - Resolution picker for Official Music Videos. A runtime probe confirmed music.youtube.com's #movie_player exposes real, selectable quality levels (captions came back empty, so no caption picker). Discovery is keyed to the active videoId via a MusicVideoQualitySource seam and latches only on a successful fetch, so it repopulates across track changes and retries a not-yet-ready player.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06b1d757ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR works on the video-playback paths in Kaset (a native macOS YouTube Music client). It has two parts. First, it extracts origin-neutral playback machinery that was duplicated between the music WebView (SingletonPlayerWebView) and the regular-YouTube WebView (YouTubeWatchWebView) into a single WebPlayerScripts source of truth (volume clamp/bootstrap, generic <video> play/pause/seek, WKWebView.reparent(into:), and content-process crash recovery), with golden-string tests to pin the generated JS. Second, it borrows two video features onto the music side: native fullscreen for the floating video window and a resolution picker for Official Music Videos, keyed to the active videoId via a MusicVideoQualitySource seam so the menu repopulates across track changes and retries a not-yet-ready player. Two ADRs document the rationale.
Changes:
- Introduce
WebPlayerScriptsand migrate the YouTube watch path (play/pause/seek, reparenting, crash recovery, volume bootstrap) onto it; migrate the music path's play/pause/seek (removing deadreturnstrings). - Add music-video fullscreen (
VideoWindowController.toggleFullscreen()+.fullScreenPrimary, hover button / double-click / ⌃⌘F inVideoPlayerWindow). - Add a music-video resolution picker (
PlayerService+VideoQuality,SingletonPlayerWebView+VideoQuality,PlayerBargearshape menu) with discovery/retry logic and tests.
Show a summary per file
| File | Description |
|---|---|
Sources/Kaset/Views/WebPlayerScripts.swift |
New shared origin-neutral playback primitives (volume, video commands, reparent, recovery). |
Sources/Kaset/Views/YouTube/YouTubeWatchWebView+Scripts.swift |
YouTube play/pause/seek now compose from WebPlayerScripts. |
Sources/Kaset/Views/YouTube/YouTubeWatchWebView.swift |
YouTube reparenting, volume bootstrap, and crash recovery delegate to WebPlayerScripts. |
Sources/Kaset/Views/SingletonPlayerWebView+PlaybackControls.swift |
Music play/pause/seek compose from WebPlayerScripts; dead return strings removed. |
Sources/Kaset/Views/SingletonPlayerWebView+VideoQuality.swift |
New JS bridge for music #movie_player quality levels. |
Sources/Kaset/Services/Player/PlayerService+VideoQuality.swift |
MusicVideoQualitySource seam + per-track discovery/retry logic. |
Sources/Kaset/Services/Player/PlayerService.swift |
New observable quality state + injectable quality source. |
Sources/Kaset/Services/Player/PlayerService+Library.swift |
resetTrackStatus() clears quality options. |
Sources/Kaset/Views/PlayerBar+VideoQuality.swift / PlayerBar.swift |
Resolution menu shown while video mode is active with known levels. |
Sources/Kaset/Views/MainWindow.swift |
Triggers quality discovery on video-open and track change. |
Sources/Kaset/VideoWindowController.swift / Views/VideoPlayerWindow.swift |
Native fullscreen support and affordances. |
Sources/Kaset/Utilities/AccessibilityIdentifiers.swift |
New videoQuality button identifier. |
Tests/KasetTests/WebPlayerScriptsTests.swift / VideoSupportTests.swift |
Golden-string and quality-discovery tests. |
docs/adr/0023-*.md, docs/adr/0020-*.md, docs/adr/README.md |
ADR documenting the shared-scripts decision and follow-ups. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 19/19 changed files
- Comments generated: 2
…menu Resolve the actionable code-review threads on #316: - Re-apply the origin-neutral consolidation to the music SingletonPlayerWebView, which an earlier probe-revert had silently dropped: ensureInHierarchy now uses WKWebView.reparent(into:), the singleton Coordinator's terminate handler uses WebPlayerScripts.recoverFromContentProcessTermination, and pageBootstrapScript uses WebPlayerScripts.targetVolumeBootstrap. The WebPlayerScripts doc comments ("shared by both playback singletons") are now true, restoring ADR-0023's "a fix lands once" guarantee. Behavior-preserving (AutoplayRecoveryTests green). - refreshVideoQualityOptionsIfNeeded retries internally (3x@1500ms, re-checking showVideo/videoId between attempts) instead of relying on a future onChange, so the quality picker still populates when the first probe beats the player's format enumeration. - Expose the resolution picker in the compact player bar too (extracted the menu into a reusable VideoQualityMenu view used by both layouts).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26be98e496
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…th limit The compact-bar quality picker pushed PlayerBar.swift to 899 lines (1 under the 900 limit); merged with main's independent edits to the same file it tipped over to 905, failing the SwiftLint file_length check in CI (which lints the PR merge commit). Move CompactVisibleActionButtons into its own file — PlayerBar.swift drops to 806, leaving headroom.
Follow-up to the prior quality-discovery fix. A skip while video mode is open updates currentTrack before SingletonPlayerWebView.loadVideo finishes navigating, so MainWindow's videoId onChange could read the still-loaded previous #movie_player, latch its levels under the new videoId, and never probe the real new video. refreshVideoQualityOptionsIfNeeded now confirms the player reports the requested video as loaded (via a new MusicVideoQualitySource.loadedVideoId, backed by currentPlaybackSnapshot) before trusting/latching levels; otherwise it retries. Adds tests for the stale-page skip and the catch-up case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d47d79e2cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
- Decouple video-quality clearing from resetTrackStatus: a same-videoId metadata refresh (title/artist glitch) no longer blanks the picker, since MainWindow only re-discovers on a videoId change. Quality is now cleared videoId-keyed — refreshVideoQualityOptionsIfNeeded drops the previous video's displayed levels before probing a new one, and video-mode close fully resets. - Make the discovery retry delay injectable (PlayerService.videoQualityRetryDelay, mirroring HistoryViewModel.playbackRefreshDelay); tests set .zero, cutting ~4.5s of real wall-clock waiting from the suite. - Document the music video shortcuts (⌘⇧V toggle, ⌃⌘F fullscreen) in docs/keyboard-shortcuts.md per AGENTS.md.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32c2c0698d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…ortcut routing - Reset the per-video fetch guard when clearing quality for a new probe, so a skip A→B that never reports levels leaves the guard unset; returning to A re-discovers instead of short-circuiting on a stale guard. - Clear quality state on the nil-track path: when stop() nils currentTrack while video mode stays open, MainWindow's videoId observer now resets quality rather than early-returning, so the gear menu doesn't linger with no active track. - Move the ⌃⌘F fullscreen key equivalent from a button inside the (non-key) floating video window to the app's Playback CommandMenu, so it fires while the main window stays key. The in-window button and double-click still work.
Resolved conflicts: - YouTubeWatchWebView.pageBootstrapScript: keep main's new pendingSeek resume-position param AND use the shared WebPlayerScripts.targetVolumeBootstrap for the volume line (byte-identical output; preserves the ADR-0024 consolidation). - docs/adr/README.md: list main's 0022 (YouTube Home Sections) and 0023 (Brand-Account History) plus this branch's ADR. - ADR numbering: renumbered this branch's "Shared Web-Player Scripts" ADR from 0023 to 0024 (main published 0023 for Brand-Account History); updated all code comments, the 0020 cross-link, and the README accordingly. - Removed the now-superfluous `swiftlint:disable file_length` in MiniPlayerWebView.swift (the consolidation brought it under the limit).
Comment on lines
+196
to
197
| /// Document-start state handed to each new watch page. | ||
| /// Document-start state handed to each new watch page. |
| | [0021](0021-liquid-glass-sidebar-slide-under.md) | Liquid Glass Sidebar Slide-Under | Accepted | | ||
| | [0022](0022-youtube-home-sections.md) | YouTube Home Sections — Continue Watching + Personalized Topic Rails | Accepted | | ||
| | [0023](0023-brand-account-history-session-switch.md) | Brand-Account History via WebView Session-Identity Switch | Accepted | | ||
| | [0024](0024-shared-web-player-scripts.md) | Shared Web-Player Scripts Across Music and YouTube Playback | Accepted | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two layers of work on the video-playback paths:
1. Consolidate duplicated web-player machinery (ADR-0023, extends ADR-0020).
The music (
SingletonPlayerWebView) and regular-YouTube (YouTubeWatchWebView) stacks had independently duplicated origin-neutral JavaScript and WebView boilerplate. Extracted into oneWebPlayerScriptssource of truth that both compose from:<video>play/pause/seek(parameterized by element selector)WKWebView.reparent(into:)The music path was touched behavior-preservingly — the only removed code was provably-dead
returnstrings (every caller usescompletionHandler: nil). Golden-string tests (WebPlayerScriptsTests) pin the generated JS so neither path can silently drift. Observers,setVolume, and the video-extraction scripts stay parallel by design.2. Borrow two video features onto the music side (additive; audio path untouched):
.fullScreenPrimary+toggleFullscreen(), with hover-button / double-click /^⌘Faffordances, mirroringYouTubeVideoWindowController. Tradeoff: the window no longer floats over other apps' fullscreen spaces.#movie_playerconfirmed music.youtube.com exposes real, selectable quality levels (hd720/large/medium/small/tiny/auto) and that forcing a level actually changes resolution. Captions came back empty for OMVs, so no caption picker was built. Discovery is keyed to the activevideoIdvia aMusicVideoQualitySourceseam and latches only on a successful fetch — so the menu repopulates across track changes and retries a not-yet-ready player.Note: ADR numbered 0023 (0022 is taken by youtube-home-sections on the current
main; this branch is based on an earlier commit).Test plan
swift buildcleanswift test --skip KasetUITests— full suite green (1418 tests); added 9 tests (WebPlayerScriptsgolden output + music video-quality discovery, including track-change re-discovery and empty-then-ready retry)swiftlint --strict— 0 violationsswiftformat .— clean⌘⇧V→ confirm fullscreen (^⌘F/ double-click) and the resolution menu appear and workmain; rebase/merge before landing