diff --git a/.gitea/workflows/test-go.yml b/.gitea/workflows/test-go.yml index 0eae539f..b3eb990f 100644 --- a/.gitea/workflows/test-go.yml +++ b/.gitea/workflows/test-go.yml @@ -27,6 +27,7 @@ on: - 'go.mod' - 'go.sum' - 'sqlc.yaml' + - 'Makefile' - 'internal/**' - 'cmd/**' - '.golangci.yml' @@ -53,6 +54,9 @@ jobs: go version golangci-lint --version + - name: Generated code matches queries (sqlc) + run: make verify-generate + - name: go vet run: go vet ./... diff --git a/Makefile b/Makefile index fa1e2e04..7b5f63c0 100644 --- a/Makefile +++ b/Makefile @@ -1,10 +1,34 @@ -.PHONY: generate test test-short test-integration lint build +.PHONY: generate generate-go verify-generate test test-short test-integration lint build +# renovate: datasource=docker depName=sqlc/sqlc SQLC_VERSION := 1.31.1 +# Local codegen. Containerised so a dev needs no sqlc install. generate: docker run --rm -v "$(CURDIR):/src" -w /src sqlc/sqlc:$(SQLC_VERSION) generate +# Same codegen, run as a Go tool instead of a container. This is the CI path: +# the ci-go image already has Go, so it avoids docker-in-docker. Pinned to the +# SAME version as `generate` above so both routes emit identical output. +generate-go: + go run github.com/sqlc-dev/sqlc/cmd/sqlc@v$(SQLC_VERSION) generate + +# Fail if the committed generated code no longer matches the .sql sources. +# +# Nothing verified this before, so internal/db/dbq could silently drift from +# internal/db/queries — a hand-edit, a half-applied regen, or a schema change +# without a regen would all pass CI while the typed layer lied about the SQL. +# +# The diff is printed BEFORE the exit-code check on purpose: when this fails, +# the log then contains sqlc's exact expected output, which is what you commit. +verify-generate: generate-go + # -N (intent-to-add) so a BRAND-NEW generated file is visible to `git + # diff`, which otherwise ignores untracked paths entirely — a whole + # missing *.sql.go would sail through the check below. + git add -N -- internal/db/dbq + git --no-pager diff -- internal/db/dbq + git diff --quiet -- internal/db/dbq + test: go test -race ./... diff --git a/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt b/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt index 9614eacf..cd0215ea 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/api/endpoints/DiscoverApi.kt @@ -3,9 +3,13 @@ package com.fabledsword.minstrel.api.endpoints import com.fabledsword.minstrel.models.wire.ArtistSuggestionWire import com.fabledsword.minstrel.models.wire.CreateRequestBody import com.fabledsword.minstrel.models.wire.LidarrSearchResultWire +import com.fabledsword.minstrel.models.wire.SnoozeSuggestionBody +import com.fabledsword.minstrel.models.wire.SuggestionSnoozeWire import retrofit2.http.Body +import retrofit2.http.DELETE import retrofit2.http.GET import retrofit2.http.POST +import retrofit2.http.Path import retrofit2.http.Query /** @@ -30,4 +34,30 @@ interface DiscoverApi { @POST("api/requests") suspend fun createRequest(@Body body: CreateRequestBody) + + /** + * Parks a suggestion — "not right now", NOT a dislike. Time-boxed + * server-side (90 days) and never fed into the taste profile. + * + * [body] must carry the artist's name: candidates are out-of-library, so + * the server has no local row to resolve a display name from and returns + * 400 without it. + */ + @POST("api/discover/suggestions/{mbid}/snooze") + suspend fun snoozeSuggestion( + @Path("mbid") mbid: String, + @Body body: SnoozeSuggestionBody, + ) + + /** Brings a parked suggestion back. 404 when it wasn't snoozed. */ + @DELETE("api/discover/suggestions/{mbid}/snooze") + suspend fun unsnoozeSuggestion(@Path("mbid") mbid: String) + + /** + * Currently-parked suggestions. Server filters expired rows, so every + * row returned is still snoozed. This is the only route back to an + * un-snooze once the card has left the deck. + */ + @GET("api/discover/snoozes") + suspend fun listSnoozes(): List } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationQueue.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationQueue.kt index 79dda6ce..84a0bdf0 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationQueue.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationQueue.kt @@ -35,6 +35,12 @@ object MutationKind { // background avoids the duplicate + orphan row the old offline-on-stop // path produced (see 2026-06-11 contract audit). const val PLAY_ENDED: String = "play_ended" + + // #2374 suggestion snooze. ONE toggle kind rather than separate + // snooze/unsnooze kinds, mirroring LIKE_TOGGLE, so a snooze followed by + // an undo collapses to the latest intent instead of replaying as two + // opposed calls whose order decides the outcome. + const val SUGGESTION_SNOOZE_TOGGLE: String = "suggestion_snooze_toggle" } /** @@ -152,6 +158,25 @@ class MutationQueue @Inject constructor( ), ) + /** + * Queues a suggestion snooze (or its undo) for replay. [desiredSnoozed] + * is the TARGET state, so repeated taps collapse to one replay. + * + * [name] is carried even for an un-snooze, where the server ignores it, + * so a single payload shape serves both directions. + */ + suspend fun enqueueSuggestionSnoozeToggle( + mbid: String, + name: String, + desiredSnoozed: Boolean, + ): Long = insertUserDriven( + MutationKind.SUGGESTION_SNOOZE_TOGGLE, + json.encodeToString( + SuggestionSnoozeTogglePayload.serializer(), + SuggestionSnoozeTogglePayload(mbid, name, desiredSnoozed), + ), + ) + suspend fun enqueueRequestCancel(requestId: String): Long = insertUserDriven( MutationKind.REQUEST_CANCEL, json.encodeToString( @@ -192,6 +217,21 @@ class MutationQueue @Inject constructor( } } +/** + * Persisted payload for `MutationKind.SUGGESTION_SNOOZE_TOGGLE` (#2374). + * `desiredSnoozed` is the *target* state, matching [LikeTogglePayload], so + * the replayer can collapse repeated toggles for one candidate down to the + * last intent. Both directions are idempotent server-side: re-snoozing + * extends the window, and un-snoozing something already back is a 404 the + * replayer treats as permanent (nothing left to do). + */ +@Serializable +data class SuggestionSnoozeTogglePayload( + val mbid: String, + val name: String, + val desiredSnoozed: Boolean, +) + /** * Persisted payload for `MutationKind.QUARANTINE_UNFLAG` — the * `DELETE /api/quarantine/{trackId}` call lost during a connectivity diff --git a/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationReplayer.kt b/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationReplayer.kt index c3ddfb2d..ef54a76a 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationReplayer.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/cache/mutations/MutationReplayer.kt @@ -16,6 +16,7 @@ import com.fabledsword.minstrel.connectivity.NetworkStatusController import com.fabledsword.minstrel.connectivity.ServerHealth import com.fabledsword.minstrel.models.wire.PlayEndedRequest import com.fabledsword.minstrel.models.wire.PlayOfflineRequest +import com.fabledsword.minstrel.models.wire.SnoozeSuggestionBody import com.fabledsword.minstrel.auth.AuthStore import com.fabledsword.minstrel.cache.db.dao.CachedMutationDao import com.fabledsword.minstrel.cache.db.entities.CachedMutationEntity @@ -114,11 +115,12 @@ class MutationReplayer @Inject constructor( private suspend fun drain() { val rows = dao.getAll() - // Collapse superseded like-toggles: only the latest desired state per - // (entity) is replayed; older toggles for the same entity are dropped - // unsent. Without this, partial-failure + differential retry could - // replay an older toggle last and invert the final like state. - val superseded = supersededLikeToggleIds(rows) + // Collapse superseded toggles (likes, suggestion snoozes): only the + // latest desired state per entity is replayed; older toggles for the + // same entity are dropped unsent. Without this, partial-failure + + // differential retry could replay an older toggle last and invert the + // final state — a snooze the user already undid would come back. + val superseded = supersededToggleIds(rows, json) for (row in rows) { if (row.id in superseded) { dao.delete(row.id) @@ -131,25 +133,6 @@ class MutationReplayer @Inject constructor( } } - /** Row ids of like-toggles superseded by a later toggle for the same entity. */ - private fun supersededLikeToggleIds(rows: List): Set { - val latestByEntity = HashMap() - val superseded = HashSet() - rows.asSequence() - .filter { it.kind == MutationKind.LIKE_TOGGLE } - .forEach { row -> - val decoded = runCatching { - json.decodeFromString(LikeTogglePayload.serializer(), row.payload) - }.getOrNull() - if (decoded != null) { - val key = "${decoded.entityType}:${decoded.entityId}" - // `rows` is ascending by id, so a prior entry is always older. - latestByEntity.put(key, row.id)?.let(superseded::add) - } - } - return superseded - } - private suspend fun outcomeFor(row: CachedMutationEntity): Outcome = try { dispatch(row) } catch (e: HttpException) { @@ -182,6 +165,7 @@ class MutationReplayer @Inject constructor( MutationKind.PLAY_ENDED -> dispatchPlayEnded(row.payload) MutationKind.REQUEST_CANCEL -> dispatchRequestCancel(row.payload) MutationKind.PLAYBACK_ERROR_REPORT -> dispatchPlaybackErrorReport(row.payload) + MutationKind.SUGGESTION_SNOOZE_TOGGLE -> dispatchSuggestionSnoozeToggle(row.payload) // Unknown kind — drop so a stale schema entry can't wedge the queue. else -> Outcome.DROP } @@ -277,6 +261,24 @@ class MutationReplayer @Inject constructor( return Outcome.SENT } + /** + * Replays a suggestion snooze in whichever direction the payload asks for. + * + * The un-snooze branch can legitimately 404 (the row already lapsed, or a + * previous attempt landed and the response was lost). [outcomeFor] classes + * 404 as permanent → DROP, which is right: the user's intended end state + * already holds, so there is nothing left to send. + */ + private suspend fun dispatchSuggestionSnoozeToggle(payload: String): Outcome { + val decoded = json.decodeFromString(SuggestionSnoozeTogglePayload.serializer(), payload) + if (decoded.desiredSnoozed) { + discoverApi.snoozeSuggestion(decoded.mbid, SnoozeSuggestionBody(name = decoded.name)) + } else { + discoverApi.unsnoozeSuggestion(decoded.mbid) + } + return Outcome.SENT + } + private suspend fun dispatchPlaybackErrorReport(payload: String): Outcome { val decoded = json.decodeFromString(PlaybackErrorReportPayload.serializer(), payload) playbackErrorsApi.report( @@ -297,3 +299,46 @@ class MutationReplayer @Inject constructor( const val HTTP_TOO_MANY = 429 } } + +/** + * Row ids of desired-state toggles superseded by a later toggle for the same + * entity. Applies to every kind whose payload encodes a TARGET state rather + * than an action — like-toggles and suggestion snoozes (#2374) — because + * replaying a stale one last would invert the final state. + * + * Top-level and pure so it can be unit-tested without standing up a Retrofit + * instance. [rows] must be ascending by id (FIFO), which is what + * `CachedMutationDao.getAll()` returns. + */ +internal fun supersededToggleIds(rows: List, json: Json): Set { + val latestByEntity = HashMap() + val superseded = HashSet() + rows.asSequence() + .mapNotNull { row -> toggleKeyOf(row, json)?.let { key -> key to row.id } } + .forEach { (key, id) -> + // Ascending ids mean a prior entry for this key is always older. + latestByEntity.put(key, id)?.let(superseded::add) + } + return superseded +} + +/** + * Collapse key for a toggle row, or null when the row isn't a toggle — or its + * payload won't decode. Undecodable rows are deliberately left alone rather + * than grouped under a shared "corrupt" key, so one bad row can't suppress a + * good one behind it; the dispatcher DROPs it on its own. + * + * The kind is part of the key so two toggle kinds can never collide on the + * same entity id. + */ +private fun toggleKeyOf(row: CachedMutationEntity, json: Json): String? = when (row.kind) { + MutationKind.LIKE_TOGGLE -> runCatching { + json.decodeFromString(LikeTogglePayload.serializer(), row.payload) + }.getOrNull()?.let { "${row.kind}:${it.entityType}:${it.entityId}" } + + MutationKind.SUGGESTION_SNOOZE_TOGGLE -> runCatching { + json.decodeFromString(SuggestionSnoozeTogglePayload.serializer(), row.payload) + }.getOrNull()?.let { "${row.kind}:${it.mbid}" } + + else -> null +} diff --git a/android/app/src/main/java/com/fabledsword/minstrel/discover/data/DiscoverRepository.kt b/android/app/src/main/java/com/fabledsword/minstrel/discover/data/DiscoverRepository.kt index 2d561fd4..3a0db0e4 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/discover/data/DiscoverRepository.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/discover/data/DiscoverRepository.kt @@ -7,10 +7,14 @@ import com.fabledsword.minstrel.models.ArtistSuggestionRef import com.fabledsword.minstrel.models.LidarrRequestKind import com.fabledsword.minstrel.models.LidarrSearchResultRef import com.fabledsword.minstrel.models.SeedContributionRef +import com.fabledsword.minstrel.models.SuggestionSnoozeRef import com.fabledsword.minstrel.models.wire.ArtistSuggestionWire import com.fabledsword.minstrel.models.wire.CreateRequestBody import com.fabledsword.minstrel.models.wire.LidarrSearchResultWire import com.fabledsword.minstrel.models.wire.SeedContributionWire +import com.fabledsword.minstrel.models.wire.SnoozeSuggestionBody +import com.fabledsword.minstrel.models.wire.SuggestionSnoozeWire +import retrofit2.HttpException import retrofit2.Retrofit import retrofit2.create import javax.inject.Inject @@ -46,6 +50,69 @@ class DiscoverRepository @Inject constructor( suspend fun listSuggestions(): List = api.listSuggestions().map { it.toDomain() } + suspend fun listSnoozes(): List = + api.listSnoozes().map { it.toDomain() } + + /** + * Parks a suggestion ("not right now"). Offline-first per rule #100: on + * transport failure the target state is queued for the replayer rather + * than dropped. + * + * Always reports success to the caller. Unlike a request, a snooze has no + * meaningful failed state to show — the user asked for a card to go away, + * and it will, either now or when the queue drains. + */ + suspend fun snoozeSuggestion(mbid: String, name: String): Unit = toggleSnooze( + mbid = mbid, + name = name, + desiredSnoozed = true, + ) { api.snoozeSuggestion(mbid, SnoozeSuggestionBody(name = name)) } + + /** Brings a parked suggestion back. Same offline-first contract. */ + suspend fun unsnoozeSuggestion(mbid: String, name: String): Unit = toggleSnooze( + mbid = mbid, + name = name, + desiredSnoozed = false, + ) { api.unsnoozeSuggestion(mbid) } + + private suspend fun toggleSnooze( + mbid: String, + name: String, + desiredSnoozed: Boolean, + call: suspend () -> Unit, + ) { + try { + call() + } catch (e: HttpException) { + // A 4xx is the server's considered answer, not a lost call, so + // queueing it would be wrong twice over: the replay is guaranteed + // to fail again, and the enqueue would raise a "will sync when + // online" snackbar for something already settled. The common case + // is a 404 from un-snoozing a row that already lapsed — which is + // the end state the user wanted anyway. + if (!isPermanent(e.code())) { + mutationQueue.enqueueSuggestionSnoozeToggle(mbid, name, desiredSnoozed) + } + } catch ( + @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable, + ) { + // Transport failure — intentional swallow, same offline-first + // rationale as createRequest above. The queue carries the desired + // STATE, so a later undo supersedes this rather than fighting it + // on replay. + mutationQueue.enqueueSuggestionSnoozeToggle(mbid, name, desiredSnoozed) + } + } + + /** + * Mirrors MutationReplayer's classification so the enqueue decision here + * and the drop decision there can't disagree: 4xx is permanent except the + * two "retry me" statuses. + */ + private fun isPermanent(code: Int): Boolean = + code in HTTP_CLIENT_ERR_MIN..HTTP_CLIENT_ERR_MAX && + code != HTTP_TIMEOUT && code != HTTP_TOO_MANY + suspend fun search(query: String, kind: LidarrRequestKind): List = api.search(query = query, kind = kind.wire).map { it.toDomain() } @@ -85,6 +152,13 @@ class DiscoverRepository @Inject constructor( RequestOutcome.QUEUED } } + + private companion object { + const val HTTP_CLIENT_ERR_MIN = 400 + const val HTTP_CLIENT_ERR_MAX = 499 + const val HTTP_TIMEOUT = 408 + const val HTTP_TOO_MANY = 429 + } } // ── Mappers (internal — wire types stay out of UI) ── @@ -105,6 +179,7 @@ private fun ArtistSuggestionWire.toDomain(): ArtistSuggestionRef = ArtistSuggest name = name, imageUrl = imageUrl, attribution = attribution.map { it.toDomain() }, + matchedTags = matchedTags, ) private fun SeedContributionWire.toDomain(): SeedContributionRef = SeedContributionRef( @@ -112,6 +187,12 @@ private fun SeedContributionWire.toDomain(): SeedContributionRef = SeedContribut isLiked = isLiked, ) +private fun SuggestionSnoozeWire.toDomain(): SuggestionSnoozeRef = SuggestionSnoozeRef( + mbid = mbid, + name = name, + snoozedUntil = snoozedUntil, +) + private fun RequestCreatePayload.toBody(): CreateRequestBody = CreateRequestBody( kind = kind, artistMbid = artistMbid, diff --git a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverScreen.kt b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverScreen.kt index 56630ac7..02a21224 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverScreen.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverScreen.kt @@ -40,6 +40,7 @@ import com.fabledsword.minstrel.discover.data.RequestOutcome import com.fabledsword.minstrel.models.ArtistSuggestionRef import com.fabledsword.minstrel.models.LidarrRequestKind import com.fabledsword.minstrel.models.LidarrSearchResultRef +import com.fabledsword.minstrel.models.SuggestionSnoozeRef import com.fabledsword.minstrel.nav.Discover import com.fabledsword.minstrel.shared.widgets.ErrorRetry import com.fabledsword.minstrel.shared.widgets.LoadingCentered @@ -104,6 +105,18 @@ private fun DiscoverBody( ResultsState.Idle -> SuggestionsPane( state = state.suggestions, locallyRequestedMbids = state.locallyRequestedMbids, + snoozeUi = SnoozeUi( + locallySnoozedMbids = state.locallySnoozedMbids, + snoozes = state.snoozes, + // No snackbar on snooze: the row itself flips to "Not + // right now" with an Undo, so a snackbar would only + // repeat what the user can already see — and cover the + // next row while doing it. + onSnooze = { s -> scope.launch { viewModel.snoozeSuggestion(s) } }, + onUnsnooze = { mbid, name -> + scope.launch { viewModel.unsnoozeSuggestion(mbid, name) } + }, + ), onRequest = { s -> scope.launch { val outcome = viewModel.requestSuggestion(s) @@ -178,10 +191,23 @@ private fun KindChips(kind: LidarrRequestKind, onChange: (LidarrRequestKind) -> } } +/** + * The snooze surface's data and callbacks, bundled rather than threaded + * through as four more parameters — the pane grew from one action to three + * with slice 4 and the signatures stopped being readable. + */ +private data class SnoozeUi( + val locallySnoozedMbids: Set, + val snoozes: List, + val onSnooze: (ArtistSuggestionRef) -> Unit, + val onUnsnooze: (String, String) -> Unit, +) + @Composable private fun SuggestionsPane( state: SuggestionState, locallyRequestedMbids: Set, + snoozeUi: SnoozeUi, onRequest: (ArtistSuggestionRef) -> Unit, onRetry: () -> Unit, ) { @@ -194,6 +220,7 @@ private fun SuggestionsPane( ) is SuggestionState.Loaded -> SuggestionsList( items = state.items.filter { it.mbid !in locallyRequestedMbids }, + snoozeUi = snoozeUi, onRequest = onRequest, ) } @@ -202,6 +229,7 @@ private fun SuggestionsPane( @Composable private fun SuggestionsList( items: List, + snoozeUi: SnoozeUi, onRequest: (ArtistSuggestionRef) -> Unit, ) { LazyColumn( @@ -210,13 +238,61 @@ private fun SuggestionsList( ) { item { SuggestionsHeader() } if (items.isEmpty()) { - item { CenteredMessage("Listen to or like an artist to fill this in.") } + // An empty deck used to mean one thing — no listening signal yet. + // With snoozing it can also mean "you parked them all", and telling + // that user to go listen to something would be wrong advice. + item { + CenteredMessage( + if (snoozeUi.snoozes.isEmpty()) { + "Listen to or like an artist to fill this in." + } else { + "Nothing new right now — the artists you've parked are below." + }, + ) + } } else { items(items = items, key = { it.mbid }) { s -> - SuggestionTile(s = s, onRequest = { onRequest(s) }) + SuggestionTile( + s = s, + snoozed = s.mbid in snoozeUi.locallySnoozedMbids, + onRequest = { onRequest(s) }, + onSnooze = { snoozeUi.onSnooze(s) }, + onUnsnooze = { snoozeUi.onUnsnooze(s.mbid, s.name) }, + ) HorizontalDivider() } } + // Parked candidates live at the bottom of the same scroll, not behind a + // separate screen: it's a short list the user rarely needs, but it must + // be reachable — a snoozed candidate is gone from the deck above, so + // this is the only way back to it. + if (snoozeUi.snoozes.isNotEmpty()) { + item { SnoozedHeader() } + items(items = snoozeUi.snoozes, key = { "snoozed-${it.mbid}" }) { row -> + SnoozedTile( + row = row, + onUnsnooze = { snoozeUi.onUnsnooze(row.mbid, row.name) }, + ) + HorizontalDivider() + } + } + } +} + +@Composable +private fun SnoozedHeader() { + Column(modifier = Modifier.padding(horizontal = 16.dp, vertical = 12.dp)) { + HorizontalDivider(modifier = Modifier.padding(bottom = 12.dp)) + Text( + text = "Not right now", + style = MaterialTheme.typography.titleMedium, + color = MaterialTheme.colorScheme.onBackground, + ) + Text( + text = "These come back on their own. Nothing here counts against your taste profile.", + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt index 613c7bae..cbad59cb 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverTiles.kt @@ -14,8 +14,10 @@ import androidx.compose.foundation.shape.RoundedCornerShape import androidx.compose.material3.AssistChip import androidx.compose.material3.Button import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text +import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -24,14 +26,22 @@ import androidx.compose.ui.graphics.vector.ImageVector import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import coil3.compose.AsyncImage +import com.composables.icons.lucide.Clock import com.composables.icons.lucide.Disc3 import com.composables.icons.lucide.Lucide import com.composables.icons.lucide.User import com.fabledsword.minstrel.models.ArtistSuggestionRef import com.fabledsword.minstrel.models.LidarrSearchResultRef +import com.fabledsword.minstrel.models.SuggestionSnoozeRef @Composable -internal fun SuggestionTile(s: ArtistSuggestionRef, onRequest: () -> Unit) { +internal fun SuggestionTile( + s: ArtistSuggestionRef, + snoozed: Boolean, + onRequest: () -> Unit, + onSnooze: () -> Unit, + onUnsnooze: () -> Unit, +) { Row( modifier = Modifier .fillMaxWidth() @@ -48,9 +58,13 @@ internal fun SuggestionTile(s: ArtistSuggestionRef, onRequest: () -> Unit) { maxLines = 1, overflow = TextOverflow.Ellipsis, ) - if (s.attributionText.isNotEmpty()) { + // Once parked, the "because you liked X" line is no longer the + // useful thing to say — confirming what just happened is. + val secondary = + if (snoozed) "Not right now — hidden for a while" else s.reasonText + if (secondary.isNotEmpty()) { Text( - text = s.attributionText, + text = secondary, style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant, maxLines = 2, @@ -58,7 +72,52 @@ internal fun SuggestionTile(s: ArtistSuggestionRef, onRequest: () -> Unit) { ) } } - Button(onClick = onRequest) { Text("Request") } + if (snoozed) { + TextButton(onClick = onUnsnooze) { Text("Undo") } + } else { + Button(onClick = onRequest) { Text("Request") } + IconButton(onClick = onSnooze) { + Icon( + imageVector = Lucide.Clock, + // Rule #101: the label states what happens, and passes no + // judgement on the music. Never "not for me". + contentDescription = "Not right now — hide ${s.name} for a while", + tint = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } + } + } +} + +/** + * One row of the parked list. This exists because a snoozed candidate is by + * definition absent from the deck above, so without it there is no route back + * to an un-snooze once the card has gone. + */ +@Composable +internal fun SnoozedTile(row: SuggestionSnoozeRef, onUnsnooze: () -> Unit) { + Row( + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = 16.dp, vertical = 8.dp), + verticalAlignment = Alignment.CenterVertically, + horizontalArrangement = Arrangement.spacedBy(12.dp), + ) { + Column(modifier = Modifier.weight(1f)) { + Text( + text = row.name, + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurface, + maxLines = 1, + overflow = TextOverflow.Ellipsis, + ) + Text( + text = "Back ${row.returnsIn()}", + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } + TextButton(onClick = onUnsnooze) { Text("Bring back") } } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverViewModel.kt index ab0c7997..f26011c3 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/discover/ui/DiscoverViewModel.kt @@ -10,6 +10,7 @@ import com.fabledsword.minstrel.discover.data.RequestOutcome import com.fabledsword.minstrel.models.ArtistSuggestionRef import com.fabledsword.minstrel.models.LidarrRequestKind import com.fabledsword.minstrel.models.LidarrSearchResultRef +import com.fabledsword.minstrel.models.SuggestionSnoozeRef import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow @@ -27,6 +28,17 @@ data class DiscoverState( val suggestions: SuggestionState = SuggestionState.Loading, val results: ResultsState = ResultsState.Idle, val locallyRequestedMbids: Set = emptySet(), + /** + * Parked candidates, for the manage list under the feed. Empty is the + * normal case and hides the section entirely. + */ + val snoozes: List = emptyList(), + /** + * Just-snoozed MBIDs. These keep their row visible showing an Undo rather + * than yanking it out from under the user's finger; the row is gone on the + * next load, and [snoozes] is the way back after that. + */ + val locallySnoozedMbids: Set = emptySet(), ) sealed interface SuggestionState { @@ -96,6 +108,47 @@ class DiscoverViewModel @Inject constructor( ) } } + // Refresh the parked list alongside the deck: a snooze made on another + // client should show up here, and one whose window lapsed should drop + // off. Sequenced after the deck load rather than raced with it so the + // two panes can't disagree about a candidate mid-refresh. + loadSnoozes() + } + + /** + * Loads the parked list. Failure is deliberately silent: this is a + * secondary pane, and an error banner for it would sit above the suggestion + * feed the user actually came for. The list stays as-is and the next + * refresh retries. + */ + private suspend fun loadSnoozes() { + try { + val rows = repository.listSnoozes() + internal.update { it.copy(snoozes = rows) } + } catch ( + @Suppress("TooGenericExceptionCaught", "SwallowedException") e: Throwable, + ) { + // Keep whatever we last showed rather than blanking the section. + } + } + + /** + * Parks a suggestion. Flips the row locally first so the tap registers + * immediately; the repository handles the offline case, so there is no + * failure branch to revert here — unlike the web client, where the fetch + * either lands or doesn't. + */ + suspend fun snoozeSuggestion(s: ArtistSuggestionRef) { + internal.update { it.copy(locallySnoozedMbids = it.locallySnoozedMbids + s.mbid) } + repository.snoozeSuggestion(s.mbid, s.name) + loadSnoozes() + } + + /** Brings a parked suggestion back, from either the card or the list. */ + suspend fun unsnoozeSuggestion(mbid: String, name: String) { + internal.update { it.copy(locallySnoozedMbids = it.locallySnoozedMbids - mbid) } + repository.unsnoozeSuggestion(mbid, name) + loadSnoozes() } fun runSearch() { diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt index b04e9fe8..bd05de60 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/Discover.kt @@ -1,5 +1,8 @@ package com.fabledsword.minstrel.models +import kotlinx.datetime.Instant +import kotlin.math.roundToInt + /** * Kind of Lidarr request being created. Wire form is the lowercase * enum name; the helper [wire] keeps that mapping in one place. @@ -49,6 +52,8 @@ data class ArtistSuggestionRef( val name: String, val imageUrl: String = "", val attribution: List = emptyList(), + /** Taste-profile tags this candidate matches, strongest first (#2377). */ + val matchedTags: List = emptyList(), ) { val attributionText: String get() { @@ -63,7 +68,92 @@ data class ArtistSuggestionRef( } } + /** + * The subtitle line for the card. + * + * Prefers the taste-tag reason over seed attribution when we have one, + * because it describes the MUSIC ("sounds like what you like") rather than + * the graph ("adjacent to something you played") — the whole point of + * milestone #268 slice 6. Falls back to attribution, which is the common + * case: tag coverage for out-of-library artists is partial by nature + * (#2376), so most candidates have no matched tags. + * + * Kept in lockstep with the web client's reasonText() in + * SuggestionFeed.svelte — same wording, same Oxford comma. + */ + val reasonText: String + get() { + val tags = matchedTags.take(MAX_ATTRIBUTION_PHRASES) + return when (tags.size) { + 0 -> attributionText + 1 -> "Matches your taste in ${tags[0]}." + 2 -> "Matches your taste in ${tags[0]} and ${tags[1]}." + else -> "Matches your taste in ${tags[0]}, ${tags[1]}, and ${tags[2]}." + } + } + companion object { private const val MAX_ATTRIBUTION_PHRASES = 3 } } + +/** + * A suggestion the user parked with "not right now" (#2374). + * + * Deliberately NOT a dislike: it carries no verdict on the artist, expires on + * its own, and never reaches the taste profile. Anything that treats this as + * negative preference signal is a bug. + * + * [snoozedUntil] is the raw RFC3339 string from the wire. Only the server + * decides whether a snooze is still in effect — every row the client receives + * already is — so this is read purely to phrase "back in about 3 months". + */ +data class SuggestionSnoozeRef( + val mbid: String, + val name: String, + val snoozedUntil: String, +) { + /** + * Relative return phrase for the manage list. Relative rather than a + * calendar date because the exact day a 90-day snooze lapses is noise the + * user never asked for. + * + * [nowMs] is injectable so this is testable without freezing the clock. + * Returns "shortly" for an unparseable or already-past timestamp: the row + * is on screen, so the server still considers it snoozed, and guessing is + * better than rendering an empty line. + */ + fun returnsIn(nowMs: Long = System.currentTimeMillis()): String { + val remainingMs = runCatching { Instant.parse(snoozedUntil).toEpochMilliseconds() } + .getOrNull()?.minus(nowMs) + // Two ways to have nothing to state: an unparseable timestamp, or one + // already lapsed by our clock though the server still returned the row + // (the two disagree). Neither is "today", which would read as a real + // prediction. + if (remainingMs == null || remainingMs <= 0) return "shortly" + val days = (remainingMs.toDouble() / MILLIS_PER_DAY).roundToInt() + return when { + days < 1 -> "today" + days == 1 -> "tomorrow" + days < DAYS_BEFORE_MONTHS -> "in $days days" + else -> { + val months = (days.toDouble() / DAYS_PER_MONTH).roundToInt() + if (months == 1) "in about a month" else "in about $months months" + } + } + } + + private companion object { + const val MILLIS_PER_DAY = 86_400_000.0 + + // Below this, days read more naturally than a rounded month count. + // + // Must be <= DAYS_PER_MONTH, or the singular "in about a month" is + // unreachable: a rounded month count of 1 needs 15..44 days, and any + // threshold above 30 sends all of those down the days branch instead. + // This was 45 and the singular branch was dead code — the unit test + // for it is what surfaced that. + const val DAYS_BEFORE_MONTHS = 30 + const val DAYS_PER_MONTH = 30.0 + } +} diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt index bf09d37d..25ae924e 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/DiscoverWire.kt @@ -35,6 +35,13 @@ data class ArtistSuggestionWire( val name: String = "", @SerialName("image_url") val imageUrl: String = "", val attribution: List = emptyList(), + /** + * Tags this candidate shares with the user's taste profile, strongest + * first (max 3, #2377). Absent for most candidates — tag coverage for + * out-of-library artists is partial by nature (#2376) — so the default + * empty list is the common case, not an error. + */ + @SerialName("matched_tags") val matchedTags: List = emptyList(), ) /** @@ -48,6 +55,36 @@ data class SeedContributionWire( @SerialName("is_liked") val isLiked: Boolean = false, ) +/** + * One row of `GET /api/discover/snoozes` — a suggestion the user parked + * with "not right now". The server only returns rows that are still in + * effect, so the client never compares [snoozedUntil] against the clock to + * decide whether to show it; it reads it only to say when the artist comes + * back. + */ +@Serializable +data class SuggestionSnoozeWire( + val mbid: String = "", + val name: String = "", + @SerialName("snoozed_until") val snoozedUntil: String = "", + @SerialName("created_at") val createdAt: String = "", +) + +/** + * Body for `POST /api/discover/suggestions/{mbid}/snooze`. + * + * [name] is required by the server, not decorative: suggestions are + * out-of-library, so there is no artists row to resolve a display name from + * and the snooze list would have nothing to render. Omitting it is a 400. + * + * No `days` field. The duration is the server's to own (90 days); pinning it + * client-side would freeze the default at whatever this build shipped. + */ +@Serializable +data class SnoozeSuggestionBody( + val name: String, +) + /** * Body posted to `POST /api/requests`. Mirrors the Flutter `createRequest` * payload shape. Optional fields are emitted only when non-null diff --git a/android/app/src/test/java/com/fabledsword/minstrel/cache/mutations/SupersededToggleIdsTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/cache/mutations/SupersededToggleIdsTest.kt new file mode 100644 index 00000000..b280ab80 --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/cache/mutations/SupersededToggleIdsTest.kt @@ -0,0 +1,132 @@ +package com.fabledsword.minstrel.cache.mutations + +import com.fabledsword.minstrel.cache.db.entities.CachedMutationEntity +import kotlinx.serialization.json.Json +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Collapse rules for desired-state toggles in the offline queue. + * + * The hazard this guards against is real and silent: without collapsing, a + * queued snooze that replays AFTER the user's undo re-hides an artist they + * asked to see again, and nothing surfaces the contradiction. + */ +class SupersededToggleIdsTest { + + private val json = Json { ignoreUnknownKeys = true } + + private fun snoozeRow(id: Long, mbid: String, desiredSnoozed: Boolean) = CachedMutationEntity( + id = id, + kind = MutationKind.SUGGESTION_SNOOZE_TOGGLE, + payload = json.encodeToString( + SuggestionSnoozeTogglePayload.serializer(), + SuggestionSnoozeTogglePayload(mbid, "Name", desiredSnoozed), + ), + ) + + private fun likeRow(id: Long, entityId: String, desired: Boolean) = CachedMutationEntity( + id = id, + kind = MutationKind.LIKE_TOGGLE, + payload = json.encodeToString( + LikeTogglePayload.serializer(), + LikeTogglePayload("artist", entityId, desired), + ), + ) + + @Test + fun `a snooze followed by its undo drops the snooze`() { + val rows = listOf( + snoozeRow(1, "mb-a", desiredSnoozed = true), + snoozeRow(2, "mb-a", desiredSnoozed = false), + ) + // Only the later intent (the undo) survives to be replayed. + assertEquals(setOf(1L), supersededToggleIds(rows, json)) + } + + @Test + fun `toggles for different candidates never collapse into each other`() { + val rows = listOf( + snoozeRow(1, "mb-a", desiredSnoozed = true), + snoozeRow(2, "mb-b", desiredSnoozed = true), + ) + assertTrue(supersededToggleIds(rows, json).isEmpty()) + } + + @Test + fun `only the newest of several toggles for one candidate survives`() { + val rows = listOf( + snoozeRow(1, "mb-a", desiredSnoozed = true), + snoozeRow(2, "mb-a", desiredSnoozed = false), + snoozeRow(3, "mb-a", desiredSnoozed = true), + ) + assertEquals(setOf(1L, 2L), supersededToggleIds(rows, json)) + } + + // The kind is part of the collapse key, so a snooze and a like that happen + // to share an id string must not shadow one another. + @Test + fun `a like and a snooze on the same id string do not collide`() { + val rows = listOf( + likeRow(1, "same-id", desired = true), + snoozeRow(2, "same-id", desiredSnoozed = true), + ) + assertTrue(supersededToggleIds(rows, json).isEmpty()) + } + + @Test + fun `like toggles still collapse — the pre-existing behaviour is intact`() { + val rows = listOf( + likeRow(1, "artist-1", desired = true), + likeRow(2, "artist-1", desired = false), + ) + assertEquals(setOf(1L), supersededToggleIds(rows, json)) + } + + @Test + fun `non-toggle kinds are never collapsed, even repeated for one entity`() { + // Two appends to the same playlist are two real actions, not one + // desired state — collapsing them would lose a write. + val rows = listOf( + CachedMutationEntity( + id = 1, + kind = MutationKind.PLAYLIST_APPEND, + payload = json.encodeToString( + PlaylistAppendPayload.serializer(), + PlaylistAppendPayload("pl-1", listOf("t1")), + ), + ), + CachedMutationEntity( + id = 2, + kind = MutationKind.PLAYLIST_APPEND, + payload = json.encodeToString( + PlaylistAppendPayload.serializer(), + PlaylistAppendPayload("pl-1", listOf("t2")), + ), + ), + ) + assertTrue(supersededToggleIds(rows, json).isEmpty()) + } + + // A row whose payload won't decode gets no key at all, rather than sharing + // a "corrupt" bucket — otherwise one bad row could suppress a good one + // behind it. The dispatcher DROPs the bad row on its own. + @Test + fun `an undecodable payload does not suppress a valid later row`() { + val rows = listOf( + CachedMutationEntity( + id = 1, + kind = MutationKind.SUGGESTION_SNOOZE_TOGGLE, + payload = "{ not json", + ), + snoozeRow(2, "mb-a", desiredSnoozed = true), + ) + assertTrue(supersededToggleIds(rows, json).isEmpty()) + } + + @Test + fun `an empty queue collapses nothing`() { + assertTrue(supersededToggleIds(emptyList(), json).isEmpty()) + } +} diff --git a/android/app/src/test/java/com/fabledsword/minstrel/models/ArtistSuggestionReasonTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/models/ArtistSuggestionReasonTest.kt new file mode 100644 index 00000000..0000c507 --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/models/ArtistSuggestionReasonTest.kt @@ -0,0 +1,85 @@ +package com.fabledsword.minstrel.models + +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals + +/** + * The card's subtitle line (#2377). Wording is kept in lockstep with the web + * client's reasonText() in SuggestionFeed.svelte — these assertions are the + * record of what that wording IS, so a change on one client without the other + * shows up as a failure rather than as silent divergence between the two + * surfaces. + */ +class ArtistSuggestionReasonTest { + + private val seeds = listOf( + SeedContributionRef(name = "Seed", isLiked = true), + ) + + private fun suggestion( + matched: List = emptyList(), + attribution: List = seeds, + ) = ArtistSuggestionRef( + mbid = "mb", + name = "Candidate", + attribution = attribution, + matchedTags = matched, + ) + + @Test + fun `one matched tag reads in the singular`() { + assertEquals( + "Matches your taste in shoegaze.", + suggestion(matched = listOf("shoegaze")).reasonText, + ) + } + + @Test + fun `two matched tags join with and`() { + assertEquals( + "Matches your taste in shoegaze and dream pop.", + suggestion(matched = listOf("shoegaze", "dream pop")).reasonText, + ) + } + + @Test + fun `three matched tags use an Oxford comma`() { + assertEquals( + "Matches your taste in a, b, and c.", + suggestion(matched = listOf("a", "b", "c")).reasonText, + ) + } + + // The server caps at 3, but the client must not render a run-on line if a + // future server sends more. + @Test + fun `more than three matched tags are capped at three`() { + assertEquals( + "Matches your taste in a, b, and c.", + suggestion(matched = listOf("a", "b", "c", "d", "e")).reasonText, + ) + } + + // The COMMON case: most candidates have no cached tags (#2376), so the card + // must fall back to seed attribution rather than going blank. + @Test + fun `no matched tags falls back to seed attribution`() { + assertEquals("Because you liked Seed.", suggestion().reasonText) + } + + // Nothing to say at all — a candidate with neither tags nor attribution + // yields an empty line, which the tile suppresses rather than rendering as + // a blank row. + @Test + fun `neither tags nor attribution yields an empty line`() { + assertEquals("", suggestion(attribution = emptyList()).reasonText) + } + + // The taste reason WINS over attribution when both exist: describing the + // music beats describing the similarity graph. + @Test + fun `a taste match supersedes seed attribution`() { + val got = suggestion(matched = listOf("shoegaze")).reasonText + assertEquals("Matches your taste in shoegaze.", got) + } +} diff --git a/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt new file mode 100644 index 00000000..2dc598ec --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/models/SuggestionSnoozeRefTest.kt @@ -0,0 +1,83 @@ +package com.fabledsword.minstrel.models + +import kotlinx.datetime.Instant +import org.junit.jupiter.api.Test +import kotlin.test.assertEquals + +/** + * `returnsIn` phrasing for the parked-suggestions list (#2375). The clock is + * injected rather than frozen, so these assertions are stable. + */ +class SuggestionSnoozeRefTest { + + private val now = 1_800_000_000_000L // fixed epoch ms; any value works + + private fun snoozeIn(days: Double) = SuggestionSnoozeRef( + mbid = "mb", + name = "Parked", + snoozedUntil = Instant + .fromEpochMilliseconds(now + (days * 86_400_000L).toLong()) + .toString(), + ) + + @Test + fun `the default 90-day snooze reads as about 3 months`() { + assertEquals("in about 3 months", snoozeIn(90.0).returnsIn(now)) + } + + @Test + fun `a month reads in the singular`() { + assertEquals("in about a month", snoozeIn(30.0).returnsIn(now)) + } + + @Test + fun `under the month threshold it counts days`() { + assertEquals("in 14 days", snoozeIn(14.0).returnsIn(now)) + } + + // Pins the days→months boundary. The singular branch was originally dead + // code because the threshold (45) sat above the divisor (30), so no day + // count could ever round to one month without being caught by the days + // branch first. Asserting both sides of the seam keeps that from + // regressing silently. + @Test + fun `the days-to-months boundary is exactly at 30 days`() { + assertEquals("in 29 days", snoozeIn(29.0).returnsIn(now)) + assertEquals("in about a month", snoozeIn(30.0).returnsIn(now)) + } + + @Test + fun `well past a month still reads in the singular rather than jumping to two`() { + assertEquals("in about a month", snoozeIn(40.0).returnsIn(now)) + } + + @Test + fun `tomorrow is named, not rendered as 1 days`() { + assertEquals("tomorrow", snoozeIn(1.0).returnsIn(now)) + } + + @Test + fun `later today rounds down to today rather than going negative`() { + assertEquals("today", snoozeIn(0.1).returnsIn(now)) + } + + // The server only ever returns unexpired rows, so a past timestamp means + // our clock and the server's disagree. The row is on screen either way, so + // say something plausible rather than leaving the line blank. + @Test + fun `an already-past expiry degrades to shortly`() { + assertEquals("shortly", snoozeIn(-5.0).returnsIn(now)) + } + + @Test + fun `an unparseable timestamp degrades to shortly`() { + val row = SuggestionSnoozeRef(mbid = "mb", name = "Parked", snoozedUntil = "not-a-date") + assertEquals("shortly", row.returnsIn(now)) + } + + @Test + fun `an empty timestamp degrades to shortly`() { + val row = SuggestionSnoozeRef(mbid = "mb", name = "Parked", snoozedUntil = "") + assertEquals("shortly", row.returnsIn(now)) + } +} diff --git a/internal/api/admin_recommendation_tuning.go b/internal/api/admin_recommendation_tuning.go index a97276c5..918a3b55 100644 --- a/internal/api/admin_recommendation_tuning.go +++ b/internal/api/admin_recommendation_tuning.go @@ -67,15 +67,30 @@ func tasteRespFrom(t recsettings.TasteTuning) tasteTuningResp { } } +// discoverTuningResp is the Discover scope on the wire (#2377). +type discoverTuningResp struct { + TagOverlapWeight float64 `json:"tag_overlap_weight"` + SnoozeDays float64 `json:"snooze_days"` +} + +func discoverRespFrom(d recsettings.DiscoverTuning) discoverTuningResp { + return discoverTuningResp{ + TagOverlapWeight: d.TagOverlapWeight, + SnoozeDays: d.SnoozeDays, + } +} + // tuningSnapshot is both the GET response and the post-mutation echo: // current values alongside shipped defaults so the card can mark // which knobs deviate. type tuningSnapshot struct { Profiles map[string]weightsResp `json:"profiles"` Taste tasteTuningResp `json:"taste"` + Discover discoverTuningResp `json:"discover"` Shipped struct { Profiles map[string]weightsResp `json:"profiles"` Taste tasteTuningResp `json:"taste"` + Discover discoverTuningResp `json:"discover"` } `json:"shipped"` } @@ -86,11 +101,13 @@ func (h *handlers) tuningSnapshot() tuningSnapshot { recsettings.ScopeDailyMix: weightsRespFrom(h.recSettings.Weights(recsettings.ScopeDailyMix)), } out.Taste = tasteRespFrom(h.recSettings.Taste()) + out.Discover = discoverRespFrom(h.recSettings.Discover()) out.Shipped.Profiles = map[string]weightsResp{ recsettings.ScopeRadio: weightsRespFrom(recsettings.ShippedRadioWeights()), recsettings.ScopeDailyMix: weightsRespFrom(recsettings.ShippedDailyMixWeights()), } out.Shipped.Taste = tasteRespFrom(recsettings.ShippedTasteTuning()) + out.Shipped.Discover = discoverRespFrom(recsettings.ShippedDiscoverTuning()) return out } @@ -120,9 +137,12 @@ func (h *handlers) handlePatchRecommendationTuning(w http.ResponseWriter, r *htt } var err error - if scope == recsettings.ScopeTaste { + switch scope { + case recsettings.ScopeTaste: err = h.recSettings.UpdateTaste(r.Context(), body.Values) - } else { + case recsettings.ScopeDiscover: + err = h.recSettings.UpdateDiscover(r.Context(), body.Values) + default: err = h.recSettings.UpdateProfile(r.Context(), scope, body.Values) } if err != nil { diff --git a/internal/api/api.go b/internal/api/api.go index a16f8f23..7e04e4c8 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -104,6 +104,11 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Get("/search", h.handleSearch) authed.Get("/radio", h.handleRadio) authed.Get("/discover/suggestions", h.handleListSuggestions) + // Snooze = "not right now", time-boxed and self-expiring + // (#2374). Not a dislike — see the migration for why. + authed.Post("/discover/suggestions/{mbid}/snooze", h.handleSnoozeSuggestion) + authed.Delete("/discover/suggestions/{mbid}/snooze", h.handleUnsnoozeSuggestion) + authed.Get("/discover/snoozes", h.handleListSuggestionSnoozes) authed.Get("/home", h.handleGetHome) authed.Get("/home/index", h.handleGetHomeIndex) authed.Post("/events", h.handleEvents) diff --git a/internal/api/suggestions.go b/internal/api/suggestions.go index 1ad5bf7f..977d571f 100644 --- a/internal/api/suggestions.go +++ b/internal/api/suggestions.go @@ -2,13 +2,19 @@ package api import ( "context" + "encoding/json" + "errors" + "io" "net/http" "strconv" + "strings" "sync" + "github.com/go-chi/chi/v5" "github.com/jackc/pgx/v5/pgtype" "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/lidarr" "git.fabledsword.com/bvandeusen/minstrel/internal/recommendation" ) @@ -19,6 +25,12 @@ type suggestionView struct { Name string `json:"name"` Score float64 `json:"score"` Attribution []seedContributionView `json:"attribution"` + // MatchedTags are the candidate's tags that overlap the user's taste + // profile, strongest first (#2377) — the "matches: shoegaze, melancholic" + // line. Omitted when empty, which is common: tag coverage for + // out-of-library artists is permanently partial (#2376), and the card + // falls back to the seed attribution it has always shown. + MatchedTags []string `json:"matched_tags,omitempty"` // ImageURL is resolved on-demand from Lidarr (out-of-library // artists have no local art row). Omitted when Lidarr is disabled // or has no match — the client falls back to a placeholder. Not @@ -65,7 +77,11 @@ func (h *handlers) handleListSuggestions(w http.ResponseWriter, r *http.Request) halfLife = f } - suggestions, err := recommendation.SuggestArtists(r.Context(), h.pool, user.ID, halfLife, limit) + // Read the tuned weight per request so an admin change takes effect on the + // next refresh, no restart (rule #25). + tagWeight := h.recSettings.Discover().TagOverlapWeight + suggestions, err := recommendation.SuggestArtists( + r.Context(), h.pool, user.ID, halfLife, limit, tagWeight) if err != nil { h.logger.Error("api: list suggestions", "err", err) writeErr(w, apierror.InternalMsg("failed to load suggestions", err)) @@ -86,12 +102,154 @@ func (h *handlers) handleListSuggestions(w http.ResponseWriter, r *http.Request) } out = append(out, suggestionView{ MBID: s.MBID, Name: s.Name, Score: s.Score, Attribution: attr, + MatchedTags: s.MatchedTags, }) } h.resolveSuggestionArt(r.Context(), out) writeJSON(w, http.StatusOK, out) } +// maxSnoozeDays caps a client-supplied duration. The DEFAULT is not here: it's +// a DB-backed knob on the admin tuning card (rule #25), read per request via +// recSettings.Discover().SnoozeDays. See #2377. +const maxSnoozeDays = 365.0 + +// snoozeRequest is the POST body. Both fields are optional in the JSON sense +// (an absent body snoozes for the default), but Name is required in practice: +// candidates are out-of-library, so the server has no artists row to resolve a +// display name from and the un-snooze list would have nothing to show. The +// client always has it — it just rendered the card. +type snoozeRequest struct { + Name string `json:"name"` + Days float64 `json:"days"` +} + +// snoozeView is one row of GET /api/discover/snoozes. +type snoozeView struct { + MBID string `json:"mbid"` + Name string `json:"name"` + SnoozedUntil pgtype.Timestamptz `json:"snoozed_until"` + CreatedAt pgtype.Timestamptz `json:"created_at"` +} + +// handleSnoozeSuggestion implements +// POST /api/discover/suggestions/{mbid}/snooze. +// +// Parks a candidate for `days` (default 90, capped at 365). Idempotent: +// snoozing an already-snoozed candidate extends it rather than conflicting. +// +// This is NOT negative feedback. It records no verdict on the artist and is +// never read by internal/taste — see 0049_suggestion_snoozes.up.sql for the +// rule #101 reasoning. Returns 204. +func (h *handlers) handleSnoozeSuggestion(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + mbid := strings.TrimSpace(chi.URLParam(r, "mbid")) + if mbid == "" { + writeErr(w, apierror.BadRequest("invalid_id", "missing mbid")) + return + } + + // An empty body is a valid "snooze this for the default period", so EOF + // is not an error here — decodeBody would reject it as a malformed body. + var body snoozeRequest + if err := json.NewDecoder(r.Body).Decode(&body); err != nil && !errors.Is(err, io.EOF) { + writeErr(w, apierror.BadRequest("invalid_body", "")) + return + } + name := strings.TrimSpace(body.Name) + if name == "" { + writeErr(w, apierror.BadRequest("invalid_body", "name is required")) + return + } + days := body.Days + if days <= 0 { + days = h.recSettings.Discover().SnoozeDays + } + if days > maxSnoozeDays { + // Clamp rather than reject: a client asking for longer than we allow + // still means "park this", and failing the write would leave the card + // sitting there as if the tap did nothing. + days = maxSnoozeDays + } + + q := dbq.New(h.pool) + if err := q.SnoozeSuggestion(r.Context(), dbq.SnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: mbid, + CandidateName: name, + Column4: days, + }); err != nil { + h.logger.Error("api: snooze suggestion", "err", err) + writeErr(w, apierror.InternalMsg("failed to snooze suggestion", err)) + return + } + w.WriteHeader(http.StatusNoContent) +} + +// handleUnsnoozeSuggestion implements +// DELETE /api/discover/suggestions/{mbid}/snooze. +// +// Brings a parked candidate back immediately. 404s an MBID this user never +// snoozed, so the client can tell "undone" from "there was nothing there". +func (h *handlers) handleUnsnoozeSuggestion(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + mbid := strings.TrimSpace(chi.URLParam(r, "mbid")) + if mbid == "" { + writeErr(w, apierror.BadRequest("invalid_id", "missing mbid")) + return + } + q := dbq.New(h.pool) + rows, err := q.UnsnoozeSuggestion(r.Context(), dbq.UnsnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: mbid, + }) + if err != nil { + h.logger.Error("api: unsnooze suggestion", "err", err) + writeErr(w, apierror.InternalMsg("failed to unsnooze suggestion", err)) + return + } + if rows == 0 { + writeErr(w, apierror.NotFound("snooze")) + return + } + w.WriteHeader(http.StatusNoContent) +} + +// handleListSuggestionSnoozes implements GET /api/discover/snoozes. +// +// The un-snooze surface needs this: a parked candidate is by definition +// absent from the suggestion deck, so without a list there is no way to +// reach the DELETE above. Scoped to the caller (rule #47). Expired rows are +// already filtered by the query — the hourly gc sweep only reclaims space. +func (h *handlers) handleListSuggestionSnoozes(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + rows, err := dbq.New(h.pool).ListActiveSuggestionSnoozes(r.Context(), user.ID) + if err != nil { + h.logger.Error("api: list suggestion snoozes", "err", err) + writeErr(w, apierror.InternalMsg("failed to load snoozes", err)) + return + } + out := make([]snoozeView, 0, len(rows)) + for _, row := range rows { + out = append(out, snoozeView{ + MBID: row.CandidateMbid, + Name: row.CandidateName, + SnoozedUntil: row.SnoozedUntil, + CreatedAt: row.CreatedAt, + }) + } + writeJSON(w, http.StatusOK, out) +} + // resolveSuggestionArt fills ImageURL on-demand from Lidarr's artist // lookup, matched by MBID (foreignArtistId). Best-effort and cache-free: // Lidarr is the only source — when it's disabled, unreachable, or has diff --git a/internal/db/dbq/candidate_artist_tags.sql.go b/internal/db/dbq/candidate_artist_tags.sql.go new file mode 100644 index 00000000..a733e4a2 --- /dev/null +++ b/internal/db/dbq/candidate_artist_tags.sql.go @@ -0,0 +1,228 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: candidate_artist_tags.sql + +package dbq + +import ( + "context" +) + +const countCandidateArtistTagCoverage = `-- name: CountCandidateArtistTagCoverage :one +SELECT count(*)::bigint AS processed, + count(*) FILTER (WHERE tag_source <> 'none')::bigint AS with_tags + FROM candidate_artist_tag_state +` + +type CountCandidateArtistTagCoverageRow struct { + Processed int64 + WithTags int64 +} + +// Operator-facing coverage: how many distinct candidates have been processed, +// and how many of those actually yielded tags. The gap is the honest ceiling +// from the task — obscure artists with no MBID presence or no upstream tags +// stay thin no matter how long the worker runs, and that is worth being able +// to see rather than inferring from a silent surface. +func (q *Queries) CountCandidateArtistTagCoverage(ctx context.Context) (CountCandidateArtistTagCoverageRow, error) { + row := q.db.QueryRow(ctx, countCandidateArtistTagCoverage) + var i CountCandidateArtistTagCoverageRow + err := row.Scan(&i.Processed, &i.WithTags) + return i, err +} + +const deleteCandidateArtistTags = `-- name: DeleteCandidateArtistTags :exec +DELETE FROM candidate_artist_tags WHERE candidate_mbid = $1 +` + +// Clear a candidate's cached tags before rewriting (atomic replace by the +// caller, same shape as DeleteTrackTags). +func (q *Queries) DeleteCandidateArtistTags(ctx context.Context, candidateMbid string) error { + _, err := q.db.Exec(ctx, deleteCandidateArtistTags, candidateMbid) + return err +} + +const gcDeleteOrphanedCandidateArtistTagState = `-- name: GcDeleteOrphanedCandidateArtistTagState :execrows +DELETE FROM candidate_artist_tag_state s + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = s.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = s.candidate_mbid) +` + +// Same sweep for the bookkeeping rows. Kept as a separate statement rather +// than a cascade: the two tables are independent by design (a 'none' outcome +// has state but no tags), so neither can be the parent of the other. +func (q *Queries) GcDeleteOrphanedCandidateArtistTagState(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteOrphanedCandidateArtistTagState) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const gcDeleteOrphanedCandidateArtistTags = `-- name: GcDeleteOrphanedCandidateArtistTags :execrows +DELETE FROM candidate_artist_tags t + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = t.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = t.candidate_mbid) +` + +// Drops cached tags for candidates that no longer appear in the similarity +// feed, or that have since been added to the library (their tags now live in +// track_tags). The feed is refetched periodically and churns, so without this +// the cache only ever grows. +func (q *Queries) GcDeleteOrphanedCandidateArtistTags(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteOrphanedCandidateArtistTags) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const insertCandidateArtistTag = `-- name: InsertCandidateArtistTag :exec +INSERT INTO candidate_artist_tags (candidate_mbid, tag, weight) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid, tag) + DO UPDATE SET weight = GREATEST(candidate_artist_tags.weight, EXCLUDED.weight) +` + +type InsertCandidateArtistTagParams struct { + CandidateMbid string + Tag string + Weight float64 +} + +// Upsert one (candidate, tag); keep the stronger weight when two providers +// agree on a tag with different folksonomy strengths. +func (q *Queries) InsertCandidateArtistTag(ctx context.Context, arg InsertCandidateArtistTagParams) error { + _, err := q.db.Exec(ctx, insertCandidateArtistTag, arg.CandidateMbid, arg.Tag, arg.Weight) + return err +} + +const listCandidateArtistTagsForMbids = `-- name: ListCandidateArtistTagsForMbids :many +SELECT candidate_mbid, tag, weight + FROM candidate_artist_tags + WHERE candidate_mbid = ANY($1::text[]) +` + +// Cached tags for a set of candidates, for slice 6's taste-overlap ranking. +// One row per (candidate, tag). +func (q *Queries) ListCandidateArtistTagsForMbids(ctx context.Context, dollar_1 []string) ([]CandidateArtistTag, error) { + rows, err := q.db.Query(ctx, listCandidateArtistTagsForMbids, dollar_1) + if err != nil { + return nil, err + } + defer rows.Close() + var items []CandidateArtistTag + for rows.Next() { + var i CandidateArtistTag + if err := rows.Scan(&i.CandidateMbid, &i.Tag, &i.Weight); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listCandidateArtistsMissingTags = `-- name: ListCandidateArtistsMissingTags :many + +SELECT u.candidate_mbid, + coalesce(max(u.candidate_name), '')::text AS candidate_name, + sum(u.score)::float8 AS total_score + FROM artist_similarity_unmatched u + LEFT JOIN candidate_artist_tag_state s ON s.candidate_mbid = u.candidate_mbid + WHERE NOT EXISTS (SELECT 1 FROM artists a WHERE a.mbid = u.candidate_mbid) + AND ( + s.candidate_mbid IS NULL + OR (s.tag_source = 'none' AND s.tag_sources_version < $1) + ) + GROUP BY u.candidate_mbid + ORDER BY total_score DESC, u.candidate_mbid + LIMIT $2 +` + +type ListCandidateArtistsMissingTagsParams struct { + TagSourcesVersion int32 + Limit int32 +} + +type ListCandidateArtistsMissingTagsRow struct { + CandidateMbid string + CandidateName string + TotalScore float64 +} + +// Folksonomy tags for out-of-library Discover candidates (#2376). Parallel to +// track_tags.sql, but keyed by MBID because the artist has no local row. See +// 0050_candidate_artist_tags.up.sql for why the bookkeeping is its own table. +// Candidates eligible for tag enrichment: never processed (no state row) or +// settled 'none' under an older provider version. +// +// artist_similarity_unmatched holds one row per (seed, candidate, source), so +// this GROUPs to one row per candidate — enriching the same MBID once per seed +// that pointed at it would multiply the API calls for no gain. +// +// ORDER BY summed similarity DESC is the load-bearing part. The candidate pool +// is O(library artists x neighbours per artist) — thousands — and MusicBrainz +// allows ~1 req/s, so it can NEVER be fully enriched in one pass. Draining in +// strength order means the candidates most likely to actually reach a user's +// deck get tags first, and the long tail fills in over subsequent ticks +// instead of starving behind it. +// +// Already-in-library candidates are skipped: they have an artists row, so +// their tags belong in track_tags, and the suggestion query filters them out +// anyway. $1 = current tag_sources_version, $2 = limit. +// candidate_name is coalesced to the empty string so it lands non-nullable in +// Go: the name is only a Last.fm lookup key, and empty simply means "MBID-keyed +// providers only", which the provider chain already handles. max() is an +// arbitrary-but-deterministic pick when several seeds spell one MBID +// differently. +func (q *Queries) ListCandidateArtistsMissingTags(ctx context.Context, arg ListCandidateArtistsMissingTagsParams) ([]ListCandidateArtistsMissingTagsRow, error) { + rows, err := q.db.Query(ctx, listCandidateArtistsMissingTags, arg.TagSourcesVersion, arg.Limit) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListCandidateArtistsMissingTagsRow + for rows.Next() { + var i ListCandidateArtistsMissingTagsRow + if err := rows.Scan(&i.CandidateMbid, &i.CandidateName, &i.TotalScore); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const setCandidateArtistTagState = `-- name: SetCandidateArtistTagState :exec +INSERT INTO candidate_artist_tag_state (candidate_mbid, tag_source, tag_sources_version) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid) DO UPDATE + SET tag_source = EXCLUDED.tag_source, + tag_sources_version = EXCLUDED.tag_sources_version, + updated_at = now() +` + +type SetCandidateArtistTagStateParams struct { + CandidateMbid string + TagSource string + TagSourcesVersion int32 +} + +// Stamp the enrichment outcome so the drainer skips settled candidates. +// $2 = 'musicbrainz' | 'lastfm' | 'mixed' | 'none', $3 = current version. +func (q *Queries) SetCandidateArtistTagState(ctx context.Context, arg SetCandidateArtistTagStateParams) error { + _, err := q.db.Exec(ctx, setCandidateArtistTagState, arg.CandidateMbid, arg.TagSource, arg.TagSourcesVersion) + return err +} diff --git a/internal/db/dbq/models.go b/internal/db/dbq/models.go index 10e990ef..bfbb4ef8 100644 --- a/internal/db/dbq/models.go +++ b/internal/db/dbq/models.go @@ -240,6 +240,19 @@ type AuditLog struct { CreatedAt pgtype.Timestamptz } +type CandidateArtistTag struct { + CandidateMbid string + Tag string + Weight float64 +} + +type CandidateArtistTagState struct { + CandidateMbid string + TagSource string + TagSourcesVersion int32 + UpdatedAt pgtype.Timestamptz +} + type ContextualLike struct { ID pgtype.UUID UserID pgtype.UUID @@ -277,6 +290,13 @@ type DiagnosticEvent struct { ReceivedAt pgtype.Timestamptz } +type DiscoverTuning struct { + Singleton bool + TagOverlapWeight float64 + SnoozeDays float64 + UpdatedAt pgtype.Timestamptz +} + type GeneralLike struct { UserID pgtype.UUID TrackID pgtype.UUID @@ -518,6 +538,14 @@ type SmtpConfig struct { UpdatedAt pgtype.Timestamptz } +type SuggestionSnooze struct { + UserID pgtype.UUID + CandidateMbid string + CandidateName string + SnoozedUntil pgtype.Timestamptz + CreatedAt pgtype.Timestamptz +} + type SystemPlaylistRotationState struct { UserID pgtype.UUID PlaylistKind string diff --git a/internal/db/dbq/recommendation.sql.go b/internal/db/dbq/recommendation.sql.go index 13167583..d6ed888c 100644 --- a/internal/db/dbq/recommendation.sql.go +++ b/internal/db/dbq/recommendation.sql.go @@ -1022,20 +1022,50 @@ func (q *Queries) LoadRadioCandidatesV2(ctx context.Context, arg LoadRadioCandid } const suggestArtistsForUser = `-- name: SuggestArtistsForUser :many -WITH seeds AS ( +WITH artist_plays AS ( + -- Completed plays only. The previous seed query counted every play_event, + -- so skipping an artist repeatedly INCREASED its signal and pushed more of + -- its neighbours at the user (issue #2367 mechanism 3). + SELECT t.artist_id, count(*)::bigint AS play_count + FROM play_events pe + JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 AND pe.was_skipped = false + GROUP BY t.artist_id +), +profile_seeds AS ( + SELECT tpa.artist_id, tpa.weight AS raw_signal + FROM taste_profile_artists tpa + WHERE tpa.user_id = $1 AND tpa.weight > 0 +), +fallback_seeds AS ( SELECT a.id AS artist_id, 5.0 * (CASE WHEN gla.artist_id IS NOT NULL THEN 1 ELSE 0 END) + COALESCE(SUM(EXP(- EXTRACT(epoch FROM now() - pe.started_at) / ($2::float8 * 86400.0))), 0) - AS signal, - (gla.artist_id IS NOT NULL) AS is_liked, - COUNT(pe.id)::bigint AS play_count + AS raw_signal FROM artists a LEFT JOIN general_likes_artists gla ON gla.artist_id = a.id AND gla.user_id = $1 LEFT JOIN tracks t ON t.artist_id = a.id - LEFT JOIN play_events pe ON pe.track_id = t.id AND pe.user_id = $1 - WHERE gla.artist_id IS NOT NULL OR pe.id IS NOT NULL + LEFT JOIN play_events pe + ON pe.track_id = t.id AND pe.user_id = $1 AND pe.was_skipped = false + WHERE (gla.artist_id IS NOT NULL OR pe.id IS NOT NULL) + AND NOT EXISTS (SELECT 1 FROM profile_seeds) GROUP BY a.id, gla.artist_id ), +seeds AS ( + SELECT s.artist_id, + ln(1.0 + s.raw_signal) AS signal, + (gla.artist_id IS NOT NULL) AS is_liked, + COALESCE(ap.play_count, 0)::bigint AS play_count + FROM ( + SELECT artist_id, raw_signal FROM profile_seeds + UNION ALL + SELECT artist_id, raw_signal FROM fallback_seeds + ) s + LEFT JOIN general_likes_artists gla + ON gla.artist_id = s.artist_id AND gla.user_id = $1 + LEFT JOIN artist_plays ap ON ap.artist_id = s.artist_id + WHERE s.raw_signal > 0 +), contributions AS ( SELECT u.candidate_mbid, u.candidate_name, @@ -1052,6 +1082,18 @@ contributions AS ( AND r.lidarr_artist_mbid = u.candidate_mbid AND r.status NOT IN ('rejected', 'failed') ) + -- Snoozed by this user and not yet expired (#2374). Time-boxed and + -- per-user: the candidate returns on its own once snoozed_until + -- passes, and stays visible to everyone else meanwhile. Deliberately + -- filtered at the candidate stage, NOT folded into the score — a + -- snooze carries no opinion about the music, so it must not become a + -- ranking signal. + AND NOT EXISTS ( + SELECT 1 FROM suggestion_snoozes s + WHERE s.user_id = $1 + AND s.candidate_mbid = u.candidate_mbid + AND s.snoozed_until > now() + ) ) SELECT candidate_mbid, candidate_name, @@ -1082,13 +1124,24 @@ type SuggestArtistsForUserRow struct { TopPlayCounts []int64 } -// M5c: per-user artist suggestions ranked by signal x similarity. The -// seeds CTE collects the user's likes (x5) plus recency-decayed plays -// (exp(-age_days / $2)). The contributions CTE joins those seeds against -// artist_similarity_unmatched and filters out candidates already in -// library or already in a non-terminal lidarr_request. The outer SELECT -// aggregates per candidate, returning the top-3 contributing seeds for -// attribution. $1=user_id, $2=half_life_days, $3=limit. +// Per-user artist suggestions ranked by taste signal x similarity, projected +// through artist_similarity_unmatched (out-of-library candidates only). +// +// Seeds are TIERED (rule #131) so the surface never empties: +// +// tier 1 - taste_profile_artists.weight: engagement-graded, time-decayed and +// SIGNED by internal/taste, so an artist the user has drifted away +// from stops contributing instead of accumulating forever. +// tier 2 - likes + completed plays, used ONLY when the profile has no rows +// (new account, or before the first daily recompute). +// +// The signal is log-damped: contribution is signal x similarity, and the old +// undamped sum let one heavily-played artist's neighbours take every slot -- +// entrenching harder the MORE the user listened (issue #2367 mechanism 2). +// +// Candidates already in the library, already requested and not terminal, or +// snoozed by this user, are excluded. $1=user_id, $2=half_life_days (tier 2 +// decay), $3=limit. func (q *Queries) SuggestArtistsForUser(ctx context.Context, arg SuggestArtistsForUserParams) ([]SuggestArtistsForUserRow, error) { rows, err := q.db.Query(ctx, suggestArtistsForUser, arg.UserID, arg.Column2, arg.Limit) if err != nil { diff --git a/internal/db/dbq/recommendation_tuning.sql.go b/internal/db/dbq/recommendation_tuning.sql.go index f90aaa97..251eeed5 100644 --- a/internal/db/dbq/recommendation_tuning.sql.go +++ b/internal/db/dbq/recommendation_tuning.sql.go @@ -9,6 +9,22 @@ import ( "context" ) +const getDiscoverTuning = `-- name: GetDiscoverTuning :one +SELECT singleton, tag_overlap_weight, snooze_days, updated_at FROM discover_tuning WHERE singleton = true +` + +func (q *Queries) GetDiscoverTuning(ctx context.Context) (DiscoverTuning, error) { + row := q.db.QueryRow(ctx, getDiscoverTuning) + var i DiscoverTuning + err := row.Scan( + &i.Singleton, + &i.TagOverlapWeight, + &i.SnoozeDays, + &i.UpdatedAt, + ) + return i, err +} + const getTasteTuning = `-- name: GetTasteTuning :one SELECT singleton, half_life_days, engagement_hard_skip, engagement_neutral, engagement_full, updated_at, enriched_tag_scale, era_scale, mood_scale FROM taste_tuning WHERE singleton = true ` @@ -118,6 +134,32 @@ func (q *Queries) ListWeightProfiles(ctx context.Context) ([]RecommendationWeigh return items, nil } +const updateDiscoverTuning = `-- name: UpdateDiscoverTuning :one +UPDATE discover_tuning + SET tag_overlap_weight = $1, + snooze_days = $2, + updated_at = now() + WHERE singleton = true +RETURNING singleton, tag_overlap_weight, snooze_days, updated_at +` + +type UpdateDiscoverTuningParams struct { + TagOverlapWeight float64 + SnoozeDays float64 +} + +func (q *Queries) UpdateDiscoverTuning(ctx context.Context, arg UpdateDiscoverTuningParams) (DiscoverTuning, error) { + row := q.db.QueryRow(ctx, updateDiscoverTuning, arg.TagOverlapWeight, arg.SnoozeDays) + var i DiscoverTuning + err := row.Scan( + &i.Singleton, + &i.TagOverlapWeight, + &i.SnoozeDays, + &i.UpdatedAt, + ) + return i, err +} + const updateTasteTuning = `-- name: UpdateTasteTuning :one UPDATE taste_tuning SET half_life_days = $1, @@ -226,6 +268,24 @@ func (q *Queries) UpdateWeightProfile(ctx context.Context, arg UpdateWeightProfi return i, err } +const upsertDiscoverTuningDefaults = `-- name: UpsertDiscoverTuningDefaults :exec +INSERT INTO discover_tuning (singleton, tag_overlap_weight, snooze_days) +VALUES (true, $1, $2) +ON CONFLICT (singleton) DO NOTHING +` + +type UpsertDiscoverTuningDefaultsParams struct { + TagOverlapWeight float64 + SnoozeDays float64 +} + +// Boot reconcile for the Discover scope (#2377). Never overwrites +// operator-tuned values, same contract as the other two. +func (q *Queries) UpsertDiscoverTuningDefaults(ctx context.Context, arg UpsertDiscoverTuningDefaultsParams) error { + _, err := q.db.Exec(ctx, upsertDiscoverTuningDefaults, arg.TagOverlapWeight, arg.SnoozeDays) + return err +} + const upsertTasteTuningDefaults = `-- name: UpsertTasteTuningDefaults :exec INSERT INTO taste_tuning ( singleton, half_life_days, engagement_hard_skip, diff --git a/internal/db/dbq/suggestion_snoozes.sql.go b/internal/db/dbq/suggestion_snoozes.sql.go new file mode 100644 index 00000000..f1642a34 --- /dev/null +++ b/internal/db/dbq/suggestion_snoozes.sql.go @@ -0,0 +1,126 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: suggestion_snoozes.sql + +package dbq + +import ( + "context" + + "github.com/jackc/pgx/v5/pgtype" +) + +const gcDeleteExpiredSuggestionSnoozes = `-- name: GcDeleteExpiredSuggestionSnoozes :execrows +DELETE FROM suggestion_snoozes WHERE snoozed_until < now() +` + +// Keeps the table from growing without bound. Every read already filters on +// snoozed_until > now(), so deleting an expired row changes no behaviour — +// this is purely reclamation. +func (q *Queries) GcDeleteExpiredSuggestionSnoozes(ctx context.Context) (int64, error) { + result, err := q.db.Exec(ctx, gcDeleteExpiredSuggestionSnoozes) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} + +const listActiveSuggestionSnoozes = `-- name: ListActiveSuggestionSnoozes :many +SELECT candidate_mbid, candidate_name, snoozed_until, created_at + FROM suggestion_snoozes + WHERE user_id = $1 AND snoozed_until > now() + ORDER BY snoozed_until, candidate_mbid +` + +type ListActiveSuggestionSnoozesRow struct { + CandidateMbid string + CandidateName string + SnoozedUntil pgtype.Timestamptz + CreatedAt pgtype.Timestamptz +} + +// Backs the manage / un-snooze surface. Expired rows are filtered HERE +// rather than left to the sweeper: gc runs on an hourly tick, so a row can +// outlive its expiry by up to a tick and must not read as still-snoozed in +// the meantime. +func (q *Queries) ListActiveSuggestionSnoozes(ctx context.Context, userID pgtype.UUID) ([]ListActiveSuggestionSnoozesRow, error) { + rows, err := q.db.Query(ctx, listActiveSuggestionSnoozes, userID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []ListActiveSuggestionSnoozesRow + for rows.Next() { + var i ListActiveSuggestionSnoozesRow + if err := rows.Scan( + &i.CandidateMbid, + &i.CandidateName, + &i.SnoozedUntil, + &i.CreatedAt, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const snoozeSuggestion = `-- name: SnoozeSuggestion :exec + +INSERT INTO suggestion_snoozes (user_id, candidate_mbid, candidate_name, snoozed_until) +VALUES ($1, $2, $3, now() + ($4::float8 * INTERVAL '1 day')) +ON CONFLICT (user_id, candidate_mbid) DO UPDATE + SET snoozed_until = EXCLUDED.snoozed_until, + candidate_name = EXCLUDED.candidate_name +` + +type SnoozeSuggestionParams struct { + UserID pgtype.UUID + CandidateMbid string + CandidateName string + Column4 float64 +} + +// Time-boxed "not right now" on a Discover artist suggestion (#2374). +// +// See 0049_suggestion_snoozes.up.sql for why this is a snooze and not a +// dismissal: it records no verdict on the music, expires on its own, and +// must never reach the taste profile. Nothing in internal/taste may read +// this table. +// Upsert, so re-snoozing an already-snoozed candidate EXTENDS it instead of +// erroring on the PK. The name is refreshed too — a later suggestion may +// carry a corrected spelling from the similarity feed. +// $1=user_id, $2=candidate_mbid, $3=candidate_name, $4=duration in days. +func (q *Queries) SnoozeSuggestion(ctx context.Context, arg SnoozeSuggestionParams) error { + _, err := q.db.Exec(ctx, snoozeSuggestion, + arg.UserID, + arg.CandidateMbid, + arg.CandidateName, + arg.Column4, + ) + return err +} + +const unsnoozeSuggestion = `-- name: UnsnoozeSuggestion :execrows +DELETE FROM suggestion_snoozes + WHERE user_id = $1 AND candidate_mbid = $2 +` + +type UnsnoozeSuggestionParams struct { + UserID pgtype.UUID + CandidateMbid string +} + +// Row count is returned so the handler can 404 an MBID that was never +// snoozed rather than reporting success for a no-op. +func (q *Queries) UnsnoozeSuggestion(ctx context.Context, arg UnsnoozeSuggestionParams) (int64, error) { + result, err := q.db.Exec(ctx, unsnoozeSuggestion, arg.UserID, arg.CandidateMbid) + if err != nil { + return 0, err + } + return result.RowsAffected(), nil +} diff --git a/internal/db/migrations/0049_suggestion_snoozes.down.sql b/internal/db/migrations/0049_suggestion_snoozes.down.sql new file mode 100644 index 00000000..05f6770d --- /dev/null +++ b/internal/db/migrations/0049_suggestion_snoozes.down.sql @@ -0,0 +1,2 @@ +DROP INDEX IF EXISTS suggestion_snoozes_expiry_idx; +DROP TABLE IF EXISTS suggestion_snoozes; diff --git a/internal/db/migrations/0049_suggestion_snoozes.up.sql b/internal/db/migrations/0049_suggestion_snoozes.up.sql new file mode 100644 index 00000000..02b0907f --- /dev/null +++ b/internal/db/migrations/0049_suggestion_snoozes.up.sql @@ -0,0 +1,33 @@ +-- 0049_suggestion_snoozes.up.sql — time-boxed "not right now" on a Discover +-- artist suggestion (#2374, milestone #268 slice 3). +-- +-- This is NOT a dislike. Rule #101 forbids a "Not for me" / thumbs-down / +-- exclusion UI, and a snooze deliberately isn't one: it records no verdict on +-- the music, expires on its own, and MUST NEVER feed the taste profile. It is +-- acquisition triage — "I don't want to request this right now" — so the same +-- candidate is free to return once snoozed_until passes. Anything that reads +-- this table as negative preference signal is a bug. +-- +-- Per-user (rule #47), never global: one household member parking a +-- suggestion must not remove it from anyone else's deck. +-- +-- candidate_mbid is text with NO foreign key, on purpose. Suggestions come +-- from artist_similarity_unmatched and are out-of-library BY DEFINITION, so +-- there is no artists row to reference — the MBID is the only stable identity +-- available. candidate_name is denormalized for the same reason: the manage / +-- un-snooze list has nowhere else to resolve a display name from. + +CREATE TABLE suggestion_snoozes ( + user_id uuid NOT NULL REFERENCES users(id) ON DELETE CASCADE, + candidate_mbid text NOT NULL, + candidate_name text NOT NULL, + snoozed_until timestamptz NOT NULL, + created_at timestamptz NOT NULL DEFAULT now(), + PRIMARY KEY (user_id, candidate_mbid) +); + +-- Supports the gc sweep's unqualified `WHERE snoozed_until < now()` scan. The +-- composite PK already covers every per-user read, so this is the only extra +-- index worth its write cost at household-scale row counts (same reasoning as +-- lidarr_quarantine in 0011). +CREATE INDEX suggestion_snoozes_expiry_idx ON suggestion_snoozes (snoozed_until); diff --git a/internal/db/migrations/0050_candidate_artist_tags.down.sql b/internal/db/migrations/0050_candidate_artist_tags.down.sql new file mode 100644 index 00000000..cd7cd862 --- /dev/null +++ b/internal/db/migrations/0050_candidate_artist_tags.down.sql @@ -0,0 +1,3 @@ +DROP INDEX IF EXISTS candidate_artist_tag_state_source_idx; +DROP TABLE IF EXISTS candidate_artist_tag_state; +DROP TABLE IF EXISTS candidate_artist_tags; diff --git a/internal/db/migrations/0050_candidate_artist_tags.up.sql b/internal/db/migrations/0050_candidate_artist_tags.up.sql new file mode 100644 index 00000000..660a3da7 --- /dev/null +++ b/internal/db/migrations/0050_candidate_artist_tags.up.sql @@ -0,0 +1,54 @@ +-- 0050_candidate_artist_tags.up.sql — folksonomy tags for OUT-OF-LIBRARY +-- artists (#2376, milestone #268 slice 5). +-- +-- track_tags (0042) cannot hold these: it is FK'd to tracks(id), and a +-- Discover candidate has no local row by definition. So this is a parallel +-- cache keyed by the candidate's MusicBrainz MBID — the only stable identity +-- available for an artist we don't have. +-- +-- Purpose is slice 6: rank suggestions by overlap between these tags and the +-- user's taste_profile_tags, turning "neighbour of an artist you play" into +-- "matches the sound you like". +-- +-- GLOBAL, not per-user: a candidate's tags are a property of the artist, not +-- of anyone's taste. Nothing here is user-scoped, so rule #47 has nothing to +-- scope — the per-user part lives entirely in slice 6's ranking. +-- +-- weight is a normalized folksonomy strength in [0,1], same scale as +-- track_tags, so the two can be compared without a conversion step. +CREATE TABLE candidate_artist_tags ( + candidate_mbid text NOT NULL, + tag text NOT NULL, + weight double precision NOT NULL DEFAULT 1, + PRIMARY KEY (candidate_mbid, tag) +); + +-- Enrichment bookkeeping. This is a SEPARATE table rather than columns on the +-- tags table, because the "providers had nothing" outcome must be recordable +-- for a candidate with zero tag rows — otherwise every empty candidate stays +-- eligible forever and the worker re-fetches it on every tick. +-- +-- tracks solved the same problem with columns on `tracks` (0042), but there is +-- no per-candidate row anywhere to hang them off: artist_similarity_unmatched +-- is keyed (seed_artist_id, candidate_mbid, source) and holds MANY rows per +-- candidate. +-- +-- Absence of a row here means "never processed", so unlike tracks.tag_source +-- this column can be NOT NULL — there is no null-means-pending state to model. +-- 'musicbrainz' | 'lastfm' | 'mixed' → found, cached +-- 'none' → providers confirmed nothing +-- tag_sources_version → bump to re-process settled 'none' +-- +-- A transient failure writes NO row at all (rather than a row it would then +-- have to distinguish), which leaves the candidate eligible for the next pass. +CREATE TABLE candidate_artist_tag_state ( + candidate_mbid text PRIMARY KEY, + tag_source text NOT NULL, + tag_sources_version integer NOT NULL DEFAULT 0, + updated_at timestamptz NOT NULL DEFAULT now() +); + +-- Serves the eligibility scan's "settled 'none' under an older version" +-- branch. The PK already covers the per-candidate lookups. +CREATE INDEX candidate_artist_tag_state_source_idx + ON candidate_artist_tag_state (tag_source, tag_sources_version); diff --git a/internal/db/migrations/0051_discover_tuning.down.sql b/internal/db/migrations/0051_discover_tuning.down.sql new file mode 100644 index 00000000..1da70de4 --- /dev/null +++ b/internal/db/migrations/0051_discover_tuning.down.sql @@ -0,0 +1,10 @@ +-- Drop any audit rows under the scope the constraint is about to forbid, +-- otherwise re-adding the narrower CHECK fails against existing data. +DELETE FROM recommendation_tuning_audit WHERE scope = 'discover'; +ALTER TABLE recommendation_tuning_audit + DROP CONSTRAINT recommendation_tuning_audit_scope_check; +ALTER TABLE recommendation_tuning_audit + ADD CONSTRAINT recommendation_tuning_audit_scope_check + CHECK (scope IN ('radio', 'daily_mix', 'taste')); + +DROP TABLE IF EXISTS discover_tuning; diff --git a/internal/db/migrations/0051_discover_tuning.up.sql b/internal/db/migrations/0051_discover_tuning.up.sql new file mode 100644 index 00000000..54bc2be0 --- /dev/null +++ b/internal/db/migrations/0051_discover_tuning.up.sql @@ -0,0 +1,38 @@ +-- 0051_discover_tuning.up.sql — tunable knobs for the Discover request +-- surface (#2377, milestone #268 slice 6). +-- +-- A FOURTH tuning scope alongside radio / daily_mix / taste. Its own scope +-- rather than extra columns on taste_tuning, for a reason that matters: +-- snooze_days lives here, and a snooze must never be read as taste signal +-- (#2374). Filing it under 'taste' would put it one careless join away from +-- exactly the leak that design forbids. +-- +-- Per rule #25 these are DB-backed and editable in the admin UI with no +-- restart — the shipped values below are defaults, not settings. +CREATE TABLE discover_tuning ( + singleton boolean PRIMARY KEY DEFAULT true + CONSTRAINT discover_tuning_singleton_check CHECK (singleton), + -- How strongly taste-tag overlap boosts a candidate's similarity score. + -- The blend is MULTIPLICATIVE: score * (1 + w * overlap), overlap in + -- [0,1]. So 0 disables the feature outright and leaves pure similarity + -- ranking, 1.0 lets a perfectly-matching candidate double its score, and + -- a candidate with no cached tags is unchanged rather than penalised + -- (rule #131 — tag coverage is permanently partial, see #2376). + tag_overlap_weight double precision NOT NULL, + -- Default snooze duration in days. Was a Go constant in + -- internal/api/suggestions.go; moved here per rule #25. + snooze_days double precision NOT NULL, + updated_at timestamptz NOT NULL DEFAULT now() +); + +INSERT INTO discover_tuning (singleton, tag_overlap_weight, snooze_days) +VALUES (true, 1.0, 90); + +-- Rule #36: a new value for a CHECK-gated column needs the constraint +-- rewritten in the SAME change, or the first audit row written under the new +-- scope fails at runtime rather than at migrate time. +ALTER TABLE recommendation_tuning_audit + DROP CONSTRAINT recommendation_tuning_audit_scope_check; +ALTER TABLE recommendation_tuning_audit + ADD CONSTRAINT recommendation_tuning_audit_scope_check + CHECK (scope IN ('radio', 'daily_mix', 'taste', 'discover')); diff --git a/internal/db/queries/candidate_artist_tags.sql b/internal/db/queries/candidate_artist_tags.sql new file mode 100644 index 00000000..4403689f --- /dev/null +++ b/internal/db/queries/candidate_artist_tags.sql @@ -0,0 +1,103 @@ +-- Folksonomy tags for out-of-library Discover candidates (#2376). Parallel to +-- track_tags.sql, but keyed by MBID because the artist has no local row. See +-- 0050_candidate_artist_tags.up.sql for why the bookkeeping is its own table. + +-- name: ListCandidateArtistsMissingTags :many +-- Candidates eligible for tag enrichment: never processed (no state row) or +-- settled 'none' under an older provider version. +-- +-- artist_similarity_unmatched holds one row per (seed, candidate, source), so +-- this GROUPs to one row per candidate — enriching the same MBID once per seed +-- that pointed at it would multiply the API calls for no gain. +-- +-- ORDER BY summed similarity DESC is the load-bearing part. The candidate pool +-- is O(library artists x neighbours per artist) — thousands — and MusicBrainz +-- allows ~1 req/s, so it can NEVER be fully enriched in one pass. Draining in +-- strength order means the candidates most likely to actually reach a user's +-- deck get tags first, and the long tail fills in over subsequent ticks +-- instead of starving behind it. +-- +-- Already-in-library candidates are skipped: they have an artists row, so +-- their tags belong in track_tags, and the suggestion query filters them out +-- anyway. $1 = current tag_sources_version, $2 = limit. +-- candidate_name is coalesced to the empty string so it lands non-nullable in +-- Go: the name is only a Last.fm lookup key, and empty simply means "MBID-keyed +-- providers only", which the provider chain already handles. max() is an +-- arbitrary-but-deterministic pick when several seeds spell one MBID +-- differently. +SELECT u.candidate_mbid, + coalesce(max(u.candidate_name), '')::text AS candidate_name, + sum(u.score)::float8 AS total_score + FROM artist_similarity_unmatched u + LEFT JOIN candidate_artist_tag_state s ON s.candidate_mbid = u.candidate_mbid + WHERE NOT EXISTS (SELECT 1 FROM artists a WHERE a.mbid = u.candidate_mbid) + AND ( + s.candidate_mbid IS NULL + OR (s.tag_source = 'none' AND s.tag_sources_version < $1) + ) + GROUP BY u.candidate_mbid + ORDER BY total_score DESC, u.candidate_mbid + LIMIT $2; + +-- name: DeleteCandidateArtistTags :exec +-- Clear a candidate's cached tags before rewriting (atomic replace by the +-- caller, same shape as DeleteTrackTags). +DELETE FROM candidate_artist_tags WHERE candidate_mbid = $1; + +-- name: InsertCandidateArtistTag :exec +-- Upsert one (candidate, tag); keep the stronger weight when two providers +-- agree on a tag with different folksonomy strengths. +INSERT INTO candidate_artist_tags (candidate_mbid, tag, weight) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid, tag) + DO UPDATE SET weight = GREATEST(candidate_artist_tags.weight, EXCLUDED.weight); + +-- name: SetCandidateArtistTagState :exec +-- Stamp the enrichment outcome so the drainer skips settled candidates. +-- $2 = 'musicbrainz' | 'lastfm' | 'mixed' | 'none', $3 = current version. +INSERT INTO candidate_artist_tag_state (candidate_mbid, tag_source, tag_sources_version) +VALUES ($1, $2, $3) +ON CONFLICT (candidate_mbid) DO UPDATE + SET tag_source = EXCLUDED.tag_source, + tag_sources_version = EXCLUDED.tag_sources_version, + updated_at = now(); + +-- name: ListCandidateArtistTagsForMbids :many +-- Cached tags for a set of candidates, for slice 6's taste-overlap ranking. +-- One row per (candidate, tag). +SELECT candidate_mbid, tag, weight + FROM candidate_artist_tags + WHERE candidate_mbid = ANY($1::text[]); + +-- name: CountCandidateArtistTagCoverage :one +-- Operator-facing coverage: how many distinct candidates have been processed, +-- and how many of those actually yielded tags. The gap is the honest ceiling +-- from the task — obscure artists with no MBID presence or no upstream tags +-- stay thin no matter how long the worker runs, and that is worth being able +-- to see rather than inferring from a silent surface. +SELECT count(*)::bigint AS processed, + count(*) FILTER (WHERE tag_source <> 'none')::bigint AS with_tags + FROM candidate_artist_tag_state; + +-- name: GcDeleteOrphanedCandidateArtistTags :execrows +-- Drops cached tags for candidates that no longer appear in the similarity +-- feed, or that have since been added to the library (their tags now live in +-- track_tags). The feed is refetched periodically and churns, so without this +-- the cache only ever grows. +DELETE FROM candidate_artist_tags t + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = t.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = t.candidate_mbid); + +-- name: GcDeleteOrphanedCandidateArtistTagState :execrows +-- Same sweep for the bookkeeping rows. Kept as a separate statement rather +-- than a cascade: the two tables are independent by design (a 'none' outcome +-- has state but no tags), so neither can be the parent of the other. +DELETE FROM candidate_artist_tag_state s + WHERE NOT EXISTS ( + SELECT 1 FROM artist_similarity_unmatched u + WHERE u.candidate_mbid = s.candidate_mbid + ) + OR EXISTS (SELECT 1 FROM artists a WHERE a.mbid = s.candidate_mbid); diff --git a/internal/db/queries/recommendation.sql b/internal/db/queries/recommendation.sql index bf9e10f7..fcb35e8b 100644 --- a/internal/db/queries/recommendation.sql +++ b/internal/db/queries/recommendation.sql @@ -258,27 +258,67 @@ ORDER BY started_at DESC LIMIT 1; -- name: SuggestArtistsForUser :many --- M5c: per-user artist suggestions ranked by signal x similarity. The --- seeds CTE collects the user's likes (x5) plus recency-decayed plays --- (exp(-age_days / $2)). The contributions CTE joins those seeds against --- artist_similarity_unmatched and filters out candidates already in --- library or already in a non-terminal lidarr_request. The outer SELECT --- aggregates per candidate, returning the top-3 contributing seeds for --- attribution. $1=user_id, $2=half_life_days, $3=limit. -WITH seeds AS ( +-- Per-user artist suggestions ranked by taste signal x similarity, projected +-- through artist_similarity_unmatched (out-of-library candidates only). +-- +-- Seeds are TIERED (rule #131) so the surface never empties: +-- tier 1 - taste_profile_artists.weight: engagement-graded, time-decayed and +-- SIGNED by internal/taste, so an artist the user has drifted away +-- from stops contributing instead of accumulating forever. +-- tier 2 - likes + completed plays, used ONLY when the profile has no rows +-- (new account, or before the first daily recompute). +-- +-- The signal is log-damped: contribution is signal x similarity, and the old +-- undamped sum let one heavily-played artist's neighbours take every slot -- +-- entrenching harder the MORE the user listened (issue #2367 mechanism 2). +-- +-- Candidates already in the library, already requested and not terminal, or +-- snoozed by this user, are excluded. $1=user_id, $2=half_life_days (tier 2 +-- decay), $3=limit. +WITH artist_plays AS ( + -- Completed plays only. The previous seed query counted every play_event, + -- so skipping an artist repeatedly INCREASED its signal and pushed more of + -- its neighbours at the user (issue #2367 mechanism 3). + SELECT t.artist_id, count(*)::bigint AS play_count + FROM play_events pe + JOIN tracks t ON t.id = pe.track_id + WHERE pe.user_id = $1 AND pe.was_skipped = false + GROUP BY t.artist_id +), +profile_seeds AS ( + SELECT tpa.artist_id, tpa.weight AS raw_signal + FROM taste_profile_artists tpa + WHERE tpa.user_id = $1 AND tpa.weight > 0 +), +fallback_seeds AS ( SELECT a.id AS artist_id, 5.0 * (CASE WHEN gla.artist_id IS NOT NULL THEN 1 ELSE 0 END) + COALESCE(SUM(EXP(- EXTRACT(epoch FROM now() - pe.started_at) / ($2::float8 * 86400.0))), 0) - AS signal, - (gla.artist_id IS NOT NULL) AS is_liked, - COUNT(pe.id)::bigint AS play_count + AS raw_signal FROM artists a LEFT JOIN general_likes_artists gla ON gla.artist_id = a.id AND gla.user_id = $1 LEFT JOIN tracks t ON t.artist_id = a.id - LEFT JOIN play_events pe ON pe.track_id = t.id AND pe.user_id = $1 - WHERE gla.artist_id IS NOT NULL OR pe.id IS NOT NULL + LEFT JOIN play_events pe + ON pe.track_id = t.id AND pe.user_id = $1 AND pe.was_skipped = false + WHERE (gla.artist_id IS NOT NULL OR pe.id IS NOT NULL) + AND NOT EXISTS (SELECT 1 FROM profile_seeds) GROUP BY a.id, gla.artist_id ), +seeds AS ( + SELECT s.artist_id, + ln(1.0 + s.raw_signal) AS signal, + (gla.artist_id IS NOT NULL) AS is_liked, + COALESCE(ap.play_count, 0)::bigint AS play_count + FROM ( + SELECT artist_id, raw_signal FROM profile_seeds + UNION ALL + SELECT artist_id, raw_signal FROM fallback_seeds + ) s + LEFT JOIN general_likes_artists gla + ON gla.artist_id = s.artist_id AND gla.user_id = $1 + LEFT JOIN artist_plays ap ON ap.artist_id = s.artist_id + WHERE s.raw_signal > 0 +), contributions AS ( SELECT u.candidate_mbid, u.candidate_name, @@ -295,6 +335,18 @@ contributions AS ( AND r.lidarr_artist_mbid = u.candidate_mbid AND r.status NOT IN ('rejected', 'failed') ) + -- Snoozed by this user and not yet expired (#2374). Time-boxed and + -- per-user: the candidate returns on its own once snoozed_until + -- passes, and stays visible to everyone else meanwhile. Deliberately + -- filtered at the candidate stage, NOT folded into the score — a + -- snooze carries no opinion about the music, so it must not become a + -- ranking signal. + AND NOT EXISTS ( + SELECT 1 FROM suggestion_snoozes s + WHERE s.user_id = $1 + AND s.candidate_mbid = u.candidate_mbid + AND s.snoozed_until > now() + ) ) SELECT candidate_mbid, candidate_name, diff --git a/internal/db/queries/recommendation_tuning.sql b/internal/db/queries/recommendation_tuning.sql index b017e542..92ae3bce 100644 --- a/internal/db/queries/recommendation_tuning.sql +++ b/internal/db/queries/recommendation_tuning.sql @@ -53,6 +53,24 @@ UPDATE taste_tuning WHERE singleton = true RETURNING *; +-- name: UpsertDiscoverTuningDefaults :exec +-- Boot reconcile for the Discover scope (#2377). Never overwrites +-- operator-tuned values, same contract as the other two. +INSERT INTO discover_tuning (singleton, tag_overlap_weight, snooze_days) +VALUES (true, $1, $2) +ON CONFLICT (singleton) DO NOTHING; + +-- name: GetDiscoverTuning :one +SELECT * FROM discover_tuning WHERE singleton = true; + +-- name: UpdateDiscoverTuning :one +UPDATE discover_tuning + SET tag_overlap_weight = $1, + snooze_days = $2, + updated_at = now() + WHERE singleton = true +RETURNING *; + -- name: InsertTuningAudit :exec -- changes is a jsonb array of {field, old, new} objects. INSERT INTO recommendation_tuning_audit (scope, action, changes) diff --git a/internal/db/queries/suggestion_snoozes.sql b/internal/db/queries/suggestion_snoozes.sql new file mode 100644 index 00000000..952b1ef7 --- /dev/null +++ b/internal/db/queries/suggestion_snoozes.sql @@ -0,0 +1,39 @@ +-- Time-boxed "not right now" on a Discover artist suggestion (#2374). +-- +-- See 0049_suggestion_snoozes.up.sql for why this is a snooze and not a +-- dismissal: it records no verdict on the music, expires on its own, and +-- must never reach the taste profile. Nothing in internal/taste may read +-- this table. + +-- name: SnoozeSuggestion :exec +-- Upsert, so re-snoozing an already-snoozed candidate EXTENDS it instead of +-- erroring on the PK. The name is refreshed too — a later suggestion may +-- carry a corrected spelling from the similarity feed. +-- $1=user_id, $2=candidate_mbid, $3=candidate_name, $4=duration in days. +INSERT INTO suggestion_snoozes (user_id, candidate_mbid, candidate_name, snoozed_until) +VALUES ($1, $2, $3, now() + ($4::float8 * INTERVAL '1 day')) +ON CONFLICT (user_id, candidate_mbid) DO UPDATE + SET snoozed_until = EXCLUDED.snoozed_until, + candidate_name = EXCLUDED.candidate_name; + +-- name: UnsnoozeSuggestion :execrows +-- Row count is returned so the handler can 404 an MBID that was never +-- snoozed rather than reporting success for a no-op. +DELETE FROM suggestion_snoozes + WHERE user_id = $1 AND candidate_mbid = $2; + +-- name: ListActiveSuggestionSnoozes :many +-- Backs the manage / un-snooze surface. Expired rows are filtered HERE +-- rather than left to the sweeper: gc runs on an hourly tick, so a row can +-- outlive its expiry by up to a tick and must not read as still-snoozed in +-- the meantime. +SELECT candidate_mbid, candidate_name, snoozed_until, created_at + FROM suggestion_snoozes + WHERE user_id = $1 AND snoozed_until > now() + ORDER BY snoozed_until, candidate_mbid; + +-- name: GcDeleteExpiredSuggestionSnoozes :execrows +-- Keeps the table from growing without bound. Every read already filters on +-- snoozed_until > now(), so deleting an expired row changes no behaviour — +-- this is purely reclamation. +DELETE FROM suggestion_snoozes WHERE snoozed_until < now(); diff --git a/internal/dbtest/reset.go b/internal/dbtest/reset.go index b499dfeb..c71b9aae 100644 --- a/internal/dbtest/reset.go +++ b/internal/dbtest/reset.go @@ -52,6 +52,18 @@ var dataTables = []string{ "sessions", "lidarr_quarantine_actions", "lidarr_quarantine", + // #2374. Keyed by (user_id, candidate_mbid) with no FK to artists — + // candidates are out-of-library — so the CASCADE from artists/users + // does NOT reach it for a leftover row whose user survived. Truncate + // explicitly or a stale snooze silently hides a candidate from the + // next test's suggestion assertions. + "suggestion_snoozes", + // #2376. Same reasoning: keyed by candidate MBID with no FK anywhere, + // so nothing cascades to them. A leftover tag row would make a + // candidate look enriched to the next test, and a leftover state row + // would make it look already-settled and thus ineligible. + "candidate_artist_tags", + "candidate_artist_tag_state", "playlist_tracks", "playlists", "library_changes", // M7 #357 — must reset to keep cursor isolated per test @@ -70,6 +82,10 @@ var dataTables = []string{ // (#1250), so truncating gives each test pristine tuning values. "recommendation_weight_profiles", "taste_tuning", + // #2377. Same reasoning as taste_tuning above: recsettings.New re-seeds + // shipped defaults on every construction, so truncating gives each test + // pristine Discover knobs rather than whatever a previous test tuned. + "discover_tuning", "recommendation_tuning_audit", "tracks", "albums", diff --git a/internal/gc/worker.go b/internal/gc/worker.go index b133e6c4..39ca9a3c 100644 --- a/internal/gc/worker.go +++ b/internal/gc/worker.go @@ -17,6 +17,9 @@ // - GcResetStuckSystemPlaylistRuns (#574) // - GcDeleteExpiredPasswordResets (#575) // - GcPruneDiagnostics (M9 — diagnostics 30d retention) +// - GcDeleteExpiredSuggestionSnoozes (#2374 — snoozes expire, then go) +// - GcDeleteOrphanedCandidateArtistTags(+State) (#2376 — the similarity +// feed churns, so cached candidate tags outlive their candidates) package gc import ( @@ -84,6 +87,14 @@ func (w *Worker) tickOnce(ctx context.Context) { w.runSweep(ctx, "reset_stuck_system_runs", q.GcResetStuckSystemPlaylistRuns) w.runSweep(ctx, "delete_expired_password_resets", q.GcDeleteExpiredPasswordResets) w.runSweep(ctx, "prune_diagnostics", q.GcPruneDiagnostics) + w.runSweep(ctx, "delete_expired_suggestion_snoozes", q.GcDeleteExpiredSuggestionSnoozes) + // Tags before state: if the process dies between the two, a candidate left + // with a state row and no tags simply reads as "settled, nothing found", + // which is already a valid state. The reverse order could leave tags with + // no state row, which the drainer would treat as never-processed and + // re-fetch on top of rows that are already there. + w.runSweep(ctx, "orphaned_candidate_artist_tags", q.GcDeleteOrphanedCandidateArtistTags) + w.runSweep(ctx, "orphaned_candidate_artist_tag_state", q.GcDeleteOrphanedCandidateArtistTagState) } // runSweep is a small adapter so each sweep call site is a one-liner diff --git a/internal/recommendation/candidate_tags_integration_test.go b/internal/recommendation/candidate_tags_integration_test.go new file mode 100644 index 00000000..19cefbc8 --- /dev/null +++ b/internal/recommendation/candidate_tags_integration_test.go @@ -0,0 +1,455 @@ +package recommendation + +import ( + "context" + "testing" + + "github.com/jackc/pgx/v5/pgtype" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// Slice 5 (#2376): the candidate-artist tag cache for out-of-library Discover +// candidates. These cover the SQL rather than the provider chain — the +// eligibility query is the piece with real risk in it (a GROUP BY over a +// many-rows-per-candidate table, a LEFT JOIN to bookkeeping, and two exclusion +// branches), and it's consumed by this package's slice-6 ranking. +// +// Fixtures live here because the harness and seedUnmatched do. + +const ( + tagVersionCurrent = 2 + tagVersionOld = 1 +) + +// listEligible is the query under test, at the current provider version. +func listEligible(t *testing.T, pool *pgxpool.Pool, limit int32) []dbq.ListCandidateArtistsMissingTagsRow { + t.Helper() + rows, err := dbq.New(pool).ListCandidateArtistsMissingTags(context.Background(), + dbq.ListCandidateArtistsMissingTagsParams{ + TagSourcesVersion: tagVersionCurrent, + Limit: limit, + }) + if err != nil { + t.Fatalf("ListCandidateArtistsMissingTags: %v", err) + } + return rows +} + +func setState(t *testing.T, pool *pgxpool.Pool, mbid, source string, version int32) { + t.Helper() + if err := dbq.New(pool).SetCandidateArtistTagState(context.Background(), + dbq.SetCandidateArtistTagStateParams{ + CandidateMbid: mbid, TagSource: source, TagSourcesVersion: version, + }); err != nil { + t.Fatalf("SetCandidateArtistTagState: %v", err) + } +} + +func mbidsOf(rows []dbq.ListCandidateArtistsMissingTagsRow) []string { + out := make([]string, 0, len(rows)) + for _, r := range rows { + out = append(out, r.CandidateMbid) + } + return out +} + +func TestCandidateTags_NeverProcessedIsEligible(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, user.ID, seed.ID) + seedUnmatched(t, pool, seed.ID, "cand-1", "Candidate One", 0.9) + + rows := listEligible(t, pool, 10) + if len(rows) != 1 { + t.Fatalf("len = %d, want 1: %v", len(rows), mbidsOf(rows)) + } + if rows[0].CandidateName != "Candidate One" { + t.Errorf("name = %q, want Candidate One", rows[0].CandidateName) + } + if rows[0].TotalScore != 0.9 { + t.Errorf("score = %v, want 0.9", rows[0].TotalScore) + } +} + +// An in-library candidate's tags belong in track_tags, and the suggestion query +// filters it out anyway — enriching it would be wasted API budget. +func TestCandidateTags_InLibraryCandidateIsExcluded(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedArtist(t, pool, "Already Here", "cand-in-lib") + seedUnmatched(t, pool, seed.ID, "cand-in-lib", "Already Here", 0.9) + + if rows := listEligible(t, pool, 10); len(rows) != 0 { + t.Errorf("len = %d, want 0: %v", len(rows), mbidsOf(rows)) + } +} + +func TestCandidateTags_SettledWithTagsIsExcluded(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "cand-1", "Candidate One", 0.9) + setState(t, pool, "cand-1", "musicbrainz", tagVersionCurrent) + + if rows := listEligible(t, pool, 10); len(rows) != 0 { + t.Errorf("len = %d, want 0 (already enriched): %v", len(rows), mbidsOf(rows)) + } +} + +// A candidate that settled 'none' becomes eligible again when the provider set +// widens (version bump) — that's the whole point of the version column. It must +// NOT be eligible at the current version, or the worker re-fetches it forever. +func TestCandidateTags_SettledNoneReopensOnlyOnVersionBump(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "cand-1", "Candidate One", 0.9) + + setState(t, pool, "cand-1", "none", tagVersionCurrent) + if rows := listEligible(t, pool, 10); len(rows) != 0 { + t.Errorf("current version: len = %d, want 0 (settled)", len(rows)) + } + + setState(t, pool, "cand-1", "none", tagVersionOld) + if rows := listEligible(t, pool, 10); len(rows) != 1 { + t.Errorf("older version: len = %d, want 1 (eligible again)", len(rows)) + } +} + +// artist_similarity_unmatched holds one row per (seed, candidate, source). +// Without the GROUP BY, a candidate that five seeds point at would be fetched +// five times — five times the MusicBrainz budget for identical data. +func TestCandidateTags_ManySeedsCollapseToOneRowAndSumScores(t *testing.T) { + pool := newPool(t) + seedA := seedArtist(t, pool, "Seed A", "") + seedB := seedArtist(t, pool, "Seed B", "") + seedUnmatched(t, pool, seedA.ID, "cand-1", "Candidate One", 0.4) + seedUnmatched(t, pool, seedB.ID, "cand-1", "Candidate One", 0.3) + + rows := listEligible(t, pool, 10) + if len(rows) != 1 { + t.Fatalf("len = %d, want 1 (grouped): %v", len(rows), mbidsOf(rows)) + } + if got := rows[0].TotalScore; got < 0.69 || got > 0.71 { + t.Errorf("total_score = %v, want ~0.7 (summed across seeds)", got) + } +} + +// The pool is far larger than one pass can drain at ~1 req/s, so the ordering +// IS the feature: the strongest candidates must be enriched first, or the ones +// that actually reach a user's deck starve behind the long tail. +func TestCandidateTags_StrongestCandidatesComeFirst(t *testing.T) { + pool := newPool(t) + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "weak", "Weak", 0.1) + seedUnmatched(t, pool, seed.ID, "strong", "Strong", 0.95) + seedUnmatched(t, pool, seed.ID, "middle", "Middle", 0.5) + + rows := listEligible(t, pool, 10) + want := []string{"strong", "middle", "weak"} + got := mbidsOf(rows) + if len(got) != 3 { + t.Fatalf("len = %d, want 3: %v", len(got), got) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("order = %v, want %v", got, want) + } + } + + // And the limit takes the strongest, not an arbitrary slice. + if top := mbidsOf(listEligible(t, pool, 1)); len(top) != 1 || top[0] != "strong" { + t.Errorf("limit 1 returned %v, want [strong]", top) + } +} + +func TestCandidateTags_InsertKeepsTheStrongerWeight(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + ins := func(w float64) { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: "cand-1", Tag: "shoegaze", Weight: w, + }); err != nil { + t.Fatalf("InsertCandidateArtistTag: %v", err) + } + } + ins(0.8) + ins(0.3) // weaker second write must not clobber + + rows, err := q.ListCandidateArtistTagsForMbids(ctx, []string{"cand-1"}) + if err != nil { + t.Fatalf("ListCandidateArtistTagsForMbids: %v", err) + } + if len(rows) != 1 { + t.Fatalf("len = %d, want 1", len(rows)) + } + if rows[0].Weight != 0.8 { + t.Errorf("weight = %v, want 0.8 (GREATEST)", rows[0].Weight) + } +} + +func TestCandidateTags_ListForMbidsIgnoresUnaskedCandidates(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + for _, mbid := range []string{"want-1", "want-2", "other"} { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: mbid, Tag: "rock", Weight: 1, + }); err != nil { + t.Fatalf("insert %s: %v", mbid, err) + } + } + rows, err := q.ListCandidateArtistTagsForMbids(ctx, []string{"want-1", "want-2"}) + if err != nil { + t.Fatalf("ListCandidateArtistTagsForMbids: %v", err) + } + if len(rows) != 2 { + t.Errorf("len = %d, want 2", len(rows)) + } + for _, r := range rows { + if r.CandidateMbid == "other" { + t.Error("returned a candidate that wasn't asked for") + } + } +} + +// The similarity feed is refetched and churns, so without the sweep the cache +// only grows. Both halves must survive/die together for the right candidates. +func TestCandidateTags_GcDropsOrphansAndKeepsLiveOnes(t *testing.T) { + pool := newPool(t) + q := dbq.New(pool) + ctx := context.Background() + + seed := seedArtist(t, pool, "Seed", "") + seedUnmatched(t, pool, seed.ID, "live", "Live", 0.9) + // "gone" is cached but no longer in the feed; "adopted" got added to the + // library since, so its tags belong in track_tags now. + seedArtist(t, pool, "Adopted", "adopted") + seedUnmatched(t, pool, seed.ID, "adopted", "Adopted", 0.8) + + for _, mbid := range []string{"live", "gone", "adopted"} { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: mbid, Tag: "rock", Weight: 1, + }); err != nil { + t.Fatalf("insert %s: %v", mbid, err) + } + setState(t, pool, mbid, "musicbrainz", tagVersionCurrent) + } + + deleted, err := q.GcDeleteOrphanedCandidateArtistTags(ctx) + if err != nil { + t.Fatalf("GcDeleteOrphanedCandidateArtistTags: %v", err) + } + if deleted != 2 { + t.Errorf("deleted %d tag rows, want 2 (gone + adopted)", deleted) + } + deletedState, err := q.GcDeleteOrphanedCandidateArtistTagState(ctx) + if err != nil { + t.Fatalf("GcDeleteOrphanedCandidateArtistTagState: %v", err) + } + if deletedState != 2 { + t.Errorf("deleted %d state rows, want 2", deletedState) + } + + rows, err := q.ListCandidateArtistTagsForMbids(ctx, []string{"live", "gone", "adopted"}) + if err != nil { + t.Fatalf("ListCandidateArtistTagsForMbids: %v", err) + } + if len(rows) != 1 || rows[0].CandidateMbid != "live" { + t.Errorf("survivors = %v, want [live] only", rows) + } +} + +// Coverage is the operator's window onto the honest ceiling: processed vs +// actually-tagged. A big gap means thin upstream data, not a broken worker. +func TestCandidateTags_CoverageCountsProcessedAndTagged(t *testing.T) { + pool := newPool(t) + setState(t, pool, "has-tags", "musicbrainz", tagVersionCurrent) + setState(t, pool, "mixed-tags", "mixed", tagVersionCurrent) + setState(t, pool, "no-tags", "none", tagVersionCurrent) + + got, err := dbq.New(pool).CountCandidateArtistTagCoverage(context.Background()) + if err != nil { + t.Fatalf("CountCandidateArtistTagCoverage: %v", err) + } + if got.Processed != 3 { + t.Errorf("processed = %d, want 3", got.Processed) + } + if got.WithTags != 2 { + t.Errorf("with_tags = %d, want 2 ('none' excluded)", got.WithTags) + } +} + +// --- Slice 6 (#2377): the taste-tag blend, end to end --- +// +// The pure tests in tagoverlap_test.go cover the scoring maths. These cover the +// wiring the pure tests cannot reach: that loadTagInputs actually reads both +// sides from the DB and that the blend reaches the returned deck. + +func seedTasteTag(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID, tag string, weight float64) { + t.Helper() + if _, err := pool.Exec(context.Background(), + `INSERT INTO taste_profile_tags (user_id, tag, weight) VALUES ($1, $2, $3) + ON CONFLICT (user_id, tag) DO UPDATE SET weight = EXCLUDED.weight`, + userID, tag, weight, + ); err != nil { + t.Fatalf("seedTasteTag: %v", err) + } +} + +func seedCandidateTag(t *testing.T, pool *pgxpool.Pool, mbid, tag string, weight float64) { + t.Helper() + if err := dbq.New(pool).InsertCandidateArtistTag(context.Background(), + dbq.InsertCandidateArtistTagParams{CandidateMbid: mbid, Tag: tag, Weight: weight}, + ); err != nil { + t.Fatalf("seedCandidateTag: %v", err) + } +} + +// twoCandidatePool wires a liked seed with two unmatched neighbours: "loud" +// scores higher on similarity, "match" lower. Skewed on purpose — with equal +// similarity the reorder assertion below could not fail. +func twoCandidatePool(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID) { + t.Helper() + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, userID, seed.ID) + seedUnmatched(t, pool, seed.ID, "loud", "Loud Neighbour", 0.9) + seedUnmatched(t, pool, seed.ID, "match", "Taste Match", 0.6) +} + +func TestSuggestArtists_TasteTagMatchOvertakesStrongerSimilarity(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + twoCandidatePool(t, pool, user.ID) + + seedTasteTag(t, pool, user.ID, "shoegaze", 10) + seedCandidateTag(t, pool, "match", "shoegaze", 1.0) + seedCandidateTag(t, pool, "loud", "death metal", 1.0) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 1.0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 2 { + t.Fatalf("len = %d, want 2", len(out)) + } + if out[0].MBID != "match" { + t.Errorf("first = %q, want match (0.6×2 beats 0.9×1)", out[0].MBID) + } + // And it explains itself. + if len(out[0].MatchedTags) != 1 || out[0].MatchedTags[0] != "shoegaze" { + t.Errorf("matched tags = %v, want [shoegaze]", out[0].MatchedTags) + } + if out[1].MatchedTags != nil { + t.Errorf("non-matching candidate got matched tags: %v", out[1].MatchedTags) + } +} + +// The same fixture with the knob at 0 must return pure similarity order — the +// operator's off switch, verified against a real DB rather than assumed. +func TestSuggestArtists_ZeroTagWeightKeepsSimilarityOrder(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + twoCandidatePool(t, pool, user.ID) + + seedTasteTag(t, pool, user.ID, "shoegaze", 10) + seedCandidateTag(t, pool, "match", "shoegaze", 1.0) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if out[0].MBID != "loud" { + t.Errorf("first = %q, want loud (tag term disabled)", out[0].MBID) + } +} + +// A user with no taste profile must still get a full deck in similarity order: +// the cold-start path, which is every user's first days (rule #131). +func TestSuggestArtists_NoTasteTagsStillReturnsSimilarityOrder(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + twoCandidatePool(t, pool, user.ID) + // Candidate tags exist, but the user has no taste tags to match them. + seedCandidateTag(t, pool, "match", "shoegaze", 1.0) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 1.0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 2 { + t.Fatalf("len = %d, want 2 (nothing dropped)", len(out)) + } + if out[0].MBID != "loud" { + t.Errorf("first = %q, want loud", out[0].MBID) + } +} + +// An untagged candidate must never be dropped or sunk just because another +// candidate has tags — permanently-partial coverage (#2376) must not become a +// permanent ranking penalty. +func TestSuggestArtists_UntaggedCandidateSurvivesAlongsideTagged(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + twoCandidatePool(t, pool, user.ID) + + seedTasteTag(t, pool, user.ID, "shoegaze", 10) + seedCandidateTag(t, pool, "match", "shoegaze", 1.0) + // "loud" is deliberately left with NO cached tags at all. + + // Compared against the same request with the term disabled, rather than + // against a literal. The pool score is signal-weighted by the seed query + // (ln(1+signal) x similarity), so hardcoding a number here would assert + // against the seeding maths — a different layer — and break whenever that + // changes. The property under test is only that the blend leaves an + // untagged candidate alone. + blended, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 1.0) + if err != nil { + t.Fatalf("SuggestArtists(blended): %v", err) + } + baseline, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists(baseline): %v", err) + } + if len(blended) != 2 { + t.Fatalf("len = %d, want 2 — the untagged candidate must still appear", len(blended)) + } + + scoreOf := func(out []ArtistSuggestion, mbid string) (float64, bool) { + for _, s := range out { + if s.MBID == mbid { + return s.Score, true + } + } + return 0, false + } + want, ok := scoreOf(baseline, "loud") + if !ok { + t.Fatal("untagged candidate missing from the baseline deck") + } + got, ok := scoreOf(blended, "loud") + if !ok { + t.Fatal("untagged candidate vanished once the tag term was enabled") + } + if got != want { + t.Errorf("untagged score = %v, want %v (identical to the term-disabled run)", got, want) + } + // Sanity: the tagged candidate DID move, so the comparison above is + // meaningful rather than both runs being trivially identical. + if tagged, _ := scoreOf(blended, "match"); tagged == mustScore(t, baseline, "match") { + t.Error("the tagged candidate's score did not change — the term did nothing") + } +} + +func mustScore(t *testing.T, out []ArtistSuggestion, mbid string) float64 { + t.Helper() + for _, s := range out { + if s.MBID == mbid { + return s.Score + } + } + t.Fatalf("no candidate %q in %v", mbid, out) + return 0 +} diff --git a/internal/recommendation/suggestions.go b/internal/recommendation/suggestions.go index dfe970e3..348ebf6d 100644 --- a/internal/recommendation/suggestions.go +++ b/internal/recommendation/suggestions.go @@ -1,12 +1,23 @@ -// suggestions.go is the M5c per-user artist-suggestion service. Reads -// the user's likes + plays, projects them through artist_similarity_unmatched -// via a single CTE, returns top-N candidates with top-3 attribution seeds -// resolved to artist names. +// suggestions.go is the per-user artist-suggestion service behind the +// Discover request surface. Seeds from the taste profile (falling back to +// likes + completed plays for a user who has none yet), projects those seeds +// through artist_similarity_unmatched via a single CTE, and returns top-N +// out-of-library candidates with top-3 attribution seeds resolved to names. +// +// Originally M5c, which seeded from raw likes + plays. That signal grew +// without bound and counted skips as engagement, so a few heavily-played +// artists monopolized every slot and the surface entrenched harder the more +// the user listened. Reworked to seed from the taste profile in issue #2367; +// see internal/db/queries/recommendation.sql for the tiering. package recommendation import ( "context" + "crypto/md5" + "encoding/hex" "fmt" + "sort" + "time" "github.com/jackc/pgx/v5/pgtype" "github.com/jackc/pgx/v5/pgxpool" @@ -20,6 +31,15 @@ type ArtistSuggestion struct { Name string Score float64 Attribution []SeedContribution + // MatchedTags are the candidate's own tags that overlap the user's taste + // profile, strongest first (max 3) — the "matches: shoegaze, melancholic" + // explanation (#2377). Empty when the candidate has no cached tags, which + // is common and not an error: coverage is permanently partial (#2376). + MatchedTags []string + // TagOverlap is the [0,1] share of the candidate's tag mass the user likes. + // Exposed for the admin tuning lab — seeing the term's actual distribution + // is how the operator picks a weight rather than guessing at one. + TagOverlap float64 } // SeedContribution is one of the top-3 contributing seeds for a candidate. @@ -32,9 +52,16 @@ type SeedContribution struct { } // SuggestArtists returns top-N artist suggestions for the user. limit is -// capped at 50 (default 12 when out of range); halfLifeDays is the -// recency-decay half-life for plays (default 30, operator-tunable). -func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, halfLifeDays float64, limit int) ([]ArtistSuggestion, error) { +// capped at 50 (default 12 when out of range). +// +// halfLifeDays (default 30) is the recency-decay half-life for the TIER-2 +// seed path only — the likes + completed-plays fallback used while the user +// has no taste-profile rows yet. Once the profile is populated it seeds +// instead, carrying its own decay, so this knob stops applying. +func SuggestArtists( + ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, + halfLifeDays float64, limit int, tagOverlapWeight float64, +) ([]ArtistSuggestion, error) { if limit <= 0 || limit > 50 { limit = 12 } @@ -42,10 +69,14 @@ func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, halfLifeDays = 30 } q := dbq.New(pool) + // Over-fetch so there is something to rotate through. A deterministic + // top-N over a score ordering shows the same faces every day until one is + // requested away — the tail of the ranking was unreachable (#2367 + // mechanism 1). Selection from this pool happens in selectSuggestions. rows, err := q.SuggestArtistsForUser(ctx, dbq.SuggestArtistsForUserParams{ UserID: userID, Column2: halfLifeDays, - Limit: int32(limit), + Limit: int32(poolSizeFor(limit)), }) if err != nil { return nil, fmt.Errorf("suggest: query: %w", err) @@ -98,5 +129,195 @@ func SuggestArtists(ctx context.Context, pool *pgxpool.Pool, userID pgtype.UUID, Attribution: attribution, }) } - return out, nil + + // Taste-tag term (#2377). Applied to the whole over-fetched pool BEFORE + // selection, so the rotation and diversity rules in selectSuggestions + // operate on taste-blended scores — boosting only the twelve already + // chosen by similarity would leave the actual re-ranking undone. + // + // A query error here is returned, NOT degraded past. Graceful degradation + // is for expected absence — no taste profile yet, no cached tags for a + // candidate — and both of those are handled explicitly as empty inputs + // below. A failing query is neither: swallowing it would hide a broken DB + // behind a subtly worse ranking that nothing reports. + tasteTags, candTags, err := loadTagInputs(ctx, q, userID, out) + if err != nil { + return nil, err + } + out = applyTagOverlap(out, candTags, tasteTags, tagOverlapWeight) + + return selectSuggestions(out, limit, rotationDay(time.Now())), nil +} + +// tasteTagLimit caps how many of the user's taste tags participate. The +// profile's long tail is near-zero weight and contributes nothing after +// normalization, so this bounds the query rather than the meaning. +const tasteTagLimit = 50 + +// loadTagInputs fetches both sides of the overlap comparison: the user's taste +// tags and the cached tags for exactly the candidates in this pool. +func loadTagInputs( + ctx context.Context, q *dbq.Queries, userID pgtype.UUID, pool []ArtistSuggestion, +) (TagWeights, map[string]TagWeights, error) { + tasteRows, err := q.ListTasteProfileTagsForUser(ctx, dbq.ListTasteProfileTagsForUserParams{ + UserID: userID, + Limit: tasteTagLimit, + }) + if err != nil { + return nil, nil, fmt.Errorf("suggest: taste tags: %w", err) + } + // No taste tags is a cold start, not a failure — return early and skip the + // candidate-tag fetch entirely, since nothing could match. + if len(tasteRows) == 0 { + return nil, nil, nil + } + taste := make(TagWeights, len(tasteRows)) + for _, r := range tasteRows { + taste[r.Tag] = r.Weight + } + + mbids := make([]string, 0, len(pool)) + for _, s := range pool { + mbids = append(mbids, s.MBID) + } + tagRows, err := q.ListCandidateArtistTagsForMbids(ctx, mbids) + if err != nil { + return nil, nil, fmt.Errorf("suggest: candidate tags: %w", err) + } + byCandidate := make(map[string]TagWeights, len(pool)) + for _, r := range tagRows { + if byCandidate[r.CandidateMbid] == nil { + byCandidate[r.CandidateMbid] = TagWeights{} + } + byCandidate[r.CandidateMbid][r.Tag] = r.Weight + } + return taste, byCandidate, nil +} + +// Pool multiplier: how many scored candidates to fetch per slot shown, so the +// rotation has somewhere to rotate. 4x keeps a day's deck genuinely different +// from yesterday's without pulling the whole long tail (whose scores are noise) +// into every request. +const suggestionPoolFactor = 4 + +// Hard ceiling on the fetched pool. The scored tail past this is low-signal, so +// paying for it would buy churn rather than better suggestions. +const suggestionPoolMax = 60 + +func poolSizeFor(limit int) int { + n := limit * suggestionPoolFactor + if n > suggestionPoolMax { + return suggestionPoolMax + } + return n +} + +// rotationDay is the bucket the daily rotation hashes against. Server-local +// date, matching the `current_date` the Home rows already rotate on, so the +// whole product turns over at the same moment. +func rotationDay(now time.Time) string { + return now.Format("2006-01-02") +} + +// selectSuggestions turns the scored pool into the response. +// +// Pure by design — no DB, no clock — so the rotation and diversity rules are +// unit-testable without Postgres. Callers pass the day bucket in. +// +// Three rules, in order: +// +// 1. Diversity cap. Walking in score order, a candidate is dropped once its +// dominant seed already owns maxPerSeed slots. Nothing capped this before, +// so all twelve slots could be neighbours of one artist and the surface +// read as a single narrow cluster (#2367 mechanism 4). +// 2. Head. The best few by score always lead, so the strongest matches are +// never rotated out of sight. Mirrors For You's head/tail shape. +// 3. Tail. The rest of the slots are drawn from the remaining pool ordered by +// md5(mbid + day) — the same daily-stable idiom the Home rows use. Stable +// within a day, different tomorrow, and it needs no stored state. +func selectSuggestions(pool []ArtistSuggestion, limit int, day string) []ArtistSuggestion { + if len(pool) <= limit { + return pool + } + preferred, overflow := partitionPerSeed(pool, maxPerSeedFor(limit)) + + headCount := limit / 3 + if headCount > len(preferred) { + headCount = len(preferred) + } + out := make([]ArtistSuggestion, 0, limit) + out = append(out, preferred[:headCount]...) + + // Hash once per candidate, not once per comparison. + rest := preferred[headCount:] + tail := make([]rotatable, 0, len(rest)) + for _, s := range rest { + tail = append(tail, rotatable{key: rotationKey(s.MBID, day), suggestion: s}) + } + sort.SliceStable(tail, func(i, j int) bool { return tail[i].key < tail[j].key }) + for _, r := range tail { + if len(out) >= limit { + break + } + out = append(out, r.suggestion) + } + // Diversity is a PREFERENCE, not a quota that may starve the deck. A user + // whose whole pool hangs off one or two seeds would otherwise get a + // three-card surface, which is worse than the monoculture we were avoiding + // (and is the vanish-or-nothing shape rule #131 exists to prevent). Top up + // in score order from what the cap set aside. + for _, s := range overflow { + if len(out) >= limit { + break + } + out = append(out, s) + } + return out +} + +// rotatable pairs a candidate with its precomputed daily rotation key. +type rotatable struct { + key string + suggestion ArtistSuggestion +} + +// maxPerSeedFor keeps roughly a quarter of the deck attributable to any single +// seed artist — enough for a strong affinity to be well represented, not enough +// for it to BE the deck. Floored at 1 so a small limit still returns something. +func maxPerSeedFor(limit int) int { + n := limit / 4 + if n < 1 { + return 1 + } + return n +} + +// partitionPerSeed splits the pool by whether each candidate's dominant +// (highest-contributing) seed still has allowance left. Input must be in score +// order; both outputs preserve it. +// +// Candidates with no attribution are always preferred — there is no seed to +// attribute them to, so they cannot be the cause of a monoculture. +func partitionPerSeed(pool []ArtistSuggestion, maxPerSeed int) (preferred, overflow []ArtistSuggestion) { + perSeed := make(map[pgtype.UUID]int, len(pool)) + preferred = make([]ArtistSuggestion, 0, len(pool)) + for _, s := range pool { + if len(s.Attribution) == 0 { + preferred = append(preferred, s) + continue + } + dominant := s.Attribution[0].ArtistID + if perSeed[dominant] >= maxPerSeed { + overflow = append(overflow, s) + continue + } + perSeed[dominant]++ + preferred = append(preferred, s) + } + return preferred, overflow +} + +func rotationKey(mbid, day string) string { + sum := md5.Sum([]byte(mbid + day)) + return hex.EncodeToString(sum[:]) } diff --git a/internal/recommendation/suggestions_integration_test.go b/internal/recommendation/suggestions_integration_test.go index 9b5cf0c4..7a8cd40c 100644 --- a/internal/recommendation/suggestions_integration_test.go +++ b/internal/recommendation/suggestions_integration_test.go @@ -147,7 +147,7 @@ func TestSuggestArtists_LikesAndPlaysContributeToScore(t *testing.T) { seedUnmatched(t, pool, seedA.ID, "out-mbid", "Outsider", 0.9) seedUnmatched(t, pool, seedB.ID, "out-mbid", "Outsider", 0.5) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -174,7 +174,7 @@ func TestSuggestArtists_Top12Cap(t *testing.T) { for i := 0; i < 30; i++ { seedUnmatched(t, pool, seed.ID, fmt.Sprintf("mbid-%02d", i), fmt.Sprintf("Artist %02d", i), 0.99-float64(i)*0.01) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -195,7 +195,7 @@ func TestSuggestArtists_AttributionTopThree(t *testing.T) { likeArtist(t, pool, user.ID, seeds[i].ID) seedUnmatched(t, pool, seeds[i].ID, "shared-mbid", "Shared", 0.9-float64(i)*0.1) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -227,7 +227,7 @@ func TestSuggestArtists_RecencyDecayDownweightsOldPlays(t *testing.T) { seedUnmatched(t, pool, recentSeed.ID, "cand", "Cand", 0.5) seedUnmatched(t, pool, oldSeed.ID, "cand", "Cand", 0.5) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -255,7 +255,7 @@ func TestSuggestArtists_FiltersInLibraryCandidates(t *testing.T) { seedArtist(t, pool, "InLib", inLibMBID) seedUnmatched(t, pool, seed.ID, inLibMBID, "InLib", 0.9) - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -279,7 +279,7 @@ func TestSuggestArtists_FiltersAlreadyRequested(t *testing.T) { t.Fatalf("CreateLidarrRequest: %v", err) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -310,7 +310,7 @@ func TestSuggestArtists_RejectedRequestStillShown(t *testing.T) { t.Fatalf("RejectLidarrRequest: %v", err) } - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -322,7 +322,7 @@ func TestSuggestArtists_RejectedRequestStillShown(t *testing.T) { func TestSuggestArtists_EmptyForNewUser(t *testing.T) { pool := newPool(t) user := seedUser(t, pool, "newbie") - out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12) + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) if err != nil { t.Fatalf("SuggestArtists: %v", err) } @@ -330,3 +330,359 @@ func TestSuggestArtists_EmptyForNewUser(t *testing.T) { t.Errorf("len = %d, want 0 (new user has no signal)", len(out)) } } + +// --- Slice 1 (#2372): taste-profile seeding, tiering, and the skip fix --- + +func setTasteWeight(t *testing.T, pool *pgxpool.Pool, userID, artistID pgtype.UUID, weight float64) { + t.Helper() + if _, err := pool.Exec(context.Background(), + `INSERT INTO taste_profile_artists (user_id, artist_id, weight) VALUES ($1, $2, $3) + ON CONFLICT (user_id, artist_id) DO UPDATE SET weight = EXCLUDED.weight`, + userID, artistID, weight, + ); err != nil { + t.Fatalf("set taste weight: %v", err) + } +} + +func insertSkippedPlayEvent(t *testing.T, pool *pgxpool.Pool, userID, trackID pgtype.UUID, startedAt time.Time) { + t.Helper() + ctx := context.Background() + var sessionID pgtype.UUID + if err := pool.QueryRow(ctx, + `INSERT INTO play_sessions (user_id, started_at, last_event_at, client_id) + VALUES ($1, $2, $2, 'skip-test') RETURNING id`, + userID, startedAt, + ).Scan(&sessionID); err != nil { + t.Fatalf("insert play_session: %v", err) + } + if _, err := pool.Exec(ctx, + `INSERT INTO play_events (user_id, track_id, session_id, started_at, was_skipped) + VALUES ($1, $2, $3, $4, true)`, + userID, trackID, sessionID, startedAt, + ); err != nil { + t.Fatalf("insert skipped play_event: %v", err) + } +} + +// Tier 1: a taste-profile weight alone seeds a suggestion, with no like and no +// play on the seed artist. Before #2367 the profile was ignored entirely here. +func TestSuggestArtists_TasteProfileWeightSeedsTier1(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seed := seedArtist(t, pool, "Profile Seed", "") + + setTasteWeight(t, pool, user.ID, seed.ID, 4.0) + seedUnmatched(t, pool, seed.ID, "out-mbid", "Outsider", 0.9) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Fatalf("len = %d, want 1 (taste weight alone should seed)", len(out)) + } + if out[0].MBID != "out-mbid" { + t.Errorf("mbid = %q, want out-mbid", out[0].MBID) + } + if out[0].Score <= 0 { + t.Errorf("score = %v, want > 0", out[0].Score) + } +} + +// Once the profile has any positive row, tier 2 is not consulted — so an artist +// the user played but that the taste engine did not keep does NOT seed. That is +// the point of the rewrite: the taste engine decides what counts as affinity. +func TestSuggestArtists_TasteProfileSupersedesRawPlays(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + kept := seedArtist(t, pool, "Kept By Taste", "") + dropped := seedArtist(t, pool, "Dropped By Taste", "") + + setTasteWeight(t, pool, user.ID, kept.ID, 3.0) + + // `dropped` has real completed plays but no profile row. + album := seedAlbumForArtist(t, pool, dropped.ID, "Album") + track := seedTrackOnAlbum(t, pool, album.ID, dropped.ID, "Track") + insertPlayEvent(t, pool, user.ID, track.ID, time.Now().Add(-1*time.Hour)) + + seedUnmatched(t, pool, kept.ID, "kept-cand", "Kept Candidate", 0.9) + seedUnmatched(t, pool, dropped.ID, "dropped-cand", "Dropped Candidate", 0.9) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Fatalf("len = %d, want 1 (tier 2 must not run alongside tier 1)", len(out)) + } + if out[0].MBID != "kept-cand" { + t.Errorf("mbid = %q, want kept-cand", out[0].MBID) + } +} + +// A non-positive taste weight is not affinity. Guarded by a second, positive +// row so tier 1 stays active — otherwise an empty tier 1 would fall through to +// tier 2 and the assertion would pass for the wrong reason. +func TestSuggestArtists_NonPositiveTasteWeightDoesNotSeed(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + positive := seedArtist(t, pool, "Still Liked", "") + abandoned := seedArtist(t, pool, "Abandoned", "") + + setTasteWeight(t, pool, user.ID, positive.ID, 2.0) + setTasteWeight(t, pool, user.ID, abandoned.ID, -1.5) + + seedUnmatched(t, pool, positive.ID, "pos-cand", "Positive Candidate", 0.9) + seedUnmatched(t, pool, abandoned.ID, "neg-cand", "Negative Candidate", 0.9) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + for _, s := range out { + if s.MBID == "neg-cand" { + t.Fatalf("negative-weight artist seeded a suggestion: %+v", s) + } + } + if len(out) != 1 || out[0].MBID != "pos-cand" { + t.Errorf("out = %+v, want only pos-cand", out) + } +} + +// Tier 2 counts COMPLETED plays only. Previously every play_event counted, so +// skipping an artist repeatedly increased its signal and pushed more of its +// neighbours at the user (#2367 mechanism 3). +func TestSuggestArtists_SkippedPlaysDoNotSeedTier2(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + skipped := seedArtist(t, pool, "Only Skipped", "") + + album := seedAlbumForArtist(t, pool, skipped.ID, "Album") + track := seedTrackOnAlbum(t, pool, album.ID, skipped.ID, "Track") + for i := 0; i < 5; i++ { + insertSkippedPlayEvent(t, pool, user.ID, track.ID, time.Now().Add(-time.Duration(i+1)*time.Hour)) + } + seedUnmatched(t, pool, skipped.ID, "skip-cand", "Skip Candidate", 0.9) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 0 { + t.Errorf("len = %d, want 0 (skips are not affinity): %+v", len(out), out) + } +} + +// --- Slice 3 (#2374): time-boxed suggestion snooze --- +// +// The defining property under test is that a snooze EXPIRES. A permanent +// dismissal would pass most of these; only TestSuggestArtists_ExpiredSnooze +// distinguishes the two, and it is the reason this shape was approved over an +// exclusion UI (rule #101). + +// snoozeUntil inserts a snooze row with an explicit absolute expiry, so a +// test can place it in the past without depending on the duration arithmetic +// in SnoozeSuggestion. +func snoozeUntil(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID, mbid, name string, until time.Time) { + t.Helper() + if _, err := pool.Exec(context.Background(), + `INSERT INTO suggestion_snoozes (user_id, candidate_mbid, candidate_name, snoozed_until) + VALUES ($1, $2, $3, $4) + ON CONFLICT (user_id, candidate_mbid) DO UPDATE SET snoozed_until = EXCLUDED.snoozed_until`, + userID, mbid, name, until, + ); err != nil { + t.Fatalf("snoozeUntil: %v", err) + } +} + +// seedOneCandidate wires the minimum that puts exactly one candidate in the +// deck: a liked seed artist with one unmatched neighbour. +func seedOneCandidate(t *testing.T, pool *pgxpool.Pool, userID pgtype.UUID, mbid, name string) { + t.Helper() + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, userID, seed.ID) + seedUnmatched(t, pool, seed.ID, mbid, name, 0.9) +} + +func TestSuggestArtists_ActiveSnoozeHidesCandidate(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seedOneCandidate(t, pool, user.ID, "snoozed-mbid", "Parked Artist") + + if err := dbq.New(pool).SnoozeSuggestion(context.Background(), dbq.SnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: "snoozed-mbid", + CandidateName: "Parked Artist", + Column4: 90, + }); err != nil { + t.Fatalf("SnoozeSuggestion: %v", err) + } + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 0 { + t.Errorf("len = %d, want 0 (active snooze should hide the candidate): %+v", len(out), out) + } +} + +// The whole point of a snooze over a dismissal: it comes back on its own. +func TestSuggestArtists_ExpiredSnoozeShowsCandidateAgain(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seedOneCandidate(t, pool, user.ID, "expired-mbid", "Returning Artist") + snoozeUntil(t, pool, user.ID, "expired-mbid", "Returning Artist", time.Now().Add(-time.Hour)) + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Fatalf("len = %d, want 1 (an expired snooze must not hide anything): %+v", len(out), out) + } + if out[0].MBID != "expired-mbid" { + t.Errorf("mbid = %q, want expired-mbid", out[0].MBID) + } +} + +// Rule #47: one household member parking a suggestion must not remove it +// from anyone else's deck. +func TestSuggestArtists_SnoozeIsPerUser(t *testing.T) { + pool := newPool(t) + alice := seedUser(t, pool, "alice") + bob := seedUser(t, pool, "bob") + + seed := seedArtist(t, pool, "Seed", "") + likeArtist(t, pool, alice.ID, seed.ID) + likeArtist(t, pool, bob.ID, seed.ID) + seedUnmatched(t, pool, seed.ID, "shared-mbid", "Shared Candidate", 0.9) + + snoozeUntil(t, pool, alice.ID, "shared-mbid", "Shared Candidate", time.Now().Add(24*time.Hour)) + + aliceOut, err := SuggestArtists(context.Background(), pool, alice.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists(alice): %v", err) + } + if len(aliceOut) != 0 { + t.Errorf("alice len = %d, want 0 (she snoozed it)", len(aliceOut)) + } + bobOut, err := SuggestArtists(context.Background(), pool, bob.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists(bob): %v", err) + } + if len(bobOut) != 1 { + t.Errorf("bob len = %d, want 1 (alice's snooze is not his)", len(bobOut)) + } +} + +func TestUnsnoozeSuggestion_RestoresImmediately(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + seedOneCandidate(t, pool, user.ID, "undo-mbid", "Undo Artist") + snoozeUntil(t, pool, user.ID, "undo-mbid", "Undo Artist", time.Now().Add(90*24*time.Hour)) + + q := dbq.New(pool) + rows, err := q.UnsnoozeSuggestion(context.Background(), dbq.UnsnoozeSuggestionParams{ + UserID: user.ID, CandidateMbid: "undo-mbid", + }) + if err != nil { + t.Fatalf("UnsnoozeSuggestion: %v", err) + } + if rows != 1 { + t.Errorf("rows = %d, want 1", rows) + } + // A second delete affects nothing — the handler turns this into a 404 + // rather than reporting success for a no-op. + rows, err = q.UnsnoozeSuggestion(context.Background(), dbq.UnsnoozeSuggestionParams{ + UserID: user.ID, CandidateMbid: "undo-mbid", + }) + if err != nil { + t.Fatalf("UnsnoozeSuggestion (repeat): %v", err) + } + if rows != 0 { + t.Errorf("repeat rows = %d, want 0", rows) + } + + out, err := SuggestArtists(context.Background(), pool, user.ID, 30, 12, 0) + if err != nil { + t.Fatalf("SuggestArtists: %v", err) + } + if len(out) != 1 { + t.Errorf("len = %d, want 1 (unsnooze restores the candidate)", len(out)) + } +} + +// Re-snoozing must extend, not conflict on the PK. +func TestSnoozeSuggestion_UpsertExtends(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + q := dbq.New(pool) + + snoozeUntil(t, pool, user.ID, "extend-mbid", "Old Name", time.Now().Add(time.Hour)) + if err := q.SnoozeSuggestion(context.Background(), dbq.SnoozeSuggestionParams{ + UserID: user.ID, + CandidateMbid: "extend-mbid", + CandidateName: "New Name", + Column4: 90, + }); err != nil { + t.Fatalf("SnoozeSuggestion (re-snooze): %v", err) + } + + rows, err := q.ListActiveSuggestionSnoozes(context.Background(), user.ID) + if err != nil { + t.Fatalf("ListActiveSuggestionSnoozes: %v", err) + } + if len(rows) != 1 { + t.Fatalf("len = %d, want 1 (upsert, not a second row)", len(rows)) + } + if rows[0].CandidateName != "New Name" { + t.Errorf("name = %q, want New Name (upsert refreshes it)", rows[0].CandidateName) + } + if got := time.Until(rows[0].SnoozedUntil.Time); got < 80*24*time.Hour { + t.Errorf("snoozed_until is %v away, want ~90d (re-snooze should extend)", got) + } +} + +// ListActiveSuggestionSnoozes filters expired rows itself rather than +// trusting the hourly gc sweep to have run. +func TestListActiveSuggestionSnoozes_ExcludesExpired(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + snoozeUntil(t, pool, user.ID, "live-mbid", "Live", time.Now().Add(24*time.Hour)) + snoozeUntil(t, pool, user.ID, "dead-mbid", "Dead", time.Now().Add(-24*time.Hour)) + + rows, err := dbq.New(pool).ListActiveSuggestionSnoozes(context.Background(), user.ID) + if err != nil { + t.Fatalf("ListActiveSuggestionSnoozes: %v", err) + } + if len(rows) != 1 { + t.Fatalf("len = %d, want 1 (expired row must not be listed)", len(rows)) + } + if rows[0].CandidateMbid != "live-mbid" { + t.Errorf("mbid = %q, want live-mbid", rows[0].CandidateMbid) + } +} + +func TestGcDeleteExpiredSuggestionSnoozes_KeepsActive(t *testing.T) { + pool := newPool(t) + user := seedUser(t, pool, "alice") + snoozeUntil(t, pool, user.ID, "live-mbid", "Live", time.Now().Add(24*time.Hour)) + snoozeUntil(t, pool, user.ID, "dead-mbid", "Dead", time.Now().Add(-24*time.Hour)) + + q := dbq.New(pool) + deleted, err := q.GcDeleteExpiredSuggestionSnoozes(context.Background()) + if err != nil { + t.Fatalf("GcDeleteExpiredSuggestionSnoozes: %v", err) + } + if deleted != 1 { + t.Errorf("deleted = %d, want 1 (only the expired row)", deleted) + } + rows, err := q.ListActiveSuggestionSnoozes(context.Background(), user.ID) + if err != nil { + t.Fatalf("ListActiveSuggestionSnoozes: %v", err) + } + if len(rows) != 1 { + t.Errorf("len = %d, want 1 (active snooze survives the sweep)", len(rows)) + } +} diff --git a/internal/recommendation/suggestions_select_test.go b/internal/recommendation/suggestions_select_test.go new file mode 100644 index 00000000..c73ad91c --- /dev/null +++ b/internal/recommendation/suggestions_select_test.go @@ -0,0 +1,170 @@ +package recommendation + +import ( + "fmt" + "testing" + + "github.com/jackc/pgx/v5/pgtype" +) + +// selectSuggestions is deliberately pure (no DB, no clock), so the rotation and +// diversity rules from slice #2373 are covered here in the fast lane rather +// than behind the integration gate. + +func seedID(n byte) pgtype.UUID { + var u pgtype.UUID + u.Bytes[0] = n + u.Valid = true + return u +} + +// candidate builds a pool entry attributed to the given dominant seed. +func candidate(mbid string, score float64, dominant byte) ArtistSuggestion { + return ArtistSuggestion{ + MBID: mbid, + Name: mbid, + Score: score, + Attribution: []SeedContribution{ + {ArtistID: seedID(dominant), Contribution: score}, + }, + } +} + +// poolOf returns n candidates in descending score order, spread across +// `seeds` distinct dominant seeds. +func poolOf(n int, seeds int) []ArtistSuggestion { + out := make([]ArtistSuggestion, 0, n) + for i := 0; i < n; i++ { + out = append(out, candidate(fmt.Sprintf("mbid-%02d", i), 1.0-float64(i)*0.01, byte(i%seeds))) + } + return out +} + +func mbids(in []ArtistSuggestion) []string { + out := make([]string, 0, len(in)) + for _, s := range in { + out = append(out, s.MBID) + } + return out +} + +func TestSelectSuggestions_ReturnsPoolUnchangedWhenNotOverfetched(t *testing.T) { + pool := poolOf(5, 5) + got := selectSuggestions(pool, 12, "2026-08-01") + if len(got) != 5 { + t.Fatalf("len = %d, want 5 (nothing to rotate)", len(got)) + } + if got[0].MBID != "mbid-00" { + t.Errorf("first = %q, want mbid-00", got[0].MBID) + } +} + +func TestSelectSuggestions_FillsTheRequestedLimit(t *testing.T) { + got := selectSuggestions(poolOf(48, 8), 12, "2026-08-01") + if len(got) != 12 { + t.Fatalf("len = %d, want 12", len(got)) + } +} + +// The reported symptom: the deck must change day to day without the user +// requesting anything. +func TestSelectSuggestions_RotatesAcrossDays(t *testing.T) { + pool := poolOf(48, 8) + day1 := mbids(selectSuggestions(pool, 12, "2026-08-01")) + day2 := mbids(selectSuggestions(pool, 12, "2026-08-02")) + + if fmt.Sprint(day1) == fmt.Sprint(day2) { + t.Fatalf("deck identical across days: %v", day1) + } + // ...but stable WITHIN a day, or the surface would reshuffle on every + // pull-to-refresh, which reads as broken rather than fresh. + again := mbids(selectSuggestions(pool, 12, "2026-08-01")) + if fmt.Sprint(day1) != fmt.Sprint(again) { + t.Errorf("same day differed:\n %v\n %v", day1, again) + } +} + +// The strongest matches should never rotate out of sight. +func TestSelectSuggestions_HeadIsStableTopScorers(t *testing.T) { + pool := poolOf(48, 8) + for _, day := range []string{"2026-08-01", "2026-08-02", "2026-09-15"} { + got := selectSuggestions(pool, 12, day) + if got[0].MBID != "mbid-00" { + t.Errorf("day %s: first = %q, want mbid-00 (top score leads)", day, got[0].MBID) + } + } +} + +// #2367 mechanism 4: all twelve slots could be neighbours of one artist. +// +// The pool is deliberately SKEWED — one seed owns the entire top of the score +// ranking — because that is the only shape where a cap can bite. With scores +// spread evenly across seeds the top-N is already diverse and the cap is +// untestable (an earlier version of this test asserted against an even pool and +// could not fail). +func TestSelectSuggestions_CapsOneDominantSeed(t *testing.T) { + const dominantRun = 20 + pool := make([]ArtistSuggestion, 0, 48) + for i := 0; i < dominantRun; i++ { // seed 0 owns the 20 best scores + pool = append(pool, candidate(fmt.Sprintf("dom-%02d", i), 1.0-float64(i)*0.01, 0)) + } + for i := 0; i < 28; i++ { // seeds 1..9 hold everything below + pool = append(pool, candidate(fmt.Sprintf("oth-%02d", i), 0.80-float64(i)*0.01, byte(1+i%9))) + } + + got := selectSuggestions(pool, 12, "2026-08-01") + if len(got) != 12 { + t.Fatalf("len = %d, want 12", len(got)) + } + + perSeed := map[pgtype.UUID]int{} + for _, s := range got { + perSeed[s.Attribution[0].ArtistID]++ + } + // Uncapped, the top 12 by score would be 12 of seed 0's neighbours. + if n := perSeed[seedID(0)]; n > maxPerSeedFor(12) { + t.Errorf("dominant seed owns %d of 12 slots, want <= %d: %v", + n, maxPerSeedFor(12), mbids(got)) + } + if len(perSeed) < 4 { + t.Errorf("deck spans only %d seeds, want >= 4: %v", len(perSeed), mbids(got)) + } +} + +// Diversity must not starve the deck (rule #131): a pool hanging off a single +// seed should still fill, not collapse to maxPerSeed entries. +func TestSelectSuggestions_SingleSeedPoolStillFills(t *testing.T) { + got := selectSuggestions(poolOf(30, 1), 12, "2026-08-01") + if len(got) != 12 { + t.Fatalf("len = %d, want 12 (cap must not starve the deck)", len(got)) + } + if got[0].MBID != "mbid-00" { + t.Errorf("first = %q, want mbid-00", got[0].MBID) + } +} + +// A candidate with no attribution has no seed to blame, so it must never be +// held back by the diversity cap. +func TestPartitionPerSeed_UnattributedNeverCapped(t *testing.T) { + pool := []ArtistSuggestion{ + candidate("a", 0.9, 1), + candidate("b", 0.8, 1), + {MBID: "orphan", Score: 0.1}, + } + preferred, overflow := partitionPerSeed(pool, 1) + if len(preferred) != 2 || preferred[1].MBID != "orphan" { + t.Errorf("preferred = %v, want [a orphan]", mbids(preferred)) + } + if len(overflow) != 1 || overflow[0].MBID != "b" { + t.Errorf("overflow = %v, want [b]", mbids(overflow)) + } +} + +func TestPoolSizeFor_OverfetchesButIsBounded(t *testing.T) { + if got := poolSizeFor(12); got != 48 { + t.Errorf("poolSizeFor(12) = %d, want 48", got) + } + if got := poolSizeFor(50); got != suggestionPoolMax { + t.Errorf("poolSizeFor(50) = %d, want the %d ceiling", got, suggestionPoolMax) + } +} diff --git a/internal/recommendation/tagoverlap.go b/internal/recommendation/tagoverlap.go new file mode 100644 index 00000000..8e2d3bbd --- /dev/null +++ b/internal/recommendation/tagoverlap.go @@ -0,0 +1,156 @@ +// tagoverlap.go — the taste-tag term for the Discover request surface +// (#2377, milestone #268 slice 6). +// +// Slices 1-2 made the deck stop repeating; this is what makes it *relevant*. +// Before this, a candidate's only claim on a slot was "some artist you play is +// similar to it" — a graph-adjacency fact that says nothing about whether the +// music sounds like anything you actually like. Here the candidate's own +// folksonomy tags (cached by slice 5) are compared against the user's +// taste-profile tags, so the surface can rank on "matches the sound you like" +// and say WHY. +// +// Pure by design — no DB, no clock — so the scoring rules are unit-testable in +// the fast lane rather than behind the integration gate. +package recommendation + +import "sort" + +// maxMatchedTags caps the "matches: …" explanation. Three is what the existing +// seed attribution shows, and a longer list stops being a reason and becomes a +// tag dump. +const maxMatchedTags = 3 + +// TagWeights is a tag → weight map. Both sides of the comparison use it: +// candidate tags (normalized [0,1] by the enrichment providers) and the user's +// taste-profile tags (accumulated, unbounded — normalized here). +type TagWeights map[string]float64 + +// tagOverlap scores how much of a candidate's tag identity the user actually +// likes, in [0,1], and returns the matched tags ordered by contribution. +// +// The measure is: of this candidate's total tag mass, what share sits on tags +// the user likes — each weighted by how strongly they like it? +// +// overlap = Σ(shared) candWeight × normalizedTasteWeight ÷ Σ(all) candWeight +// +// Normalizing the taste side by the user's STRONGEST tag is what makes this +// comparable across users: taste weights accumulate with listening, so a +// heavy listener's raw numbers dwarf a new user's while meaning the same +// thing — "this is my favourite tag". Dividing by the candidate's own total +// mass makes it comparable across candidates, so a densely-tagged artist +// can't out-score a sparsely-tagged one just by having more tags. +// +// Returns (0, nil) when either side is empty. That is the load-bearing +// degradation path: tag coverage for out-of-library candidates is permanently +// partial (#2376), and a cold-start user has no taste tags at all. Both must +// leave the candidate's similarity score untouched rather than sink it — +// rule #131, tiered degradation, never vanish-or-nothing. +func tagOverlap(candidate, taste TagWeights) (float64, []string) { + if len(candidate) == 0 || len(taste) == 0 { + return 0, nil + } + + maxTaste := 0.0 + for _, w := range taste { + if w > maxTaste { + maxTaste = w + } + } + // Every taste weight <= 0 carries no preference to match against. Guarding + // here also avoids dividing by zero below. + if maxTaste <= 0 { + return 0, nil + } + + totalMass := 0.0 + for _, w := range candidate { + // Negative or zero candidate weights would let a tag subtract from the + // denominator and inflate the ratio past 1. + if w > 0 { + totalMass += w + } + } + if totalMass <= 0 { + return 0, nil + } + + type contribution struct { + tag string + score float64 + } + var matched []contribution + sum := 0.0 + for tag, candWeight := range candidate { + if candWeight <= 0 { + continue + } + tasteWeight, ok := taste[tag] + if !ok || tasteWeight <= 0 { + continue + } + c := candWeight * (tasteWeight / maxTaste) + sum += c + matched = append(matched, contribution{tag: tag, score: c}) + } + if len(matched) == 0 { + return 0, nil + } + + // Strongest contribution first; tag name breaks ties so the explanation is + // deterministic for a given input rather than map-iteration order. + sort.Slice(matched, func(i, j int) bool { + if matched[i].score != matched[j].score { + return matched[i].score > matched[j].score + } + return matched[i].tag < matched[j].tag + }) + names := make([]string, 0, min(len(matched), maxMatchedTags)) + for i := 0; i < len(matched) && i < maxMatchedTags; i++ { + names = append(names, matched[i].tag) + } + return sum / totalMass, names +} + +// applyTagOverlap re-scores and re-orders a candidate pool by taste-tag +// overlap, stamping the matched tags onto each suggestion for the UI. +// +// The blend is MULTIPLICATIVE — score × (1 + weight × overlap) — not additive, +// and the difference is the whole safety argument: +// +// - A candidate with no tags has overlap 0, so its score is EXACTLY +// unchanged. Partial tag coverage costs a candidate nothing. +// - Nothing can leapfrog on tags alone. An additive term with a large +// weight would let a near-zero-similarity artist outrank a strong match +// just for sharing a popular tag, which reads as noise to the user. +// - weight 0 disables the feature completely and restores pure similarity +// order, so the operator's knob has a real off position. +// +// Callers must pass the pool in similarity order; it is returned in blended +// order. Mutates the elements in place (they're the caller's own slice built +// per request), and re-sorts, because selectSuggestions downstream relies on +// score order for its head/tail split. +func applyTagOverlap( + pool []ArtistSuggestion, candidateTags map[string]TagWeights, taste TagWeights, weight float64, +) []ArtistSuggestion { + // A zero weight is the operator turning the feature off. Skip the work + // AND the re-sort so the ordering is bit-for-bit the pre-slice-6 result. + if weight == 0 || len(taste) == 0 { + return pool + } + for i := range pool { + overlap, matched := tagOverlap(candidateTags[pool[i].MBID], taste) + pool[i].MatchedTags = matched + pool[i].TagOverlap = overlap + pool[i].Score *= 1 + weight*overlap + } + sort.SliceStable(pool, func(i, j int) bool { + if pool[i].Score != pool[j].Score { + return pool[i].Score > pool[j].Score + } + // Stable tiebreak by MBID. Without it, two candidates on equal scores + // could swap between requests within the same day, which the daily + // rotation (#2373) exists to prevent. + return pool[i].MBID < pool[j].MBID + }) + return pool +} diff --git a/internal/recommendation/tagoverlap_test.go b/internal/recommendation/tagoverlap_test.go new file mode 100644 index 00000000..e66269e4 --- /dev/null +++ b/internal/recommendation/tagoverlap_test.go @@ -0,0 +1,244 @@ +package recommendation + +import ( + "fmt" + "testing" +) + +// tagOverlap / applyTagOverlap are pure, so slice 6's ranking rules are covered +// in the fast lane rather than behind the integration gate. +// +// Fixtures here are deliberately SKEWED — candidates that match the taste +// profile to clearly different degrees. An evenly-matching pool cannot exercise +// a re-ranking at all: every candidate gets the same multiplier and the order is +// unchanged whether the blend works or not. Slice 2's first diversity test had +// exactly that defect, so it's called out explicitly here. + +func TestTagOverlap_FullMatchScoresOne(t *testing.T) { + // Every unit of the candidate's tag mass sits on the user's single + // strongest tag → the whole mass matches at full strength. + got, matched := tagOverlap( + TagWeights{"shoegaze": 1.0}, + TagWeights{"shoegaze": 5.0}, + ) + if got != 1.0 { + t.Errorf("overlap = %v, want 1.0", got) + } + if len(matched) != 1 || matched[0] != "shoegaze" { + t.Errorf("matched = %v, want [shoegaze]", matched) + } +} + +func TestTagOverlap_NoSharedTagsScoresZero(t *testing.T) { + got, matched := tagOverlap( + TagWeights{"death metal": 1.0}, + TagWeights{"shoegaze": 5.0}, + ) + if got != 0 { + t.Errorf("overlap = %v, want 0", got) + } + if matched != nil { + t.Errorf("matched = %v, want nil", matched) + } +} + +// Half the candidate's mass is on a matching tag, and that tag is the user's +// strongest → 0.5. +func TestTagOverlap_PartialMassMatchIsProportional(t *testing.T) { + got, _ := tagOverlap( + TagWeights{"shoegaze": 1.0, "death metal": 1.0}, + TagWeights{"shoegaze": 5.0}, + ) + if got != 0.5 { + t.Errorf("overlap = %v, want 0.5", got) + } +} + +// Matching a tag the user barely likes must score below matching one they love. +func TestTagOverlap_WeakTasteTagScoresLowerThanStrong(t *testing.T) { + taste := TagWeights{"shoegaze": 10.0, "polka": 1.0} + strong, _ := tagOverlap(TagWeights{"shoegaze": 1.0}, taste) + weak, _ := tagOverlap(TagWeights{"polka": 1.0}, taste) + if !(strong > weak) { + t.Errorf("strong=%v weak=%v — a favourite tag must outscore a marginal one", strong, weak) + } + if weak != 0.1 { // 1.0 * (1/10) / 1.0 + t.Errorf("weak = %v, want 0.1", weak) + } +} + +// THE degradation path. Tag coverage for out-of-library candidates is +// permanently partial (#2376) and cold-start users have no taste tags, so both +// must be scored 0 — never negative, never dropped (rule #131). +func TestTagOverlap_EmptyEitherSideIsZeroNotNegative(t *testing.T) { + cases := []struct { + name string + candidate, taste TagWeights + }{ + {"no candidate tags", nil, TagWeights{"shoegaze": 5}}, + {"no taste tags", TagWeights{"shoegaze": 1}, nil}, + {"both empty", nil, nil}, + {"taste weights all zero", TagWeights{"shoegaze": 1}, TagWeights{"shoegaze": 0}}, + {"candidate weights all zero", TagWeights{"shoegaze": 0}, TagWeights{"shoegaze": 5}}, + } + for _, c := range cases { + got, matched := tagOverlap(c.candidate, c.taste) + if got != 0 { + t.Errorf("%s: overlap = %v, want 0", c.name, got) + } + if matched != nil { + t.Errorf("%s: matched = %v, want nil", c.name, matched) + } + } +} + +// A densely-tagged artist must not out-score a focused one merely by having +// more tags — that's what dividing by the candidate's own mass buys. +func TestTagOverlap_IsNotAPopularityContest(t *testing.T) { + taste := TagWeights{"shoegaze": 5.0} + focused, _ := tagOverlap(TagWeights{"shoegaze": 1.0}, taste) + sprawling, _ := tagOverlap(TagWeights{ + "shoegaze": 1.0, "rock": 1.0, "alternative": 1.0, "90s": 1.0, + }, taste) + if !(focused > sprawling) { + t.Errorf("focused=%v sprawling=%v — extra unmatched tags must dilute, not add", + focused, sprawling) + } +} + +// Taste weights accumulate with listening, so raw magnitudes differ wildly +// between a new user and a heavy one while meaning the same thing. Normalizing +// by the user's own strongest tag is what makes the score comparable. +func TestTagOverlap_IsInvariantToTasteMagnitude(t *testing.T) { + candidate := TagWeights{"shoegaze": 1.0, "dream pop": 1.0} + newUser, _ := tagOverlap(candidate, TagWeights{"shoegaze": 2.0, "polka": 1.0}) + heavyUser, _ := tagOverlap(candidate, TagWeights{"shoegaze": 2000.0, "polka": 1000.0}) + if newUser != heavyUser { + t.Errorf("newUser=%v heavyUser=%v — scaling all taste weights must not change the result", + newUser, heavyUser) + } +} + +func TestTagOverlap_MatchedTagsAreOrderedAndCapped(t *testing.T) { + got, matched := tagOverlap( + TagWeights{"a": 0.2, "b": 0.9, "c": 0.5, "d": 0.7}, + TagWeights{"a": 10, "b": 10, "c": 10, "d": 10}, + ) + if got <= 0 { + t.Fatalf("overlap = %v, want > 0", got) + } + // All taste weights equal, so candidate weight decides: b(.9) d(.7) c(.5). + want := []string{"b", "d", "c"} + if fmt.Sprint(matched) != fmt.Sprint(want) { + t.Errorf("matched = %v, want %v (strongest first, capped at %d)", + matched, want, maxMatchedTags) + } +} + +// --- applyTagOverlap --- + +func poolFor(specs ...struct { + mbid string + score float64 +}) []ArtistSuggestion { + out := make([]ArtistSuggestion, 0, len(specs)) + for _, s := range specs { + out = append(out, ArtistSuggestion{MBID: s.mbid, Name: s.mbid, Score: s.score}) + } + return out +} + +type spec = struct { + mbid string + score float64 +} + +// The payoff, and the reason the fixture is skewed: a taste-matching candidate +// that started BELOW another must be able to overtake it. With an evenly +// matching pool this assertion could not fail. +func TestApplyTagOverlap_TasteMatchOvertakesAStrongerNonMatch(t *testing.T) { + pool := poolFor(spec{"loud", 1.0}, spec{"match", 0.7}) + candTags := map[string]TagWeights{ + "loud": {"death metal": 1.0}, + "match": {"shoegaze": 1.0}, + } + taste := TagWeights{"shoegaze": 5.0} + + got := applyTagOverlap(pool, candTags, taste, 1.0) + if got[0].MBID != "match" { + t.Errorf("first = %q, want match (0.7×2 = 1.4 beats 1.0×1)", got[0].MBID) + } + if got[0].MatchedTags == nil { + t.Error("matched tags not stamped onto the winner") + } +} + +// An untagged candidate keeps its score EXACTLY. This is the multiplicative +// blend's whole safety argument: partial coverage costs a candidate nothing. +func TestApplyTagOverlap_UntaggedCandidateScoreIsUnchanged(t *testing.T) { + pool := poolFor(spec{"untagged", 0.9}) + got := applyTagOverlap(pool, map[string]TagWeights{}, TagWeights{"shoegaze": 5}, 1.0) + if got[0].Score != 0.9 { + t.Errorf("score = %v, want 0.9 exactly (no tags must not penalise)", got[0].Score) + } + if got[0].MatchedTags != nil { + t.Errorf("matched = %v, want nil", got[0].MatchedTags) + } +} + +// Weight 0 is the operator's off switch: the ordering must be bit-for-bit the +// pre-slice-6 result, not merely similar. +func TestApplyTagOverlap_ZeroWeightIsAnExactNoOp(t *testing.T) { + pool := poolFor(spec{"a", 1.0}, spec{"b", 0.7}) + candTags := map[string]TagWeights{"b": {"shoegaze": 1.0}} + got := applyTagOverlap(pool, candTags, TagWeights{"shoegaze": 5.0}, 0) + if got[0].MBID != "a" || got[0].Score != 1.0 || got[1].Score != 0.7 { + t.Errorf("weight 0 changed the pool: %+v", got) + } + // And it must not stamp matched tags either — the UI would otherwise + // explain a boost that never happened. + if got[1].MatchedTags != nil { + t.Errorf("matched tags stamped while the feature is off: %v", got[1].MatchedTags) + } +} + +// A user with no taste profile is the cold-start case: every candidate scores +// the same multiplier of 1, so similarity order must survive intact. +func TestApplyTagOverlap_NoTasteProfileLeavesOrderIntact(t *testing.T) { + pool := poolFor(spec{"a", 1.0}, spec{"b", 0.7}, spec{"c", 0.4}) + candTags := map[string]TagWeights{"c": {"shoegaze": 1.0}} + got := applyTagOverlap(pool, candTags, nil, 1.0) + for i, want := range []string{"a", "b", "c"} { + if got[i].MBID != want { + t.Fatalf("order = %v..., want a b c", got[i].MBID) + } + } +} + +// Bounded boost: even a perfect match at the maximum weight can only multiply +// by (1 + w), so a candidate cannot be catapulted arbitrarily far. +func TestApplyTagOverlap_BoostIsBoundedByOnePlusWeight(t *testing.T) { + pool := poolFor(spec{"perfect", 1.0}) + got := applyTagOverlap(pool, + map[string]TagWeights{"perfect": {"shoegaze": 1.0}}, + TagWeights{"shoegaze": 5.0}, 2.0) + if got[0].Score != 3.0 { + t.Errorf("score = %v, want 3.0 (1.0 × (1 + 2×1))", got[0].Score) + } + if got[0].TagOverlap != 1.0 { + t.Errorf("TagOverlap = %v, want 1.0", got[0].TagOverlap) + } +} + +// Equal blended scores must resolve deterministically, or two candidates could +// swap between requests inside one day — the exact churn the daily rotation +// (#2373) exists to prevent. +func TestApplyTagOverlap_TiesBreakDeterministically(t *testing.T) { + first := applyTagOverlap(poolFor(spec{"zzz", 1.0}, spec{"aaa", 1.0}), + map[string]TagWeights{}, TagWeights{"x": 1}, 1.0) + second := applyTagOverlap(poolFor(spec{"aaa", 1.0}, spec{"zzz", 1.0}), + map[string]TagWeights{}, TagWeights{"x": 1}, 1.0) + if first[0].MBID != "aaa" || second[0].MBID != "aaa" { + t.Errorf("tie order not deterministic: %q then %q", first[0].MBID, second[0].MBID) + } +} diff --git a/internal/recsettings/patch.go b/internal/recsettings/patch.go index 6ae86833..f7139cdf 100644 --- a/internal/recsettings/patch.go +++ b/internal/recsettings/patch.go @@ -161,6 +161,70 @@ func applyTastePatch(current TasteTuning, patch map[string]float64) (TasteTuning return next, changes, nil } +// Discover tuning bounds. +const ( + // A tag-overlap weight above this stops being a boost and becomes the + // ranking — at 10, a perfect match multiplies similarity by 11, which lets + // tag agreement swamp the similarity signal entirely. The bound is for + // typos, not to constrain exploration; the multiplicative blend keeps even + // the maximum from reordering an untagged candidate. + tagOverlapWeightMax = 10.0 + // Snooze duration: at least a day (anything less isn't a snooze, it's a + // flicker), at most a year — past that it's a permanent dismissal wearing a + // snooze's clothes, which is exactly the shape rule #101 rules out. + snoozeDaysMin = 1.0 + snoozeDaysMax = 365.0 +) + +// applyDiscoverPatch validates and applies a partial Discover update. +func applyDiscoverPatch( + current DiscoverTuning, patch map[string]float64, +) (DiscoverTuning, []fieldChange, error) { + next := current + var changes []fieldChange + for field, v := range patch { + var target *float64 + switch field { + case "tag_overlap_weight": + if v < 0 || v > tagOverlapWeightMax { + return current, nil, fmt.Errorf("%w: %s = %v (must be in [0, %v])", + ErrOutOfRange, field, v, tagOverlapWeightMax) + } + target = &next.TagOverlapWeight + case "snooze_days": + if v < snoozeDaysMin || v > snoozeDaysMax { + return current, nil, fmt.Errorf("%w: %s = %v (must be in [%v, %v])", + ErrOutOfRange, field, v, snoozeDaysMin, snoozeDaysMax) + } + target = &next.SnoozeDays + default: + return current, nil, fmt.Errorf("%w: %q", ErrUnknownField, field) + } + if *target == v { + continue + } + changes = append(changes, fieldChange{Field: field, Old: *target, New: v}) + *target = v + } + return next, changes, nil +} + +// diffDiscover returns per-field changes from a to b (empty when equal). +func diffDiscover(a, b DiscoverTuning) []fieldChange { + var out []fieldChange + if a.TagOverlapWeight != b.TagOverlapWeight { + out = append(out, fieldChange{ + Field: "tag_overlap_weight", Old: a.TagOverlapWeight, New: b.TagOverlapWeight, + }) + } + if a.SnoozeDays != b.SnoozeDays { + out = append(out, fieldChange{ + Field: "snooze_days", Old: a.SnoozeDays, New: b.SnoozeDays, + }) + } + return out +} + // diffWeights returns per-field changes from a to b (empty when equal). func diffWeights(a, b recommendation.ScoringWeights) []fieldChange { var out []fieldChange diff --git a/internal/recsettings/service.go b/internal/recsettings/service.go index 22640847..29b41d14 100644 --- a/internal/recsettings/service.go +++ b/internal/recsettings/service.go @@ -35,6 +35,11 @@ const ( ScopeRadio = "radio" ScopeDailyMix = "daily_mix" ScopeTaste = "taste" + // ScopeDiscover is the Discover request surface (#2377). Its own scope + // rather than columns on taste: SnoozeDays lives here, and a snooze must + // never be read as taste signal (#2374) — filing it under taste would put + // it one careless join from the leak that design forbids. + ScopeDiscover = "discover" ) // TasteTuning is the tunable subset of taste.Config: the engagement @@ -87,6 +92,33 @@ func ShippedDailyMixWeights() recommendation.ScoringWeights { } } +// DiscoverTuning is the tunable set for the Discover request surface (#2377). +type DiscoverTuning struct { + // TagOverlapWeight scales the taste-tag term: score × (1 + w × overlap). + // 0 disables it and restores pure similarity ranking. + TagOverlapWeight float64 + // SnoozeDays is the default "not right now" duration (#2374). + SnoozeDays float64 +} + +// ShippedDiscoverTuning are the shipped Discover defaults. +// +// TagOverlapWeight 1.0 lets a perfect tag match at most double a candidate's +// similarity score — enough to reorder the deck meaningfully, not enough for a +// popular-tag coincidence to beat a genuinely strong similarity match. It is a +// starting point for the tuning lab, not a tuned value: the honest way to pick +// it is the metrics trend view after some real use. +// +// SnoozeDays 90 matches the operator's approved shape: long enough that a +// parked suggestion stops nagging, short enough that a taste shift brings it +// back on its own. +func ShippedDiscoverTuning() DiscoverTuning { + return DiscoverTuning{ + TagOverlapWeight: 1.0, + SnoozeDays: 90, + } +} + // ShippedTasteTuning mirrors taste.DefaultConfig's tunable subset. func ShippedTasteTuning() TasteTuning { d := taste.DefaultConfig() @@ -110,6 +142,7 @@ type Service struct { mu sync.RWMutex profiles map[string]recommendation.ScoringWeights taste TasteTuning + discover DiscoverTuning } // New boots the service: seeds shipped defaults for missing rows, @@ -151,6 +184,13 @@ func (s *Service) reconcile(ctx context.Context) error { }); err != nil { return fmt.Errorf("seed taste tuning: %w", err) } + sd := ShippedDiscoverTuning() + if err := q.UpsertDiscoverTuningDefaults(ctx, dbq.UpsertDiscoverTuningDefaultsParams{ + TagOverlapWeight: sd.TagOverlapWeight, + SnoozeDays: sd.SnoozeDays, + }); err != nil { + return fmt.Errorf("seed discover tuning: %w", err) + } rows, err := q.ListWeightProfiles(ctx) if err != nil { @@ -160,6 +200,10 @@ func (s *Service) reconcile(ctx context.Context) error { if err != nil { return fmt.Errorf("get taste tuning: %w", err) } + dt, err := q.GetDiscoverTuning(ctx) + if err != nil { + return fmt.Errorf("get discover tuning: %w", err) + } s.mu.Lock() s.profiles = map[string]recommendation.ScoringWeights{} @@ -175,6 +219,10 @@ func (s *Service) reconcile(ctx context.Context) error { EraScale: tt.EraScale, MoodScale: tt.MoodScale, } + s.discover = DiscoverTuning{ + TagOverlapWeight: dt.TagOverlapWeight, + SnoozeDays: dt.SnoozeDays, + } s.mu.Unlock() s.push() @@ -208,6 +256,15 @@ func (s *Service) Taste() TasteTuning { return s.taste } +// Discover returns the cached Discover-tuning values. Read per request by the +// suggestions handler, so an admin change takes effect on the next refresh +// with no restart (rule #25). +func (s *Service) Discover() DiscoverTuning { + s.mu.RLock() + defer s.mu.RUnlock() + return s.discover +} + // TasteConfig assembles the full taste.Config the profile builder // consumes: shipped non-tunable knobs (like bonuses, floors, caps) // plus the tuned half-life and curve. WindowDays scales with the @@ -267,6 +324,19 @@ func (s *Service) UpdateTaste(ctx context.Context, patch map[string]float64) err return s.persistTaste(ctx, next, "update", changes) } +// UpdateDiscover applies a partial update to the Discover tuning singleton. +func (s *Service) UpdateDiscover(ctx context.Context, patch map[string]float64) error { + current := s.Discover() + next, changes, err := applyDiscoverPatch(current, patch) + if err != nil { + return err + } + if len(changes) == 0 { + return nil + } + return s.persistDiscover(ctx, next, "update", changes) +} + // Reset restores a scope to its shipped defaults, with one audit row // carrying the full diff. A scope already at defaults is a no-op. func (s *Service) Reset(ctx context.Context, scope string) error { @@ -288,6 +358,13 @@ func (s *Service) Reset(ctx context.Context, scope string) error { return nil } return s.persistTaste(ctx, shipped, "reset", changes) + case ScopeDiscover: + shipped := ShippedDiscoverTuning() + changes := diffDiscover(s.Discover(), shipped) + if len(changes) == 0 { + return nil + } + return s.persistDiscover(ctx, shipped, "reset", changes) default: return fmt.Errorf("%w: %q", ErrUnknownScope, scope) } @@ -338,6 +415,28 @@ func (s *Service) persistTaste( return nil } +// persistDiscover writes the discover row + audit entry and refreshes the +// cache. No push(): unlike taste and daily_mix, nothing precomputes from these +// — the suggestions handler reads Discover() per request. +func (s *Service) persistDiscover( + ctx context.Context, d DiscoverTuning, action string, changes []fieldChange, +) error { + q := dbq.New(s.pool) + if _, err := q.UpdateDiscoverTuning(ctx, dbq.UpdateDiscoverTuningParams{ + TagOverlapWeight: d.TagOverlapWeight, + SnoozeDays: d.SnoozeDays, + }); err != nil { + return fmt.Errorf("update discover tuning: %w", err) + } + if err := s.audit(ctx, q, ScopeDiscover, action, changes); err != nil { + return err + } + s.mu.Lock() + s.discover = d + s.mu.Unlock() + return nil +} + // audit writes one recommendation_tuning_audit row. Changes are // sorted by field so rows are deterministic and diff-friendly. func (s *Service) audit( diff --git a/internal/recsettings/service_test.go b/internal/recsettings/service_test.go index cf96f8a3..083a38bd 100644 --- a/internal/recsettings/service_test.go +++ b/internal/recsettings/service_test.go @@ -281,3 +281,118 @@ func TestUpdate_NoOpWritesNoAudit(t *testing.T) { t.Errorf("no-op update wrote %d audit rows, want 0", len(rows)) } } + +// --- Discover scope (#2377, milestone #268 slice 6) --- + +func TestNew_SeedsDiscoverDefaults(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + got := s.Discover() + want := ShippedDiscoverTuning() + if got != want { + t.Errorf("Discover() = %+v, want shipped %+v", got, want) + } +} + +func TestUpdateDiscover_PersistsAndAudits(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + if err := s.UpdateDiscover(context.Background(), map[string]float64{ + "tag_overlap_weight": 2.5, + "snooze_days": 30, + }); err != nil { + t.Fatalf("UpdateDiscover: %v", err) + } + if got := s.Discover().TagOverlapWeight; got != 2.5 { + t.Errorf("TagOverlapWeight = %v, want 2.5", got) + } + if got := s.Discover().SnoozeDays; got != 30 { + t.Errorf("SnoozeDays = %v, want 30", got) + } + + // The audit row must land under the new scope. This is the assertion that + // would have caught a missing rule-#36 CHECK migration: without expanding + // recommendation_tuning_audit's whitelist, this INSERT fails at runtime. + rows := auditRows(t, pool) + if len(rows) != 1 { + t.Fatalf("audit rows = %d, want 1", len(rows)) + } + if rows[0].Scope != ScopeDiscover { + t.Errorf("audit scope = %q, want %q", rows[0].Scope, ScopeDiscover) + } + if len(rows[0].Changes) != 2 { + t.Errorf("audit changes = %+v, want both fields", rows[0].Changes) + } + + // Reload from the DB to prove it persisted rather than only caching. + s2 := newService(t, pool) + if got := s2.Discover().TagOverlapWeight; got != 2.5 { + t.Errorf("after reload TagOverlapWeight = %v, want 2.5 (not re-seeded to shipped)", got) + } +} + +func TestUpdateDiscover_Validation(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + cases := []struct { + name string + patch map[string]float64 + }{ + {"unknown field", map[string]float64{"nope": 1}}, + {"negative weight", map[string]float64{"tag_overlap_weight": -1}}, + {"weight past the typo bound", map[string]float64{"tag_overlap_weight": 100}}, + // A sub-day snooze isn't a snooze, it's a flicker. + {"snooze under a day", map[string]float64{"snooze_days": 0.5}}, + // Past a year it's a permanent dismissal wearing a snooze's clothes — + // the shape rule #101 rules out. + {"snooze past a year", map[string]float64{"snooze_days": 400}}, + } + for _, c := range cases { + if err := s.UpdateDiscover(context.Background(), c.patch); err == nil { + t.Errorf("%s: expected rejection, got nil", c.name) + } + } + if got := s.Discover(); got != ShippedDiscoverTuning() { + t.Errorf("a rejected patch mutated state: %+v", got) + } + if rows := auditRows(t, pool); len(rows) != 0 { + t.Errorf("rejected patches wrote %d audit rows, want 0", len(rows)) + } +} + +// Weight 0 must be accepted — it's the operator's off switch for the whole +// tag term, so a "must be positive" bound would remove their ability to +// disable the feature. +func TestUpdateDiscover_ZeroWeightIsAllowed(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + if err := s.UpdateDiscover(context.Background(), + map[string]float64{"tag_overlap_weight": 0}); err != nil { + t.Fatalf("UpdateDiscover(0): %v", err) + } + if got := s.Discover().TagOverlapWeight; got != 0 { + t.Errorf("TagOverlapWeight = %v, want 0", got) + } +} + +func TestResetDiscover_RestoresShippedDefaults(t *testing.T) { + pool := newPool(t) + s := newService(t, pool) + if err := s.UpdateDiscover(context.Background(), + map[string]float64{"tag_overlap_weight": 4}); err != nil { + t.Fatalf("UpdateDiscover: %v", err) + } + if err := s.Reset(context.Background(), ScopeDiscover); err != nil { + t.Fatalf("Reset: %v", err) + } + if got := s.Discover(); got != ShippedDiscoverTuning() { + t.Errorf("after reset = %+v, want shipped %+v", got, ShippedDiscoverTuning()) + } + // Already-at-defaults is a no-op: update + reset = 2 rows, not 3. + if err := s.Reset(context.Background(), ScopeDiscover); err != nil { + t.Fatalf("second Reset: %v", err) + } + if rows := auditRows(t, pool); len(rows) != 2 { + t.Errorf("audit rows = %d, want 2 (the no-op reset must not audit)", len(rows)) + } +} diff --git a/internal/tags/enricher.go b/internal/tags/enricher.go index 60b9909f..b41ac4cb 100644 --- a/internal/tags/enricher.go +++ b/internal/tags/enricher.go @@ -61,40 +61,25 @@ func NewEnricher(pool *pgxpool.Pool, logger *slog.Logger, settings *SettingsServ // enabled) the row settles to 'none'. If a provider fails transiently and // no tags surfaced, the row is left NULL for a next-pass retry. func (e *Enricher) EnrichTrack(ctx context.Context, trackID pgtype.UUID, ref TrackRef) (outcome, error) { - merged := map[string]float64{} - contributors := map[string]bool{} - anyTransient := false - - for _, provider := range e.settings.EnabledTrackTagProviders() { - tags, perr := provider.FetchTrackTags(ctx, ref) - switch { - case perr == nil: - for _, t := range tags { - if t.Weight > merged[t.Name] { - merged[t.Name] = t.Weight - } - } - if len(tags) > 0 { - contributors[provider.ID()] = true - } - case errors.Is(perr, ErrNotFound): - // Clean "no data from this source" — try the next provider. - default: - anyTransient = true - e.logger.Warn("tags: provider fetch failed; continuing", - "track_id", uuidString(trackID), "provider", provider.ID(), "err", perr) - } + providers := e.settings.EnabledTrackTagProviders() + calls := make([]tagFetch, 0, len(providers)) + for _, provider := range providers { + calls = append(calls, tagFetch{ + providerID: provider.ID(), + fetch: func(c context.Context) ([]Tag, error) { return provider.FetchTrackTags(c, ref) }, + }) } + res := e.runChain(ctx, calls, "track_id", uuidString(trackID)) - if len(merged) > 0 { - top := topKByWeight(merged, e.topK) - source := sourceLabel(contributorIDs(contributors)) + if len(res.merged) > 0 { + top := topKByWeight(res.merged, e.topK) + source := sourceLabel(res.contributors) if err := e.writeTags(ctx, trackID, top, source, e.settings.CurrentVersion()); err != nil { return outcomeLeftNull, err } return outcomeEnriched, nil } - if anyTransient { + if res.anyTransient { // Nothing landed but a source may recover — leave NULL for retry. return outcomeLeftNull, nil } @@ -106,6 +91,65 @@ func (e *Enricher) EnrichTrack(ctx context.Context, trackID pgtype.UUID, ref Tra return outcomeNone, nil } +// tagFetch pairs a provider ID with a bound fetch call, so the merge-and- +// classify loop below is shared between the track chain and the candidate- +// artist chain (#2376) instead of being written twice with one word changed. +type tagFetch struct { + providerID string + fetch func(context.Context) ([]Tag, error) +} + +// chainResult is what running a provider chain produced. contributors is the +// sorted set of provider IDs that actually returned tags — the input to +// sourceLabel. +type chainResult struct { + merged map[string]float64 + contributors []string + anyTransient bool +} + +// runChain queries every provider in order and UNIONS the results (max weight +// wins on overlap), unlike coverart's first-success-wins. +// +// A clean ErrNotFound means "this source has nothing" and moves to the next. +// Anything else is transient and recorded, so the caller can leave the row +// eligible for a retry rather than wrongly settling it as "nothing exists" — +// the distinction between those two is the whole point of the return value. +// +// logKey/logVal identify the subject in warnings (a track id or a candidate +// MBID), since this is shared across entity types. +func (e *Enricher) runChain(ctx context.Context, calls []tagFetch, logKey, logVal string) chainResult { + merged := map[string]float64{} + contributors := map[string]bool{} + anyTransient := false + + for _, c := range calls { + tags, perr := c.fetch(ctx) + switch { + case perr == nil: + for _, t := range tags { + if t.Weight > merged[t.Name] { + merged[t.Name] = t.Weight + } + } + if len(tags) > 0 { + contributors[c.providerID] = true + } + case errors.Is(perr, ErrNotFound): + // Clean "no data from this source" — try the next provider. + default: + anyTransient = true + e.logger.Warn("tags: provider fetch failed; continuing", + logKey, logVal, "provider", c.providerID, "err", perr) + } + } + return chainResult{ + merged: merged, + contributors: contributorIDs(contributors), + anyTransient: anyTransient, + } +} + // writeTags atomically replaces a track's cached tags and stamps the source // + version. tags may be empty (the 'none' settle path), which just clears // any prior tags and records the outcome. @@ -196,6 +240,146 @@ func (e *Enricher) EnrichTrackBatch(ctx context.Context, limit int, return processed, enriched, settledNone + leftNull + errored, nil } +// EnrichCandidateArtist runs the artist-tag chain for one out-of-library +// Discover candidate and caches the merged result (#2376). +// +// Mirrors EnrichTrack, with one deliberate difference in the transient case: +// there is no row to "leave NULL", because eligibility is the ABSENCE of a +// candidate_artist_tag_state row. So a transient failure writes nothing at all, +// which leaves the candidate eligible for the next tick. Writing a state row +// here would settle a candidate whose tags we simply failed to fetch. +func (e *Enricher) EnrichCandidateArtist(ctx context.Context, mbid, name string) (outcome, error) { + providers := e.settings.EnabledArtistTagProviders() + ref := ArtistRef{MBID: mbid, Name: name} + calls := make([]tagFetch, 0, len(providers)) + for _, provider := range providers { + calls = append(calls, tagFetch{ + providerID: provider.ID(), + fetch: func(c context.Context) ([]Tag, error) { return provider.FetchArtistTags(c, ref) }, + }) + } + res := e.runChain(ctx, calls, "candidate_mbid", mbid) + version := e.settings.CurrentVersion() + + if len(res.merged) > 0 { + top := topKByWeight(res.merged, e.topK) + if err := e.writeCandidateTags(ctx, mbid, top, sourceLabel(res.contributors), version); err != nil { + return outcomeLeftNull, err + } + return outcomeEnriched, nil + } + if res.anyTransient { + return outcomeLeftNull, nil + } + if err := e.writeCandidateTags(ctx, mbid, nil, sourceNone, version); err != nil { + return outcomeNone, err + } + return outcomeNone, nil +} + +// writeCandidateTags atomically replaces a candidate's cached tags and stamps +// its state. tags may be empty (the 'none' settle path), which clears any prior +// tags and records that the providers had nothing. +func (e *Enricher) writeCandidateTags( + ctx context.Context, mbid string, tags []Tag, source string, version int32, +) error { + tx, err := e.pool.Begin(ctx) + if err != nil { + return fmt.Errorf("begin tx: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + q := dbq.New(tx) + if err := q.DeleteCandidateArtistTags(ctx, mbid); err != nil { + return fmt.Errorf("delete candidate artist tags: %w", err) + } + for _, t := range tags { + if err := q.InsertCandidateArtistTag(ctx, dbq.InsertCandidateArtistTagParams{ + CandidateMbid: mbid, Tag: t.Name, Weight: t.Weight, + }); err != nil { + return fmt.Errorf("insert candidate artist tag: %w", err) + } + } + if err := q.SetCandidateArtistTagState(ctx, dbq.SetCandidateArtistTagStateParams{ + CandidateMbid: mbid, + TagSource: source, + TagSourcesVersion: version, + }); err != nil { + return fmt.Errorf("set candidate artist tag state: %w", err) + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("commit: %w", err) + } + return nil +} + +// EnrichCandidateArtistBatch drains up to limit out-of-library candidates and +// enriches each serially. Same limit semantics as EnrichTrackBatch: 0 = +// disabled, >0 = bounded, <0 = unbounded. +// +// The candidate pool is far larger than the track pool (every library artist's +// neighbours) and can never be drained in one pass at MusicBrainz's ~1 req/s, +// so the query hands them back in descending similarity order — see +// ListCandidateArtistsMissingTags. A bounded batch here is therefore normal +// operation, not a degraded mode. +func (e *Enricher) EnrichCandidateArtistBatch(ctx context.Context, limit int) ( + processed, succeeded, failed int, err error, +) { + if limit == 0 { + return 0, 0, 0, nil + } + queryLimit := int32(limit) + if limit < 0 { + queryLimit = 1<<31 - 1 + } + q := dbq.New(e.pool) + rows, qerr := q.ListCandidateArtistsMissingTags(ctx, dbq.ListCandidateArtistsMissingTagsParams{ + TagSourcesVersion: e.settings.CurrentVersion(), + Limit: queryLimit, + }) + if qerr != nil { + return 0, 0, 0, fmt.Errorf("list candidate artists missing tags: %w", qerr) + } + + var enriched, settledNone, leftNull, errored int + for _, r := range rows { + if ctx.Err() != nil { + e.logCandidateBatchSummary(len(rows), processed, enriched, settledNone, leftNull, errored) + return processed, enriched, settledNone + leftNull + errored, ctx.Err() + } + processed++ + oc, eerr := e.EnrichCandidateArtist(ctx, r.CandidateMbid, r.CandidateName) + if eerr != nil { + e.logger.Warn("tags: candidate batch entry failed", + "candidate_mbid", r.CandidateMbid, "err", eerr) + errored++ + continue + } + switch oc { + case outcomeEnriched: + enriched++ + case outcomeNone: + settledNone++ + case outcomeLeftNull: + leftNull++ + } + } + e.logCandidateBatchSummary(len(rows), processed, enriched, settledNone, leftNull, errored) + return processed, enriched, settledNone + leftNull + errored, nil +} + +// logCandidateBatchSummary mirrors logBatchSummary. `settled_none` is the +// honest-ceiling counter for this surface: candidates whose MBID has no +// upstream tags at all, which no amount of retrying will fix. +func (e *Enricher) logCandidateBatchSummary(eligible, processed, enriched, settledNone, leftNull, errored int) { + e.logger.Info("tags: candidate-artist enrichment batch complete", + "eligible", eligible, + "processed", processed, + "enriched", enriched, + "settled_none", settledNone, + "left_null", leftNull, + "errored", errored) +} + // logBatchSummary emits one Info line with the category breakdown — the // enriched/settled/left-null split is the operator's diagnostic for a // "0 enriched" symptom the collapsed tally can't explain. diff --git a/internal/tags/enricher_chain_test.go b/internal/tags/enricher_chain_test.go new file mode 100644 index 00000000..23b73fb4 --- /dev/null +++ b/internal/tags/enricher_chain_test.go @@ -0,0 +1,142 @@ +package tags + +import ( + "context" + "errors" + "io" + "log/slog" + "testing" +) + +// runChain is the merge-and-classify loop shared by the track and +// candidate-artist drains (#2376). It touches no DB, so it is testable with a +// bare Enricher. +// +// The classification is the part that matters: "every source cleanly had +// nothing" and "a source failed" lead to opposite persistence decisions +// (settle vs. leave eligible for retry), and conflating them either writes off +// an artist over a transient blip or re-fetches a genuinely untagged one +// forever. + +func chainEnricher() *Enricher { + return &Enricher{logger: slog.New(slog.NewTextHandler(io.Discard, nil))} +} + +func fixedFetch(tags []Tag, err error) func(context.Context) ([]Tag, error) { + return func(context.Context) ([]Tag, error) { return tags, err } +} + +func TestRunChain_UnionsAcrossProvidersMaxWeightWins(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "musicbrainz", fetch: fixedFetch([]Tag{ + {Name: "shoegaze", Weight: 0.4}, + {Name: "noise", Weight: 0.9}, + }, nil)}, + {providerID: "lastfm", fetch: fixedFetch([]Tag{ + {Name: "shoegaze", Weight: 0.8}, // higher — should win + {Name: "dream pop", Weight: 0.3}, + }, nil)}, + }, "subject", "x") + + if got := res.merged["shoegaze"]; got != 0.8 { + t.Errorf("shoegaze = %v, want 0.8 (max across providers)", got) + } + if got := res.merged["noise"]; got != 0.9 { + t.Errorf("noise = %v, want 0.9", got) + } + if got := res.merged["dream pop"]; got != 0.3 { + t.Errorf("dream pop = %v, want 0.3", got) + } + if len(res.merged) != 3 { + t.Errorf("merged has %d tags, want 3: %v", len(res.merged), res.merged) + } + if res.anyTransient { + t.Error("anyTransient set with no failures") + } + if len(res.contributors) != 2 { + t.Errorf("contributors = %v, want both providers", res.contributors) + } +} + +// A lower weight arriving second must not overwrite a higher one — the +// ordering of the chain must not change the result. +func TestRunChain_LowerWeightSecondDoesNotClobber(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch([]Tag{{Name: "rock", Weight: 0.9}}, nil)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "rock", Weight: 0.2}}, nil)}, + }, "subject", "x") + if got := res.merged["rock"]; got != 0.9 { + t.Errorf("rock = %v, want 0.9", got) + } +} + +func TestRunChain_NotFoundIsSkippedNotTransient(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, ErrNotFound)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "folk", Weight: 1}}, nil)}, + }, "subject", "x") + + if res.anyTransient { + t.Error("ErrNotFound must not be treated as transient — it would keep a settled subject eligible forever") + } + if len(res.contributors) != 1 || res.contributors[0] != "b" { + t.Errorf("contributors = %v, want [b] only (a returned nothing)", res.contributors) + } +} + +func TestRunChain_TransientIsRecordedEvenWhenAnotherProviderSucceeds(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, ErrTransient)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "folk", Weight: 1}}, nil)}, + }, "subject", "x") + + if !res.anyTransient { + t.Error("anyTransient should be set — 'a' may have had tags we never saw") + } + // Tags DID land, so the caller writes them; anyTransient only decides the + // no-tags case. Asserting both here pins that they're independent. + if len(res.merged) != 1 { + t.Errorf("merged = %v, want folk", res.merged) + } +} + +// An unexpected error type is transient, not terminal. Defaulting the other way +// would settle a subject on any bug in a provider. +func TestRunChain_UnknownErrorIsTransient(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, errors.New("boom"))}, + }, "subject", "x") + if !res.anyTransient { + t.Error("unknown error should count as transient") + } + if len(res.merged) != 0 { + t.Errorf("merged = %v, want empty", res.merged) + } +} + +// A provider returning (empty, nil) is not a contributor: sourceLabel would +// otherwise stamp its ID onto a subject it gave nothing to. +func TestRunChain_EmptySuccessIsNotAContributor(t *testing.T) { + res := chainEnricher().runChain(context.Background(), []tagFetch{ + {providerID: "a", fetch: fixedFetch(nil, nil)}, + {providerID: "b", fetch: fixedFetch([]Tag{{Name: "folk", Weight: 1}}, nil)}, + }, "subject", "x") + if len(res.contributors) != 1 || res.contributors[0] != "b" { + t.Errorf("contributors = %v, want [b]", res.contributors) + } +} + +// No enabled providers is a clean "nothing found", NOT a failure — the caller +// settles the subject rather than retrying an empty chain on every tick. +func TestRunChain_EmptyChainSettlesRatherThanRetries(t *testing.T) { + res := chainEnricher().runChain(context.Background(), nil, "subject", "x") + if res.anyTransient { + t.Error("an empty chain must not look transient") + } + if len(res.merged) != 0 || len(res.contributors) != 0 { + t.Errorf("empty chain produced %v / %v", res.merged, res.contributors) + } + if sourceLabel(res.contributors) != sourceNone { + t.Errorf("sourceLabel = %q, want %q", sourceLabel(res.contributors), sourceNone) + } +} diff --git a/internal/tags/provider.go b/internal/tags/provider.go index d1d8c8ce..900825f3 100644 --- a/internal/tags/provider.go +++ b/internal/tags/provider.go @@ -87,6 +87,32 @@ type TrackTagProvider interface { FetchTrackTags(ctx context.Context, ref TrackRef) ([]Tag, error) } +// ArtistRef is the lookup key for artist-level tags. MBID is the artist's +// MusicBrainz ID (required by MBID-keyed providers); Name is the fallback for +// name-based providers (Last.fm). At least one must be set or every provider +// returns ErrNotFound. +type ArtistRef struct { + MBID string + Name string +} + +// ArtistTagProvider is the artist-level tag capability, added for tag-space +// Discover (#2376). Separate from TrackTagProvider — exactly the split that +// interface's doc comment anticipated — so a source can implement either +// without the other, and so the enricher can ask for the capability it needs +// rather than checking at the call site. +// +// The subject here is an artist Minstrel does NOT have locally, so there is no +// track to fall back to and no recording-level tag to prefer. +type ArtistTagProvider interface { + Provider + // FetchArtistTags returns the artist's folksonomy tags, or ErrNotFound + // (terminal — nothing upstream) / ErrTransient (retry). A disabled + // provider, or one missing a required key, returns ErrNotFound so the + // chain simply skips it. + FetchArtistTags(ctx context.Context, ref ArtistRef) ([]Tag, error) +} + // TestableProvider is an opt-in capability for the admin Test-Connection // button: answer "is my config working?" without a full enrichment cycle. type TestableProvider interface { diff --git a/internal/tags/provider_artist_tags_test.go b/internal/tags/provider_artist_tags_test.go new file mode 100644 index 00000000..489be798 --- /dev/null +++ b/internal/tags/provider_artist_tags_test.go @@ -0,0 +1,173 @@ +package tags + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" +) + +// Artist-level tag fetching for out-of-library Discover candidates (#2376). +// Reuses the mbEntityServer / newMBProvider / newLastfmProvider helpers from +// the per-provider test files. + +func TestMusicBrainzFetchArtistTags_UsesFullWeightNotTheFallbackDiscount(t *testing.T) { + // THE distinction worth a test. artistTagWeightFactor (0.6) exists because + // FetchTrackTags uses artist tags as a *proxy* for a track's tags. Here the + // artist IS the subject, so weights must land unscaled — otherwise these + // are not comparable with track_tags, which is the exact comparison slice 6 + // is built on. + srv := mbEntityServer(``, `{"tags":[{"count":4,"name":"shoegaze"},{"count":2,"name":"dream pop"}]}`) + defer srv.Close() + old := mbBaseURL + mbBaseURL = srv.URL + defer func() { mbBaseURL = old }() + + tags, err := newMBProvider(true).FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1", Name: "Some Band"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + m := tagsByName(tags) + if got := m["shoegaze"]; got != 1.0 { + t.Errorf("shoegaze = %v, want 1.0 — not discounted by artistTagWeightFactor", got) + } + if got := m["dream pop"]; got != 0.5 { + t.Errorf("dream pop = %v, want 0.5", got) + } +} + +func TestMusicBrainzFetchArtistTags_GatedOff(t *testing.T) { + if _, err := newMBProvider(false).FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1"}); !errors.Is(err, ErrNotFound) { + t.Errorf("disabled: err = %v, want ErrNotFound", err) + } + // No MBID → nothing MusicBrainz can look up. A name-based guess could + // silently attach the wrong artist's tags, so it deliberately doesn't try. + if _, err := newMBProvider(true).FetchArtistTags(context.Background(), + ArtistRef{Name: "Some Band"}); !errors.Is(err, ErrNotFound) { + t.Errorf("no MBID: err = %v, want ErrNotFound", err) + } +} + +// fetchEntityTags reports an existing-but-untagged entity as (empty, nil) so +// FetchTrackTags can fall through to the artist level. FetchArtistTags has no +// next level, so it must convert that to the terminal ErrNotFound — otherwise +// the enricher would read "no error" as success and settle the candidate as +// enriched with zero tags. +func TestMusicBrainzFetchArtistTags_UntaggedArtistIsNotFound(t *testing.T) { + srv := mbEntityServer(``, `{"tags":[]}`) + defer srv.Close() + old := mbBaseURL + mbBaseURL = srv.URL + defer func() { mbBaseURL = old }() + + _, err := newMBProvider(true).FetchArtistTags(context.Background(), ArtistRef{MBID: "art-1"}) + if !errors.Is(err, ErrNotFound) { + t.Errorf("err = %v, want ErrNotFound", err) + } +} + +func TestLastfmFetchArtistTags_CallsArtistGetTopTags(t *testing.T) { + var gotMethod, gotArtist, gotMBID string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotMethod = r.URL.Query().Get("method") + gotArtist = r.URL.Query().Get("artist") + gotMBID = r.URL.Query().Get("mbid") + _, _ = w.Write([]byte(`{"toptags":{"tag":[{"name":"post-punk","count":100},{"name":"moody","count":40}]}}`)) + })) + defer srv.Close() + old := lastfmBaseURL + lastfmBaseURL = srv.URL + "/" + defer func() { lastfmBaseURL = old }() + + p := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = p.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + + tags, err := p.FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1", Name: "Some Band"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + if gotMethod != "artist.gettoptags" { + t.Errorf("method = %q, want artist.gettoptags", gotMethod) + } + if gotArtist != "Some Band" { + t.Errorf("artist = %q, want Some Band", gotArtist) + } + // MBID is sent as a disambiguating hint when we have one. + if gotMBID != "art-1" { + t.Errorf("mbid = %q, want art-1", gotMBID) + } + m := tagsByName(tags) + if m["post-punk"] != 1.0 { + t.Errorf("post-punk = %v, want 1.0", m["post-punk"]) + } + if m["moody"] != 0.4 { + t.Errorf("moody = %v, want 0.4", m["moody"]) + } +} + +func TestLastfmFetchArtistTags_GatedOff(t *testing.T) { + unkeyed := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = unkeyed.Configure(ProviderSettings{Enabled: true}) + if _, err := unkeyed.FetchArtistTags(context.Background(), + ArtistRef{Name: "A"}); !errors.Is(err, ErrNotFound) { + t.Errorf("unkeyed: err = %v, want ErrNotFound", err) + } + + keyed := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = keyed.Configure(ProviderSettings{Enabled: false, APIKey: "k"}) + if _, err := keyed.FetchArtistTags(context.Background(), + ArtistRef{Name: "A"}); !errors.Is(err, ErrNotFound) { + t.Errorf("disabled: err = %v, want ErrNotFound", err) + } + + // Name-based provider with no name → nothing to query. This is the case + // that matters in practice: a candidate whose name coalesced to '' still + // has an MBID, so MusicBrainz can serve it while Last.fm cannot. + enabled := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = enabled.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + if _, err := enabled.FetchArtistTags(context.Background(), + ArtistRef{MBID: "art-1"}); !errors.Is(err, ErrNotFound) { + t.Errorf("no name: err = %v, want ErrNotFound", err) + } +} + +func TestLastfmFetchArtistTags_TransientErrorCodeRetries(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"error":29}`)) // rate limited + })) + defer srv.Close() + old := lastfmBaseURL + lastfmBaseURL = srv.URL + "/" + defer func() { lastfmBaseURL = old }() + + p := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = p.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + _, err := p.FetchArtistTags(context.Background(), ArtistRef{Name: "A"}) + // Must be ErrTransient, not ErrNotFound: the enricher settles a candidate + // to 'none' on ErrNotFound, which would permanently write off an artist we + // were merely throttled on. + if !errors.Is(err, ErrTransient) { + t.Errorf("err = %v, want ErrTransient", err) + } +} + +func TestLastfmFetchArtistTags_UnknownArtistIsNotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"error":6}`)) // "not found" — terminal + })) + defer srv.Close() + old := lastfmBaseURL + lastfmBaseURL = srv.URL + "/" + defer func() { lastfmBaseURL = old }() + + p := &lastfmProvider{client: newHTTPClient(httpClientOptions{Name: "lastfm", MaxRetries: 1})} + _ = p.Configure(ProviderSettings{Enabled: true, APIKey: "k"}) + if _, err := p.FetchArtistTags(context.Background(), + ArtistRef{Name: "Nobody"}); !errors.Is(err, ErrNotFound) { + t.Errorf("err = %v, want ErrNotFound", err) + } +} diff --git a/internal/tags/provider_lastfm.go b/internal/tags/provider_lastfm.go index e37a3eb8..12917333 100644 --- a/internal/tags/provider_lastfm.go +++ b/internal/tags/provider_lastfm.go @@ -121,6 +121,50 @@ func (p *lastfmProvider) FetchTrackTags(ctx context.Context, ref TrackRef) ([]Ta return out, nil } +// FetchArtistTags looks up an artist's top tags by name, with the MBID as an +// extra hint when present (#2376). Name-first is deliberate and the opposite +// emphasis from MusicBrainz: Last.fm's tag data is keyed on its own artist +// pages, and autocorrect resolves most spelling drift from the similarity feed. +// +// `artist.getTopTags` returns the same `toptags` envelope as +// `track.getTopTags`, so the response type and normalizer are reused as-is — +// the 0-100 popularity scale is identical. +func (p *lastfmProvider) FetchArtistTags(ctx context.Context, ref ArtistRef) ([]Tag, error) { + if !p.enabled.Load() || p.currentKey() == "" { + return nil, ErrNotFound + } + if ref.Name == "" { + return nil, ErrNotFound + } + + q := url.Values{ + "method": {"artist.gettoptags"}, + "api_key": {p.currentKey()}, + "format": {"json"}, + "artist": {ref.Name}, + "autocorrect": {"1"}, + } + if ref.MBID != "" { + q.Set("mbid", ref.MBID) + } + + var resp lastfmTopTags + if err := p.client.getJSON(ctx, lastfmBaseURL+"?"+q.Encode(), &resp); err != nil { + return nil, err + } + if resp.Error != 0 { + if lastfmTransientErrors[resp.Error] { + return nil, ErrTransient + } + return nil, ErrNotFound + } + out := normalizeLastfmTags(resp.TopTags.Tag) + if len(out) == 0 { + return nil, ErrNotFound + } + return out, nil +} + // TestConnection verifies the key against a well-known track. func (p *lastfmProvider) TestConnection(ctx context.Context) error { if p.currentKey() == "" { @@ -163,6 +207,7 @@ func normalizeLastfmTags(raw []lastfmTag) []Tag { // Compile-time capability checks. var ( - _ TrackTagProvider = (*lastfmProvider)(nil) - _ TestableProvider = (*lastfmProvider)(nil) + _ TrackTagProvider = (*lastfmProvider)(nil) + _ ArtistTagProvider = (*lastfmProvider)(nil) + _ TestableProvider = (*lastfmProvider)(nil) ) diff --git a/internal/tags/provider_musicbrainz.go b/internal/tags/provider_musicbrainz.go index 5c89e7d9..5a0362c5 100644 --- a/internal/tags/provider_musicbrainz.go +++ b/internal/tags/provider_musicbrainz.go @@ -96,6 +96,33 @@ func (p *musicbrainzProvider) FetchTrackTags(ctx context.Context, ref TrackRef) return nil, ErrNotFound } +// FetchArtistTags looks up an artist's own tags by MBID (#2376). MBID-only: +// MusicBrainz has no name-based tag lookup worth trusting for this, and a +// wrong-artist match would poison the tag cache silently. +// +// Note the scale is 1.0, NOT artistTagWeightFactor. That discount exists +// because FetchTrackTags uses artist tags as a *proxy* for a track's tags, and +// the artist's overall character is the coarser signal of the two. Here the +// artist IS the subject, so there is nothing to discount relative to — and +// applying it would make these weights incomparable with track_tags, which is +// exactly the comparison slice 6 depends on. +func (p *musicbrainzProvider) FetchArtistTags(ctx context.Context, ref ArtistRef) ([]Tag, error) { + if !p.enabled.Load() || ref.MBID == "" { + return nil, ErrNotFound + } + tags, err := p.fetchEntityTags(ctx, "artist", ref.MBID, 1.0) + if err != nil { + return nil, err + } + // fetchEntityTags reports an existing-but-untagged entity as (empty, nil) + // so FetchTrackTags can fall through to the next level. There is no next + // level here, so empty is the terminal "nothing upstream". + if len(tags) == 0 { + return nil, ErrNotFound + } + return tags, nil +} + // fetchEntityTags loads folksonomy tags for a MusicBrainz entity ("recording" // or "artist") by MBID and scales the normalized weights by `scale`. Returns // an empty slice (not ErrNotFound) when the entity exists but is untagged, so @@ -153,6 +180,7 @@ func normalizeMBTags(raw []mbTag) []Tag { // Compile-time capability checks. var ( - _ TrackTagProvider = (*musicbrainzProvider)(nil) - _ TestableProvider = (*musicbrainzProvider)(nil) + _ TrackTagProvider = (*musicbrainzProvider)(nil) + _ ArtistTagProvider = (*musicbrainzProvider)(nil) + _ TestableProvider = (*musicbrainzProvider)(nil) ) diff --git a/internal/tags/settings.go b/internal/tags/settings.go index b16a3117..3914bb3d 100644 --- a/internal/tags/settings.go +++ b/internal/tags/settings.go @@ -135,6 +135,27 @@ func (s *SettingsService) EnabledTrackTagProviders() []TrackTagProvider { return out } +// EnabledArtistTagProviders returns the enabled providers implementing +// ArtistTagProvider, in registration order. Snapshot — do not mutate. +// +// Separate from EnabledTrackTagProviders rather than one call with a capability +// argument: the two chains are consumed by different drains, and a provider may +// implement one capability without the other. +func (s *SettingsService) EnabledArtistTagProviders() []ArtistTagProvider { + s.mu.RLock() + defer s.mu.RUnlock() + var out []ArtistTagProvider + for _, p := range AllProviders() { + if !s.enabledIDs[p.ID()] { + continue + } + if ap, ok := p.(ArtistTagProvider); ok { + out = append(out, ap) + } + } + return out +} + // CurrentVersion returns the version the enricher stamps onto rows. func (s *SettingsService) CurrentVersion() int32 { s.mu.RLock() diff --git a/internal/tags/worker.go b/internal/tags/worker.go index 701a08f3..35ab883c 100644 --- a/internal/tags/worker.go +++ b/internal/tags/worker.go @@ -14,22 +14,32 @@ import ( // providers' httpClients, so a tick just drains a bounded batch and the // external APIs pace themselves. type Worker struct { - enricher *Enricher - logger *slog.Logger - tick time.Duration - batch int + enricher *Enricher + logger *slog.Logger + tick time.Duration + batch int + candidateBatch int } // NewWorker constructs a worker with production defaults: an initial drain // shortly after boot, then every 30 minutes, up to 200 tracks per tick. // MusicBrainz's 1 req/s ceiling is the real throttle, so the batch size // mainly bounds how long one tick runs, not the request rate. +// +// candidateBatch is smaller than the track batch on purpose. Library tracks are +// a finite set that drains to completion and then costs nothing; out-of-library +// candidates (#2376) are effectively unbounded — every library artist's +// neighbours — so this arm would otherwise monopolise every tick forever and +// starve the track arm. 50/tick at ~1 req/s is roughly a minute of work, and +// the query hands back the highest-similarity candidates first so the ones that +// can actually reach a user's deck are enriched first. func NewWorker(enricher *Enricher, logger *slog.Logger) *Worker { return &Worker{ - enricher: enricher, - logger: logger, - tick: 30 * time.Minute, - batch: 200, + enricher: enricher, + logger: logger, + tick: 30 * time.Minute, + batch: 200, + candidateBatch: 50, } } @@ -48,12 +58,27 @@ func (w *Worker) Run(ctx context.Context) { } } -// tickOnce drains one bounded batch. EnrichTrackBatch already logs a -// category breakdown, so this only surfaces a fatal batch error. +// tickOnce drains one bounded batch of each kind. Both Enrich*Batch methods +// already log a category breakdown, so this only surfaces a fatal batch error. +// +// Tracks first: they back the taste profile the whole app reads from, whereas +// candidate tags only affect the Discover request surface. On a fresh install +// both are cold, and getting the profile warm matters more. func (w *Worker) tickOnce(ctx context.Context) { if _, _, _, err := w.enricher.EnrichTrackBatch(ctx, w.batch, nil); err != nil { if ctx.Err() == nil { w.logger.Error("tags: enrichment tick failed", "err", err) } } + // Not gated on the track arm's success: the two drains share nothing but a + // provider chain, and a track-side failure says nothing about whether + // candidate lookups will work. + if ctx.Err() != nil { + return + } + if _, _, _, err := w.enricher.EnrichCandidateArtistBatch(ctx, w.candidateBatch); err != nil { + if ctx.Err() == nil { + w.logger.Error("tags: candidate-artist enrichment tick failed", "err", err) + } + } } diff --git a/web/src/lib/api/queries.ts b/web/src/lib/api/queries.ts index f59d7260..73db1e60 100644 --- a/web/src/lib/api/queries.ts +++ b/web/src/lib/api/queries.ts @@ -58,6 +58,7 @@ export const qk = { smtpConfig: () => ['smtpConfig'] as const, suggestions: (limit?: number) => ['suggestions', { limit: limit ?? 12 }] as const, + suggestionSnoozes: () => ['suggestionSnoozes'] as const, home: () => ['home'] as const, albumsAlpha: () => ['albumsAlpha'] as const, artistTracks: (artistId: string) => ['artistTracks', artistId] as const, diff --git a/web/src/lib/api/suggestions.test.ts b/web/src/lib/api/suggestions.test.ts index 094bd4ba..86b2c861 100644 --- a/web/src/lib/api/suggestions.test.ts +++ b/web/src/lib/api/suggestions.test.ts @@ -1,13 +1,18 @@ import { afterEach, describe, expect, test, vi } from 'vitest'; vi.mock('./client', () => ({ - api: { get: vi.fn() } + api: { get: vi.fn(), post: vi.fn(), del: vi.fn() } })); -import { listSuggestions } from './suggestions'; +import { + listSuggestions, + listSnoozes, + snoozeSuggestion, + unsnoozeSuggestion +} from './suggestions'; import { qk } from './queries'; import { api } from './client'; -import type { ArtistSuggestion } from './types'; +import type { ArtistSuggestion, SuggestionSnooze } from './types'; afterEach(() => vi.clearAllMocks()); @@ -40,3 +45,56 @@ describe('suggestions client', () => { expect(qk.suggestions(20)).toEqual(['suggestions', { limit: 20 }]); }); }); + +describe('suggestion snoozes (#2375)', () => { + test('snoozeSuggestion sends the name — the server 400s without it', async () => { + (api.post as ReturnType).mockResolvedValueOnce(null); + await snoozeSuggestion('mb-1', 'Parked Artist'); + expect(api.post).toHaveBeenCalledWith('/api/discover/suggestions/mb-1/snooze', { + name: 'Parked Artist' + }); + }); + + test('snoozeSuggestion sends no days, leaving the default to the server', async () => { + (api.post as ReturnType).mockResolvedValueOnce(null); + await snoozeSuggestion('mb-1', 'Parked Artist'); + const body = (api.post as ReturnType).mock.calls[0][1] as Record; + expect(body).not.toHaveProperty('days'); + }); + + // MBIDs are UUIDs today, but the column is free-text and the value comes + // from an external similarity feed, so it goes through encodeURIComponent. + test('the mbid is URL-encoded into the path', async () => { + (api.post as ReturnType).mockResolvedValueOnce(null); + await snoozeSuggestion('weird/id?x', 'Odd'); + expect(api.post).toHaveBeenCalledWith( + '/api/discover/suggestions/weird%2Fid%3Fx/snooze', + { name: 'Odd' } + ); + }); + + test('unsnoozeSuggestion DELETEs the same path', async () => { + (api.del as ReturnType).mockResolvedValueOnce(null); + await unsnoozeSuggestion('mb-1'); + expect(api.del).toHaveBeenCalledWith('/api/discover/suggestions/mb-1/snooze'); + }); + + test('listSnoozes hits the snoozes collection', async () => { + const fixture: SuggestionSnooze[] = [ + { + mbid: 'mb-1', + name: 'Parked Artist', + snoozed_until: '2026-11-01T00:00:00Z', + created_at: '2026-08-03T00:00:00Z' + } + ]; + (api.get as ReturnType).mockResolvedValueOnce(fixture); + const got = await listSnoozes(); + expect(api.get).toHaveBeenCalledWith('/api/discover/snoozes'); + expect(got).toEqual(fixture); + }); + + test('qk.suggestionSnoozes key shape', () => { + expect(qk.suggestionSnoozes()).toEqual(['suggestionSnoozes']); + }); +}); diff --git a/web/src/lib/api/suggestions.ts b/web/src/lib/api/suggestions.ts index 2d998898..2abcd30a 100644 --- a/web/src/lib/api/suggestions.ts +++ b/web/src/lib/api/suggestions.ts @@ -1,7 +1,7 @@ import { createQuery } from '@tanstack/svelte-query'; import { api } from './client'; import { qk } from './queries'; -import type { ArtistSuggestion } from './types'; +import type { ArtistSuggestion, SuggestionSnooze } from './types'; export async function listSuggestions(limit = 12): Promise { return api.get(`/api/discover/suggestions?limit=${limit}`); @@ -14,3 +14,36 @@ export function createSuggestionsQuery(limit = 12) { staleTime: 5 * 60_000 // 5 minutes — see M5c spec §5 }); } + +// Parks a suggestion for the server's default period (90 days). `name` is +// REQUIRED by the server and is not optional bookkeeping: candidates are +// out-of-library, so there is no artists row to resolve a display name from +// and the snooze list would have nothing to render. Omitting it is a 400. +// +// No `days` is sent. There is deliberately no duration UI yet — that knob is +// slice 6 (#2377) — and hardcoding a value here would pin the default to the +// client instead of the server that owns it. +export async function snoozeSuggestion(mbid: string, name: string): Promise { + await api.post(`/api/discover/suggestions/${encodeURIComponent(mbid)}/snooze`, { name }); +} + +// Brings a parked suggestion back immediately. The server 404s an MBID that +// was never snoozed; callers treat that as already-unsnoozed rather than as a +// failure, since the end state the user asked for is the one they get. +export async function unsnoozeSuggestion(mbid: string): Promise { + await api.del(`/api/discover/suggestions/${encodeURIComponent(mbid)}/snooze`); +} + +export async function listSnoozes(): Promise { + return api.get('/api/discover/snoozes'); +} + +export function createSnoozesQuery() { + return createQuery({ + queryKey: qk.suggestionSnoozes(), + queryFn: listSnoozes + // No staleTime, unlike the suggestions query: this list is the only route + // back to an un-snooze, so it must reflect a snooze made seconds ago + // rather than a cached view of the world. + }); +} diff --git a/web/src/lib/api/tuning.ts b/web/src/lib/api/tuning.ts index fae0f40f..10d1979d 100644 --- a/web/src/lib/api/tuning.ts +++ b/web/src/lib/api/tuning.ts @@ -26,14 +26,24 @@ export type TasteTuning = { mood_scale: number; }; -export type TuningScope = 'radio' | 'daily_mix' | 'taste'; +// Discover request-surface knobs (#2377). Its own scope rather than part of +// taste: snooze_days belongs here, and a snooze must never be read as taste +// signal (#2374). +export type DiscoverTuning = { + tag_overlap_weight: number; + snooze_days: number; +}; + +export type TuningScope = 'radio' | 'daily_mix' | 'taste' | 'discover'; export type TuningSnapshot = { profiles: Record<'radio' | 'daily_mix', WeightProfile>; taste: TasteTuning; + discover: DiscoverTuning; shipped: { profiles: Record<'radio' | 'daily_mix', WeightProfile>; taste: TasteTuning; + discover: DiscoverTuning; }; }; diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index c8cdf030..17a7f791 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -332,6 +332,20 @@ export type ArtistSuggestion = { score: number; attribution: SeedContribution[]; // up to 3 entries, ordered by contribution DESC image_url?: string; // resolved on-demand from Lidarr; absent → card placeholder + // Tags this candidate shares with your taste profile, strongest first (max 3, + // #2377). Absent for most candidates — tag coverage for artists you don't own + // is partial by nature (#2376) — so the card falls back to seed attribution. + matched_tags?: string[]; +}; + +// One parked suggestion — "not right now", not a dislike. The server only +// ever returns rows whose snoozed_until is still in the future, so the client +// never has to compare against the clock to decide what to show. +export type SuggestionSnooze = { + mbid: string; + name: string; + snoozed_until: string; // RFC3339 + created_at: string; }; // Mirrors internal/api/types.go HomePayload. All slices are non-null diff --git a/web/src/lib/components/DiscoverResultCard.svelte b/web/src/lib/components/DiscoverResultCard.svelte index 5738092b..ea3ae967 100644 --- a/web/src/lib/components/DiscoverResultCard.svelte +++ b/web/src/lib/components/DiscoverResultCard.svelte @@ -1,10 +1,14 @@
@@ -58,7 +147,16 @@ {#if !query.isPending && suggestions.length === 0} -

Listen to something or like an artist to start getting suggestions.

+ +

+ {snoozes.length === 0 + ? 'Listen to something or like an artist to start getting suggestions.' + : "Nothing new right now — the artists you've parked are below."} +

{:else if suggestions.length > 0}
{#each suggestions.filter(visible) as s (s.mbid)} @@ -66,11 +164,49 @@ kind="artist" title={s.name} imageUrl={s.image_url} - state="requestable" - attribution={attributionText(s.attribution)} + state={cardState(s)} + attribution={reasonText(s)} onRequest={() => onRequest(s)} + onSnooze={() => onSnooze(s)} + onUnsnooze={() => onUnsnooze(s.mbid, s.name)} /> {/each}
{/if} + + + {#if snoozes.length > 0} +
+

+ Not right now +

+

+ These come back on their own. Nothing here counts against your taste profile. +

+
    + {#each snoozes as snoozed (snoozed.mbid)} +
  • +
    +
    {snoozed.name}
    +
    + Back {returnsIn(snoozed.snoozed_until)} +
    +
    + +
    • + {/each} +
+
+ {/if}
diff --git a/web/src/lib/components/SuggestionFeed.test.ts b/web/src/lib/components/SuggestionFeed.test.ts index 0e6fdb43..c1f8a260 100644 --- a/web/src/lib/components/SuggestionFeed.test.ts +++ b/web/src/lib/components/SuggestionFeed.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, test, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; import { render, screen, fireEvent } from '@testing-library/svelte'; import { mockQuery } from '../../test-utils/query'; @@ -9,17 +9,30 @@ vi.mock('@tanstack/svelte-query', async (orig) => { }); vi.mock('$lib/api/suggestions', () => ({ - createSuggestionsQuery: vi.fn() + createSuggestionsQuery: vi.fn(), + createSnoozesQuery: vi.fn(), + snoozeSuggestion: vi.fn().mockResolvedValue(undefined), + unsnoozeSuggestion: vi.fn().mockResolvedValue(undefined) })); vi.mock('$lib/api/requests', () => ({ createRequest: vi.fn().mockResolvedValue({}) })); +const pushToastMock = vi.fn(); +vi.mock('$lib/stores/toast.svelte', () => ({ + pushToast: (...args: unknown[]) => pushToastMock(...args) +})); + import SuggestionFeed from './SuggestionFeed.svelte'; -import { createSuggestionsQuery } from '$lib/api/suggestions'; +import { + createSuggestionsQuery, + createSnoozesQuery, + snoozeSuggestion, + unsnoozeSuggestion +} from '$lib/api/suggestions'; import { createRequest } from '$lib/api/requests'; -import type { ArtistSuggestion } from '$lib/api/types'; +import type { ArtistSuggestion, SuggestionSnooze } from '$lib/api/types'; const oneSeed: ArtistSuggestion = { mbid: 'mb1', @@ -51,46 +64,50 @@ const threeSeeds: ArtistSuggestion = { ] }; +/** Days from now as an RFC3339 string, for snooze fixtures. */ +function inDays(n: number): string { + return new Date(Date.now() + n * 86_400_000).toISOString(); +} + +function setSuggestions(data: ArtistSuggestion[]) { + (createSuggestionsQuery as ReturnType).mockReturnValue(mockQuery({ data })); +} + +function setSnoozes(data: SuggestionSnooze[]) { + (createSnoozesQuery as ReturnType).mockReturnValue(mockQuery({ data })); +} + +beforeEach(() => setSnoozes([])); afterEach(() => vi.clearAllMocks()); describe('SuggestionFeed', () => { test('renders one card per suggestion', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [oneSeed, twoSeeds] }) - ); + setSuggestions([oneSeed, twoSeeds]); render(SuggestionFeed); expect(screen.getByText('Outsider')).toBeInTheDocument(); expect(screen.getByText('Outsider Two')).toBeInTheDocument(); }); test('attribution copy: 1 seed → "Because you liked X."', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [oneSeed] }) - ); + setSuggestions([oneSeed]); render(SuggestionFeed); expect(screen.getByText(/because you liked seed\./i)).toBeInTheDocument(); }); test('attribution copy: 2 seeds → "Because you liked A and played B."', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [twoSeeds] }) - ); + setSuggestions([twoSeeds]); render(SuggestionFeed); expect(screen.getByText(/because you liked a and played b\./i)).toBeInTheDocument(); }); test('attribution copy: 3 seeds → Oxford comma', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [threeSeeds] }) - ); + setSuggestions([threeSeeds]); render(SuggestionFeed); expect(screen.getByText(/because you liked x, played y, and played z\./i)).toBeInTheDocument(); }); test('Request button calls createRequest with artist-kind body', async () => { - (createSuggestionsQuery as ReturnType).mockReturnValue( - mockQuery({ data: [oneSeed] }) - ); + setSuggestions([oneSeed]); render(SuggestionFeed); await fireEvent.click(screen.getByRole('button', { name: /request outsider/i })); expect(createRequest).toHaveBeenCalledWith({ @@ -102,8 +119,174 @@ describe('SuggestionFeed', () => { }); test('empty state when data is []', () => { - (createSuggestionsQuery as ReturnType).mockReturnValue(mockQuery({ data: [] })); + setSuggestions([]); render(SuggestionFeed); expect(screen.getByText(/listen to something or like an artist/i)).toBeInTheDocument(); }); }); + +describe('SuggestionFeed snooze (#2375)', () => { + test('snooze sends BOTH mbid and name — the server 400s without the name', async () => { + setSuggestions([oneSeed]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /not right now/i })); + expect(snoozeSuggestion).toHaveBeenCalledWith('mb1', 'Outsider'); + }); + + test('the card stays in place showing Undo, rather than vanishing', async () => { + setSuggestions([oneSeed]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /not right now/i })); + // Still on screen — the disappearance happens on refetch, not under the + // cursor (rule #24). + expect(screen.getByText('Outsider')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: /bring outsider back/i })).toBeInTheDocument(); + expect(screen.getByRole('status')).toHaveTextContent('Not right now'); + }); + + test('a failed snooze reverts the card and says so', async () => { + (snoozeSuggestion as ReturnType).mockRejectedValueOnce(new Error('offline')); + setSuggestions([oneSeed]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /not right now/i })); + // Back to requestable — a snooze that silently did nothing would leave + // the user tapping it again. + expect(screen.getByRole('button', { name: /request outsider/i })).toBeInTheDocument(); + expect(pushToastMock).toHaveBeenCalledWith("Couldn't hide Outsider", 'error'); + }); + + test('undo on the card calls unsnoozeSuggestion', async () => { + setSuggestions([oneSeed]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /not right now/i })); + await fireEvent.click(screen.getByRole('button', { name: /bring outsider back/i })); + expect(unsnoozeSuggestion).toHaveBeenCalledWith('mb1'); + }); + + test('the snoozed list is the way back once the card is gone', async () => { + // Deck empty, one parked artist: exactly the state after a refetch. + setSuggestions([]); + setSnoozes([ + { mbid: 'mbX', name: 'Parked', snoozed_until: inDays(90), created_at: inDays(0) } + ]); + render(SuggestionFeed); + expect(screen.getByRole('heading', { name: /not right now/i })).toBeInTheDocument(); + expect(screen.getByText('Parked')).toBeInTheDocument(); + await fireEvent.click(screen.getByRole('button', { name: /bring parked back now/i })); + expect(unsnoozeSuggestion).toHaveBeenCalledWith('mbX'); + }); + + test('a 404 from unsnooze is not surfaced as an error', async () => { + (unsnoozeSuggestion as ReturnType).mockRejectedValueOnce({ status: 404 }); + setSuggestions([]); + setSnoozes([ + { mbid: 'mbX', name: 'Parked', snoozed_until: inDays(90), created_at: inDays(0) } + ]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /bring parked back now/i })); + // Already-unsnoozed IS the end state the user asked for. + expect(pushToastMock).not.toHaveBeenCalled(); + }); + + test('a non-404 unsnooze failure does surface', async () => { + (unsnoozeSuggestion as ReturnType).mockRejectedValueOnce({ status: 500 }); + setSuggestions([]); + setSnoozes([ + { mbid: 'mbX', name: 'Parked', snoozed_until: inDays(90), created_at: inDays(0) } + ]); + render(SuggestionFeed); + await fireEvent.click(screen.getByRole('button', { name: /bring parked back now/i })); + expect(pushToastMock).toHaveBeenCalledWith("Couldn't bring Parked back", 'error'); + }); + + test('return time reads as a relative phrase, not a calendar date', () => { + setSuggestions([]); + setSnoozes([ + { mbid: 'a', name: 'Quarter', snoozed_until: inDays(90), created_at: inDays(0) }, + { mbid: 'b', name: 'Fortnight', snoozed_until: inDays(14), created_at: inDays(0) } + ]); + render(SuggestionFeed); + expect(screen.getByText(/back in about 3 months/i)).toBeInTheDocument(); + expect(screen.getByText(/back in 14 days/i)).toBeInTheDocument(); + }); + + // Pins the days→months seam. The singular branch was originally dead code + // here too: the threshold (45) sat above the divisor (30), so no day count + // could round to one month without hitting the days branch first. Kept in + // lockstep with SuggestionSnoozeRefTest on Android. + test('the days-to-months boundary sits at 30 days, so "a month" is reachable', () => { + setSuggestions([]); + setSnoozes([ + { mbid: 'a', name: 'JustUnder', snoozed_until: inDays(29), created_at: inDays(0) }, + { mbid: 'b', name: 'JustOver', snoozed_until: inDays(30), created_at: inDays(0) } + ]); + render(SuggestionFeed); + expect(screen.getByText(/back in 29 days/i)).toBeInTheDocument(); + expect(screen.getByText(/back in about a month/i)).toBeInTheDocument(); + }); + + test('no snoozed section when nothing is parked', () => { + setSuggestions([oneSeed]); + setSnoozes([]); + render(SuggestionFeed); + expect(screen.queryByRole('heading', { name: /not right now/i })).not.toBeInTheDocument(); + }); + + // An empty deck has two causes now, and the advice differs. Telling someone + // who parked everything to go listen to music would be wrong. + test('empty-deck copy distinguishes "no signal" from "you parked them all"', () => { + setSuggestions([]); + setSnoozes([ + { mbid: 'a', name: 'Parked', snoozed_until: inDays(90), created_at: inDays(0) } + ]); + render(SuggestionFeed); + expect(screen.getByText(/nothing new right now/i)).toBeInTheDocument(); + expect(screen.queryByText(/listen to something or like an artist/i)).not.toBeInTheDocument(); + }); +}); + +describe('SuggestionFeed taste-tag reason (#2377)', () => { + const tagged = (over: Partial = {}): ArtistSuggestion => ({ + ...oneSeed, + mbid: 'mbT', + name: 'Tag Match', + ...over + }); + + test('matched tags replace seed attribution as the reason line', () => { + setSuggestions([tagged({ matched_tags: ['shoegaze', 'dream pop'] })]); + render(SuggestionFeed); + expect( + screen.getByText(/matches your taste in shoegaze and dream pop\./i) + ).toBeInTheDocument(); + // The graph-adjacency line is superseded — describing the music beats + // describing the graph when we can do both. + expect(screen.queryByText(/because you liked/i)).not.toBeInTheDocument(); + }); + + test('one matched tag reads in the singular', () => { + setSuggestions([tagged({ matched_tags: ['shoegaze'] })]); + render(SuggestionFeed); + expect(screen.getByText(/matches your taste in shoegaze\./i)).toBeInTheDocument(); + }); + + test('three matched tags use an Oxford comma, matching the seed copy', () => { + setSuggestions([tagged({ matched_tags: ['a', 'b', 'c'] })]); + render(SuggestionFeed); + expect(screen.getByText(/matches your taste in a, b, and c\./i)).toBeInTheDocument(); + }); + + // The common case: most candidates have no cached tags, and the card must + // still explain itself rather than going blank. + test('no matched tags falls back to seed attribution', () => { + setSuggestions([oneSeed]); + render(SuggestionFeed); + expect(screen.getByText(/because you liked seed\./i)).toBeInTheDocument(); + }); + + test('an empty matched_tags array also falls back, not to an empty line', () => { + setSuggestions([tagged({ matched_tags: [] })]); + render(SuggestionFeed); + expect(screen.getByText(/because you liked seed\./i)).toBeInTheDocument(); + }); +}); diff --git a/web/src/routes/admin/tuning/+page.svelte b/web/src/routes/admin/tuning/+page.svelte index 79e96668..33bb0b19 100644 --- a/web/src/routes/admin/tuning/+page.svelte +++ b/web/src/routes/admin/tuning/+page.svelte @@ -9,6 +9,7 @@ type TuningSnapshot, type WeightProfile, type TasteTuning, + type DiscoverTuning, type TrendsResponse, type TrendSeries, type TrendMarker @@ -43,6 +44,11 @@ { key: 'mood_scale', label: 'Mood weight', hint: 'How strongly a mood-tagged play imprints on the mood facet (from folksonomy tags), in [0, 1]. 0 = mood ignored.' } ]; + const discoverFields: { key: keyof DiscoverTuning; label: string; hint: string }[] = [ + { key: 'tag_overlap_weight', label: 'Taste-tag weight', hint: "How strongly a candidate's tags matching your taste boosts it. score x (1 + w x overlap), so 0 turns the tag term off and ranks on similarity alone. An artist with no cached tags is never penalised." }, + { key: 'snooze_days', label: 'Snooze length (days)', hint: 'How long "not right now" parks a suggestion before it returns on its own. Records no opinion about the artist and never feeds the taste profile.' } + ]; + const profileScopes: { scope: 'radio' | 'daily_mix'; label: string; blurb: string }[] = [ { scope: 'radio', label: 'Radio', blurb: 'Seed-directed listening — the user picked a direction.' }, { scope: 'daily_mix', label: 'Daily mixes', blurb: 'For You, Songs like…, and the discovery mixes.' } @@ -55,11 +61,17 @@ let saving = $state(null); function fillForm(snap: TuningSnapshot) { - const f: Record> = { radio: {}, daily_mix: {}, taste: {} }; + const f: Record> = { + radio: {}, + daily_mix: {}, + taste: {}, + discover: {} + }; for (const p of ['radio', 'daily_mix'] as const) { for (const { key } of weightFields) f[p][key] = String(snap.profiles[p][key]); } for (const { key } of tasteFields) f.taste[key] = String(snap.taste[key]); + for (const { key } of discoverFields) f.discover[key] = String(snap.discover[key]); form = f; } @@ -76,11 +88,17 @@ // A knob deviates when its CURRENT SAVED value differs from shipped; // the dot marks where this install has drifted from defaults. - function deviates(scope: 'radio' | 'daily_mix' | 'taste', key: string): boolean { + function deviates(scope: TuningScope, key: string): boolean { if (!snapshot) return false; if (scope === 'taste') { return snapshot.taste[key as keyof TasteTuning] !== snapshot.shipped.taste[key as keyof TasteTuning]; } + if (scope === 'discover') { + return ( + snapshot.discover[key as keyof DiscoverTuning] !== + snapshot.shipped.discover[key as keyof DiscoverTuning] + ); + } return ( snapshot.profiles[scope][key as keyof WeightProfile] !== snapshot.shipped.profiles[scope][key as keyof WeightProfile] @@ -90,6 +108,7 @@ function currentValue(scope: TuningScope, key: string): number { if (!snapshot) return 0; if (scope === 'taste') return snapshot.taste[key as keyof TasteTuning]; + if (scope === 'discover') return snapshot.discover[key as keyof DiscoverTuning]; return snapshot.profiles[scope][key as keyof WeightProfile]; } @@ -276,6 +295,7 @@ type="button" disabled={saving !== null} onclick={() => reset(p.scope)} + aria-label="Reset {p.label} to defaults" class="rounded border border-border px-3 py-1.5 text-sm text-text-secondary hover:text-text-primary disabled:opacity-50" > Reset to defaults @@ -328,6 +348,63 @@ type="button" disabled={saving !== null} onclick={() => reset('taste')} + aria-label="Reset taste profile build to defaults" + class="rounded border border-border px-3 py-1.5 text-sm text-text-secondary hover:text-text-primary disabled:opacity-50" + > + Reset to defaults + + + + + +
+
+

Discover requests

+

+ How the Discover suggestion deck ranks out-of-library artists. Tag coverage for + artists you don't own is partial by nature, so an untagged candidate keeps its + similarity score rather than being pushed down. +

+
+
+ {#each discoverFields as f (f.key)} +
+ + +
+ {/each} +
+
+ +