From 8652d7124e8b76e49ee720fb61e1e0399d45cf63 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 15 May 2026 10:45:11 -0400 Subject: [PATCH] fix(web): #426 classify in-queue track end as play_ended, not skip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The events dispatcher closed every prior open row as play_skipped on any track-id change. Server-side RecordPlaySkipped force-sets was_skipped=true regardless of completion, so a queue played start to finish reported tracks 1..N-1 as skips — inflating recommendation skip-ratios (-skipRatio*SkipPenalty) and degrading For You / Discover quality for web listeners. Now: on track change, close the prior row as play_ended if that track reached ~its duration (3s tolerance, matching the Flutter PlayEventsReporter), else play_skipped at the real last position. Race fix: the store synchronously resets position/duration to 0 on track change, so reading lastPositionMs at change-time would see 0 and misclassify. Track per-open-row state (openReachedEnd, openLastPositionMs, openDurationMs) updated ONLY while the open track is current — a track change can't clobber them before the close branch runs. Brings web wire behavior back in line with Flutter. Test added: auto-advance after reaching duration → play_ended, never skipped; existing mid-track-skip and pause-at-end tests still hold. Co-Authored-By: Claude Opus 4.7 (1M context) --- web/src/lib/player/events.svelte.test.ts | 29 +++++++++++ web/src/lib/player/events.svelte.ts | 61 +++++++++++++++++++----- 2 files changed, 79 insertions(+), 11 deletions(-) diff --git a/web/src/lib/player/events.svelte.test.ts b/web/src/lib/player/events.svelte.test.ts index 74e324f8..d7ba8cee 100644 --- a/web/src/lib/player/events.svelte.test.ts +++ b/web/src/lib/player/events.svelte.test.ts @@ -92,6 +92,35 @@ describe('useEventsDispatcher', () => { cleanup(); }); + test('auto-advance after reaching duration POSTs play_ended, not skip (#426)', async () => { + const cleanup = $effect.root(() => useEventsDispatcher()); + + playQueue([track('1', 200), track('2', 200)]); + flushSync(); + reportDuration(200); + reportStateFromAudio('playing'); + flushSync(); + await Promise.resolve(); + await Promise.resolve(); + // Track 1 plays through to its end... + reportTimeUpdate(200); + flushSync(); + // ...then the queue advances to track 2 (track id changes, store + // resets position/duration to 0 synchronously). + skipNext(); + flushSync(); + await Promise.resolve(); + + const closesForPe1 = (api.post as ReturnType).mock.calls + .map((c) => c[1] as { type: string; play_event_id?: string }) + .filter((b) => b.play_event_id === 'pe-1'); + // The finished track must close as ended, never skipped. + expect(closesForPe1.some((b) => b.type === 'play_ended')).toBe(true); + expect(closesForPe1.some((b) => b.type === 'play_skipped')).toBe(false); + + cleanup(); + }); + test('user-initiated skip mid-track POSTs play_skipped', async () => { const cleanup = $effect.root(() => useEventsDispatcher()); diff --git a/web/src/lib/player/events.svelte.ts b/web/src/lib/player/events.svelte.ts index ab383c51..44f1d148 100644 --- a/web/src/lib/player/events.svelte.ts +++ b/web/src/lib/player/events.svelte.ts @@ -9,21 +9,49 @@ import type { PlayStartedResponse, EventOkResponse } from '$lib/api/types'; // The dispatcher is a small state machine over (current track id, state). // Per spec data-flow: // - First transition into 'playing' for a new track → POST play_started. -// - Track id changes while a play is open → POST play_skipped on the prior. +// - Track id changes while a play is open → close the prior row: +// play_ended if it reached ~its duration, else play_skipped (#426). +// Without this every auto-advanced in-queue track was reported as a +// skip, force-flagging was_skipped=true server-side and poisoning +// the recommendation skip-ratio. Mirrors the Flutter reporter. // - Natural completion (state moves from 'playing' to 'paused' AND -// position >= duration > 0) → POST play_ended on the open row. +// position >= duration > 0) → POST play_ended on the open row +// (covers the last track / explicit pause-at-end). // - pagehide with a live row → navigator.sendBeacon a play_skipped. + +// Within this much of the known duration counts as "finished", not a +// skip. Matches the Flutter PlayEventsReporter tolerance. +const COMPLETION_TOLERANCE_MS = 3000; + export function useEventsDispatcher(): void { let openPlayEventId: string | null = null; let openTrackId: string | null = null; + // lastPositionMs tracks the *current* track's position (used by the + // pagehide beacon). openLastPositionMs / openReachedEnd track the + // *open row's* track specifically and are updated ONLY while that + // track is current — so a track change (which synchronously resets + // the store's position to 0 and duration to 0) can't clobber them + // before the close branch reads them. This is the #426 race fix. let lastPositionMs = 0; + let openLastPositionMs = 0; + let openDurationMs = 0; + let openReachedEnd = false; let prevTrackId: string | null = null; let prevState: string | null = null; const clientId = getOrCreateClientId(); - // Track the latest position for use by close events. Cheap; no API call. $effect(() => { - lastPositionMs = Math.round(player.position * 1000); + const posMs = Math.round(player.position * 1000); + lastPositionMs = posMs; + if (openTrackId && player.current?.id === openTrackId) { + openLastPositionMs = posMs; + if (player.duration > 0) { + openDurationMs = Math.round(player.duration * 1000); + if (posMs >= openDurationMs - COMPLETION_TOLERANCE_MS) { + openReachedEnd = true; + } + } + } }); // Main state machine. Reacts only to (current track id, state) transitions. @@ -32,17 +60,25 @@ export function useEventsDispatcher(): void { const s = player.state; const tid = t?.id ?? null; - // Track changed: close any prior open play as a skip. + // Track changed: close the prior open row. If that track reached + // ~its duration it finished naturally (auto-advance) → play_ended + // with its full duration; otherwise the user moved on early → + // play_skipped at the last position it actually reached (#426). if (tid !== prevTrackId && openPlayEventId) { const id = openPlayEventId; - const pos = lastPositionMs; + const finished = openReachedEnd; + const endedMs = openDurationMs > 0 ? openDurationMs : openLastPositionMs; + const skipMs = openLastPositionMs; openPlayEventId = null; openTrackId = null; - api.post('/api/events', { - type: 'play_skipped', - play_event_id: id, - position_ms: pos - }).catch(() => {}); + openDurationMs = 0; + openLastPositionMs = 0; + openReachedEnd = false; + api.post('/api/events', + finished + ? { type: 'play_ended', play_event_id: id, duration_played_ms: endedMs } + : { type: 'play_skipped', play_event_id: id, position_ms: skipMs } + ).catch(() => {}); } // Entered playing for a new track (no open row yet for this track). @@ -62,6 +98,9 @@ export function useEventsDispatcher(): void { const dur = lastPositionMs; openPlayEventId = null; openTrackId = null; + openDurationMs = 0; + openLastPositionMs = 0; + openReachedEnd = false; api.post('/api/events', { type: 'play_ended', play_event_id: id,