fix: un-freeze the seek bar after a stream restart or state round-trip - #61
Merged
Conversation
The progress bar could stop advancing permanently. Four pieces combined so that nothing could rebuild the extrapolation anchor: - ApplyMetadata let the group playback_state override the progress object's playback_speed, anchoring at effective speed 0 for any non-Playing state. - Freeze() nulls the anchor, and only fresh progress (a new PlaybackProgress instance, detected by ReferenceEquals) ever rebuilt it. - OnPlaybackStateChanged called Freeze() on every transition away from Playing with no counterpart on the way back. - OnPositionTimerTick returned early unless PlaybackState was Playing. The SDK synthesizes state transitions the server never sent, forcing Playing on stream/start and Idle on stream/end. A stream restart (format renegotiation, or a server implementing seek as end + start) therefore produces Idle then Playing, both carrying the same carried-forward progress instance. Nothing was fresh, Freeze() had run on Idle, and the Playing event could not re-anchor: the bar was dead for the rest of the track. A server signalling resume via a group/update carrying only playback_state hit the same dead end. Per spec (Sendspin/spec @ 3632c68) there is no 'paused' playback_state (README:681); playback_speed = 0 is the protocol's only pause signal (README:1448), and it is a required field whenever progress is sent (README:1446-1448). The state override was modelling a state the protocol does not have, using a signal that is not the pause signal. Derive advance/freeze from playback_speed alone, and add Resume(now) as the counterpart to Freeze(): with no anchor or a speed-0 anchor it re-anchors at the currently displayed position, using now as the anchor instant so the paused wall time is never added to the position, and the last known non-zero speed factor (1.0 until the server reports otherwise). It is a no-op while the position is already advancing and while no server-confirmed position exists, including after ResetForPendingTrackChange(). This matches the SendspinDroid reference. Freeze() itself is unchanged - capturing the last position on a genuine stop was always correct. Also remove the PlaybackState gate in OnPositionTimerTick. The tracker is now authoritative: it returns null with no anchor and holds the position still on a speed-0 anchor, so the gate could only suppress updates the tracker had already decided were correct. For a conformant pause the state stays 'playing', so it did nothing useful there, while for the SDK-synthesized Idle it silently discarded ticks - re-introducing exactly the coupling this fix removes. Two existing tests encoded the old semantics and were rewritten. PauseWithFreshProgress_OmittingSpeed_StaysFrozen becomes ConformantPause_SpeedZeroWhilePlaying_StaysFrozen: a spec-conformant pause carries speed 0, so it no longer needs the state to freeze. PauseWithFreshProgress_ExplicitNormalSpeed_StaysFrozen asserted that a not-playing state wins over an explicit speed of 1000 - the bug itself - and is replaced by FreshProgressWithSpeed_AdvancesRegardlessOfGroupState, which asserts what the spec requires. Two more now express a pause as speed 0 rather than a PlaybackState.Paused the protocol never emits.
Records the root cause, the spec lines that settle it (Sendspin/spec @ 3632c68 README:681, 1445-1448), the fix, and a behavior matrix covering conformant pause/resume, stream restart, non-conformant resume, and track change.
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
The progress bar could stop advancing permanently — audio kept playing while the bar sat still for the rest of the track. Found by comparing our implementation against the protocol spec and the reference implementations, not from logs.
Root cause: the anchor lifecycle was driven by
PlaybackState, but only fresh progress could ever rebuild an anchor. Two real paths reached the dead state:Playingonstream/startandIdleonstream/end. A format renegotiation (or a server implementing seek as end+start) emitsIdlethenPlaying, both carrying the same carried-forwardPlaybackProgressinstance.Freeze()ran onIdle; nothing was fresh, so thePlayingevent could not re-anchor.group/updateonly, which carries justplayback_state/group_id/group_name— no progress to re-anchor from.Spec basis
Validated against Sendspin/spec @
3632c68:playback_stateis only'playing' | 'stopped'(README:681) — there is nopausedstate, so playback state cannot be the pause signal.playback_speed: 0means paused (README:1448) — this is the protocol's only pause signal.progress,track_progress/track_duration/playback_speedare all required (README:1446-1448), so a conformant server always sends the speed when it sends progress.Changes
playback_speedalone; the group playback state no longer overrides it.TrackProgressTracker.Resume(now)re-anchors at the current displayed position withnowas the anchor time and the last known non-zero speed. Anchoring atnowis what prevents a jump forward by the paused duration. No-op when already advancing, when no position is known yet, or after a pending track-change reset.MainViewModel.OnPlaybackStateChanged: Playing →Resume(), otherwiseFreeze().PlaybackState != Playingearly return in the position timer. It was wrong in both directions: on a conformant pause the state staysplayingso it did nothing, and during a synthesizedIdleit discarded ticks the tracker had already judged correct. The tracker is now authoritative; genuine stops are still covered byFreeze()nulling the anchor.playbackStateparameter now emits a Debug log when speed and state disagree — useful signal for diagnosing non-conformant servers.docs/superpowers/specs/2026-07-20-seek-bar-freeze-design.md.Tests
8 new cases covering both trigger paths, resume-in-place (no pause-duration jump), speed-0 anchors, and the no-op guards. Tracker suite 43/43. Full suite 140/142 — the 2 failures are the known pre-existing resampler ones, unchanged. Release build clean at the 553-warning baseline.
Four existing tests were rewritten because they encoded the bug: two asserted that a non-playing state overrides an explicit
playback_speed, and depended on aPlaybackState.Pausedthe protocol never emits plus aplayback_speedthe spec makes required. They passed only because the test supplied both sides of a boundary that never behaves that way on the wire.Manual verification