Repository navigation
fix(bot): prevent duplicate now-playing message from /play command - #542
Conversation
trackNowPlaying: export registerNowPlayingMessage so callers can pre-register an existing message as the guild's now-playing display. play/index.ts: when a track starts immediately (queuePosition === 0), fetch the interaction reply and register it via registerNowPlayingMessage. The playerStart handler then edits that message (adding control buttons) instead of sending a second 'Now Playing' embed in the channel. play/index.spec.ts: add regression test that verifies fetchReply is called and registerNowPlayingMessage receives the reply id+channelId. Documents the false-positive gap where playerStart was never simulated.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds message registration to prevent duplicate "Now Playing" messages. When the Changes
Sequence DiagramsequenceDiagram
participant User
participant PlayCmd as Play Command
participant Handler as Message Handler
participant Player as Player Handler
User->>PlayCmd: Execute /play command (queuePosition=0)
PlayCmd->>PlayCmd: Send interaction reply
PlayCmd->>Handler: Fetch interaction reply (id, channelId)
PlayCmd->>Handler: registerNowPlayingMessage(guildId, messageId, channelId)
Handler->>Handler: Pre-populate songInfoMessages map
PlayCmd-->>User: Command response sent
Note over Player: Later, track starts playing
Player->>Handler: sendNowPlayingEmbed() looks up songInfoMessages
Handler->>Handler: Finds pre-registered message
Player->>Player: Edit existing message (no duplicate created)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
🤖 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/functions/music/commands/play/index.ts`:
- Around line 277-279: The empty catch in the play command's registration
fallback swallows errors; change it to catch the error (e.g., catch (err)) and
log a debug/warn message including the error stack and contextual identifiers
(guildId, userId, trackId or similar) so duplicate-message diagnostics are
possible; update the catch inside the play command (the block around
playerStart/fallback registration) to call the existing logger (processLogger or
logger) with a clear message and the error object.
- Around line 269-280: When queuePosition === 0 you must avoid registering the
playlist-queued confirmation as the now-playing message; change the block around
interaction.fetchReply() and registerNowPlayingMessage(...) to first detect and
skip playlist-start replies (e.g., check a flag/option that indicates a playlist
start or inspect the fetched reply content/components to ensure it is not the
playlistQueued confirmation) and only call registerNowPlayingMessage when the
reply is the actual now-playing message (keep references to queuePosition,
interaction.fetchReply, and registerNowPlayingMessage to locate the code).
🪄 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: 0380cf58-28c9-46bc-9df6-84cac340ed1c
📒 Files selected for processing (3)
packages/bot/src/functions/music/commands/play/index.spec.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/handlers/player/trackNowPlaying.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 (20)
**/*.{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/trackNowPlaying.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.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/trackNowPlaying.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.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/trackNowPlaying.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.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/trackNowPlaying.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.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/trackNowPlaying.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.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/trackNowPlaying.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.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/trackNowPlaying.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/trackNowPlaying.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.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/trackNowPlaying.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.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/trackNowPlaying.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.spec.ts
**/index.ts
📄 CodeRabbit inference engine (.cursor/rules/pattern.mdc)
Use
index.tsonly to re-export a small, intentional surface per module
Files:
packages/bot/src/functions/music/commands/play/index.ts
packages/bot/src/functions/*/commands/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/lucky-discord-bot.mdc)
packages/bot/src/functions/*/commands/**/*.{ts,tsx}: Command model must includedata(slash builder),execute, andcategoryproperties exported frompackages/bot/src/models/Command.ts
Use@discordjs/buildersfor building thedata(SlashCommandBuilder) in command definitions
Commandexecutefunction must receive{ interaction, client }parameters fromCommandExecuteParamstype
UseinteractionReplyandcreateUserFriendlyErrorutilities from@lucky/shared/generalutils for command replies and error handling
Use existing validators frompackages/bot/src/utils/command/for voice channel, queue, and guild validations in commands
Files:
packages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.spec.ts
packages/bot/src/functions/{general,music,download}/commands/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/subagent-discord.mdc)
packages/bot/src/functions/{general,music,download}/commands/**/*.ts: Apply.cursor/rules/lucky-discord-bot.mdcrules for Discord bot commands and player implementation
Use.cursor/skills/discord-commands/SKILL.mdfor implementing slash commands
Files:
packages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.spec.ts
packages/bot/src/functions/music/commands/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/subagent-discord.mdc)
Use
.cursor/skills/music-queue-player/SKILL.mdfor play, queue, skip, volume commands and player lifecycle management
Files:
packages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.spec.ts
packages/bot/src/functions/music/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/subagent-discord.mdc)
Use existing voice/queue/guild validators before manipulating player or queue state
Files:
packages/bot/src/functions/music/commands/play/index.tspackages/bot/src/functions/music/commands/play/index.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/functions/music/commands/play/index.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/functions/music/commands/play/index.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/functions/music/commands/play/index.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/functions/music/commands/play/index.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/quality.mdc)
Unit tests must use naming convention
*.spec.ts
Files:
packages/bot/src/functions/music/commands/play/index.spec.ts
🔇 Additional comments (2)
packages/bot/src/handlers/player/trackNowPlaying.ts (1)
32-38: Clean abstraction for pre-registering now-playing messages.Good addition—this keeps the map mutation encapsulated and enables
/playto reuse/edit the existing message path cleanly.packages/bot/src/functions/music/commands/play/index.spec.ts (1)
733-780: Solid regression coverage for the duplicate-now-playing path.This test correctly validates the new registration flow by asserting
fetchReply()and the exactregisterNowPlayingMessage(guildId, messageId, channelId)call.
| if (queuePosition === 0 && interaction.guildId) { | ||
| try { | ||
| const reply = await interaction.fetchReply() | ||
| registerNowPlayingMessage( | ||
| interaction.guildId, | ||
| reply.id, | ||
| reply.channelId, | ||
| ) | ||
| } catch { | ||
| // non-critical — worst case playerStart sends a fresh message | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard registration to non-playlist replies to avoid overwriting playlist confirmation.
queuePosition === 0 is also true for an empty-queue playlist start. In that case, this registers the playlistQueued reply, and playerStart may overwrite that message with now-playing content.
💡 Proposed fix
- if (queuePosition === 0 && interaction.guildId) {
+ if (queuePosition === 0 && !isPlaylist && interaction.guildId) {
try {
const reply = await interaction.fetchReply()
registerNowPlayingMessage(
interaction.guildId,
reply.id,
reply.channelId,
)
} catch {
// non-critical — worst case playerStart sends a fresh message
}
}📝 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.
| if (queuePosition === 0 && interaction.guildId) { | |
| try { | |
| const reply = await interaction.fetchReply() | |
| registerNowPlayingMessage( | |
| interaction.guildId, | |
| reply.id, | |
| reply.channelId, | |
| ) | |
| } catch { | |
| // non-critical — worst case playerStart sends a fresh message | |
| } | |
| } | |
| if (queuePosition === 0 && !isPlaylist && interaction.guildId) { | |
| try { | |
| const reply = await interaction.fetchReply() | |
| registerNowPlayingMessage( | |
| interaction.guildId, | |
| reply.id, | |
| reply.channelId, | |
| ) | |
| } catch { | |
| // non-critical — worst case playerStart sends a fresh message | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/functions/music/commands/play/index.ts` around lines 269 -
280, When queuePosition === 0 you must avoid registering the playlist-queued
confirmation as the now-playing message; change the block around
interaction.fetchReply() and registerNowPlayingMessage(...) to first detect and
skip playlist-start replies (e.g., check a flag/option that indicates a playlist
start or inspect the fetched reply content/components to ensure it is not the
playlistQueued confirmation) and only call registerNowPlayingMessage when the
reply is the actual now-playing message (keep references to queuePosition,
interaction.fetchReply, and registerNowPlayingMessage to locate the code).
| } catch { | ||
| // non-critical — worst case playerStart sends a fresh message | ||
| } |
There was a problem hiding this comment.
Avoid silent failure in registration fallback path.
The empty catch makes duplicate-message diagnostics harder when fetch/register fails intermittently. Add at least a debug/warn log.
🪵 Proposed fix
- } catch {
- // non-critical — worst case playerStart sends a fresh message
+ } catch (error) {
+ debugLog({
+ message: 'Failed to register now-playing interaction reply',
+ error,
+ data: { guildId: interaction.guildId },
+ })
}As per coding guidelines, "Implement error handling and error logging".
📝 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.
| } catch { | |
| // non-critical — worst case playerStart sends a fresh message | |
| } | |
| } catch (error) { | |
| debugLog({ | |
| message: 'Failed to register now-playing interaction reply', | |
| error, | |
| data: { guildId: interaction.guildId }, | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/functions/music/commands/play/index.ts` around lines 277 -
279, The empty catch in the play command's registration fallback swallows
errors; change it to catch the error (e.g., catch (err)) and log a debug/warn
message including the error stack and contextual identifiers (guildId, userId,
trackId or similar) so duplicate-message diagnostics are possible; update the
catch inside the play command (the block around playerStart/fallback
registration) to call the existing logger (processLogger or logger) with a clear
message and the error object.
) trackNowPlaying: export registerNowPlayingMessage so callers can pre-register an existing message as the guild's now-playing display. play/index.ts: when a track starts immediately (queuePosition === 0), fetch the interaction reply and register it via registerNowPlayingMessage. The playerStart handler then edits that message (adding control buttons) instead of sending a second 'Now Playing' embed in the channel. play/index.spec.ts: add regression test that verifies fetchReply is called and registerNowPlayingMessage receives the reply id+channelId. Documents the false-positive gap where playerStart was never simulated.
This branch was successfully deployed
1 participant
Problem
When
/playis used and the track starts immediately (empty queue), users see two "Now Playing" messages:/playinteraction reply —"🎵 Now Playing: [track]"playerStarthandler callingsendNowPlayingEmbedRoot cause:
sendNowPlayingEmbedcheckssongInfoMessages(an internal Map) to decide whether to edit an existing message or send a new one. The interaction reply is never registered in that map, so the handler always sends a fresh message.Fix
trackNowPlaying.ts— ExportregisterNowPlayingMessage(guildId, messageId, channelId)that writes tosongInfoMessages.play/index.ts— After sending the interaction reply whenqueuePosition === 0(track starts immediately), callinteraction.fetchReply()and register the reply viaregisterNowPlayingMessage. TheplayerStarthandler then edits that message (refreshing buttons) rather than sending a second one.False positive analysis
The previous play command tests never simulated the
playerStartevent firing, so the duplicate was invisible to the test suite. Added a regression test that explicitly verifiesfetchReplyis called andregisterNowPlayingMessagereceives the correctmessageId+channelId.Test plan
registers interaction reply as now-playing message to prevent duplicate/play some songin empty voice channel → only ONE "Now Playing" embed appearsSummary by CodeRabbit
New Features
Tests