Repository navigation
feat(bot): multi-user voice channel taste blend - #573
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds multi-user Last.fm seed blending: voice-channel member IDs are captured into queue metadata, Last.fm links are resolved for VC members, blended round-robin seeds are produced when multiple users are linked, and autoplay status embeds show a “Blending taste” indicator when applicable. Changes
Sequence DiagramsequenceDiagram
participant PlayCmd as Play Command
participant VoiceChannel as Voice Channel
participant Queue as Queue Metadata
participant LastFmService as Last.fm Service
participant SeedLogic as Seed Consumption
participant Autoplay as Autoplay / Status Embed
PlayCmd->>VoiceChannel: request member list
VoiceChannel-->>PlayCmd: return vcMemberIds
PlayCmd->>Queue: store vcMemberIds in metadata
Autoplay->>Queue: read metadata.vcMemberIds
Autoplay->>LastFmService: getByDiscordId for each member
LastFmService-->>Autoplay: return linked Last.fm usernames
Autoplay->>SeedLogic: if >1 users -> consumeBlendedSeedSlice(userIds, count)
Autoplay->>SeedLogic: if 1 user -> consumeLastFmSeedSlice(userId, count)
SeedLogic-->>Autoplay: return seed tracks
Autoplay->>Autoplay: build embed description lines (include "Blending taste" if >1)
Autoplay-->>Queue: enqueue tracks / update status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bot/src/utils/music/queueManipulation.ts (1)
702-704: Consider using a type predicate for cleaner null filtering.The current pattern works but the
as string[]cast could be avoided with a type predicate.♻️ Optional: Type-safe null filtering
- const linkedUserIds = linkedUsers.filter( - (id) => id !== null, - ) as string[] + const linkedUserIds = linkedUsers.filter( + (id): id is string => id !== null, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/queueManipulation.ts` around lines 702 - 704, The current null-filtering for linkedUsers casts the result to string[] which is unnecessary; update the filter to use a type predicate so the compiler knows nulls are removed (e.g., replace the predicate with one like (id): id is string => id !== null) and then assign to linkedUserIds without the as string[] cast, referencing the linkedUsers array and linkedUserIds constant in queueManipulation.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 702-704: The current null-filtering for linkedUsers casts the
result to string[] which is unnecessary; update the filter to use a type
predicate so the compiler knows nulls are removed (e.g., replace the predicate
with one like (id): id is string => id !== null) and then assign to
linkedUserIds without the as string[] cast, referencing the linkedUsers array
and linkedUserIds constant in queueManipulation.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72ce0e04-c3c4-4dd5-8737-c1a63d0a03c2
📒 Files selected for processing (8)
packages/bot/src/functions/music/commands/autoplay.tspackages/bot/src/functions/music/commands/play/index.tspackages/bot/src/lastfm/index.tspackages/bot/src/types/QueueMetadata.tspackages/bot/src/utils/music/autoplay/lastFmSeeds.spec.tspackages/bot/src/utils/music/autoplay/lastFmSeeds.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/utils/music/queueManipulation.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Quality Gates
- GitHub Check: SonarCloud Scan
🔇 Additional comments (9)
packages/bot/src/utils/music/autoplay/lastFmSeeds.ts (1)
133-157: LGTM! Clean implementation of multi-user seed blending.The round-robin interleaving logic is correct, and delegating to
consumeLastFmSeedSlicefor each user ensures per-user locking and error handling are preserved. TheMath.ceilforperUserCountensures sufficient tracks are fetched from each user before interleaving.packages/bot/src/types/QueueMetadata.ts (1)
8-8: LGTM!The new optional
vcMemberIdsfield is appropriately typed and follows the existing pattern of optional metadata properties.packages/bot/src/lastfm/index.ts (1)
15-18: LGTM!Clean re-export of both seed slice functions, maintaining consistent module boundaries.
packages/bot/src/functions/music/commands/play/index.ts (2)
131-135: LGTM! Correctly captures voice channel members for blending.The implementation properly filters out the bot user and extracts member IDs. The defensive check for
voiceChannel.membersexistence is a safe pattern even though it should always be defined for voice channels.
141-141: LGTM!Metadata correctly includes
vcMemberIdsfor downstream consumption by autoplay logic.packages/bot/src/functions/music/commands/autoplay.ts (1)
158-180: LGTM! Clear implementation of blending status display.The logic correctly:
- Fetches Last.fm links in parallel for efficiency
- Only shows the blending message when 2+ users have accounts linked
- Matches the behavior in
queueManipulation.tsfor consistencypackages/bot/src/utils/music/queueManipulation.spec.ts (1)
2373-2538: LGTM! Comprehensive test coverage for multi-user blending scenarios.The test suite correctly covers:
- Blended seeds when multiple VC members have Last.fm linked
- Fallback to single-user when only one member is linked
- Fallback when VC has only one user
- Proper usage of
metadata.vcMemberIdsGood use of mock setup to simulate different Last.fm link states.
packages/bot/src/utils/music/queueManipulation.ts (1)
689-723: LGTM! Well-structured multi-user blending logic.The implementation correctly handles all scenarios:
- Multiple linked VC users → blended seeds
- Single linked user (regardless of who) → single-user seeds
- No other VC members → requester's seeds only
The parallel Last.fm link lookups via
Promise.allare efficient.packages/bot/src/utils/music/autoplay/lastFmSeeds.spec.ts (1)
433-532: LGTM! Good coverage ofconsumeBlendedSeedSlicebehavior.The tests cover essential scenarios including empty input, deduplication, count limits, and fallback behavior.
One observation: the interleaving test (lines 449-470) uses the same mock data for both users, which doesn't fully demonstrate round-robin interleaving between distinct track sets. The deduplication test compensates for this somewhat. Consider enhancing interleaving verification in a follow-up if needed.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bot/src/functions/music/commands/autoplay.spec.ts (1)
242-276: Tighten status assertions to validate count + lookup calls.Current checks only look for
'Blending taste'. Consider asserting exact count text andgetByDiscordIdcall inputs so regressions in linked-user counting are caught (Line 252 and Line 271 paths).Suggested assertion hardening
it('should show blend info when multiple vc members have last.fm', async () => { getLastFmLinkMock.mockResolvedValue({ lastFmUsername: 'user' }) const interaction = createInteraction('status') const queue = createQueue({ vcMemberIds: ['user-1', 'user-2'] }) const client = createClient() resolveGuildQueueMock.mockReturnValue({ queue }) await autoplayCommand.execute({ client, interaction } as any) + expect(getLastFmLinkMock).toHaveBeenCalledTimes(2) + expect(getLastFmLinkMock).toHaveBeenNthCalledWith(1, 'user-1') + expect(getLastFmLinkMock).toHaveBeenNthCalledWith(2, 'user-2') expect(createEmbedMock).toHaveBeenCalledWith( expect.objectContaining({ - description: expect.stringContaining('Blending taste'), + description: expect.stringContaining('🎭 Blending taste for 2 users'), }), ) }) it('should not show blend when only one member has last.fm', async () => { getLastFmLinkMock .mockResolvedValueOnce({ lastFmUsername: 'user1' }) .mockResolvedValueOnce(null) @@ resolveGuildQueueMock.mockReturnValue({ queue }) await autoplayCommand.execute({ client, interaction } as any) + expect(getLastFmLinkMock).toHaveBeenCalledTimes(2) expect(createEmbedMock).toHaveBeenCalledWith( expect.objectContaining({ description: expect.not.stringContaining('Blending taste'), }), ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/functions/music/commands/autoplay.spec.ts` around lines 242 - 276, Tighten the two autoplay.spec.ts tests by asserting the exact blend-count text and verifying the lookup calls: after executing autoplayCommand.execute, check createEmbedMock was called with a description that includes the precise count phrase (e.g., "Blending taste (2)" or whatever the UI shows) rather than just substring matching, and also assert that getLastFmLinkMock (or the user lookup function getByDiscordId if used) was called with the expected vc member IDs (e.g., 'user-1' and 'user-2') and the correct number of times; update both the "multiple vc members" test and the "only one member" test to include these exact description and mock-call assertions referencing getLastFmLinkMock/createEmbedMock (and getByDiscordId if present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bot/src/functions/music/commands/autoplay.spec.ts`:
- Around line 242-276: Tighten the two autoplay.spec.ts tests by asserting the
exact blend-count text and verifying the lookup calls: after executing
autoplayCommand.execute, check createEmbedMock was called with a description
that includes the precise count phrase (e.g., "Blending taste (2)" or whatever
the UI shows) rather than just substring matching, and also assert that
getLastFmLinkMock (or the user lookup function getByDiscordId if used) was
called with the expected vc member IDs (e.g., 'user-1' and 'user-2') and the
correct number of times; update both the "multiple vc members" test and the
"only one member" test to include these exact description and mock-call
assertions referencing getLastFmLinkMock/createEmbedMock (and getByDiscordId if
present).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90de6aef-da38-487d-ac3b-8666094c1d6c
📒 Files selected for processing (2)
packages/bot/src/functions/music/commands/autoplay.spec.tspackages/bot/src/functions/music/commands/play/index.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/bot/src/functions/music/commands/play/index.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: SonarCloud Scan
- GitHub Check: Quality Gates
🔇 Additional comments (3)
packages/bot/src/functions/music/commands/autoplay.spec.ts (3)
52-67: Good mock extension for Last.fm link lookup.
getLastFmLinkMockis cleanly wired intolastFmLinkService.getByDiscordId, which matches the new status-path dependency and keeps tests isolated.
101-106: Queue fixture update is on point.Adding configurable
metadatamakes the test setup align with runtimeQueueMetadatausage forvcMemberIds.
139-139: Nice default reset for Last.fm mock.Defaulting to
nullinbeforeEachkeeps test isolation predictable across status cases.
|
* feat(bot): add vc blend and blended last.fm seed slices * test(bot): fix blend coverage — mock lastfm links for blend paths * test(bot): add blend status and vcmemberids coverage tests
This branch was successfully deployed
1 participant
Summary
QueueMetadata.vcMemberIdswhen queue is createdconsumeBlendedSeedSlice(userIds, count)— parallel Last.fm slice fetching with round-robin interleave + dedup by normalized track key/autoplay statusshows "🎭 Blending taste for N users" when blend is activeBehavior
Single user → unchanged (existing
consumeLastFmSeedSlicepath)Multiple users with Last.fm → round-robin blend of their top tracks
Test plan
/autoplay statusshows blend messageconsumeBlendedSeedSliceunit tests: interleave, dedup, count capSummary by CodeRabbit
New Features
Tests