Repository navigation
fix(play): log Spotify errors, add fallback chain to executePlayAtTop, fix Last.fm artist parsing - #590
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 54 minutes and 4 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 (1)
📝 WalkthroughWalkthroughThis pull request enhances the music play command's fallback behavior when searches fail, adds defensive artist parsing in Last.fm API responses to handle objects with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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: 1
🧹 Nitpick comments (2)
packages/bot/src/functions/music/commands/play/index.ts (1)
178-183: Inconsistent error logging in YouTube fallback.The YouTube fallback warning at line 182 only logs
{ query }but omits the actual error. This is inconsistent with:
- The primary error logging at line 170 which includes
error: String(primaryError)- The equivalent code in
queryUtils.ts(line 132) which includeserror: String(youtubeError)For consistent debugging, consider adding the error here as well.
♻️ Suggested fix
warnLog({ message: 'YouTube search failed, falling back to SoundCloud', - data: { query }, + data: { query, error: String(youtubeError) }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/music/commands/play/index.ts` around lines 178 - 183, The YouTube fallback catch block currently calls warnLog without including the caught youtubeError; update the catch in the play command (the catch handling youtubeError near the warnLog call) to add the error to the logged data (e.g., include error: String(youtubeError) or youtubeError.message) so it matches the primary error logging and the implementation in queryUtils.ts for consistent debugging.packages/bot/src/utils/music/autoplay/lastFmSeeds.ts (1)
33-33: Consider trimming before the empty-field guard.Line 33 filters falsy values, but
" "still passes. Using trimmed checks avoids whitespace-only seeds entering dedupe.Proposed tweak
- if (!t.artist || !t.title) return false + if (!t.artist?.trim() || !t.title?.trim()) return false🤖 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.ts` at line 33, Trim artist and title fields before the emptiness guard to prevent whitespace-only strings from passing; update the check around the seed object (where t.artist and t.title are validated) to use trimmed values (e.g., check t.artist?.trim() and t.title?.trim()) and use those trimmed values for downstream dedupe/usage in the functions handling seeds so whitespace-only seeds are rejected.
🤖 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.ts`:
- Around line 218-223: The mapper currently assumes t.artist is non-null and
will throw if an item has a nullish artist; update the artist extraction in the
mapping logic (the ternary that checks typeof t.artist === 'string') to first
guard against null/undefined (e.g., check t.artist != null), use optional
chaining/careful type narrowing for object access ((t.artist as Record<string,
string> | null)?.['#text'] ?? (t.artist as any)?.name ?? ''), and ensure this
defensive pattern is applied to both occurrences referenced (the artist
extraction around the ternary and the similar block at lines ~307-310) so a
single malformed entry returns an empty string for artist instead of throwing
and causing the whole call to return [].
---
Nitpick comments:
In `@packages/bot/src/functions/music/commands/play/index.ts`:
- Around line 178-183: The YouTube fallback catch block currently calls warnLog
without including the caught youtubeError; update the catch in the play command
(the catch handling youtubeError near the warnLog call) to add the error to the
logged data (e.g., include error: String(youtubeError) or youtubeError.message)
so it matches the primary error logging and the implementation in queryUtils.ts
for consistent debugging.
In `@packages/bot/src/utils/music/autoplay/lastFmSeeds.ts`:
- Line 33: Trim artist and title fields before the emptiness guard to prevent
whitespace-only strings from passing; update the check around the seed object
(where t.artist and t.title are validated) to use trimmed values (e.g., check
t.artist?.trim() and t.title?.trim()) and use those trimmed values for
downstream dedupe/usage in the functions handling seeds so whitespace-only seeds
are rejected.
🪄 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: 480f26a8-f8b8-46b6-b335-00d02600f644
📒 Files selected for processing (6)
packages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/queryUtils.tspackages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/utils/music/autoplay/lastFmSeeds.spec.tspackages/bot/src/utils/music/autoplay/lastFmSeeds.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 (4)
packages/bot/src/functions/music/commands/play/queryUtils.ts (1)
109-141: LGTM! Fallback chain implementation is correct.The nested try-catch structure properly cascades through search engines (primary → YouTube → SoundCloud), and error logging captures each failure stage. The condition
searchEngine !== QueryType.AUTOcorrectly avoids redundant retries when AUTO is already used. This aligns with the established fallback pattern inqueueManipulation.ts.One observation: this fallback logic is nearly identical to the implementation in
index.ts(lines 154-192). Consider extracting to a shared helper in a future refactor to reduce duplication.packages/bot/src/functions/music/commands/play/index.ts (1)
170-170: LGTM! Primary error now captured in logs.Adding
error: String(primaryError)addresses the PR objective of logging Spotify errors for better diagnosis of SpotifyExtractor failures.packages/bot/src/lastfm/lastFmApi.spec.ts (1)
283-306: Good coverage for real Last.fmartist['#text']payload shape.These tests validate the exact response format that caused the regression and protect both recent/loved parsing paths.
Also applies to: 564-582
packages/bot/src/utils/music/autoplay/lastFmSeeds.spec.ts (1)
148-164: Nice regression test for malformed track filtering.This directly validates the new defensive behavior and protects seed quality.
| artist: | ||
| typeof t.artist === 'string' | ||
| ? t.artist | ||
| : ((t.artist as Record<string, string>)['#text'] ?? | ||
| t.artist.name ?? | ||
| ''), |
There was a problem hiding this comment.
Harden artist extraction to avoid dropping the entire result set on malformed entries.
Line 221 and Line 308 still assume t.artist is always non-null before property access. If one item has a nullish artist, the mapper throws and the catch path returns [] for the whole call.
Proposed fix
+function extractLastFmArtist(artist: unknown): string {
+ if (typeof artist === 'string') return artist.trim()
+ if (!artist || typeof artist !== 'object') return ''
+ const record = artist as Record<string, unknown>
+ const text =
+ typeof record['#text'] === 'string' ? record['#text'].trim() : ''
+ const name = typeof record.name === 'string' ? record.name.trim() : ''
+ return text || name
+}
+
export async function getRecentTracks(
@@
.filter((t) => !t['@attr']?.nowplaying)
.map((t) => ({
- artist:
- typeof t.artist === 'string'
- ? t.artist
- : ((t.artist as Record<string, string>)['#text'] ??
- t.artist.name ??
- ''),
+ artist: extractLastFmArtist(t.artist),
title: t.name,
}))
@@
return (data.lovedtracks?.track ?? []).map((t) => ({
- artist:
- (t.artist as Record<string, string>)['#text'] ??
- t.artist.name ??
- '',
+ artist: extractLastFmArtist(t.artist),
title: t.name,
}))Also applies to: 307-310
🤖 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 218 - 223, The mapper
currently assumes t.artist is non-null and will throw if an item has a nullish
artist; update the artist extraction in the mapping logic (the ternary that
checks typeof t.artist === 'string') to first guard against null/undefined
(e.g., check t.artist != null), use optional chaining/careful type narrowing for
object access ((t.artist as Record<string, string> | null)?.['#text'] ??
(t.artist as any)?.name ?? ''), and ensure this defensive pattern is applied to
both occurrences referenced (the artist extraction around the ternary and the
similar block at lines ~307-310) so a single malformed entry returns an empty
string for artist instead of throwing and causing the whole call to return [].
|
…tTop fallback (#590) * fix(play): log primary error and add fallback chain to executePlayAtTop * fix(lastfm): handle #text artist field in recenttracks and lovedtracks * test(lastfm): add coverage for #text artist format and undefined guard * test(play): add executePlayAtTop fallback chain coverage
This branch was successfully deployed
1 participant
Summary
/playprimary search catch block so we can diagnose why SpotifyExtractor failsexecutePlayAtTop(/playnow,/playtop) — previously had no fallback at alluser.getrecenttracksreturnsartist: { '#text': '...' }not{ name: '...' }, causingTypeError: Cannot read properties of undefined (reading 'toLowerCase')that broke all Last.fm seed track loadingRoot cause of same-song loop
The Last.fm TypeError caused
getLastFmSeedTracksto always return[], disabling the personalized seed diversity entirely. Without Last.fm seeds, autoplay falls back to searching YouTube for "Don't Look Back In Anger" variants and keeps recommending the same Oasis tracks.Test plan
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests