Integrate upstream #408 playback reliability - #25
Merged
Conversation
…o the fork) Cherry-pick of upstream sozercan/kaset dfdb4ba ("fix: improve YouTube playback reliability"), hand-resolved against the fork's 175-commit divergence and reconciled with the fork's own #19 (⌘R refresh + network auto-retry). Taken from sozercan#408 (the reliability core): - Loading-timeout + media-error recovery in YouTubePlayerService (finishPlaybackLoadingIfReady / deferMediaErrorIfNeeded, ready-media based), replacing the fork's duration>0 loading-clear proxy. Kept the fork's sozercan#374 conditional-assignment perf optimization and ad-skip/live-edge fields. - Runtime liveness (currentMediaIsLive) for URL launches / SPA drift; adopted in the seek-to-end guard. - ScrollForwardingWebView (only forwards wheel events off the extracted watch doc, not off consent/sign-in/CAPTCHA interstitials). - Generalized interstitial handling (isTrustedIntermediaryURL + revealInterstitialScript) in the watch coordinator; kept the fork's clearPlaybackLoadingForInterstitial() call. - YouTubeItemParser thumbnail helpers; new document-generation/loading-timeout/ media-error test suites. Kept the fork's version (superseded upstream's incidental UI changes): - YouTubePlayerBar, YouTubeShortsView, YouTubeVideoWindowController — the fork deliberately reworked these (overflow menu, shorts rail, PiP always-on-top). - Subscribe strings retranslated to the free-subscribe sense ("Iscriviti"), matching the app's abbonati(paid)/iscriviti(free) terminology. Notes: - Dropped upstream's YouTubeVideoWindowFrameAutosaveState/FullscreenIntent refactor (fork handles fullscreen-frame persistence inline + keeps PiP posture) and its extraction-script test harness (fork extracts differently). - Scoped LocalizationCatalogParityTests to the fork's shipped locale subset. - Pre-existing test-target failures on main (AppLocalizationTests bundle loading, SettingsManager/ListenBrainz, storyboard flake) are unchanged; no playback/reliability test regressed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…gration - Restore the fork's duration>0 loading-finish signal (the Shorts thumbnail cover gates on isPlaybackLoading) alongside sozercan#408's stricter ready-media check, excluding ads/media-error frames so recovery is unaffected. - ScrollForwardingWebView forwards to nextResponder (the fork's long-standing behaviour) instead of enclosingScrollView, which the SwiftUI ScrollView host bypasses and lags. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+18
to
+20
| init(expectedVideoId: String? = nil) { | ||
| self.expectedVideoId = expectedVideoId | ||
| } |
There was a problem hiding this comment.
Suggestion: The default nil makes the ownership guard ineffective for every existing caller: the Shorts page still constructs YouTubeWatchSurfaceView() without an expected ID, so an outgoing page can continue to reclaim the singleton WebView despite this change. Pass the page's video ID from ShortPage (and keep the default only for intentionally unscoped watch/floating hosts), or remove the default where the guard is required. [logic error]
Severity Level: Critical 🚨
- ❌ Shorts transitions can move playback into the outgoing page.
- ⚠️ Singleton WebView ownership remains race-prone during paging.
- ⚠️ Existing watch and floating hosts intentionally remain unscoped.Steps of Reproduction ✅
1. Open the Shorts experience, whose pager creates each `ShortPage` at
`Sources/Kaset/Views/YouTube/YouTubeShortsView.swift:128-135`.
2. Let the pager select a short; `ShortPage.body` creates `YouTubeWatchSurfaceView()`
without an argument at `Sources/Kaset/Views/YouTube/YouTubeShortsView.swift:302-305`, so
the initializer at `Sources/Kaset/Views/YouTube/YouTubeWatchSurfaceView.swift:18-20`
stores `expectedVideoId == nil`.
3. Scroll to another short. The pager updates `currentShortId` at
`Sources/Kaset/Views/YouTube/YouTubeShortsView.swift:141`, and `schedulePlay` eventually
calls `play(shortId:)` at lines 224-242, changing `youtubePlayer.currentVideo`.
4. During the transition, SwiftUI may update the outgoing page's representable. Its
`updateNSView` at `Sources/Kaset/Views/YouTube/YouTubeWatchSurfaceView.swift:29-34` calls
`ensureInHierarchy` with `expectedVideoId == nil`, so the guard in
`YouTubeWatchSurfaceAttachment.claim` at lines 49-51 does not reject the stale page. The
outgoing page can therefore reparent the singleton WebView after the newly selected page
has claimed it, defeating the regression protection tested by
`YouTubeWatchSurfaceOwnershipTests.swift:9-42`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Sources/Kaset/Views/YouTube/YouTubeWatchSurfaceView.swift
**Line:** 18:20
**Comment:**
*Logic Error: The default `nil` makes the ownership guard ineffective for every existing caller: the Shorts page still constructs `YouTubeWatchSurfaceView()` without an expected ID, so an outgoing page can continue to reclaim the singleton WebView despite this change. Pass the page's video ID from `ShortPage` (and keep the default only for intentionally unscoped watch/floating hosts), or remove the default where the guard is required.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+112
to
+123
| func ensureInHierarchy( | ||
| container: NSView, | ||
| expectedVideoId: String? = nil, | ||
| selectedVideoId: String? = nil | ||
| ) { | ||
| guard let webView else { return } | ||
| guard YouTubeWatchSurfaceAttachment.claim( | ||
| surface: webView, | ||
| in: container, | ||
| expectedVideoId: expectedVideoId, | ||
| currentVideoId: selectedVideoId | ||
| ) else { return } |
There was a problem hiding this comment.
Suggestion: The stale-surface protection is effectively disabled for production callers because expectedVideoId defaults to nil, and all current YouTubeWatchSurfaceView call sites use that default. When overlapping Shorts or watch views update out of order, an old view can therefore pass this claim and reparent the singleton WebView away from the currently selected video. Pass the owning video ID from each surface or require it for views that can overlap. [stale reference]
Severity Level: Critical 🚨
- ❌ Late Shorts updates can move playback into an outgoing page’s container.
- ❌ The active video surface can disappear or render in the wrong pager page.
- ⚠️ Floating and inline hosts also remain unprotected because their call sites use the nil default.Steps of Reproduction ✅
1. Open the Shorts pager, whose `ShortPage` creates an unscoped
`YouTubeWatchSurfaceView()` at
`Sources/Kaset/Views/YouTube/YouTubeShortsView.swift:301-305`; `ShortPage` is active when
the player’s selected video and inline surface match at
`Sources/Kaset/Views/YouTube/YouTubeShortsView.swift:219-222`.
2. Begin paging from one short to another. The pager renders pages through `ShortPage` at
`Sources/Kaset/Views/YouTube/YouTubeShortsView.swift:127-133`, so outgoing and incoming
native hosts can overlap while SwiftUI updates them.
3. When either host receives `updateNSView`, `YouTubeWatchSurfaceView` passes its default
`expectedVideoId == nil` at
`Sources/Kaset/Views/YouTube/YouTubeWatchSurfaceView.swift:18-19,29-34`;
`ensureInHierarchy` forwards that nil value to `YouTubeWatchSurfaceAttachment.claim` at
`Sources/Kaset/Views/YouTube/YouTubeWatchWebView.swift:112-123`.
4. Because `claim` only rejects mismatches when `expectedVideoId` is non-nil
(`Sources/Kaset/Views/YouTube/YouTubeWatchSurfaceView.swift:43-55`), a late update from
the outgoing host is accepted and reparents the singleton WebView into the stale
container. Pass the owning ID from `YouTubeWatchView`, `ShortPage`, and the floating
window, or require an ID for overlapping hosts.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Sources/Kaset/Views/YouTube/YouTubeWatchWebView.swift
**Line:** 112:123
**Comment:**
*Stale Reference: The stale-surface protection is effectively disabled for production callers because `expectedVideoId` defaults to `nil`, and all current `YouTubeWatchSurfaceView` call sites use that default. When overlapping Shorts or watch views update out of order, an old view can therefore pass this claim and reparent the singleton WebView away from the currently selected video. Pass the owning video ID from each surface or require it for views that can overlap.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
1228
to
1236
| self.captionTracks = tracks | ||
| self.qualityLevels = await self.playbackController.availableQualityLevels() | ||
| let levels = await self.playbackController.availableQualityLevels() | ||
| // The player accepts "auto" even when it does not advertise it, and | ||
| // the menu needs that row to represent an unpinned quality. | ||
| self.qualityLevels = levels.isEmpty || levels.contains("auto") | ||
| ? levels | ||
| : levels + ["auto"] | ||
| self.currentQuality = await self.playbackController.currentQualityLevel() | ||
| self.activeCaptionLanguageCode = await self.playbackController.currentCaptionLanguageCode() |
There was a problem hiding this comment.
Suggestion: Re-check currentVideo?.videoId after all of the additional awaited playback-option calls. The task only validates the video immediately after fetching captions, so switching videos while availableQualityLevels, currentQualityLevel, or currentCaptionLanguageCode is suspended can assign the previous video's quality and caption state to the newly selected video. [race condition]
Severity Level: Major ⚠️
- ⚠️ Rapid video switching can show stale quality options.
- ⚠️ Caption selection state can belong to the previous video.
- ⚠️ Playback metadata UI becomes inconsistent after navigation.Steps of Reproduction ✅
1. Start playback of video `abc`; `refreshPlaybackMetadataIfNeeded` launches an
unstructured `Task` at
`Sources/Kaset/Services/Player/YouTubePlayerService.swift:1727-1732` after playback
begins.
2. While `refreshPlaybackOptions()` is suspended in `availableQualityLevels()` at
`Sources/Kaset/Services/Player/YouTubePlayerService.swift:1229`, select another video
through the service's normal playback path, which replaces `currentVideo`.
3. The only identity check occurs immediately after `availableCaptionTracks()` at
`Sources/Kaset/Services/Player/YouTubePlayerService.swift:1226`; execution then awaits
quality and caption-state calls at lines 1229, 1235, and 1236 without rechecking the
captured video ID.
4. When those awaits complete, lines 1230-1236 assign the earlier video's quality and
caption state while `currentVideo` refers to the newly selected video. Rechecking
`currentVideo?.videoId` after the final await would prevent the stale assignment.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Sources/Kaset/Services/Player/YouTubePlayerService.swift
**Line:** 1228:1236
**Comment:**
*Race Condition: Re-check `currentVideo?.videoId` after all of the additional awaited playback-option calls. The task only validates the video immediately after fetching captions, so switching videos while `availableQualityLevels`, `currentQualityLevel`, or `currentCaptionLanguageCode` is suspended can assign the previous video's quality and caption state to the newly selected video.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+190
to
+193
| const isLive = !!( | ||
| (data && data.isLive === true) | ||
| || (hasReadyMedia && !isFinite(video.duration)) | ||
| ); |
There was a problem hiding this comment.
Suggestion: isFinite(NaN) is false, so a ready media element whose duration is temporarily NaN during metadata loading or error recovery is classified as live. The native service then records currentMediaIsLive and can suppress terminal-seek handling for an ordinary finite video. Restrict the runtime fallback to an explicitly unbounded duration such as positive infinity and exclude media-error or invalid-duration states. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Ordinary videos can be treated as live during recovery.
- ⚠️ Terminal seek handling can be suppressed.
- ⚠️ Media errors are not excluded from runtime classification.Steps of Reproduction ✅
1. The observer script at
`Sources/Kaset/Views/YouTube/YouTubeWatchWebView+Scripts.swift:187-193` runs for every
playback state update and marks media ready when `video.currentSrc` exists and `readyState
>= 1`.
2. During metadata loading or media recovery, a ready video can expose `video.duration ===
NaN`; JavaScript `isFinite(NaN)` returns false.
3. The fallback at line 192 therefore sets `isLive` to true even when `videoData().isLive`
is not true and the media is an ordinary finite video.
4. The bridge sends that value in `STATE_UPDATE` at lines 200-215;
`YouTubePlayerService.updatePlaybackState` records it as `currentMediaIsLive` at
`Sources/Kaset/Services/Player/YouTubePlayerService.swift:1625-1626`.
5. Terminal seek handling checks `!self.currentMediaIsLive` at
`Sources/Kaset/Services/Player/YouTubePlayerService+Seeking.swift:43-45`, so the
false-positive classification can suppress normal seek-to-end completion behavior until
per-video state resets.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Sources/Kaset/Views/YouTube/YouTubeWatchWebView+Scripts.swift
**Line:** 190:193
**Comment:**
*Incorrect Condition Logic: `isFinite(NaN)` is false, so a ready media element whose duration is temporarily `NaN` during metadata loading or error recovery is classified as live. The native service then records `currentMediaIsLive` and can suppress terminal-seek handling for an ordinary finite video. Restrict the runtime fallback to an explicitly unbounded duration such as positive infinity and exclude media-error or invalid-duration states.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Comment on lines
+257
to
+260
| service.reloadCurrentVideoForIdentitySwitch() | ||
| try await Task.sleep(for: .milliseconds(400)) | ||
|
|
||
| #expect(service.isPlaybackLoading) |
There was a problem hiding this comment.
Suggestion: The test relies on a fixed 300-millisecond margin between the 1-second loading timeout and the assertion. On a busy CI runner, the initial sleep or the main-actor scheduling can exceed that margin, allowing the replacement timeout to fire before the isPlaybackLoading assertion and causing an intermittent failure. Synchronize on the replacement load starting, or assert using an injected clock instead of wall-clock sleeps. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Loading-timeout regression test can fail intermittently.
- ⚠️ CI results become unreliable under runner contention.
- ⚠️ Failure obscures genuine playback reliability regressions.Steps of Reproduction ✅
1. Run `YouTubePlayerLoadingTimeoutTests.sameVideoReplacementReceivesIndependentTimeout()`
from `Tests/KasetTests/YouTubePlayerLoadingTimeoutTests.swift:247` on a busy or heavily
scheduled CI runner; the suite is `@MainActor` and `.serialized` at lines 4-5, so actor
scheduling affects these wall-clock sleeps.
2. `service.play(video:)` at `Tests/KasetTests/YouTubePlayerLoadingTimeoutTests.swift:254`
starts loading through `YouTubePlayerService.play(video:)` at
`Sources/Kaset/Services/Player/YouTubePlayerService.swift:517-545`, which schedules a
one-second timeout in `beginPlaybackLoading()` at
`Sources/Kaset/Services/Player/YouTubePlayerService.swift:1346-1364`.
3. After the 700-millisecond sleep at test line 257,
`reloadCurrentVideoForIdentitySwitch()` at line 258 starts another load and schedules a
fresh one-second timeout through `YouTubePlayerService.swift:579-616`.
4. The 400-millisecond sleep at test line 259 provides only 600 milliseconds of margin
before the replacement timeout. If the main actor or CI runner delays resumption by more
than that margin, the timeout executes at `YouTubePlayerService.swift:1354-1371`, clears
loading via `deferCurrentVideoReload()`, and the assertion at line 260 observes
`isPlaybackLoading == false`, producing an intermittent test failure.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** Tests/KasetTests/YouTubePlayerLoadingTimeoutTests.swift
**Line:** 257:260
**Comment:**
*Possible Bug: The test relies on a fixed 300-millisecond margin between the 1-second loading timeout and the assertion. On a busy CI runner, the initial sleep or the main-actor scheduling can exceed that margin, allowing the replacement timeout to fire before the `isPlaybackLoading` assertion and causing an intermittent failure. Synchronize on the replacement load starting, or assert using an injected clock instead of wall-clock sleeps.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
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
2 participants
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.
User description
Hand-merged cherry-pick of upstream sozercan/kaset sozercan#408 (
dfdb4ba, "improve YouTube playback reliability") onto the fork's 175-commit divergence, reconciled with the fork's #19 (⌘R refresh + network auto-retry).Taken (reliability core): loading-timeout + media-error recovery, runtime liveness (
currentMediaIsLive),ScrollForwardingWebView, generalized interstitial handling (isTrustedIntermediaryURL), item-parser thumbnail helpers, new reliability test suites.Kept the fork's version: PlayerBar / ShortsView / VideoWindowController (overflow menu, shorts rail, PiP always-on-top), plus the sozercan#374 perf optimization and ad-skip/live-edge fields. Subscribe strings → "Iscriviti" (free sense).
Follow-up fixes (same branch): restored the fork's duration>0 loading-finish so Shorts load, and
ScrollForwardingWebViewforwards to nextResponder to fix watch-page scroll lag.No playback/reliability test regressed. Pre-existing test-target failures on main (AppLocalizationTests, SettingsManager/ListenBrainz, storyboard flake) are unchanged.
🤖 Generated with Claude Code
CodeAnt-AI Description
Improve YouTube playback recovery, audio control, and watch-page interaction
What Changed
Impact
✅ Fewer videos stuck loading✅ No unintended audio during playback startup✅ Smoother scrolling and Shorts transitions💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.