Repository navigation
refactor(bot): move queue-management cluster out of utils into services - #1989
Merged
LucasSantana-Dev merged 9 commits intoAug 10, 2026
Merged
Conversation
First bounded slice of #1968: watchdog.ts holds a stateful class (timers, private state Maps, recurring scan loop) mislabeled as a util. Moves it to services/musicManagement/, updates the 13 import sites, and deletes utils/misc/pathUtils.ts (confirmed zero import sites, flagged safe-to-delete in the same audit). The full split #1968 proposes (~20+ files into services/musicRecommendation/ and services/musicManagement/) is out of scope for this PR -- too large for one mechanical move, per the issue's own recommendation to scope it. This is the first unit; rest follows as separate PRs.
Second bounded slice of #1968: collaborativePlaylist.ts holds a stateful class (CollaborativePlaylistService, module-level Map state) mislabeled as a util, with zero internal coupling of its own. Moves it to services/musicRecommendation/, updates 4 real import sites plus 2 jest.mock() string-literal mocks a prior grep-only pass missed. Stacks on refactor/bot-watchdog-to-services (#1981, not yet merged) -- both touch idleDisconnect.ts and leave.ts.
Third bounded slice of #1968. Moves sessionSnapshots.ts to services/musicRecommendation/, and sessionStartupRestore.ts + namedSessions.ts to services/musicManagement/ -- both of the latter depend on sessionSnapshots, now a cross-service import mirroring the same-directory coupling that already existed. Updates 11 import sites plus watchdog.ts/spec (from #1981) which also depends on sessionSnapshots. Found and filed separately while scoping this slice, not fixed here: #1983 (restoreSessionsOnStartup is never called -- dead code) and #1984 (utils/music/index.ts barrel has zero external consumers). Stacks on refactor/bot-collaborative-playlist-to-services (#1982).
Fourth bounded slice of #1968. Moves replenishSuppressionStore.ts, voteSkipStore.ts, and idleDisconnect.ts (all module-level mutable state, zero internal coupling among themselves) to services/musicManagement/. Updates 8 import sites plus a dynamic import() in tests/setup.ts. Deliberately skips service.ts (TrackManagementService) from the original slice plan -- found zero external consumers while scoping this move, filed separately as #1984 rather than relocating dead code. Found and filed separately, not fixed here: #1986 (tests/setup.ts calls a cache-clear function that doesn't exist on replenishSuppressionStore.ts -- pre-existing, silently swallowed by a try/catch, unrelated to this move). Stacks on refactor/bot-session-cluster-to-services (#1985).
Fifth bounded slice of #1968, the largest so far. Moves the entire 27-file autoplay/ directory plus candidateFallback.ts (tightly coupled to autoplay/, bundled so their sibling imports stay intact) to services/musicRecommendation/. Keeps autoplayManager.ts as a thin facade at its original utils/music/ location -- only its one internal import needed fixing, so its 6 external consumers needed no changes at all. Also fixes queueEditOps.ts, queueManipulation.ts, and queueRescue.ts (plus their ~7 spec files), which reach into autoplay/ from utils/music/, and removes a now-redundant utils/music/autoplay entry from queueResolver.guard.spec.ts's architecture-guard target list (already covered by the existing services/musicRecommendation entry via recursion). This slice took five rounds of fix-and-recheck via typecheck/test runs, not just grep: a substring collision (autoplayManager matched by an autoplay-prefix sed), two off-by-one relative-path depth errors, three files missed on the first pass, several spec-only jest.mock() paths, and one bare require() call inside a test body that only surfaced by actually running the suite. Adversarial critic pass (second one on this slice) spot-checked all of the above plus mock-path fidelity on 4 spec files -- ACCEPT, nothing further found. Stacks on refactor/bot-state-stores-to-services (#1987).
Sixth bounded slice of #1968. Moves queueEditOps.ts, queueRescue.ts, queueManipulation.ts (the re-export aggregator for the first two), queueOperations.ts, and queueStateManager.ts to services/musicManagement/. Also bundles in a newly-discovered stateful file, utils/music/queue/asyncQueueManager.ts (single consumer: queueOperations.ts) -- its pure sibling smartShuffle.ts stays behind in utils/music/queue/, splitting that directory since only one of its two files is actually a service. Fixes utils/music/index.ts's barrel re-exports for the two moved files (queueStateManager, queueOperations), and utils/music/service.ts (the #1984 dead-code file, deliberately left in place) whose imports of those two files needed updating since its dependencies moved out from under it. Two rounds of fix via typecheck+test, not just grep: a missed trackNormalization import in queueManipulation.ts, two off-by-one relative-path errors in service.ts, and service.spec.ts's jest.mock() paths (which typecheck can't catch since mock string literals aren't type-checked). Critic pass: ACCEPT, one stale doc comment fixed. Stacks on refactor/bot-autoplay-to-services (#1988).
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (38)
📝 WalkthroughWalkthroughThe PR centralizes music queue management under ChangesMusic management services
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: lockfile failed supply-chain policy check. Run Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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 |
|
Failed to generate code suggestions for PR |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 37 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
This was referenced Aug 10, 2026
…nagement-to-services # Conflicts: # packages/bot/src/functions/music/commands/play/handlers/playHandler.ts # packages/bot/src/handlers/player/lifecycleHandlers.ts # packages/bot/src/services/musicRecommendation/autoplay/replenisher.spec.ts # packages/bot/src/utils/music/queueManipulation.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
restore the nosonar suppression to the new regexp( line it was originally attached to (moving it to its own line breaks sonar's line-scoped suppression); remove the unused queuestatemgr import in service.spec.ts, dead since before this refactor.
prettier's arg-wrap on the multi-line new regexp() call kept forcing the nosonar comment onto its own line on every commit, silently dropping the fix (lint-staged reformats then restages, so a reverted diff never lands). prettier-ignore plus a single-line call keeps the suppression on the line sonar actually attributes the issue to.
|
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This pull request appears to relocate music queue-related modules from a utils/music path to a services/musicManagement path, updating import statements and corresponding jest.mock paths across command files, handlers, and spec files. It also applies formatting changes (multi-line reformatting of function calls, whitespace cleanup) to affected files. The surface area spans autoplay commands, play handlers, queue manipulation/rescue/replenish services, track handlers, and their associated tests.
No blocking issues surfaced. 1 lower-confidence candidate did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 423 functions depend on the 365 functions this change touches.
Health — grade A; 10 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
replenishQueue()— 19 callers, 41 callees (high)calculateRecommendationScore()— 16 callers, 8 callees (high)handleEvents()— 5 callers, 15 callees (high)normalizeTrackKey()— 16 callers, 4 callees (high)collectLastFmCandidates()— 4 callers, 13 callees (high)collectRecommendationCandidates()— 6 callers, 8 callees (high)collectSpotifyRecommendationCandidates()— 3 callers, 15 callees (high)upsertScoredCandidate()— 14 callers, 3 callees (high)- …and 2 more
Verification — 423 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 415 function(s) in the blast radius were not formally verified this run
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
packages/bot/src/services/musicManagement/queue/asyncQueueManager.spec.ts (1)
109-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe test name does not match the assertion.
The test is named "returns empty array for empty input", but
addTracksSafelyreturns a count, not an array. Rename it to describe the count result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queue/asyncQueueManager.spec.ts` around lines 109 - 117, Rename the test case describing empty input in AsyncQueueManager.addTracksSafely to state that it returns a zero count, matching the tracksAdded assertion; leave the test behavior and assertions unchanged.packages/bot/src/services/musicManagement/queueManipulation.operations.spec.ts (1)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe local
anyaliases remove type checking from the mocks.
type GuildQueue = anyandtype Track = anyshadow thediscord-playertypes. Everyas unknown as GuildQueuecast then becomes meaningless, and the many(queue as any)accesses follow from it. Import the real types and cast the fixtures once, so a signature change inrescueQueueormoveTrackInQueuestill fails the typecheck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.operations.spec.ts` around lines 37 - 38, Replace the local any aliases GuildQueue and Track with the actual discord-player types, then type the mock fixtures with those real types and remove unnecessary unknown/any casts such as queue as any. Preserve the existing rescueQueue and moveTrackInQueue test behavior while ensuring their signatures are checked by TypeScript.packages/bot/src/services/musicManagement/queueRescue.ts (1)
71-88: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftProbing runs sequentially, so rescue time scales with queue length.
Each iteration awaits one
player.searchcall with a timeout of up toprobeTimeoutMs. For a 50-track queue with the default 5000 ms timeout, worst-case duration is about 250 seconds. Callers that awaitrescueQueuestay blocked for that period.Probe in bounded parallel batches, or cap the number of probed tracks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueRescue.ts` around lines 71 - 88, Update the probing flow in rescueQueue’s track loop to avoid awaiting probeTrackResolvable sequentially for every track. Process probeTrackResolvable calls in bounded parallel batches with a defined concurrency limit, while preserving isPlayableTrack filtering, removedTracks accounting, keptTracks ordering, and probeTimeoutMs behavior.packages/bot/src/services/musicManagement/queueEditOps.spec.ts (1)
9-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock
recommendationTelemetryas well.
queueEditOps.tsline 7 importsrecordRecommendationOutcome. The tests do not mock that module, soblendAutoplayTracksloads the real telemetry module and its Prisma client during the blending tests. The current tests still pass because the telemetry function swallows its own errors, but the tests then depend on database-client module loading and cannot assert the rejection writes.♻️ Proposed mock
jest.mock('../../services/musicRecommendation/autoplay/replenisher', () => ({ replenishQueue: (...args: unknown[]) => replenishQueueMock(...args), })) + +jest.mock( + '../../services/musicRecommendation/recommendationTelemetry', + () => ({ + recordRecommendationOutcome: (...args: unknown[]) => + recordOutcomeMock(...args), + }), +)Declare
const recordOutcomeMock = jest.fn()next to the other mock functions and setrecordOutcomeMock.mockResolvedValue(undefined)inbeforeEach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueEditOps.spec.ts` around lines 9 - 20, Mock the recommendation telemetry dependency used by queueEditOps.ts, specifically recordRecommendationOutcome, alongside the existing module mocks. Add a recordOutcomeMock with the other test doubles, configure it to resolve successfully in beforeEach, and map the telemetry module export to that mock so blending tests avoid loading the real Prisma-backed implementation.packages/bot/src/services/musicManagement/queueEditOps.ts (1)
19-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
clearQueueandshuffleQueueare declaredasyncwithout anyawait.Both functions run synchronously. The
Promise<boolean>signature is kept for the existing call sites, so this is optional. If callers alreadyawaitthe results, keeping the signature is acceptable. Otherwise, make them synchronous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueEditOps.ts` around lines 19 - 51, Review the async declarations on clearQueue and shuffleQueue in the queue-edit operations module. Since neither function awaits asynchronous work, remove async and change their return types to boolean only if existing callers do not require Promise results; otherwise retain the current Promise<boolean> contract and leave the functions unchanged.packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts (1)
399-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that queries were captured before iterating them.
If
searchMockis never called,capturedQueriesis empty. Theforloop then runs zero times and the test passes without checking any query. Add a length assertion so the test fails when the Spotify search path does not run.💚 Proposed fix
+ expect(capturedQueries.length).toBeGreaterThan(0) for (const { query } of capturedQueries) { expect(query).not.toMatch(/\b(similar|like|playlist|mix)\b/) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts` around lines 399 - 404, Add a non-empty length assertion for capturedQueries before the loop in the relevant queue manipulation test, ensuring the Spotify search path executed; keep the existing per-query modifier assertions unchanged.packages/bot/src/services/musicManagement/queueStateManager.spec.ts (1)
91-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace branch-in-table
it.eachcases with explicit tests.Three tables carry data that the test body ignores or overrides:
- Lines 91-108: for the
'null currentTrack'row, thedurationandexpectedcolumns are unused. The body branches on the label string instead.- Lines 290-305: the
trackscolumn values ([() => undefined],null,[]) are not the fixtures used. The body reconstructs the track list from the shape of the column. The'multi-track'and'single-track'rows also assert the same expected value.- Lines 242-256: the
if (expected.averageDuration !== 0 || tracks.length > 0)guard means the'empty queue'row never assertstotalDuration.Split the label-dependent rows into standalone
itblocks. Then each table row maps directly to one assertion path.♻️ Example for getNextTrack
- it.each([ - ['multi-track queue → first', [() => undefined], 'mockTrack2'], - ['single-track queue → that track', null, 'mockTrack2'], - ['empty queue → null', [], null], - ] as const)('%s', (_label, tracks, expected) => { - const lookup = { mockTrack2 } - if (tracks === null) withTracks([mockTrack2]) - else if (Array.isArray(tracks) && tracks.length === 0) - withTracks([]) - else withTracks([mockTrack2, mockTrack3]) - const next = getNextTrack(mockQueue) - expect(next).toBe( - expected ? lookup[expected as keyof typeof lookup] : null, - ) - }) + it('returns the first track of a multi-track queue', () => { + withTracks([mockTrack2, mockTrack3]) + expect(getNextTrack(mockQueue)).toBe(mockTrack2) + }) + + it('returns the only track of a single-track queue', () => { + withTracks([mockTrack2]) + expect(getNextTrack(mockQueue)).toBe(mockTrack2) + }) + + it('returns null for an empty queue', () => { + withTracks([]) + expect(getNextTrack(mockQueue)).toBeNull() + })Also applies to: 242-256, 290-305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueStateManager.spec.ts` around lines 91 - 108, Replace the branch-dependent parameterized tests in the currentTrack duration cases with explicit it blocks, including a separate null-currentTrack test. Apply the same split to the totalDuration cases around the relevant duration test and the getNextTrack cases: remove fixture values that the body ignores or reconstructs, and ensure each standalone test has one direct assertion path, including totalDuration for an empty queue.packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts (2)
1-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFour spec files carry a near-identical 140-line mock preamble. The blocks mock the same modules, declare the same mock function constants, and define the same
QueueMocktype andcreateQueueMockfactory. The copies already diverge:packages/bot/src/services/musicManagement/queueManipulation.priority.spec.tsdeclareslastFmLinkService.getByDiscordIdas a barejest.fn()with no module-level mock constant, while the other three route it throughgetLastFmLinkMock. Divergence in shared test scaffolding produces suites that exercise different dependency states for the same source module.Extract the
jest.mockcalls, mock constants,QueueMocktype, andcreateQueueMockfactory into one shared test-support module underpackages/bot/src/services/musicManagement/, then import it from each spec. Keep only the per-suitebeforeEachdefaults in the individual files.
packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts#L1-L141: promote this copy to the shared module. It is the most complete version and it documents why the telemetry mock precedes the import.packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts#L1-L169: replace the preamble with an import of the shared module.packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts#L1-L169: replace the preamble with an import of the shared module.packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts#L1-L168: replace the preamble with an import of the shared module, and drop the divergentlastFmLinkServicemock in favor of the sharedgetLastFmLinkMock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts` around lines 1 - 141, Consolidate the duplicated mock preamble into one shared test-support module under packages/bot/src/services/musicManagement/, promoting the complete scaffolding from queueManipulation.replenish.spec.ts, including all jest.mock calls, mock constants, QueueMock, and createQueueMock. Import that module from queueManipulation.autoplay.spec.ts, queueManipulation.dedup.spec.ts, and queueManipulation.priority.spec.ts, leaving only suite-specific beforeEach defaults; in the priority suite, replace its divergent lastFmLinkService mock with the shared getLastFmLinkMock.
599-648: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the candidate count in the comment and add a lower-bound assertion.
Line 600 states five candidates. The mock supplies four (
Y Song 1throughY Song 4).The only assertion is
expect(youtubeCount).toBeLessThanOrEqual(3). That assertion also passes when no track is added, so the test does not prove that the source cap selected tracks. Add a lower bound.💚 Proposed fix
- // 5 candidates all from 'youtube'. With MAX_TRACKS_PER_SOURCE=3 (default), at most 3 selected. + // 4 candidates all from 'youtube'. With MAX_TRACKS_PER_SOURCE=3 (default), at most 3 selected.const youtubeCount = calls.filter( (c) => (c[0] as Track).source === 'youtube', ).length + expect(youtubeCount).toBeGreaterThan(0) expect(youtubeCount).toBeLessThanOrEqual(3)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts` around lines 599 - 648, Update the test description comment in the autoplay-cap test to state that there are four YouTube candidates, matching the mock data, and add a lower-bound assertion ensuring at least one YouTube track was added while retaining the existing maximum-of-three assertion.packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts (1)
722-727: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive assertion so these filter tests cannot pass on an empty result.
Both tests assert only the absence of unwanted tracks. If
replenishQueueadds nothing,allAddedDurations.forEachruns zero times andaddedTitlesis an empty string. Both tests then pass without proving that filtering, rather than a total failure of replenishment, produced the result.The second test already supplies a legitimate candidate (
Regular Song). Assert that it was added.💚 Proposed fix for the ambient/EDM filter test
const addedTitles = addedTracks .map((t: any) => t.title?.toLowerCase?.() || '') .join('|') + expect(addedTitles).toContain('regular song') expect( addedTitles.includes('rain') || addedTitles.includes('dj set') || addedTitles.includes('edm'), ).toBe(false)Also applies to: 776-785
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts` around lines 722 - 727, Add positive assertions to both filter tests around replenishQueue so they verify a valid track was added, not only that rejected tracks are absent. In the second test, assert that addedTitles contains the supplied legitimate candidate “Regular Song”; apply the corresponding positive-result assertion to the ambient/EDM filter test while preserving its existing exclusion checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bot/src/services/musicManagement/queue/asyncQueueManager.ts`:
- Around line 43-50: Update the catch block in the async queue operation to
return the current tracks-added counter instead of hardcoded zero, preserving
the partial-success count for caller reporting while leaving the existing error
message handling unchanged.
- Around line 22-29: Update the playNext branch in the track-processing loop to
insert each track at an incrementing offset, preserving the batch’s original
order while placing it before existing queued tracks. Keep append behavior
unchanged, and update the affected asyncQueueManager test assertions to expect
the preserved order.
In `@packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts`:
- Around line 171-213: Update the title-only deduplication suite around
replenishQueue to configure the feedback mocks with the same defaults
established in the existing beforeEach block near the later tests, avoiding the
resetMocks undefined-value pipeline failure. Extend the test data with a
genuinely non-duplicate candidate and assert it is added alongside the existing
assertion that duplicate candidates are excluded.
In
`@packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts`:
- Around line 587-677: Strengthen replenishment tests so empty results fail: in
packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts
(lines 587-677), remove both addTrack guards, assert queue.addTrack was called,
then inspect recommendationReason; in
packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts
(lines 275-283), assert addedTracks.length is greater than zero before checking
the author; at lines 722-727, assert candidates exist before the duration
iteration; at line 548, replace the tautological nonnegative length assertion
with a positive count assertion; in
packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts
(lines 399-404), assert capturedQueries is non-empty before iteration; and in
packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts
(lines 599-648), require youtubeCount to be greater than zero alongside the
existing upper bound.
In `@packages/bot/src/services/musicManagement/queueManipulation.scoring.spec.ts`:
- Around line 160-177: Update the no-link test around enrichWithAudioFeatures to
mock getValidAccessToken so it resolves a valid Spotify token before invocation,
ensuring execution reaches the empty spotifyIds guard; preserve the existing
unchanged-result assertion.
In `@packages/bot/src/services/musicManagement/queueRescue.ts`:
- Around line 6-13: Validate the parsed values used by
QUEUE_RESCUE_PROBE_TIMEOUT_MS and QUEUE_RESCUE_REFILL_THRESHOLD, falling back to
5000 and 3 respectively whenever parsing produces NaN or an invalid value.
Preserve the existing environment-variable overrides for valid numeric inputs
and ensure the validated constants are used by the probe timeout and
replenishment threshold logic.
---
Nitpick comments:
In `@packages/bot/src/services/musicManagement/queue/asyncQueueManager.spec.ts`:
- Around line 109-117: Rename the test case describing empty input in
AsyncQueueManager.addTracksSafely to state that it returns a zero count,
matching the tracksAdded assertion; leave the test behavior and assertions
unchanged.
In `@packages/bot/src/services/musicManagement/queueEditOps.spec.ts`:
- Around line 9-20: Mock the recommendation telemetry dependency used by
queueEditOps.ts, specifically recordRecommendationOutcome, alongside the
existing module mocks. Add a recordOutcomeMock with the other test doubles,
configure it to resolve successfully in beforeEach, and map the telemetry module
export to that mock so blending tests avoid loading the real Prisma-backed
implementation.
In `@packages/bot/src/services/musicManagement/queueEditOps.ts`:
- Around line 19-51: Review the async declarations on clearQueue and
shuffleQueue in the queue-edit operations module. Since neither function awaits
asynchronous work, remove async and change their return types to boolean only if
existing callers do not require Promise results; otherwise retain the current
Promise<boolean> contract and leave the functions unchanged.
In
`@packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts`:
- Around line 722-727: Add positive assertions to both filter tests around
replenishQueue so they verify a valid track was added, not only that rejected
tracks are absent. In the second test, assert that addedTitles contains the
supplied legitimate candidate “Regular Song”; apply the corresponding
positive-result assertion to the ambient/EDM filter test while preserving its
existing exclusion checks.
In
`@packages/bot/src/services/musicManagement/queueManipulation.operations.spec.ts`:
- Around line 37-38: Replace the local any aliases GuildQueue and Track with the
actual discord-player types, then type the mock fixtures with those real types
and remove unnecessary unknown/any casts such as queue as any. Preserve the
existing rescueQueue and moveTrackInQueue test behavior while ensuring their
signatures are checked by TypeScript.
In
`@packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts`:
- Around line 399-404: Add a non-empty length assertion for capturedQueries
before the loop in the relevant queue manipulation test, ensuring the Spotify
search path executed; keep the existing per-query modifier assertions unchanged.
In
`@packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts`:
- Around line 1-141: Consolidate the duplicated mock preamble into one shared
test-support module under packages/bot/src/services/musicManagement/, promoting
the complete scaffolding from queueManipulation.replenish.spec.ts, including all
jest.mock calls, mock constants, QueueMock, and createQueueMock. Import that
module from queueManipulation.autoplay.spec.ts, queueManipulation.dedup.spec.ts,
and queueManipulation.priority.spec.ts, leaving only suite-specific beforeEach
defaults; in the priority suite, replace its divergent lastFmLinkService mock
with the shared getLastFmLinkMock.
- Around line 599-648: Update the test description comment in the autoplay-cap
test to state that there are four YouTube candidates, matching the mock data,
and add a lower-bound assertion ensuring at least one YouTube track was added
while retaining the existing maximum-of-three assertion.
In `@packages/bot/src/services/musicManagement/queueRescue.ts`:
- Around line 71-88: Update the probing flow in rescueQueue’s track loop to
avoid awaiting probeTrackResolvable sequentially for every track. Process
probeTrackResolvable calls in bounded parallel batches with a defined
concurrency limit, while preserving isPlayableTrack filtering, removedTracks
accounting, keptTracks ordering, and probeTimeoutMs behavior.
In `@packages/bot/src/services/musicManagement/queueStateManager.spec.ts`:
- Around line 91-108: Replace the branch-dependent parameterized tests in the
currentTrack duration cases with explicit it blocks, including a separate
null-currentTrack test. Apply the same split to the totalDuration cases around
the relevant duration test and the getNextTrack cases: remove fixture values
that the body ignores or reconstructs, and ensure each standalone test has one
direct assertion path, including totalDuration for an empty queue.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bbac1c77-b875-44c2-a067-e9fa2caf756c
📒 Files selected for processing (38)
packages/bot/src/functions/music/commands/album.tspackages/bot/src/functions/music/commands/artist.tspackages/bot/src/functions/music/commands/autoplay.spec.tspackages/bot/src/functions/music/commands/autoplay/queueHandlers.spec.tspackages/bot/src/functions/music/commands/autoplay/queueHandlers.tspackages/bot/src/functions/music/commands/play/handlers/playHandler.tspackages/bot/src/functions/music/commands/play/handlers/postPlayBackgroundOps.spec.tspackages/bot/src/functions/music/commands/play/handlers/postPlayBackgroundOps.tspackages/bot/src/functions/music/commands/play/index.spec.tspackages/bot/src/functions/music/commands/queue/index.tspackages/bot/src/handlers/musicButtonHandler.spec.tspackages/bot/src/handlers/musicButtonHandler.tspackages/bot/src/handlers/player/lifecycleHandlers.spec.tspackages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/handlers/player/trackHandlers.spec.tspackages/bot/src/handlers/player/trackHandlers.tspackages/bot/src/services/musicManagement/queue/asyncQueueManager.spec.tspackages/bot/src/services/musicManagement/queue/asyncQueueManager.tspackages/bot/src/services/musicManagement/queueEditOps.spec.tspackages/bot/src/services/musicManagement/queueEditOps.tspackages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.tspackages/bot/src/services/musicManagement/queueManipulation.dedup.spec.tspackages/bot/src/services/musicManagement/queueManipulation.operations.spec.tspackages/bot/src/services/musicManagement/queueManipulation.priority.spec.tspackages/bot/src/services/musicManagement/queueManipulation.replenish.spec.tspackages/bot/src/services/musicManagement/queueManipulation.scoring.spec.tspackages/bot/src/services/musicManagement/queueManipulation.tspackages/bot/src/services/musicManagement/queueOperations.tspackages/bot/src/services/musicManagement/queueRescue.spec.tspackages/bot/src/services/musicManagement/queueRescue.tspackages/bot/src/services/musicManagement/queueStateManager.spec.tspackages/bot/src/services/musicManagement/queueStateManager.tspackages/bot/src/services/musicRecommendation/autoplay/replenisher.spec.tspackages/bot/src/utils/music/index.tspackages/bot/src/utils/music/searchQueryCleaner.tspackages/bot/src/utils/music/service.spec.tspackages/bot/src/utils/music/service.tssonar-project.properties
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🧹 Nitpick comments (10)
packages/bot/src/services/musicManagement/queue/asyncQueueManager.spec.ts (1)
109-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe test name does not match the assertion.
The test is named "returns empty array for empty input", but
addTracksSafelyreturns a count, not an array. Rename it to describe the count result.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queue/asyncQueueManager.spec.ts` around lines 109 - 117, Rename the test case describing empty input in AsyncQueueManager.addTracksSafely to state that it returns a zero count, matching the tracksAdded assertion; leave the test behavior and assertions unchanged.packages/bot/src/services/musicManagement/queueManipulation.operations.spec.ts (1)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe local
anyaliases remove type checking from the mocks.
type GuildQueue = anyandtype Track = anyshadow thediscord-playertypes. Everyas unknown as GuildQueuecast then becomes meaningless, and the many(queue as any)accesses follow from it. Import the real types and cast the fixtures once, so a signature change inrescueQueueormoveTrackInQueuestill fails the typecheck.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.operations.spec.ts` around lines 37 - 38, Replace the local any aliases GuildQueue and Track with the actual discord-player types, then type the mock fixtures with those real types and remove unnecessary unknown/any casts such as queue as any. Preserve the existing rescueQueue and moveTrackInQueue test behavior while ensuring their signatures are checked by TypeScript.packages/bot/src/services/musicManagement/queueRescue.ts (1)
71-88: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftProbing runs sequentially, so rescue time scales with queue length.
Each iteration awaits one
player.searchcall with a timeout of up toprobeTimeoutMs. For a 50-track queue with the default 5000 ms timeout, worst-case duration is about 250 seconds. Callers that awaitrescueQueuestay blocked for that period.Probe in bounded parallel batches, or cap the number of probed tracks.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueRescue.ts` around lines 71 - 88, Update the probing flow in rescueQueue’s track loop to avoid awaiting probeTrackResolvable sequentially for every track. Process probeTrackResolvable calls in bounded parallel batches with a defined concurrency limit, while preserving isPlayableTrack filtering, removedTracks accounting, keptTracks ordering, and probeTimeoutMs behavior.packages/bot/src/services/musicManagement/queueEditOps.spec.ts (1)
9-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock
recommendationTelemetryas well.
queueEditOps.tsline 7 importsrecordRecommendationOutcome. The tests do not mock that module, soblendAutoplayTracksloads the real telemetry module and its Prisma client during the blending tests. The current tests still pass because the telemetry function swallows its own errors, but the tests then depend on database-client module loading and cannot assert the rejection writes.♻️ Proposed mock
jest.mock('../../services/musicRecommendation/autoplay/replenisher', () => ({ replenishQueue: (...args: unknown[]) => replenishQueueMock(...args), })) + +jest.mock( + '../../services/musicRecommendation/recommendationTelemetry', + () => ({ + recordRecommendationOutcome: (...args: unknown[]) => + recordOutcomeMock(...args), + }), +)Declare
const recordOutcomeMock = jest.fn()next to the other mock functions and setrecordOutcomeMock.mockResolvedValue(undefined)inbeforeEach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueEditOps.spec.ts` around lines 9 - 20, Mock the recommendation telemetry dependency used by queueEditOps.ts, specifically recordRecommendationOutcome, alongside the existing module mocks. Add a recordOutcomeMock with the other test doubles, configure it to resolve successfully in beforeEach, and map the telemetry module export to that mock so blending tests avoid loading the real Prisma-backed implementation.packages/bot/src/services/musicManagement/queueEditOps.ts (1)
19-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
clearQueueandshuffleQueueare declaredasyncwithout anyawait.Both functions run synchronously. The
Promise<boolean>signature is kept for the existing call sites, so this is optional. If callers alreadyawaitthe results, keeping the signature is acceptable. Otherwise, make them synchronous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueEditOps.ts` around lines 19 - 51, Review the async declarations on clearQueue and shuffleQueue in the queue-edit operations module. Since neither function awaits asynchronous work, remove async and change their return types to boolean only if existing callers do not require Promise results; otherwise retain the current Promise<boolean> contract and leave the functions unchanged.packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts (1)
399-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that queries were captured before iterating them.
If
searchMockis never called,capturedQueriesis empty. Theforloop then runs zero times and the test passes without checking any query. Add a length assertion so the test fails when the Spotify search path does not run.💚 Proposed fix
+ expect(capturedQueries.length).toBeGreaterThan(0) for (const { query } of capturedQueries) { expect(query).not.toMatch(/\b(similar|like|playlist|mix)\b/) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts` around lines 399 - 404, Add a non-empty length assertion for capturedQueries before the loop in the relevant queue manipulation test, ensuring the Spotify search path executed; keep the existing per-query modifier assertions unchanged.packages/bot/src/services/musicManagement/queueStateManager.spec.ts (1)
91-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace branch-in-table
it.eachcases with explicit tests.Three tables carry data that the test body ignores or overrides:
- Lines 91-108: for the
'null currentTrack'row, thedurationandexpectedcolumns are unused. The body branches on the label string instead.- Lines 290-305: the
trackscolumn values ([() => undefined],null,[]) are not the fixtures used. The body reconstructs the track list from the shape of the column. The'multi-track'and'single-track'rows also assert the same expected value.- Lines 242-256: the
if (expected.averageDuration !== 0 || tracks.length > 0)guard means the'empty queue'row never assertstotalDuration.Split the label-dependent rows into standalone
itblocks. Then each table row maps directly to one assertion path.♻️ Example for getNextTrack
- it.each([ - ['multi-track queue → first', [() => undefined], 'mockTrack2'], - ['single-track queue → that track', null, 'mockTrack2'], - ['empty queue → null', [], null], - ] as const)('%s', (_label, tracks, expected) => { - const lookup = { mockTrack2 } - if (tracks === null) withTracks([mockTrack2]) - else if (Array.isArray(tracks) && tracks.length === 0) - withTracks([]) - else withTracks([mockTrack2, mockTrack3]) - const next = getNextTrack(mockQueue) - expect(next).toBe( - expected ? lookup[expected as keyof typeof lookup] : null, - ) - }) + it('returns the first track of a multi-track queue', () => { + withTracks([mockTrack2, mockTrack3]) + expect(getNextTrack(mockQueue)).toBe(mockTrack2) + }) + + it('returns the only track of a single-track queue', () => { + withTracks([mockTrack2]) + expect(getNextTrack(mockQueue)).toBe(mockTrack2) + }) + + it('returns null for an empty queue', () => { + withTracks([]) + expect(getNextTrack(mockQueue)).toBeNull() + })Also applies to: 242-256, 290-305
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueStateManager.spec.ts` around lines 91 - 108, Replace the branch-dependent parameterized tests in the currentTrack duration cases with explicit it blocks, including a separate null-currentTrack test. Apply the same split to the totalDuration cases around the relevant duration test and the getNextTrack cases: remove fixture values that the body ignores or reconstructs, and ensure each standalone test has one direct assertion path, including totalDuration for an empty queue.packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts (2)
1-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftFour spec files carry a near-identical 140-line mock preamble. The blocks mock the same modules, declare the same mock function constants, and define the same
QueueMocktype andcreateQueueMockfactory. The copies already diverge:packages/bot/src/services/musicManagement/queueManipulation.priority.spec.tsdeclareslastFmLinkService.getByDiscordIdas a barejest.fn()with no module-level mock constant, while the other three route it throughgetLastFmLinkMock. Divergence in shared test scaffolding produces suites that exercise different dependency states for the same source module.Extract the
jest.mockcalls, mock constants,QueueMocktype, andcreateQueueMockfactory into one shared test-support module underpackages/bot/src/services/musicManagement/, then import it from each spec. Keep only the per-suitebeforeEachdefaults in the individual files.
packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts#L1-L141: promote this copy to the shared module. It is the most complete version and it documents why the telemetry mock precedes the import.packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts#L1-L169: replace the preamble with an import of the shared module.packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts#L1-L169: replace the preamble with an import of the shared module.packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts#L1-L168: replace the preamble with an import of the shared module, and drop the divergentlastFmLinkServicemock in favor of the sharedgetLastFmLinkMock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts` around lines 1 - 141, Consolidate the duplicated mock preamble into one shared test-support module under packages/bot/src/services/musicManagement/, promoting the complete scaffolding from queueManipulation.replenish.spec.ts, including all jest.mock calls, mock constants, QueueMock, and createQueueMock. Import that module from queueManipulation.autoplay.spec.ts, queueManipulation.dedup.spec.ts, and queueManipulation.priority.spec.ts, leaving only suite-specific beforeEach defaults; in the priority suite, replace its divergent lastFmLinkService mock with the shared getLastFmLinkMock.
599-648: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the candidate count in the comment and add a lower-bound assertion.
Line 600 states five candidates. The mock supplies four (
Y Song 1throughY Song 4).The only assertion is
expect(youtubeCount).toBeLessThanOrEqual(3). That assertion also passes when no track is added, so the test does not prove that the source cap selected tracks. Add a lower bound.💚 Proposed fix
- // 5 candidates all from 'youtube'. With MAX_TRACKS_PER_SOURCE=3 (default), at most 3 selected. + // 4 candidates all from 'youtube'. With MAX_TRACKS_PER_SOURCE=3 (default), at most 3 selected.const youtubeCount = calls.filter( (c) => (c[0] as Track).source === 'youtube', ).length + expect(youtubeCount).toBeGreaterThan(0) expect(youtubeCount).toBeLessThanOrEqual(3)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts` around lines 599 - 648, Update the test description comment in the autoplay-cap test to state that there are four YouTube candidates, matching the mock data, and add a lower-bound assertion ensuring at least one YouTube track was added while retaining the existing maximum-of-three assertion.packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts (1)
722-727: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive assertion so these filter tests cannot pass on an empty result.
Both tests assert only the absence of unwanted tracks. If
replenishQueueadds nothing,allAddedDurations.forEachruns zero times andaddedTitlesis an empty string. Both tests then pass without proving that filtering, rather than a total failure of replenishment, produced the result.The second test already supplies a legitimate candidate (
Regular Song). Assert that it was added.💚 Proposed fix for the ambient/EDM filter test
const addedTitles = addedTracks .map((t: any) => t.title?.toLowerCase?.() || '') .join('|') + expect(addedTitles).toContain('regular song') expect( addedTitles.includes('rain') || addedTitles.includes('dj set') || addedTitles.includes('edm'), ).toBe(false)Also applies to: 776-785
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts` around lines 722 - 727, Add positive assertions to both filter tests around replenishQueue so they verify a valid track was added, not only that rejected tracks are absent. In the second test, assert that addedTitles contains the supplied legitimate candidate “Regular Song”; apply the corresponding positive-result assertion to the ambient/EDM filter test while preserving its existing exclusion checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bot/src/services/musicManagement/queue/asyncQueueManager.ts`:
- Around line 43-50: Update the catch block in the async queue operation to
return the current tracks-added counter instead of hardcoded zero, preserving
the partial-success count for caller reporting while leaving the existing error
message handling unchanged.
- Around line 22-29: Update the playNext branch in the track-processing loop to
insert each track at an incrementing offset, preserving the batch’s original
order while placing it before existing queued tracks. Keep append behavior
unchanged, and update the affected asyncQueueManager test assertions to expect
the preserved order.
In `@packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts`:
- Around line 171-213: Update the title-only deduplication suite around
replenishQueue to configure the feedback mocks with the same defaults
established in the existing beforeEach block near the later tests, avoiding the
resetMocks undefined-value pipeline failure. Extend the test data with a
genuinely non-duplicate candidate and assert it is added alongside the existing
assertion that duplicate candidates are excluded.
In
`@packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts`:
- Around line 587-677: Strengthen replenishment tests so empty results fail: in
packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts
(lines 587-677), remove both addTrack guards, assert queue.addTrack was called,
then inspect recommendationReason; in
packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts
(lines 275-283), assert addedTracks.length is greater than zero before checking
the author; at lines 722-727, assert candidates exist before the duration
iteration; at line 548, replace the tautological nonnegative length assertion
with a positive count assertion; in
packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts
(lines 399-404), assert capturedQueries is non-empty before iteration; and in
packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts
(lines 599-648), require youtubeCount to be greater than zero alongside the
existing upper bound.
In `@packages/bot/src/services/musicManagement/queueManipulation.scoring.spec.ts`:
- Around line 160-177: Update the no-link test around enrichWithAudioFeatures to
mock getValidAccessToken so it resolves a valid Spotify token before invocation,
ensuring execution reaches the empty spotifyIds guard; preserve the existing
unchanged-result assertion.
In `@packages/bot/src/services/musicManagement/queueRescue.ts`:
- Around line 6-13: Validate the parsed values used by
QUEUE_RESCUE_PROBE_TIMEOUT_MS and QUEUE_RESCUE_REFILL_THRESHOLD, falling back to
5000 and 3 respectively whenever parsing produces NaN or an invalid value.
Preserve the existing environment-variable overrides for valid numeric inputs
and ensure the validated constants are used by the probe timeout and
replenishment threshold logic.
---
Nitpick comments:
In `@packages/bot/src/services/musicManagement/queue/asyncQueueManager.spec.ts`:
- Around line 109-117: Rename the test case describing empty input in
AsyncQueueManager.addTracksSafely to state that it returns a zero count,
matching the tracksAdded assertion; leave the test behavior and assertions
unchanged.
In `@packages/bot/src/services/musicManagement/queueEditOps.spec.ts`:
- Around line 9-20: Mock the recommendation telemetry dependency used by
queueEditOps.ts, specifically recordRecommendationOutcome, alongside the
existing module mocks. Add a recordOutcomeMock with the other test doubles,
configure it to resolve successfully in beforeEach, and map the telemetry module
export to that mock so blending tests avoid loading the real Prisma-backed
implementation.
In `@packages/bot/src/services/musicManagement/queueEditOps.ts`:
- Around line 19-51: Review the async declarations on clearQueue and
shuffleQueue in the queue-edit operations module. Since neither function awaits
asynchronous work, remove async and change their return types to boolean only if
existing callers do not require Promise results; otherwise retain the current
Promise<boolean> contract and leave the functions unchanged.
In
`@packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts`:
- Around line 722-727: Add positive assertions to both filter tests around
replenishQueue so they verify a valid track was added, not only that rejected
tracks are absent. In the second test, assert that addedTitles contains the
supplied legitimate candidate “Regular Song”; apply the corresponding
positive-result assertion to the ambient/EDM filter test while preserving its
existing exclusion checks.
In
`@packages/bot/src/services/musicManagement/queueManipulation.operations.spec.ts`:
- Around line 37-38: Replace the local any aliases GuildQueue and Track with the
actual discord-player types, then type the mock fixtures with those real types
and remove unnecessary unknown/any casts such as queue as any. Preserve the
existing rescueQueue and moveTrackInQueue test behavior while ensuring their
signatures are checked by TypeScript.
In
`@packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts`:
- Around line 399-404: Add a non-empty length assertion for capturedQueries
before the loop in the relevant queue manipulation test, ensuring the Spotify
search path executed; keep the existing per-query modifier assertions unchanged.
In
`@packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts`:
- Around line 1-141: Consolidate the duplicated mock preamble into one shared
test-support module under packages/bot/src/services/musicManagement/, promoting
the complete scaffolding from queueManipulation.replenish.spec.ts, including all
jest.mock calls, mock constants, QueueMock, and createQueueMock. Import that
module from queueManipulation.autoplay.spec.ts, queueManipulation.dedup.spec.ts,
and queueManipulation.priority.spec.ts, leaving only suite-specific beforeEach
defaults; in the priority suite, replace its divergent lastFmLinkService mock
with the shared getLastFmLinkMock.
- Around line 599-648: Update the test description comment in the autoplay-cap
test to state that there are four YouTube candidates, matching the mock data,
and add a lower-bound assertion ensuring at least one YouTube track was added
while retaining the existing maximum-of-three assertion.
In `@packages/bot/src/services/musicManagement/queueRescue.ts`:
- Around line 71-88: Update the probing flow in rescueQueue’s track loop to
avoid awaiting probeTrackResolvable sequentially for every track. Process
probeTrackResolvable calls in bounded parallel batches with a defined
concurrency limit, while preserving isPlayableTrack filtering, removedTracks
accounting, keptTracks ordering, and probeTimeoutMs behavior.
In `@packages/bot/src/services/musicManagement/queueStateManager.spec.ts`:
- Around line 91-108: Replace the branch-dependent parameterized tests in the
currentTrack duration cases with explicit it blocks, including a separate
null-currentTrack test. Apply the same split to the totalDuration cases around
the relevant duration test and the getNextTrack cases: remove fixture values
that the body ignores or reconstructs, and ensure each standalone test has one
direct assertion path, including totalDuration for an empty queue.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bbac1c77-b875-44c2-a067-e9fa2caf756c
📒 Files selected for processing (38)
packages/bot/src/functions/music/commands/album.tspackages/bot/src/functions/music/commands/artist.tspackages/bot/src/functions/music/commands/autoplay.spec.tspackages/bot/src/functions/music/commands/autoplay/queueHandlers.spec.tspackages/bot/src/functions/music/commands/autoplay/queueHandlers.tspackages/bot/src/functions/music/commands/play/handlers/playHandler.tspackages/bot/src/functions/music/commands/play/handlers/postPlayBackgroundOps.spec.tspackages/bot/src/functions/music/commands/play/handlers/postPlayBackgroundOps.tspackages/bot/src/functions/music/commands/play/index.spec.tspackages/bot/src/functions/music/commands/queue/index.tspackages/bot/src/handlers/musicButtonHandler.spec.tspackages/bot/src/handlers/musicButtonHandler.tspackages/bot/src/handlers/player/lifecycleHandlers.spec.tspackages/bot/src/handlers/player/lifecycleHandlers.tspackages/bot/src/handlers/player/trackHandlers.spec.tspackages/bot/src/handlers/player/trackHandlers.tspackages/bot/src/services/musicManagement/queue/asyncQueueManager.spec.tspackages/bot/src/services/musicManagement/queue/asyncQueueManager.tspackages/bot/src/services/musicManagement/queueEditOps.spec.tspackages/bot/src/services/musicManagement/queueEditOps.tspackages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.tspackages/bot/src/services/musicManagement/queueManipulation.dedup.spec.tspackages/bot/src/services/musicManagement/queueManipulation.operations.spec.tspackages/bot/src/services/musicManagement/queueManipulation.priority.spec.tspackages/bot/src/services/musicManagement/queueManipulation.replenish.spec.tspackages/bot/src/services/musicManagement/queueManipulation.scoring.spec.tspackages/bot/src/services/musicManagement/queueManipulation.tspackages/bot/src/services/musicManagement/queueOperations.tspackages/bot/src/services/musicManagement/queueRescue.spec.tspackages/bot/src/services/musicManagement/queueRescue.tspackages/bot/src/services/musicManagement/queueStateManager.spec.tspackages/bot/src/services/musicManagement/queueStateManager.tspackages/bot/src/services/musicRecommendation/autoplay/replenisher.spec.tspackages/bot/src/utils/music/index.tspackages/bot/src/utils/music/searchQueryCleaner.tspackages/bot/src/utils/music/service.spec.tspackages/bot/src/utils/music/service.tssonar-project.properties
🛑 Comments failed to post (6)
packages/bot/src/services/musicManagement/queue/asyncQueueManager.ts (2)
22-29: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
playNextreverses the order of a multi-track batch.Each track is inserted at index 0. For a batch of tracks, the last track ends up first. A "play next" request for an album or playlist therefore plays in reverse order. Insert at an incrementing offset instead.
🐛 Proposed fix
// Add tracks to queue directly without complex task queue management - for (const track of tracks) { + for (const [offset, track] of tracks.entries()) { if (playNext) { - queue.insertTrack(track, 0) // Insert at beginning + queue.insertTrack(track, offset) // Preserve batch order at the front } else { queue.addTrack(track) // Add to end } tracksAdded++ }Note:
packages/bot/src/services/musicManagement/queue/asyncQueueManager.spec.tslines 86-87 assert the current reversing behavior, so update that test with the fix.📝 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.for (const [offset, track] of tracks.entries()) { if (playNext) { queue.insertTrack(track, offset) // Preserve batch order at the front } else { queue.addTrack(track) // Add to end } tracksAdded++ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queue/asyncQueueManager.ts` around lines 22 - 29, Update the playNext branch in the track-processing loop to insert each track at an incrementing offset, preserving the batch’s original order while placing it before existing queued tracks. Keep append behavior unchanged, and update the affected asyncQueueManager test assertions to expect the preserved order.
43-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The error path reports
tracksAdded: 0after partial success.
queue.insertTrack,queue.addTrack, orqueue.node.play()can throw after some tracks are already in the queue. The result then claims zero tracks were added while the queue holds them. The caller inpackages/bot/src/services/musicManagement/queueOperations.tslines 69-73 uses this count for user-facing reporting.Return the real counter value in the catch block.
🐛 Proposed fix
): Promise<{ success: boolean; tracksAdded: number; error?: string }> { + let tracksAdded = 0 try { - let tracksAdded = 0 let success = false @@ } catch (error) { errorLog({ message: 'Error in async queue operation:', error }) return { success: false, - tracksAdded: 0, + tracksAdded, error: error instanceof Error ? error.message : 'Unknown 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.} catch (error) { errorLog({ message: 'Error in async queue operation:', error }) return { success: false, tracksAdded, error: error instanceof Error ? error.message : 'Unknown error', } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queue/asyncQueueManager.ts` around lines 43 - 50, Update the catch block in the async queue operation to return the current tracks-added counter instead of hardcoded zero, preserving the partial-success count for caller reporting while leaving the existing error message handling unchanged.packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts (1)
171-213: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Confirm resetMocks is enabled and inspect replenishQueue error handling around feedback weights. set -euo pipefail echo '=== jest config: resetMocks / restoreMocks ===' fd -e ts -e js -e json -e mjs -e cjs 'jest.config' --exec rg -n 'resetMocks|restoreMocks|clearMocks' {} \; fd 'package.json' -d 3 --exec rg -n 'resetMocks|restoreMocks|clearMocks' {} \; echo '=== replenishQueue implementation ===' fd 'replenisher.ts' --exec ast-grep outline {} --items all \; echo '=== feedback weight consumption sites ===' rg -nP --type=ts -C4 'getDislikedTrackWeights|getLikedTrackWeights|getGuildImplicitDislikeKeys' -g '!**/*.spec.ts'Repository: LucasSantana-Dev/Lucky
Length of output: 423
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '=== Jest configuration matches ===' rg -n -C4 'resetMocks|clearMocks|restoreMocks' --glob 'jest.config.*' --glob 'package.json' --glob '**/package.json' . echo '=== Candidate queue-management files ===' fd -i 'queueManipulation|replenisher' --type f echo '=== Test setup and target cases ===' target=$(fd -i 'queueManipulation.dedup.spec.ts' --type f | head -n1) printf '%s\n' "$target" cat -n "$target" | sed -n '1,430p' echo '=== replenishQueue definitions and callers ===' rg -n -C8 'function replenishQueue|const replenishQueue|replenishQueue\s*=|replenishQueue\(' packages/bot/src echo '=== Feedback mock declarations and setup ===' rg -n -C8 'dislikedTrackWeightsMock|likedTrackWeightsMock|getPreferredArtistKeysMock|getGuildSettingsMock|getGuildImplicitDislikeKeys' packages/bot/src/services/musicManagementRepository: LucasSantana-Dev/Lucky
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '=== Target test file ===' cat -n packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts | sed -n '1,430p' echo '=== Autoplay setup referenced by the comment ===' cat -n packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts | sed -n '340,435p' echo '=== Jest reset configuration, with file names ===' rg -l 'resetMocks\s*:\s*true' --glob 'jest.config.*' --glob 'package.json' --glob '**/package.json' . | while IFS= read -r f; do echo "--- $f" rg -n -C5 'resetMocks|clearMocks|restoreMocks' "$f" done echo '=== Relevant implementation files ===' fd -i 'replenisher' packages/bot/src --type f rg -l 'getDislikedTrackWeights|getLikedTrackWeights|getGuildImplicitDislikeKeys' packages/bot/src --glob '!**/*.spec.ts' | while IFS= read -r f; do echo "--- $f" rg -n -C8 'getDislikedTrackWeights|getLikedTrackWeights|getGuildImplicitDislikeKeys' "$f" doneRepository: LucasSantana-Dev/Lucky
Length of output: 26744
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '=== Queue replenishment implementation ===' cat -n packages/bot/src/services/musicRecommendation/autoplay/replenisher.ts | sed -n '200,380p' echo '=== Deduplication and error-handling logic ===' rg -n -C12 'dedup|duplicate|normalize|catch|score|weights|candidate' packages/bot/src/services/musicRecommendation/autoplay packages/bot/src/services/musicManagement/queueManipulation.ts echo '=== Remaining dedup test region ===' cat -n packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts | sed -n '215,390p' echo '=== Static verifier for mock defaults and assertions ===' python3 - <<'PY' from pathlib import Path import re test = Path("packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts").read_text() autoplay = Path("packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts").read_text() replenisher = Path("packages/bot/src/services/musicRecommendation/autoplay/replenisher.ts").read_text() required = [ "dislikedTrackWeightsMock", "likedTrackWeightsMock", "getPreferredArtistKeysMock", "getBlockedArtistKeysMock", "getImplicitDislikeKeysMock", "getImplicitLikeKeysMock", "getGuildImplicitDislikeKeysMock", "getTrackHistoryMock", "getGuildSettingsMock", ] print("dedup beforeEach:", bool(re.search(r"describe\\([^\\n]*dedup[\\s\\S]*?beforeEach", test))) for name in required: print(f"{name}: declared={name in test}, defaulted_in_dedup={bool(re.search(r'describe\\([^\\n]*dedup[\\s\\S]*?'+re.escape(name)+r'\\.mock(?:ResolvedValue|ReturnValue)', test))}") print("autoplay defaults present:", all(name in autoplay for name in required)) print("replenisher catches errors:", bool(re.search(r"catch\\s*\\([^)]*\\)", replenisher))) print("replenisher consumes weight maps:", "getLikedTrackWeights" in replenisher and "getDislikedTrackWeights" in replenisher) PYRepository: LucasSantana-Dev/Lucky
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '=== Replenisher scoring context ===' cat -n packages/bot/src/services/musicRecommendation/autoplay/replenisher.ts | sed -n '220,335p' echo '=== Candidate collection call and error handling ===' rg -n -C15 'collectRecommendationCandidates|calculateRecommendationScore|catch' packages/bot/src/services/musicRecommendation/autoplay/replenisher.ts packages/bot/src/services/musicRecommendation/autoplay/candidateCollector.ts echo '=== Queue-management replenish wrapper ===' rg -n -C20 'export.*replenishQueue|function replenishQueue|const replenishQueue|replenishQueue' packages/bot/src/services/musicManagement/queueManipulation.ts echo '=== Dedup implementation ===' rg -n -C15 'isDuplicateCandidate|normalizeTrackKey|title-only|titleOnly' packages/bot/src/services/musicRecommendation/autoplay packages/bot/src/utils packages/bot/src/services/musicManagement echo '=== Static verifier ===' python3 - <<'PY' from pathlib import Path import re test_path = Path("packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts") test = test_path.read_text() autoplay = Path("packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts").read_text() replenisher = Path("packages/bot/src/services/musicRecommendation/autoplay/replenisher.ts").read_text() start = test.index("describe('queueManipulation — title-only deduplication'") end = test.index("describe('queueManipulation.replenishQueue query variation'") dedup = test[start:end] names = [ "dislikedTrackWeightsMock", "likedTrackWeightsMock", "getPreferredArtistKeysMock", "getBlockedArtistKeysMock", "getImplicitDislikeKeysMock", "getImplicitLikeKeysMock", "getGuildImplicitDislikeKeysMock", "getTrackHistoryMock", "getGuildSettingsMock", ] print("dedup block has beforeEach:", "beforeEach" in dedup) for name in names: print(f"{name}: default in dedup block =", bool(re.search( rf"{re.escape(name)}\\.mock(?:ResolvedValue|ReturnValue)\\s*\\(", dedup ))) print("defaults exist in autoplay setup:", all( re.search(rf"{re.escape(name)}\\.mock(?:ResolvedValue|ReturnValue)\\s*\\(", autoplay) for name in names )) print("replenisher has catch:", bool(re.search(r"\\.catch\\s*\\(", replenisher))) print("replenisher calls feedback weights:", "getLikedTrackWeights" in replenisher and "getDislikedTrackWeights" in replenisher) PYRepository: LucasSantana-Dev/Lucky
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' from pathlib import Path dedup_path = Path("packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts") replenisher_path = Path("packages/bot/src/services/musicRecommendation/autoplay/replenisher.ts") autoplay_path = Path("packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts") dedup = dedup_path.read_text() replenisher = replenisher_path.read_text() autoplay = autoplay_path.read_text() dedup_start = dedup.index("describe('queueManipulation — title-only deduplication'") dedup_end = dedup.index("describe('queueManipulation.replenishQueue query variation'") dedup_region = dedup[dedup_start:dedup_end] required_defaults = [ "dislikedTrackWeightsMock.mockResolvedValue(new Map())", "likedTrackWeightsMock.mockResolvedValue(new Map())", "getPreferredArtistKeysMock.mockResolvedValue(new Set())", "getBlockedArtistKeysMock.mockResolvedValue(new Set())", "getImplicitDislikeKeysMock.mockResolvedValue(new Set())", "getImplicitLikeKeysMock.mockResolvedValue(new Set())", "getGuildImplicitDislikeKeysMock.mockReturnValue(new Set())", "getTrackHistoryMock.mockResolvedValue([])", "getGuildSettingsMock.mockResolvedValue({ autoplayMode: 'similar' })", ] print("dedup region has beforeEach:", "beforeEach" in dedup_region) print("missing defaults in dedup region:") for default in required_defaults: if default not in dedup_region: print(" ", default) print("autoplay setup has all referenced defaults:", all(default in autoplay for default in required_defaults)) checks = { "feedback maps are consumed with .get": "ctx.dislikedWeights.get(" in Path( "packages/bot/src/services/musicRecommendation/autoplay/candidateCollector.ts" ).read_text(), "undefined feedback values reach Set spread": "...implicitDislikeKeys" in replenisher and "...guildImplicitDislikeKeys" in replenisher, "replenisher swallows pass errors": "catch (error)" in replenisher and "Error replenishing queue:" in replenisher, } for name, result in checks.items(): print(f"{name}: {result}") PY node - <<'JS' function brokenPass() { try { const weights = undefined weights.get('candidate') } catch { return 0 } return 1 } function brokenSetMerge() { try { const keys = undefined return new Set([...keys]) } catch { return 0 } } console.log("undefined weight access is swallowed as zero additions:", brokenPass() === 0) console.log("undefined key-set merge is swallowed as zero additions:", brokenSetMerge() === 0) JSRepository: LucasSantana-Dev/Lucky
Length of output: 1051
Add feedback defaults and a positive-control assertion
Because
resetMocks: trueis enabled, these feedback mocks returnundefined.replenishQueuecatches the resulting pipeline error, soaddedTracks.length === 0does not prove deduplication. Add thebeforeEachdefaults used at Lines 405-419, and assert that a non-duplicate candidate is added in the same run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.dedup.spec.ts` around lines 171 - 213, Update the title-only deduplication suite around replenishQueue to configure the feedback mocks with the same defaults established in the existing beforeEach block near the later tests, avoiding the resetMocks undefined-value pipeline failure. Extend the test data with a genuinely non-duplicate candidate and assert it is added alongside the existing assertion that duplicate candidates are excluded.packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts (1)
587-677: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Several new tests cannot fail when
replenishQueueadds no track. The shared root cause is assertion shape: guardedifblocks, iteration over a possibly empty collection, one-sided bounds, and one tautology. Each of these tests reports success when the replenishment pipeline produces nothing, which is the exact regression they should catch. Add a positive assertion that a track reached the queue before asserting the specific behavior.
packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts#L587-L677: remove theif (queue.addTrack.mock.calls.length > 0)guards at Lines 623 and 669. Assertexpect(queue.addTrack).toHaveBeenCalled()first, then assert therecommendationReasonannotation.packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts#L275-L283: remove theif (addedTracks.length > 0)guard and assertexpect(addedTracks.length).toBeGreaterThan(0)before the author check.packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts#L722-L727: assert that at least one candidate was added before theforEachduration check, so an empty result fails the test.packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts#L399-L404: assertexpect(capturedQueries.length).toBeGreaterThan(0)before iteratingcapturedQueries.packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts#L599-L648: addexpect(youtubeCount).toBeGreaterThan(0)alongside the existingtoBeLessThanOrEqual(3)bound.Line 548 of
packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.tsis the strongest case:expect(addedTracks.length).toBeGreaterThanOrEqual(0)can never fail for any array.📍 Affects 3 files
packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts#L587-L677(this comment)packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts#L275-L283packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts#L722-L727packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts#L399-L404packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts#L599-L648🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts` around lines 587 - 677, Strengthen replenishment tests so empty results fail: in packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts (lines 587-677), remove both addTrack guards, assert queue.addTrack was called, then inspect recommendationReason; in packages/bot/src/services/musicManagement/queueManipulation.autoplay.spec.ts (lines 275-283), assert addedTracks.length is greater than zero before checking the author; at lines 722-727, assert candidates exist before the duration iteration; at line 548, replace the tautological nonnegative length assertion with a positive count assertion; in packages/bot/src/services/musicManagement/queueManipulation.priority.spec.ts (lines 399-404), assert capturedQueries is non-empty before iteration; and in packages/bot/src/services/musicManagement/queueManipulation.replenish.spec.ts (lines 599-648), require youtubeCount to be greater than zero alongside the existing upper bound.packages/bot/src/services/musicManagement/queueManipulation.scoring.spec.ts (1)
160-177: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Confirm resetMocks setting and the guard order in enrichWithAudioFeatures. set -euo pipefail fd -e ts -e js -e json -e mjs -e cjs 'jest.config' --exec rg -n 'resetMocks|restoreMocks|clearMocks' {} \; fd 'candidateFallback.ts' --exec ast-grep run --pattern 'export async function enrichWithAudioFeatures($$$) { $$$ }' --lang typescript {} \;Repository: LucasSantana-Dev/Lucky
Length of output: 376
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- candidate files ---' fd -i 'queueManipulation.scoring.spec.ts|candidateFallback.ts|jest.config' . printf '%s\n' '--- target test and implementation references ---' rg -n -C 8 'returns unchanged when no Spotify links|enrichWithAudioFeatures|getValidAccessToken|spotifyIds' packages/bot/src packages/shared 2>/dev/null || true printf '%s\n' '--- Jest configuration ---' fd -e ts -e js -e json -e mjs -e cjs -i 'jest.config' --exec rg -n -C 4 'clearMocks|resetMocks|restoreMocks' {} \;Repository: LucasSantana-Dev/Lucky
Length of output: 50379
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- scoring test setup ---' sed -n '1,45p' packages/bot/src/services/musicManagement/queueManipulation.scoring.spec.ts sed -n '124,180p' packages/bot/src/services/musicManagement/queueManipulation.scoring.spec.ts printf '%s\n' '--- implementation locations ---' rg -n -C 20 'function enrichWithAudioFeatures|const enrichWithAudioFeatures|enrichWithAudioFeatures' packages/bot/src/services/musicManagement/queueManipulation.ts packages/bot/src/services/musicRecommendation/candidateFallback.ts printf '%s\n' '--- Jest configurations ---' for f in jest.config.cjs packages/bot/jest.config.cjs; do printf '%s\n' "--- $f ---" cat -n "$f" doneRepository: LucasSantana-Dev/Lucky
Length of output: 13320
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- enrichWithAudioFeatures body ---' sed -n '227,285p' packages/bot/src/services/musicRecommendation/candidateFallback.ts printf '%s\n' '--- bot test command and Jest config selection ---' cat -n packages/bot/package.json printf '%s\n' '--- root package scripts ---' cat -n package.json | sed -n '1,180p' printf '%s\n' '--- config references to bot scoring tests ---' rg -n -C 3 'packages/bot|jest.*config|queueManipulation.scoring' package.json packages/*/package.json README.md 2>/dev/null || trueRepository: LucasSantana-Dev/Lucky
Length of output: 16321
Set a Spotify token before testing the no-link guard
enrichWithAudioFeaturesreturns at the token guard becausegetValidAccessTokenresolvesnull. Configure it to resolve a token so the test reaches the emptyspotifyIdsguard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueManipulation.scoring.spec.ts` around lines 160 - 177, Update the no-link test around enrichWithAudioFeatures to mock getValidAccessToken so it resolves a valid Spotify token before invocation, ensuring execution reaches the empty spotifyIds guard; preserve the existing unchanged-result assertion.packages/bot/src/services/musicManagement/queueRescue.ts (1)
6-13: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the parsed environment values against
NaN.
Number.parseIntreturnsNaNfor a non-numeric or empty-string environment value. Two failure modes follow:
setTimeout(..., NaN)fires immediately, so every probe resolves tonullandrescueQueueremoves every track whenprobeResolvableis true.queue.tracks.size < NaNis always false, so replenishment never runs.Validate both values and fall back to the defaults.
🛡️ Proposed fix
-const QUEUE_RESCUE_PROBE_TIMEOUT_MS = Number.parseInt( - process.env.QUEUE_RESCUE_PROBE_TIMEOUT_MS ?? '5000', - 10, -) -const QUEUE_RESCUE_REFILL_THRESHOLD = Number.parseInt( - process.env.QUEUE_RESCUE_REFILL_THRESHOLD ?? '3', - 10, -) +function readPositiveIntEnv(raw: string | undefined, fallback: number): number { + const parsed = Number.parseInt(raw ?? '', 10) + return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback +} + +const QUEUE_RESCUE_PROBE_TIMEOUT_MS = readPositiveIntEnv( + process.env.QUEUE_RESCUE_PROBE_TIMEOUT_MS, + 5000, +) +const QUEUE_RESCUE_REFILL_THRESHOLD = readPositiveIntEnv( + process.env.QUEUE_RESCUE_REFILL_THRESHOLD, + 3, +)📝 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.function readPositiveIntEnv(raw: string | undefined, fallback: number): number { const parsed = Number.parseInt(raw ?? '', 10) return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback } const QUEUE_RESCUE_PROBE_TIMEOUT_MS = readPositiveIntEnv( process.env.QUEUE_RESCUE_PROBE_TIMEOUT_MS, 5000, ) const QUEUE_RESCUE_REFILL_THRESHOLD = readPositiveIntEnv( process.env.QUEUE_RESCUE_REFILL_THRESHOLD, 3, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bot/src/services/musicManagement/queueRescue.ts` around lines 6 - 13, Validate the parsed values used by QUEUE_RESCUE_PROBE_TIMEOUT_MS and QUEUE_RESCUE_REFILL_THRESHOLD, falling back to 5000 and 3 respectively whenever parsing produces NaN or an invalid value. Preserve the existing environment-variable overrides for valid numeric inputs and ensure the validated constants are used by the probe timeout and replenishment threshold logic.
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Auto-approved: Mechanical refactor: moves queue-management files into services/musicManagement, updating import paths, barrel exports, and jest mocks; all other diffs are formatting or NOSONAR placement. No behavior, contract, or operational changes present.
Re-trigger cubic
LucasSantana-Dev
added a commit
that referenced
this pull request
Aug 10, 2026
Stacked on #1989 (not yet merged). Last content-moving slice of #1968. ## Summary - Moves `queueResolver.ts` (+ its colocated spec) to `services/musicManagement/queueResolver.ts` — the single highest-traffic file in this refactor series, 34+ external importers across `functions/music/commands/*`, `handlers/webMusic/*`, and `handlers/musicButtonHandler.ts`. - Cleanest slice in the series: `queueResolver.ts`'s own imports needed no changes, and all 55 external files reference it via the identical string `utils/music/queueResolver` regardless of their own relative depth, so one sed pass fixed everything. Clean typecheck and test run on the first attempt. - `queueResolver.guard.spec.ts` (similarly named but unrelated — a generic queue-lookup guard test) does not import `queueResolver.ts` at all and correctly stays untouched — confirmed by reading its full contents. ## Note on the critic pass The automated critic agent on this diff returned a false REJECT (reported "no staged diff exists" after only 5 tool calls in 24s — an execution failure, not a real finding). Verified directly instead: re-confirmed the staged diff was intact (`git status`/`git diff --cached --stat`), ran the critic's specific requested checks by hand (`require()` sweep, path-alias sweep, cross-package reference sweep, read `queueResolver.guard.spec.ts` in full, spot-checked 4 of the 55 fixed files) — all clean. ## Verification - [x] `npm run type:check --workspace=@lucky/bot` — clean, first attempt - [x] `npm run test:bot` — 242/243 suites (same pre-existing skip, unchanged count) - [x] `npm run lint --workspace=@lucky/bot` — 0 errors, 73 pre-existing warnings (unchanged) - [x] Manual verification in place of a failed automated critic run — see above <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Moved the music queue resolver to `services/musicManagement/queueResolver` and updated all command, handler, and test imports. Also extracted `requireDJRoleInGuild` to dedupe DJ-role guard logic in `replay` and `volume`; queue resolution behavior is unchanged. - **Refactors** - Moved the colocated spec to `services/musicManagement/queueResolver.spec.ts`. - Added `requireDJRoleInGuild` in `utils/command/commandValidations` and updated `replay` and `volume` to use it; added `utils/command/commandValidations.spec.ts`. - Strengthened `requireDJRoleInGuild` tests to assert delegation to guild settings using the resolved `guildId`, and reset its mocks between tests to avoid order dependence. - Left `queueResolver.guard.spec.ts` unchanged (unrelated guard test). <sup>Written for commit 4e90098. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1990?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Improvements** - Improved reliability when locating music queues across active playback sessions. - Added more robust handling and diagnostics when a music queue cannot be found. - Strengthened guild-based DJ permission checks across music commands. - Standardized music command responses and formatting for a more consistent experience. - **Tests** - Expanded coverage for queue detection, permission validation, and music command behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
3 tasks done
LucasSantana-Dev
added a commit
that referenced
this pull request
Sep 6, 2026
## Summary Closes #1968. Deleted `packages/bot/src/utils/music/service.ts` (`TrackManagementService`) and its spec. Confirmed via repo-wide grep: zero production importers, direct or via barrel - the `utils/music/index.ts` barrel that once re-exported it was already deleted separately. Only its own 445-line spec file referenced it. This was #1984's finding (closed, but the actual deletion never landed - #1989's PR body says it was "deliberately left in place" pending a maintainer call). #1968 explicitly named this file as in its scope, so making that call here. **#1968's broader state**, re-verified while investigating this: the directory it originally flagged (136 files, 10.5k LOC of misplaced stateful services mixed into a `utils/` folder) is now 34 files / 3535 LOC. Prior bounded-slice PRs (#1985-#1989 and others, all explicitly scoped as "Nth bounded slice of #1968") already moved every DB/API-touching, timer-driven service out to `services/musicRecommendation/` and `services/musicManagement/` (`watchdog.ts`, `queueResolver.ts`, `queueStateManager.ts`, `sessionStartupRestore.ts`, the `queue*.ts` mutation files, the `autoplay/` cluster including `replenisher.ts`/`candidateScorer.ts`, `candidateFallback.ts`, `sessionSnapshots.ts`, `collaborativePlaylist.ts` - all exactly matching the issue's original "suggested approach" destinations). Checked every remaining class-based file in `utils/music/` for the same misplacement pattern (DB access, external API calls, timers/recurring background state): - `search/engineManager.ts`'s only `setTimeout` is an inline retry-backoff delay inside a function call, not a persistent scheduler - `titleComparison/service.ts`'s `Map` is a plain in-memory memoization cache, no DB/API/timers - Everything else in the remaining 34 files is pure functions/type helpers (`trackValidator.ts`, `trackNormalization.ts`, `searchQueryCleaner.ts`, `buttonComponents.ts`, `skipReasonMap.ts`, etc.) - matching the issue's own "Keep in utils/" list None of the remaining files match the pattern that made the original 136-file audit finding real. #1968 is functionally complete with this PR. ## Test plan - [x] Repo-wide grep confirms zero references to `TrackManagementService`/`trackManagementService` outside the deleted files - [x] `tsc --noEmit` clean across bot/backend/frontend/shared - [x] Full bot suite: 250 suites / 3285 tests pass (down from 251/3286 by exactly the deleted spec's own tests, no other regressions) <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Removes the unused `TrackManagementService` from `packages/bot/src/utils/music/service.ts` and its spec file. It has no production importers (the barrel re-export was already removed), so only its own tests referenced it. **Context** - It was orphaned after queue management logic migrated to `services/musicManagement/` as part of #1968. - Reviewed the remaining 34 files in `utils/music/`; none match the misplaced-stateful-service pattern (DB access, API calls, timers), so #1968 is complete with this deletion. - `tsc` is clean and the bot suite passes with only the deleted spec's tests gone (250 suites / 3285 tests). <sup>Written for commit 900210d. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2276?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
LucasSantana-Dev
added a commit
that referenced
this pull request
Sep 10, 2026
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
First slice of #1991. Does not close it: four subdirectories remain. ## Why this is not a utils module `utils/music/trackUtils/` is a stateful service tree: - `TrackUtils` (barrel) holds a `TrackProcessor` - `TrackProcessor` owns a `TrackCacheManager` and a cleanup timer (`startCacheCleanup`) - `TrackCacheManager` wraps an `LRUCache` - the barrel exports a module-level singleton: `export const trackUtils = new TrackUtils()` Same pattern as every finding in #1968, which established `services/musicManagement/` as the destination across its 7 slices (#1981, #1982, #1985, #1987, #1988, #1989, #1990). ## Surface Only `getTrackInfo` escapes the directory, so the external surface is four files: | file | reference | |---|---| | `functions/music/commands/queue/queueDisplay.ts` | `import { getTrackInfo }` | | `functions/music/commands/queue/queueStats.ts` | `import { getTrackInfo }` | | `functions/music/commands/queue/queueDisplay.spec.ts` | `jest.mock('...')` string literal | | `functions/music/commands/queue/queueStats.spec.ts` | `jest.mock('...')` string literal | The `jest.mock()` string literals do not move with an import rewrite. #1991 flags these as the most common miss in the #1968 slices, and the same trap applied *inside* the moved specs, which mock `'../titleComparison'` and `require()` it in five more places. ## Path arithmetic Both directories sit three levels under `src/`, so relative depth is unchanged. Only the one sibling import needed rewriting: ``` '../titleComparison' -> '../../../utils/music/titleComparison' ``` That is services depending on utils, which is the correct direction. The inversion #1991 is about is utils depending on services, which this avoids by moving the whole tree rather than `cacheManager.ts` alone. `titleComparison` is itself on the #1991 list and moves in a later slice. ## Verification - `packages/bot` full suite: 273 suites passed, 3586 tests passed, 1 skipped, matching the pre-move baseline exactly (pure move, no test added or removed) - `npx tsc --noEmit`: clean - `npx madge --circular --extensions ts packages/bot/src`: `No circular dependency found!` - `npm run build --workspace=packages/bot`: succeeds - repo-wide grep for `utils/music/trackUtils` across ts/tsx/js/json/yml/md: no remaining references ## Remaining in #1991 `search/engineManager.ts`, `search/providerHealth.ts` (8+ external consumers, the largest slice), `titleComparison/service.ts`, `youtubeErrorHandler/analyzer.ts`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Relocates the stateful `trackUtils` service from `utils/music/` to `services/musicManagement/`, matching the destination established in `#1968` and continuing the `#1991` refactor. Since only the queue commands consume `getTrackInfo`, the external change is limited to updating their imports and the `jest.mock()` path literals in their specs. **Notes** - Moved specs also updated their `jest.mock('../titleComparison')` and `require()` calls to the new relative path. - `titleComparison` remains in `utils` and will move in a later slice; `search/engineManager`, `search/providerHealth`, and `youtubeErrorHandler/analyzer` are still pending. - Full test suite and `tsc` pass with no circular dependencies. <sup>Written for commit 5a734c9. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2353?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. --> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
Stacked on #1988 (not yet merged).
Summary
queueEditOps.ts,queueRescue.ts,queueManipulation.ts(the re-export aggregator for the first two),queueOperations.ts, andqueueStateManager.tstoservices/musicManagement/.utils/music/queue/asyncQueueManager.ts(single consumer:queueOperations.ts). Its pure siblingsmartShuffle.tsstays behind inutils/music/queue/— splitting that directory since only one of its two files is actually a service.utils/music/index.ts's barrel re-exports andutils/music/service.ts(the chore(bot): utils/music/index.ts barrel has zero external consumers #1984 dead-code file, deliberately left in place) whose imports of the two moved files needed updating.Verification history
Two rounds of fix via typecheck+test, not grep alone: a missed
trackNormalizationimport inqueueManipulation.ts, two off-by-one relative-path errors inservice.ts, andservice.spec.ts'sjest.mock()paths (mock string literals aren't type-checked, only surfaced by running the suite). Critic pass: ACCEPT, one stale doc comment fixed (searchQueryCleaner.tsreferencedqueueManipulation.ts's old location).Verification
npm run type:check --workspace=@lucky/bot— cleannpm run test:bot— 242/243 suites (same pre-existing skip, unchanged count)npm run lint --workspace=@lucky/bot— 0 errors, 73 pre-existing warnings (unchanged)require()calls, learned from refactor(bot): move autoplay engine out of utils into services #1988) — zero stale referencesSummary by cubic
Moves music queue management from utils into
services/musicManagementto reflect its service role and reduce utils bloat. Updates imports/tests and CPD exclusions, and fixes Sonar/CodeQL by pinning the NOSONAR on a regex and removing an unused test import; no behavior changes.Refactors
queueEditOps.ts,queueRescue.ts,queueManipulation.ts,queueOperations.ts,queueStateManager.ts, andqueue/asyncQueueManager.tstoservices/musicManagement/(kept purequeue/smartShuffle.tsin utils).services/musicManagement/*; adjusted internals (queueManipulation.tsnow re-exports from../../utils/music/trackNormalization,queueOperations.ts/queueStateManager.tsuse../../utils/music/{types,trackValidator}); fixedutils/music/{index.ts,service.ts}; refreshed CPD exclusions for new paths.Bug Fixes
searchQueryCleaner.tsand added prettier-ignore to prevent reflow that dropped the suppression.utils/music/service.spec.ts.Written for commit 8de7ff0. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests