Repository navigation
fix: plug within-cycle dedup gaps for same-song variants - #583
Conversation
selectDiverseCandidates only checked normalizeTitleOnly in selectedTitleKeys so "Beyoncé - Halo" and "Halo - Beyoncé (Lyrics)" both passed and two variants of the same song were selected in the same replenish cycle. addSelectedTracks only added 2 exclusion keys (normalizeTrackKey + normalizeTitleOnly), missing the extractSongCore key, so subsequent phases within the same invocation also couldn't catch cross-format duplicates. Both now compute and check extractSongCore — same logic as isDuplicateCandidate. Also adds warnLog when Spotify search falls back to YouTube so connectivity issues are observable in production logs.
|
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 45 minutes and 16 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)
📝 WalkthroughWalkthroughEnhanced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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.
🧹 Nitpick comments (1)
packages/bot/src/utils/music/queueManipulation.ts (1)
803-809: Refine fallback warning semantics to avoid misleading operational signals.At Line 807, the warning says primary search “returned no results” for any fallback hit (
idx > 0), including cases where the primary search threw. Consider tracking primary outcome (emptyvserror) and logging that explicitly.Suggested patch
async function searchSeedCandidates( @@ ): Promise<Track[]> { @@ - for (const [idx, engine] of engines.entries()) { + let primaryOutcome: 'empty' | 'error' | null = null + for (const [idx, engine] of engines.entries()) { try { const searchResult = await queue.player.search(query, { requestedBy: requestedBy ?? undefined, searchEngine: engine, }) @@ + if (idx === 0 && tracks.length === 0) { + primaryOutcome = 'empty' + } if (tracks.length > 0) { - if (idx > 0) { + if (idx > 0 && primaryOutcome) { warnLog({ - message: - 'Autoplay: primary search returned no results, using fallback engine', - data: { engine, query }, + message: + primaryOutcome === 'empty' + ? 'Autoplay: primary search returned no results, using fallback engine' + : 'Autoplay: primary search failed, using fallback engine', + data: { engine, query, primaryOutcome }, }) } return tracks } } catch (error) { + if (idx === 0) primaryOutcome = 'error' debugLog({ message: 'Search failed for seed, trying next engine', data: { query, engine, error: String(error) }, }) } }🤖 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 803 - 809, The current warnLog in the autoplay fallback branch always states the "primary search returned no results" when idx > 0, which is misleading for cases where the primary search threw; update the logic in the function handling autoplay fallback (the block that checks tracks.length and idx) to track the primary search outcome (e.g., a boolean or enum like primaryResult = 'empty' | 'error') and if an error occurred capture the error object and include it in warnLog, otherwise keep the existing empty-results message; reference the same variables used now (tracks, idx, engine, query) and change the warnLog message to explicitly indicate either "primary search returned no results" or "primary search failed" and include the error details when primaryResult === 'error'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 803-809: The current warnLog in the autoplay fallback branch
always states the "primary search returned no results" when idx > 0, which is
misleading for cases where the primary search threw; update the logic in the
function handling autoplay fallback (the block that checks tracks.length and
idx) to track the primary search outcome (e.g., a boolean or enum like
primaryResult = 'empty' | 'error') and if an error occurred capture the error
object and include it in warnLog, otherwise keep the existing empty-results
message; reference the same variables used now (tracks, idx, engine, query) and
change the warnLog message to explicitly indicate either "primary search
returned no results" or "primary search failed" and include the error details
when primaryResult === 'error'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e820278-a652-4644-a95d-aab5b1213753
📒 Files selected for processing (1)
packages/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: Quality Gates
- GitHub Check: SonarCloud Scan
🔇 Additional comments (2)
packages/bot/src/utils/music/queueManipulation.ts (2)
1233-1248: Core-key dedup in selection is a solid fix for same-song variants.Line 1242 and Line 1248 correctly gate and record the normalized
extractSongCorekey, which closes the within-cycle duplicate selection gap.
1277-1281: Exclusion-key parity after add is now consistent.Adding the normalized core key here ensures subsequent passes in the same replenish call can block cross-format duplicates, matching the duplicate detector behavior.
|
This branch was successfully deployed
1 participant
Root cause
PR #582 fixed the cross-cycle dedup (history → candidate comparison) but left two within-cycle gaps where the same song could be selected multiple times in a single replenish call.
Gap 1 —
selectDiverseCandidatesselectedTitleKeysonly storednormalizeTitleOnly(title), so:"beyonchalo"to selectedTitleKeys"halobeyonc"→ not in set → selected againGap 2 —
addSelectedTracksOnly added 2 exclusion keys (
normalizeTrackKey+normalizeTitleOnly) after a track was selected. Missing theextractSongCorekey, so subsequent phases within the same invocation couldn't catch cross-format duplicates.Fix
Both
selectDiverseCandidatesandaddSelectedTracksnow also computeextractSongCore(title, author)and include it in their dedup sets — consistent withisDuplicateCandidate.Also adds a
warnLogwhen Spotify search returns 0 results and falls back to YouTube, making provider connectivity issues observable in production logs (addressing the "still using YouTube" concern).Test plan
Summary by CodeRabbit