Fix YouTube Home load (stuck skeleton, latency) + floating window drag/crash#308
Open
sozercan wants to merge 14 commits into
Open
Fix YouTube Home load (stuck skeleton, latency) + floating window drag/crash#308sozercan wants to merge 14 commits into
sozercan wants to merge 14 commits into
Conversation
The pop-out video window was nearly impossible to move: its content is a corner-to-corner WKWebView that reports mouseDownCanMoveWindow == false and swallows mouseDown, defeating the window's isMovableByWindowBackground. Add a native top drag strip that drives NSWindow.performDrag explicitly (double-click still zooms). Also fix a SIGABRT when the window is resized very small: contentAspectRatio + contentMinSize are competing constraints, so AppKit let one axis slip below the floor and the macOS 26 NSHostingView safe-area corner-inset update raised an uncaught NSException in the display-cycle commit. Replace them with a single windowWillResize clamp (NSWindowDelegate) that floors and 16:9-snaps every proposed frame, and tighten observer cleanup.
…t cache Measured cold-launch time-to-first-grid was dominated by redundant work and a cache miss: - The 2 MB FEwhat_to_watch response was fetched and walked up to 3x (feed, chips, shelves). Coalesce into one getHomeBundle() that fetches once and parses off the main actor (raw Data -> value types), so deserialize + the recursive walk no longer block first paint. - On cold launch the account identity isn't loaded yet, so nothing was cached and getHomeShelves became a second serial 2 MB fetch on the critical path. Scope the cache to a stable "pending" identity until the real one resolves (invalidated on account switch, so no cross-account leakage), and check the cache before building auth headers. - Cache topic-rail browses (getHomeTopicFeed) so returning to Home is instant. Adds a raw-Data cache path (getData/setData) reusing the existing TTL/LRU store.
VideoCard requested a 640x360 target that ImageCache doubled to a 1280px decode for a <=320pt card — ~4x the pixels, thrashing the 50 MB memory cache. Right-size to 320x180. Make the disk read + ImageIO downsample nonisolated so warm-cache decodes parallelize across cores instead of serializing on the ImageCache actor, and prefetch the first screenful of thumbnails when the grid publishes.
Two cold-launch bugs in the Home view model, both diagnosed from a timestamped file trace (the app is sandboxed, so os_log streaming was unreliable): - Stuck on the skeleton until you navigated away and back. SwiftUI restarts the load .task twice ~18 ms apart on first paint; the restart cancelled load #1, load #2 bailed on the idle-guard, and #1's cancellation reset to .idle with nothing running. Make load() single-flight via a stored unstructured Task so the work survives .task cancellation and concurrent callers coalesce onto it. - The grid appeared ~840 ms before the side-scrolling rows, then snapped. All 8 topic rails were published atomically after the slowest one (~770 ms), even though fast rails finished at ~300 ms. Stream each rail into `sections` as it resolves, slotted in chip order, so rows appear ~480 ms sooner and fill in progressively. (A first attempt that revealed a strict in-order prefix was caught by re-measuring — it just gated on the slow first chip.) Replaces the old cancel-aborts-the-load test (it encoded the bug) with tests for single-flight survival, refresh() restart, and incremental ordered publish.
The YouTube detail views used a bare .task { load() }, which does not re-fire
when the account swap rebuilds the view model (a fresh, idle instance) — so the
tab could sit unloaded until a navigation changed view identity. Key the load
task on ObjectIdentifier(viewModel) (the pattern YouTubeExploreView already
uses) on Home, Shorts, Subscriptions, History, and Playlists.
Home also fades the side-scrolling rails in (and eases the grid down) as they
stream in, so the progressive fill reads as intentional motion.
Add a Debugging & Measurement section to AGENTS.md (measure before fixing timing/lifecycle bugs; the app is sandboxed so os_log and /tmp writes fail silently — use NSTemporaryDirectory file traces) and two new common-bug-patterns entries: the sandbox-safe trace template and the .task-restart cancellation deadlock with the single-flight load fix.
![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: afc79fcbcb
ℹ️ 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 is a batch of YouTube-mode fixes centered on the Home feed cold-launch experience and the pop-out video window. On Home, it replaces the idle-guarded load() with a single-flight unstructured Task that survives SwiftUI .task cancellation (fixing the stuck-skeleton bug), coalesces the home feed/chips/shelves into one off-main-actor getHomeBundle() fetch+parse, streams topic rails into sections as each resolves (instead of one atomic snap), enables cold-launch caching under a stable "pending" scope, and right-sizes/parallelizes thumbnail decodes. For the floating video window, it swaps the conflicting contentAspectRatio+contentMinSize pair for a single windowWillResize clamp delegate (fixing the SIGABRT on tiny resizes) and adds a native top drag strip. It also documents a measurement-first debugging workflow.
Changes:
- Home load reliability + perceived latency: single-flight load, coalesced
getHomeBundle()parsed off-main, incremental rail streaming, cache-before-auth +pendingcache scope, thumbnail right-sizing/prefetch,.task(id:)re-keying across YouTube views. - Floating window:
YouTubeVideoWindowResizeGuarddelegate (16:9 + floor clamp) replacing competing constraints, plus aWindowDragHandledrivingNSWindow.performDrag; fullscreen observers now torn down in cleanup. - Docs: measurement-first / sandbox-logging guidance in
AGENTS.mdanddocs/common-bug-patterns.md.
Show a summary per file
| File | Description |
|---|---|
Sources/Kaset/ViewModels/YouTube/YouTubeHomeViewModel.swift |
Single-flight load(), streamTopicRails, refresh() cancel; latent loadTask clear race noted. |
Sources/Kaset/Services/API/YouTubeClient.swift |
getHomeBundle/requestData/performRequestData, cache-before-auth, pending scope helper. |
Sources/Kaset/Services/API/Parsers/YouTube/YouTubeFeedParser.swift |
parseHomeBundle(from: Data) one-pass parse; lacks a direct test. |
Sources/Kaset/Services/API/APICache.swift |
getData/setData raw-bytes boxing reusing TTL/LRU machinery. |
Sources/Kaset/Services/YouTubeProtocols.swift |
Adds getHomeBundle() to the protocol. |
Sources/Kaset/Services/API/MockUITestYouTubeClient.swift |
UI-test mock getHomeBundle. |
Sources/Kaset/Models/YouTube/YouTubeFeed.swift |
New YouTubeHomeBundle value type. |
Sources/Kaset/Utilities/ImageCache.swift |
Off-actor disk decode via nonisolated helpers + detached task. |
Sources/Kaset/Views/YouTube/VideoCard.swift |
Thumbnail decode target 640×360 → 320×180. |
Sources/Kaset/Views/YouTube/YouTubeHomeView.swift |
Rail-insertion animation, thumbnail prefetch task, .task(id:) keying. |
Sources/Kaset/Views/YouTube/YouTube{Shorts,Subscriptions,History,Playlists}View.swift |
.task(id: ObjectIdentifier(viewModel)) re-keying. |
Sources/Kaset/Views/YouTube/YouTubeVideoWindowController.swift |
Resize-guard delegate, drag handle, fullscreen-observer teardown. |
Tests/KasetTests/* |
New view-model, mock, and APICache raw-data tests. |
AGENTS.md, docs/common-bug-patterns.md |
Measurement-first debugging + single-flight pattern docs. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 20/20 changed files
- Comments generated: 2
- Restore auth validation before serving a cached personalized YouTube response
(request + requestData): a cleared/expired session must not keep rendering
private cached data until the entry expires.
- Key the image memory cache by URL *and* target size so a Home 320x180 decode
no longer satisfies the watch view's 1280x720 request (which left it blurry).
Disk still holds URL-keyed raw bytes, so sizes share one download.
- Single-flight load: token-gate `defer { loadTask = nil }` so a stale run
resuming late cannot null a newer run's handle, and cancel the outgoing Home
model's load in resetForAccountChange so a discarded model stops using the
shared client after the cache scope moved to the new account.
- Add direct parseHomeBundle(from:) tests (success + non-JSON throws) and a
single-flight regression test for the stale-resume race.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d19e4768f4
ℹ️ 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".
…story split Follow-ups from the review of the previous fix commit: - Cache the raw home bytes only AFTER a successful parse (and skip the write on cancellation), so a transient non-JSON 200 can't poison the 5-minute Home cache and make retries fail without re-fetching. - Check cancellation before mutating `homeContinuation`: the detached bundle parse isn't cancelled when the Home model is discarded, so an account switch must not let the stale task overwrite the new account's continuation. - Key the image in-flight table by URL + size too (not just the memory cache), so a small in-flight decode isn't handed back to a larger request. - Invalidate the API cache on signOut/sessionExpired, so the account-unknown "pending" cache scope can't serve the previous user's data after a re-login. - Start the topic rails without awaiting watch history: history runs as one more concurrent rail in the streamer (the single writer), Continue Watching slots in at the front when it lands, so a slow history call no longer delays the rows or keeps an empty grid on the skeleton. Adds tests for the rail/history split and the new mock history gate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c445f3d0f0
ℹ️ 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".
- Register the image in-flight task BEFORE the off-actor disk read. The detached loadFromDisk introduced an await between the memory miss and inFlight registration, so two concurrent cold requests for the same URL+size (e.g. the first-screen prefetch and a visible card) could both miss and each start a separate download. The whole disk-then-network path now runs inside one registered task so concurrent callers coalesce; the disk decode still runs detached for cross-core parallelism. - Fix a botched streamTopicRails doc comment (dangling "Measured…" fragment plus a duplicated summary line).
- Don't clear the Home skeleton on an empty publish. The first rail result is often history returning nil (no resumable watch history); with an empty grid and no shelves, flipping `.loaded` then flashed the "No recommendations" state before the topic rows arrived. Only clear when there is real content; the genuinely-empty case is still handled by the terminal `.loaded`. - Reset Shorts `currentShortId` when the view-model identity changes, so an account swap autoplays the new model's first short instead of leaving a stale, not-in-pager ID selected. - Capture the Home cache key before the network await (the authenticated scope) and only write if it still matches afterwards, so a sign-out mid-flight can't store the previous user's bytes under the account-unknown `pending` scope. Adds an empty-grid-no-flash regression test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83c641c069
ℹ️ 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".
… loadingMore - Restore the shared FEwhat_to_watch cache between Home and Shorts. getHomeBundle cached raw bytes under yt:data:browse while getShorts still read yt:browse, so navigating between them re-fetched the 2 MB response. Both now go through one homeResponseData() that reads/writes the single shared key. - Honor height-driven window resizes: normalizedContentSize takes the current size and follows whichever axis the user drags, so a vertical-edge drag no longer snaps back to the old width-derived 16:9 height. - Honor the user's "double-click a window's title bar to" setting (Zoom / Minimize / None) instead of always zooming; the comment now matches behavior. - Preserve `.loadingMore` during rail updates: the skeleton-clear and terminal publish now only flip from `.loading`, so a concurrent loadMore() on an empty-grid first page can't be reset to `.loaded` and start a duplicate continuation. Adds normalizedContentSize tests (width/height-driven + floor).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eac1a281a
ℹ️ 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".
The round-5 resize tests recomputed the expected value via the same float expression as the impl ((700*16/9).rounded()), which compared as unequal at the sub-ULP level on the CI runners (passed locally). Assert against the exact rounded integer literals (1244, 563) instead — matching the style of the width-driven test that passed.
…eneration The prior fix compared only the cache-scope key before/after the fetch, which can't catch a sign-out during an account-unknown load: the key stays `pending` across a nil->nil account transition, so the previous user's FEwhat_to_watch bytes were still written back under `pending` (readable after a re-login). APICache now exposes a monotonic `generation` bumped on every `invalidateAll()` (account switch / sign-out / session expiry). homeResponseData captures it before the network await and writes only if BOTH the scope key and the generation are unchanged, so a sign-out mid-flight rejects the stale write.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8fa3cd6e9
ℹ️ 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".
…on guard - Keep the home parse off the main actor. The prior revision validated the 2 MB response with JSONSerialization on the @mainactor client (cold path) and then parsed it again off-main — a main-thread 2 MB deserialize before first paint plus a redundant second parse, undercutting this PR's core goal. The off-main parseHomeBundle (which already throws on non-JSON) now drives caching: fetch -> parse off-main -> cache the raw bytes only after a successful parse. Home and Shorts share one homeBundle() loader. - Extend the cache-generation guard to request(): getHistory and the topic-rail browses cache through request(), which wrote results after its await without the generation check. A sign-out mid-flight (pending->pending, key unchanged) could store the previous user's history/topic data. Capture the generation before the await and only write if it's unchanged.
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
A batch of YouTube-mode fixes, mostly on the Home feed, plus the pop-out video window. Each fix was verified by tests and — for the runtime/timing bugs — by live measurement on a cold launch.
Home feed
.tasktwice ~18 ms apart on first paint; the restart cancelled load chore(deps): bump actions/checkout from 4.2.2 to 6.0.1 in the github-actions group #1, load chore(deps): bump the github-actions group across 1 directory with 2 updates #2 bailed on the idle-guard, and chore(deps): bump actions/checkout from 4.2.2 to 6.0.1 in the github-actions group #1's cancellation reset state to.idlewith nothing running. Fixed by makingload()single-flight via a stored unstructuredTaskthat survives.taskcancellation; concurrent callers coalesce onto it.sectionsas it resolves, slotted in chip order — first row ~480 ms sooner, progressive fill instead of a snap.FEwhat_to_watchresponse was fetched + walked up to 3×; coalesced into onegetHomeBundle()that parses off the main actor. Cold-launch caching was disabled until the account identity resolved (forcing a second serial 2 MB fetch on the critical path) — scoped to a stablependingidentity, cache-before-auth, and topic rails are now cached..task { load() }on Home/Shorts/Subscriptions/History/Playlists didn't re-fire when an account swap rebuilds the view model — keyed onObjectIdentifier(viewModel).Floating video window
isMovableByWindowBackground; added a native top drag strip drivingNSWindow.performDrag.contentAspectRatio+contentMinSizewere competing constraints; replaced with a singlewindowWillResizeclamp.Docs
/tmpwrites fail silently → useNSTemporaryDirectory()file traces) inAGENTS.md+docs/common-bug-patterns.md.Test plan
swift buildcleanswiftformat .andswiftlint --strictcleanswift test --skip KasetUITests— 1428 tests pass (added: single-flight survival, refresh restart, incremental ordered publish, raw-Data cache round-trip, single coalesced home fetch)