Repository navigation
fix: use yt-dlp youtube search for spotify-sourced tracks - #610
Conversation
Spotify URLs cannot be streamed directly by yt-dlp (DRM), so the bridge was always failing and falling back to a bare SoundCloud search like "Title Artist" — which finds remixes and covers. - Remove open.spotify.com from ALLOWED_YTDLP_DOMAINS (yt-dlp always fails) - Add streamViaYtDlpSearch() for ytsearch1: YouTube queries via yt-dlp - In createResilientStream, Spotify-sourced tracks now try yt-dlp with "Title Artist official audio" before falling back to SoundCloud - Tests: add streamViaYtDlpSearch suite + Spotify bridge path cases
|
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 31 minutes and 56 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)
📝 WalkthroughWalkthroughThe PR adds Spotify URL detection and specialized handling to the audio streaming pipeline. When Spotify URLs are detected, the system routes them through a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant createResilientStream
participant isSpotifyDetection as Spotify Detection
participant streamViaYtDlpSearch
participant ytdlpYoutube as yt-dlp (ytsearch1)
participant StreamSoundCloud as SoundCloud Fallback
Client->>createResilientStream: Track with Spotify URL
createResilientStream->>isSpotifyDetection: Check if open.spotify.com
isSpotifyDetection-->>createResilientStream: true
createResilientStream->>streamViaYtDlpSearch: Query: title + author + "official audio"
streamViaYtDlpSearch->>ytdlpYoutube: Spawn with ytsearch1:<query>
alt YouTube Search Success
ytdlpYoutube-->>streamViaYtDlpSearch: Stream first chunk
streamViaYtDlpSearch-->>createResilientStream: Readable stream
createResilientStream-->>Client: Audio stream
else YouTube Search Fails
ytdlpYoutube-->>streamViaYtDlpSearch: Exit non-zero / timeout
streamViaYtDlpSearch-->>createResilientStream: Rejection
createResilientStream->>StreamSoundCloud: Fallback to SoundCloud search
StreamSoundCloud-->>createResilientStream: Readable stream
createResilientStream-->>Client: Audio stream
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 1
🧹 Nitpick comments (2)
packages/bot/src/handlers/player/playerFactory.ts (1)
283-347: Consider extracting shared yt-dlp spawn logic to reduce duplication.
streamViaYtDlpSearchshares ~90% of its implementation withstreamViaYtDlp(spawn setup, settled guard, stderr collection, PassThrough piping, timeout handling). A helper function could reduce duplication and ensure both paths stay in sync.This is not blocking given the code works correctly, but worth considering for maintainability.
♻️ Example helper extraction
function spawnYtDlpStream( args: string[], timeoutMs: number, errorPrefix: string, ): Promise<Readable> { return new Promise<Readable>((resolve, reject) => { const proc = spawn('yt-dlp', args, { stdio: ['ignore', 'pipe', 'pipe'] }) const timeout = setTimeout(() => { proc.kill() reject(new Error(`${errorPrefix}: timed out`)) }, timeoutMs) const stderrChunks: Buffer[] = [] proc.stderr!.on('data', (chunk: Buffer) => stderrChunks.push(chunk)) let settled = false // ... rest of shared logic }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/handlers/player/playerFactory.ts` around lines 283 - 347, streamViaYtDlpSearch duplicates almost all logic from streamViaYtDlp; extract the shared spawn/timeout/stderr/settled/PassThrough piping logic into a helper (e.g. spawnYtDlpStream(args: string[], timeoutMs: number, errorPrefix: string): Promise<Readable>) and have both streamViaYtDlpSearch and streamViaYtDlp call it with their specific yt-dlp args (like `ytsearch1:${query}` vs a direct URL), a 20_000 timeout, and distinct errorPrefix strings; move the spawn call, stderrChunks collection, settled guard, timeout clear, proc.once('error'), proc.once('close') handling, and the firstChunk piping into that helper so both functions simply build args and return spawnYtDlpStream(...).packages/bot/src/handlers/player/playerFactory.bridge.spec.ts (1)
396-430: Consider adding tests for spawn error and timeout scenarios.The test suite covers the main cases, but
streamViaYtDlphas additional edge-case tests (spawnerrorevent at lines 355-369, timeout, double-settle race guard) that would provide more confidence forstreamViaYtDlpSearchas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/handlers/player/playerFactory.bridge.spec.ts` around lines 396 - 430, Add tests to cover spawn `error` event, timeout, and double-settle race for streamViaYtDlpSearch: reuse the existing spawnMock helpers (e.g., makeSpawnError, makeSpawnSuccess/makeSpawnTimeout or create a mock that emits an 'error' event) to add three specs mirroring streamViaYtDlp tests—(1) simulate the child process emitting an 'error' and assert the promise rejects with that error, (2) simulate a hung/slow child or a makeSpawnTimeout to trigger the timeout path and assert rejection with a timeout message, and (3) simulate both an exit/error/timeout race to ensure streamViaYtDlpSearch only resolves/rejects once (double-settle guard) by asserting spawnMock is called once and the returned promise settles exactly once; reference streamViaYtDlpSearch, spawnMock, and your helper factories (makeSpawnError/makeSpawnSuccess/makeSpawnTimeout) when adding the tests.
🤖 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/playerFactory.ts`:
- Around line 400-421: The Spotify branch builds ytQuery from
cleanSearchQuery(cleanedTitle, cleanedAuthor) and can produce an
empty/whitespace-only query; update the isSpotifyUrl block (around ytQuery,
cleanSearchQuery, and streamViaYtDlpSearch usage) to trim and check the cleaned
query before calling streamViaYtDlpSearch, and if it's empty skip the YouTube
search (log a warning via warnLog/infoLog mentioning the empty cleaned query) so
execution falls through to the existing SoundCloud fallback; ensure you
reference the same variables (ytQuery, cleanedTitle, cleanedAuthor) and preserve
the existing try/catch behavior only when a non-empty query is present.
---
Nitpick comments:
In `@packages/bot/src/handlers/player/playerFactory.bridge.spec.ts`:
- Around line 396-430: Add tests to cover spawn `error` event, timeout, and
double-settle race for streamViaYtDlpSearch: reuse the existing spawnMock
helpers (e.g., makeSpawnError, makeSpawnSuccess/makeSpawnTimeout or create a
mock that emits an 'error' event) to add three specs mirroring streamViaYtDlp
tests—(1) simulate the child process emitting an 'error' and assert the promise
rejects with that error, (2) simulate a hung/slow child or a makeSpawnTimeout to
trigger the timeout path and assert rejection with a timeout message, and (3)
simulate both an exit/error/timeout race to ensure streamViaYtDlpSearch only
resolves/rejects once (double-settle guard) by asserting spawnMock is called
once and the returned promise settles exactly once; reference
streamViaYtDlpSearch, spawnMock, and your helper factories
(makeSpawnError/makeSpawnSuccess/makeSpawnTimeout) when adding the tests.
In `@packages/bot/src/handlers/player/playerFactory.ts`:
- Around line 283-347: streamViaYtDlpSearch duplicates almost all logic from
streamViaYtDlp; extract the shared spawn/timeout/stderr/settled/PassThrough
piping logic into a helper (e.g. spawnYtDlpStream(args: string[], timeoutMs:
number, errorPrefix: string): Promise<Readable>) and have both
streamViaYtDlpSearch and streamViaYtDlp call it with their specific yt-dlp args
(like `ytsearch1:${query}` vs a direct URL), a 20_000 timeout, and distinct
errorPrefix strings; move the spawn call, stderrChunks collection, settled
guard, timeout clear, proc.once('error'), proc.once('close') handling, and the
firstChunk piping into that helper so both functions simply build args and
return spawnYtDlpStream(...).
🪄 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: 084cf575-eb1a-4b95-a550-3bb34d4e03c5
📒 Files selected for processing (2)
packages/bot/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/playerFactory.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 (7)
packages/bot/src/handlers/player/playerFactory.bridge.spec.ts (3)
57-61: LGTM!Import correctly updated to include
streamViaYtDlpSearchfor the new test coverage.
327-332: LGTM!Good explicit test to ensure Spotify URLs are rejected by
streamViaYtDlpsince yt-dlp cannot stream DRM-protected Spotify content.
562-597: LGTM!Good coverage for the Spotify-specific routing: verifies that Spotify URLs bypass direct yt-dlp streaming in favor of YouTube search, and that SoundCloud fallback works when the search fails.
packages/bot/src/handlers/player/playerFactory.ts (4)
77-78: LGTM!Log message formatting improvements. No functional changes.
Also applies to: 85-86, 104-106, 113-115
366-377: LGTM!Spotify URL detection and debug logging are correctly implemented. The
includes('open.spotify.com')check is sufficient for user-facing Spotify track URLs.
380-398: LGTM!Correctly guards the direct yt-dlp path to skip Spotify URLs, which cannot be streamed directly due to DRM.
349-359: LGTM!Docstring accurately reflects the updated fallback chain.
Cleanup from rebase conflict - remove duplicate old implementation.
10307f9 to
e774905
Compare
|
* fix: use yt-dlp youtube search for spotify-sourced tracks Spotify URLs cannot be streamed directly by yt-dlp (DRM), so the bridge was always failing and falling back to a bare SoundCloud search like "Title Artist" — which finds remixes and covers. - Remove open.spotify.com from ALLOWED_YTDLP_DOMAINS (yt-dlp always fails) - Add streamViaYtDlpSearch() for ytsearch1: YouTube queries via yt-dlp - In createResilientStream, Spotify-sourced tracks now try yt-dlp with "Title Artist official audio" before falling back to SoundCloud - Tests: add streamViaYtDlpSearch suite + Spotify bridge path cases * fix: remove duplicate streamviaytdlpsearch function Cleanup from rebase conflict - remove duplicate old implementation. * chore: suppress sonarcloud s4036 hotspot on yt-dlp spawn * refactor: delegate streamviaytdlpsearch to streamviaytdlp
This branch was successfully deployed
1 participant
Problem
When a user plays a direct Spotify URL (e.g. `/play https://open.spotify.com/track/...\`), the bot plays a wrong version — remix, cover, or live recording — instead of the original.
Root cause: `open.spotify.com` was in `ALLOWED_YTDLP_DOMAINS`, so yt-dlp was attempted against the Spotify URL (which always fails — Spotify is DRM-protected). The bridge then fell back to a bare SoundCloud search like `"Title Artist"` with no version qualifiers, so SoundCloud's first result was whatever it ranked highest — often a remix.
Fix
Priority order for Spotify tracks is now:
Tests
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests