Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions packages/bot/src/functions/music/commands/play/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ jest.mock('../../../../utils/music/queueResolver', () => ({
jest.mock('../../../../utils/command/commandValidations', () => ({
requireVoiceChannel: (interaction: unknown) =>
requireVoiceChannelMock(interaction),
requireDJRole: (...args: unknown[]) => requireDJRoleMock(...args)
requireDJRole: (...args: unknown[]) => requireDJRoleMock(...args),
}))

jest.mock('@lucky/shared/utils', () => ({
Expand Down Expand Up @@ -133,6 +133,12 @@ jest.mock('../../../../utils/general/errorSanitizer', () => ({
createUserFriendlyError: (error: unknown) => 'User friendly error',
}))

const registerNowPlayingMessageMock = jest.fn()
jest.mock('../../../../handlers/player/trackNowPlaying', () => ({
registerNowPlayingMessage: (...args: unknown[]) =>
registerNowPlayingMessageMock(...args),
}))

import playCommand from './index'

function createInteraction(guildId: string | null) {
Expand All @@ -151,6 +157,9 @@ function createInteraction(guildId: string | null) {
reply: jest.fn(),
deferReply: jest.fn(),
editReply: jest.fn(),
fetchReply: jest
.fn()
.mockResolvedValue({ id: 'msg-123', channelId: 'channel-1' }),
} as any
}

Expand Down Expand Up @@ -327,7 +336,9 @@ describe('play command', () => {
await playCommand.execute({ client, interaction } as any)

expect(errorLogMock).toHaveBeenCalledWith(
expect.objectContaining({ message: 'Failed to record contribution' }),
expect.objectContaining({
message: 'Failed to record contribution',
}),
)
expect(interactionReplyMock).toHaveBeenCalled()
})
Expand Down Expand Up @@ -718,4 +729,53 @@ describe('play command', () => {
}),
)
})

it('registers interaction reply as now-playing message to prevent duplicate (queuePosition=0)', async () => {
// When the track starts immediately (no pre-existing queue), the /play
// interaction reply shows "Now Playing". Without registration, the
// playerStart handler sends a second "Now Playing" message. This test
// verifies the registration happens so the handler edits instead.
const interaction = createInteraction('guild-1')
const track = {
id: 'track-1',
url: 'https://youtube.com/watch?v=abc',
title: 'Test Song',
author: 'Test Artist',
duration: '3:30',
thumbnail: null,
requestedBy: { id: 'user-1' },
metadata: {},
}

resolveGuildQueueMock.mockReturnValue({ queue: null }) // no pre-existing queue
const queue = {
tracks: { size: 0, toArray: jest.fn(() => []) },
repeatMode: 0,
}
resolveGuildQueueMock
.mockReturnValueOnce({ queue: null })
.mockReturnValue({ queue })

await playCommand.execute({
client: createClient(
async () => ({
track,
searchResult: { playlist: null, tracks: [track] },
}),
{ tracksSize: 0 },
),
interaction,
} as any)

await flushPromises()

// fetchReply must be called and registerNowPlayingMessage invoked with
// the reply's id and channelId β€” this prevents the duplicate embed.
expect(interaction.fetchReply).toHaveBeenCalled()
expect(registerNowPlayingMessageMock).toHaveBeenCalledWith(
'guild-1',
'msg-123',
'channel-1',
)
})
})
18 changes: 18 additions & 0 deletions packages/bot/src/functions/music/commands/play/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
} from '../../../../utils/music/queueManipulation'
import { buildPlayResponseEmbed } from '../../../../utils/music/nowPlayingEmbed'
import { createMusicControlButtons } from '../../../../utils/music/buttonComponents'
import { registerNowPlayingMessage } from '../../../../handlers/player/trackNowPlaying'
import {
DISCORD_UNKNOWN_INTERACTION_CODE,
isUnknownInteractionError,
Expand Down Expand Up @@ -261,6 +262,23 @@ export default new Command({
content: { embeds: [embed], components },
})

// When the track starts playing immediately (queuePosition === 0),
// register the interaction reply as the "now playing" message so
// the playerStart handler edits it (adding buttons) rather than
// sending a second "Now Playing" message in the channel.
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
}
Comment on lines +277 to +279

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 | 🟑 Minor

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.

Suggested change
} 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.

}
Comment on lines +269 to +280

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

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.

Suggested change
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).


// Start background ops (apply autoplay pref then blend) without awaiting.
// This lets the response reach the user immediately.
// The Promise is not awaited, allowing the command handler to return while
Expand Down
14 changes: 14 additions & 0 deletions packages/bot/src/handlers/player/trackNowPlaying.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,20 @@ const songInfoMessages = new Map<
string,
{ messageId: string; channelId: string }
>()

/**
* Register an existing message as the "now playing" display for a guild.
* Used by the /play command to pre-register its interaction reply so that
* the playerStart handler edits it (with buttons) instead of sending a
* duplicate "Now Playing" message.
*/
export function registerNowPlayingMessage(
guildId: string,
messageId: string,
channelId: string,
): void {
songInfoMessages.set(guildId, { messageId, channelId })
}
const lastFmTrackStartTime = new Map<string, number>()

function getLastFmRequesterId(
Expand Down