Repository navigation
fix(autoplay): block Spanish gospel tracks when Last.fm is not linked - #820
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR extends Last.fm error handling with a typed session-expiration error, adds optional Spotify-fallback support to artist tag fetching (used when Last.fm returns empty results), integrates token-driven Spotify genre lookup into queue replenishment, and hardens SoundCloud stream error reporting. All changes include comprehensive test coverage. ChangesLast.fm Session Error & Spotify-Fallback Artist Tag Caching
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🤖 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/handlers/player/soundcloudMatcher.ts`:
- Around line 39-43: The catch block in soundcloudMatcher's stream creation
constructs an Error using (err as Error).message which becomes "undefined" for
non-Error throws; update the throw in the catch to build the message from a safe
conversion (e.g., use err instanceof Error ? err.message : String(err)) so the
thrown Error for SoundCloud stream creation includes meaningful text for
match.name, and keep the original err as the cause when rethrowing; locate the
catch around playdl.stream usage in soundcloudMatcher.ts and replace the unsafe
(err as Error).message usage with the safe conversion.
In `@packages/bot/src/handlers/player/streamBridge.spec.ts`:
- Around line 177-187: The test enables fake timers with jest.useFakeTimers()
but restores them only at the end of the test, which can leak if the assertion
fails; move the restoration into a shared cleanup (e.g., an afterEach) so
jest.useRealTimers() always runs; locate uses of
jest.useFakeTimers()/jest.useRealTimers() in the streamViaYtDlp spec (test
"kills proc and rejects on timeout") and add an afterEach that calls
jest.useRealTimers() (and any necessary timer cleanup) to guarantee real timers
are restored even on test failures.
🪄 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: 82ff024d-5379-4cd3-b6b2-2783df99ec5d
📒 Files selected for processing (13)
packages/bot/src/functions/music/commands/autoplay/queueHandlers.tspackages/bot/src/handlers/player/playerFactory.tspackages/bot/src/handlers/player/soundcloudMatcher.spec.tspackages/bot/src/handlers/player/soundcloudMatcher.tspackages/bot/src/handlers/player/streamBridge.spec.tspackages/bot/src/handlers/player/streamBridge.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/utils/music/autoplay/artistTagCache.tspackages/bot/src/utils/music/autoplay/replenisher.tspackages/bot/src/utils/music/candidateFallback.spec.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/utils/music/languageHeuristics.tspackages/bot/src/utils/music/queueManipulation.spec.ts
👮 Files not reviewed due to content moderation or server errors (8)
- packages/bot/src/utils/music/languageHeuristics.ts
- packages/bot/src/lastfm/lastFmApi.ts
- packages/bot/src/functions/music/commands/autoplay/queueHandlers.ts
- packages/bot/src/utils/music/queueManipulation.spec.ts
- packages/bot/src/utils/music/autoplay/replenisher.ts
- packages/bot/src/utils/music/candidateFallback.ts
- packages/bot/src/utils/music/candidateFallback.spec.ts
- packages/bot/src/utils/music/autoplay/artistTagCache.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 context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use theisPrisma*Error()helper functions to check for specific Prisma error types (e.g.,isPrismaForeignKeyError,isPrismaUniqueConstraintError) instead of manually checking error codes
Use Prisma's$transaction()method to ensure database operations are atomic and avoid partial updates when multiple related tables are modified
Always useselectorincludein Prisma queries to explicitly specify which fields to return, avoiding unnecessary data transfer
For Redis operations, use connection pooling and implement exponential backoff retry logic for transient failures
Always uselogAndRethrow()orlogAndSwallow()utilities when handling errors to ensure errors are logged with context before propagating or suppressing
Files:
packages/bot/src/functions/music/commands/autoplay/queueHandlers.tspackages/bot/src/handlers/player/soundcloudMatcher.tspackages/bot/src/utils/music/autoplay/replenisher.tspackages/bot/src/utils/music/languageHeuristics.tspackages/bot/src/utils/music/autoplay/artistTagCache.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/handlers/player/playerFactory.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/handlers/player/streamBridge.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/utils/music/candidateFallback.spec.tspackages/bot/src/handlers/player/streamBridge.spec.tspackages/bot/src/handlers/player/soundcloudMatcher.spec.ts
packages/{bot,backend,shared}/src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/{bot,backend,shared}/src/**/*.ts: For feature toggles, check both global and guild-specific toggles usingFeatureToggleService.isEnabledForGuild()rather than checking them separately
Use branded types (e.g.,GuildId,UserId,ChannelId) for Discord IDs throughout the codebase to prevent type-level ID confusion
Files:
packages/bot/src/functions/music/commands/autoplay/queueHandlers.tspackages/bot/src/handlers/player/soundcloudMatcher.tspackages/bot/src/utils/music/autoplay/replenisher.tspackages/bot/src/utils/music/languageHeuristics.tspackages/bot/src/utils/music/autoplay/artistTagCache.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/handlers/player/playerFactory.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/handlers/player/streamBridge.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/utils/music/candidateFallback.spec.tspackages/bot/src/handlers/player/streamBridge.spec.tspackages/bot/src/handlers/player/soundcloudMatcher.spec.ts
packages/bot/src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building Discord embeds, use
EmbedBuilderService.createTemplate()orEmbedBuilderService.getTemplate()instead of constructing embeds directly
Files:
packages/bot/src/functions/music/commands/autoplay/queueHandlers.tspackages/bot/src/handlers/player/soundcloudMatcher.tspackages/bot/src/utils/music/autoplay/replenisher.tspackages/bot/src/utils/music/languageHeuristics.tspackages/bot/src/utils/music/autoplay/artistTagCache.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/handlers/player/playerFactory.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/handlers/player/streamBridge.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/utils/music/candidateFallback.spec.tspackages/bot/src/handlers/player/streamBridge.spec.tspackages/bot/src/handlers/player/soundcloudMatcher.spec.ts
packages/bot/src/handlers/player/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
MusicSessionSnapshotServiceto save and restore queue state, enabling users to resume playback after disconnections
Files:
packages/bot/src/handlers/player/soundcloudMatcher.tspackages/bot/src/handlers/player/playerFactory.tspackages/bot/src/handlers/player/streamBridge.tspackages/bot/src/handlers/player/streamBridge.spec.tspackages/bot/src/handlers/player/soundcloudMatcher.spec.ts
packages/bot/src/utils/music/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement error recovery in music queue operations by using
rescueQueue()when tracks fail to load or dependencies are unavailable
Files:
packages/bot/src/utils/music/autoplay/replenisher.tspackages/bot/src/utils/music/languageHeuristics.tspackages/bot/src/utils/music/autoplay/artistTagCache.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/utils/music/candidateFallback.spec.ts
packages/bot/src/utils/music/autoplay/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For autoplay candidate selection, use genre-family-based penalties via
getGenreFamilies()to avoid monotonous genre clustering
Files:
packages/bot/src/utils/music/autoplay/replenisher.tspackages/bot/src/utils/music/autoplay/artistTagCache.ts
packages/bot/src/{spotify,utils/music}/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Extract Spotify track IDs using
extractSpotifyTrackId()before passing to Spotify API calls to prevent malformed requests
Files:
packages/bot/src/utils/music/autoplay/replenisher.tspackages/bot/src/utils/music/languageHeuristics.tspackages/bot/src/utils/music/autoplay/artistTagCache.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/utils/music/candidateFallback.spec.ts
🔇 Additional comments (4)
packages/bot/src/handlers/player/playerFactory.ts (1)
37-58: LGTM — extractor initialization order correctly updated.The header comment accurately documents the new sequence, and
initPlayDlSoundCloud()correctly runs beforeloadYoutubeExtractorso the SoundCloud token is available before the bridge is needed.packages/bot/src/handlers/player/streamBridge.ts (1)
84-90: LGTM —proc.kill()correctly placed beforerejectin the error handler.Setting
settled = truebefore callingproc.kill()ensures the subsequentcloseevent is correctly short-circuited by theif (settled) returnguard, preventing a double-reject.packages/bot/src/handlers/player/soundcloudMatcher.spec.ts (1)
1-228: Well-structured test suite with thorough coverage of all changed paths.Boundary tests for the 75% token threshold, the ±30 s duration window (including exact-boundary acceptance), and the new error-wrapping behavior are all correctly validated.
packages/bot/src/handlers/player/streamBridge.spec.ts (1)
217-327: ⚡ Quick winThe Jest configuration already has
clearMocks: trueenabled globally.The
packages/bot/jest.config.cjs(which applies to this test file) explicitly setsclearMocks: true,resetMocks: true, andrestoreMocks: true(lines 46–48). Mock calls are automatically cleared before each test, so assertions usingtoHaveBeenCalledTimes()andmock.calls[n]are not fragile, and the test order-dependency concern does not apply here.> Likely an incorrect or invalid review comment.
When Last.fm is unlinked the artist-tag fetcher returned [] for every
artist, silently bypassing the cross-locale Spanish veto. Spanish
gospel artists like Marcos Witt, Alex Zurdo, and Christine D'Clario
carry no Spanish diacritics in their names, so the text-only heuristic
also missed them.
Three-layer fix:
• artistTagCache: accept an optional Spotify genre fallback so the veto
fires on Spotify genre strings ('latin worship', 'musica cristiana',
'latin gospel') even without Last.fm
• replenisher: fetch a single Spotify token early and thread it through
createArtistTagFetcher, covering every candidate collector in the pass
• candidateFallback: wire genreContext (including getArtistTags) into
collectBroadFallbackCandidates, the only path that previously received
no genre context at all
• languageHeuristics: add 'latin worship', 'ccm en español',
'spanish ccm' to SPANISH_GENRE_MARKERS and 11 gospel-specific tokens
to SPANISH_DISTINCT_TOKENS
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add comprehensive test suite for artistTagCache utility, covering cache behavior, Spotify fallback logic, and concurrent request coalescing. Add tests for new lastFmApi functions: parseArtists (splits featured artists from primary artist) and LastFmSessionExpiredError (error class for expired session keys). Suppress SonarCloud hotspot for internal Last.fm API key in GET request URLs (not user-controlled data) with NOSONAR comments. - artistTagCache.spec.ts: 13 test cases covering cache mechanics, fallback behavior - lastFmApi tests: 12 new tests for parseArtists, 3 for LastFmSessionExpiredError - All 246 affected tests pass Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
e6f2e43 to
da6e091
Compare
…agFetcher When getValidAccessToken returns a token, the code passes a Spotify genre-fetching function to createArtistTagFetcher instead of undefined. Add a test that verifies this branch is exercised to satisfy SonarCloud new-code coverage requirement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…er spec The coverage test triggers the Spotify path which calls getUserSpotifySeeds, which calls spotifyLinkService.getByDiscordId. Without this in the mock the test throws TypeError at runtime. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…edBy on mock track The test for createArtistTagFetcher Spotify path requires currentTrack.requestedBy.id to be truthy so the token fetch branch is entered. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Adds 9 tests covering success path, cache hit, !response.ok, data.error, missing track, empty artist/title, network error, and metadata without album — pushing new code coverage above 80%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Resolved conflicts in lastFmApi.ts/spec.ts after release branch absorbed PR #821 (canonical metadata + S5852-safe parseArtists + getTrackMetadata). Strategy: take release branch's superset version of lastFmApi.ts (it has S5852-safe bounded-repetition regex, full caching, scrobble improvements, in-flight dedup), then patch back the LastFmSessionExpiredError class + error-9 trap from this PR (unique value not in #821). Spec: take release branch's expanded test coverage, add back LastFmSessionExpiredError test block + import. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…ExpiredError
The new LastFmSessionExpiredError class is thrown by signedPost on
error code 9, but its default message ('Last.fm session key has
expired (error code 9)') doesn't match isLastFmInvalidSessionError's
patterns ('error':9 regex, ' 9 - ' substring, 'invalid session key'
substring), so the typed error was being silently dropped by the
detector — exactly the path callers rely on for re-auth.
Add an instanceof short-circuit at the top of isLastFmInvalidSessionError.
Add a regression test covering both the default message and a
custom-message variant.
Addresses Greptile feedback on PR #820.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1. soundcloudMatcher.ts: use type-safe error message extraction ((err as Error).message produces 'undefined' for non-Error throws; use 'err instanceof Error ? err.message : String(err)' instead) 2. streamBridge.spec.ts: protect against fake-timer leak on assertion failure by moving jest.useRealTimers() into afterEach inside the process-lifecycle describe block Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
LucasSantana-Dev has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/bot/src/utils/music/autoplay/artistTagCache.spec.ts (2)
147-160: ⚡ Quick winAdd case-insensitive caching test for Spotify fallback.
The existing caching test for fallback results calls
fetcher('Artist')twice with identical casing. The Last.fm path has a dedicated test (lines 48-62) verifying case-insensitive caching (e.g., "Daft Punk" vs "daft punk"). Consider adding a similar test to verify the Spotify fallback results are cached case-insensitively.🧪 Suggested test case
it('caches fallback results same as Last.fm results', async () => { getArtistTopTags.mockResolvedValue([]) const spotifyFallback = jest .fn<(artist: string) => Promise<string[]>>() .mockResolvedValue(['genre1', 'genre2']) const fetcher = createArtistTagFetcher(spotifyFallback) - const result1 = await fetcher('Artist') - const result2 = await fetcher('Artist') + const result1 = await fetcher('Artist Name') + const result2 = await fetcher('artist name') + const result3 = await fetcher('ARTIST NAME') expect(spotifyFallback).toHaveBeenCalledTimes(1) expect(result1).toEqual(result2) + expect(result2).toEqual(result3) })🤖 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/utils/music/autoplay/artistTagCache.spec.ts` around lines 147 - 160, Add a case-insensitive caching test for the Spotify fallback path: using the existing mocks (getArtistTopTags and spotifyFallback) set getArtistTopTags.mockResolvedValue([]) and spotifyFallback.mockResolvedValue([...]), create the fetcher via createArtistTagFetcher(spotifyFallback), call fetcher('Artist') then fetcher('artist'), and assert spotifyFallback was called only once and both results are equal; reference createArtistTagFetcher, spotifyFallback, getArtistTopTags, and fetcher to locate where to add the test.
12-12: 💤 Low valueConsider using dynamic import for consistency.
The mock is accessed via
require()after ES6 imports at the top. While this pattern works with Jest, consider using dynamicimport()in thebeforeEachblock for consistency with the module imports.♻️ Alternative approach using async beforeEach
-const { getArtistTopTags } = require('../../../lastfm') - describe('createArtistTagFetcher', () => { + let getArtistTopTags: jest.Mock + beforeEach(() => { + const lastfm = require('../../../lastfm') + getArtistTopTags = lastfm.getArtistTopTags jest.clearAllMocks() })🤖 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/utils/music/autoplay/artistTagCache.spec.ts` at line 12, The test currently uses require('../../../lastfm') to get getArtistTopTags after ES module imports; switch to a dynamic import inside the beforeEach so the module is loaded asynchronously and consistently with other ES6 imports: replace the require call with await import('../../../lastfm') inside the beforeEach and extract getArtistTopTags from the imported module (or reassign the module variable), ensuring any mocks are applied before the module is used in tests (refer to getArtistTopTags and beforeEach).packages/bot/src/utils/music/autoplay/replenisher.spec.ts (2)
20-20: 💤 Low valueVerify if
getByDiscordIdmock is needed.The
getByDiscordIdmethod is added to thespotifyLinkServicemock but is never called in any test. Confirm whether this is preparation for future test cases or can be removed.🤖 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/utils/music/autoplay/replenisher.spec.ts` at line 20, The mock property getByDiscordId on the spotifyLinkService mock in replenisher.spec.ts is not used by any test; either remove getByDiscordId from the spotifyLinkService mock object to keep the test surface minimal, or if future tests will call it, add a focused test that uses spotifyLinkService.getByDiscordId (mock its return value via jest.fn()) and assert expected behavior—update the spotifyLinkService mock declaration and any related beforeEach/arrange code accordingly.
338-340: ⚡ Quick winConsider strengthening the assertion.
The test only verifies that
createArtistTagFetcheris called withexpect.any(Function), which doesn't confirm the function is the correct Spotify genre fallback. Consider either:
- Capturing and invoking the function argument to verify it calls the Spotify API
- Adding a comment explaining that the function's behavior is tested elsewhere
🧪 Option 1: Verify the function behavior
+ const { getArtistGenres } = require('../../../spotify') + await replenishQueue(queue) - expect(createArtistTagFetcher).toHaveBeenCalledWith( - expect.any(Function), - ) + const callArgs = createArtistTagFetcher.mock.calls[0] + expect(callArgs[0]).toBeInstanceOf(Function) + + // Verify the fallback function invokes Spotify API + await callArgs[0]('Test Artist') + expect(getArtistGenres).toHaveBeenCalledWith('Test Artist', 'test-token')🤖 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/utils/music/autoplay/replenisher.spec.ts` around lines 338 - 340, The test's assertion that createArtistTagFetcher was called with expect.any(Function) is weak; instead capture the actual callback argument from createArtistTagFetcher's mock (e.g., const fallback = createArtistTagFetcher.mock.calls[0][0]) and invoke it with a representative artist id or object, then assert the Spotify API mock (the Spotify genre fallback function) was called with the expected params; update the assertion to verify the captured function delegates to the Spotify call, or if you choose not to invoke it here add a concise comment referencing the separate test that covers the Spotify genre fallback behavior.
🤖 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.
Nitpick comments:
In `@packages/bot/src/utils/music/autoplay/artistTagCache.spec.ts`:
- Around line 147-160: Add a case-insensitive caching test for the Spotify
fallback path: using the existing mocks (getArtistTopTags and spotifyFallback)
set getArtistTopTags.mockResolvedValue([]) and
spotifyFallback.mockResolvedValue([...]), create the fetcher via
createArtistTagFetcher(spotifyFallback), call fetcher('Artist') then
fetcher('artist'), and assert spotifyFallback was called only once and both
results are equal; reference createArtistTagFetcher, spotifyFallback,
getArtistTopTags, and fetcher to locate where to add the test.
- Line 12: The test currently uses require('../../../lastfm') to get
getArtistTopTags after ES module imports; switch to a dynamic import inside the
beforeEach so the module is loaded asynchronously and consistently with other
ES6 imports: replace the require call with await import('../../../lastfm')
inside the beforeEach and extract getArtistTopTags from the imported module (or
reassign the module variable), ensuring any mocks are applied before the module
is used in tests (refer to getArtistTopTags and beforeEach).
In `@packages/bot/src/utils/music/autoplay/replenisher.spec.ts`:
- Line 20: The mock property getByDiscordId on the spotifyLinkService mock in
replenisher.spec.ts is not used by any test; either remove getByDiscordId from
the spotifyLinkService mock object to keep the test surface minimal, or if
future tests will call it, add a focused test that uses
spotifyLinkService.getByDiscordId (mock its return value via jest.fn()) and
assert expected behavior—update the spotifyLinkService mock declaration and any
related beforeEach/arrange code accordingly.
- Around line 338-340: The test's assertion that createArtistTagFetcher was
called with expect.any(Function) is weak; instead capture the actual callback
argument from createArtistTagFetcher's mock (e.g., const fallback =
createArtistTagFetcher.mock.calls[0][0]) and invoke it with a representative
artist id or object, then assert the Spotify API mock (the Spotify genre
fallback function) was called with the expected params; update the assertion to
verify the captured function delegates to the Spotify call, or if you choose not
to invoke it here add a concise comment referencing the separate test that
covers the Spotify genre fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54fb0ca0-a20b-4f78-a1b5-880553f54b70
📒 Files selected for processing (9)
packages/bot/src/handlers/player/soundcloudMatcher.tspackages/bot/src/handlers/player/streamBridge.spec.tspackages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/utils/music/autoplay/artistTagCache.spec.tspackages/bot/src/utils/music/autoplay/artistTagCache.tspackages/bot/src/utils/music/autoplay/replenisher.spec.tspackages/bot/src/utils/music/autoplay/replenisher.tspackages/bot/src/utils/music/queueManipulation.spec.ts
✅ Files skipped from review due to trivial changes (2)
- packages/bot/src/handlers/player/streamBridge.spec.ts
- packages/bot/src/utils/music/queueManipulation.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bot/src/handlers/player/soundcloudMatcher.ts
- packages/bot/src/utils/music/autoplay/replenisher.ts
- packages/bot/src/utils/music/autoplay/artistTagCache.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 context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use theisPrisma*Error()helper functions to check for specific Prisma error types (e.g.,isPrismaForeignKeyError,isPrismaUniqueConstraintError) instead of manually checking error codes
Use Prisma's$transaction()method to ensure database operations are atomic and avoid partial updates when multiple related tables are modified
Always useselectorincludein Prisma queries to explicitly specify which fields to return, avoiding unnecessary data transfer
For Redis operations, use connection pooling and implement exponential backoff retry logic for transient failures
Always uselogAndRethrow()orlogAndSwallow()utilities when handling errors to ensure errors are logged with context before propagating or suppressing
Files:
packages/bot/src/utils/music/autoplay/replenisher.spec.tspackages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/utils/music/autoplay/artistTagCache.spec.tspackages/bot/src/lastfm/lastFmApi.ts
packages/bot/src/utils/music/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement error recovery in music queue operations by using
rescueQueue()when tracks fail to load or dependencies are unavailable
Files:
packages/bot/src/utils/music/autoplay/replenisher.spec.tspackages/bot/src/utils/music/autoplay/artistTagCache.spec.ts
packages/{bot,backend,shared}/src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
packages/{bot,backend,shared}/src/**/*.ts: For feature toggles, check both global and guild-specific toggles usingFeatureToggleService.isEnabledForGuild()rather than checking them separately
Use branded types (e.g.,GuildId,UserId,ChannelId) for Discord IDs throughout the codebase to prevent type-level ID confusion
Files:
packages/bot/src/utils/music/autoplay/replenisher.spec.tspackages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/utils/music/autoplay/artistTagCache.spec.tspackages/bot/src/lastfm/lastFmApi.ts
packages/bot/src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When building Discord embeds, use
EmbedBuilderService.createTemplate()orEmbedBuilderService.getTemplate()instead of constructing embeds directly
Files:
packages/bot/src/utils/music/autoplay/replenisher.spec.tspackages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/utils/music/autoplay/artistTagCache.spec.tspackages/bot/src/lastfm/lastFmApi.ts
packages/bot/src/utils/music/autoplay/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For autoplay candidate selection, use genre-family-based penalties via
getGenreFamilies()to avoid monotonous genre clustering
Files:
packages/bot/src/utils/music/autoplay/replenisher.spec.tspackages/bot/src/utils/music/autoplay/artistTagCache.spec.ts
packages/bot/src/{spotify,utils/music}/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Extract Spotify track IDs using
extractSpotifyTrackId()before passing to Spotify API calls to prevent malformed requests
Files:
packages/bot/src/utils/music/autoplay/replenisher.spec.tspackages/bot/src/utils/music/autoplay/artistTagCache.spec.ts
🔇 Additional comments (4)
packages/bot/src/lastfm/lastFmApi.ts (2)
14-23: Good addition of a typed session-expiration error.This gives callers a stable way to branch re-auth logic instead of parsing strings.
97-99: Nice propagation of error code 9 into typed detection.Mapping Last.fm
error === 9toLastFmSessionExpiredErrorand recognizing it inisLastFmInvalidSessionErrorcloses the brittle message-matching gap.Also applies to: 348-348
packages/bot/src/lastfm/lastFmApi.spec.ts (1)
25-25: Strong coverage for the new typed-error path.These tests validate constructor behavior and protect the new
isLastFmInvalidSessionErrorbranch from regressions.Also applies to: 237-258, 545-554
packages/bot/src/utils/music/autoplay/artistTagCache.spec.ts (1)
163-188: LGTM!The
hasGenreTagtest suite provides comprehensive coverage of edge cases (empty arrays, case-insensitivity, whitespace trimming) and the core matching logic.
|
…#820) * fix(autoplay): block Spanish gospel tracks when Last.fm is not linked When Last.fm is unlinked the artist-tag fetcher returned [] for every artist, silently bypassing the cross-locale Spanish veto. Spanish gospel artists like Marcos Witt, Alex Zurdo, and Christine D'Clario carry no Spanish diacritics in their names, so the text-only heuristic also missed them. Three-layer fix: • artistTagCache: accept an optional Spotify genre fallback so the veto fires on Spotify genre strings ('latin worship', 'musica cristiana', 'latin gospel') even without Last.fm • replenisher: fetch a single Spotify token early and thread it through createArtistTagFetcher, covering every candidate collector in the pass • candidateFallback: wire genreContext (including getArtistTags) into collectBroadFallbackCandidates, the only path that previously received no genre context at all • languageHeuristics: add 'latin worship', 'ccm en español', 'spanish ccm' to SPANISH_GENRE_MARKERS and 11 gospel-specific tokens to SPANISH_DISTINCT_TOKENS * test(autoplay): improve coverage for SonarCloud quality gate Add comprehensive test suite for artistTagCache utility, covering cache behavior, Spotify fallback logic, and concurrent request coalescing. Add tests for new lastFmApi functions: parseArtists (splits featured artists from primary artist) and LastFmSessionExpiredError (error class for expired session keys). Suppress SonarCloud hotspot for internal Last.fm API key in GET request URLs (not user-controlled data) with NOSONAR comments. - artistTagCache.spec.ts: 13 test cases covering cache mechanics, fallback behavior - lastFmApi tests: 12 new tests for parseArtists, 3 for LastFmSessionExpiredError - All 246 affected tests pass * test(replenisher): cover Spotify genre fallback path in createArtistTagFetcher When getValidAccessToken returns a token, the code passes a Spotify genre-fetching function to createArtistTagFetcher instead of undefined. Add a test that verifies this branch is exercised to satisfy SonarCloud new-code coverage requirement. * fix(test): add getByDiscordId to spotifyLinkService mock in replenisher spec The coverage test triggers the Spotify path which calls getUserSpotifySeeds, which calls spotifyLinkService.getByDiscordId. Without this in the mock the test throws TypeError at runtime. * test(replenisher): fix Spotify genre fallback test by setting requestedBy on mock track The test for createArtistTagFetcher Spotify path requires currentTrack.requestedBy.id to be truthy so the token fetch branch is entered. * test(lastfm): add getTrackMetadata coverage for PR #820 Adds 9 tests covering success path, cache hit, !response.ok, data.error, missing track, empty artist/title, network error, and metadata without album — pushing new code coverage above 80%. * fix(lastfm): make isLastFmInvalidSessionError recognize LastFmSessionExpiredError The new LastFmSessionExpiredError class is thrown by signedPost on error code 9, but its default message ('Last.fm session key has expired (error code 9)') doesn't match isLastFmInvalidSessionError's patterns ('error':9 regex, ' 9 - ' substring, 'invalid session key' substring), so the typed error was being silently dropped by the detector — exactly the path callers rely on for re-auth. Add an instanceof short-circuit at the top of isLastFmInvalidSessionError. Add a regression test covering both the default message and a custom-message variant. Addresses Greptile feedback on PR #820. * fix: address CodeRabbit minor issues on PR #820 1. soundcloudMatcher.ts: use type-safe error message extraction ((err as Error).message produces 'undefined' for non-Error throws; use 'err instanceof Error ? err.message : String(err)' instead) 2. streamBridge.spec.ts: protect against fake-timer leak on assertion failure by moving jest.useRealTimers() into afterEach inside the process-lifecycle describe block --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Promote [Unreleased] entries into the v2.10.0 section, bump root + workspace versions from 2.9.0 to 2.10.0, and update the lockfile. Release highlights: - Spotify 429 retry hardening (#808) - Last.fm canonical metadata + multi-artist scrobble fix (#821) - Autoplay Spanish-gospel-block + sertanejo prioritization series (#817-#820, #827, #829, #830) - Review-tools revamp: Claude review + Danger + chilled CodeRabbit via org-level reusable workflows (#838) - Coverage threshold pinned for phase-2 test cleanup (#835) - CI extended to release/** branches (#816)
This branch was successfully deployed
1 participant
Summary
Root cause:
collectBroadFallbackCandidateswas the only candidate collection path that calledcalculateRecommendationScorewith nogenreContext, completely bypassing the cross-locale Spanish veto. Additionally, when Last.fm is not linked the artist-tag fetcher always returned[], silencing the veto for all paths — including Spanish gospel artists like Marcos Witt, Alex Zurdo, and Christine D'Clario whose names carry no Spanish diacritics.Three-layer fix:
artistTagCache.ts:createArtistTagFetchernow accepts an optionalspotifyFallback. When Last.fm returns[], the fetcher calls the Spotify genres API and caches the result, enabling the veto on genre strings like'latin worship','musica cristiana','latin gospel'.replenisher.ts: Fetches a Spotify token once early in_replenishQueueand threads it throughcreateArtistTagFetcher, so every candidate collector in the pass (seed loop, Spotify recs, Last.fm, broad fallback) benefits automatically.candidateFallback.ts:collectBroadFallbackCandidatesnow accepts and uses agenreContextparameter (includinggetArtistTags), closing the last unprotected path.languageHeuristics.ts: Added'latin worship','ccm en español','spanish ccm'toSPANISH_GENRE_MARKERSand 11 gospel-specific Spanish tokens (eres,nuestro/a,siervo/a,digno,fuego,cielos) toSPANISH_DISTINCT_TOKENS.Test plan
candidateFallback.spec.tscovering: tag passthrough to scorer,genreContextoverride,-Infinityhard-reject for Spanish gospel tracks, full genreContext propagationartistTagCache.tsupdated tests pass (Spotify fallback coalescing,Promise.resolvenormalization)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
artistTagCachegains aspotifyFallbackpath when Last.fm returns[],replenisherfetches a Spotify token once per pass and threads it through all collectors, andcollectBroadFallbackCandidatesnow receives a fullgenreContext.lastFmApi.tsaddsLastFmSessionExpiredError(thrown on error-code 9 insignedPost),getTrackMetadatafor scrobble enrichment, andparseArtistsfor splitting featured artists — butupdateNowPlayingwas updated to useparseArtistswhilescrobblestill usesnormalizeLastFmArtist, creating a P1 artist-string mismatch for featured-artist tracks.parseArtists,LastFmSessionExpiredError, Spotify fallback coalescing, and the extragetValidAccessTokenmock in the multi-user blend integration test.Confidence Score: 4/5
Safe to merge with one fix:
scrobblemust useparseArtiststo stay consistent with the updatedupdateNowPlaying.A single isolated P1 —
scrobblestill sends the rawnormalizeLastFmArtiststring whileupdateNowPlayingnow sendsparseArtists().primary. For any track whose artist field contains a feat./& separator, the two Last.fm calls will submit different artist values, breaking track matching in user play history. No P0s; no security concerns; all other changes are well-structured and well-tested.packages/bot/src/lastfm/lastFmApi.ts —
scrobbleartist normalization needs updating to matchupdateNowPlaying.Important Files Changed
LastFmSessionExpiredError,LastFmTrackMetadata,parseArtists,getTrackMetadata; throws typed error on error-code 9 insignedPost;updateNowPlayingnow usesparseArtistswhilescrobblestill usesnormalizeLastFmArtist, creating a P1 mismatch for featured-artist strings.spotifyFallbacktocreateArtistTagFetcher; fallback is called and cached when Last.fm returns[], with proper error handling and non-array guard.spotifyFallbacktocreateArtistTagFetcher, propagating genre-aware vetoing to all candidate collectors including the broad fallback path.parseArtists(covering separators, whitespace, empty string) andLastFmSessionExpiredErrorconstruction; all cases look correct.mockResolvedValueOnce(null)for the newgetValidAccessTokencall introduced in_replenishQueuefor the Spotify artist-tag fallback token.Sequence Diagram
sequenceDiagram participant R as replenisher participant SLS as spotifyLinkService participant ATF as createArtistTagFetcher participant LFM as getArtistTopTags (Last.fm) participant SPT as getArtistGenres (Spotify) R->>SLS: getValidAccessToken(userId) SLS-->>R: spotifyToken (or null) R->>ATF: createArtistTagFetcher(spotifyFallback?) Note over ATF: per-pass in-memory cache R->>ATF: getArtistTags(artist) ATF->>LFM: getArtistTopTags(artist) LFM-->>ATF: tags[] alt "tags.length === 0 && spotifyFallback" ATF->>SPT: getArtistGenres(token, artist) SPT-->>ATF: genres[] ATF-->>R: genres[] (cached) else tags present ATF-->>R: tags[] (cached) end R->>R: "candidateGenreContext = { getArtistTags, currentTrackTags, sessionGenreFamilies }" R->>R: collectRecommendationCandidates(... candidateGenreContext) R->>R: collectLastFmCandidates(... candidateGenreContext) R->>R: collectBroadFallbackCandidates(... candidateGenreContext)Comments Outside Diff (1)
packages/bot/src/lastfm/lastFmApi.ts, line 220 (link)scrobbleartist diverges fromupdateNowPlayingafterparseArtistsrefactorupdateNowPlayingnow callsparseArtists(artist).primary, strippingfeat./ft./&collaborators from the artist field.scrobblestill callsnormalizeLastFmArtist(artist), which only strips comma/slash-delimited prefixes. For an artist string like"Drake feat. Rihanna",updateNowPlayingsendsartist = "Drake"whilescrobblesendsartist = "Drake feat. Rihanna". Last.fm matchestrack.updateNowPlayingandtrack.scrobbleentries by artist+track; mismatched artist strings break that matching and can result in unscrobbled tracks or duplicate/orphaned play history.Reviews (4): Last reviewed commit: "Merge branch 'release/v2.10.0' into fix/..." | Re-trigger Greptile