From d0265dff6af9fc35947f2de3944ffb62188b717f Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 1 Jun 2026 08:38:54 -0400 Subject: [PATCH] feat(web): backflow play-event server-side classification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Web client now always POSTs play_ended with the actual position played to, matching the Android fix at d82e744d (Scribe 519). The server's skip rule (skip_max_completion_ratio + skip_max_duration_played_ms, default 50%/30s) classifies was_skipped from the duration_played_ms; the dispatcher no longer makes the call itself. Three transitions all collapse to play_ended: - Track change (was: reachedEnd ? play_ended : play_skipped) - Natural end via paused-at-duration (was already play_ended) - pagehide via sendBeacon (was: play_skipped) Drops the openReachedEnd field, COMPLETION_TOLERANCE_MS constant, and the position-vs-duration threshold check that backed the prior classification. play_skipped wire endpoint stays in place for a future explicit-dislike affordance. Two test cases updated: - "user-initiated next mid-track" now asserts play_ended with duration_played_ms = the partial position, not play_skipped. - "pagehide fires sendBeacon" now parses the beacon payload and asserts type=play_ended + duration_played_ms = last position. Flutter NOT touched — deprecated per M8. --- web/src/lib/player/events.svelte.test.ts | 33 ++++++++-- web/src/lib/player/events.svelte.ts | 79 ++++++++++-------------- 2 files changed, 60 insertions(+), 52 deletions(-) diff --git a/web/src/lib/player/events.svelte.test.ts b/web/src/lib/player/events.svelte.test.ts index d7ba8cee..4d3eca99 100644 --- a/web/src/lib/player/events.svelte.test.ts +++ b/web/src/lib/player/events.svelte.test.ts @@ -121,27 +121,40 @@ describe('useEventsDispatcher', () => { cleanup(); }); - test('user-initiated skip mid-track POSTs play_skipped', async () => { + test('user-initiated next mid-track POSTs play_ended with the actual position', async () => { + // Under the post-d82e744d shape the client always sends play_ended; + // the server's skip rule (skip_max_completion_ratio + + // skip_max_duration_played_ms) decides was_skipped from the + // duration_played_ms. play_skipped is reserved for a future + // explicit-dislike affordance and isn't called from track-change. const cleanup = $effect.root(() => useEventsDispatcher()); playQueue([track('1'), track('2')]); flushSync(); + reportDuration(200); reportStateFromAudio('playing'); flushSync(); await Promise.resolve(); await Promise.resolve(); + // Played 40 seconds in, then user hit next. + reportTimeUpdate(40); + flushSync(); skipNext(); flushSync(); await Promise.resolve(); - const calls = (api.post as ReturnType).mock.calls; - const types = calls.map((c) => (c[1] as { type: string }).type); - expect(types).toContain('play_skipped'); + const closesForPe1 = (api.post as ReturnType).mock.calls + .map((c) => c[1] as { type: string; play_event_id?: string; duration_played_ms?: number }) + .filter((b) => b.play_event_id === 'pe-1'); + expect(closesForPe1.some((b) => b.type === 'play_ended')).toBe(true); + expect(closesForPe1.some((b) => b.type === 'play_skipped')).toBe(false); + const ended = closesForPe1.find((b) => b.type === 'play_ended'); + expect(ended?.duration_played_ms).toBe(40000); cleanup(); }); - test('pagehide fires sendBeacon when a play is open', async () => { + test('pagehide fires sendBeacon with play_ended carrying last position', async () => { // jsdom doesn't ship navigator.sendBeacon — install a stub before spy. if (typeof navigator.sendBeacon !== 'function') { Object.defineProperty(navigator, 'sendBeacon', { @@ -152,17 +165,25 @@ describe('useEventsDispatcher', () => { const cleanup = $effect.root(() => useEventsDispatcher()); const beacon = vi.spyOn(navigator, 'sendBeacon').mockReturnValue(true); - playQueue([track('1')]); + playQueue([track('1', 200)]); flushSync(); + reportDuration(200); reportStateFromAudio('playing'); flushSync(); await Promise.resolve(); await Promise.resolve(); + reportTimeUpdate(73); + flushSync(); window.dispatchEvent(new Event('pagehide')); expect(beacon).toHaveBeenCalled(); expect(beacon.mock.calls[0][0]).toBe('/api/events'); + const blob = beacon.mock.calls[0][1] as Blob; + const text = await blob.text(); + const payload = JSON.parse(text) as { type: string; duration_played_ms: number }; + expect(payload.type).toBe('play_ended'); + expect(payload.duration_played_ms).toBe(73000); cleanup(); beacon.mockRestore(); diff --git a/web/src/lib/player/events.svelte.ts b/web/src/lib/player/events.svelte.ts index 44f1d148..d5468668 100644 --- a/web/src/lib/player/events.svelte.ts +++ b/web/src/lib/player/events.svelte.ts @@ -6,36 +6,34 @@ import type { PlayStartedResponse, EventOkResponse } from '$lib/api/types'; // useEventsDispatcher installs $effect-based watchers that emit play events // to /api/events. Call once from +layout.svelte's mount. // -// The dispatcher is a small state machine over (current track id, state). -// Per spec data-flow: +// The dispatcher is a small state machine over (current track id, state): // - First transition into 'playing' for a new track → POST play_started. -// - 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. +// - Track id changes while a play is open → close the prior row with +// play_ended carrying the actual position played to. Server applies +// its skip rule (skip_max_completion_ratio + skip_max_duration_played_ms, +// default 50%/30s) and decides was_skipped from that. // - Natural completion (state moves from 'playing' to 'paused' AND -// 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; +// position >= duration > 0) → POST play_ended on the open row. +// - pagehide with a live row → navigator.sendBeacon a play_ended with +// the last position. The server's rule still classifies skip-vs-not. +// +// Earlier shape made the skip-vs-ended call client-side via a "within 3s +// of duration" rule, which left the server's threshold dead code and +// treated every "next" tap as a skip even after most of the track played. +// Backflow of the Android fix (commit d82e744d, 2026-06-01); see Scribe +// task 519. Flutter client is deprecated and not updated. 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. + // pagehide beacon). openLastPositionMs tracks the *open row's* track + // specifically and is 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 it before the close branch reads + // it. 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(); @@ -45,12 +43,6 @@ export function useEventsDispatcher(): void { 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; - } - } } }); @@ -60,25 +52,20 @@ export function useEventsDispatcher(): void { const s = player.state; const tid = t?.id ?? null; - // 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). + // Track changed: close the prior open row. Always send play_ended + // with the actual position played; the server's skip rule decides + // whether to flag was_skipped. if (tid !== prevTrackId && openPlayEventId) { const id = openPlayEventId; - const finished = openReachedEnd; - const endedMs = openDurationMs > 0 ? openDurationMs : openLastPositionMs; - const skipMs = openLastPositionMs; + const dur = openLastPositionMs; openPlayEventId = null; openTrackId = null; - 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(() => {}); + api.post('/api/events', { + type: 'play_ended', + play_event_id: id, + duration_played_ms: dur + }).catch(() => {}); } // Entered playing for a new track (no open row yet for this track). @@ -98,9 +85,7 @@ 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, @@ -112,14 +97,16 @@ export function useEventsDispatcher(): void { prevState = s; }); - // Page close: sendBeacon a play_skipped if an open play exists. + // Page close: sendBeacon a play_ended if an open play exists. The + // server's skip rule decides classification from the duration we + // actually played. if (typeof window !== 'undefined') { const onPageHide = () => { if (!openPlayEventId) return; const payload = JSON.stringify({ - type: 'play_skipped', + type: 'play_ended', play_event_id: openPlayEventId, - position_ms: lastPositionMs + duration_played_ms: lastPositionMs }); navigator.sendBeacon('/api/events', new Blob([payload], { type: 'application/json' })); };