Skip to content

fix(player): stop the page URL being stored as track artwork - #447

Merged
sozercan merged 1 commit into
sozercan:mainfrom
tsibog:fix/player-bar-thumbnail-src
Aug 19, 2026
Merged

fix(player): stop the page URL being stored as track artwork#447
sozercan merged 1 commit into
sozercan:mainfrom
tsibog:fix/player-bar-thumbnail-src

Conversation

@tsibog

@tsibog tsibog commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Description

currentTrack.thumbnailURL is sometimes set to https://music.youtube.com/ — the app's own page URL — instead of artwork.

The observer script read the player-bar image with thumbEl.src. That is the IDL property, which resolves against the document base URL, so an <img> whose src attribute is empty or missing reports the page URL rather than an empty string. YouTube Music leaves the attribute momentarily empty while swapping tracks. Because the resolved value is a non-empty (truthy) string, the existing thumbEl.getAttribute('src') fallback never ran:

thumbnailUrl = thumbEl.src || thumbEl.getAttribute('src') || '';

The value was then written straight into the track model with no validation:

let thumbnailURL = URL(string: thumbnailUrl)

The result is a URL that fetches successfully — HTTP 200, ~2 KB of HTML — and then fails to decode as an image. Any surface that renders it without a fallback sits on its loading placeholder forever, because CachedAsyncImage has no error branch and simply keeps showing placeholder() when the decode returns nil.

This has been masked so far: SongThumbnailView recovers by falling back to i.ytimg.com/vi/<id>/hqdefault.jpg via its onFailure hook, which covers most thumbnails in the app.

Found while testing #440, whose artwork viewer has no such fallback and so spins indefinitely. Fixed here separately because the bad data is written on main today and affects every consumer of currentTrack.thumbnailURL.

How it was diagnosed

Temporary instrumentation in the artwork viewer logged the URL plus an independent GET of it to the sandbox tmp directory:

artworkURL=https://yt3.googleusercontent.com/ugDC…=w544-h544-l90-rj
status=200 bytes=142912          <- first track, real artwork

artworkURL=https://music.youtube.com/
status=200 bytes=2012            <- after a track change, the page URL

The instrumentation has been removed.

AI Prompt (Optional)

🤖 AI Prompt Used
N/A - Manual implementation

AI Tool: Claude

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 📚 Documentation update
  • 🎨 UI/UX improvement
  • ♻️ Refactoring (no functional changes)
  • 🧪 Test update
  • 🔧 Build/CI configuration

Related Issues

Relates to #440

Changes Made

  • SingletonPlayerWebView+ObserverScript.swift: Gate on the literal src attribute before trusting the resolved img.src, so an unset image reports '' instead of the document URL.
  • MiniPlayerWebView+Coordinator.swift: Added playbackBridgeThumbnailURLString(from:) and applied it at the single bridge ingestion point. Rejects empty, non-http(s), hostless, and bare-/ values so no non-image URL can reach the track model, whatever the DOM reports.
  • AutoplayRecoveryTests.swift: Coverage for the document URL, empty/whitespace/nil/non-string inputs, and pass-through of real artwork and i.ytimg.com fallback URLs.

Rejected input yields an empty string, which every existing caller already treats as "keep current artwork" — keepQueueSongVisible uses URL(string:) ?? song.thumbnailURL, and updateTrackMetadata guards let thumbnailURL before applying it or falls back to ?? currentTrack.thumbnailURL. No call site needed changing.

Testing

  • Unit tests pass (swift test --skip KasetUITests) — 2959 tests in 232 suites
  • Manual testing performed
  • UI tested on macOS 26+

Verified in a packaged build: play a track, change tracks, and confirm artwork-dependent surfaces keep real artwork instead of the page URL.

PlayerServiceQueueTests failed once during a full-suite run here, then passed 3/3 in isolation and 2/2 on subsequent full-suite runs, with a different subset of tests failing each time. This is the pre-existing parallel-execution flakiness in persisted state (same family as the FavoritesManagerTests/SettingsManagerTests/HistoryViewModelTests flakes seen on other branches); nothing here touches playback persistence.

Checklist

  • My code follows the project's style guidelines
  • I have run swiftlint --strict && swiftformat . — both clean
  • I have added tests that prove my fix/feature works
  • New and existing unit tests pass locally
  • I have updated documentation if needed
  • I have checked for any performance implications
  • My changes generate no new warnings

Additional Notes

The JS change keeps using thumbEl.src for the resolved absolute URL when the attribute is genuinely present, so relative artwork sources still resolve correctly. Only the empty/missing case changes behaviour.

The Swift guard is deliberate defence in depth: the DOM is third-party and can change shape without notice, and the failure mode here is a permanent loading state rather than a visible error, which makes it expensive to notice.

`img.src` resolves against the document base URL, so the player-bar image
reports `https://music.youtube.com/` whenever its `src` attribute is empty —
which happens while YouTube Music swaps tracks. That value is truthy, so the
existing `getAttribute('src')` fallback never ran, and it was written straight
into `currentTrack.thumbnailURL`.

The bad URL fetches successfully as HTML and then fails to decode, so surfaces
without a fallback show a permanent loading state. `SongThumbnailView` masked
it by falling back to the public video thumbnail.

Gate on the literal attribute before trusting the resolved value, and reject
non-image URLs at the bridge ingestion point so nothing downstream can store
one. Callers already treat an empty string as "keep existing artwork".

@sozercan sozercan left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! lgtm

@sozercan
sozercan merged commit 53763c1 into sozercan:main Aug 19, 2026
8 checks passed
httperry added a commit to httperry/kaset that referenced this pull request Aug 22, 2026
httperry added a commit to httperry/kaset that referenced this pull request Aug 22, 2026
Yoddikko pushed a commit to Yoddikko/kasetPlus that referenced this pull request Aug 27, 2026
Yoddikko added a commit to Yoddikko/kasetPlus that referenced this pull request Aug 27, 2026
…zercan#448

Cherry-picked from sozercan/kaset:
- sozercan#426 Bengali romanizer mixed-script crash fix
- sozercan#425 refresh Home suggestions on demand (adapted: kept fork shorts aggregation + chip-bar callers pass forceRefresh:false)
- sozercan#447 stop page URL stored as track artwork
- sozercan#448 eliminate cross-suite unit test flakes (WebKit cookie handling)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012LaRrBbQsc5W5agXFkYc7u
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants