Skip to content

fix(desktop): five corrections to the recent-played history (#419) - #625

Draft
TheZupZup wants to merge 1 commit into
mainfrom
fix/419-playback-history-followups
Draft

fix(desktop): five corrections to the recent-played history (#419)#625
TheZupZup wants to merge 1 commit into
mainfrom
fix/419-playback-history-followups

Conversation

@TheZupZup

Copy link
Copy Markdown
Owner

Follow-up to #611 (already merged). The Codex review landed five findings on the merged commit; I checked each against the code on main and all five reproduce, so each gets a fix and a test that fails without it.

The one that mattered (P1)

playbackHistoryProvider was lazy, and its only reader was the embedded QueueSheet — which is only built when the queue pane is open, and the pane starts closed. The controller's state stream is a plain broadcast stream with no replay, so nothing was recorded until the listener first opened the pane. Play an album, open Recently played, get an empty list. The feature didn't work for its main flow.

It is now read in bootstrapApplication beside the other side-effect-only services (smartPrecacheServiceProvider, remotePrebufferServiceProvider, …), which is where that pattern already lives. Off desktop build still creates no recorder, so the read costs one empty value.

The other four (P2)

Finding Why it's real
Repeat-one inherits the previous pass The controller publishes no completed status under repeat-one — _onCompleted calls _replayCurrent() and returns without emitting — so the recorder only sees the position jump back to zero on the same track. _furthest stayed at the first pass's end, and skipping the second pass halfway recorded a completed play. Each pass is now closed at the restart. A backwards seek is not a restart: it lands mid-track, not within replayThreshold of zero, and a track that never reached its end isn't replaying anything.
Fixed tolerance breaks short tracks _furthest >= _duration - endTolerance with a flat 2s puts the threshold at ≤ 0 for any track of 2s or less, so _furthest = 0 satisfied it and every skip of a short interlude read as "Played to the end". The tolerance is now capped at a quarter of the track, and reaching the end also requires _furthest > 0.
paused counted as played Crash restore loads the queue paused on purpose (restoreSession_playCurrent(autoplay: false)), and the engine reaches paused/ready before an autoplay call takes effect. A restored track nobody heard was flushed as "skipped". Paused now only preserves a _started that an active status already set.
Duplicate queue entry replays the wrong copy indexWhere picks the oldest match. For a queue history of [A, X, A] the entry stands for the second A, but tapping it stepped back past X to the first one. Now lastIndexWhere.

Tests

Six new tests, each verified to fail against the merged code before the fix (I stashed lib/ and re-ran — 5 failed in the two existing suites, plus the bootstrap one):

  • test/app/playback_history_bootstrap_test.dart (new) — walks the real bootstrapApplication graph and asserts container.exists(playbackHistoryProvider). Deliberately exists, not read: reading would create the provider and pass whether or not bootstrap wired it, which is precisely the bug. Plus the touch-host case still recording nothing.
  • playback_history_recorder_test.dart — repeat-one: each pass reported, finish-then-skip recorded as a skip, and a restart before the end still counting as one skip; short tracks: a 2s interlude skipped is a skip, played through is completed, and the tolerance scaling rather than vanishing; paused: a restored-and-never-played track earning nothing, while pausing a track that did play still records it.
  • queue_history_pane_test.dart — a song queued twice stepping back to its most recent play (asserting up-next afterwards), and tracks played before the pane is ever opened being kept.

Two existing tests encoded the buggy behaviour and are corrected:

  • "a repeat-one loop keeps one entry, not one per pass" asserted on the recorder's raw callbacks. Collapsing repeats is the history's job (dedup by uri, already tested); the recorder losing the pass boundary is what caused the mislabelling. Replaced with the paused-restore cases.
  • "seeking back from the end…" described the engine reporting position 0 for the outgoing track during a track change. It doesn't — a change carries the new track. That shape is what repeat-one publishes, so the test now scrubs to mid-track, which is what a real seek does.

test/app/linux_startup_test.dart gains the provider so the startup smoke keeps mirroring bootstrap.

Checks

  • dart format --set-exit-if-changed lib test — clean
  • flutter analyze lib test — no issues
  • flutter test — full suite, 4989 tests, all passing

Linux validation

docs/queue.md gains the repeat-one and short-track rules, the "recording starts with the app" note, and two manual checks: playing without opening the pane and then opening it, and finishing a song under repeat-one then skipping the next pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LLUbVqJ6e9qfW5GTEFm1gN


Generated by Claude Code

Follow-up to #611. An automated review flagged five issues on the merged
commit; all five reproduce, so each one gets a fix and a test that fails
without it.

The one that mattered: the history recorder was created by the queue pane, and
the pane starts closed. The controller's state stream does not replay, so
nothing was recorded until the listener first opened the pane — and the list
they opened it to read was empty. It is now started during bootstrap, beside
the other side-effect-only services. Off desktop it still creates no recorder.

The other four:

- repeat one never publishes a "finished" status (it seeks to zero and plays
  again), so the second pass inherited the first pass's furthest position and a
  skip halfway through it was recorded as a completed play. Each pass is now
  closed at the restart. A backwards seek is not a restart: it lands mid-track,
  not at zero, and a track that never reached its end is not replaying;
- the end tolerance was a flat two seconds, which puts the threshold at or
  below zero for a track of two seconds or less — so skipping a short interlude
  always read as a completed play. The tolerance is now capped at a quarter of
  the track, and reaching the end requires actual progress;
- a paused state set "this track played". A crash-restored queue is loaded
  paused on purpose and the engine reaches paused before an autoplay call takes
  effect, so a restored track nobody heard was recorded as skipped. Paused now
  only preserves a start an active status already made;
- replaying a song that appears twice in the queue jumped to the first copy.
  The entry stands for the most recent play, so it steps back to the last one.

docs/queue.md gains the repeat-one and short-track rules, the "recording starts
with the app" note, and two manual checks.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LLUbVqJ6e9qfW5GTEFm1gN
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.

1 participant