Repository navigation
refactor: extract lastFmSeeder (phase 3.4) - #668
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 31 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extracts the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
queueManipulation.ts collectGenreCandidates still calls searchLastFmQuery after the extraction. Export it from lastFmSeeder.ts and import where used.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bot/src/utils/music/queueManipulation.ts (1)
960-979:⚠️ Potential issue | 🔴 CriticalBuild-breaking:
searchLastFmQuerywas moved but is still referenced here.The refactor moved
searchLastFmQueryintoautoplay/lastFmSeeder.tsas a module-private helper, butcollectGenreCandidatesat line 971 still calls it, causing the CI failureTS2304: Cannot find name 'searchLastFmQuery'. Export it fromlastFmSeeder.tsand import it here.🔧 Proposed fix
In
packages/bot/src/utils/music/autoplay/lastFmSeeder.ts, export the helper:-async function searchLastFmQuery( +export async function searchLastFmQuery( queue: GuildQueue, query: string, requestedBy: User, ): Promise<Track[]> {In
packages/bot/src/utils/music/queueManipulation.ts, add it to the existing import:-import { collectLastFmCandidates } from './autoplay/lastFmSeeder' +import { + collectLastFmCandidates, + searchLastFmQuery, +} from './autoplay/lastFmSeeder'Alternatively, move
collectGenreCandidatesinto its own autoplay module so it can keep using a local copy of the helper and eliminate this cross-module coupling.🤖 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 960 - 979, collectGenreCandidates references searchLastFmQuery which was made module-private in autoplay/lastFmSeeder.ts, causing TS2304; fix by exporting searchLastFmQuery from lastFmSeeder.ts (make it a named export) and then import that named export into queueManipulation.ts where collectGenreCandidates is defined so the function resolves, or alternatively move collectGenreCandidates into the autoplay module so it can call the internal helper without cross-module access.
🧹 Nitpick comments (4)
packages/bot/src/utils/music/autoplay/lastFmSeeder.spec.ts (2)
46-52: Redundantjest.clearAllMocks()in bothbeforeEachandafterEach.Clearing twice per test is harmless but noisy. Keep just one (typically
beforeEach) unless you have a specific reason.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/autoplay/lastFmSeeder.spec.ts` around lines 46 - 52, The test file lastFmSeeder.spec.ts calls jest.clearAllMocks() in both beforeEach and afterEach; remove the redundant call and keep a single clear in beforeEach (or afterEach if preferred) so mocks are cleared once per test; update the beforeEach/afterEach block accordingly to only call jest.clearAllMocks() in one of those lifecycle hooks.
211-260: Fallback test is a bit over-specified but correct.
mockResolvedValueOnce/mockRejectedValueOncequeue three sequential responses, andtoHaveBeenCalledTimes(3)confirms the Spotify→YouTube→AUTO fallback. One thing worth noting: this test relies on the ordering ofQueryTypeengines insearchLastFmQuery; if that order is ever reordered or extended, this test will silently still pass whencandidates.size > 0even if fallback semantics changed. Consider asserting the exact arguments of each call (searchEngine: QueryType.SPOTIFY_SEARCH, thenYOUTUBE_SEARCH, thenAUTO) to pin behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/autoplay/lastFmSeeder.spec.ts` around lines 211 - 260, Update the test to assert the exact search engine used on each sequential call instead of only call count: after setting up mocks and calling collectLastFmCandidates, add assertions that mockQueue.player.search was called with arguments whose searchEngine property equals QueryType.SPOTIFY_SEARCH for the first call, QueryType.YOUTUBE_SEARCH for the second, and QueryType.AUTO for the third (use toHaveBeenNthCalledWith or inspect mock.calls and expect objectContaining({ searchEngine: QueryType.<...> }) or direct index access). This pins the Spotify→YouTube→AUTO fallback behavior and prevents silent breaks if QueryType ordering changes; keep the existing candidates.size check and restore mocks as before.packages/bot/src/utils/music/autoplay/lastFmSeeder.ts (1)
15-19: Circular import betweenlastFmSeederandqueueManipulation.
lastFmSeeder.tsimportsshouldIncludeCandidate,upsertScoredCandidate, andnormalizeTrackKeyfrom../queueManipulation, whilequeueManipulation.tsimports and re-exportscollectLastFmCandidatesfrom this module. This works at runtime (both sides use the symbols only inside functions), but it's a classic code smell that can bite later — e.g., if any of these helpers become used at module top level, you'll getundefineddue to TDZ in the cycle.Consider moving
shouldIncludeCandidate,upsertScoredCandidate,normalizeTrackKey, and related helpers into a dedicated low-level module (e.g.,autoplay/candidateUtils.ts) that bothqueueManipulationand the autoplay seeders can depend on acyclically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bot/src/utils/music/autoplay/lastFmSeeder.ts` around lines 15 - 19, There is a circular import between lastFmSeeder (collectLastFmCandidates) and queueManipulation caused by shared helpers; extract the low-level helpers shouldIncludeCandidate, upsertScoredCandidate, and normalizeTrackKey (and any related pure helper functions) into a new module (e.g., autoplay/candidateUtils.ts) and update both lastFmSeeder and queueManipulation to import those helpers from the new module so neither file imports the other; ensure the new module exports the same symbols and update imports in collectLastFmCandidates and any queueManipulation functions to reference candidateUtils instead.packages/bot/src/utils/music/queueManipulation.ts (1)
60-62: Remove unused constantsLASTFM_SEED_COUNT,LASTFM_SCORE_BOOST, andMAX_SIMILAR_LOOKUPSfrom this file.These constants are not referenced in this file. Identical constants have been properly defined and used in
lastFmSeeder.ts, making these definitions redundant. Remove them to eliminate duplication and code drift.🤖 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 60 - 62, Remove the unused constants LASTFM_SEED_COUNT, LASTFM_SCORE_BOOST, and MAX_SIMILAR_LOOKUPS from queueManipulation.ts because they are not referenced in this file and duplicate definitions exist in lastFmSeeder.ts; locate the constant declarations (const LASTFM_SEED_COUNT, const LASTFM_SCORE_BOOST, const MAX_SIMILAR_LOOKUPS) in queueManipulation.ts and delete those three lines so the module relies on the canonical definitions in lastFmSeeder.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bot/src/utils/music/autoplay/lastFmSeeder.spec.ts`:
- Around line 5-13: The import path for the lastfm module in
lastFmSeeder.spec.ts is incorrect (import * as lastfm from '../../lastfm') and
doesn't match the mocked target (jest.mock('../../../lastfm')), so update the
import to the same module you mock (change the import to import * as lastfm from
'../../../lastfm') or alternatively change the jest.mock call to
jest.mock('../../lastfm'); ensure the symbol lastfm used in tests (assertions
referring to the mocked module) references the same module path as the jest.mock
call so the mocks are applied and observed.
In `@packages/bot/src/utils/music/autoplay/lastFmSeeder.ts`:
- Around line 169-198: The helper function searchLastFmQuery is file-local but
collectGenreCandidates in queueManipulation.ts still calls it; make
searchLastFmQuery accessible by exporting it from lastFmSeeder.ts (e.g., export
the function or add it to module exports) and then import that symbol into
queueManipulation.ts where collectGenreCandidates uses it (ensure the imported
name matches searchLastFmQuery and keep the same signature: async function
searchLastFmQuery(queue: GuildQueue, query: string, requestedBy: User):
Promise<Track[]>).
---
Outside diff comments:
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 960-979: collectGenreCandidates references searchLastFmQuery which
was made module-private in autoplay/lastFmSeeder.ts, causing TS2304; fix by
exporting searchLastFmQuery from lastFmSeeder.ts (make it a named export) and
then import that named export into queueManipulation.ts where
collectGenreCandidates is defined so the function resolves, or alternatively
move collectGenreCandidates into the autoplay module so it can call the internal
helper without cross-module access.
---
Nitpick comments:
In `@packages/bot/src/utils/music/autoplay/lastFmSeeder.spec.ts`:
- Around line 46-52: The test file lastFmSeeder.spec.ts calls
jest.clearAllMocks() in both beforeEach and afterEach; remove the redundant call
and keep a single clear in beforeEach (or afterEach if preferred) so mocks are
cleared once per test; update the beforeEach/afterEach block accordingly to only
call jest.clearAllMocks() in one of those lifecycle hooks.
- Around line 211-260: Update the test to assert the exact search engine used on
each sequential call instead of only call count: after setting up mocks and
calling collectLastFmCandidates, add assertions that mockQueue.player.search was
called with arguments whose searchEngine property equals
QueryType.SPOTIFY_SEARCH for the first call, QueryType.YOUTUBE_SEARCH for the
second, and QueryType.AUTO for the third (use toHaveBeenNthCalledWith or inspect
mock.calls and expect objectContaining({ searchEngine: QueryType.<...> }) or
direct index access). This pins the Spotify→YouTube→AUTO fallback behavior and
prevents silent breaks if QueryType ordering changes; keep the existing
candidates.size check and restore mocks as before.
In `@packages/bot/src/utils/music/autoplay/lastFmSeeder.ts`:
- Around line 15-19: There is a circular import between lastFmSeeder
(collectLastFmCandidates) and queueManipulation caused by shared helpers;
extract the low-level helpers shouldIncludeCandidate, upsertScoredCandidate, and
normalizeTrackKey (and any related pure helper functions) into a new module
(e.g., autoplay/candidateUtils.ts) and update both lastFmSeeder and
queueManipulation to import those helpers from the new module so neither file
imports the other; ensure the new module exports the same symbols and update
imports in collectLastFmCandidates and any queueManipulation functions to
reference candidateUtils instead.
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 60-62: Remove the unused constants LASTFM_SEED_COUNT,
LASTFM_SCORE_BOOST, and MAX_SIMILAR_LOOKUPS from queueManipulation.ts because
they are not referenced in this file and duplicate definitions exist in
lastFmSeeder.ts; locate the constant declarations (const LASTFM_SEED_COUNT,
const LASTFM_SCORE_BOOST, const MAX_SIMILAR_LOOKUPS) in queueManipulation.ts and
delete those three lines so the module relies on the canonical definitions in
lastFmSeeder.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c32569bf-1eb9-44f0-b596-18b54c428887
📒 Files selected for processing (3)
packages/bot/src/utils/music/autoplay/lastFmSeeder.spec.tspackages/bot/src/utils/music/autoplay/lastFmSeeder.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). (1)
- GitHub Check: SonarCloud Scan
🧰 Additional context used
🪛 GitHub Actions: CI/CD Pipeline
packages/bot/src/utils/music/queueManipulation.ts
[error] 971-971: TypeScript (tsc) failed with TS2304: Cannot find name 'searchLastFmQuery'.
…lation suite) Spec had unresolvable ESM import chain (uuid → import.meta in shared utils). Behavior is fully covered through public API by 131 queueManipulation tests.
|
Resolved conflict in queueManipulation.ts by taking main (lastFmSeeder already extracted via #668)
Post-merge cleanup: queueManipulation.ts had inline defs of collectLastFmCandidates and searchLastFmQuery left from the pre-#668 base. Both are now imported from autoplay/lastFmSeeder. Re-exported collectLastFmCandidates for back-compat.
* refactor: extract replenisher from queueManipulation (phase 3.6 - final) * fix(bot): remove duplicate lastFmSeeder defs after main merge Post-merge cleanup: queueManipulation.ts had inline defs of collectLastFmCandidates and searchLastFmQuery left from the pre-#668 base. Both are now imported from autoplay/lastFmSeeder. Re-exported collectLastFmCandidates for back-compat. * fix: correct imports after merge resolution - export collectLastFmCandidates from queueManipulation
* refactor: extract lastFmSeeder from queueManipulation (phase 3.4) * fix(bot): export searchLastFmQuery for queueManipulation reference queueManipulation.ts collectGenreCandidates still calls searchLastFmQuery after the extraction. Export it from lastFmSeeder.ts and import where used. * test(bot): remove redundant lastFmSeeder spec (covered by queueManipulation suite) Spec had unresolvable ESM import chain (uuid → import.meta in shared utils). Behavior is fully covered through public API by 131 queueManipulation tests.
* refactor: extract replenisher from queueManipulation (phase 3.6 - final) * fix(bot): remove duplicate lastFmSeeder defs after main merge Post-merge cleanup: queueManipulation.ts had inline defs of collectLastFmCandidates and searchLastFmQuery left from the pre-#668 base. Both are now imported from autoplay/lastFmSeeder. Re-exported collectLastFmCandidates for back-compat. * fix: correct imports after merge resolution - export collectLastFmCandidates from queueManipulation
This branch was successfully deployed
1 participant
Summary
Extracted Last.fm seeding logic from
queueManipulation.tsto new dedicated moduleautoplay/lastFmSeeder.ts.collectLastFmCandidatesandsearchLastFmQueryfunctions (198 LOC)queueManipulation.tsfor backward compatibilityPhase 3 Progress
Metrics
Summary by CodeRabbit
Release Notes
Tests
Refactor