diff --git a/packages/frontend/src/hooks/__tests__/usePlayTracking.test.ts b/packages/frontend/src/hooks/__tests__/usePlayTracking.test.ts index 529af828..d4611869 100644 --- a/packages/frontend/src/hooks/__tests__/usePlayTracking.test.ts +++ b/packages/frontend/src/hooks/__tests__/usePlayTracking.test.ts @@ -129,8 +129,9 @@ describe('usePlayTracking', () => { expect(plays).toHaveLength(0); }); - it('records a play once the threshold is reached', async () => { + it('records a play when the track is done with, not partway through', async () => { const track = createMockTrack('track-2'); + const next = createMockTrack('track-2b'); const { rerender } = renderHook(() => usePlayTracking()); act(() => { @@ -141,6 +142,15 @@ describe('usePlayTracking', () => { usePlayerStore.setState({ currentTime: 95 }); }); rerender(); + await flush(); + + // Past half the track, which used to be enough to send it. Nothing goes yet. + expect(mockDeliver.mock.calls.filter((c) => c[1] === 'played')).toHaveLength(0); + + act(() => { + usePlayerStore.setState({ currentTrack: next, currentTime: 0, _advanceReason: 'ended' }); + }); + rerender(); await waitFor(() => { expect(mockDeliver).toHaveBeenCalledWith( @@ -152,6 +162,42 @@ describe('usePlayTracking', () => { }, { timeout: 2000 }); }); + /** + * The defect this hook's note describes, pinned. + * + * Delivering at the halfway mark froze `completion_ratio` at ~0.5 for every web play + * regardless of how much was really heard — 289 of 357 completed rows on the live + * database sat in the 0.5–0.6 bucket. Completion is what ADR-0005 ranks on, so a + * constant made the whole signal worthless. A track heard almost to the end must + * report almost 1. + */ + it('reports the ratio of the whole listen, not the moment it passed half', async () => { + const track = createMockTrack('track-full'); + const next = createMockTrack('track-full-b'); + const { rerender } = renderHook(() => usePlayTracking()); + + act(() => { + usePlayerStore.setState({ currentTrack: track, isPlaying: true, duration: 180, currentTime: 0 }); + }); + rerender(); + // Straight past the old halfway trigger and on to the end of the track. + act(() => { usePlayerStore.setState({ currentTime: 90 }); }); + rerender(); + act(() => { usePlayerStore.setState({ currentTime: 178 }); }); + rerender(); + + act(() => { + usePlayerStore.setState({ currentTrack: next, currentTime: 0, _advanceReason: 'ended' }); + }); + rerender(); + + await waitFor(() => { + const play = mockDeliver.mock.calls.find((c) => c[0] === 'track-full' && c[1] === 'played'); + expect(play).toBeDefined(); + expect((play![2] as { completion_ratio?: number }).completion_ratio).toBeCloseTo(0.99, 2); + }, { timeout: 2000 }); + }); + it('does not accumulate time while paused', async () => { const track = createMockTrack('track-3'); const { rerender } = renderHook(() => usePlayTracking()); @@ -299,22 +345,20 @@ describe('usePlayTracking', () => { const b = { ...createMockTrack('track-b'), duration_seconds: 374 }; const { rerender } = renderHook(() => usePlayTracking()); - // A plays past its threshold and is recorded. + // A plays, then the crossfade advances to B — which is where A is recorded. act(() => { usePlayerStore.setState({ currentTrack: a, isPlaying: true, duration: 75, currentTime: 0 }); }); rerender(); act(() => { usePlayerStore.setState({ currentTime: 40 }); }); rerender(); - await waitFor(() => { - expect(mockDeliver.mock.calls.filter((c) => c[1] === 'played')).toHaveLength(1); - }); - - // Crossfade advances to B... act(() => { usePlayerStore.setState({ currentTrack: b, currentTime: 0, _advanceReason: 'crossfade' }); }); rerender(); + await waitFor(() => { + expect(mockDeliver.mock.calls.filter((c) => c[1] === 'played')).toHaveLength(1); + }); // ...then fails, rolling straight back to A having played none of B. act(() => { usePlayerStore.setState({ currentTrack: a, currentTime: 40, _advanceReason: 'error' }); @@ -334,42 +378,86 @@ describe('usePlayTracking', () => { it('still records a genuine replay after another track was listened to', async () => { const a = createMockTrack('track-a'); const b = createMockTrack('track-b'); + const c = createMockTrack('track-c'); const { rerender } = renderHook(() => usePlayTracking()); + // A plays, then B — which records A. act(() => { usePlayerStore.setState({ currentTrack: a, isPlaying: true, duration: 180, currentTime: 0 }); }); rerender(); act(() => { usePlayerStore.setState({ currentTime: 95 }); }); rerender(); + act(() => { + usePlayerStore.setState({ currentTrack: b, currentTime: 0, _advanceReason: 'ended' }); + }); + rerender(); await waitFor(() => { expect(mockDeliver.mock.calls.filter((c) => c[1] === 'played')).toHaveLength(1); }); - // B genuinely plays through. + // B genuinely plays through, then back to A — which records B and, because a + // different track was really listened to, frees A to be recorded again. + act(() => { usePlayerStore.setState({ currentTime: 95 }); }); + rerender(); act(() => { - usePlayerStore.setState({ currentTrack: b, currentTime: 0, _advanceReason: 'ended' }); + usePlayerStore.setState({ currentTrack: a, currentTime: 0, _advanceReason: 'user' }); }); rerender(); - act(() => { usePlayerStore.setState({ currentTime: 95 }); }); - rerender(); await waitFor(() => { expect(mockDeliver.mock.calls.filter((c) => c[1] === 'played')).toHaveLength(2); }); - // Returning to A is a real replay and must count. + // The replay of A is a real listen and must count on the way out of it. + act(() => { usePlayerStore.setState({ currentTime: 95 }); }); + rerender(); act(() => { - usePlayerStore.setState({ currentTrack: a, currentTime: 0, _advanceReason: 'user' }); + usePlayerStore.setState({ currentTrack: c, currentTime: 0, _advanceReason: 'ended' }); }); rerender(); - act(() => { usePlayerStore.setState({ currentTime: 95 }); }); - rerender(); await waitFor(() => { - const plays = mockDeliver.mock.calls.filter((c) => c[1] === 'played' && c[0] === 'track-a'); + const plays = mockDeliver.mock.calls.filter((call) => call[1] === 'played' && call[0] === 'track-a'); expect(plays).toHaveLength(2); }); }); + + /** + * The durability the old halfway delivery was quietly providing: close the tab + * mid-track and the play still counted, because it had already been sent. Reporting + * at the end would have lost that silently, so it is restored here — best effort, but + * with the ratio that was actually heard rather than a frozen one. + */ + it('reports the track in progress when the page goes away', async () => { + const track = createMockTrack('track-hide'); + const { rerender } = renderHook(() => usePlayTracking()); + + act(() => { + usePlayerStore.setState({ currentTrack: track, isPlaying: true, duration: 180, currentTime: 0 }); + }); + rerender(); + act(() => { usePlayerStore.setState({ currentTime: 120 }); }); + rerender(); + await flush(); + expect(mockDeliver.mock.calls.filter((c) => c[1] === 'played')).toHaveLength(0); + + act(() => { window.dispatchEvent(new Event('pagehide')); }); + await flush(); + + const play = mockDeliver.mock.calls.find((c) => c[0] === 'track-hide' && c[1] === 'played'); + expect(play).toBeDefined(); + expect((play![2] as { completion_ratio?: number }).completion_ratio).toBeCloseTo(0.667, 2); + + // And it must not be reported a second time if the page survives and the track + // is later advanced past. + act(() => { + usePlayerStore.setState({ currentTrack: createMockTrack('track-after'), currentTime: 0, _advanceReason: 'ended' }); + }); + rerender(); + await flush(); + + expect(mockDeliver.mock.calls.filter((c) => c[0] === 'track-hide' && c[1] === 'played')).toHaveLength(1); + }); }); describe('context', () => { diff --git a/packages/frontend/src/hooks/usePlayTracking.ts b/packages/frontend/src/hooks/usePlayTracking.ts index 058bb8a3..197527ca 100644 --- a/packages/frontend/src/hooks/usePlayTracking.ts +++ b/packages/frontend/src/hooks/usePlayTracking.ts @@ -9,7 +9,6 @@ const log = createLogger('PlayTracking'); /** Reaching the scrobble threshold is what counts as a play. */ const MIN_PLAY_SECONDS = 30; -const MAX_PLAY_SECONDS = 4 * 60; /** * Map the client's advance reason onto the backend's `StopReason` (ADR-0004). @@ -33,6 +32,21 @@ function toStopReason(reason: AdvanceReason): ListenStopReason | null { } } +/** + * The share of the track that was heard, or undefined when the duration is unknown. + * + * Clamped, because the two figures come from different places: played time is accumulated + * from the engine's clock while the duration is track metadata, and a track that loops or + * whose tag understates its length can produce more played seconds than the track is long. + * The server clamps too, but sending 1.1 makes every stored figure a question about which + * end did the clamping — and the native client (`PlaybackReport.completionRatio`) has + * always clamped, so this is the two clients agreeing rather than a new rule. + */ +function completionRatio(playedSeconds: number, durationSeconds: number): number | undefined { + if (!durationSeconds) return undefined; + return Math.min(Math.max(playedSeconds / durationSeconds, 0), 1); +} + /** Queue source types line up with the backend's PlayContext, apart from 'library'. */ function toContext(sourceType: string | undefined): ListenContext | undefined { if (!sourceType) return undefined; @@ -46,8 +60,7 @@ function toContext(sourceType: string | undefined): ListenContext | undefined { * Track listening and report it to the backend. * * Two things are recorded: - * - a **play**, on the Last.fm rule — ≥30s listened AND ≥min(half the track, 4 min). - * This bumps ProfilePlayHistory and is unchanged from before. + * - a **play**, once ≥30s has been listened to. This bumps ProfilePlayHistory. * - a **listening event** for every track the listener moves on from, including short * skips, which previously produced no API call at all. This is the negative signal * ADR-0005's radio needs, and it only accumulates in real time. @@ -55,6 +68,21 @@ function toContext(sourceType: string | undefined): ListenContext | undefined { * The outcome is derived server-side from the stop reason plus completion ratio, so a * crossfade (which advances early, around 0.9) is not mistaken for a skip and a failed * load is never mistaken for dislike. + * + * **Reported when the track is done with, not partway through — and that is the whole + * point.** This used to deliver the play the moment listening crossed + * `min(duration / 2, 4 min)`, sending `completion_ratio` as measured *at that instant*. + * Nothing ever revised it, so every web play landed with a ratio of almost exactly 0.5 + * whether the listener heard half the track or all of it. Measured on the live database + * on 2026-08-01: **289 of 357 completed events sat in the 0.5–0.6 bucket**, against native + * client rows correctly reading 0.95–1.00. Completion is the taste signal ADR-0005 ranks + * on, and a constant carries none — so a month of "accumulating data" would have been a + * month of noise. + * + * Nothing changes about *which* plays count. The early delivery only decided *when*: a + * play short of the half mark but past 30s was already reported on track change, by the + * same rule that now reports all of them. The Last.fm half/4-minute threshold turned out + * to gate nothing, which is why it is gone rather than merely moved. */ export function usePlayTracking() { const { currentTrack, currentTime, duration, isPlaying } = usePlayerStore( @@ -100,14 +128,16 @@ export function usePlayTracking() { // Already counted as a play; nothing further to say about it. log.debug('outgoing track already recorded as a play', { previousId }); } else if (playedSeconds >= MIN_PLAY_SECONDS) { - // Partial play past the minimum but short of the scrobble threshold. Counts - // toward the aggregate, exactly as it did before. + // A play, reported now that the final figure is known. Marked as recorded for the + // same reason the threshold delivery used to be: so a crossfade rollback cannot + // report the same listen twice. + recordedTrackRef.current = previousId; void deliverListenEvent( previousId, 'played', { track_duration: outgoingDuration || undefined, - completion_ratio: outgoingDuration ? playedSeconds / outgoingDuration : undefined, + completion_ratio: completionRatio(playedSeconds, outgoingDuration), context, }, playedSeconds, @@ -175,30 +205,55 @@ export function usePlayTracking() { recordedTrackRef.current = null; } - const recordThreshold = Math.min(duration / 2, MAX_PLAY_SECONDS); + // Nothing is delivered here. The play is reported when the track is done with, where + // the completion ratio is final — see this hook's own note on why. + // eslint-disable-next-line react-hooks/exhaustive-deps -- Only re-run when track ID changes, not object reference + }, [currentTrack?.id, currentTime, duration, isPlaying]); - if (accumulatedTimeRef.current < MIN_PLAY_SECONDS) return; + // Report the track in progress if the page goes away before it is finished with. + // + // The cost of reporting at the end rather than partway through: closing the tab mid-track + // used to still count the play, because it had already been sent at the halfway mark. + // This restores that without restoring the frozen ratio — the figures here are whatever + // has actually been heard. + // + // Best effort, and honestly so. `pagehide` gives the request a chance to leave and the + // IndexedDB fallback a chance to catch it, but a browser tearing the page down owes + // neither of them anything. `visibilitychange` is the one that actually fires on mobile, + // where tabs are discarded without warning; marking the track recorded keeps a later + // track change from reporting the same listen twice if the page survives after all. + useEffect(() => { + const flush = () => { + const trackId = currentTrackIdRef.current; + const playedSeconds = accumulatedTimeRef.current; + if (!trackId || playedSeconds < MIN_PLAY_SECONDS) return; + if (recordedTrackRef.current === trackId) return; - if (accumulatedTimeRef.current >= recordThreshold) { - recordedTrackRef.current = currentTrack.id; - const played = accumulatedTimeRef.current; + const outgoingDuration = lastDurationRef.current; const { queueSource } = usePlayerStore.getState(); + recordedTrackRef.current = trackId; - deliverListenEvent( - currentTrack.id, + void deliverListenEvent( + trackId, 'played', { - track_duration: duration, - completion_ratio: duration ? played / duration : undefined, + track_duration: outgoingDuration || undefined, + completion_ratio: completionRatio(playedSeconds, outgoingDuration), context: toContext(queueSource?.type), }, - played, - ).catch((err) => { - // Reset so we can retry on the next threshold check - log.error('Failed to record play:', err); - recordedTrackRef.current = null; - }); - } - // eslint-disable-next-line react-hooks/exhaustive-deps -- Only re-run when track ID changes, not object reference - }, [currentTrack?.id, currentTime, duration, isPlaying]); + playedSeconds, + ); + }; + + const onVisibilityChange = () => { + if (document.visibilityState === 'hidden') flush(); + }; + + window.addEventListener('pagehide', flush); + document.addEventListener('visibilitychange', onVisibilityChange); + return () => { + window.removeEventListener('pagehide', flush); + document.removeEventListener('visibilitychange', onVisibilityChange); + }; + }, []); }