fix(player): stop Add-to-Playlist submenu flicker during playback - #421
fix(player): stop Add-to-Playlist submenu flicker during playback#421YuriNachos wants to merge 1 commit into
Conversation
cd1b651 to
eb0af49
Compare
|
The The two failures were both in That test keeps Supporting evidence: Happy to open a separate PR making that test wait on a signal instead of a sleep, if you'd like. |
|
Follow-up data point, which I think settles it: on the re-run of the same code, the matrix legs swapped.
Two different suites, neither of which this PR touches (the diff is one file, Both failures share a shape — a wall-clock delay on the mock client used to hold an async call open while a second call races it:
On a loaded runner those windows are not reliable. The fix in both cases is to gate on a signal the mock controls (e.g. resume from a continuation the test releases) rather than on elapsed time. Glad to send that as a separate PR if it's welcome — it's out of scope for this one. |
The red
|
| Run | Commit | Failing leg | Failing test |
|---|---|---|---|
| 31219673884 | cd1b651 |
macos-15 (macos-26 green) |
HistoryViewModelTests — "Load more is blocked while refresh rewinds the history cursor" |
| 31254736096 | eb0af49 |
macos-26 (macos-15 green) |
PlaylistDetailViewModelTests — "Generation-mismatched rollback restores the full pre-removal snapshot" |
A real regression does not swap legs and suites between runs.
Shared root cause. Both tests gate a concurrency assertion on a wall-clock delay in the mock client and then assume the in-flight task loses the race:
HistoryViewModelTests.swift:182—getHistoryDelay = .milliseconds(100), then assertsgetHistoryContinuationCallCount == 1. Observed on the failing runner:3.PlaylistDetailViewModelTests.swift:166—playlistContinuationDelay = .milliseconds(150), cancels the continuation task, then asserts the cancelled page was not appended. Observed: track"c"had already been appended →["a","b","c"].
On a loaded GitHub runner the sleep elapses before the cancel/guard lands, and the test flips. 100–150 ms is well inside the scheduling jitter of a shared runner. grep -c finds 67 call sites of this …Delay = .milliseconds(…) pattern across Tests/KasetTests, so the same shape can surface elsewhere.
Offer: happy to open a separate PR replacing the wall-clock gates with a deterministic handshake (a continuation the mock signals when it has been entered, released explicitly by the test) for these suites — it is unrelated to this change and belongs in its own PR. Say the word and I'll put it up.
To get a current result I refreshed the branch head (eb0af49 → b44d3ab) — the tree is byte-identical, no content changed.
eb0af49 to
b44d3ab
Compare
|
Update — the refreshed run landed, and it makes the flake case decisive: a third run, a third different suite, all on effectively the same code.
The newest one is in a suite I hadn't seen fail before and that is even further from this PR's diff: Three runs, three different suites, one unchanged one-file diff ( The deflake offer from my previous comment stands, and I'd now widen it beyond the two view-model suites — the cookie-restore failure looks like shared-state leakage between suites rather than the wall-clock race, so it may be a second, separate flake source worth a look. |
|
The only failing check is |
The context menu content was rebuilt on every playback-progress tick because it lived in PlayerBar.body's dependency graph. Extract it into a dedicated @observable view (CurrentSongContextMenuContent) that reads only low-frequency song/library state, so progress ticks no longer invalidate the menu. No playback behavior change; the full suite stays green.
b44d3ab to
fe590e9
Compare
|
The red This PR touches exactly one file — Across the last 25
Two things stand out:
The I have force-pushed the identical commit to trigger a fresh run so this PR can go green. Happy to open a separate issue or PR for the flaky suites if that would be useful — it looks like the cookie store and the view-model fixtures need per-suite isolation, but that is a different concern from this fix and I did not want to widen this PR. |
|
All the CI legs are green on this now, including the macos-26 run that flaked earlier — the run-link diagnosis of that flake is upthread in case it's ever useful. The change itself is still the single-file PlayerBar.swift fix that stops the Add-to-Playlist submenu from re-resolving and flickering mid-playback. Is there anything you'd like added or done differently before you get a chance to look — a UI test around the submenu, a smaller diff, anything? |
1 participant
Description
The "Add to Playlist" submenu (and the whole now-playing song context menu) was hidden and rebuilt roughly once per second while a song was actively playing, making it nearly impossible to use during playback — the flicker described in #320.
Root cause: the context menu content was built directly inside
PlayerBar.body.PlayerBar.bodyreadsPlayerService.progress, which ticks about every second during playback, so every progress tick invalidated and rebuilt the context menu content, producing the periodic hide/rebuild.Type of Change
Related Issues
Fixes #320.
Changes Made
CurrentSongContextMenuContent) that reads only the low-frequency song/library state it actually needs — not playback progress.PlayerBar.bodyno longer owns the menu's reactive content, so progress ticks stop invalidating it; the menu stays stable during playback.Sources/Kaset/Views/PlayerBar.swiftis the only changed file.Testing
swift buildgreen;swift test --skip KasetUITests: 2953 tests, 231 suites, all passed.Lint/format:
swiftlint lint --strict— 0 violations (615 files).swiftformat --lint .— 0 files require formatting.Checklist
swiftlint --strictandswiftformat --lint .(both clean)