Repository navigation
fix: stabilize autoplay recommendations and shuffle behavior - #129
Closed
LucasSantana-Dev wants to merge 3 commits into
Closed
LucasSantana-Dev wants to merge 3 commits into
LucasSantana-Dev wants to merge 3 commits into
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
✅ Deploy Preview for regal-bunny-0c8efe ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
dependencies
📝 WalkthroughWalkthroughThis PR introduces autoplay repeat mode support with anti-repeat filtering and queue buffering, refactors OAuth redirect URI handling for proxy deployments, extends music repeat modes end-to-end across backend/bot/frontend/shared types, and improves E2E test stability with role-based locators. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
LucasSantana-Dev
force-pushed
the
fix/autoplay-recommendation-loop-shuffle
branch
from
March 10, 2026 02:44
fdcb4c1 to
0ffd4bd
Compare
github-actions
Bot
added
size/xl
and removed
dependencies
|
Size Change: +14 B (0%) Total Size: 291 kB
ℹ️ View Unchanged
|
github-actions
Bot
added
dependencies
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/trackHandlers.ts (1)
138-138:⚠️ Potential issue | 🟡 MinorReplace magic number
3withQueueRepeatMode.AUTOPLAY.This line uses a hardcoded
3while lines 99 and 155 correctly useQueueRepeatMode.AUTOPLAY. Since the constant is already imported, this should be updated for consistency.🔧 Proposed fix
- const isAutoplayEnabled = queue.repeatMode === 3 + const isAutoplayEnabled = queue.repeatMode === QueueRepeatMode.AUTOPLAY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/handlers/player/trackHandlers.ts` at line 138, Replace the magic number by using the enum: change the assignment of isAutoplayEnabled (which currently checks queue.repeatMode === 3) to compare against QueueRepeatMode.AUTOPLAY instead; update the expression that reads queue.repeatMode so it uses QueueRepeatMode.AUTOPLAY (no import change needed since QueueRepeatMode is already imported) to match lines 99 and 155.
🧹 Nitpick comments (11)
packages/frontend/tests/e2e/visual/pages.spec.ts (1)
49-52: Consider a role-based locator for the sidebar.Using
page.locator('aside').first()is fragile and couples the test to DOM structure. If the page adds another<aside>element before this one, the test will silently capture the wrong element.A role-based approach is more resilient and aligns with accessibility testing patterns:
♻️ Proposed refactor
- const sidebar = page.locator('aside').first() + const sidebar = page.getByRole('complementary').first()Alternatively, if the sidebar has a test ID or accessible name, prefer
getByTestIdorgetByRole('complementary', { name: '...' })for even more specificity. As per coding guidelines: "Provide accessible UI components using semantic HTML and ARIA attributes where necessary."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/tests/e2e/visual/pages.spec.ts` around lines 49 - 52, The test uses a fragile locator page.locator('aside').first() assigned to sidebar before calling sidebar.toHaveScreenshot; replace this with a role- or test-id-based locator to target the real sidebar reliably (e.g., use page.getByRole('complementary', { name: 'Sidebar' }) or page.getByTestId('sidebar') if a data-testid exists) and keep the toHaveScreenshot assertion (toHaveScreenshot) against that new locator; update the test markup or add an accessible name/test id if needed so the locator is stable and descriptive.packages/frontend/tests/e2e/visual/login-page.spec.ts (2)
33-35: Consider role-based locators for buttons.Using
page.locator('button:has-text("...")')couples tests to exact button text. Role-based locators are more resilient to copy changes and align better with accessibility testing:♻️ Proposed refactor using getByRole
- const loginButton = page.locator( - 'button:has-text("Login with Discord")', - ) + const loginButton = page.getByRole('button', { + name: /login with discord/i, + })- const loadingButton = page.locator('button:has-text("Connecting")') + const loadingButton = page.getByRole('button', { name: /connecting/i })The regex with case-insensitive flag makes it tolerant of minor text casing changes while still validating the accessible name. As per coding guidelines: "Provide accessible UI components using semantic HTML and ARIA attributes where necessary."
Also applies to: 48-50, 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/tests/e2e/visual/login-page.spec.ts` around lines 33 - 35, Replace the string-based locator page.locator('button:has-text("Login with Discord")') with a role-based accessible locator using page.getByRole('button', { name: /login with discord/i }) to make the test resilient to copy/casing changes; apply the same change for the other similar occurrences (the other page.locator('button:has-text(...)') instances) so all button finds use page.getByRole('button', { name: /.../i }) with an appropriate case-insensitive regex for the accessible name.
52-55: Preferpage.waitForTimeoutover rawsetTimeout.The change from
page.waitForTimeout(1000)tonew Promise((resolve) => setTimeout(resolve, 1000))is functionally identical but less idiomatic. Playwright's built-in method is clearer in intent, integrates with Playwright's tracing/debugging, and is the established pattern elsewhere in this codebase.♻️ Revert to idiomatic Playwright API
await page.route('**/api/auth/discord', async (route) => { - await new Promise((resolve) => setTimeout(resolve, 1000)) + await page.waitForTimeout(1000) await route.continue() })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/tests/e2e/visual/login-page.spec.ts` around lines 52 - 55, In the page.route handler that delays the Discord auth request, replace the manual Promise-based delay with Playwright's idiomatic page.waitForTimeout(1000): inside the async callback passed to page.route('**/api/auth/discord', async (route) => { ... }), call await page.waitForTimeout(1000) before invoking route.continue() so the delay integrates with Playwright tracing and matches the codebase pattern.packages/bot/src/functions/music/commands/queue/queueStats.ts (1)
70-78: Consider displayingQueueRepeatMode.QUEUEstatus.The function shows status for
TRACK(single-track loop) andAUTOPLAY, but omitsQueueRepeatMode.QUEUE(entire queue loop). Users won't see a status indicator when queue-loop is active.♻️ Optional: Add QUEUE mode display
export function getQueueStatus(queue: GuildQueue): string { const status = [] if (queue.repeatMode === QueueRepeatMode.TRACK) status.push('🔁 Loop') + if (queue.repeatMode === QueueRepeatMode.QUEUE) status.push('🔁 Queue Loop') if (queue.repeatMode === QueueRepeatMode.AUTOPLAY) status.push('🔄 Autoplay') if (queue.node.isPaused()) status.push('⏸️ Paused') return status.length > 0 ? status.join(' • ') : '▶️ Playing' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/music/commands/queue/queueStats.ts` around lines 70 - 78, getQueueStatus currently only shows TRACK and AUTOPLAY repeat modes; add a check for QueueRepeatMode.QUEUE so users see when the whole queue is looping. Update the getQueueStatus function to include a branch like "if (queue.repeatMode === QueueRepeatMode.QUEUE) status.push('🔁 Queue')" (or another concise label you prefer) alongside the existing checks for QueueRepeatMode.TRACK and QueueRepeatMode.AUTOPLAY; reference QueueRepeatMode and the getQueueStatus function to locate where to insert this new condition.packages/frontend/tests/e2e/servers-page.spec.ts (3)
60-64: Conditional test logic may silently pass without assertion.Several tests use
if (condition)guards around assertions (lines 61-63, 73-75, 85-92, 103-109). If the condition evaluates tofalse, the test passes without verifying anything. This can hide fixture data issues or unexpected changes toMOCK_GUILDS.Consider either:
- Asserting that the expected data exists in fixtures as a precondition.
- Removing the conditional and ensuring fixtures always contain the required test data.
♻️ Example fix for lines 60-64
test('displays Bot Added badge for servers with bot', async ({ page }) => { await navigateToServers(page) await waitForServerList(page) const serverWithBot = MOCK_GUILDS.find((g) => g.hasBot) - if (serverWithBot) { - await verifyBadge(page, 'Bot Added') - } + expect(serverWithBot).toBeDefined() + await verifyBadge(page, 'Bot Added') })Also applies to: 72-76, 84-92, 102-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/tests/e2e/servers-page.spec.ts` around lines 60 - 64, The test currently uses a conditional guard around the assertion (serverWithBot = MOCK_GUILDS.find(...)) so if the fixture lacks the item the test silently passes; update the test to make the existence a hard precondition by asserting the fixture first (e.g., assert that MOCK_GUILDS.some(g => g.hasBot) or expect(serverWithBot).toBeDefined()) before calling verifyBadge, or remove the if and ensure fixtures always include the required guild; locate the usage of MOCK_GUILDS, serverWithBot and verifyBadge in the servers-page.spec.ts test cases and replace the conditional with a deterministic assertion that fails when test data is missing.
78-93: HardcodedwaitForTimeoutcalls reduce test reliability.Lines 90, 108, and 168 use
page.waitForTimeout()with fixed delays. This pattern leads to flaky tests—either failing under slow CI or wasting time waiting longer than necessary. Prefer waiting for specific conditions:
- Line 90-91: Wait for URL change instead of arbitrary delay.
- Line 108: Consider waiting for a navigation event or expected UI state.
- Line 168: Wait for error UI to appear rather than a fixed 2-second delay.
♻️ Suggested fix for lines 90-91
- await page.waitForTimeout(1000) - expect(page.url()).not.toContain('/servers') + await expect(page).not.toHaveURL(/\/servers/)Also applies to: 95-110, 156-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/tests/e2e/servers-page.spec.ts` around lines 78 - 93, The test uses hardcoded page.waitForTimeout(1000) after clicking the manage button which causes flakiness; replace it with a precise wait such as awaiting navigation or URL change (e.g., use page.waitForURL or page.waitForNavigation) or assert the expected UI element on the dashboard appears before checking the URL; locate the click in the test "Manage button navigates to dashboard for servers with bot" (helpers: navigateToServers, waitForServerList, getManageButton) and swap the fixed timeout for a targeted wait for the navigation/element that indicates the dashboard has loaded.
112-132: Inconsistent route cleanup across tests.The added
page.unroute('**/api/guilds')at line 131 is good practice for cleanup, but this creates inconsistency within the file. The tests at lines 134-154 and 156-169 also set up custom routes on**/api/guildsbut do not callunroute. Consider applying the same cleanup pattern to all tests that set up custom routes, or rely on Playwright's test isolation if cleanup is unnecessary.Additionally, the change from
page.waitForTimeout(1000)tonew Promise(resolve => setTimeout(resolve, 1000))is functionally equivalent for introducing delay in route handlers—neither approach improves test reliability over the other.♻️ Suggested fix: Apply consistent cleanup to other tests
Add cleanup to the empty state test:
if (isEmptyVisible) { await expect(emptyState).toBeVisible() } + + await page.unroute('**/api/guilds') })Add cleanup to the error handling test:
await page.waitForTimeout(2000) + + await page.unroute('**/api/guilds') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/tests/e2e/servers-page.spec.ts` around lines 112 - 132, Any test that sets up a custom route for '**/api/guilds' (e.g., the "shows loading skeleton during data fetch" test that calls page.route) should clean it up; add page.unroute('**/api/guilds') at the end of every test that calls page.route('**/api/guilds') (or centralize cleanup in a test.afterEach hook) to make route teardown consistent; leave the existing route delay implementation as-is (the new Promise delay is fine but does not affect cleanup).packages/backend/tests/integration/routes/auth.test.ts (1)
60-79: Consider using try/finally for environment variable cleanup.The test correctly validates redirect URI derivation from forwarded headers, but the environment variable restoration could be skipped if the test throws before reaching the cleanup code.
♻️ Suggested improvement for robust cleanup
test('should derive redirect uri from forwarded host when env is unset', async () => { const originalRedirectUri = process.env.WEBAPP_REDIRECT_URI delete process.env.WEBAPP_REDIRECT_URI - const response = await request(app) - .get('/api/auth/discord') - .set('x-forwarded-proto', 'https') - .set('x-forwarded-host', 'lucky.lucassantana.tech') - .expect(302) - - expect(response.headers.location).toContain( - encodeURIComponent( - 'https://lucky.lucassantana.tech/api/auth/callback', - ), - ) - - if (originalRedirectUri) { - process.env.WEBAPP_REDIRECT_URI = originalRedirectUri - } + try { + const response = await request(app) + .get('/api/auth/discord') + .set('x-forwarded-proto', 'https') + .set('x-forwarded-host', 'lucky.lucassantana.tech') + .expect(302) + + expect(response.headers.location).toContain( + encodeURIComponent( + 'https://lucky.lucassantana.tech/api/auth/callback', + ), + ) + } finally { + if (originalRedirectUri) { + process.env.WEBAPP_REDIRECT_URI = originalRedirectUri + } + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/integration/routes/auth.test.ts` around lines 60 - 79, The test that deletes process.env.WEBAPP_REDIRECT_URI should ensure restoration in a finally block: wrap the request(app).get('/api/auth/discord') call and assertions inside a try, and move the environment restore of process.env.WEBAPP_REDIRECT_URI into a finally so it always runs even if the test throws; reference the existing test block (the 'should derive redirect uri from forwarded host when env is unset' test), the process.env.WEBAPP_REDIRECT_URI variable, and the request(app).get('/api/auth/discord') invocation when making this change.packages/frontend/src/types/music.ts (1)
13-20: Extract a frontendTRepeatModetype alias here.This union appears in 4 locations: this file (QueueState.repeatMode),
packages/frontend/src/pages/Music.tsx(repeat mode cycling),packages/frontend/src/services/musicApi.ts(repeat function parameter), andpackages/frontend/src/hooks/useMusicCommands.ts(setRepeatMode hook parameter). Exporting a singleTRepeatModetype frompackages/frontend/src/types/music.tsand reusing it will keep the frontend contract consistent.♻️ Proposed diff
+export type TRepeatMode = 'off' | 'track' | 'queue' | 'autoplay' + export interface QueueState { guildId: string currentTrack: TrackInfo | null tracks: TrackInfo[] isPlaying: boolean isPaused: boolean volume: number - repeatMode: 'off' | 'track' | 'queue' | 'autoplay' + repeatMode: TRepeatMode shuffled: boolean position: number voiceChannelId: string | null voiceChannelName: string | null timestamp: number🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/types/music.ts` around lines 13 - 20, Create and export a type alias TRepeatMode in packages/frontend/src/types/music.ts and replace the inline union in QueueState.repeatMode with this alias; then update all frontend usages to import and use TRepeatMode (specifically the repeat mode cycling logic in Music.tsx, the repeat function parameter in services/musicApi.ts, and the setRepeatMode hook parameter in hooks/useMusicCommands.ts) so the union 'off' | 'track' | 'queue' | 'autoplay' is centralized and consistent across QueueState.repeatMode, the repeat handler in Music.tsx, the repeat API function signature, and the setRepeatMode hook.packages/bot/src/handlers/webMusic/mappers.ts (1)
53-63: Type this mapper with the sharedRepeatModecontract.The function accepts
stringwhich drops the sharedRepeatModecontract and silently defaults invalid values toOFF. Change the parameter toRepeatModeand add an explicit'off'case to enforce type safety at the module boundary and force callers to validate input before calling this mapper.♻️ Proposed diff
import type { MusicTrackInfo as TrackInfo, QueueState, + RepeatMode, } from '@lucky/shared/services' @@ -export function repeatModeToEnum(mode: string): QueueRepeatMode { +export function repeatModeToEnum(mode: RepeatMode): QueueRepeatMode { switch (mode) { + case 'off': + return QueueRepeatMode.OFF case 'track': return QueueRepeatMode.TRACK case 'queue': return QueueRepeatMode.QUEUE case 'autoplay': return QueueRepeatMode.AUTOPLAY - default: - return QueueRepeatMode.OFF } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/handlers/webMusic/mappers.ts` around lines 53 - 63, Change the repeatModeToEnum signature to accept the shared RepeatMode type instead of string, add an explicit case for 'off' that returns QueueRepeatMode.OFF, and stop silently defaulting invalid strings (replace the current default with either no default and an exhaustive-check throw or an assertion) so callers must supply a valid RepeatMode; update the function repeatModeToEnum to use RepeatMode and include cases 'track', 'queue', 'autoplay', and 'off' (and an explicit error for any unexpected value).packages/bot/src/utils/music/queueManipulation.ts (1)
105-202: Extract the autoplay recommendation pipeline into smaller units.This now mixes buffer math, history extraction, remote search, duplicate filtering, scoring, and enqueue side effects in one path. Splitting those steps into focused helpers/modules will make the autoplay rules easier to verify and bring the module back inside the repo's size limits.
As per coding guidelines "Functions must be less than 50 lines with cyclomatic complexity less than 10" and "Files must not exceed 250 lines and this is enforced".
Also applies to: 204-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/queueManipulation.ts` around lines 105 - 202, The replenishQueue function is doing too many responsibilities; extract its steps into small helpers: (1) move buffer math and early returns into a helper like computeMissingTracks(queue) returning missingTracks and currentTrack; (2) extract history/seed/exclusion building into buildSeedsAndExclusions(queue, currentTrack) that returns seeds, excludedUrls, excludedKeys, recentArtists; (3) extract remote searching per seed into fetchCandidatesFromSeed(seed, requestedBy, queue.player) which returns raw candidate tracks; (4) encapsulate duplicate filtering and scoring into filterAndScoreCandidates(candidates, excludedUrls, excludedKeys, currentTrack, recentArtists) that returns a Map<string,ScoredTrack>; and (5) move selection, marking and enqueue side-effects into selectAndEnqueue(candidatesMap, missingTracks, queue). Update replenishQueue to orchestrate these helpers and keep it under 50 lines, ensuring only minimal orchestration and error handling remain in replenishQueue while all logic lives in the new helper functions (use existing symbols: replenishQueue, getHistoryTracks, normalizeTrackKey, calculateRecommendationScore, markAsAutoplayTrack, queue.addTrack).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/sonarcloud.yml:
- Around line 46-48: The "Run frontend tests with coverage (lcov)" step
currently uses continue-on-error: true which swallows frontend test failures;
remove the continue-on-error: true setting from that step so the workflow fails
on test/coverage errors, and instead ensure the Sonar scan step uses if: ${{
always() }} (add that condition to the Sonar-related job/step) so the scan still
runs even when tests fail.
In `@CHANGELOG.md`:
- Line 43: This line "Web music repeat mode now supports `autoplay` end-to-end
(bot mapper, backend validation, shared/frontend types)" is a new feature note
misplaced under the `### Fixed` section; move that bullet out of the `Fixed`
section and place it under `### Added` (or `### Changed`) in CHANGELOG.md so the
release notes correctly reflect it as a new capability rather than a bug fix.
- Around line 41-45: Update the unreleased bullets in CHANGELOG.md by appending
the PR reference " (`#129`)" to each of the listed items (e.g., after the lines
starting "Autoplay no longer keeps cycling the same recommendations...",
"Shuffle now works reliably...", "Web music repeat mode now supports
`autoplay`...", "OAuth callback now reuses the same redirect URI...", and "E2E
stability improvements...") so every bullet points back to PR `#129` for
traceability.
In `@packages/backend/src/routes/music/playbackRoutes.ts`:
- Around line 114-118: Replace the inline includes(...) validation in the
playback route with a Zod body schema and validateBody: add and export
repeatModeBodySchema = z.object({ mode:
z.enum(['off','track','queue','autoplay']) }) in backend/src/schemas, import
that schema into the playback route and call validateBody(repeatModeBodySchema,
req) at the start of the handler, then use the validated mode instead of reading
req.body and remove the AppError.badRequest(...) branch; ensure imports and
types are updated so the handler receives the validated payload.
In `@packages/backend/src/utils/oauthRedirectUri.ts`:
- Around line 13-23: buildRequestRedirectUri currently trusts x-forwarded-host
and req.get('host') without validation; replicate the allowed-origin validation
used in frontendOrigin.ts to avoid header injection. Update
buildRequestRedirectUri to derive the host (from getForwardedHeader or
req.get('host')), then validate that host against the configured allowed origins
list (same source/logic as frontendOrigin.ts) and reject/replace unrecognized
hosts (fallback to localhost with WEBAPP_PORT) before composing the final
`${protocol}://${host}/api/auth/callback`; ensure getForwardedHeader usage
remains but its result is validated and sanitized, and surface a safe default
when validation fails.
In `@packages/bot/src/utils/music/duplicateDetection/duplicateChecker.ts`:
- Around line 141-155: The code calls trackHistoryService.addTrackToHistory(...)
but treats it as always successful; change both call sites (the
addTrackToHistory invocations in duplicateChecker.ts) to capture the returned
boolean result and only emit the "Track added to history" success log when the
result is true; if it returns false, log a warning or error that includes
identifying info (e.g., track.id or track.url and guildId) so dropped writes are
visible and autoplay de-duplication won’t be silently weakened.
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 160-168: The candidates map must use a normalized key so the same
song from different URLs/providers doesn't appear multiple times: replace usages
of getTrackKey(candidate) when reading/writing the candidates Map (see
getTrackKey, candidates) with a normalized identifier derived from the track
(e.g., prefer track.id if present, otherwise normalize/canonicalize the URL) and
use that normalizedKey for candidates.get and candidates.set; apply the same
change to the other occurrences noted (the blocks around the other getTrackKey
calls at the referenced ranges) so aggregation dedupes by canonical track
identity before comparing scores.
---
Outside diff comments:
In `@packages/bot/src/handlers/player/trackHandlers.ts`:
- Line 138: Replace the magic number by using the enum: change the assignment of
isAutoplayEnabled (which currently checks queue.repeatMode === 3) to compare
against QueueRepeatMode.AUTOPLAY instead; update the expression that reads
queue.repeatMode so it uses QueueRepeatMode.AUTOPLAY (no import change needed
since QueueRepeatMode is already imported) to match lines 99 and 155.
---
Nitpick comments:
In `@packages/backend/tests/integration/routes/auth.test.ts`:
- Around line 60-79: The test that deletes process.env.WEBAPP_REDIRECT_URI
should ensure restoration in a finally block: wrap the
request(app).get('/api/auth/discord') call and assertions inside a try, and move
the environment restore of process.env.WEBAPP_REDIRECT_URI into a finally so it
always runs even if the test throws; reference the existing test block (the
'should derive redirect uri from forwarded host when env is unset' test), the
process.env.WEBAPP_REDIRECT_URI variable, and the
request(app).get('/api/auth/discord') invocation when making this change.
In `@packages/bot/src/functions/music/commands/queue/queueStats.ts`:
- Around line 70-78: getQueueStatus currently only shows TRACK and AUTOPLAY
repeat modes; add a check for QueueRepeatMode.QUEUE so users see when the whole
queue is looping. Update the getQueueStatus function to include a branch like
"if (queue.repeatMode === QueueRepeatMode.QUEUE) status.push('🔁 Queue')" (or
another concise label you prefer) alongside the existing checks for
QueueRepeatMode.TRACK and QueueRepeatMode.AUTOPLAY; reference QueueRepeatMode
and the getQueueStatus function to locate where to insert this new condition.
In `@packages/bot/src/handlers/webMusic/mappers.ts`:
- Around line 53-63: Change the repeatModeToEnum signature to accept the shared
RepeatMode type instead of string, add an explicit case for 'off' that returns
QueueRepeatMode.OFF, and stop silently defaulting invalid strings (replace the
current default with either no default and an exhaustive-check throw or an
assertion) so callers must supply a valid RepeatMode; update the function
repeatModeToEnum to use RepeatMode and include cases 'track', 'queue',
'autoplay', and 'off' (and an explicit error for any unexpected value).
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 105-202: The replenishQueue function is doing too many
responsibilities; extract its steps into small helpers: (1) move buffer math and
early returns into a helper like computeMissingTracks(queue) returning
missingTracks and currentTrack; (2) extract history/seed/exclusion building into
buildSeedsAndExclusions(queue, currentTrack) that returns seeds, excludedUrls,
excludedKeys, recentArtists; (3) extract remote searching per seed into
fetchCandidatesFromSeed(seed, requestedBy, queue.player) which returns raw
candidate tracks; (4) encapsulate duplicate filtering and scoring into
filterAndScoreCandidates(candidates, excludedUrls, excludedKeys, currentTrack,
recentArtists) that returns a Map<string,ScoredTrack>; and (5) move selection,
marking and enqueue side-effects into selectAndEnqueue(candidatesMap,
missingTracks, queue). Update replenishQueue to orchestrate these helpers and
keep it under 50 lines, ensuring only minimal orchestration and error handling
remain in replenishQueue while all logic lives in the new helper functions (use
existing symbols: replenishQueue, getHistoryTracks, normalizeTrackKey,
calculateRecommendationScore, markAsAutoplayTrack, queue.addTrack).
In `@packages/frontend/src/types/music.ts`:
- Around line 13-20: Create and export a type alias TRepeatMode in
packages/frontend/src/types/music.ts and replace the inline union in
QueueState.repeatMode with this alias; then update all frontend usages to import
and use TRepeatMode (specifically the repeat mode cycling logic in Music.tsx,
the repeat function parameter in services/musicApi.ts, and the setRepeatMode
hook parameter in hooks/useMusicCommands.ts) so the union 'off' | 'track' |
'queue' | 'autoplay' is centralized and consistent across QueueState.repeatMode,
the repeat handler in Music.tsx, the repeat API function signature, and the
setRepeatMode hook.
In `@packages/frontend/tests/e2e/servers-page.spec.ts`:
- Around line 60-64: The test currently uses a conditional guard around the
assertion (serverWithBot = MOCK_GUILDS.find(...)) so if the fixture lacks the
item the test silently passes; update the test to make the existence a hard
precondition by asserting the fixture first (e.g., assert that
MOCK_GUILDS.some(g => g.hasBot) or expect(serverWithBot).toBeDefined()) before
calling verifyBadge, or remove the if and ensure fixtures always include the
required guild; locate the usage of MOCK_GUILDS, serverWithBot and verifyBadge
in the servers-page.spec.ts test cases and replace the conditional with a
deterministic assertion that fails when test data is missing.
- Around line 78-93: The test uses hardcoded page.waitForTimeout(1000) after
clicking the manage button which causes flakiness; replace it with a precise
wait such as awaiting navigation or URL change (e.g., use page.waitForURL or
page.waitForNavigation) or assert the expected UI element on the dashboard
appears before checking the URL; locate the click in the test "Manage button
navigates to dashboard for servers with bot" (helpers: navigateToServers,
waitForServerList, getManageButton) and swap the fixed timeout for a targeted
wait for the navigation/element that indicates the dashboard has loaded.
- Around line 112-132: Any test that sets up a custom route for '**/api/guilds'
(e.g., the "shows loading skeleton during data fetch" test that calls
page.route) should clean it up; add page.unroute('**/api/guilds') at the end of
every test that calls page.route('**/api/guilds') (or centralize cleanup in a
test.afterEach hook) to make route teardown consistent; leave the existing route
delay implementation as-is (the new Promise delay is fine but does not affect
cleanup).
In `@packages/frontend/tests/e2e/visual/login-page.spec.ts`:
- Around line 33-35: Replace the string-based locator
page.locator('button:has-text("Login with Discord")') with a role-based
accessible locator using page.getByRole('button', { name: /login with discord/i
}) to make the test resilient to copy/casing changes; apply the same change for
the other similar occurrences (the other page.locator('button:has-text(...)')
instances) so all button finds use page.getByRole('button', { name: /.../i })
with an appropriate case-insensitive regex for the accessible name.
- Around line 52-55: In the page.route handler that delays the Discord auth
request, replace the manual Promise-based delay with Playwright's idiomatic
page.waitForTimeout(1000): inside the async callback passed to
page.route('**/api/auth/discord', async (route) => { ... }), call await
page.waitForTimeout(1000) before invoking route.continue() so the delay
integrates with Playwright tracing and matches the codebase pattern.
In `@packages/frontend/tests/e2e/visual/pages.spec.ts`:
- Around line 49-52: The test uses a fragile locator
page.locator('aside').first() assigned to sidebar before calling
sidebar.toHaveScreenshot; replace this with a role- or test-id-based locator to
target the real sidebar reliably (e.g., use page.getByRole('complementary', {
name: 'Sidebar' }) or page.getByTestId('sidebar') if a data-testid exists) and
keep the toHaveScreenshot assertion (toHaveScreenshot) against that new locator;
update the test markup or add an accessible name/test id if needed so the
locator is stable and descriptive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fea4de5-764d-4029-a821-dc73984ae89e
⛔ Files ignored due to path filters (10)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/frontend/tests/e2e/visual/login-page.spec.ts-snapshots/login-button-hover-chromium-darwin.pngis excluded by!**/*.pngpackages/frontend/tests/e2e/visual/login-page.spec.ts-snapshots/login-page-chromium-darwin.pngis excluded by!**/*.pngpackages/frontend/tests/e2e/visual/login-page.spec.ts-snapshots/login-page-error-chromium-darwin.pngis excluded by!**/*.pngpackages/frontend/tests/e2e/visual/pages.spec.ts-snapshots/dashboard-page-chromium-darwin.pngis excluded by!**/*.pngpackages/frontend/tests/e2e/visual/pages.spec.ts-snapshots/features-page-chromium-darwin.pngis excluded by!**/*.pngpackages/frontend/tests/e2e/visual/pages.spec.ts-snapshots/servers-page-chromium-darwin.pngis excluded by!**/*.pngpackages/frontend/tests/e2e/visual/pages.spec.ts-snapshots/servers-page-error-chromium-darwin.pngis excluded by!**/*.pngpackages/frontend/tests/e2e/visual/pages.spec.ts-snapshots/servers-page-loading-chromium-darwin.pngis excluded by!**/*.pngpackages/frontend/tests/e2e/visual/pages.spec.ts-snapshots/sidebar-chromium-darwin.pngis excluded by!**/*.png
📒 Files selected for processing (30)
.github/workflows/sonarcloud.ymlCHANGELOG.mdREADME.mdpackages/backend/src/routes/auth.tspackages/backend/src/routes/authCallback.tspackages/backend/src/routes/music/playbackRoutes.tspackages/backend/src/services/DiscordOAuthService.tspackages/backend/src/types/session.d.tspackages/backend/src/utils/oauthRedirectUri.tspackages/backend/tests/integration/api.test.tspackages/backend/tests/integration/routes/auth.test.tspackages/bot/src/functions/music/commands/queue/queueStats.tspackages/bot/src/handlers/player/trackHandlers.tspackages/bot/src/handlers/webMusic/mappers.test.tspackages/bot/src/handlers/webMusic/mappers.tspackages/bot/src/utils/music/duplicateDetection/duplicateChecker.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/utils/music/queueManipulation.tspackages/frontend/package.jsonpackages/frontend/src/hooks/useMusicCommands.tspackages/frontend/src/pages/Music.tsxpackages/frontend/src/services/musicApi.tspackages/frontend/src/types/music.tspackages/frontend/tests/e2e/dashboard-page.spec.tspackages/frontend/tests/e2e/servers-page.spec.tspackages/frontend/tests/e2e/track-history-page.spec.tspackages/frontend/tests/e2e/visual/login-page.spec.tspackages/frontend/tests/e2e/visual/pages.spec.tspackages/shared/src/services/music/types.tssonar-project.properties
Comment on lines
+46
to
+48
| - name: Run frontend tests with coverage (lcov) | ||
| run: npm run test --workspace=packages/frontend -- --coverage --coverage.reporter=lcov --coverage.reporter=text | ||
| continue-on-error: true |
There was a problem hiding this comment.
Don't swallow frontend test failures.
Line 48 makes this new test/coverage step non-blocking, so a broken frontend suite or bad coverage config can still leave the workflow green and Sonar running with incomplete data. Let the test step fail, and put if: ${{ always() }} on the Sonar step if you still want the scan to execute.
Suggested change
- name: Run frontend tests with coverage (lcov)
run: npm run test --workspace=packages/frontend -- --coverage --coverage.reporter=lcov --coverage.reporter=text
- continue-on-error: true
- name: SonarCloud Scan
+ if: ${{ always() }}
uses: SonarSource/sonarqube-scan-action@v6🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/sonarcloud.yml around lines 46 - 48, The "Run frontend
tests with coverage (lcov)" step currently uses continue-on-error: true which
swallows frontend test failures; remove the continue-on-error: true setting from
that step so the workflow fails on test/coverage errors, and instead ensure the
Sonar scan step uses if: ${{ always() }} (add that condition to the
Sonar-related job/step) so the scan still runs even when tests fail.
Comment on lines
+41
to
+45
| - Autoplay no longer keeps cycling the same recommendations; queue top-up now uses anti-repeat filtering and keeps a 4-track buffer | ||
| - Shuffle now works reliably while autoplay is enabled because autoplay maintains enough upcoming tracks | ||
| - Web music repeat mode now supports `autoplay` end-to-end (bot mapper, backend validation, shared/frontend types) | ||
| - OAuth callback now reuses the same redirect URI across auth start/callback token exchange, with forwarded-host fallback for proxied HTTPS deployments | ||
| - E2E stability improvements: dashboard/servers/track-history tests now use deterministic locators and route-delay handling |
There was a problem hiding this comment.
Add the PR reference to these release notes.
These new unreleased bullets should point back to #129 so the shipped behavior can be traced after release.
As per coding guidelines, "Update CHANGELOG.md with all changes, include breaking changes documentation, and reference issues and PRs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 41 - 45, Update the unreleased bullets in
CHANGELOG.md by appending the PR reference " (`#129`)" to each of the listed items
(e.g., after the lines starting "Autoplay no longer keeps cycling the same
recommendations...", "Shuffle now works reliably...", "Web music repeat mode now
supports `autoplay`...", "OAuth callback now reuses the same redirect URI...",
and "E2E stability improvements...") so every bullet points back to PR `#129` for
traceability.
| - Vercel build now generates Prisma client before shared/frontend builds to prevent missing generated client errors | ||
| - Autoplay no longer keeps cycling the same recommendations; queue top-up now uses anti-repeat filtering and keeps a 4-track buffer | ||
| - Shuffle now works reliably while autoplay is enabled because autoplay maintains enough upcoming tracks | ||
| - Web music repeat mode now supports `autoplay` end-to-end (bot mapper, backend validation, shared/frontend types) |
There was a problem hiding this comment.
Move the autoplay repeat mode note out of Fixed.
Line 43 documents a new end-to-end capability, not a bug fix. Keeping it under ### Fixed will make the release notes misleading; ### Added or ### Changed fits better.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 43, This line "Web music repeat mode now supports
`autoplay` end-to-end (bot mapper, backend validation, shared/frontend types)"
is a new feature note misplaced under the `### Fixed` section; move that bullet
out of the `Fixed` section and place it under `### Added` (or `### Changed`) in
CHANGELOG.md so the release notes correctly reflect it as a new capability
rather than a bug fix.
Comment on lines
114
to
+118
| const { mode } = req.body | ||
| if (!['off', 'track', 'queue'].includes(mode)) { | ||
| throw AppError.badRequest('Mode must be off, track, or queue') | ||
| if (!['off', 'track', 'queue', 'autoplay'].includes(mode)) { | ||
| throw AppError.badRequest( | ||
| 'Mode must be off, track, queue, or autoplay', | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move repeat-mode validation into the backend schema layer.
This works, but it keeps HTTP input validation duplicated inside the route and creates another string-literal source of truth for repeat modes. Please push this through a backend/src/schemas/ Zod body schema and validateBody(...) instead of extending the inline includes(...) check here.
Suggested direction
- const { mode } = req.body
- if (!['off', 'track', 'queue', 'autoplay'].includes(mode)) {
- throw AppError.badRequest(
- 'Mode must be off, track, queue, or autoplay',
- )
- }
+ const { mode } = repeatModeBodySchema.parse(req.body)export const repeatModeBodySchema = z.object({
mode: z.enum(['off', 'track', 'queue', 'autoplay']),
})As per coding guidelines "packages/backend/src/**/*.ts: Validation must use Zod schemas in backend/src/schemas/ and be applied via validateBody, validateParams, or validateQuery".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/routes/music/playbackRoutes.ts` around lines 114 - 118,
Replace the inline includes(...) validation in the playback route with a Zod
body schema and validateBody: add and export repeatModeBodySchema = z.object({
mode: z.enum(['off','track','queue','autoplay']) }) in backend/src/schemas,
import that schema into the playback route and call
validateBody(repeatModeBodySchema, req) at the start of the handler, then use
the validated mode instead of reading req.body and remove the
AppError.badRequest(...) branch; ensure imports and types are updated so the
handler receives the validated payload.
Comment on lines
+13
to
+23
| const buildRequestRedirectUri = (req: Request): string => { | ||
| const forwardedProto = getForwardedHeader(req, 'x-forwarded-proto') | ||
| const forwardedHost = getForwardedHeader(req, 'x-forwarded-host') | ||
| const protocol = forwardedProto ?? req.protocol ?? 'http' | ||
| const host = | ||
| forwardedHost ?? | ||
| req.get('host') ?? | ||
| `localhost:${process.env.WEBAPP_PORT ?? '3000'}` | ||
|
|
||
| return `${protocol}://${host}/api/auth/callback` | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if trust proxy is configured anywhere in the backend
rg -n "trust.?proxy" packages/backend/src/Repository: LucasSantana-Dev/Lucky
Length of output: 48
🏁 Script executed:
# First, let's read the actual file
cat -n packages/backend/src/utils/oauthRedirectUri.tsRepository: LucasSantana-Dev/Lucky
Length of output: 1302
🏁 Script executed:
# Check where and how getOAuthRedirectUri is used
rg -n "getOAuthRedirectUri\|buildRequestRedirectUri" packages/backend/src/ -B 2 -A 2Repository: LucasSantana-Dev/Lucky
Length of output: 48
🏁 Script executed:
# Look at server.ts to see if express app configuration includes trust proxy
cat -n packages/backend/src/server.ts | head -50Repository: LucasSantana-Dev/Lucky
Length of output: 2101
🏁 Script executed:
# Check how WEBAPP_FRONTEND_URL is used elsewhere
rg -n "WEBAPP_FRONTEND_URL" packages/ -B 2 -A 2Repository: LucasSantana-Dev/Lucky
Length of output: 1924
🏁 Script executed:
# Search for auth routes that might use this utility
fd "auth" packages/backend/src/routes/ --type fRepository: LucasSantana-Dev/Lucky
Length of output: 145
🏁 Script executed:
# Read the frontendOrigin.ts file to see the existing validation pattern
cat -n packages/backend/src/utils/frontendOrigin.tsRepository: LucasSantana-Dev/Lucky
Length of output: 646
🏁 Script executed:
# Search for any other forwarded header usage in the backend
rg -n "x-forwarded|req\.headers" packages/backend/src/ -t tsRepository: LucasSantana-Dev/Lucky
Length of output: 484
🏁 Script executed:
# Check the auth routes to see if and how getOAuthRedirectUri is used
cat -n packages/backend/src/routes/auth.ts | head -120Repository: LucasSantana-Dev/Lucky
Length of output: 5135
🏁 Script executed:
# Also check authCallback.ts
cat -n packages/backend/src/routes/authCallback.ts | head -80Repository: LucasSantana-Dev/Lucky
Length of output: 3195
🏁 Script executed:
# Search specifically for getOAuthRedirectUri import and usage
rg -n "getOAuthRedirectUri" packages/backend/src/Repository: LucasSantana-Dev/Lucky
Length of output: 572
Add host validation to prevent header injection attacks.
The utility reads x-forwarded-host without validation, and Express doesn't have trust proxy configured. While Discord validates the redirect URI against registered callbacks (mitigating the direct risk), an attacker who can inject headers could still cause unexpected behavior. The codebase already has a validation pattern in frontendOrigin.ts—apply the same approach here to validate the derived host against configured allowed origins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/backend/src/utils/oauthRedirectUri.ts` around lines 13 - 23,
buildRequestRedirectUri currently trusts x-forwarded-host and req.get('host')
without validation; replicate the allowed-origin validation used in
frontendOrigin.ts to avoid header injection. Update buildRequestRedirectUri to
derive the host (from getForwardedHeader or req.get('host')), then validate that
host against the configured allowed origins list (same source/logic as
frontendOrigin.ts) and reject/replace unrecognized hosts (fallback to localhost
with WEBAPP_PORT) before composing the final
`${protocol}://${host}/api/auth/callback`; ensure getForwardedHeader usage
remains but its result is validated and sanitized, and surface a safe default
when validation fails.
Comment on lines
+141
to
+155
| await trackHistoryService.addTrackToHistory( | ||
| { | ||
| id: track.id || track.url, | ||
| title: track.title, | ||
| author: track.author, | ||
| duration: | ||
| typeof track.duration === 'string' | ||
| ? track.duration | ||
| : String(track.duration), | ||
| url: track.url, | ||
| metadata: { isAutoplay: Boolean(metadata?.isAutoplay) }, | ||
| }, | ||
| guildId, | ||
| track.requestedBy?.id, | ||
| ) |
There was a problem hiding this comment.
Don't log a successful history write when the shared service returned false.
trackHistoryService.addTrackToHistory() reports failures by returning false, not by rethrowing. This path still emits "Track added to history" after a dropped write, which silently loses play history and weakens autoplay de-duplication.
Suggested fix
- await trackHistoryService.addTrackToHistory(
+ const persisted = await trackHistoryService.addTrackToHistory(
{
id: track.id || track.url,
title: track.title,
author: track.author,
duration:
typeof track.duration === 'string'
? track.duration
: String(track.duration),
url: track.url,
metadata: { isAutoplay: Boolean(metadata?.isAutoplay) },
},
guildId,
track.requestedBy?.id,
)
+
+ if (!persisted) {
+ errorLog({
+ message: 'Failed to persist track history',
+ data: { title: track.title, author: track.author, guildId },
+ })
+ return
+ }
debugLog({Also applies to: 157-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/utils/music/duplicateDetection/duplicateChecker.ts` around
lines 141 - 155, The code calls trackHistoryService.addTrackToHistory(...) but
treats it as always successful; change both call sites (the addTrackToHistory
invocations in duplicateChecker.ts) to capture the returned boolean result and
only emit the "Track added to history" success log when the result is true; if
it returns false, log a warning or error that includes identifying info (e.g.,
track.id or track.url and guildId) so dropped writes are visible and autoplay
de-duplication won’t be silently weakened.
Comment on lines
+160
to
+168
| const candidateKey = getTrackKey(candidate) | ||
| const score = calculateRecommendationScore( | ||
| candidate, | ||
| currentTrack, | ||
| recentArtists, | ||
| ) | ||
| const existing = candidates.get(candidateKey) | ||
| if (!existing || score > existing.score) { | ||
| candidates.set(candidateKey, { track: candidate, score }) |
There was a problem hiding this comment.
Normalize the candidate map key before ranking.
candidates is keyed by track.id || track.url, so the same song coming back from different URLs/providers survives aggregation as multiple entries and can take multiple slots in one replenish pass. That reintroduces the near-repeat behavior this change is trying to remove.
Suggested fix
- const candidateKey = getTrackKey(candidate)
+ const normalizedCandidateKey = normalizeTrackKey(
+ candidate.title,
+ candidate.author,
+ )
+ const candidateKey =
+ normalizedCandidateKey === '::'
+ ? candidate.url
+ : normalizedCandidateKey
const score = calculateRecommendationScore(
candidate,
currentTrack,
recentArtists,
)-function getTrackKey(track: Track): string {
- return track.id || track.url || normalizeTrackKey(track.title, track.author)
-}Also applies to: 173-175, 229-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bot/src/utils/music/queueManipulation.ts` around lines 160 - 168,
The candidates map must use a normalized key so the same song from different
URLs/providers doesn't appear multiple times: replace usages of
getTrackKey(candidate) when reading/writing the candidates Map (see getTrackKey,
candidates) with a normalized identifier derived from the track (e.g., prefer
track.id if present, otherwise normalize/canonicalize the URL) and use that
normalizedKey for candidates.get and candidates.set; apply the same change to
the other occurrences noted (the blocks around the other getTrackKey calls at
the referenced ranges) so aggregation dedupes by canonical track identity before
comparing scores.
Owner
Author
|
Superseded by split PRs:\n- #130 feat(music): improve autoplay recommendations and shuffle behavior\n- #131 fix(auth): stabilize oauth redirect/session handling and api origin\n- #132 ci(sonar): include frontend coverage and stabilize visual snapshots\n\nClosing this mixed-scope PR to keep one feature/fix per branch and per PR. |
Owner
Author
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resumo
autoplayponta a ponta (bot/backend/shared/frontend)Principais mudanças
queueManipulation: buffer de autoplay (4), deduplicação por URL + título/artista normalizados, ranqueamento anti-repetiçãotrackHandlers: replenish só quando repeat mode está em AUTOPLAYduplicateChecker: grava histórico real viatrackHistoryServiceautoplayValidação local
npm run lintnpm run type:checknpm run buildnpm run test --workspace=packages/bot -- queueManipulation.spec.ts mappers.test.ts --runInBandnpm run test --workspace=packages/frontend -- src/pages/Music.test.tsSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests