Repository navigation
fix(lastfm): resolve canonical metadata for album art and multi-artist scrobbles - #821
Merged
Merged
Conversation
These files contained only toHaveBeenCalled assertions — no behavioral assertions on return values, state changes, or reply content. TypeScript enforces the same routing contracts at compile time. ~455 tests removed (3339 → 2884), zero regression risk. All tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removed individual it() blocks where EVERY expect() call was toHaveBeenCalled/toHaveBeenCalledWith with zero assertions on return values, state, reply content, or thrown errors. Kept all blocks with behavioral assertions. Files trimmed: queueManipulation, trackHandlers, autoplay, case. 455 pure-delegation tests removed; 2777 behavioral tests remain. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removed pure-delegation test blocks from giveaway, level, and music specs. Retained all behavioral assertions on reply content, error messages, and conditional logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…se 4) Deleted 2 test blocks that only asserted toHaveBeenCalled() with no assertions on return values, state, or reply content. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 (4)
📝 WalkthroughWalkthroughAdds Last.fm track metadata API and forwards it in now-playing/scrobble handlers, introduces fallback debug logging, adds an autoplay pipeline integration test, updates exports/tests, removes many Jest specs, and adds documentation. ChangesLast.fm metadata plumbing and test updates
Sequence Diagram(s)sequenceDiagram
participant Bot
participant NowPlaying
participant LastFm
Bot->>NowPlaying: track context
NowPlaying->>LastFm: getTrackMetadata(artist,title)
LastFm-->>NowPlaying: metadata|null
NowPlaying->>LastFm: updateNowPlaying/scrobble(..., metadata?)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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.
🧹 Nitpick comments (1)
packages/bot/src/utils/music/queueManipulation.spec.ts (1)
1721-1872: 💤 Low valueAdd a
addedTrackdefined-guard before asserting on metadata for clearer failure diagnostics.The four new tests in this
describeblock (skipped-before penalty,completed-before boost,long-track penalty,spotify preferred) destructure the first added track and immediately assert onaddedTrack?.metadata?.recommendationReason. If a future scoring change causes the candidate to be filtered out instead of penalised,queue.addTrack.mock.calls[0]?.[0]becomesundefinedand the matcher will fail with a confusing message aboutundefinednot containing the substring rather than telling you the track wasn't queued at all. The pattern used elsewhere in this file (e.g. lines 455-460, 490-495, 527-532, 624-629, 665-670) explicitly assertsexpect(addedTrack).toBeDefined()first — worth aligning the new cases for consistency and cleaner diagnostics.♻️ Example tightening for the dislike-key case (apply the same pattern to the other three)
await replenishQueue(queue as unknown as GuildQueue) const addedTrack = queue.addTrack.mock.calls[0]?.[0] as Track + expect(addedTrack).toBeDefined() expect(addedTrack?.metadata?.recommendationReason).toContain( 'skipped before', )🤖 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/queueManipulation.spec.ts` around lines 1721 - 1872, Each of the four tests that reads const addedTrack = queue.addTrack.mock.calls[0]?.[0] (tests: "applies skipped-before penalty", "applies completed-before boost", "applies long-track penalty", "boosts candidates when both current and candidate are from spotify") should first assert that addedTrack is defined before inspecting metadata; add expect(addedTrack).toBeDefined() immediately after retrieving addedTrack in each test (the code that calls replenishQueue and reads queue.addTrack.mock.calls should remain unchanged) so failures clearly indicate the candidate wasn't queued rather than producing a confusing undefined-containing-substring error.
🤖 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/queueManipulation.spec.ts`:
- Around line 1721-1872: Each of the four tests that reads const addedTrack =
queue.addTrack.mock.calls[0]?.[0] (tests: "applies skipped-before penalty",
"applies completed-before boost", "applies long-track penalty", "boosts
candidates when both current and candidate are from spotify") should first
assert that addedTrack is defined before inspecting metadata; add
expect(addedTrack).toBeDefined() immediately after retrieving addedTrack in each
test (the code that calls replenishQueue and reads queue.addTrack.mock.calls
should remain unchanged) so failures clearly indicate the candidate wasn't
queued rather than producing a confusing undefined-containing-substring error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74b19403-865b-4152-bb27-f1a1ee47dfa2
📒 Files selected for processing (61)
ads-math-10-per-month.mdpackages/backend/tests/unit/middleware/asyncHandler.test.tspackages/backend/tests/unit/routes/index.test.tspackages/backend/tests/unit/routes/musicSSEBridge.test.tspackages/bot/src/config/constants.spec.tspackages/bot/src/functions/general/commands/giveaway.spec.tspackages/bot/src/functions/general/commands/level.spec.tspackages/bot/src/functions/general/commands/starboard.spec.tspackages/bot/src/functions/general/commands/version.spec.tspackages/bot/src/functions/general/handlers/twitchHandlers.spec.tspackages/bot/src/functions/management/handlers/automessageHandlers.spec.tspackages/bot/src/functions/moderation/commands/case.spec.tspackages/bot/src/functions/moderation/commands/unban.spec.tspackages/bot/src/functions/moderation/commands/unmute.spec.tspackages/bot/src/functions/moderation/commands/warn.spec.tspackages/bot/src/functions/music/commands/album.spec.tspackages/bot/src/functions/music/commands/artist.spec.tspackages/bot/src/functions/music/commands/autoplay.spec.tspackages/bot/src/functions/music/commands/autoplay/artistHandlers.spec.tspackages/bot/src/functions/music/commands/autoplay/settingsHandlers.spec.tspackages/bot/src/functions/music/commands/clear.spec.tspackages/bot/src/functions/music/commands/effects.spec.tspackages/bot/src/functions/music/commands/leavecleanup.spec.tspackages/bot/src/functions/music/commands/lyrics.spec.tspackages/bot/src/functions/music/commands/music.spec.tspackages/bot/src/functions/music/commands/pause.spec.tspackages/bot/src/functions/music/commands/play/index.spec.tspackages/bot/src/functions/music/commands/playlist.spec.tspackages/bot/src/functions/music/commands/queue/index.spec.tspackages/bot/src/functions/music/commands/queueResolverWiring.spec.tspackages/bot/src/functions/music/commands/recommendation/handlers/feedbackHandler.spec.tspackages/bot/src/functions/music/commands/recommendation/index.spec.tspackages/bot/src/functions/music/commands/remove.spec.tspackages/bot/src/functions/music/commands/replay.spec.tspackages/bot/src/functions/music/commands/seek.spec.tspackages/bot/src/functions/music/commands/shuffle.spec.tspackages/bot/src/functions/music/commands/skip.spec.tspackages/bot/src/functions/music/commands/skipto.spec.tspackages/bot/src/functions/music/commands/songinfo.spec.tspackages/bot/src/functions/music/commands/stop.spec.tspackages/bot/src/functions/music/handlers/play/handlePlay.spec.tspackages/bot/src/handlers/auditHandler.spec.tspackages/bot/src/handlers/externalScrobbler.spec.tspackages/bot/src/handlers/messageHandler.spec.tspackages/bot/src/handlers/musicButtonHandler.spec.tspackages/bot/src/handlers/player/queueExhaustion.spec.tspackages/bot/src/handlers/player/trackHandlers.spec.tspackages/bot/src/handlers/reactionHandler.spec.tspackages/bot/src/handlers/webMusic/index.spec.tspackages/bot/src/index.spec.tspackages/bot/src/register.spec.tspackages/bot/src/scripts/sentryTestCli.spec.tspackages/bot/src/twitch/index.spec.tspackages/bot/src/utils/general/deferredInteractionReply.spec.tspackages/bot/src/utils/monitoring/metrics.spec.tspackages/bot/src/utils/music/collaborativePlaylist.spec.tspackages/bot/src/utils/music/idleDisconnect.spec.tspackages/bot/src/utils/music/queueManipulation.spec.tspackages/bot/src/utils/music/sessionStartupRestore.spec.tspackages/bot/src/utils/music/watchdog.rejoin.spec.tspackages/shared/src/utils/errorHandler.spec.ts
💤 Files with no reviewable changes (59)
- packages/bot/src/functions/music/commands/queueResolverWiring.spec.ts
- packages/bot/src/functions/music/commands/remove.spec.ts
- packages/bot/src/functions/music/commands/music.spec.ts
- packages/bot/src/functions/music/commands/recommendation/handlers/feedbackHandler.spec.ts
- packages/bot/src/functions/music/commands/shuffle.spec.ts
- packages/bot/src/functions/general/commands/starboard.spec.ts
- packages/backend/tests/unit/routes/index.test.ts
- packages/bot/src/functions/music/commands/leavecleanup.spec.ts
- packages/bot/src/functions/music/commands/autoplay/artistHandlers.spec.ts
- packages/bot/src/functions/general/handlers/twitchHandlers.spec.ts
- packages/bot/src/config/constants.spec.ts
- packages/bot/src/utils/music/collaborativePlaylist.spec.ts
- packages/bot/src/functions/moderation/commands/unmute.spec.ts
- packages/bot/src/functions/music/commands/songinfo.spec.ts
- packages/bot/src/utils/monitoring/metrics.spec.ts
- packages/bot/src/scripts/sentryTestCli.spec.ts
- packages/bot/src/functions/moderation/commands/case.spec.ts
- packages/bot/src/functions/moderation/commands/warn.spec.ts
- packages/backend/tests/unit/middleware/asyncHandler.test.ts
- packages/bot/src/utils/general/deferredInteractionReply.spec.ts
- packages/shared/src/utils/errorHandler.spec.ts
- packages/bot/src/functions/music/commands/play/index.spec.ts
- packages/bot/src/functions/music/commands/playlist.spec.ts
- packages/bot/src/functions/music/commands/seek.spec.ts
- packages/bot/src/handlers/auditHandler.spec.ts
- packages/bot/src/handlers/player/queueExhaustion.spec.ts
- packages/bot/src/functions/music/commands/stop.spec.ts
- packages/backend/tests/unit/routes/musicSSEBridge.test.ts
- packages/bot/src/functions/management/handlers/automessageHandlers.spec.ts
- packages/bot/src/functions/music/commands/clear.spec.ts
- packages/bot/src/functions/moderation/commands/unban.spec.ts
- packages/bot/src/utils/music/sessionStartupRestore.spec.ts
- packages/bot/src/functions/general/commands/level.spec.ts
- packages/bot/src/register.spec.ts
- packages/bot/src/functions/music/commands/skipto.spec.ts
- packages/bot/src/functions/music/commands/autoplay/settingsHandlers.spec.ts
- packages/bot/src/functions/music/commands/replay.spec.ts
- packages/bot/src/handlers/player/trackHandlers.spec.ts
- packages/bot/src/functions/music/commands/pause.spec.ts
- packages/bot/src/handlers/webMusic/index.spec.ts
- packages/bot/src/functions/music/commands/album.spec.ts
- packages/bot/src/handlers/musicButtonHandler.spec.ts
- packages/bot/src/twitch/index.spec.ts
- packages/bot/src/functions/music/commands/artist.spec.ts
- packages/bot/src/handlers/externalScrobbler.spec.ts
- packages/bot/src/utils/music/watchdog.rejoin.spec.ts
- packages/bot/src/functions/music/handlers/play/handlePlay.spec.ts
- packages/bot/src/functions/music/commands/skip.spec.ts
- packages/bot/src/functions/general/commands/version.spec.ts
- packages/bot/src/utils/music/idleDisconnect.spec.ts
- packages/bot/src/functions/music/commands/lyrics.spec.ts
- packages/bot/src/index.spec.ts
- packages/bot/src/functions/music/commands/queue/index.spec.ts
- packages/bot/src/functions/general/commands/giveaway.spec.ts
- packages/bot/src/functions/music/commands/effects.spec.ts
- packages/bot/src/functions/music/commands/autoplay.spec.ts
- packages/bot/src/handlers/reactionHandler.spec.ts
- packages/bot/src/functions/music/commands/recommendation/index.spec.ts
- packages/bot/src/handlers/messageHandler.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). (3)
- GitHub Check: Greptile Review
- GitHub Check: Quality Gates
- GitHub Check: SonarCloud Scan
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/queueManipulation.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/queueManipulation.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/queueManipulation.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/utils/music/queueManipulation.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/queueManipulation.spec.ts
🔇 Additional comments (2)
ads-math-10-per-month.md (1)
1-43: This file does not match the PR objectives.The PR description explicitly states this is a test cleanup effort that "only .spec.ts files modified/deleted" with "Source code files unchanged." However, this file is a new Markdown document about advertising economics, completely unrelated to the test suite redesign work described in the PR objectives.
This suggests either:
- The file was accidentally committed to the wrong branch/PR
- The PR objectives/description are incorrect or outdated
- Multiple unrelated changes were bundled together
Please verify that this file belongs in this PR, or move it to a separate PR focused on documentation/marketing strategy.
packages/bot/src/utils/music/queueManipulation.spec.ts (1)
1-2810: Test-only PR scope and behavioral retention look sound.The reshaped Last.fm failure test and the four new VC-blend scoring tests (implicit dislike/like, long-track penalty, Spotify-preferred) all assert on
recommendationReasoncontent rather than just mock-call counts, which aligns with the PR's stated retention criteria. No source files are touched in this file's scope, no dynamicRegExpis introduced, and the broader cleanup of pure-delegation cases is consistent with the suite reduction described in the objectives.
- Add pipeline.integration.spec.ts: end-to-end test of collectRecommendationCandidates with real candidateScorer, diversitySelector, languageHeuristics. Proves the cross-locale veto (dominantLocale: null + Spanish gospel → -Infinity → dropped). - Delete replenisher.spec.ts (8 tests): pure coordinator — all collaborators mocked, only tested call routing. - Delete recommendations.spec.ts (16 tests): pure coordinator with no algorithmic logic. - Trim candidateCollector.spec.ts: remove 8 coordinator-level collectRecommendationCandidates tests (covered by integration spec); keep 10 unit tests for shouldIncludeCandidate + upsertScoredCandidate. - Trim counters.spec.ts: remove log assertions and coordinator-call verifications; keep 11 behavioral tests (24 → 11). - Trim stats.spec.ts: remove log assertions and redundant paths; keep 11 behavioral tests with boundary cases (21 → 11). Net: -46 tests across autoplay module. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… multi-artist parsing - Add `getTrackMetadata()` that calls `track.getInfo?autocorrect=1` to resolve canonical artist/title/album/albumArtist/mbid; results cached 24 h (5000-entry map) to avoid redundant API calls. - Add `parseArtists()` to split multi-artist strings (feat./ft./&/×/x/ vs./with) into primary + featured[]; `updateNowPlaying` and `scrobble` now send only the primary artist, matching Last.fm's expectation. - Fix `FEAT_ARTIST_SEPARATORS` regex: drop trailing `\b` on `vs\.?` so "Artist vs. Other" with a trailing dot splits correctly. - Pipe metadata (album, albumArtist, mbid) into signed POST params for both `track.updateNowPlaying` and `track.scrobble`, enabling album-art display on scrobble cards. - Update `trackNowPlaying.ts` and `externalScrobbler.ts` to fetch metadata once per track and pass it down to all API calls. - Export `getTrackMetadata`, `parseArtists`, `LastFmTrackMetadata` from the `lastfm` barrel. - Add 30 new unit tests covering `parseArtists` (all separators, edge cases) and `getTrackMetadata` (success, caching, error paths). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
LucasSantana-Dev
changed the title
- commandsHandler.spec.ts: 21 → 16 tests - service.spec.ts: 44 → 37 tests - feedbackService.spec.ts: 38 → 34 tests - queueManipulation.spec.ts: 90 tests (no changes, all verify behavior) - queueStateManager.spec.ts: 60 → 59 tests Removed 17 tests total that only verified mock calls or Redis implementation details without testing actual behavior. Kept all tests that verify return values, state mutations, error handling, and observable side effects. All 2725 tests passing. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add afterEach hook to getTrackMetadata describe block to guarantee jest.useRealTimers() cleanup (prevents fake timer leaks if assertions fail) - Remove inline jest.useRealTimers() from TTL test body - Fix unicode test to use consistent .toEqual() assertion pattern matching all other parseArtists tests
Deleted 5 tests that had only log or mock-call assertions with no behavioral coverage: - 'logs error when guild delete cleanup fails' (only asserted mock.toHaveBeenCalled) - 'logs error when channel cleanup fails' (only asserted errorLogMock call) - 'logs when client is ready' (only asserted infoLogMock call) - 'logs command count when ready' (only asserted debugLogMock call) - 'logs error if ai dev toolkit fails to start' (only asserted errorLogMock call) Kept 17 tests with behavioral assertions. All remaining tests verify: - Error handling + proper error log messages - Autocomplete response behavior and edge cases - Button routing logic - Guild/channel cleanup execution - Async toolkit service startup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…berHandler Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… watchdog 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.
- Replace S5852 NOSONAR on FEAT_ARTIST_SEPARATORS with bounded
whitespace quantifiers (\s{0,4} / \s{1,4}); regex now provably
linear, eliminating the SonarCloud security hotspot.
- getTrackMetadata: extract primary artist via parseArtists() before
calling track.getInfo. Last.fm's autocorrect does not split
collaboration strings, so 'Drake feat. Rihanna' previously returned
error 6 and broke album-art resolution — exactly the regression the
PR was meant to fix. New spec covers the case.
- getArtistTopTags: restore logAndWarn (had been downgraded to
logAndSwallow), so autoplay tag-fetch failures stay observable in
production logs.
- Remove ads-math-10-per-month.md — unrelated marketing scratch file
accidentally included in the spec-redesign branch.
Refs PR #821 (CodeRabbit summary, Greptile P1 ×3, SonarCloud S5852)
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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/bot/src/lastfm/lastFmApi.ts (2)
259-303: 💤 Low valueOptional: prefer canonical
metadata.artistfor the outbound artist param when available.Both
updateNowPlayingandscrobbleuseparseArtists(artist).primaryeven when a resolvedmetadatais passed in. SincegetTrackMetadataalready calls Last.fm withautocorrect=1,metadata.artistis the canonical name (correctly cased, deduped), so callers that go through the metadata path could benefit from using it directly and falling back toparseArtists(...).primaryonly when no metadata is available. Same idea applies tometadata.titlefor thetrackparam.This is a behavior-preserving refinement, not a bug — flagging for consideration since you've already gone through the trouble of resolving canonical fields for
album/albumArtist/mbid.🤖 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/lastfm/lastFmApi.ts` around lines 259 - 303, Both updateNowPlaying and scrobble currently set artist/track from parseArtists(...).primary and normalizeLastFmTitle(...) even when canonical metadata is available; update each function to prefer metadata.artist (for artist) and metadata.title (for track) when metadata exists and is non-empty, falling back to parseArtists(artist).primary and normalizeLastFmTitle(track) only when metadata fields are absent, and keep the rest of the param logic (duration, album, albumArtist, mbid, signedPost) unchanged.
144-163: ⚡ Quick winAdd the same blank-input guard to
getTrackMetadata.
updateNowPlaying/scrobblewere updated to early-return on blank artist/track (lines 267, 290), butgetTrackMetadatahas no such guard. With an empty or whitespace-only argument, the cache key still becomes"::"-shaped, anin-flightslot is created, and atrack.getInforequest is sent to Last.fm with emptyartist=/track=. Trim and bail before the fetch to keep behavior consistent and avoid wasted requests/cache pollution.♻️ Proposed guard
export async function getTrackMetadata( artist: string, title: string, ): Promise<LastFmTrackMetadata | null> { const config = getApiConfig() if (!config) return null - const key = `${artist.toLowerCase()}::${title.toLowerCase()}` + const trimmedArtist = artist?.trim() + const trimmedTitle = title?.trim() + if (!trimmedArtist || !trimmedTitle) return null + const key = `${trimmedArtist.toLowerCase()}::${trimmedTitle.toLowerCase()}`…and use
trimmedArtist/trimmedTitlein theencodeURIComponentcalls below.🤖 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/lastfm/lastFmApi.ts` around lines 144 - 163, getTrackMetadata should trim and guard against blank inputs: at the start of getTrackMetadata create trimmedArtist and trimmedTitle (artist.trim(), title.trim()), if either is empty return null immediately, then use trimmedArtist/trimmedTitle when building the cache/in-flight key (instead of artist/title), when checking/setting TRACK_METADATA_CACHE and TRACK_METADATA_IN_FLIGHT, and when calling encodeURIComponent in the fetch URL so you avoid creating a "::" key, avoid polluting TRACK_METADATA_IN_FLIGHT, and prevent unnecessary Last.fm requests.packages/bot/src/lastfm/lastFmApi.spec.ts (1)
924-1125: 💤 Low valueLGTM on
getTrackMetadatacoverage; consider clearing module cache between tests.
TRACK_METADATA_CACHEandTRACK_METADATA_IN_FLIGHTare module-level Maps that persist across tests. Current tests avoid collisions by using distinct artist/title combinations, but this is fragile — adding a new test (or reordering) with a colliding key would silently get cached results from a prior test. Consider exporting a__resetMetadataCacheForTests()helper and calling it inbeforeEachfor thegetTrackMetadatadescribe block. Same applies to the existingARTIST_TAG_CACHEalready exercised in this file.🤖 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/lastfm/lastFmApi.spec.ts` around lines 924 - 1125, Tests for getTrackMetadata rely on module-level Maps TRACK_METADATA_CACHE and TRACK_METADATA_IN_FLIGHT that persist across test cases; export a test-only helper named __resetMetadataCacheForTests (or similar) from the module that clears both TRACK_METADATA_CACHE and TRACK_METADATA_IN_FLIGHT, then call this helper in the describe('getTrackMetadata') beforeEach so each test starts with a clean cache; reference the getTrackMetadata function and the TRACK_METADATA_CACHE / TRACK_METADATA_IN_FLIGHT symbols when adding the export and the beforeEach call.packages/bot/src/handlers/player/trackNowPlaying.spec.ts (1)
507-523: 💤 Low valueTest fixture shape doesn't match
LastFmTrackMetadata.
testMetadata = { mbid: 'test-mbid', listeners: 1000 }is missing the requiredartist,title,album,albumArtist, anddurationfields, andlistenersis not part ofLastFmTrackMetadata. The tests pass because the production code only readsmetadata?.mbidetc. with optional chaining and the mocks are loosely typed, but the fixture is misleading and won't catch regressions if the type tightens. Recommend constructing a fixture that matches the real shape.♻️ Suggested fixture
- const testMetadata = { mbid: 'test-mbid', listeners: 1000 } + const testMetadata = { + artist: 'Test Artist', + title: 'Test Song', + album: 'Test Album', + albumArtist: 'Test Artist', + mbid: 'test-mbid', + duration: 225000, + }Also applies to: 680-697
🤖 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/handlers/player/trackNowPlaying.spec.ts` around lines 507 - 523, The test fixture testMetadata for LastFmTrackMetadata is missing required fields and contains an extra listeners key; update the fixture used in the spec so it matches the real LastFmTrackMetadata shape (include artist, title, album, albumArtist, duration, mbid if present) and remove unrelated keys like listeners; update the places referencing testMetadata (the test case in trackNowPlaying.spec.ts and the other occurrence around lines noted) so getTrackMetadataMock.mockResolvedValue(...) returns a properly shaped object, ensuring updateLastFmNowPlaying and lastFmUpdateNowPlayingMock are exercised with realistic metadata.
🤖 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/utils/music/candidateFallback.ts`:
- Around line 156-159: The suppressed fallback errors currently logged with
debugLog in candidateFallback should use the logAndSwallow utility so errors are
logged with standardized context before being swallowed; replace the
debugLog(...) in the getArtistGenres catch (where candidateTags is assigned) and
the other similar catch blocks in candidateFallback with calls to
logAndSwallow(err, { message: 'candidateFallback: getArtistGenres (Spotify
fallback) failed', data: { author: track.author } }) (and analogous contextual
messages for the other catches), then return the same [] as string[] to preserve
behavior. Ensure you import/require logAndSwallow if not present and keep the
NOSONAR comment and return behavior intact.
---
Nitpick comments:
In `@packages/bot/src/handlers/player/trackNowPlaying.spec.ts`:
- Around line 507-523: The test fixture testMetadata for LastFmTrackMetadata is
missing required fields and contains an extra listeners key; update the fixture
used in the spec so it matches the real LastFmTrackMetadata shape (include
artist, title, album, albumArtist, duration, mbid if present) and remove
unrelated keys like listeners; update the places referencing testMetadata (the
test case in trackNowPlaying.spec.ts and the other occurrence around lines
noted) so getTrackMetadataMock.mockResolvedValue(...) returns a properly shaped
object, ensuring updateLastFmNowPlaying and lastFmUpdateNowPlayingMock are
exercised with realistic metadata.
In `@packages/bot/src/lastfm/lastFmApi.spec.ts`:
- Around line 924-1125: Tests for getTrackMetadata rely on module-level Maps
TRACK_METADATA_CACHE and TRACK_METADATA_IN_FLIGHT that persist across test
cases; export a test-only helper named __resetMetadataCacheForTests (or similar)
from the module that clears both TRACK_METADATA_CACHE and
TRACK_METADATA_IN_FLIGHT, then call this helper in the
describe('getTrackMetadata') beforeEach so each test starts with a clean cache;
reference the getTrackMetadata function and the TRACK_METADATA_CACHE /
TRACK_METADATA_IN_FLIGHT symbols when adding the export and the beforeEach call.
In `@packages/bot/src/lastfm/lastFmApi.ts`:
- Around line 259-303: Both updateNowPlaying and scrobble currently set
artist/track from parseArtists(...).primary and normalizeLastFmTitle(...) even
when canonical metadata is available; update each function to prefer
metadata.artist (for artist) and metadata.title (for track) when metadata exists
and is non-empty, falling back to parseArtists(artist).primary and
normalizeLastFmTitle(track) only when metadata fields are absent, and keep the
rest of the param logic (duration, album, albumArtist, mbid, signedPost)
unchanged.
- Around line 144-163: getTrackMetadata should trim and guard against blank
inputs: at the start of getTrackMetadata create trimmedArtist and trimmedTitle
(artist.trim(), title.trim()), if either is empty return null immediately, then
use trimmedArtist/trimmedTitle when building the cache/in-flight key (instead of
artist/title), when checking/setting TRACK_METADATA_CACHE and
TRACK_METADATA_IN_FLIGHT, and when calling encodeURIComponent in the fetch URL
so you avoid creating a "::" key, avoid polluting TRACK_METADATA_IN_FLIGHT, and
prevent unnecessary Last.fm requests.
🪄 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: 70383cc7-fdfe-46bd-82de-ad1d81eafe41
📒 Files selected for processing (26)
packages/bot/TEST_MAP.mdpackages/bot/src/bot/start/initializer.spec.tspackages/bot/src/handlers/commandsHandler.spec.tspackages/bot/src/handlers/eventHandler.spec.tspackages/bot/src/handlers/externalScrobbler.spec.tspackages/bot/src/handlers/externalScrobbler.tspackages/bot/src/handlers/interactionHandler.spec.tspackages/bot/src/handlers/memberHandler.spec.tspackages/bot/src/handlers/player/errorHandlers.spec.tspackages/bot/src/handlers/player/trackHandlers.spec.tspackages/bot/src/handlers/player/trackNowPlaying.spec.tspackages/bot/src/handlers/player/trackNowPlaying.tspackages/bot/src/lastfm/index.tspackages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/services/musicRecommendation/feedbackService.spec.tspackages/bot/src/utils/general/interactionReply.spec.tspackages/bot/src/utils/music/autoplay/autoplayAudit.spec.tspackages/bot/src/utils/music/autoplay/counters.spec.tspackages/bot/src/utils/music/autoplay/pipeline.integration.spec.tspackages/bot/src/utils/music/autoplay/recommendations.spec.tspackages/bot/src/utils/music/autoplay/stats.spec.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/utils/music/queueStateManager.spec.tspackages/bot/src/utils/music/service.spec.tspackages/bot/src/utils/music/watchdog.spec.ts
💤 Files with no reviewable changes (13)
- packages/bot/src/utils/general/interactionReply.spec.ts
- packages/bot/src/utils/music/queueStateManager.spec.ts
- packages/bot/src/utils/music/autoplay/recommendations.spec.ts
- packages/bot/src/handlers/memberHandler.spec.ts
- packages/bot/src/handlers/interactionHandler.spec.ts
- packages/bot/src/handlers/commandsHandler.spec.ts
- packages/bot/src/handlers/eventHandler.spec.ts
- packages/bot/src/services/musicRecommendation/feedbackService.spec.ts
- packages/bot/src/utils/music/watchdog.spec.ts
- packages/bot/src/handlers/player/errorHandlers.spec.ts
- packages/bot/src/bot/start/initializer.spec.ts
- packages/bot/src/utils/music/service.spec.ts
- packages/bot/src/handlers/player/trackHandlers.spec.ts
✅ Files skipped from review due to trivial changes (2)
- packages/bot/src/lastfm/index.ts
- packages/bot/TEST_MAP.md
📜 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/utils/music/autoplay/pipeline.integration.spec.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/handlers/player/trackNowPlaying.tspackages/bot/src/handlers/externalScrobbler.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/utils/music/autoplay/autoplayAudit.spec.tspackages/bot/src/handlers/player/trackNowPlaying.spec.tspackages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/utils/music/autoplay/stats.spec.tspackages/bot/src/utils/music/autoplay/counters.spec.tspackages/bot/src/handlers/externalScrobbler.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/pipeline.integration.spec.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/utils/music/autoplay/autoplayAudit.spec.tspackages/bot/src/utils/music/autoplay/stats.spec.tspackages/bot/src/utils/music/autoplay/counters.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/pipeline.integration.spec.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/handlers/player/trackNowPlaying.tspackages/bot/src/handlers/externalScrobbler.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/utils/music/autoplay/autoplayAudit.spec.tspackages/bot/src/handlers/player/trackNowPlaying.spec.tspackages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/utils/music/autoplay/stats.spec.tspackages/bot/src/utils/music/autoplay/counters.spec.tspackages/bot/src/handlers/externalScrobbler.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/utils/music/autoplay/pipeline.integration.spec.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/handlers/player/trackNowPlaying.tspackages/bot/src/handlers/externalScrobbler.tspackages/bot/src/lastfm/lastFmApi.tspackages/bot/src/utils/music/autoplay/autoplayAudit.spec.tspackages/bot/src/handlers/player/trackNowPlaying.spec.tspackages/bot/src/lastfm/lastFmApi.spec.tspackages/bot/src/utils/music/autoplay/stats.spec.tspackages/bot/src/utils/music/autoplay/counters.spec.tspackages/bot/src/handlers/externalScrobbler.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/pipeline.integration.spec.tspackages/bot/src/utils/music/autoplay/autoplayAudit.spec.tspackages/bot/src/utils/music/autoplay/stats.spec.tspackages/bot/src/utils/music/autoplay/counters.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/pipeline.integration.spec.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/utils/music/autoplay/autoplayAudit.spec.tspackages/bot/src/utils/music/autoplay/stats.spec.tspackages/bot/src/utils/music/autoplay/counters.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/trackNowPlaying.tspackages/bot/src/handlers/player/trackNowPlaying.spec.ts
🔇 Additional comments (8)
packages/bot/src/utils/music/autoplay/counters.spec.ts (1)
18-19: Test updates look solidThe streamlined mocks and retained assertions still cover the key behavior boundaries (Redis fallback, in-memory state updates, and failure-path handling) without introducing brittleness.
Also applies to: 43-47, 56-130
packages/bot/src/utils/music/autoplay/pipeline.integration.spec.ts (1)
115-205: Great integration coverage on candidate gating pathsThese tests exercise the real scorer/collector behavior for locale veto and non-finite score rejection, which gives strong confidence in the autoplay pipeline’s safety rails.
Also applies to: 220-251
packages/bot/src/utils/music/autoplay/stats.spec.ts (1)
37-113: Stats and threshold coverage is in good shapeThe revised suite still preserves key behavior validation for defaults, temporal cutoffs, and autoplay enable/disable boundaries.
Also applies to: 116-152
packages/bot/src/utils/music/autoplay/autoplayAudit.spec.ts (1)
45-126: Nice strengthening of audit payload assertionsThe helper-based capture plus explicit field-level checks materially improves confidence in
AutoplayAuditCollector.emit()output stability.Also applies to: 130-213
packages/bot/src/handlers/externalScrobbler.spec.ts (1)
152-225: Same fixture shape concern astrackNowPlaying.spec.ts.
testMeta = { mbid: 'test-mbid', album: 'Test Album' }and{ mbid: 'scrobble-mbid' }are partial and don't matchLastFmTrackMetadata. See the consolidated comment ontrackNowPlaying.spec.ts— same suggestion applies here.packages/bot/src/lastfm/lastFmApi.ts (1)
106-142: LGTM on the cache + dedup design.In-flight map populated synchronously between the cache miss check and the first
awaitinside the IIFE means a second concurrent caller will see the in-flight promise and reuse it — safe in single-threaded JS. TheNOSONARannotation on the FEAT_ARTIST_SEPARATORS rationale is appropriate given the alternation contains only literal tokens.packages/bot/src/handlers/externalScrobbler.ts (1)
109-211: LGTM — metadata fetched once per track and forwarded correctly.Resolving metadata before iterating members avoids redundant
track.getInfocalls per user, andmeta ?? undefinedcorrectly normalizes thenullreturn type to match the optional parameter signature inupdateNowPlaying/scrobble.packages/bot/src/handlers/player/trackNowPlaying.ts (1)
238-326: LGTM.Metadata is fetched only after the session-key short-circuit, so no unnecessary
track.getInfocalls for users without a linked Last.fm account. The "metadata not found" debug log is fine at debug level given the cache will absorb retries on the same track.
- getTrackMetadata: trim+early-return on blank artist/title to avoid
polluting TRACK_METADATA_CACHE / TRACK_METADATA_IN_FLIGHT and stop
burning Last.fm requests on "::"-shaped keys. New it.each test
covers blank/whitespace inputs.
- updateNowPlaying / scrobble: prefer canonical metadata.artist /
metadata.title (Last.fm autocorrect already canonicalised them) when
a resolved metadata payload is supplied; fall back to
parseArtists().primary / normalizeLastFmTitle only when the caller
had no metadata to thread through.
- Export __resetMetadataCacheForTests() and call it from the
getTrackMetadata describe block so module-level cache state can't
leak between tests if ordering changes.
- trackNowPlaying.spec.ts: replace { mbid, listeners } stubs with the
full LastFmTrackMetadata-shaped fixture at both sites so a regression
is caught if the type tightens.
- candidateFallback.ts: replace 6 debugLog catch-block calls with
logAndSwallow so suppressed errors carry standardized context
(file::operation, structured data) for production diagnosis.
Refs PR #821 (CodeRabbit nits ×4 + actionable ×1)
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.
|
Owner
Author
|
@coderabbitai review All five findings from your previous review have been addressed in commit
Please re-review when convenient. |
|
✅ Actions performedReview triggered.
|
LucasSantana-Dev
added a commit
that referenced
this pull request
May 9, 2026
Lock the bot package's coverage gate at the round-down of the post-#821 baseline so subsequent test-cleanup work has a tripwire. Numbers are chosen to be binding without forcing emergency repair on day 1; tighten 2-3 % per phase as the suite shrinks toward proportional size. Current vs floor: Statements 67.03 % (>= 65) Branches 63.47 % (>= 60) Functions 63.89 % (>= 60) Lines 68.15 % (>= 65) Refs .agents/plans/test-cleanup-phase2.md
LucasSantana-Dev
added a commit
that referenced
this pull request
May 9, 2026
Records the Phase 5 decisions from /fix-the-suite: - Adopt Lucky-specific ≤1,500 test target (full-stack 39k LOC scaled 2.6× from skill table, not the ≤30-commands 50–200 bracket). - Pin coverage floor at 65/60/60/65 (round-down of post-#821 baseline). - Cleanup proceeds in single-file gate-checked batches; deletion only when no distinct branch is exercised. it.each consolidates LOC but not jest's reported test count. - Defer mutation testing (Stryker not installed); safety rests on preserved (input → expected) pairs and unchanged coverage numbers. Phase-2 batch 1 (PR #836) shipped under this strategy: 2 files, −21 tests, −1097 LOC, gate held at 67/63/64/68. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LucasSantana-Dev
added a commit
that referenced
this pull request
May 10, 2026
Scopes the integration-test rewrite plan (.agents/plans/autoplay-integration-rewrite.md) to its harness-only first phase. Phase B/C/D deferred pending evidence the harness gets adopted by an autoplay-touching PR within 4 weeks of Phase A merging. Rationale: v2.10.0 isn't blocked on test count (2825 is functional, 22s suite). No forcing function for committing to all 6 sessions. Phase A is the smallest reversible step that builds the highest- leverage reusable artifact (the __fixtures__/ harness + PR-#821 sertanejo smoke test). Revisit triggers explicit: Phase A merged + ≥1 PR uses harness + next cleanup batch scoped → consider Phase B. No adoption in 4 weeks → harness was speculative. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LucasSantana-Dev
added a commit
that referenced
this pull request
May 10, 2026
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>
Merged
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
Records the Phase 5 decisions from /fix-the-suite: - Adopt Lucky-specific ≤1,500 test target (full-stack 39k LOC scaled 2.6× from skill table, not the ≤30-commands 50–200 bracket). - Pin coverage floor at 65/60/60/65 (round-down of post-#821 baseline). - Cleanup proceeds in single-file gate-checked batches; deletion only when no distinct branch is exercised. it.each consolidates LOC but not jest's reported test count. - Defer mutation testing (Stryker not installed); safety rests on preserved (input → expected) pairs and unchanged coverage numbers. Phase-2 batch 1 (PR #836) shipped under this strategy: 2 files, −21 tests, −1097 LOC, gate held at 67/63/64/68. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
…1 tests) (#836) * test(youtube): consolidate analyzer.spec via it.each + remove redundant negatives - Collapse 13 individual error-detection it() blocks into one it.each table (one row per category × match style); positive cases preserved. - Replace 6 identical 'does not detect X' negatives with one 'returns all-false flags for unrelated error' assertion (saves 5 tests). - Replace 6 near-identical getErrorResponse describes (full YouTubeErrorInfo literal each) with one it.each table + shared infoWith() factory. - Fold 6 'should prioritize X' tests into one assertion that walks the full priority chain (saves 5 tests). - Replace 4 integration scenarios with one it.each (one row per scenario). Net: 56 → 41 tests (-15) on this file. Bot suite: 2848 → 2833. Coverage unchanged at 67/63/64/68 — gate (65/60/60/65) holds with same headroom. Refs .agents/plans/test-cleanup-phase2.md * test(queue): consolidate queueStateManager.spec via it.each tables - isQueueEmpty: 4 tests → 1 it.each (3 rows) - isQueueFull: 7 tests → 1 it.each (7 rows, default-arg covered inline) - getQueueState duration coercion: 3 tests → 1 it.each (3 rows) - getQueueState position fallback: 3 tests → 1 it.each (3 rows) - getQueueStats: collapse separate 'unique artists' + 'empty when no author' + 'deduplicate' into one comprehensive test - getTrackAtPosition: 6 position-validity tests → 1 it.each (6 rows) - isTrackInQueue: 3 match/no-match tests → 1 it.each (3 rows) - getTrackPosition: 5 lookup tests → 1 it.each (5 rows) - Extract withTracks/withTracksThrow helpers to remove repeated toArray.mockReturnValue/mockImplementation boilerplate. Net: 59 → 53 tests (-6) on this file. Full bot suite: 2833 → 2827. Coverage unchanged at 67/63/64/68 — gate (65/60/60/65) holds. Refs .agents/plans/test-cleanup-phase2.md * docs(adr): bot test suite cleanup strategy and proportionality target Records the Phase 5 decisions from /fix-the-suite: - Adopt Lucky-specific ≤1,500 test target (full-stack 39k LOC scaled 2.6× from skill table, not the ≤30-commands 50–200 bracket). - Pin coverage floor at 65/60/60/65 (round-down of post-#821 baseline). - Cleanup proceeds in single-file gate-checked batches; deletion only when no distinct branch is exercised. it.each consolidates LOC but not jest's reported test count. - Defer mutation testing (Stryker not installed); safety rests on preserved (input → expected) pairs and unchanged coverage numbers. Phase-2 batch 1 (PR #836) shipped under this strategy: 2 files, −21 tests, −1097 LOC, gate held at 67/63/64/68. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(bot): restore exact assertions + negative-flags coverage Addresses pr-test-analyzer findings: - queueStateManager.spec.ts: restore exact averageDuration assertion (was toBeGreaterThan(0) — mutation-survivable) Changed: expect(stats.averageDuration).toBeCloseTo(206666.66666666666, 5) (620000/3 = 206666.666... exact float) - Added dedicated edge case test for equal-duration tracks (3 × 200000ms → 200000 exact average) - analyzer.spec.ts: add explicit negative-flags rows to it.each table so "set X without setting Y/Z" is parameterized coverage (3 rows) Per ADR 2026-05-09: coverage gate (65/60/60/65) held — see PR comment with text-summary output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
…t scrobbles (#821) * test: remove 52 pure-delegation spec files (Phase 1) These files contained only toHaveBeenCalled assertions — no behavioral assertions on return values, state changes, or reply content. TypeScript enforces the same routing contracts at compile time. ~455 tests removed (3339 → 2884), zero regression risk. All tests pass. * test: trim delegation-only it() blocks from mixed spec files (Phase 2) Removed individual it() blocks where EVERY expect() call was toHaveBeenCalled/toHaveBeenCalledWith with zero assertions on return values, state, reply content, or thrown errors. Kept all blocks with behavioral assertions. Files trimmed: queueManipulation, trackHandlers, autoplay, case. 455 pure-delegation tests removed; 2777 behavioral tests remain. * test: trim delegation-only it() blocks from command spec files (Phase 3) Removed pure-delegation test blocks from giveaway, level, and music specs. Retained all behavioral assertions on reply content, error messages, and conditional logic. * test: remove pure-delegation it() blocks from play/index.spec.ts (Phase 4) Deleted 2 test blocks that only asserted toHaveBeenCalled() with no assertions on return values, state, or reply content. * test: add autoplay pipeline integration test, trim coordinator specs - Add pipeline.integration.spec.ts: end-to-end test of collectRecommendationCandidates with real candidateScorer, diversitySelector, languageHeuristics. Proves the cross-locale veto (dominantLocale: null + Spanish gospel → -Infinity → dropped). - Delete replenisher.spec.ts (8 tests): pure coordinator — all collaborators mocked, only tested call routing. - Delete recommendations.spec.ts (16 tests): pure coordinator with no algorithmic logic. - Trim candidateCollector.spec.ts: remove 8 coordinator-level collectRecommendationCandidates tests (covered by integration spec); keep 10 unit tests for shouldIncludeCandidate + upsertScoredCandidate. - Trim counters.spec.ts: remove log assertions and coordinator-call verifications; keep 11 behavioral tests (24 → 11). - Trim stats.spec.ts: remove log assertions and redundant paths; keep 11 behavioral tests with boundary cases (21 → 11). Net: -46 tests across autoplay module. * fix(lastfm): resolve canonical metadata to fix scrobble album art and multi-artist parsing - Add `getTrackMetadata()` that calls `track.getInfo?autocorrect=1` to resolve canonical artist/title/album/albumArtist/mbid; results cached 24 h (5000-entry map) to avoid redundant API calls. - Add `parseArtists()` to split multi-artist strings (feat./ft./&/×/x/ vs./with) into primary + featured[]; `updateNowPlaying` and `scrobble` now send only the primary artist, matching Last.fm's expectation. - Fix `FEAT_ARTIST_SEPARATORS` regex: drop trailing `\b` on `vs\.?` so "Artist vs. Other" with a trailing dot splits correctly. - Pipe metadata (album, albumArtist, mbid) into signed POST params for both `track.updateNowPlaying` and `track.scrobble`, enabling album-art display on scrobble cards. - Update `trackNowPlaying.ts` and `externalScrobbler.ts` to fetch metadata once per track and pass it down to all API calls. - Export `getTrackMetadata`, `parseArtists`, `LastFmTrackMetadata` from the `lastfm` barrel. - Add 30 new unit tests covering `parseArtists` (all separators, edge cases) and `getTrackMetadata` (success, caching, error paths). * fix(lastfm): fix cache race, empty artist guard, and log level in metadata fetch * test: trim log-assertion and coordinator-call tests from 5 spec files - commandsHandler.spec.ts: 21 → 16 tests - service.spec.ts: 44 → 37 tests - feedbackService.spec.ts: 38 → 34 tests - queueManipulation.spec.ts: 90 tests (no changes, all verify behavior) - queueStateManager.spec.ts: 60 → 59 tests Removed 17 tests total that only verified mock calls or Redis implementation details without testing actual behavior. Kept all tests that verify return values, state mutations, error handling, and observable side effects. All 2725 tests passing. * test(lastfm): add TTL expiry, empty string, and unicode edge-case tests * test(lastfm): fix timer cleanup and assertion style in edge-case tests - Add afterEach hook to getTrackMetadata describe block to guarantee jest.useRealTimers() cleanup (prevents fake timer leaks if assertions fail) - Remove inline jest.useRealTimers() from TTL test body - Fix unicode test to use consistent .toEqual() assertion pattern matching all other parseArtists tests * test: remove coordinator/log-only tests from eventHandler Deleted 5 tests that had only log or mock-call assertions with no behavioral coverage: - 'logs error when guild delete cleanup fails' (only asserted mock.toHaveBeenCalled) - 'logs error when channel cleanup fails' (only asserted errorLogMock call) - 'logs when client is ready' (only asserted infoLogMock call) - 'logs command count when ready' (only asserted debugLogMock call) - 'logs error if ai dev toolkit fails to start' (only asserted errorLogMock call) Kept 17 tests with behavioral assertions. All remaining tests verify: - Error handling + proper error log messages - Autocomplete response behavior and edge cases - Button routing logic - Guild/channel cleanup execution - Async toolkit service startup * test: remove coordinator/log-only tests from errorHandlers * feat(lastfm): log when track metadata is unavailable in now-playing handlers * test: remove coordinator/log-only tests from interactionReply and memberHandler * test: remove coordinator/log-only tests from playerFactory bridge and watchdog * test: remove coordinator/log-only tests from interactionHandler and queueStrategy * test: remove coordinator/log-only tests from initializer and automod * test(lastfm): cover metadata parameter in updateNowPlaying and scrobble Add comprehensive test coverage for the optional metadata parameter (album, albumArtist, mbid) in updateNowPlaying and scrobble functions. Tests verify that metadata fields are included when provided and omitted when undefined. Also suppress security hotspot on api_key URL param in getTrackMetadata (internal key, not user-controlled data). * ci: trigger test suite after merge from release/v2.10.0 * test(autoplay): add AutoplayAuditCollector unit tests Adds autoplayAudit.ts (collector class + AutoplayAuditRecord interface) and autoplayAudit.spec.ts (11 tests covering recordEvaluated, setFinalSelected, and emit behaviour including sessionMood, cycleId format, and infoLog contract). * fix(test): remove unclosed it() block left by merge conflict resolution The merge commit 0b00570 included an incomplete test setup for 'adds spotify recommendation results as scored candidates' with no assertions or closing bracket, nesting subsequent it.each() calls inside it and breaking the test suite. This test was intentionally removed in the PR #821 redesign; removing the orphaned setup restores the intended structure. * fix(tests): complete 3 incomplete test blocks in queueManipulation.spec Three tests were left incomplete after the release/v2.10.0 merge: 1. 'tops up autoplay queue with multiple tracks when below buffer' — missing await replenishQueue() call, so player.search was never invoked. 2. 'adds spotify recommendation results as scored candidates' — missing queue creation, replenishQueue call, assertion, and closing }) causing three it.each blocks to be incorrectly nested inside it. 3. 'returns without adding tracks when candidate set is exhausted' — missing assertion and closing }) causing 'tags session novelty' to be nested inside it (Tests cannot be nested error). All 95 queueManipulation tests now pass. * test(externalScrobbler): restore spec with getTrackMetadata mock and coverage The spec was accidentally dropped during the test suite redesign. Restores all 5 original tests plus 2 new tests covering the getTrackMetadata forwarding paths added to scrobblePreviousTrack and handleExternalNowPlaying. * fix(autoplay): log swallowed errors in candidateFallback * test(lastfm): assert IN_FLIGHT dedup prevents concurrent duplicate fetches * refactor(lastfm): tighten LastFmTrackMetadata return type to non-partial or null * fix(lastfm): log HTTP status on non-ok response + test * fix(lastfm): add NOSONAR to artist.gettoptags URL to suppress security hotspot API key in URL is required by Last.fm's API design — not a security issue. * test(lastfm): add updateNowPlaying blank-input guard tests * fix(lastfm): NOSONAR S5852 on FEAT_ARTIST_SEPARATORS split regex The regex contains only literal tokens in its alternation (no nested quantifiers or overlapping branches) and is consumed by String.split() on short Last.fm artist strings, so it cannot exhibit the super-linear backtracking S5852 guards against. Document the rationale in-source so the SonarCloud quality gate stops blocking PR #821. * fix(lastfm): address Sonar ReDoS + Greptile P1 review findings - Replace S5852 NOSONAR on FEAT_ARTIST_SEPARATORS with bounded whitespace quantifiers (\s{0,4} / \s{1,4}); regex now provably linear, eliminating the SonarCloud security hotspot. - getTrackMetadata: extract primary artist via parseArtists() before calling track.getInfo. Last.fm's autocorrect does not split collaboration strings, so 'Drake feat. Rihanna' previously returned error 6 and broke album-art resolution — exactly the regression the PR was meant to fix. New spec covers the case. - getArtistTopTags: restore logAndWarn (had been downgraded to logAndSwallow), so autoplay tag-fetch failures stay observable in production logs. - Remove ads-math-10-per-month.md — unrelated marketing scratch file accidentally included in the spec-redesign branch. Refs PR #821 (CodeRabbit summary, Greptile P1 ×3, SonarCloud S5852) * fix(lastfm): address CodeRabbit review on PR #821 - getTrackMetadata: trim+early-return on blank artist/title to avoid polluting TRACK_METADATA_CACHE / TRACK_METADATA_IN_FLIGHT and stop burning Last.fm requests on "::"-shaped keys. New it.each test covers blank/whitespace inputs. - updateNowPlaying / scrobble: prefer canonical metadata.artist / metadata.title (Last.fm autocorrect already canonicalised them) when a resolved metadata payload is supplied; fall back to parseArtists().primary / normalizeLastFmTitle only when the caller had no metadata to thread through. - Export __resetMetadataCacheForTests() and call it from the getTrackMetadata describe block so module-level cache state can't leak between tests if ordering changes. - trackNowPlaying.spec.ts: replace { mbid, listeners } stubs with the full LastFmTrackMetadata-shaped fixture at both sites so a regression is caught if the type tightens. - candidateFallback.ts: replace 6 debugLog catch-block calls with logAndSwallow so suppressed errors carry standardized context (file::operation, structured data) for production diagnosis. Refs PR #821 (CodeRabbit nits ×4 + actionable ×1) --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
Lock the bot package's coverage gate at the round-down of the post-#821 baseline so subsequent test-cleanup work has a tripwire. Numbers are chosen to be binding without forcing emergency repair on day 1; tighten 2-3 % per phase as the suite shrinks toward proportional size. Current vs floor: Statements 67.03 % (>= 65) Branches 63.47 % (>= 60) Functions 63.89 % (>= 60) Lines 68.15 % (>= 65) Refs .agents/plans/test-cleanup-phase2.md
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
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)
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
…1 tests) (#836) * test(youtube): consolidate analyzer.spec via it.each + remove redundant negatives - Collapse 13 individual error-detection it() blocks into one it.each table (one row per category × match style); positive cases preserved. - Replace 6 identical 'does not detect X' negatives with one 'returns all-false flags for unrelated error' assertion (saves 5 tests). - Replace 6 near-identical getErrorResponse describes (full YouTubeErrorInfo literal each) with one it.each table + shared infoWith() factory. - Fold 6 'should prioritize X' tests into one assertion that walks the full priority chain (saves 5 tests). - Replace 4 integration scenarios with one it.each (one row per scenario). Net: 56 → 41 tests (-15) on this file. Bot suite: 2848 → 2833. Coverage unchanged at 67/63/64/68 — gate (65/60/60/65) holds with same headroom. Refs .agents/plans/test-cleanup-phase2.md * test(queue): consolidate queueStateManager.spec via it.each tables - isQueueEmpty: 4 tests → 1 it.each (3 rows) - isQueueFull: 7 tests → 1 it.each (7 rows, default-arg covered inline) - getQueueState duration coercion: 3 tests → 1 it.each (3 rows) - getQueueState position fallback: 3 tests → 1 it.each (3 rows) - getQueueStats: collapse separate 'unique artists' + 'empty when no author' + 'deduplicate' into one comprehensive test - getTrackAtPosition: 6 position-validity tests → 1 it.each (6 rows) - isTrackInQueue: 3 match/no-match tests → 1 it.each (3 rows) - getTrackPosition: 5 lookup tests → 1 it.each (5 rows) - Extract withTracks/withTracksThrow helpers to remove repeated toArray.mockReturnValue/mockImplementation boilerplate. Net: 59 → 53 tests (-6) on this file. Full bot suite: 2833 → 2827. Coverage unchanged at 67/63/64/68 — gate (65/60/60/65) holds. Refs .agents/plans/test-cleanup-phase2.md * docs(adr): bot test suite cleanup strategy and proportionality target Records the Phase 5 decisions from /fix-the-suite: - Adopt Lucky-specific ≤1,500 test target (full-stack 39k LOC scaled 2.6× from skill table, not the ≤30-commands 50–200 bracket). - Pin coverage floor at 65/60/60/65 (round-down of post-#821 baseline). - Cleanup proceeds in single-file gate-checked batches; deletion only when no distinct branch is exercised. it.each consolidates LOC but not jest's reported test count. - Defer mutation testing (Stryker not installed); safety rests on preserved (input → expected) pairs and unchanged coverage numbers. Phase-2 batch 1 (PR #836) shipped under this strategy: 2 files, −21 tests, −1097 LOC, gate held at 67/63/64/68. * test(bot): restore exact assertions + negative-flags coverage Addresses pr-test-analyzer findings: - queueStateManager.spec.ts: restore exact averageDuration assertion (was toBeGreaterThan(0) — mutation-survivable) Changed: expect(stats.averageDuration).toBeCloseTo(206666.66666666666, 5) (620000/3 = 206666.666... exact float) - Added dedicated edge case test for equal-duration tracks (3 × 200000ms → 200000 exact average) - analyzer.spec.ts: add explicit negative-flags rows to it.each table so "set X without setting Y/Z" is parameterized coverage (3 rows) Per ADR 2026-05-09: coverage gate (65/60/60/65) held — see PR comment with text-summary output. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
getTrackMetadata()— callstrack.getInfo?autocorrect=1to resolve canonical artist/title/album/albumArtist/mbid fields; cached 24 h (5 000-entry map) to avoid redundant API calls per trackparseArtists()— splits multi-artist strings (feat./ft./&/×/x/vs./with) into primary + featured[]; scrobble API calls now use only the primary artist, matching Last.fm's expectation and preventing malformed lookupsupdateNowPlayingandscrobblenow receive album/albumArtist/mbid from the resolved metadata and include them in the signed POST, enabling album-art display on Last.fm scrobble cardsexternalScrobbler.ts+trackNowPlaying.ts— both handlers fetch metadata once before member loops and forward it to all API callsindex.tsnow exportsgetTrackMetadata,parseArtists, andLastFmTrackMetadataFEAT_ARTIST_SEPARATORSregex had a trailing\bonvs\.?that prevented matchingvs.(with dot); removed so "Artist vs. Other" splits correctlyparseArtists(all separators + edge cases) andgetTrackMetadata(success, 24 h caching, non-ok response, API error payload, fetch rejection, not-configured)Test plan
npx jest --testPathPatterns="lastFmApi"— 73 tests passtsc --noEmit— no type errors on changed filesDrake feat. Rihanna) sends onlyDrakeas artist param🤖 Generated with Claude Code
Greptile Summary
getTrackMetadata()(with 24 h LRU cache and in-flight deduplication),parseArtists()for multi-artist splitting, and forwards resolved album/albumArtist/mbid toupdateNowPlayingandscrobblesigned POST calls, enabling album-art on Last.fm scrobble cards.updateNowPlayingandscrobblenow useparseArtists(artist).primaryso multi-artist strings like "Drake feat. Rihanna" send only the primary artist to Last.fm, fixing malformed lookups.getArtistTopTagserror logging was silently downgraded fromlogAndWarntologAndSwallow, hiding network/API failures from production observability (unrelated to the stated fix).Confidence Score: 3/5
Mergeable with low risk, but the silent downgrade of getArtistTopTags error logging from logAndWarn to logAndSwallow should be reverted before merging to avoid hiding production errors.
One P1 finding (logAndWarn→logAndSwallow in getArtistTopTags) silently suppresses network/API error visibility for autoplay tag lookups. This was not described in the PR and reduces observability. The ceiling for a P1 is 4/5, but with the additional context from prior-round comments about behavioral test deletions across multiple files, the overall confidence drops to 3.
packages/bot/src/lastfm/lastFmApi.ts — the incidental logAndWarn→logAndSwallow change at line 400
Important Files Changed
Sequence Diagram
sequenceDiagram participant H as trackNowPlaying / externalScrobbler participant GM as getTrackMetadata() participant Cache as MetadataCache (24h TTL) participant LFM as Last.fm API H->>GM: getTrackMetadata(artist, title) GM->>Cache: check key (lowercase artist::title) alt cache hit Cache-->>GM: cached metadata GM-->>H: LastFmTrackMetadata else cache miss / in-flight dedup GM->>LFM: "track.getInfo?autocorrect=1" LFM-->>GM: track object (name/artist/album/mbid) GM->>Cache: "store with expiresAt = now + 24h" GM-->>H: "LastFmTrackMetadata | null" end H->>LFM: updateNowPlaying / scrobble (parseArtists(artist).primary, metadata?)Comments Outside Diff (2)
packages/bot/src/handlers/externalScrobbler.spec.tsexternalScrobbler.tswas extended in this PR (addsgetTrackMetadatacalls and passes metadata toscrobble/updateNowPlaying), but the entire spec file was deleted as "pure delegation." The removed tests exercise runtime behavior that TypeScript cannot enforce:'parses now-playing line and updates Last.fm for voice members'— verifies artist/song regex parsing from raw Discord message content and iterates actual voice channel members'scrobbles previous track on next now-playing event after 30 seconds'— usesjest.spyOn(Date, 'now')to verify the 30-second threshold guards the scrobble decision'unlinks user when updateNowPlaying fails with invalid Last.fm session'— assertslastFmUnlinkMockis called anderrorLogis not triggered on the expected branch'logs unlink failure when invalid Last.fm session cannot be removed'— asserts the error log message content ('Failed to remove invalid Last.fm session') and thediscordIdpayloadThe new
getTrackMetadataintegration added inexternalScrobbler.tsis now completely untested.packages/bot/src/handlers/auditHandler.spec.tsSeveral deleted
it()blocks protect runtime branching logic that TypeScript does not enforce:'should not log when author is a bot'— assertsserverLogService.createLogis never called for bot-authored messages'should not log when content is unchanged'— verifies the old-equals-new equality check suppresses duplicate edit logs'should handle long message content by truncating'— asserts the 500-character truncation path produces the correct shortened content in thecreateLogcall'should not log when feature is disabled'— verifies thefeatureToggleService.isEnabledgate is respectedAll four cover conditional branches (
if (author.bot),if (old === new),if (!featureEnabled), length-capping) whose correctness cannot be inferred from types alone.Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'origin/rel..." | Re-trigger Greptile
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation