Skip to content

fix(bot): restore yt-dlp pipe stream for youtube audio playback - #418

Merged
LucasSantana-Dev merged 1 commit into
mainfrom
fix/restore-ytdlp-pipe-stream
Mar 31, 2026
Merged

LucasSantana-Dev merged 1 commit into
mainfrom
fix/restore-ytdlp-pipe-stream

Conversation

@LucasSantana-Dev

@LucasSantana-Dev LucasSantana-Dev commented Mar 31, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Restores getYtDlpStream() which spawns yt-dlp -f bestaudio/best -o - and returns proc.stdout as a Readable directly to discord-player
  • Removes the --get-url / execFile approach that returned a googlevideo URL string — this was already identified as broken in a prior commit ("fix: pipe yt-dlp audio stream instead of resolving url") and was accidentally re-introduced
  • Keeps checkYtDlpAvailability() to fall back to native YoutubeiExtractor IOS client when yt-dlp is not installed

Root Cause

discord-player's audio pipeline cannot reliably consume a raw googlevideo URL string passed from createStream. The pipe stream approach (returning proc.stdout) was the original working solution that was previously reverted without understanding why it worked.

Test plan

  • CI passes
  • Bot plays audio in voice channel after deploy

Summary by CodeRabbit

  • Refactor

    • Updated audio streaming implementation for YouTube content, transitioning from traditional URL resolution-based approach to direct piped streaming architecture.
  • Tests

    • Updated comprehensive test suite to properly validate the new streaming-based audio handling methodology and error conditions.
  • Bug Fixes

    • Enhanced error handling, logging, and process stream management for the audio streaming pipeline.

The --get-url approach returns a googlevideo URL that discord-player
cannot reliably consume via FFmpeg in its audio pipeline. Restore the
original working approach: spawn yt-dlp with -o - and return proc.stdout
directly as a Readable stream. This was the last known working approach
before the stream reliability regressions.
@vercel

vercel Bot commented Mar 31, 2026 •

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
lucky Ready Ready Preview, Comment Mar 31, 2026 4:19pm

Request Review

@coderabbitai

coderabbitai Bot commented Mar 31, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Refactored yt-dlp integration from URL resolution via execFile with --get-url to direct stream piping via spawn with stdout output. Modified playerFactory to return a Readable stream for YouTube URLs instead of resolved URL strings. Updated both unit and integration tests to validate the new spawn-based streaming arguments and behavior.

Changes

Cohort / File(s) Summary
yt-dlp Stream Implementation
packages/bot/src/handlers/player/playerFactory.ts
Replaced getYtDlpUrl (execFile with --get-url) with getYtDlpStream (spawn with -o - piping). Updated YouTubeExtractor.createStream contract from Promise<string> to Promise<Readable | string>. YouTube URLs now return piped stdout stream; non-YouTube URLs return original URL string. Changed yt-dlp arguments from -f bestaudio --get-url --no-check-certificates to -f bestaudio/best -o - --quiet. Added stderr logging and process error handlers.
Unit Test Updates
packages/bot/src/handlers/player/playerFactory.spec.ts
Replaced --get-url test assertions with piped streaming validation. Removed timeout assertions; now validates stdio: ['ignore','pipe','pipe'] spawn options. Replaced getYtDlpUrl mocking with expectations that non-YouTube URLs pass through unchanged while YouTube URLs are handled via spawned process stdout collection.
Integration Test Updates
packages/bot/tests/handlers/player/playerFactory.test.ts
Removed child_process.execFile and util.promisify mocks. Updated YouTube URL tests to expect spawn calls with pipe-style arguments (-o -, -f bestaudio/best). Changed createStream type expectation from Promise<string> to Promise<unknown>. Added validation that non-YouTube URLs trigger no additional spawn calls.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

bot, size/m

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: restoring yt-dlp pipe stream functionality for YouTube audio playback. It is specific, concise, and directly reflects the primary objective of replacing --get-url with piped stdout streaming.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/restore-ytdlp-pipe-stream

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
18.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@coderabbitai coderabbitai Bot added the size/m label Mar 31, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/bot/src/handlers/player/playerFactory.ts (1)

2-2: Use node:stream prefix for built-in modules.

Per Node.js ESM best practices and the SonarCloud hint, prefer the node: prefixed import for built-in modules.

🔧 Suggested fix
-import type { Readable } from 'stream'
+import type { Readable } from 'node:stream'
🤖 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` at line 2, Update the
built-in module import to use the Node.js "node:" prefix: replace the import of
the Readable type currently from 'stream' with the equivalent from 'node:stream'
(the import line referencing Readable in playerFactory.ts). This aligns with ESM
best practices and SonarCloud hints—ensure the import statement uses
"node:stream" and keep the type-only import form.
🤖 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 43-60: The getYtDlpStream function currently returns proc.stdout
but doesn't propagate process errors to the returned Readable; attach handlers
to the child process (proc.on('error') and proc.on('close'/'exit')) to create an
Error when the process fails (non-zero exit code or runtime error) and call
stream.destroy(err) so consumers receive an error event; change the return to
use a local const stream = proc.stdout as Readable and in both the 'error' and
the 'close'/'exit' handlers call stream.destroy(new Error(...)) with a clear
message including the exit code or original error to surface failures to
discord-player and other consumers.

---

Nitpick comments:
In `@packages/bot/src/handlers/player/playerFactory.ts`:
- Line 2: Update the built-in module import to use the Node.js "node:" prefix:
replace the import of the Readable type currently from 'stream' with the
equivalent from 'node:stream' (the import line referencing Readable in
playerFactory.ts). This aligns with ESM best practices and SonarCloud
hints—ensure the import statement uses "node:stream" and keep the type-only
import form.
🪄 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: a9c1ccc6-57b3-4d25-a3fe-89e2f4904d8c

📥 Commits

Reviewing files that changed from the base of the PR and between 9402ab7 and 67dff59.

📒 Files selected for processing (3)
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
  • packages/bot/tests/handlers/player/playerFactory.test.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
📓 Path-based instructions (16)
**/*.{js,jsx,ts,tsx,vue,html}

📄 CodeRabbit inference engine (.cursor/rules/accessibility-openness.mdc)

Provide accessible UI components using semantic HTML and ARIA attributes where necessary

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/dependency-injection.mdc)

**/*.{ts,tsx,js,jsx}: Prefer constructor injection for classes that require dependencies
Avoid global mutable singletons unless necessary
Use explicit interfaces for external dependencies to make testing easier

**/*.{ts,tsx,js,jsx}: Include required references in PRs/code for non-trivial logic: TypeScript (official docs), MDN (JavaScript reference), and official docs for any runtime/framework/libraries used (e.g., Node.js, React) as applicable.
Before assuming behavior of an API, include the doc link and a ≤25-word quote when the change relies on it.

**/*.{ts,tsx,js,jsx}: Prefer named exports for clear usage and easier refactors in TypeScript/JavaScript
Keep import order consistent: external first, then internal modules
Remove dead code and unused imports

**/*.{ts,tsx,js,jsx}: Use PascalCase naming convention for React/UI components
Use camelCase naming convention for variables and functions
Use UPPER_SNAKE_CASE naming convention for constants
Maintain consistent import grouping and ordering within the project, keeping third-party imports separate from local imports
For external data sources (HTTP, database), always validate and sanitize input using type guards or schema validators

**/*.{ts,tsx,js,jsx}: Use Prettier with no semicolons, single quotes, 4-space indent, 80 character width
Files must not exceed 250 lines and this is enforced

Implement TypeScript typecheck and linter in CI quality checks

**/*.{ts,tsx,js,jsx}: Use TypeScript for enhanced type safety
Implement error handling and error logging
Avoid commenting code unless extremely necessary - code should explain itself with descriptive names
Leave NO todos, placeholders or missing pieces in the code
Variables and functions must use camelCase
Constants must use UPPER_SNAKE_CASE
Use arrow functions for methods and computed properties
Avoid unnecessary curly braces in conditionals; use concise syntax for simple statements
Maintain consistent import grouping/order: external imports first, then...

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)

**/*.{js,jsx,ts,tsx}: Never throw strings. Throw Error (or typed subclasses) with descriptive messages
Include causal error as cause when available for better debugging
Define clear, stable error codes (e.g., ERR_AUTH_EXPIRED, ERR_NETWORK_TIMEOUT)
Provide optional metadata (e.g., details, retryable, status, correlationId) in error objects
Use domain error classes per area (e.g., AuthenticationError, ValidationError, NetworkError)
Log errors with structure (message, code, stack, cause, correlationId, user context where appropriate)
Mark retryable vs nonRetryable errors where helpful for operations
Set timeouts and handle aborts/cancellations; avoid dangling requests in API/network code
Implement backoff for transient failures; avoid infinite retries
Map HTTP status → domain errors; 4xx vs 5xx behave differently (e.g., retry for 5xx/network)

**/*.{js,jsx,ts,tsx}: Use functional components with hooks in React/React Native. Avoid class components.
Keep components focused on a single responsibility; extract complex logic into custom hooks.
Keep state local when possible. Use Context/Zustand/Redux only when necessary for state management.
If props or state traverse more than 3 levels, consider using context or a feature-scoped store instead of prop drilling.
Use performance optimization techniques: React.memo, useMemo, useCallback, Suspense (web), and virtualization for long lists; avoid unnecessary re-renders.
Web accessibility: use semantic HTML, labels, focus management, keyboard navigation, and aria-* attributes as needed.
React Native accessibility: use accessibility props (accessible, accessibilityLabel), proper roles and labels.
Identify and extract repetitive UI components proactively to components/ with clear props and minimal coupling.
Web styles: prefer co-located styles or design system tokens; avoid global style leakage.
React Native styles: prefer StyleSheet.create, design tokens, and theme providers; avoid in...

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
**/*.{test,spec}.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)

**/*.{test,spec}.{js,jsx,ts,tsx}: Test behavior, not implementation. Prefer Testing Library utilities for testing React/React Native components.
For React Native tests: mock native modules and test component interactions and accessibility labels.

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/pattern.mdc)

Introduce interfaces at module boundaries to enable testing and substitutions

**/*.{ts,tsx}: Avoid using any type in TypeScript. If unavoidable, use unknown with type guards and justify with a code comment
Prefer interface for defining public object shapes in TypeScript, use type for unions and utility types
Use TypeScript utility types such as Partial, Pick, Omit, Readonly, and Record when appropriate
Use I{Name} naming convention for interfaces in TypeScript
Use T{Name} naming convention for type aliases and utility types in TypeScript

**/*.{ts,tsx}: Functions must be less than 50 lines with cyclomatic complexity less than 10
Do not use any types - ESLint enforces this at error level

**/*.{ts,tsx}: Prefer types over interfaces for most cases
Don't ever use any - type safety always
Avoid enums; use const objects instead
For complex types, create a separate file to declare them and import them
Avoid using any type; if unavoidable, use unknown with type guards and justify with code comment
Prefer interface for public API shapes; use type for unions and utility types
Use TypeScript utility types (Partial, Pick, Omit, Readonly, Record)

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
**/*.{test,spec}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/pattern.mdc)

**/*.{test,spec}.{ts,tsx,js,jsx}: Test behavior, not implementation details
Prefer unit tests for core logic; add integration tests at meaningful boundaries

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
**/*.{test,spec}.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/testing-quality.mdc)

**/*.{test,spec}.{js,ts,jsx,tsx}: Use Jest + a React testing library for unit and component tests as applicable
Test behavior, not implementation details

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
**/*.{js,ts,tsx,jsx}

📄 CodeRabbit inference engine (.cursor/rules/documentation.mdc)

**/*.{js,ts,tsx,jsx}: Minimize comments in code; explain the 'why' when non-obvious, let code express the 'what' through clear naming
Document trade-offs briefly when deviating from ideal patterns

**/*.{js,ts,tsx,jsx}: Store secrets, ports, and hosts in environment variables (.env, .env.example) and never hardcode them
Avoid redundant or decorative AI comments; code should be self-explanatory and only commented when logic is non-obvious; prefer refactoring over lengthy comments

Never hardcode secrets, IPs, or ports; use .env and docs/ for required configuration variables

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
packages/bot/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/lucky-discord-bot.mdc)

packages/bot/**/*.{ts,tsx}: Use useMainPlayer() from discord-player to access the player instance; do not instantiate player directly
Do not duplicate queue or player state outside Discord Player; use shared services from @lucky/shared for persistent data like track history and session information
Use errorLog and debugLog from @lucky/shared/utils for logging throughout the bot package
Use embed and reply utilities from @lucky/shared for consistent message formatting and error sanitization across the bot
Use services from @lucky/shared (DatabaseService, Redis client) for database and cache access; do not instantiate Prisma or Redis directly in the bot package

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
packages/bot/**

📄 CodeRabbit inference engine (.cursor/rules/lucky-project.mdc)

The bot package depends on shared and contains Discord bot commands and player handlers using Discord.js and Discord Player

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
**/*.{js,mjs,ts,mts}

📄 CodeRabbit inference engine (.cursor/rules/lucky-project.mdc)

Use Node.js version ≥22 with ESM (ECMAScript modules) only; no CommonJS

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
{packages/*/tests/**/*.test.{js,ts},tests/**/*.test.{js,ts}}

📄 CodeRabbit inference engine (.cursor/rules/lucky-project.mdc)

Add or adjust unit and integration tests when changing behavior; follow existing patterns in packages/*/tests and root tests/ directories

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
**/*.{spec,test}.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/quality.mdc)

**/*.{spec,test}.{ts,tsx,js,jsx}: Use Jest for unit and integration tests
Test behavior, not implementation details
Run unit, integration tests, and coverage report in CI quality checks

Files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
packages/bot/src/handlers/player/**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/lucky-discord-bot.mdc)

Track handling, errors, and lifecycle must be managed through dedicated handlers in packages/bot/src/handlers/player/ (trackHandlers, errorHandlers, lifecycleHandlers)

Files:

  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
**/*.spec.ts

📄 CodeRabbit inference engine (.cursor/rules/quality.mdc)

Unit tests must use naming convention *.spec.ts

Files:

  • packages/bot/src/handlers/player/playerFactory.spec.ts
packages/bot/src/**/*.ts

📄 CodeRabbit inference engine (.cursor/rules/subagent-discord.mdc)

Use @lucky/shared for database, Redis, logging, and embed utilities instead of implementing them locally

Files:

  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
🧠 Learnings (11)
📚 Learning: 2026-03-09T20:20:47.877Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-discord-bot.mdc:0-0
Timestamp: 2026-03-09T20:20:47.877Z
Learning: Applies to packages/bot/**/*.{ts,tsx} : Use `useMainPlayer()` from `discord-player` to access the player instance; do not instantiate player directly

Applied to files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
📚 Learning: 2026-03-09T20:20:47.877Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-discord-bot.mdc:0-0
Timestamp: 2026-03-09T20:20:47.877Z
Learning: Applies to packages/bot/src/handlers/player/**/*.{ts,tsx} : Track handling, errors, and lifecycle must be managed through dedicated handlers in `packages/bot/src/handlers/player/` (trackHandlers, errorHandlers, lifecycleHandlers)

Applied to files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
  • packages/bot/src/handlers/player/playerFactory.ts
📚 Learning: 2026-03-09T20:21:38.098Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/subagent-backend.mdc:0-0
Timestamp: 2026-03-09T20:21:38.098Z
Learning: Applies to packages/backend/tests/**/*.ts : Follow existing patterns for unit and integration tests in `packages/backend/tests/`

Applied to files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
📚 Learning: 2026-03-09T20:21:08.612Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-project.mdc:0-0
Timestamp: 2026-03-09T20:21:08.612Z
Learning: Applies to {packages/*/tests/**/*.test.{js,ts},tests/**/*.test.{js,ts}} : Add or adjust unit and integration tests when changing behavior; follow existing patterns in `packages/*/tests` and root `tests/` directories

Applied to files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
  • packages/bot/src/handlers/player/playerFactory.spec.ts
📚 Learning: 2026-03-15T21:57:49.951Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-15T21:57:49.951Z
Learning: When working on unit tests, Jest ESM mocks, or fixing disabled tests, use the `testing-lucky` skill

Applied to files:

  • packages/bot/tests/handlers/player/playerFactory.test.ts
📚 Learning: 2026-03-09T20:21:31.459Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/quality.mdc:0-0
Timestamp: 2026-03-09T20:21:31.459Z
Learning: Applies to **/*.{spec,test}.{ts,tsx,js,jsx} : Test behavior, not implementation details

Applied to files:

  • packages/bot/src/handlers/player/playerFactory.spec.ts
📚 Learning: 2026-03-09T20:21:52.065Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/subagent-discord.mdc:0-0
Timestamp: 2026-03-09T20:21:52.065Z
Learning: Applies to packages/bot/src/functions/music/**/*.ts : Use existing voice/queue/guild validators before manipulating player or queue state

Applied to files:

  • packages/bot/src/handlers/player/playerFactory.ts
📚 Learning: 2026-03-09T20:20:47.877Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-discord-bot.mdc:0-0
Timestamp: 2026-03-09T20:20:47.877Z
Learning: Applies to packages/bot/**/*.{ts,tsx} : Do not duplicate queue or player state outside Discord Player; use shared services from `lucky/shared` for persistent data like track history and session information

Applied to files:

  • packages/bot/src/handlers/player/playerFactory.ts
📚 Learning: 2026-03-09T20:21:52.065Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/subagent-discord.mdc:0-0
Timestamp: 2026-03-09T20:21:52.065Z
Learning: Applies to packages/bot/src/functions/music/commands/**/*.ts : Use `.cursor/skills/music-queue-player/SKILL.md` for play, queue, skip, volume commands and player lifecycle management

Applied to files:

  • packages/bot/src/handlers/player/playerFactory.ts
📚 Learning: 2026-03-09T20:21:08.612Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/lucky-project.mdc:0-0
Timestamp: 2026-03-09T20:21:08.612Z
Learning: Applies to packages/bot/** : The `bot` package depends on `shared` and contains Discord bot commands and player handlers using Discord.js and Discord Player

Applied to files:

  • packages/bot/src/handlers/player/playerFactory.ts
📚 Learning: 2026-03-09T20:21:52.065Z
Learnt from: CR
Repo: LucasSantana-Dev/Lucky PR: 0
File: .cursor/rules/subagent-discord.mdc:0-0
Timestamp: 2026-03-09T20:21:52.065Z
Learning: Applies to packages/bot/src/functions/{general,music,download}/commands/**/*.ts : Apply `.cursor/rules/lucky-discord-bot.mdc` rules for Discord bot commands and player implementation

Applied to files:

  • packages/bot/src/handlers/player/playerFactory.ts
🪛 GitHub Check: SonarCloud Code Analysis
packages/bot/src/handlers/player/playerFactory.ts

[warning] 2-2: Prefer node:stream over stream.

See more on https://sonarcloud.io/project/issues?id=LucasSantana-Dev_Nexus&issues=AZ1EscfmGVh17VRJeF_Z&open=AZ1EscfmGVh17VRJeF_Z&pullRequest=418

🔇 Additional comments (6)
packages/bot/src/handlers/player/playerFactory.ts (1)

104-114: LGTM!

The createStream signature correctly reflects the dual return type, and the conditional routing between piped streams (YouTube) and passthrough URLs (other sources) is clear and well-structured.

packages/bot/tests/handlers/player/playerFactory.test.ts (3)

88-123: LGTM!

The test correctly validates that yt-dlp is spawned with the expected pipe arguments (-f bestaudio/best, -o -) and stdio configuration for YouTube URLs.


125-156: LGTM!

Good test coverage verifying that proc.stdout is returned as the stream for YouTube URLs.


158-186: LGTM!

The assertion that spawn call count doesn't increase for non-YouTube URLs correctly validates the passthrough behavior.

packages/bot/src/handlers/player/playerFactory.spec.ts (2)

60-83: LGTM!

The argument and stdio configuration assertions correctly reflect the new piping approach.


85-120: LGTM!

The createStream routing logic tests properly validate YouTube URL detection and passthrough behavior for non-YouTube sources.

Comment on lines +43 to 60
const getYtDlpStream = (url: string): Readable => {
debugLog({ message: `yt-dlp piping audio stream: ${url}` })
const proc = spawn(
'yt-dlp',
['-f', 'bestaudio/best', '-o', '-', '--no-warnings', '--quiet', url],
{ stdio: ['ignore', 'pipe', 'pipe'] },
)

proc.stderr?.on('data', (data: Buffer) => {
warnLog({ message: `yt-dlp stderr: ${data.toString().trim()}` })
})

proc.on('error', (err) => {
errorLog({ message: 'yt-dlp process error:', error: err })
})

return proc.stdout as Readable
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Process errors don't propagate to the stream consumer.

If yt-dlp exits with a non-zero code or encounters a runtime error, the returned stream silently ends without signaling an error. The discord-player pipeline will receive incomplete/truncated data with no indication of failure, causing silent playback issues.

Consider destroying the stream with the error so consumers can handle it:

🛠️ Proposed fix to propagate errors
 const getYtDlpStream = (url: string): Readable => {
     debugLog({ message: `yt-dlp piping audio stream: ${url}` })
     const proc = spawn(
         'yt-dlp',
         ['-f', 'bestaudio/best', '-o', '-', '--no-warnings', '--quiet', url],
         { stdio: ['ignore', 'pipe', 'pipe'] },
     )
+
+    const stdout = proc.stdout as Readable

     proc.stderr?.on('data', (data: Buffer) => {
         warnLog({ message: `yt-dlp stderr: ${data.toString().trim()}` })
     })

     proc.on('error', (err) => {
         errorLog({ message: 'yt-dlp process error:', error: err })
+        stdout.destroy(err)
+    })
+
+    proc.on('close', (code) => {
+        if (code !== 0 && code !== null) {
+            const err = new Error(`yt-dlp exited with code ${code}`)
+            errorLog({ message: 'yt-dlp non-zero exit:', error: err })
+            stdout.destroy(err)
+        }
     })

-    return proc.stdout as Readable
+    return stdout
 }
🤖 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 43 - 60, The
getYtDlpStream function currently returns proc.stdout but doesn't propagate
process errors to the returned Readable; attach handlers to the child process
(proc.on('error') and proc.on('close'/'exit')) to create an Error when the
process fails (non-zero exit code or runtime error) and call stream.destroy(err)
so consumers receive an error event; change the return to use a local const
stream = proc.stdout as Readable and in both the 'error' and the 'close'/'exit'
handlers call stream.destroy(new Error(...)) with a clear message including the
exit code or original error to surface failures to discord-player and other
consumers.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
77.8% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@LucasSantana-Dev
LucasSantana-Dev merged commit 04a21b4 into main Mar 31, 2026
11 of 12 checks passed
@LucasSantana-Dev
LucasSantana-Dev deleted the fix/restore-ytdlp-pipe-stream branch March 31, 2026 22:15
LucasSantana-Dev added a commit that referenced this pull request May 13, 2026
The --get-url approach returns a googlevideo URL that discord-player
cannot reliably consume via FFmpeg in its audio pipeline. Restore the
original working approach: spawn yt-dlp with -o - and return proc.stdout
directly as a Readable stream. This was the last known working approach
before the stream reliability regressions.

This branch was successfully deployed

1 active deployment
Preview — 67dff591 Deployed Mar 31, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant