Repository navigation
feat(bot): last.fm duration fix, normalizers, and top tracks seeds #382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| export { | ||
| isLastFmConfigured, | ||
| getSessionKeyForUser, | ||
| getTopTracks, | ||
| normalizeLastFmArtist, | ||
| normalizeLastFmTitle, | ||
| updateNowPlaying, | ||
| scrobble, | ||
| } from './lastFmApi' | ||
| export type { LastFmTopTrack, LastFmPeriod } from './lastFmApi' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,6 +81,64 @@ | |
| } | ||
| } | ||
|
|
||
| const TOPIC_SUFFIX = / - topic$/i | ||
| const ARTIST_SEPARATORS = /\s*[,/]\s*/ | ||
| const TITLE_NOISE_PARENS = | ||
| /\s*[([](official\s*(music\s*)?video|official\s*audio|audio|lyric\s*video|lyrics?|live|hd|4k|ft\.?[^)\]]*|feat\.?[^)\]]*)[)\]]/gi | ||
|
Check warning on line 87 in packages/bot/src/lastfm/lastFmApi.ts
|
||
| const FEAT_CLAUSE = /\s*[\[(]?feat\.?\s+[^\])[]+[\])]?/gi | ||
|
Check warning on line 88 in packages/bot/src/lastfm/lastFmApi.ts
|
||
|
|
||
| export function normalizeLastFmArtist(raw: string): string { | ||
| return raw.replace(TOPIC_SUFFIX, '').split(ARTIST_SEPARATORS)[0].trim() | ||
| } | ||
|
|
||
| export function normalizeLastFmTitle(raw: string): string { | ||
| return raw.replace(TITLE_NOISE_PARENS, '').replace(FEAT_CLAUSE, '').trim() | ||
|
Check warning on line 95 in packages/bot/src/lastfm/lastFmApi.ts
|
||
| } | ||
|
|
||
| export type LastFmTopTrack = { | ||
| artist: string | ||
| title: string | ||
| playCount: number | ||
| } | ||
| export type LastFmPeriod = '7day' | '1month' | '3month' | '6month' | '12month' | ||
|
|
||
| export async function getTopTracks( | ||
| lastFmUsername: string, | ||
| period: LastFmPeriod = '3month', | ||
| limit = 20, | ||
| ): Promise<LastFmTopTrack[]> { | ||
| const config = getApiConfig() | ||
| if (!config) return [] | ||
| const params = new URLSearchParams({ | ||
| method: 'user.getTopTracks', | ||
| user: lastFmUsername, | ||
| period, | ||
| limit: String(limit), | ||
| api_key: config.apiKey, | ||
| format: 'json', | ||
| }) | ||
| try { | ||
| const res = await fetch(`${API_BASE}?${params.toString()}`) | ||
| if (!res.ok) return [] | ||
| const data = (await res.json()) as { | ||
| toptracks?: { | ||
| track?: Array<{ | ||
| name: string | ||
| artist: { name: string } | ||
| playcount: string | ||
| }> | ||
| } | ||
| } | ||
| return (data.toptracks?.track ?? []).map((t) => ({ | ||
| artist: t.artist.name, | ||
| title: t.name, | ||
| playCount: parseInt(t.playcount, 10) || 0, | ||
|
Check warning on line 135 in packages/bot/src/lastfm/lastFmApi.ts
|
||
| })) | ||
| } catch { | ||
| return [] | ||
|
Comment on lines
+120
to
+138
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don’t cache transient Last.fm failures as “no top tracks.” Right now every non-OK/JSON/network failure becomes Proposed fix try {
const res = await fetch(`${API_BASE}?${params.toString()}`)
- if (!res.ok) return []
+ if (!res.ok) {
+ throw new Error(
+ `Last.fm user.getTopTracks failed with ${res.status}`,
+ )
+ }
const data = (await res.json()) as {
toptracks?: {
track?: Array<{
@@
return (data.toptracks?.track ?? []).map((t) => ({
artist: t.artist.name,
title: t.name,
playCount: parseInt(t.playcount, 10) || 0,
}))
- } catch {
- return []
+ } catch (error) {
+ throw new Error('Last.fm user.getTopTracks failed', { cause: error })
}
}🧰 Tools🪛 GitHub Check: SonarCloud Code Analysis[warning] 135-135: Prefer 🤖 Prompt for AI Agents |
||
| } | ||
|
Comment on lines
+120
to
+139
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add timeout to prevent dangling requests. The 🛡️ Proposed fix with AbortSignal timeout try {
- const res = await fetch(`${API_BASE}?${params.toString()}`)
+ const res = await fetch(`${API_BASE}?${params.toString()}`, {
+ signal: AbortSignal.timeout(10000),
+ })
if (!res.ok) return []As per coding guidelines: "Set timeouts and handle aborts/cancellations; avoid dangling requests in API/network code." 🧰 Tools🪛 GitHub Check: SonarCloud Code Analysis[warning] 135-135: Prefer 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| export async function updateNowPlaying( | ||
| artist: string, | ||
| track: string, | ||
|
|
@@ -89,8 +147,8 @@ | |
| ): Promise<void> { | ||
| if (!sessionKey || !getApiConfig()) return | ||
| const params: Record<string, string> = { | ||
| artist: artist.trim(), | ||
| track: track.trim(), | ||
| artist: normalizeLastFmArtist(artist), | ||
| track: normalizeLastFmTitle(track), | ||
| } | ||
| if (durationSec != null && durationSec > 0) { | ||
| params.duration = String(Math.round(durationSec)) | ||
|
|
@@ -107,8 +165,8 @@ | |
| ): Promise<void> { | ||
| if (!sessionKey || !getApiConfig()) return | ||
| const params: Record<string, string> = { | ||
| artist: artist.trim(), | ||
| track: track.trim(), | ||
| artist: normalizeLastFmArtist(artist), | ||
| track: normalizeLastFmTitle(track), | ||
| timestamp: String(Math.floor(timestamp)), | ||
| } | ||
| if (durationSec != null && durationSec > 0) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| import { | ||
| afterEach, | ||
| beforeEach, | ||
| describe, | ||
| expect, | ||
| it, | ||
| jest, | ||
| } from '@jest/globals' | ||
| import { getLastFmSeedTracks } from './lastFmSeeds' | ||
|
|
||
| const getByDiscordIdMock = jest.fn() | ||
| const getTopTracksMock = jest.fn() | ||
|
|
||
| jest.mock('@lucky/shared/services', () => ({ | ||
| lastFmLinkService: { | ||
| getByDiscordId: (...args: unknown[]) => getByDiscordIdMock(...args), | ||
| }, | ||
| })) | ||
|
|
||
| jest.mock('@lucky/shared/utils', () => ({ | ||
| debugLog: jest.fn(), | ||
| errorLog: jest.fn(), | ||
| })) | ||
|
|
||
| jest.mock('../../../lastfm', () => ({ | ||
| getTopTracks: (...args: unknown[]) => getTopTracksMock(...args), | ||
| })) | ||
|
|
||
| describe('getLastFmSeedTracks', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks() | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| jest.clearAllMocks() | ||
| }) | ||
|
|
||
| it('returns mapped tracks when user has a Last.fm link', async () => { | ||
| getByDiscordIdMock.mockResolvedValue({ lastFmUsername: 'user123' }) | ||
| getTopTracksMock.mockResolvedValue([ | ||
| { artist: 'Artist A', title: 'Song A', playCount: 10 }, | ||
| { artist: 'Artist B', title: 'Song B', playCount: 5 }, | ||
| ]) | ||
|
|
||
| const tracks = await getLastFmSeedTracks('discord-user-1') | ||
|
|
||
| expect(tracks).toEqual([ | ||
| { artist: 'Artist A', title: 'Song A' }, | ||
| { artist: 'Artist B', title: 'Song B' }, | ||
| ]) | ||
| expect(getTopTracksMock).toHaveBeenCalledWith('user123', '3month', 20) | ||
| }) | ||
|
|
||
| it('returns empty array when user has no Last.fm link', async () => { | ||
| getByDiscordIdMock.mockResolvedValue(null) | ||
|
|
||
| const tracks = await getLastFmSeedTracks('discord-user-2') | ||
|
|
||
| expect(tracks).toEqual([]) | ||
| expect(getTopTracksMock).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('returns empty array when link has no lastFmUsername', async () => { | ||
| getByDiscordIdMock.mockResolvedValue({ lastFmUsername: null }) | ||
|
|
||
| const tracks = await getLastFmSeedTracks('discord-user-3') | ||
|
|
||
| expect(tracks).toEqual([]) | ||
| }) | ||
|
|
||
| it('returns cached result on second call within TTL', async () => { | ||
| getByDiscordIdMock.mockResolvedValue({ lastFmUsername: 'cached-user' }) | ||
| getTopTracksMock.mockResolvedValue([ | ||
| { artist: 'Artist C', title: 'Song C', playCount: 3 }, | ||
| ]) | ||
|
|
||
| const first = await getLastFmSeedTracks('discord-user-cache') | ||
| const second = await getLastFmSeedTracks('discord-user-cache') | ||
|
|
||
| expect(first).toEqual(second) | ||
| expect(getTopTracksMock).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
||
| it('returns empty array when getTopTracks throws', async () => { | ||
| getByDiscordIdMock.mockResolvedValue({ lastFmUsername: 'erruser' }) | ||
| getTopTracksMock.mockRejectedValue(new Error('API error')) | ||
|
|
||
| const tracks = await getLastFmSeedTracks('discord-user-err') | ||
|
|
||
| expect(tracks).toEqual([]) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new normalizers rewrite canonical artist/title metadata.
Splitting on raw
,//turns valid artists likeTyler, The CreatorandAC/DCinto bad scrobbles, and stripping(Live)collapses a distinct release into the studio track. These helpers sit on every now-playing/scrobble path, so the bad metadata also feeds back into the user’s Last.fm top tracks and autoplay seeds.Proposed fix
If you still want collaborator collapsing, it needs to happen from structured provider metadata upstream rather than by splitting raw display strings.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 95-95: Prefer
String#replaceAll()overString#replace().See more on https://sonarcloud.io/project/issues?id=LucasSantana-Dev_Nexus&issues=AZ1AEYVJgetBJX8TlZ9L&open=AZ1AEYVJgetBJX8TlZ9L&pullRequest=382
[warning] 87-87: Simplify this regular expression to reduce its complexity from 35 to the 20 allowed.
See more on https://sonarcloud.io/project/issues?id=LucasSantana-Dev_Nexus&issues=AZ1AEYVJgetBJX8TlZ9I&open=AZ1AEYVJgetBJX8TlZ9I&pullRequest=382
[warning] 88-88: Unnecessary escape character: [.
See more on https://sonarcloud.io/project/issues?id=LucasSantana-Dev_Nexus&issues=AZ1AEYVJgetBJX8TlZ9K&open=AZ1AEYVJgetBJX8TlZ9K&pullRequest=382
[warning] 95-95: Prefer
String#replaceAll()overString#replace().See more on https://sonarcloud.io/project/issues?id=LucasSantana-Dev_Nexus&issues=AZ1AEYVJgetBJX8TlZ9M&open=AZ1AEYVJgetBJX8TlZ9M&pullRequest=382
🤖 Prompt for AI Agents