Repository navigation
fix(bot): extractor errors, dynamic node path, race and timeout guards - #548
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds defensive handling around player streaming and extractor initialization: a 10s timeout for YouTube recovery searches, try/catch logging for extractor and bridge initialization, replaces a hardcoded yt-dlp JS runtime path with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 docstrings
🧪 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: 2
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/handlers/player/playerFactory.ts (1)
184-187:⚠️ Potential issue | 🔴 Critical
streamViaYtDlpcan hang when process closes cleanly before first data chunk.With the new settled guard, if
closefires first withcode === 0, Line 223 condition is false, timeout is cleared, and the Promise never resolves/rejects.🛠️ Suggested fix
- const timeout = setTimeout(() => { - proc.kill() - reject(new Error('yt-dlp: timed out waiting for stream start')) - }, 15_000) + const timeout = setTimeout(() => { + if (settled) return + settled = true + proc.kill() + reject(new Error('yt-dlp: timed out waiting for stream start')) + }, 15_000) ... proc.once('close', (code) => { if (settled) return settled = true clearTimeout(timeout) - if (code && code !== 0) { - const stderr = Buffer.concat(stderrChunks).toString().trim() - const reason = stderr ? ` — ${stderr.split('\n')[0]}` : '' - reject(new Error(`yt-dlp exited with code ${code}${reason}`)) - } + const stderr = Buffer.concat(stderrChunks).toString().trim() + const reason = stderr ? ` — ${stderr.split('\n')[0]}` : '' + if (code && code !== 0) { + reject(new Error(`yt-dlp exited with code ${code}${reason}`)) + return + } + reject(new Error(`yt-dlp exited before emitting stream data${reason}`)) })Also applies to: 194-197, 219-227
🤖 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 184 - 187, In streamViaYtDlp, the added "settled" guard lets the 15_000ms timeout be cleared when the child 'close' event fires with code===0, leaving the promise neither resolved nor rejected; update the 'close' handler logic so that when proc emits 'close' before any data has been emitted you still settle the promise (e.g., call reject(new Error('yt-dlp: closed before stream start')) or resolve with an empty/failed sentinel) and ensure you clearTimeout(timeout) only after settling; refer to the timeout variable, the 'close' event handler and the settled flag in streamViaYtDlp to implement this consistent settle path (apply same fix to the other similar blocks around lines handling timeout/close).
🤖 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/errorHandlers.ts`:
- Around line 236-247: The timeout created as searchTimeout (setTimeout) is
never cleared when queue.player.search(...) resolves first, leaving dangling
timers; change the logic to capture the timer id (e.g., timeoutId =
setTimeout(...)) and ensure you clearTimeout(timeoutId) when the search promise
resolves (or in a finally on the raced promise), so when
queue.player.search(...) wins the race you cancel the pending timeout; update
usage around searchTimeout, queue.player.search and searchResult accordingly to
avoid orphaned timers.
In `@packages/bot/src/handlers/player/playerFactory.ts`:
- Around line 38-42: The call to player.extractors.loadMulti(DefaultExtractors)
is async and its rejection is currently discarded by using void inside a
try/catch; change it to use
Promise.resolve(player.extractors.loadMulti(DefaultExtractors)).catch(err => {
... }) so both sync throws and async rejections are handled, and log failures
via infoLog/error logger similar to the existing initPlayDlAndRegisterYoutubei()
pattern; ensure you still proceed without throwing so one bad extractor doesn't
stop others.
---
Outside diff comments:
In `@packages/bot/src/handlers/player/playerFactory.ts`:
- Around line 184-187: In streamViaYtDlp, the added "settled" guard lets the
15_000ms timeout be cleared when the child 'close' event fires with code===0,
leaving the promise neither resolved nor rejected; update the 'close' handler
logic so that when proc emits 'close' before any data has been emitted you still
settle the promise (e.g., call reject(new Error('yt-dlp: closed before stream
start')) or resolve with an empty/failed sentinel) and ensure you
clearTimeout(timeout) only after settling; refer to the timeout variable, the
'close' event handler and the settled flag in streamViaYtDlp to implement this
consistent settle path (apply same fix to the other similar blocks around lines
handling timeout/close).
🪄 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: 19faee52-dd60-4a04-8e5a-3f82c2fabf13
📒 Files selected for processing (3)
packages/bot/src/handlers/player/errorHandlers.tspackages/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 context used
📓 Path-based instructions (15)
**/*.{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/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/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 validatorsImplement 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 internal modules
Use named exports for clear usage and easier refactors
Always validate and sanitize external data (HTTP, DB) at the boundary using type guards ...
Files:
packages/bot/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/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. ThrowError(or typed subclasses) with descriptive messages
Include causal error ascausewhen 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)
MarkretryablevsnonRetryableerrors 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, andaria-*attributes as needed.
React Native accessibility: use accessibility props (accessible,accessibilityLabel), proper roles and labels.
Identify and extract repetitive UI components proactively tocomponents/with clear props and minimal coupling.
Web styles: prefer co-located styles or design system tokens; avoid global style leakage.
React Native styles: preferStyleSheet.create, design tokens, and theme providers; avoid in...
Files:
packages/bot/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/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/src/handlers/player/playerFactory.bridge.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/pattern.mdc)
Introduce interfaces at module boundaries to enable testing and substitutions
**/*.{ts,tsx}: Avoid usinganytype in TypeScript. If unavoidable, useunknownwith type guards and justify with a code comment
Preferinterfacefor defining public object shapes in TypeScript, usetypefor unions and utility types
Use TypeScript utility types such asPartial,Pick,Omit,Readonly, andRecordwhen appropriate
UseI{Name}naming convention for interfaces in TypeScript
UseT{Name}naming convention for type aliases and utility types in TypeScript
**/*.{ts,tsx}: Prefer types over interfaces for most cases
Don't ever useany- type safety always
Avoid enums; use const objects instead
For complex types, create a separate file to declare them and import them
Avoid usinganytype; if unavoidable, useunknownwith type guards and justify with code comment
Preferinterfacefor public API shapes; usetypefor unions and utility types
Use TypeScript utility types (Partial, Pick, Omit, Readonly, Record)
Files:
packages/bot/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/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/src/handlers/player/playerFactory.bridge.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/src/handlers/player/playerFactory.bridge.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
Files:
packages/bot/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/bot/src/handlers/player/playerFactory.ts
packages/bot/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/lucky-discord-bot.mdc)
packages/bot/**/*.{ts,tsx}: UseuseMainPlayer()fromdiscord-playerto access the player instance; do not instantiate player directly
Do not duplicate queue or player state outside Discord Player; use shared services from@lucky/sharedfor persistent data like track history and session information
UseerrorLoganddebugLogfrom@lucky/shared/utilsfor logging throughout the bot package
Use embed and reply utilities from@lucky/sharedfor 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/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/bot/src/handlers/player/playerFactory.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.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/bot/src/handlers/player/playerFactory.ts
packages/bot/**
📄 CodeRabbit inference engine (.cursor/rules/lucky-project.mdc)
The
botpackage depends onsharedand contains Discord bot commands and player handlers using Discord.js and Discord Player
Files:
packages/bot/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/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/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/bot/src/handlers/player/playerFactory.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/src/handlers/player/playerFactory.bridge.spec.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.bridge.spec.ts
packages/bot/src/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/subagent-discord.mdc)
Use
@lucky/sharedfor database, Redis, logging, and embed utilities instead of implementing them locally
Files:
packages/bot/src/handlers/player/playerFactory.bridge.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/bot/src/handlers/player/playerFactory.ts
🔇 Additional comments (3)
packages/bot/src/handlers/player/playerFactory.bridge.spec.ts (1)
254-263: Good update to make the spawn assertion environment-agnostic.Switching the expected runtime argument to
node:${process.execPath}correctly tracks the production change and avoids host-specific test brittleness.packages/bot/src/handlers/player/playerFactory.ts (2)
54-63: Nice: async extractor init failures are now observable.Attaching
.catch(errorLog)toinitPlayDlAndRegisterYoutubei(player)removes silent failures and improves operational visibility.
175-179: Good portability fix for yt-dlp JS runtime.Using
node:${process.execPath}instead of a hardcoded absolute path is the right cross-platform/deployment-safe approach.
| // Timeout guard: if YouTube search hangs (no response in 10s), skip the | ||
| // track rather than blocking the player indefinitely. | ||
| const searchTimeout = new Promise<null>((resolve) => | ||
| setTimeout(() => resolve(null), 10_000), | ||
| ) | ||
| const searchResult = await Promise.race([ | ||
| queue.player.search(currentTrack.title, { | ||
| requestedBy: requestedByUser, | ||
| searchEngine: QueryType.YOUTUBE_SEARCH, | ||
| }), | ||
| searchTimeout, | ||
| ]) |
There was a problem hiding this comment.
Clear the timeout when search wins the race.
Line 238 creates a timer that isn’t canceled when queue.player.search(...) resolves first. That leaves unnecessary pending timers during recovery bursts.
🔧 Suggested fix
- const searchTimeout = new Promise<null>((resolve) =>
- setTimeout(() => resolve(null), 10_000),
- )
- const searchResult = await Promise.race([
- queue.player.search(currentTrack.title, {
- requestedBy: requestedByUser,
- searchEngine: QueryType.YOUTUBE_SEARCH,
- }),
- searchTimeout,
- ])
+ let timeoutId: ReturnType<typeof setTimeout> | undefined
+ const searchTimeout = new Promise<null>((resolve) => {
+ timeoutId = setTimeout(() => resolve(null), 10_000)
+ })
+ const searchResult = await Promise.race([
+ queue.player.search(currentTrack.title, {
+ requestedBy: requestedByUser,
+ searchEngine: QueryType.YOUTUBE_SEARCH,
+ }),
+ searchTimeout,
+ ]).finally(() => {
+ if (timeoutId) clearTimeout(timeoutId)
+ })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/handlers/player/errorHandlers.ts` around lines 236 - 247,
The timeout created as searchTimeout (setTimeout) is never cleared when
queue.player.search(...) resolves first, leaving dangling timers; change the
logic to capture the timer id (e.g., timeoutId = setTimeout(...)) and ensure you
clearTimeout(timeoutId) when the search promise resolves (or in a finally on the
raced promise), so when queue.player.search(...) wins the race you cancel the
pending timeout; update usage around searchTimeout, queue.player.search and
searchResult accordingly to avoid orphaned timers.
| // Register DefaultExtractors synchronously (loadMulti is sync in this version). | ||
| // Errors are caught so a single bad extractor does not block the others. | ||
| try { | ||
| void player.extractors.loadMulti(DefaultExtractors) | ||
| infoLog({ |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the discord-player version used by this repo, what is the return type/behavior of player.extractors.loadMulti(...)? Is it synchronous or Promise-based?
💡 Result:
In discord-player (latest version 7.2.0 as of 2026-03), player.extractors.loadMulti(...) is a Promise-based asynchronous method. It returns Promise<{ success: boolean; error: null }>. This is consistently shown in official documentation and API references, where it is always awaited in usage examples (e.g., await player.extractors.loadMulti(DefaultExtractors);). It loads multiple extractors from a bundle (array of extractor classes), optionally with per-extractor options, and returns a success indicator object after asynchronously registering and activating them.
Citations:
- 1: https://mintlify.com/androz2091/discord-player/api/extractor-execution-context
- 2: https://discord-player.js.org/api/discord-player/classes/ExtractorExecutionContext
- 3: https://www.npmjs.com/package/discord-player
- 4: https://github.com/Androz2091/discord-player
- 5: https://discord-player.js.org/docs/creating-a-music-bot/02_extractors_integration
🏁 Script executed:
cd /tmp && find . -name "package.json" -type f 2>/dev/null | head -5Repository: LucasSantana-Dev/Lucky
Length of output: 48
🏁 Script executed:
# Check for package.json files and discord-player version
if [ -f "package.json" ]; then
echo "=== Root package.json ==="
cat package.json | grep -A2 -B2 "discord-player" || echo "discord-player not in root"
fi
# Check in packages/bot
if [ -f "packages/bot/package.json" ]; then
echo "=== packages/bot/package.json ==="
cat packages/bot/package.json | grep -A2 -B2 "discord-player" || echo "discord-player not in bot"
fiRepository: LucasSantana-Dev/Lucky
Length of output: 753
🏁 Script executed:
# Read playerFactory.ts lines 30-65 to see context
if [ -f "packages/bot/src/handlers/player/playerFactory.ts" ]; then
echo "=== playerFactory.ts context (lines 30-65) ==="
sed -n '30,65p' packages/bot/src/handlers/player/playerFactory.ts
fiRepository: LucasSantana-Dev/Lucky
Length of output: 1266
🏁 Script executed:
# Search for unhandled rejection handlers or global error handlers
rg "unhandledRejection|on\(.error|process\.on\(" packages/bot/src --type ts -A3 | head -30Repository: LucasSantana-Dev/Lucky
Length of output: 2400
loadMulti rejection bypasses try/catch — promise rejection can be unhandled.
Lines 38–42 wrap void player.extractors.loadMulti(DefaultExtractors) in try/catch. However, discord-player 7.1.0's loadMulti is Promise-based (async); it returns Promise<{ success: boolean; error: null }>. The void discards this Promise, and try/catch does not catch Promise rejections. Any extractor load failure is silently swallowed, degrading music features without logging.
Use Promise.resolve(loadResult).catch(...) to handle both sync throws and async rejections, consistent with the pattern already used for initPlayDlAndRegisterYoutubei() on lines 54–63.
🔧 Correct pattern (handles sync throw + async reject)
try {
- void player.extractors.loadMulti(DefaultExtractors)
+ const loadResult = player.extractors.loadMulti(DefaultExtractors)
+ void Promise.resolve(loadResult).catch((error) => {
+ errorLog({
+ message:
+ 'DefaultExtractors failed to load — music features degraded',
+ error,
+ })
+ })
infoLog({
message:
'Extractors: SoundCloud, Spotify, Apple Music, Vimeo, Attachments',
})
} catch (error) {
errorLog({
message:
'DefaultExtractors failed to load — music features degraded',
error,
})
}📝 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.
| // Register DefaultExtractors synchronously (loadMulti is sync in this version). | |
| // Errors are caught so a single bad extractor does not block the others. | |
| try { | |
| void player.extractors.loadMulti(DefaultExtractors) | |
| infoLog({ | |
| // Register DefaultExtractors synchronously (loadMulti is sync in this version). | |
| // Errors are caught so a single bad extractor does not block the others. | |
| try { | |
| const loadResult = player.extractors.loadMulti(DefaultExtractors) | |
| void Promise.resolve(loadResult).catch((error) => { | |
| errorLog({ | |
| message: | |
| 'DefaultExtractors failed to load — music features degraded', | |
| error, | |
| }) | |
| }) | |
| infoLog({ | |
| message: | |
| 'Extractors: SoundCloud, Spotify, Apple Music, Vimeo, Attachments', | |
| }) | |
| } catch (error) { | |
| errorLog({ | |
| message: | |
| 'DefaultExtractors failed to load — music features degraded', | |
| error, | |
| }) | |
| } |
🤖 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 38 - 42, The
call to player.extractors.loadMulti(DefaultExtractors) is async and its
rejection is currently discarded by using void inside a try/catch; change it to
use Promise.resolve(player.extractors.loadMulti(DefaultExtractors)).catch(err =>
{ ... }) so both sync throws and async rejections are handled, and log failures
via infoLog/error logger similar to the existing initPlayDlAndRegisterYoutubei()
pattern; ensure you still proceed without throwing so one bad extractor doesn't
stop others.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bot/src/handlers/player/errorHandlers.spec.ts (1)
550-583: Test intent says “timeout”, but it currently only tests immediatenullsearch result.
mockResolvedValue(null)skips validating the actual 10sPromise.racetimeout path. Consider simulating a never-resolving search and advancing fake timers to assert true timeout behavior.Suggested test adjustment
it('skips and notifies when YouTube search times out (Promise.race null)', async () => { - // Search returns null (simulates timeout resolution) — recovery must - // treat it the same as no results. + jest.useFakeTimers() const { queueHandlers } = createPlayerWithHandlers() providerFromTrackMock.mockReturnValue('youtube') analyzeYouTubeErrorMock.mockReturnValue({ isParserError: false }) const channelSendMock = jest.fn().mockResolvedValue({}) + const never = new Promise<never>(() => {}) const queue = { ... - // search returns null — simulates the timeout branch resolving null - player: { search: jest.fn().mockResolvedValue(null) }, + // hangs forever; timeout branch should resolve recovery path + player: { search: jest.fn().mockReturnValue(never) }, ... } ;(queueHandlers.playerError as PlayerErrorHandler)( queue as any, new Error('Could not extract stream'), ) - await flushPromises() + await jest.advanceTimersByTimeAsync(10_000) + await flushPromises() expect(queue.node.skip).toHaveBeenCalled() expect(channelSendMock).toHaveBeenCalled() + jest.useRealTimers() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/handlers/player/errorHandlers.spec.ts` around lines 550 - 583, The test currently simulates a timeout by having queue.player.search return null (mockResolvedValue(null)) but should simulate a never-resolving promise and advance fake timers to exercise the actual Promise.race timeout path; change the search mock in the "skips and notifies when YouTube search times out (Promise.race null)" test to return new Promise(()=>{}) (e.g., mockImplementation(() => new Promise(()=>{}))), enable Jest fake timers (jest.useFakeTimers()), advance timers by the timeout duration (jest.advanceTimersByTime(10000) or the configured timeout), then await flushPromises and restore timers (jest.useRealTimers()) before asserting that queue.node.skip and channelSendMock were called, leaving the rest of the test flow (providerFromTrackMock, analyzeYouTubeErrorMock, queueHandlers.playerError invocation) unchanged.
🤖 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/handlers/player/errorHandlers.spec.ts`:
- Around line 550-583: The test currently simulates a timeout by having
queue.player.search return null (mockResolvedValue(null)) but should simulate a
never-resolving promise and advance fake timers to exercise the actual
Promise.race timeout path; change the search mock in the "skips and notifies
when YouTube search times out (Promise.race null)" test to return new
Promise(()=>{}) (e.g., mockImplementation(() => new Promise(()=>{}))), enable
Jest fake timers (jest.useFakeTimers()), advance timers by the timeout duration
(jest.advanceTimersByTime(10000) or the configured timeout), then await
flushPromises and restore timers (jest.useRealTimers()) before asserting that
queue.node.skip and channelSendMock were called, leaving the rest of the test
flow (providerFromTrackMock, analyzeYouTubeErrorMock, queueHandlers.playerError
invocation) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69a55dd2-ff5b-41c7-872b-dcea6f440b76
📒 Files selected for processing (3)
packages/bot/src/handlers/player/errorHandlers.spec.tspackages/bot/src/handlers/player/errorHandlers.tspackages/bot/src/handlers/player/playerFactory.bridge.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bot/src/handlers/player/errorHandlers.ts
- packages/bot/src/handlers/player/playerFactory.bridge.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 context used
📓 Path-based instructions (15)
**/*.{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/src/handlers/player/errorHandlers.spec.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 validatorsImplement 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 internal modules
Use named exports for clear usage and easier refactors
Always validate and sanitize external data (HTTP, DB) at the boundary using type guards ...
Files:
packages/bot/src/handlers/player/errorHandlers.spec.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/error-handling.mdc)
**/*.{js,jsx,ts,tsx}: Never throw strings. ThrowError(or typed subclasses) with descriptive messages
Include causal error ascausewhen 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)
MarkretryablevsnonRetryableerrors 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, andaria-*attributes as needed.
React Native accessibility: use accessibility props (accessible,accessibilityLabel), proper roles and labels.
Identify and extract repetitive UI components proactively tocomponents/with clear props and minimal coupling.
Web styles: prefer co-located styles or design system tokens; avoid global style leakage.
React Native styles: preferStyleSheet.create, design tokens, and theme providers; avoid in...
Files:
packages/bot/src/handlers/player/errorHandlers.spec.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/src/handlers/player/errorHandlers.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/pattern.mdc)
Introduce interfaces at module boundaries to enable testing and substitutions
**/*.{ts,tsx}: Avoid usinganytype in TypeScript. If unavoidable, useunknownwith type guards and justify with a code comment
Preferinterfacefor defining public object shapes in TypeScript, usetypefor unions and utility types
Use TypeScript utility types such asPartial,Pick,Omit,Readonly, andRecordwhen appropriate
UseI{Name}naming convention for interfaces in TypeScript
UseT{Name}naming convention for type aliases and utility types in TypeScript
**/*.{ts,tsx}: Prefer types over interfaces for most cases
Don't ever useany- type safety always
Avoid enums; use const objects instead
For complex types, create a separate file to declare them and import them
Avoid usinganytype; if unavoidable, useunknownwith type guards and justify with code comment
Preferinterfacefor public API shapes; usetypefor unions and utility types
Use TypeScript utility types (Partial, Pick, Omit, Readonly, Record)
Files:
packages/bot/src/handlers/player/errorHandlers.spec.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/src/handlers/player/errorHandlers.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/src/handlers/player/errorHandlers.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
Files:
packages/bot/src/handlers/player/errorHandlers.spec.ts
packages/bot/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/lucky-discord-bot.mdc)
packages/bot/**/*.{ts,tsx}: UseuseMainPlayer()fromdiscord-playerto access the player instance; do not instantiate player directly
Do not duplicate queue or player state outside Discord Player; use shared services from@lucky/sharedfor persistent data like track history and session information
UseerrorLoganddebugLogfrom@lucky/shared/utilsfor logging throughout the bot package
Use embed and reply utilities from@lucky/sharedfor 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/src/handlers/player/errorHandlers.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/errorHandlers.spec.ts
packages/bot/**
📄 CodeRabbit inference engine (.cursor/rules/lucky-project.mdc)
The
botpackage depends onsharedand contains Discord bot commands and player handlers using Discord.js and Discord Player
Files:
packages/bot/src/handlers/player/errorHandlers.spec.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/src/handlers/player/errorHandlers.spec.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/src/handlers/player/errorHandlers.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/quality.mdc)
Unit tests must use naming convention
*.spec.ts
Files:
packages/bot/src/handlers/player/errorHandlers.spec.ts
packages/bot/src/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/subagent-discord.mdc)
Use
@lucky/sharedfor database, Redis, logging, and embed utilities instead of implementing them locally
Files:
packages/bot/src/handlers/player/errorHandlers.spec.ts
|
#548) * fix(bot): extractor errors, dynamic node path, race and timeout guards * test(bot): race guard, search timeout, timer unref for open handles
This branch was successfully deployed
1 participant
Changes (Phase 1+2 of complete analysis plan)
playerFactory.tsExtractor errors now visible
void player.extractors.loadMulti()+void initPlayDlAndRegisterYoutubei()silently swallowed all errors. Replaced with try/catch +errorLogand.catch(errorLog)— failures now appear in Sentry and logs.Dynamic Node.js path for yt-dlp
--js-runtimes node:/usr/local/bin/nodewas hardcoded and fails on any system without Node.js at that exact path. Replaced withprocess.execPath(the actual running Node.js binary) — works on all platforms/deployments.settledguard for close-before-data raceIf yt-dlp crashes before emitting any output,
proc.once('close')fires beforeproc.stdout.once('data'). Without the guard, the promise could be double-settled or resolve with a broken stream. Addedlet settled = falseflag checked in all three listeners.errorHandlers.ts10s timeout on YouTube recovery search
queue.player.search()had no timeout — if the YouTube API doesn't respond, the player hangs indefinitely. Wrapped inPromise.race([search, timeoutAfter(10s)]), which resolvesnullon timeout and falls through towarnLog + skip.Tests
--js-runtimesassertion to useprocess.execPathSummary by CodeRabbit
Bug Fixes
Tests