Repository navigation
feat(bot): intelligent autoplay — skip/completion signals, loved tracks, artist frequency, mood matching - #577
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughAdds per-user implicit like/dislike recording from player events, Redis-backed implicit-feedback APIs with TTL/size cap, Spotify audio-feature lookup and caching, Last.fm “loved tracks” ingestion for seeds, and integrates these signals into queue replenishment and recommendation scoring. Changes
Sequence Diagram(s)sequenceDiagram
participant Player as Music Player
participant Handler as Track Handler
participant Feedback as RecommendationFeedbackService
participant Redis as Redis Storage
participant Queue as Queue Manager
participant Spotify as Spotify API
autonumber
rect rgba(100, 150, 200, 0.5)
Note over Player,Handler: Implicit feedback recording
Player->>Handler: playerStart(guildId, track)
Handler->>Redis: store startTime[guildId] = now
Player->>Handler: playerFinish/playerSkip(guildId, track)
Handler->>Handler: compute playedMs / durationMS
Handler->>Feedback: recordImplicitFeedback(userId, trackKey, implicit_like/implicit_dislike)
Feedback->>Redis: load/persist implicit map (setex 14d, cap 200)
Handler->>Redis: delete startTime[guildId]
end
rect rgba(150, 200, 100, 0.5)
Note over Queue,Spotify: Replenish & scoring
Queue->>Feedback: getImplicitLikeKeys(userId)
Queue->>Feedback: getImplicitDislikeKeys(userId)
Queue->>Queue: buildArtistFrequency(history)
Queue->>Spotify: searchSpotifyTrack/getAudioFeatures (as needed)
Spotify->>Queue: return SpotifyAudioFeatures (cached)
Queue->>Queue: calculateRecommendationScore(candidate, ..., artistFrequency, implicitLikeKeys, implicitDislikeKeys)
Queue->>Queue: select top candidates for enqueue
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bot/src/utils/music/queueManipulation.ts (1)
935-947:⚠️ Potential issue | 🔴 CriticalKeep the blocked-artist hard stop in these paths too.
Both call sites upsert the candidate even when
calculateRecommendationScore()returns-Infinity. That breaks the explicit “blocked artist = never” rule when the pool is thin.Suggested fix
const rec = calculateRecommendationScore( track, currentTrack, recentArtists, likedTrackKeys, @@ implicitDislikeKeys, implicitLikeKeys, ) + if (rec.score === -Infinity) continue upsertScoredCandidate(candidates, track, { score: (rec.score + LASTFM_SCORE_BOOST) * (s.match / 100), reason: rec.reason ? `${rec.reason} • similar to your taste` : 'similar to your taste', @@ const rec = calculateRecommendationScore( track, ctx.currentTrack, ctx.recentArtists, ctx.likedTrackKeys, @@ ctx.artistFrequency, ctx.implicitDislikeKeys, ctx.implicitLikeKeys, ) + if (rec.score === -Infinity) return upsertScoredCandidate(ctx.candidates, track, { score: rec.score + GENRE_SCORE_BOOST, reason: rec.reason ? `${rec.reason} • ${tag} vibes` : `${tag} vibes`, }) }Also applies to: 1019-1034
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/queueManipulation.ts` around lines 935 - 947, The code currently upserts candidates even when calculateRecommendationScore(...) returns -Infinity, which violates the "blocked artist = never" rule; update both call sites where calculateRecommendationScore is used (the one that computes rec and then calls upsertScoredCandidate(candidates, track, ...)) to check the returned score and skip the upsert when rec === -Infinity (or isFinite(rec) is false), ensuring blocked artists are never added to candidates; apply the same guard to the other identical call site where rec is computed (also referenced in this file) so upsertScoredCandidate is only invoked for finite scores.
🧹 Nitpick comments (7)
packages/bot/src/utils/music/queueManipulation.spec.ts (1)
2114-2126: Missing implicit feedback mock initialization in this describe block.Unlike other
beforeEachblocks (lines 152-153, 1921-1922, etc.), this one doesn't initializegetImplicitDislikeKeysMockandgetImplicitLikeKeysMock. While the tests likely still pass due to Jest's mock behavior, adding the initialization would be consistent with other blocks.🔧 Proposed fix for consistency
beforeEach(() => { dislikedTrackKeysMock.mockResolvedValue(new Set()) likedTrackKeysMock.mockResolvedValue(new Set()) getPreferredArtistKeysMock.mockResolvedValue(new Set()) getBlockedArtistKeysMock.mockResolvedValue(new Set()) + getImplicitDislikeKeysMock.mockResolvedValue(new Set()) + getImplicitLikeKeysMock.mockResolvedValue(new Set()) consumeLastFmSeedSliceMock.mockResolvedValue([])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/queueManipulation.spec.ts` around lines 2114 - 2126, In the describe block for "queueManipulation.addSelectedTracks async writes" add initialization for the implicit feedback mocks: call getImplicitDislikeKeysMock.mockResolvedValue(new Set()) and getImplicitLikeKeysMock.mockResolvedValue(new Set()) in the beforeEach so they match other blocks; locate the beforeEach in that describe and add these two mockResolvedValue(new Set()) calls alongside the existing mocks like dislikedTrackKeysMock and likedTrackKeysMock.packages/bot/src/handlers/player/trackHandlers.ts (3)
54-64: Duplicate normalization logic withfeedbackService.buildTrackKey.This function duplicates the track key normalization logic from
RecommendationFeedbackService.buildTrackKey(lines 56-67 in feedbackService.ts). Both perform the same lowercase alphanumeric normalization with::delimiter. Consider reusing the service method to avoid drift.♻️ Proposed refactor to reuse service method
-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}` -}Then use
recommendationFeedbackService.buildTrackKey(cleanTitle(track.title), cleanAuthor(track.author))at the call sites.🤖 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 54 - 64, The normalizeTrackKeyForFeedback function duplicates RecommendationFeedbackService.buildTrackKey's logic; remove the local function and replace its call sites to use the service method instead (e.g., recommendationFeedbackService.buildTrackKey(cleanTitle(track.title), cleanAuthor(track.author))). Ensure the service instance (recommendationFeedbackService or RecommendationFeedbackService) is imported/available in the module and adapt any call signatures to pass the cleaned title and author strings so behavior remains identical.
242-244: Duplicate requester extraction logic.The inline requester lookup duplicates the existing
getTrackRequesterIdhelper defined at lines 42-45. Consider reusing it:♻️ Proposed refactor
- const requesterId = track.requestedBy?.id - ?? (track.metadata as { requestedById?: string } | undefined) - ?.requestedById + const requesterId = getTrackRequesterId(track)Also applies to: 299-301
🤖 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 242 - 244, The duplicate logic extracting the requesterId (using track.requestedBy?.id ?? (track.metadata as { requestedById?: string } | undefined)?.requestedById) should be replaced with the existing helper getTrackRequesterId; update the occurrences that set requesterId (e.g., the variable named requesterId around the track handling blocks) to call getTrackRequesterId(track) instead, removing the inline fallback extraction and keeping a single source of truth for requester resolution.
30-30: Potential memory growth for disconnected guilds.
guildTrackStartTimesentries are deleted on finish/skip, but if a guild disconnects mid-track without triggering these events, entries may accumulate. Consider adding cleanup ondisconnectoremptyChannelevents, or periodically evicting stale entries.🤖 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` at line 30, The guildTrackStartTimes map can leak if a guild disconnects mid-track; add cleanup logic that removes entries when a guild's voice session ends by hooking into the relevant disconnect/emptyChannel/voiceStateUpdate handlers (or your player disconnect/emptyChannel events) and calling guildTrackStartTimes.delete(guildId), and additionally implement a periodic eviction routine that scans guildTrackStartTimes and deletes entries older than a safe TTL (compare stored timestamp to Date.now()) to guard against missed events.packages/bot/src/utils/music/autoplay/lastFmSeeds.spec.ts (1)
107-127: Consider adding test coverage for loved tracks prioritization.The existing tests mock
getLovedTracksto[]but don't verify the new behavior where loved tracks are merged with highest priority. Consider adding a test case that verifies loved tracks appear before top/recent tracks in the merged result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/autoplay/lastFmSeeds.spec.ts` around lines 107 - 127, Add a new spec that mocks getByDiscordIdMock to return a lastFmUsername and stubs getLovedTracksMock to return one or more loved tracks, while keeping getTopTracksMock and getRecentTracksMock returning their sample tracks, then call getLastFmSeedTracks('discord-user-loved') and assert the loved track(s) appear first in the returned array (before top and recent tracks); reference and update the existing test helpers/mocks named getByDiscordIdMock, getLovedTracksMock, getTopTracksMock, getRecentTracksMock and the function under test getLastFmSeedTracks to implement this check.packages/bot/src/lastfm/lastFmApi.ts (1)
289-292: Missing HTTP status check before parsing response.Other API functions in this file (e.g.,
getTopTracksat line 128,getRecentTracksat line 203) checkres.okbefore parsing JSON. This function parses the response directly, which could lead to parsing errors on non-2xx responses instead of gracefully returning an empty array.🔧 Proposed fix to add status check
const response = await fetch( `${API_BASE}?method=user.getlovedtracks&user=${encodeURIComponent(username)}&limit=${limit}&format=json&api_key=${config.apiKey}`, ) + if (!response.ok) return [] const data = (await response.json()) as {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/lastfm/lastFmApi.ts` around lines 289 - 292, The getLovedTracks fetch call parses the body without checking HTTP status; update the getLovedTracks implementation so the response is checked (like getTopTracks/getRecentTracks): after awaiting the fetch (the response variable), if !response.ok return an empty array (or the function's empty result) and only call response.json() when response.ok is true; keep existing variable names (response) and downstream logic intact and optionally log the non-2xx status for debugging.packages/bot/src/services/musicRecommendation/feedbackService.ts (1)
337-338: Hardcoded TTL inconsistent with configurable explicit feedback TTL.The implicit feedback TTL is hardcoded to 14 days, while explicit feedback uses
this.ttlDays(configurable viaAUTOPLAY_FEEDBACK_TTL_DAYS). Consider extracting this to a separate configurable parameter or documenting why implicit feedback has a shorter, fixed TTL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/services/musicRecommendation/feedbackService.ts` around lines 337 - 338, The implicit feedback TTL is hardcoded to 14 days in feedbackService (the local ttlSeconds near getImplicitFeedbackRedisKey), causing inconsistency with explicit feedback which uses this.ttlDays (driven by AUTOPLAY_FEEDBACK_TTL_DAYS); update the implementation to use a configurable value instead—either reuse this.ttlDays for implicit feedback or add a new config property (e.g., this.implicitTtlDays) initialized from a new env/config entry, and replace the hardcoded 14*24*60*60 with the computed seconds from that property so both TTLs are configurable and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bot/src/spotify/spotifyApi.ts`:
- Around line 36-38: The current truthy check rejects valid energy values like
0; change the conditional that reads "!data?.energy || typeof data.valence !==
'number'" to perform a type check for energy instead (e.g., check typeof
data.energy !== 'number' while keeping the existing valence check) so that valid
numeric 0 values are accepted; locate the conditional using the data object and
update it accordingly in the function handling Spotify audio feature responses.
- Around line 14-22: Add timeout protection to Spotify fetches by using
AbortController in both getAudioFeatures and searchSpotifyTrack: create an
AbortController, start a setTimeout (e.g., 3000ms) that calls
controller.abort(), pass controller.signal into the fetch options (headers +
signal), and clear the timeout when the fetch resolves successfully; handle
abort errors appropriately (propagate or convert to a timeout error) so slow
Spotify responses won't block _replenishQueue().
In `@packages/bot/src/spotify/spotifyConfig.spec.ts`:
- Around line 45-50: The test in spotifyConfig.spec.ts is simulating a missing
env var by assigning process.env.SPOTIFY_REDIRECT_URI = undefined which leaves
the string "undefined" in the env; modify the test that calls
isSpotifyConfigured() to actually remove the variable using delete
process.env.SPOTIFY_REDIRECT_URI so the env truly is absent and the test intent
(missing redirect URI) is correctly represented.
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 43-59: The global audioFeatureCache is being populated with null
on user-specific token failures or transient lookup errors, causing permanent
misses for all users; in getTrackAudioFeatures (and the similar later block that
sets cache to null) only write to audioFeatureCache when you have a definitive
API result (i.e., non-null SpotifyAudioFeatures) or an explicit “not found”
response from Spotify, and avoid caching null when
spotifyLinkService.getValidAccessToken(userId) returns no token or when the
fetch fails transiently—so remove or guard the audioFeatureCache.set(cacheKey,
null) calls behind checks for definitive negative responses and only set the
cache after a successful features fetch.
- Around line 1383-1386: The current block always awards +0.08 for any
Spotify→Spotify pair and labels it "spotify mood match" without checking audio
features; change it to first check the actual Spotify audio feature fields
(e.g., candidate.audioFeatures and currentTrack.audioFeatures — valence, energy,
tempo, etc.) and only add the +0.08 and push the "spotify mood match" reason
when the feature-distance (choose a small threshold on valence/energy or a
combined distance) indicates mood similarity; if you still want a smaller source
bias, use a separate smaller score bump with a different reason like "spotify
source match" only when features are absent. Target the code that references
candidate.source, currentTrack.source, score, and reasons in
queueManipulation.ts.
---
Outside diff comments:
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 935-947: The code currently upserts candidates even when
calculateRecommendationScore(...) returns -Infinity, which violates the "blocked
artist = never" rule; update both call sites where calculateRecommendationScore
is used (the one that computes rec and then calls
upsertScoredCandidate(candidates, track, ...)) to check the returned score and
skip the upsert when rec === -Infinity (or isFinite(rec) is false), ensuring
blocked artists are never added to candidates; apply the same guard to the other
identical call site where rec is computed (also referenced in this file) so
upsertScoredCandidate is only invoked for finite scores.
---
Nitpick comments:
In `@packages/bot/src/handlers/player/trackHandlers.ts`:
- Around line 54-64: The normalizeTrackKeyForFeedback function duplicates
RecommendationFeedbackService.buildTrackKey's logic; remove the local function
and replace its call sites to use the service method instead (e.g.,
recommendationFeedbackService.buildTrackKey(cleanTitle(track.title),
cleanAuthor(track.author))). Ensure the service instance
(recommendationFeedbackService or RecommendationFeedbackService) is
imported/available in the module and adapt any call signatures to pass the
cleaned title and author strings so behavior remains identical.
- Around line 242-244: The duplicate logic extracting the requesterId (using
track.requestedBy?.id ?? (track.metadata as { requestedById?: string } |
undefined)?.requestedById) should be replaced with the existing helper
getTrackRequesterId; update the occurrences that set requesterId (e.g., the
variable named requesterId around the track handling blocks) to call
getTrackRequesterId(track) instead, removing the inline fallback extraction and
keeping a single source of truth for requester resolution.
- Line 30: The guildTrackStartTimes map can leak if a guild disconnects
mid-track; add cleanup logic that removes entries when a guild's voice session
ends by hooking into the relevant disconnect/emptyChannel/voiceStateUpdate
handlers (or your player disconnect/emptyChannel events) and calling
guildTrackStartTimes.delete(guildId), and additionally implement a periodic
eviction routine that scans guildTrackStartTimes and deletes entries older than
a safe TTL (compare stored timestamp to Date.now()) to guard against missed
events.
In `@packages/bot/src/lastfm/lastFmApi.ts`:
- Around line 289-292: The getLovedTracks fetch call parses the body without
checking HTTP status; update the getLovedTracks implementation so the response
is checked (like getTopTracks/getRecentTracks): after awaiting the fetch (the
response variable), if !response.ok return an empty array (or the function's
empty result) and only call response.json() when response.ok is true; keep
existing variable names (response) and downstream logic intact and optionally
log the non-2xx status for debugging.
In `@packages/bot/src/services/musicRecommendation/feedbackService.ts`:
- Around line 337-338: The implicit feedback TTL is hardcoded to 14 days in
feedbackService (the local ttlSeconds near getImplicitFeedbackRedisKey), causing
inconsistency with explicit feedback which uses this.ttlDays (driven by
AUTOPLAY_FEEDBACK_TTL_DAYS); update the implementation to use a configurable
value instead—either reuse this.ttlDays for implicit feedback or add a new
config property (e.g., this.implicitTtlDays) initialized from a new env/config
entry, and replace the hardcoded 14*24*60*60 with the computed seconds from that
property so both TTLs are configurable and consistent.
In `@packages/bot/src/utils/music/autoplay/lastFmSeeds.spec.ts`:
- Around line 107-127: Add a new spec that mocks getByDiscordIdMock to return a
lastFmUsername and stubs getLovedTracksMock to return one or more loved tracks,
while keeping getTopTracksMock and getRecentTracksMock returning their sample
tracks, then call getLastFmSeedTracks('discord-user-loved') and assert the loved
track(s) appear first in the returned array (before top and recent tracks);
reference and update the existing test helpers/mocks named getByDiscordIdMock,
getLovedTracksMock, getTopTracksMock, getRecentTracksMock and the function under
test getLastFmSeedTracks to implement this check.
In `@packages/bot/src/utils/music/queueManipulation.spec.ts`:
- Around line 2114-2126: In the describe block for
"queueManipulation.addSelectedTracks async writes" add initialization for the
implicit feedback mocks: call getImplicitDislikeKeysMock.mockResolvedValue(new
Set()) and getImplicitLikeKeysMock.mockResolvedValue(new Set()) in the
beforeEach so they match other blocks; locate the beforeEach in that describe
and add these two mockResolvedValue(new Set()) calls alongside the existing
mocks like dislikedTrackKeysMock and likedTrackKeysMock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 072d8f0e-0cc7-4b25-b447-4d5a339f5dc3
📒 Files selected for processing (11)
packages/bot/src/handlers/player/trackHandlers.tspackages/bot/src/lastfm/index.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/services/musicRecommendation/feedbackService.tspackages/bot/src/spotify/spotifyApi.tspackages/bot/src/spotify/spotifyConfig.spec.tspackages/bot/src/spotify/spotifyConfig.tspackages/bot/src/utils/music/autoplay/lastFmSeeds.spec.tspackages/bot/src/utils/music/autoplay/lastFmSeeds.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/utils/music/queueManipulation.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarCloud Scan
- GitHub Check: Quality Gates
🔇 Additional comments (6)
packages/bot/src/lastfm/index.ts (1)
8-8: LGTM!The re-export of
getLovedTracksfollows the established pattern for exposing Last.fm API functions from the module's public interface.packages/bot/src/utils/music/autoplay/lastFmSeeds.ts (1)
53-63: LGTM!The implementation correctly fetches loved tracks in parallel with existing seed sources and merges them with highest priority before deduplication, as intended by the PR objectives.
packages/bot/src/services/musicRecommendation/feedbackService.ts (1)
343-374: LGTM!The
recordImplicitFeedbackimplementation correctly handles concurrent updates, enforces the 200-entry cap with LRU-style trimming byupdatedAt, and has proper error handling that logs failures without throwing.packages/bot/src/spotify/spotifyConfig.ts (1)
1-6: LGTM!The simplified check correctly requires only
SPOTIFY_CLIENT_IDandSPOTIFY_CLIENT_SECRET, aligning with the PR objective thatSPOTIFY_REDIRECT_URIis now derived fromWEBAPP_BACKEND_URLinstead of being required.packages/bot/src/utils/music/autoplay/lastFmSeeds.spec.ts (1)
22-22: LGTM - Mock setup is correct.The mock for
getLovedTracksis properly wired and initialized to[]in all relevantbeforeEachblocks, ensuring existing tests remain unaffected.Also applies to: 38-38, 44-44
packages/bot/src/utils/music/queueManipulation.spec.ts (1)
90-106: LGTM!The new mocks for implicit feedback methods are correctly defined and wired into the
recommendationFeedbackServicemock, enabling tests to control implicit like/dislike behavior.
| const res = await fetch( | ||
| `https://api.spotify.com/v1/audio-features/${spotifyTrackId}`, | ||
| { | ||
| method: 'GET', | ||
| headers: { | ||
| Authorization: `Bearer ${accessToken}`, | ||
| }, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/bot/src/spotify/spotifyApi.tsRepository: LucasSantana-Dev/Lucky
Length of output: 2808
🏁 Script executed:
rg "getAudioFeatures|searchSpotifyTrack" --type ts --type tsx -B 2 -A 2Repository: LucasSantana-Dev/Lucky
Length of output: 93
🏁 Script executed:
rg "getAudioFeatures|searchSpotifyTrack" -B 2 -A 2Repository: LucasSantana-Dev/Lucky
Length of output: 1894
🏁 Script executed:
cat -n packages/bot/src/utils/music/queueManipulation.tsRepository: LucasSantana-Dev/Lucky
Length of output: 50378
🏁 Script executed:
sed -n '45,87p' packages/bot/src/utils/music/queueManipulation.tsRepository: LucasSantana-Dev/Lucky
Length of output: 1232
Add a timeout to both Spotify API requests to prevent slow responses from blocking queue replenishment.
These calls are used during autoplay recommendation scoring. Without an abort/timeout, a slow Spotify response can block the entire _replenishQueue() process and stall queue refill for autoplay.
Both getAudioFeatures() and searchSpotifyTrack() should implement timeout protection using AbortController with a reasonable timeout (e.g., 3000ms).
Suggested direction
+const SPOTIFY_FETCH_TIMEOUT_MS = 3000
+
export async function getAudioFeatures(
accessToken: string,
spotifyTrackId: string,
): Promise<SpotifyAudioFeatures | null> {
try {
- const res = await fetch(
- `https://api.spotify.com/v1/audio-features/${spotifyTrackId}`,
- {
- method: 'GET',
- headers: {
- Authorization: `Bearer ${accessToken}`,
- },
- },
- )
+ const controller = new AbortController()
+ const timeout = setTimeout(() => controller.abort(), SPOTIFY_FETCH_TIMEOUT_MS)
+ const res = await fetch(
+ `https://api.spotify.com/v1/audio-features/${spotifyTrackId}`,
+ {
+ method: 'GET',
+ headers: {
+ Authorization: `Bearer ${accessToken}`,
+ },
+ signal: controller.signal,
+ },
+ ).finally(() => clearTimeout(timeout))📝 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.
| const res = await fetch( | |
| `https://api.spotify.com/v1/audio-features/${spotifyTrackId}`, | |
| { | |
| method: 'GET', | |
| headers: { | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| }, | |
| ) | |
| const controller = new AbortController() | |
| const timeout = setTimeout(() => controller.abort(), SPOTIFY_FETCH_TIMEOUT_MS) | |
| const res = await fetch( | |
| `https://api.spotify.com/v1/audio-features/${spotifyTrackId}`, | |
| { | |
| method: 'GET', | |
| headers: { | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| signal: controller.signal, | |
| }, | |
| ).finally(() => clearTimeout(timeout)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/spotify/spotifyApi.ts` around lines 14 - 22, Add timeout
protection to Spotify fetches by using AbortController in both getAudioFeatures
and searchSpotifyTrack: create an AbortController, start a setTimeout (e.g.,
3000ms) that calls controller.abort(), pass controller.signal into the fetch
options (headers + signal), and clear the timeout when the fetch resolves
successfully; handle abort errors appropriately (propagate or convert to a
timeout error) so slow Spotify responses won't block _replenishQueue().
| if (!data?.energy || typeof data.valence !== 'number') { | ||
| return null | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Spotify's Audio Features API, what is the valid range for the energyfield, and is0 a valid value?
💡 Result:
The valid range for the energy field in Spotify's Audio Features API is 0.0 to 1.0. Yes, 0 is a valid value, representing the lowest possible energy (e.g., a track with minimal intensity and activity, such as a calm Bach prelude).
Citations:
- 1: https://developer.spotify.com/documentation/web-api/reference/get-audio-features
- 2: https://developer.spotify.com/documentation/web-api/reference/get-audio-features?id=11dFghVXANMlKmJXsNCbNl
- 3: https://spotifyweb.apidog.io/schema-458603
- 4: https://spotify-api.js.org/main/interface/AudioFeatures
- 5: https://hexdocs.pm/spotify_web_api/Spotify.AudioFeatures.html
- 6: https://spotify.js.org/interfaces/AudioFeatures.html
- 7: https://huggingface.co/datasets/ozefe/spotify_audio_features
🏁 Script executed:
cat -n packages/bot/src/spotify/spotifyApi.ts | head -50Repository: LucasSantana-Dev/Lucky
Length of output: 1652
Replace falsy check with type check for energy field.
Line 36-38: !data?.energy incorrectly rejects valid energy: 0 payloads. Spotify's Audio Features API returns energy values in the range 0.0–1.0, where 0 is a valid value representing the lowest energy level. This check should validate the field's type, not truthiness, consistent with how valence is validated on the same line.
Fix
- if (!data?.energy || typeof data.valence !== 'number') {
+ if (
+ typeof data?.energy !== 'number' ||
+ typeof data.valence !== 'number'
+ ) {
return null
}📝 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.
| if (!data?.energy || typeof data.valence !== 'number') { | |
| return null | |
| } | |
| if ( | |
| typeof data?.energy !== 'number' || | |
| typeof data.valence !== 'number' | |
| ) { | |
| return null | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/spotify/spotifyApi.ts` around lines 36 - 38, The current
truthy check rejects valid energy values like 0; change the conditional that
reads "!data?.energy || typeof data.valence !== 'number'" to perform a type
check for energy instead (e.g., check typeof data.energy !== 'number' while
keeping the existing valence check) so that valid numeric 0 values are accepted;
locate the conditional using the data object and update it accordingly in the
function handling Spotify audio feature responses.
| it('returns true when SPOTIFY_REDIRECT_URI is missing (optional)', () => { | ||
| process.env.SPOTIFY_CLIENT_ID = 'test-client-id' | ||
| process.env.SPOTIFY_CLIENT_SECRET = 'test-secret' | ||
| process.env.SPOTIFY_REDIRECT_URI = undefined | ||
|
|
||
| expect(isSpotifyConfigured()).toBe(false) | ||
| expect(isSpotifyConfigured()).toBe(true) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
process.env.SPOTIFY_REDIRECT_URI = 'https://example.com/callback'
process.env.SPOTIFY_REDIRECT_URI = undefined
console.log('after assignment:', JSON.stringify(process.env.SPOTIFY_REDIRECT_URI))
delete process.env.SPOTIFY_REDIRECT_URI
console.log('after delete:', JSON.stringify(process.env.SPOTIFY_REDIRECT_URI))
NODERepository: LucasSantana-Dev/Lucky
Length of output: 119
🏁 Script executed:
cat packages/bot/src/spotify/spotifyConfig.tsRepository: LucasSantana-Dev/Lucky
Length of output: 220
Use delete instead of assigning undefined.
Line 48 does not reliably simulate a missing env var in Node.js. Assigning undefined to process.env results in the string "undefined" remaining in the environment, while the test comment claims the redirect URI is "missing". Use delete to actually remove the variable and match the test intent.
Suggested fix
it('returns true when SPOTIFY_REDIRECT_URI is missing (optional)', () => {
process.env.SPOTIFY_CLIENT_ID = 'test-client-id'
process.env.SPOTIFY_CLIENT_SECRET = 'test-secret'
- process.env.SPOTIFY_REDIRECT_URI = undefined
+ delete process.env.SPOTIFY_REDIRECT_URI
expect(isSpotifyConfigured()).toBe(true)
})📝 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.
| it('returns true when SPOTIFY_REDIRECT_URI is missing (optional)', () => { | |
| process.env.SPOTIFY_CLIENT_ID = 'test-client-id' | |
| process.env.SPOTIFY_CLIENT_SECRET = 'test-secret' | |
| process.env.SPOTIFY_REDIRECT_URI = undefined | |
| expect(isSpotifyConfigured()).toBe(false) | |
| expect(isSpotifyConfigured()).toBe(true) | |
| it('returns true when SPOTIFY_REDIRECT_URI is missing (optional)', () => { | |
| process.env.SPOTIFY_CLIENT_ID = 'test-client-id' | |
| process.env.SPOTIFY_CLIENT_SECRET = 'test-secret' | |
| delete process.env.SPOTIFY_REDIRECT_URI | |
| expect(isSpotifyConfigured()).toBe(true) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/spotify/spotifyConfig.spec.ts` around lines 45 - 50, The
test in spotifyConfig.spec.ts is simulating a missing env var by assigning
process.env.SPOTIFY_REDIRECT_URI = undefined which leaves the string "undefined"
in the env; modify the test that calls isSpotifyConfigured() to actually remove
the variable using delete process.env.SPOTIFY_REDIRECT_URI so the env truly is
absent and the test intent (missing redirect URI) is correctly represented.
| const audioFeatureCache = new Map<string, SpotifyAudioFeatures | null>() | ||
|
|
||
| async function getTrackAudioFeatures( | ||
| track: Track, | ||
| userId: string, | ||
| ): Promise<SpotifyAudioFeatures | null> { | ||
| const cacheKey = normalizeTrackKey(track.title, track.author) | ||
|
|
||
| const cached = audioFeatureCache.get(cacheKey) | ||
| if (cached !== undefined) { | ||
| return cached | ||
| } | ||
|
|
||
| const token = await spotifyLinkService.getValidAccessToken(userId) | ||
| if (!token) { | ||
| audioFeatureCache.set(cacheKey, null) | ||
| return null |
There was a problem hiding this comment.
Don't cache user-specific failures in a global track cache.
This memoizes null by title/artist even when the miss happened because the current user had no Spotify token or because a lookup transiently failed. After that, every other user gets a permanent miss for the same track.
Suggested fix
async function getTrackAudioFeatures(
track: Track,
userId: string,
): Promise<SpotifyAudioFeatures | null> {
const cacheKey = normalizeTrackKey(track.title, track.author)
-
+
const cached = audioFeatureCache.get(cacheKey)
if (cached !== undefined) {
return cached
}
-
+
const token = await spotifyLinkService.getValidAccessToken(userId)
if (!token) {
- audioFeatureCache.set(cacheKey, null)
return null
}
-
+
let spotifyId: string | null = null
-
+
if (track.url && track.url.includes('open.spotify.com/track/')) {
const match = track.url.match(/track\/([a-zA-Z0-9]+)/)
if (match) {
spotifyId = match[1]
@@
- if (!spotifyId) {
- audioFeatureCache.set(cacheKey, null)
+ if (!spotifyId) {
return null
}
-
+
const features = await getAudioFeatures(token, spotifyId).catch(() => null)
- audioFeatureCache.set(cacheKey, features)
+ if (features) {
+ audioFeatureCache.set(cacheKey, features)
+ }
return features
}Also applies to: 79-86
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/utils/music/queueManipulation.ts` around lines 43 - 59, The
global audioFeatureCache is being populated with null on user-specific token
failures or transient lookup errors, causing permanent misses for all users; in
getTrackAudioFeatures (and the similar later block that sets cache to null) only
write to audioFeatureCache when you have a definitive API result (i.e., non-null
SpotifyAudioFeatures) or an explicit “not found” response from Spotify, and
avoid caching null when spotifyLinkService.getValidAccessToken(userId) returns
no token or when the fetch fails transiently—so remove or guard the
audioFeatureCache.set(cacheKey, null) calls behind checks for definitive
negative responses and only set the cache after a successful features fetch.
| if (candidate.source === 'spotify' && currentTrack.source === 'spotify') { | ||
| score += 0.08 | ||
| reasons.push('spotify mood match') | ||
| } |
There was a problem hiding this comment.
This bonus is source-based, not mood-based.
Right now every Spotify→Spotify pair gets +0.08 and the "spotify mood match" label without comparing any audio features. That biases rankings toward Spotify candidates even when they are not actually similar in mood.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/utils/music/queueManipulation.ts` around lines 1383 - 1386,
The current block always awards +0.08 for any Spotify→Spotify pair and labels it
"spotify mood match" without checking audio features; change it to first check
the actual Spotify audio feature fields (e.g., candidate.audioFeatures and
currentTrack.audioFeatures — valence, energy, tempo, etc.) and only add the
+0.08 and push the "spotify mood match" reason when the feature-distance (choose
a small threshold on valence/energy or a combined distance) indicates mood
similarity; if you still want a smaller source bias, use a separate smaller
score bump with a different reason like "spotify source match" only when
features are absent. Target the code that references candidate.source,
currentTrack.source, score, and reasons in queueManipulation.ts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bot/src/spotify/spotifyApi.spec.ts`:
- Around line 93-121: The tests for getAudioFeatures lack a boundary case for
energy === 0 which can be misinterpreted as "missing" if the implementation uses
falsy checks; add a new spec that mocks fetch to return ok:true and json
resolving to an object with energy: 0 (number) plus valid valence/danceability
(e.g., 0.5) and assert that getAudioFeatures('test-token','track-123') does not
return null but returns the expected parsed features; reference getAudioFeatures
and the existing specs "returns null when energy is missing" / "returns null
when valence is not a number" to place the new test alongside them.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41eb28d2-72d8-4ca9-8f56-ad33fba3a8a1
📒 Files selected for processing (1)
packages/bot/src/spotify/spotifyApi.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarCloud Scan
- GitHub Check: Quality Gates
🔇 Additional comments (2)
packages/bot/src/spotify/spotifyApi.spec.ts (2)
15-91: Strong coverage forgetAudioFeatureshappy/error paths.This block does a solid job validating success mapping, optional defaults, HTTP failures, JSON failures, and network exceptions.
134-266:searchSpotifyTracktests are comprehensive and practical.Good validation of success, empty/malformed payloads, transport failures, and special-character query encoding.
| it('returns null when energy is missing', async () => { | ||
| const mockResponse = { | ||
| ok: true, | ||
| json: jest.fn().mockResolvedValue({ | ||
| valence: 0.75, | ||
| danceability: 0.65, | ||
| }), | ||
| } | ||
| global.fetch = jest.fn().mockResolvedValue(mockResponse) | ||
|
|
||
| const result = await getAudioFeatures('test-token', 'track-123') | ||
|
|
||
| expect(result).toBeNull() | ||
| }) | ||
|
|
||
| it('returns null when valence is not a number', async () => { | ||
| const mockResponse = { | ||
| ok: true, | ||
| json: jest.fn().mockResolvedValue({ | ||
| energy: 0.8, | ||
| valence: 'high', | ||
| }), | ||
| } | ||
| global.fetch = jest.fn().mockResolvedValue(mockResponse) | ||
|
|
||
| const result = await getAudioFeatures('test-token', 'track-123') | ||
|
|
||
| expect(result).toBeNull() | ||
| }) |
There was a problem hiding this comment.
Add a boundary test for energy = 0 to prevent false “missing field” behavior.
Right now the suite checks missing energy, but not zero-valued energy. A zero numeric value is a valid boundary and should be tested explicitly; otherwise a falsy check can slip through undetected.
Proposed test addition
+ it('accepts energy=0 as a valid numeric value', async () => {
+ const mockResponse = {
+ ok: true,
+ json: jest.fn().mockResolvedValue({
+ energy: 0,
+ valence: 0.4,
+ }),
+ }
+ global.fetch = jest.fn().mockResolvedValue(mockResponse)
+
+ const result = await getAudioFeatures('test-token', 'track-123')
+
+ expect(result).toEqual({
+ energy: 0,
+ valence: 0.4,
+ danceability: 0,
+ tempo: 0,
+ acousticness: 0,
+ })
+ })📝 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.
| it('returns null when energy is missing', async () => { | |
| const mockResponse = { | |
| ok: true, | |
| json: jest.fn().mockResolvedValue({ | |
| valence: 0.75, | |
| danceability: 0.65, | |
| }), | |
| } | |
| global.fetch = jest.fn().mockResolvedValue(mockResponse) | |
| const result = await getAudioFeatures('test-token', 'track-123') | |
| expect(result).toBeNull() | |
| }) | |
| it('returns null when valence is not a number', async () => { | |
| const mockResponse = { | |
| ok: true, | |
| json: jest.fn().mockResolvedValue({ | |
| energy: 0.8, | |
| valence: 'high', | |
| }), | |
| } | |
| global.fetch = jest.fn().mockResolvedValue(mockResponse) | |
| const result = await getAudioFeatures('test-token', 'track-123') | |
| expect(result).toBeNull() | |
| }) | |
| it('returns null when energy is missing', async () => { | |
| const mockResponse = { | |
| ok: true, | |
| json: jest.fn().mockResolvedValue({ | |
| valence: 0.75, | |
| danceability: 0.65, | |
| }), | |
| } | |
| global.fetch = jest.fn().mockResolvedValue(mockResponse) | |
| const result = await getAudioFeatures('test-token', 'track-123') | |
| expect(result).toBeNull() | |
| }) | |
| it('returns null when valence is not a number', async () => { | |
| const mockResponse = { | |
| ok: true, | |
| json: jest.fn().mockResolvedValue({ | |
| energy: 0.8, | |
| valence: 'high', | |
| }), | |
| } | |
| global.fetch = jest.fn().mockResolvedValue(mockResponse) | |
| const result = await getAudioFeatures('test-token', 'track-123') | |
| expect(result).toBeNull() | |
| }) | |
| it('accepts energy=0 as a valid numeric value', async () => { | |
| const mockResponse = { | |
| ok: true, | |
| json: jest.fn().mockResolvedValue({ | |
| energy: 0, | |
| valence: 0.4, | |
| }), | |
| } | |
| global.fetch = jest.fn().mockResolvedValue(mockResponse) | |
| const result = await getAudioFeatures('test-token', 'track-123') | |
| expect(result).toEqual({ | |
| energy: 0, | |
| valence: 0.4, | |
| danceability: 0, | |
| tempo: 0, | |
| acousticness: 0, | |
| }) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/spotify/spotifyApi.spec.ts` around lines 93 - 121, The tests
for getAudioFeatures lack a boundary case for energy === 0 which can be
misinterpreted as "missing" if the implementation uses falsy checks; add a new
spec that mocks fetch to return ok:true and json resolving to an object with
energy: 0 (number) plus valid valence/danceability (e.g., 0.5) and assert that
getAudioFeatures('test-token','track-123') does not return null but returns the
expected parsed features; reference getAudioFeatures and the existing specs
"returns null when energy is missing" / "returns null when valence is not a
number" to place the new test alongside them.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/bot/src/services/musicRecommendation/feedbackService.spec.ts (3)
358-370: Assert the implicit-feedback TTL explicitly (14 days).Using
expect.any(Number)on Line 367 is too loose for a behavior that the PR defines as 14-day retention.Suggested test tightening
expect(setexMock).toHaveBeenCalledWith( 'music:implicit_feedback:user-1', - expect.any(Number), + 14 * 24 * 60 * 60, expect.stringContaining('implicit_dislike'), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/services/musicRecommendation/feedbackService.spec.ts` around lines 358 - 370, The test currently uses expect.any(Number) when asserting the TTL for setexMock in the recordImplicitFeedback test; change that to assert the explicit 14-day TTL (14*24*60*60 = 1209600 seconds) so the retention policy is enforced. Locate the test case for RecommendationFeedbackService.recordImplicitFeedback and replace expect.any(Number) with the explicit numeric value (1209600) in the expect(setexMock).toHaveBeenCalledWith(...) assertion so the TTL is asserted directly.
431-436: This test does not cover Redis write failure inrecordImplicitFeedback.Currently it only rejects
get, but that path is already absorbed bygetImplicitFeedbackMap. Add asetexrejection case to verify graceful handling of save errors too.Suggested additional case
it('recordImplicitFeedback handles redis error gracefully', async () => { - getMock.mockRejectedValue(new Error('redis down')) + getMock.mockResolvedValue(null) + setexMock.mockRejectedValue(new Error('redis down')) const service = new RecommendationFeedbackService(30) await expect(service.recordImplicitFeedback('user-1', 'key', 'implicit_like')).resolves.toBeUndefined() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/services/musicRecommendation/feedbackService.spec.ts` around lines 431 - 436, The test currently only mocks get to reject; add (or modify) a case to also mock the Redis save call to reject so recordImplicitFeedback's save-error branch is covered: when constructing the test where getMock rejects or returns a value, also set the Redis client's setex mock (e.g., setexMock / client.setex) to mockRejectedValue(new Error('redis down')) and assert await expect(service.recordImplicitFeedback('user-1', 'key', 'implicit_like')).resolves.toBeUndefined() to verify the method swallows setex errors gracefully; reference recordImplicitFeedback and the Redis setex mock used in the spec.
372-385: Strengthen trim assertions to validate eviction policy, not just upper bound.Line 384 only checks
<= 200, which can pass even if trimming is wrong. SincefeedbackService.tskeeps the latest 200 (entries.slice(-200)), assert exact count and key retention/eviction.Suggested assertions
const saved = JSON.parse(setexMock.mock.calls[0][2] as string) - expect(Object.keys(saved).length).toBeLessThanOrEqual(200) + expect(Object.keys(saved)).toHaveLength(200) + expect(saved).toHaveProperty('newtrack::artist') + expect(saved).not.toHaveProperty('track0::artist') + expect(saved).not.toHaveProperty('track1::artist')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/services/musicRecommendation/feedbackService.spec.ts` around lines 372 - 385, Change the weak trim assertion to verify the eviction policy: after calling RecommendationFeedbackService.recordImplicitFeedback, assert the saved map has exactly 200 entries (not just <=200), assert the newly added key ('newtrack::artist') is present in the saved object, assert the oldest key (e.g., 'track0::artist') is evicted (not present), and optionally assert a recent existing key (e.g., 'track200::artist') remains; locate these checks around the existing setexMock usage and the saved variable in the test that creates bigMap and instantiates RecommendationFeedbackService.packages/bot/src/lastfm/lastFmApi.spec.ts (1)
499-538: Add missing “API not configured” coverage forgetLovedTracks.This suite does not cover the
getApiConfig()null branch (LASTFM_API_KEY/LASTFM_API_SECRETmissing), which other API methods in this file already validate.Suggested additional test
+ it('returns empty array when api_key is not configured', async () => { + delete process.env.LASTFM_API_KEY + 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 499 - 538, Add a test covering the getApiConfig() null branch for getLovedTracks: mock getApiConfig() to return null (simulating missing LASTFM_API_KEY / LASTFM_API_SECRET), call getLovedTracks('testuser'), and assert it returns an empty array; place this new spec in the existing describe('getLovedTracks') block alongside the other cases so the API-not-configured branch in getLovedTracks is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bot/src/lastfm/lastFmApi.spec.ts`:
- Around line 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.
---
Nitpick comments:
In `@packages/bot/src/lastfm/lastFmApi.spec.ts`:
- Around line 499-538: Add a test covering the getApiConfig() null branch for
getLovedTracks: mock getApiConfig() to return null (simulating missing
LASTFM_API_KEY / LASTFM_API_SECRET), call getLovedTracks('testuser'), and assert
it returns an empty array; place this new spec in the existing
describe('getLovedTracks') block alongside the other cases so the
API-not-configured branch in getLovedTracks is exercised.
In `@packages/bot/src/services/musicRecommendation/feedbackService.spec.ts`:
- Around line 358-370: The test currently uses expect.any(Number) when asserting
the TTL for setexMock in the recordImplicitFeedback test; change that to assert
the explicit 14-day TTL (14*24*60*60 = 1209600 seconds) so the retention policy
is enforced. Locate the test case for
RecommendationFeedbackService.recordImplicitFeedback and replace
expect.any(Number) with the explicit numeric value (1209600) in the
expect(setexMock).toHaveBeenCalledWith(...) assertion so the TTL is asserted
directly.
- Around line 431-436: The test currently only mocks get to reject; add (or
modify) a case to also mock the Redis save call to reject so
recordImplicitFeedback's save-error branch is covered: when constructing the
test where getMock rejects or returns a value, also set the Redis client's setex
mock (e.g., setexMock / client.setex) to mockRejectedValue(new Error('redis
down')) and assert await expect(service.recordImplicitFeedback('user-1', 'key',
'implicit_like')).resolves.toBeUndefined() to verify the method swallows setex
errors gracefully; reference recordImplicitFeedback and the Redis setex mock
used in the spec.
- Around line 372-385: Change the weak trim assertion to verify the eviction
policy: after calling RecommendationFeedbackService.recordImplicitFeedback,
assert the saved map has exactly 200 entries (not just <=200), assert the newly
added key ('newtrack::artist') is present in the saved object, assert the oldest
key (e.g., 'track0::artist') is evicted (not present), and optionally assert a
recent existing key (e.g., 'track200::artist') remains; locate these checks
around the existing setexMock usage and the saved variable in the test that
creates bigMap and instantiates RecommendationFeedbackService.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c186043-24e3-48e5-a3fc-adf31523419d
📒 Files selected for processing (3)
packages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/services/musicRecommendation/feedbackService.spec.tssonar-project.properties
✅ Files skipped from review due to trivial changes (1)
- sonar-project.properties
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quality Gates
- GitHub Check: SonarCloud Scan
🔇 Additional comments (1)
packages/bot/src/lastfm/lastFmApi.spec.ts (1)
19-19: Import update looks correct.
getLovedTracksis correctly added to the API test imports.
| it('returns empty array on non-ok response', async () => { | ||
| fetchMock.mockResolvedValueOnce({ ok: false }) | ||
| const result = await getLovedTracks('testuser') | ||
| expect(result).toEqual([]) | ||
| }) |
There was a problem hiding this comment.
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.
| 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.
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/bot/src/utils/music/queueManipulation.ts (2)
361-363:⚠️ Potential issue | 🟠 MajorThe Spotify feature lookup is still dead work here.
currentFeaturesis fetched, but it never participates in candidate collection or scoring. At the same time, Line 1386 still grants+0.08to every Spotify→Spotify pair and labels itspotify mood matchwithout comparing any audio features. Right now this adds Spotify API latency without producing an actual mood-based score.Also applies to: 1386-1389
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/queueManipulation.ts` around lines 361 - 363, The code fetches currentFeatures via getTrackAudioFeatures(requestedBy.id) but never uses it in candidate collection or scoring, while the scoring still unconditionally grants "+0.08" labeled "spotify mood match"; either remove the unused Spotify lookup to avoid API latency or actually use currentFeatures in the scoring path: fetch candidate features (or include them from existing candidate metadata), compute a similarity/distance between currentFeatures and candidateFeatures, and only apply the +0.08 "spotify mood match" bonus when the similarity meets a threshold; update getTrackAudioFeatures calls to be conditional (skip when currentFeatures is null) and preserve existing .catch handling to fall back to null if the API fails.
43-86:⚠️ Potential issue | 🟠 MajorDon't cache
nullfor token misses or transient Spotify lookup failures.This still turns user-specific or temporary failures into a global negative cache entry keyed only by
title::author. After one user without a token hits this path, every later user gets a permanent miss for the same track.Suggested fix
const audioFeatureCache = new Map<string, SpotifyAudioFeatures | null>() async function getTrackAudioFeatures( track: Track, userId: string, ): Promise<SpotifyAudioFeatures | null> { const cacheKey = normalizeTrackKey(track.title, track.author) - + const cached = audioFeatureCache.get(cacheKey) if (cached !== undefined) { return cached } - + const token = await spotifyLinkService.getValidAccessToken(userId) if (!token) { - audioFeatureCache.set(cacheKey, null) return null } - + let spotifyId: string | null = null - + if (track.url && track.url.includes('open.spotify.com/track/')) { const match = track.url.match(/track\/([a-zA-Z0-9]+)/) if (match) { spotifyId = match[1] @@ - if (!spotifyId) { - audioFeatureCache.set(cacheKey, null) + if (!spotifyId) { return null } - + const features = await getAudioFeatures(token, spotifyId).catch(() => null) - audioFeatureCache.set(cacheKey, features) + if (features) { + audioFeatureCache.set(cacheKey, features) + } return features }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/queueManipulation.ts` around lines 43 - 86, The code currently writes null into audioFeatureCache for token misses and failed Spotify lookups, causing global negative caching; in getTrackAudioFeatures stop setting audioFeatureCache.set(cacheKey, null) when spotifyLinkService.getValidAccessToken(userId) returns falsy or when spotifyId resolution fails — simply return null in those branches without writing the cache; instead only call audioFeatureCache.set(cacheKey, features) when features is a non-null SpotifyAudioFeatures (i.e., after await getAudioFeatures(...) resolves successfully), so only successful lookups are cached; use the existing symbols audioFeatureCache, getTrackAudioFeatures, normalizeTrackKey, spotifyLinkService.getValidAccessToken, searchSpotifyTrack, and getAudioFeatures to locate and change the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bot/src/handlers/player/trackHandlers.ts`:
- Around line 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).
- Around line 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 `<`.
---
Duplicate comments:
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 361-363: The code fetches currentFeatures via
getTrackAudioFeatures(requestedBy.id) but never uses it in candidate collection
or scoring, while the scoring still unconditionally grants "+0.08" labeled
"spotify mood match"; either remove the unused Spotify lookup to avoid API
latency or actually use currentFeatures in the scoring path: fetch candidate
features (or include them from existing candidate metadata), compute a
similarity/distance between currentFeatures and candidateFeatures, and only
apply the +0.08 "spotify mood match" bonus when the similarity meets a
threshold; update getTrackAudioFeatures calls to be conditional (skip when
currentFeatures is null) and preserve existing .catch handling to fall back to
null if the API fails.
- Around line 43-86: The code currently writes null into audioFeatureCache for
token misses and failed Spotify lookups, causing global negative caching; in
getTrackAudioFeatures stop setting audioFeatureCache.set(cacheKey, null) when
spotifyLinkService.getValidAccessToken(userId) returns falsy or when spotifyId
resolution fails — simply return null in those branches without writing the
cache; instead only call audioFeatureCache.set(cacheKey, features) when features
is a non-null SpotifyAudioFeatures (i.e., after await getAudioFeatures(...)
resolves successfully), so only successful lookups are cached; use the existing
symbols audioFeatureCache, getTrackAudioFeatures, normalizeTrackKey,
spotifyLinkService.getValidAccessToken, searchSpotifyTrack, and getAudioFeatures
to locate and change the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4be3085c-3951-4f03-ae8e-d54cd4dfa62f
📒 Files selected for processing (5)
packages/bot/src/handlers/player/trackHandlers.spec.tspackages/bot/src/handlers/player/trackHandlers.tspackages/bot/src/services/musicRecommendation/feedbackService.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/utils/music/queueManipulation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bot/src/services/musicRecommendation/feedbackService.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarCloud Scan
- GitHub Check: Quality Gates
🧰 Additional context used
🪛 Betterleaks (1.1.1)
packages/bot/src/utils/music/queueManipulation.spec.ts
[high] 2662-2662: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| if (!startTime || !track.durationMS || track.durationMS < minDurationMs) { | ||
| guildTrackStartTimes.delete(guildId) | ||
| return |
There was a problem hiding this comment.
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.
| 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 `<`.
| 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', | ||
| ) | ||
| } |
There was a problem hiding this comment.
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).
…oved tracks, artist frequency, spotify audio features signals added (all implicit, no commands needed): - skip tracking: guildTrackStartTimes records when track starts; early skip (<30%, track >20s) → implicit_dislike in redis (14d) - completion tracking: >80% played → implicit_like in redis (14d); both feed calculateRecommendationScore - implicit dislike: -0.35 penalty; implicit like: +0.25 boost - last.fm loved tracks: getLovedTracks() merged first in seed list (highest priority over top/recent) - artist frequency: manual-play count from persistent history → +0.10/+0.20/+0.30 gradient - spotify audio features: getAudioFeatures() + searchSpotifyTrack() wired into replenish flow via getTrackAudioFeatures(); results cached per normalized track key - spotify-source candidate boost: +0.08 when both current and candidate are from spotify code quality: - recordPlayBehavior() helper eliminates duplication between skip/finish tracking handlers - getImplicitKeysByType() helper eliminates duplication between dislike/like key getters - sonar cpd exclusions updated for api/seed files
f159c60 to
490b457
Compare
|
…ks, artist frequency, audio features (#577) signals added (all implicit, no commands needed): - skip tracking: guildTrackStartTimes records when track starts; early skip (<30%, track >20s) → implicit_dislike in redis (14d) - completion tracking: >80% played → implicit_like in redis (14d); both feed calculateRecommendationScore - implicit dislike: -0.35 penalty; implicit like: +0.25 boost - last.fm loved tracks: getLovedTracks() merged first in seed list (highest priority over top/recent) - artist frequency: manual-play count from persistent history → +0.10/+0.20/+0.30 gradient - spotify audio features: getAudioFeatures() + searchSpotifyTrack() wired into replenish flow via getTrackAudioFeatures(); results cached per normalized track key - spotify-source candidate boost: +0.08 when both current and candidate are from spotify code quality: - recordPlayBehavior() helper eliminates duplication between skip/finish tracking handlers - getImplicitKeysByType() helper eliminates duplication between dislike/like key getters - sonar cpd exclusions updated for api/seed files
This branch was successfully deployed
1 participant
What this does
Upgrades autoplay from simple feedback to a fully implicit intelligence system — no commands required, everything inferred from listening behaviour.
New signals
Skip & completion tracking
guildTrackStartTimestracks when each guild's current track startedimplicit_dislikestored in Redis (14d TTL)implicit_likestored in Redis (14d TTL)Last.fm loved tracks
getLovedTracks(username, 50)added to Last.fm API wrapperArtist frequency from manual plays
Spotify audio features
getAudioFeatures(token, trackId)+searchSpotifyTrack(token, title, artist)added to Spotify APIgetTrackAudioFeatures()helper caches results per normalized track keyConfig fix
isSpotifyConfigured()no longer requiresSPOTIFY_REDIRECT_URI— backend derives it fromWEBAPP_BACKEND_URLScore summary (new weights)
Summary by CodeRabbit
New Features
Bug Fixes