From d82e744d87c8cf35a567ff3fe967cc8da4f1d517 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 1 Jun 2026 08:32:05 -0400 Subject: [PATCH] feat(android): move skip classification from client to server PlayEventsReporter used to make the skip-vs-ended call client-side: if the play head reached within 3s of duration -> playEnded with full duration; otherwise -> playSkipped (forces was_skipped=true on the server regardless of actual play time). That left the server's configurable skip rule (skip_max_completion_ratio + skip_max_duration_played_ms, default 50%/30s) dead code and meant any "next" tap registered as a skip even after most of the track played - too strict per operator 2026-06-01. PlayEventsReporter now always calls playEnded with the actual position played to. Server's RecordPlayEnded applies the rule and decides was_skipped. If the play ran to completion, the last observed position is approximately the track duration -> ratio ~1 -> not skipped. If the user hit next mid-track, the duration the server sees is the real listen time and the rule fires normally. Drops the curReachedEnd field and COMPLETION_TOLERANCE_MS constant that backed the prior classification. playSkipped wire endpoint stays in place for a future explicit-dislike affordance (not wired to track-change transitions any more). Parity-map row updated. Web backflow tracked as task #57 (Scribe 521). Flutter not touched - deprecated per M8. --- .../minstrel/player/PlayEventsReporter.kt | 62 ++++++++----------- 1 file changed, 27 insertions(+), 35 deletions(-) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayEventsReporter.kt b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayEventsReporter.kt index f2f076fc..a8fe713c 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/player/PlayEventsReporter.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/player/PlayEventsReporter.kt @@ -9,7 +9,6 @@ import com.fabledsword.minstrel.cache.mutations.MutationQueue import com.fabledsword.minstrel.cache.mutations.PlayOfflinePayload import com.fabledsword.minstrel.di.ApplicationScope import com.fabledsword.minstrel.models.wire.PlayEndedRequest -import com.fabledsword.minstrel.models.wire.PlaySkippedRequest import com.fabledsword.minstrel.models.wire.PlayStartedRequest import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch @@ -21,8 +20,6 @@ import java.util.UUID import javax.inject.Inject import javax.inject.Singleton -private const val COMPLETION_TOLERANCE_MS = 3_000L - /** * Mobile play-event lifecycle reporter. Closes the parity gap where * Android-played tracks never reached the server — without this, @@ -31,11 +28,18 @@ private const val COMPLETION_TOLERANCE_MS = 3_000L * and system-playlist rotation didn't advance on mobile activity. * * State machine over (current track id, playing) observed against - * [PlayerController.uiState]. Mirrors Flutter's - * `play_events_reporter.dart`. The skip-vs-ended classification uses - * a 3-second tolerance to avoid marking naturally-finished in-queue - * tracks as skipped (which would dilute the recommendation - * skip-ratio). + * [PlayerController.uiState]. The client always calls `playEnded` + * with the actual position played to; the server applies its + * configurable skip rule (`skip_max_completion_ratio` AND + * `skip_max_duration_played_ms`, default 50% AND 30s) to decide + * was_skipped. Earlier shape made the classification client-side via + * a "within 3s of duration" rule, which left the server's threshold + * dead code and treated any "next" tap as a skip — too strict + * (operator 2026-06-01). + * + * `playSkipped` endpoint stays on the wire for a future explicit- + * dislike affordance, but isn't called from normal track-change + * transitions any more. * * Live calls go through [EventsApi]; failures fall through to the * offline [MutationQueue] so a flaky connection never loses a play. @@ -59,7 +63,6 @@ class PlayEventsReporter @Inject constructor( private var curSource: String? = null private var curLastPositionMs: Long = 0 private var curDurationMs: Long = 0 - private var curReachedEnd: Boolean = false // Server play_event_id when the live play_started succeeded; // null if start failed/offline — then the close is captured into @@ -106,12 +109,7 @@ class PlayEventsReporter @Inject constructor( // change (where currentTrack is briefly the new one). if (curTrackId != null && trackId == curTrackId) { curLastPositionMs = positionMs - if (durationMs > 0) { - curDurationMs = durationMs - if (positionMs >= curDurationMs - COMPLETION_TOLERANCE_MS) { - curReachedEnd = true - } - } + if (durationMs > 0) curDurationMs = durationMs } prevTrackId = trackId @@ -123,7 +121,6 @@ class PlayEventsReporter @Inject constructor( curSource = source curLastPositionMs = 0 curDurationMs = durationMs - curReachedEnd = false openPlayEventId = null startLive(trackId = trackId, source = source) } @@ -158,30 +155,26 @@ class PlayEventsReporter @Inject constructor( resetCurrent() return } - val reached = curReachedEnd - val lastPos = curLastPositionMs - val durationMs = if (reached && curDurationMs > 0) curDurationMs else lastPos + // Always send the actual position played to; the server's + // skip rule (50%/30s defaults, configurable) classifies + // was_skipped from this. If the track ran to completion the + // last observed position is ~= duration, which gives ratio ~1 + // and the server records a non-skip. The COMPLETION_TOLERANCE + // rounding the prior shape did is no longer needed — the + // server's threshold is the source of truth. + val durationMs = curLastPositionMs val source = curSource val id = openPlayEventId if (!viaOffline && id != null) { scope.launch { try { - if (reached) { - api.playEnded( - PlayEndedRequest( - playEventId = id, - durationPlayedMs = durationMs, - ), - ) - } else { - api.playSkipped( - PlaySkippedRequest( - playEventId = id, - positionMs = lastPos, - ), - ) - } + api.playEnded( + PlayEndedRequest( + playEventId = id, + durationPlayedMs = durationMs, + ), + ) } catch ( @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable, ) { @@ -200,7 +193,6 @@ class PlayEventsReporter @Inject constructor( curSource = null curLastPositionMs = 0 curDurationMs = 0 - curReachedEnd = false openPlayEventId = null }