Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 142 additions & 0 deletions packages/bot/src/handlers/player/trackHandlers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ const infoLogMock = jest.fn()
const debugLogMock = jest.fn()
const errorLogMock = jest.fn()
const warnLogMock = jest.fn()
const recordImplicitFeedbackMock = jest.fn()

jest.mock('discord-player', () => ({
QueueRepeatMode: {
Expand Down Expand Up @@ -100,6 +101,18 @@ jest.mock('@lucky/shared/utils', () => ({
warnLog: (...args: unknown[]) => warnLogMock(...args),
}))

jest.mock('../../services/musicRecommendation/feedbackService', () => ({
recommendationFeedbackService: {
recordImplicitFeedback: (...args: unknown[]) =>
recordImplicitFeedbackMock(...args),
},
}))

jest.mock('../../utils/music/searchQueryCleaner', () => ({
cleanTitle: (title: string) => title,
cleanAuthor: (author: string) => author,
}))

type PlayerEventHandler = (queue: GuildQueue, track?: Track) => Promise<void>

function createTrack(requestedById = 'listener-1'): Track {
Expand Down Expand Up @@ -177,6 +190,7 @@ describe('trackHandlers autoplay replenishment', () => {
updateLastFmNowPlayingMock.mockResolvedValue(undefined)
scrobbleCurrentTrackIfLastFmMock.mockResolvedValue(undefined)
saveSnapshotMock.mockResolvedValue(undefined)
recordImplicitFeedbackMock.mockResolvedValue(undefined)
})

afterEach(() => {
Expand Down Expand Up @@ -483,4 +497,132 @@ describe('trackHandlers autoplay replenishment', () => {
message: 'Added "Test Song" to queue in Guild One',
})
})

it('records implicit like on playerFinish when track played > 80%', async () => {
jest.useFakeTimers()
const handlers = setupHandlers()
const playerStart = handlers.playerStart
const playerFinish = handlers.playerFinish
const queue = createQueue(QueueRepeatMode.AUTOPLAY)
const finishedTrack = {
...createTrack('listener-finish-1'),
durationMS: 100000,
}

await playerStart(queue, finishedTrack)
jest.advanceTimersByTime(85000)
await playerFinish(queue, finishedTrack)

expect(recordImplicitFeedbackMock).toHaveBeenCalledWith(
'listener-finish-1',
'testsong::testartist',
'implicit_like',
)
})

it('does not record feedback on playerFinish when track played < 80%', async () => {
jest.useFakeTimers()
const handlers = setupHandlers()
const playerStart = handlers.playerStart
const playerFinish = handlers.playerFinish
const queue = createQueue(QueueRepeatMode.AUTOPLAY)
const finishedTrack = {
...createTrack('listener-finish-2'),
durationMS: 100000,
}

await playerStart(queue, finishedTrack)
jest.advanceTimersByTime(60000)
await playerFinish(queue, finishedTrack)

expect(recordImplicitFeedbackMock).not.toHaveBeenCalled()
})

it('records implicit dislike on playerSkip when track played < 30%', async () => {
jest.useFakeTimers()
const handlers = setupHandlers()
const playerStart = handlers.playerStart
const playerSkip = handlers.playerSkip
const queue = createQueue(QueueRepeatMode.AUTOPLAY)
const skippedTrack = {
...createTrack('listener-skip-1'),
durationMS: 100000,
}

await playerStart(queue, skippedTrack)
jest.advanceTimersByTime(20000)
await playerSkip(queue, skippedTrack)

expect(recordImplicitFeedbackMock).toHaveBeenCalledWith(
'listener-skip-1',
'testsong::testartist',
'implicit_dislike',
)
})

it('does not record feedback on playerSkip when track < 20 seconds duration', async () => {
jest.useFakeTimers()
const handlers = setupHandlers()
const playerStart = handlers.playerStart
const playerSkip = handlers.playerSkip
const queue = createQueue(QueueRepeatMode.AUTOPLAY)
const shortTrack = {
...createTrack('listener-skip-2'),
durationMS: 10000,
}

await playerStart(queue, shortTrack)
jest.advanceTimersByTime(5000)
await playerSkip(queue, shortTrack)

expect(recordImplicitFeedbackMock).not.toHaveBeenCalled()
})

it('does not record feedback on playerSkip when track played > 30%', async () => {
jest.useFakeTimers()
const handlers = setupHandlers()
const playerStart = handlers.playerStart
const playerSkip = handlers.playerSkip
const queue = createQueue(QueueRepeatMode.AUTOPLAY)
const skippedTrack = {
...createTrack('listener-skip-3'),
durationMS: 100000,
}

await playerStart(queue, skippedTrack)
jest.advanceTimersByTime(50000)
await playerSkip(queue, skippedTrack)

expect(recordImplicitFeedbackMock).not.toHaveBeenCalled()
})

it('records implicit like for track with metadata requestedById on playerFinish', async () => {
jest.useFakeTimers()
const handlers = setupHandlers()
const playerStart = handlers.playerStart
const playerFinish = handlers.playerFinish
const queue = createQueue(QueueRepeatMode.AUTOPLAY)
const metadataTrack = {
id: 'track-3',
title: 'Metadata Track',
author: 'Metadata Artist',
url: 'https://example.com/track-3',
source: 'youtube',
requestedBy: undefined,
metadata: {
requestedById: 'listener-finish-3',
},
durationMS: 100000,
} as unknown as Track

await playerStart(queue, metadataTrack)
jest.advanceTimersByTime(85000)
await playerFinish(queue, metadataTrack)

expect(recordImplicitFeedbackMock).toHaveBeenCalledWith(
'listener-finish-3',
'metadatatrack::metadataartist',
'implicit_like',
)
})
})
77 changes: 77 additions & 0 deletions packages/bot/src/handlers/player/trackHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,15 @@ import {
clearIdleTimer,
} from '../../utils/music/idleDisconnect'
import { clearVotes } from '../../utils/music/voteSkipStore'
import { recommendationFeedbackService } from '../../services/musicRecommendation/feedbackService'
import { cleanTitle, cleanAuthor } from '../../utils/music/searchQueryCleaner'

const MAX_GUILD_ENTRIES = 500

export const lastPlayedTracks = new Map<string, Track>()

const guildTrackStartTimes = new Map<string, number>()

export type TrackHistoryEntry = {
url: string
title: string
Expand All @@ -47,6 +51,48 @@ function isAutoplayTrack(track: Track, clientUserId?: string): boolean {
)
}

function normalizeTrackKeyForFeedback(title: string, author: string): string {
const normalizedTitle = cleanTitle(title)
.toLowerCase()
.replaceAll(/[^a-z0-9]+/g, '')
.trim()
const normalizedAuthor = cleanAuthor(author)
.toLowerCase()
.replaceAll(/[^a-z0-9]+/g, '')
.trim()
return `${normalizedTitle}::${normalizedAuthor}`
}

async function recordPlayBehavior(
guildId: string,
track: Track | undefined,
requestedById: string | undefined,
minDurationMs: number,
ratioThreshold: number,
feedbackType: 'implicit_like' | 'implicit_dislike',
): Promise<void> {
if (!track || !requestedById) return
const startTime = guildTrackStartTimes.get(guildId)
if (!startTime || !track.durationMS || track.durationMS < minDurationMs) {
guildTrackStartTimes.delete(guildId)
return
Comment on lines +76 to +78

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use a strict > 20s gate for skip feedback.

The current check allows durationMS === 20_000, but the feature is described as recording early skips only for tracks longer than 20 seconds. This should be <= minDurationMs here, otherwise 20.0s tracks are misclassified.

Suggested fix
-    if (!startTime || !track.durationMS || track.durationMS < minDurationMs) {
+    if (!startTime || !track.durationMS || track.durationMS <= minDurationMs) {
         guildTrackStartTimes.delete(guildId)
         return
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!startTime || !track.durationMS || track.durationMS < minDurationMs) {
guildTrackStartTimes.delete(guildId)
return
if (!startTime || !track.durationMS || track.durationMS <= minDurationMs) {
guildTrackStartTimes.delete(guildId)
return
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/bot/src/handlers/player/trackHandlers.ts` around lines 76 - 78, The
duration gate currently uses `track.durationMS < minDurationMs`, which lets
tracks exactly equal to 20_000ms slip through; change the condition to
`track.durationMS <= minDurationMs` so tracks of 20.0s or shorter are excluded;
update the early-return branch around `startTime`, `track.durationMS`, and
`minDurationMs` (the block that calls `guildTrackStartTimes.delete(guildId)` in
trackHandlers.ts) to use `<=` instead of `<`.

}
const playedMs = Date.now() - startTime
const ratio = playedMs / track.durationMS
guildTrackStartTimes.delete(guildId)
const shouldRecord =
(feedbackType === 'implicit_like' && ratio > ratioThreshold) ||
(feedbackType === 'implicit_dislike' && ratio < ratioThreshold)
if (shouldRecord) {
const trackKey = normalizeTrackKeyForFeedback(track.title, track.author)
await recommendationFeedbackService.recordImplicitFeedback(
requestedById,
trackKey,
feedbackType,
)
}
}

function evictOldEntries(): void {
if (lastPlayedTracks.size > MAX_GUILD_ENTRIES) {
const oldest = lastPlayedTracks.keys().next().value
Expand Down Expand Up @@ -162,6 +208,7 @@ const handlePlayerStart = async (
): Promise<void> => {
try {
evictOldEntries()
guildTrackStartTimes.set(queue.guild.id, Date.now())
infoLog({
message: `Started playing "${track.title}" in ${queue.guild.name}`,
})
Expand Down Expand Up @@ -216,6 +263,21 @@ const handlePlayerFinish = async (
): Promise<void> => {
try {
await scrobbleAndRecord(queue, track)

if (track) {
const requesterId = track.requestedBy?.id
?? (track.metadata as { requestedById?: string } | undefined)
?.requestedById
await recordPlayBehavior(
queue.guild.id,
track,
requesterId,
0,
0.8,
'implicit_like',
)
}
Comment on lines 265 to +279

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Decouple implicit feedback from scrobble/history failures.

Both handlers await scrobbleAndRecord(...) before recordPlayBehavior(...) inside the same outer try. A transient Last.fm/history error now drops the implicit like/dislike signal entirely, and a Redis write failure from recordPlayBehavior(...) can abort replenish/snapshot/cleanup for the whole event. These side effects should fail independently and only log.

Also applies to: 314-326

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/bot/src/handlers/player/trackHandlers.ts` around lines 265 - 279,
The scrobbleAndRecord and recordPlayBehavior calls are currently awaited inside
the same try so failures in one can abort the other and downstream cleanup;
split them so each runs independently and failures only log: call
scrobbleAndRecord(queue, track) and recordPlayBehavior(queue.guild.id, track,
requesterId, 0, 0.8, 'implicit_like') in separate try/catch blocks (or use
Promise.allSettled) so exceptions from scrobbleAndRecord do not prevent
recordPlayBehavior and vice versa, log errors using the existing logger instead
of rethrowing, and apply the same change to the duplicate block around the
recordPlayBehavior usage at the other occurrence (the block at ~314-326).


if (musicWatchdogService.isIntentionalStop(queue.guild.id)) return
await replenishIfAutoplay(queue, track)
await musicSessionSnapshotService.saveSnapshot(queue)
Expand Down Expand Up @@ -248,6 +310,21 @@ const handlePlayerSkip = async (
},
})
await scrobbleAndRecord(queue, track)

if (track) {
const requesterId = track.requestedBy?.id
?? (track.metadata as { requestedById?: string } | undefined)
?.requestedById
await recordPlayBehavior(
queue.guild.id,
track,
requesterId,
20_000,
0.3,
'implicit_dislike',
)
}

if (musicWatchdogService.isIntentionalStop(queue.guild.id)) return
await replenishIfAutoplay(queue, track)
await musicSessionSnapshotService.saveSnapshot(queue)
Expand Down
1 change: 1 addition & 0 deletions packages/bot/src/lastfm/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export {
getRecentTracks,
getSimilarTracks,
getTagTopTracks,
getLovedTracks,
isLastFmInvalidSessionError,
normalizeLastFmArtist,
normalizeLastFmTitle,
Expand Down
42 changes: 42 additions & 0 deletions packages/bot/src/lastfm/lastFmApi.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
getRecentTracks,
getSimilarTracks,
getTagTopTracks,
getLovedTracks,
} from './lastFmApi'

const getSessionKeyMock =
Expand Down Expand Up @@ -494,4 +495,45 @@ describe('lastFmApi', () => {
expect(tracks).toEqual([])
})
})

describe('getLovedTracks', () => {
it('returns loved tracks array on success', async () => {
fetchMock.mockResolvedValueOnce({
ok: true,
json: async () => ({
lovedtracks: {
track: [
{ name: 'Loved Song', artist: { name: 'Artist A' } },
{ name: 'Another Fave', artist: { name: 'Artist B' } },
],
},
}),
})

const result = await getLovedTracks('testuser', 10)

expect(result).toEqual([
{ artist: 'Artist A', title: 'Loved Song' },
{ artist: 'Artist B', title: 'Another Fave' },
])
})

it('returns empty array on non-ok response', async () => {
fetchMock.mockResolvedValueOnce({ ok: false })
const result = await getLovedTracks('testuser')
expect(result).toEqual([])
})
Comment on lines +521 to +525

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Harden the non-OK test; current setup can pass for the wrong reason.

At Line 521, { ok: false } without json() makes this pass via thrown response.json + catch, so it does not prove explicit non-OK handling.

Suggested test fix
 it('returns empty array on non-ok response', async () => {
-    fetchMock.mockResolvedValueOnce({ ok: false })
+    fetchMock.mockResolvedValueOnce({
+        ok: false,
+        json: async () => ({
+            lovedtracks: {
+                track: [{ name: 'Should Not Be Returned', artist: { name: 'Ignored' } }],
+            },
+        }),
+    })
     const result = await getLovedTracks('testuser')
     expect(result).toEqual([])
 })

This will also expose the missing response.ok guard in packages/bot/src/lastfm/lastFmApi.ts (Line 283-308).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('returns empty array on non-ok response', async () => {
fetchMock.mockResolvedValueOnce({ ok: false })
const result = await getLovedTracks('testuser')
expect(result).toEqual([])
})
it('returns empty array on non-ok response', async () => {
fetchMock.mockResolvedValueOnce({
ok: false,
json: async () => ({
lovedtracks: {
track: [{ name: 'Should Not Be Returned', artist: { name: 'Ignored' } }],
},
}),
})
const result = await getLovedTracks('testuser')
expect(result).toEqual([])
})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/bot/src/lastfm/lastFmApi.spec.ts` around lines 521 - 525, The test
for non-OK responses is fragile because it relied on fetch returning { ok: false
} without a json() implementation; update the test in lastFmApi.spec.ts to mock
fetch to resolve to an object with ok: false, a realistic status (e.g., 500),
and a json() function (e.g., jest.fn(() => Promise.resolve({ error: 'err' })))
so the test fails only if non-OK handling is missing, and assert
getLovedTracks('testuser') returns []; then fix the production code in
lastFmApi.ts by adding an explicit guard in getLovedTracks that checks
response.ok and immediately returns [] (or the appropriate empty result) before
calling response.json(), ensuring non-OK responses are handled
deterministically.


it('returns empty array when lovedtracks missing', async () => {
fetchMock.mockResolvedValueOnce({ ok: true, json: async () => ({}) })
const result = await getLovedTracks('testuser')
expect(result).toEqual([])
})

it('returns empty array on fetch error', async () => {
fetchMock.mockRejectedValueOnce(new Error('network'))
const result = await getLovedTracks('testuser')
expect(result).toEqual([])
})
})
})
27 changes: 27 additions & 0 deletions packages/bot/src/lastfm/lastFmApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,3 +279,30 @@ export async function getTagTopTracks(
return []
}
}

export async function getLovedTracks(
username: string,
limit = 50,
): Promise<{ artist: string; title: string }[]> {
const config = getApiConfig()
if (!config) return []
try {
const response = await fetch(
`${API_BASE}?method=user.getlovedtracks&user=${encodeURIComponent(username)}&limit=${limit}&format=json&api_key=${config.apiKey}`,
)
const data = (await response.json()) as {
lovedtracks?: {
track?: Array<{
name: string
artist: { name: string }
}>
}
}
return (data.lovedtracks?.track ?? []).map((t) => ({
artist: t.artist.name,
title: t.name,
}))
} catch {
return []
}
}
Loading
Loading