Merge pull request 'fix: restore native Android play-event recording (History was empty)' (#89) from dev into main

android/app/src/main/java/com/fabledsword/minstrel/models/wire/EventsWire.kt

@@ -1,5 +1,9 @@
+@file:OptIn(ExperimentalSerializationApi::class)
+
 package com.fabledsword.minstrel.models.wire
 
+import kotlinx.serialization.EncodeDefault
+import kotlinx.serialization.ExperimentalSerializationApi
 import kotlinx.serialization.SerialName
 import kotlinx.serialization.Serializable
 
@@ -15,7 +19,11 @@ import kotlinx.serialization.Serializable
 
 @Serializable
 data class PlayStartedRequest(
-    val type: String = "play_started",
+    // Discriminator MUST stay on the wire. The app's Json has
+    // encodeDefaults=false, so without this a `type` left at its
+    // default is omitted entirely and the server 400s the event as
+    // "unknown event type" — silently dropping every native play.
+    @EncodeDefault(EncodeDefault.Mode.ALWAYS) val type: String = "play_started",
     @SerialName("track_id") val trackId: String,
     @SerialName("client_id") val clientId: String,
     val source: String? = null,
@@ -28,21 +36,21 @@ data class PlayStartedResponse(
 
 @Serializable
 data class PlayEndedRequest(
-    val type: String = "play_ended",
+    @EncodeDefault(EncodeDefault.Mode.ALWAYS) val type: String = "play_ended",
     @SerialName("play_event_id") val playEventId: String,
     @SerialName("duration_played_ms") val durationPlayedMs: Long,
 )
 
 @Serializable
 data class PlaySkippedRequest(
-    val type: String = "play_skipped",
+    @EncodeDefault(EncodeDefault.Mode.ALWAYS) val type: String = "play_skipped",
     @SerialName("play_event_id") val playEventId: String,
     @SerialName("position_ms") val positionMs: Long,
 )
 
 @Serializable
 data class PlayOfflineRequest(
-    val type: String = "play_offline",
+    @EncodeDefault(EncodeDefault.Mode.ALWAYS) val type: String = "play_offline",
     @SerialName("track_id") val trackId: String,
     @SerialName("client_id") val clientId: String,
     val at: String,

internal/playevents/writer.go

@@ -471,10 +471,15 @@ func (w *Writer) captureSessionVector(
 }
 
 // autoClosePriorOpen closes any open (ended_at IS NULL) play_event for the
-// user. Sets ended_at = at, duration_played_ms = min(at - started_at, track
-// duration), was_skipped = true. Skip rule is NOT applied — auto-closed
-// rows are flagged skipped regardless because we don't know what the user
-// actually heard.
+// user. Sets ended_at = at and duration_played_ms = min(at - started_at,
+// track duration) — our best estimate of how long the orphan played when
+// the client never sent play_ended (e.g. backgrounded mid-track). The same
+// skip rule as RecordPlayEnded is then applied to that estimate: an orphan
+// that sat open past the track's length reads as ratio ~1 → a real play, so
+// a fully-listened track that lost its close still lands in history instead
+// of being force-hidden. Deliberately writes no skip_events row — an
+// auto-close is an ambiguous signal, not a deliberate skip, and must not
+// feed the skip-ratio / recommendation signal.
 func (w *Writer) autoClosePriorOpen(
 	ctx context.Context,
 	q *dbq.Queries,
@@ -503,6 +508,7 @@ func (w *Writer) autoClosePriorOpen(
 	if track.DurationMs > 0 {
 		ratio = float64(elapsedMs) / float64(track.DurationMs)
 	}
+	isSkip := ratio < w.skipMaxCompletionRatio && elapsedMs < w.skipMaxDurationPlayedMs
 	ratioPtr := ratio
 	durPtr := elapsedMs
 	if _, err := q.UpdatePlayEventEnded(ctx, dbq.UpdatePlayEventEndedParams{
@@ -510,11 +516,11 @@ func (w *Writer) autoClosePriorOpen(
 		EndedAt:          pgtype.Timestamptz{Time: at, Valid: true},
 		DurationPlayedMs: &durPtr,
 		CompletionRatio:  &ratioPtr,
-		WasSkipped:       true,
+		WasSkipped:       isSkip,
 	}); err != nil {
 		return err
 	}
 	w.logger.Info("playevents: auto-closed prior open row",
-		"play_event_id", prior.ID, "elapsed_ms", elapsedMs)
+		"play_event_id", prior.ID, "elapsed_ms", elapsedMs, "was_skipped", isSkip)
 	return nil
 }

internal/playevents/writer_test.go

@@ -122,14 +122,32 @@ func TestRecordPlayStarted_AutoClosesPriorOpenRow(t *testing.T) {
 	if !prior.EndedAt.Valid {
 		t.Errorf("prior should be closed")
 	}
-	if !prior.WasSkipped {
-		t.Errorf("prior should be marked was_skipped=true (auto-close convention)")
+	// 45s elapsed on a 200s track clears the 30s duration threshold, so the
+	// skip rule's AND fails -> NOT a skip. An orphan that played long enough
+	// to count must still land in history rather than being force-hidden.
+	if prior.WasSkipped {
+		t.Errorf("auto-close at 45s should apply the skip rule -> was_skipped=false")
 	}
 	if prior.DurationPlayedMs == nil || *prior.DurationPlayedMs != 45_000 {
 		t.Errorf("duration_played_ms = %v, want 45000", prior.DurationPlayedMs)
 	}
 }
 
+func TestRecordPlayStarted_AutoCloseShortPlayIsSkip(t *testing.T) {
+	f := newFixture(t, 200_000)
+	t1 := time.Now().UTC()
+	first, _ := f.w.RecordPlayStarted(context.Background(), f.user, f.track, "c", t1)
+	// 5s elapsed: ratio 0.025 < 0.5 AND 5000 < 30000 -> both fail -> a skip.
+	t2 := t1.Add(5 * time.Second)
+	if _, err := f.w.RecordPlayStarted(context.Background(), f.user, f.track, "c", t2); err != nil {
+		t.Fatalf("second: %v", err)
+	}
+	prior, _ := f.q.GetPlayEventByID(context.Background(), first.PlayEventID)
+	if !prior.WasSkipped {
+		t.Errorf("auto-close at 5s should be was_skipped=true")
+	}
+}
+
 func TestRecordPlayStarted_AutoCloseCapsAtTrackDuration(t *testing.T) {
 	f := newFixture(t, 60_000) // 60s track
 	t1 := time.Now().UTC()