Repository navigation
fix(music): fix artist routing, stop snapshot clear, manual track priority + autoplay improvements - #805
Conversation
…ority, and improve autoplay Bug fixes: - /artist: prefer exact/word-boundary artist match before substring; prevents "Prince" from routing to "Prince Royce" (#1) - /stop: delete Redis session snapshot on stop so autoplay tracks don't persist for the next session (#2) - moveUserTrackToPriority: add reference/ID fallback lookup so Spotify tracks (resolved to YouTube URLs at queue time) are correctly re-ordered (#3) Autoplay improvements (plan item execution): - LASTFM_SCORE_BOOST 0.0 → 0.20 so Last.fm candidates get meaningful lift over cold Spotify picks - Deep-dive sessions: raise per-artist cap to 5 (from 2) when session mood detects same-artist focus, so autoplay follows the user's intent - Cache sessionMood between replenish cycles; recompute only after 3+ new tracks to prevent per-cycle mood flips - Log hard-rejected autoplay candidates (score -Infinity/NaN) at debug level for visibility into genre-veto decisions
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits 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 (3)
📝 WalkthroughWalkthroughThis PR overhauls the music autoplay recommendation system with a new scoring API, adds loved-track tracking to Last.FM seeds, implements session-mood-aware candidate selection with skip-storm logic, enhances Last.FM error logging, adds album diversity tracking, refactors the stop command to delete session snapshots, improves artist track selection via tiered matching, and adds comprehensive test coverage across music queue and autoplay modules. ChangesArtist Command Track Selection Enhancement
Stop Command Session Snapshot Cleanup
Autoplay Recommendation System Overhaul
Error Handling & Last.FM API Logging
Music Queue Infrastructure & Testing
Documentation & Context
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…k, and skip-storm detection Plan items implemented: - 1.2: Last.fm loved seed boost — LASTFM_SCORE_BOOST 0.0→0.20; +0.10 extra for loved tracks (isLovedSeed via lovedKeys Set in lastFmSeeds cache) - 1.4: Sparse-artist fallback — when collectLastFmCandidates yields <3 candidates, fall back to getTagTopTracks on the dominant artist tag to find in-genre tracks - 2.1: Tempo continuity penalty — Δtempo>40 BPM: -0.15; >25 BPM: -0.07 in enrichWithAudioFeatures - 2.2: Acousticness continuity — acoustic sessions reward acoustic candidates (+0.10); penalise acoustic candidates in electric sessions (-0.10) - 2.3: Popularity boost in restless mood — track.raw.popularity>50 adds +0.08 when restless - 2.4: Same-album soft penalty in selectDiverseCandidates — -0.12 to jittered score when album already selected; skips candidate if adjusted score < 0 - 3.3: Duration sigmoid — 0.15*tanh((durationMs-300k)/60k) replaces flat +0.1 for preferLong - 3.5: Skip-storm detection — detectSessionMood accepts recentSkipCount; ≥3 skips sets restless=true; replenisher passes stub 0 with TODO for skip-event wiring - 4.2: Query modifier expansion — BASE_QUERY_MODIFIERS (5) vs FULL_QUERY_MODIFIERS (10); restless sessions use base-only to cycle faster through similar searches
12 tests covering searchLastFmQuery (engine fallback, duration filter, result limit) and collectLastFmCandidates (seed selection, loved boost, dislike skip, shouldIncludeCandidate guard).
…rtist fallback Adds tests for the similar-tracks loop (lines 150-192) and the sparse-artist genre fallback (lines 197-254) which were completely uncovered, bringing lastFmSeeder.ts above the 80% SonarCloud threshold. Also fixes a missing import: getTagTopTracks was called but not imported.
- Remove unused LRUCache import from 'lru-cache' - Remove unused AudioFeatureEntry interface - Remove duplicate audioFeatureCache definition The canonical audioFeatureCache instance lives in queueManipulation.ts and is actively used in getTrackAudioFeatures(). This removes the dead code duplicate in candidateScorer.ts that was never instantiated or used. Test Results: 202/204 test suites passed (2,855 tests) - Pre-existing failures in lifecycleHandlers and queueResolverWiring unrelated to this cache cleanup
Replace logAndSwallow (debugLog) with logAndWarn (warnLog) in all Last.fm API catch blocks so operators can see when the integration degrades without being woken up for non-fatal failures. Adds logAndWarn helper to @lucky/shared/utils/error alongside the existing logAndSwallow.
All critical issues addressed in commit e62362d
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bot/src/utils/music/autoplay/candidateScorer.spec.ts (1)
841-935:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the stray closing brace at line 843 — it prematurely closes the
describe('enrichWithAudioFeatures')block.The five new tempo/acousticness tests (lines 845–933) and the
'sorts results by score descending'test are placed after an extra})at line 843, which closes the describe block opened at line 654. This orphans them at the outerdescribe('candidateScorer')level instead of nesting them inside the audio-features suite. While Jest executes them regardless, the test hierarchy is incorrect and any futurebeforeEach/afterEachsetup scoped todescribe('enrichWithAudioFeatures')will silently skip these tests. Delete the stray brace at line 843 and move the closing brace forenrichWithAudioFeaturesto after line 933.🤖 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/candidateScorer.spec.ts` around lines 841 - 935, There is a stray closing brace that prematurely ends the describe('enrichWithAudioFeatures') block, which leaves the five tempo/acousticness tests (e.g., "applies tempo drastic change penalty...", "boosts track with high acousticness...", "penalizes extreme acousticness swing...", "applies continuity bonus...", "penalizes acoustic candidate...") and the "sorts results by score descending" test outside that suite; remove that extra "})" that closes describe('enrichWithAudioFeatures') too early and instead place the closing "})" for describe('enrichWithAudioFeatures') after the last of those tests so all enrichWithAudioFeatures-related it() cases remain nested inside the correct describe block.
♻️ Duplicate comments (1)
packages/bot/src/utils/music/queueManipulation.ts (1)
874-879:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnguarded URL fallback can still match unrelated tracks when both URLs are falsy.
The same finding from the previous review still applies: when
track.urlisundefined/empty (or both compared values are),t.url === track.urlevaluates totrueand the wrong queue item gets promoted. Reference equality and ID comparison are short-circuit safe, but the URL leg needs a truthiness guard.🛡️ Suggested guard
const trackIndex = tracks.findIndex( (t) => t === track || (track.id && t.id === track.id) || - t.url === track.url, + (Boolean(track.url) && t.url === track.url), )🤖 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.ts` around lines 874 - 879, The URL comparison in the tracks.findIndex call can match when both t.url and track.url are falsy; update the predicate used by trackIndex so the URL branch is guarded by truthiness (e.g., only evaluate t.url === track.url when both track.url and t.url are truthy) while keeping the existing reference and id checks (refer to tracks.findIndex, the predicate parameters t and track, and fields track.id / t.id / t.url / track.url); ensure short-circuiting order remains: identity check, id equality, then guarded URL equality.
🧹 Nitpick comments (11)
packages/bot/src/utils/music/autoplay/lastFmSeeds.spec.ts (1)
560-581: 💤 Low valueTrim assertion is too weak to validate the named behavior.
The test name promises validation that "weighted allocation overshoot is trimmed to count", but the only assertion is
expect(slice.length).toBeLessThanOrEqual(3)— which is also satisfied by any path that returns 0–3 tracks for unrelated reasons (e.g., a user has no link, dedup collapses results, or no overshoot ever occurs). Consider asserting the exact length is 3 (and ideally checking the per-user composition) so the test actually exercises the trim branch.🤖 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/lastFmSeeds.spec.ts` around lines 560 - 581, The test currently only checks slice.length <= 3 which is too weak; update the test that calls consumeBlendedSeedSlice(['heavy','light'], 3, weights) to assert the exact trimmed behavior by expecting slice.length === 3 and (optionally) verifying the per-seed composition—e.g., that 'heavy' contributed 3 items before trimming or that the returned slice contains the expected tracks from getTopTracksMock for 'heavy' and 'light'; locate and modify the test around consumeBlendedSeedSlice, getByDiscordIdMock and getTopTracksMock to add these stricter assertions so the overshoot trim branch is actually exercised.packages/bot/src/utils/music/autoplay/diversitySelector.spec.ts (2)
457-518: 💤 Low valueIndentation of the new describe block doesn't match siblings.
The new
describe('same-album soft penalty', ...)at line 457 is indented 8 spaces, while sibling blocks insidedescribe('diversitySelector', ...)(e.g.,purgeDuplicatesOfCurrentTrackat line 520) are indented 4 spaces. Only a style issue, but worth aligning to keep the file consistent.🤖 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/diversitySelector.spec.ts` around lines 457 - 518, The new describe block describe('same-album soft penalty', ...) is over-indented compared to its sibling describe('diversitySelector', ...) children (e.g., purgeDuplicatesOfCurrentTrack); adjust the leading spaces so the describe('same-album soft penalty', ...) block and its inner tests are indented to match the other sibling blocks (4 spaces from the parent) to restore consistent file indentation.
477-492: 💤 Low valueTest name overstates what is asserted.
The test is titled "second track from same album is penalised by 0.12", but the assertions only check that
Track AandTrack Care present in the selected output — they never verify that Track B'sjitteredScorewas reduced by 0.12 (or that B was specifically affected by the penalty rather than excluded for some other reason). Consider asserting against the resulting score of B (or its presence/absence under tighter conditions) so the test actually validates the 0.12 magnitude described in the name.🤖 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/diversitySelector.spec.ts` around lines 477 - 492, The test "second track from same album is penalised by 0.12" currently only checks selected titles but not the actual penalty; update the test that calls selectDiverseCandidates (and uses makeCandidate) to assert Track B's resulting jitteredScore is approximately 0.85 - 0.12 (allowing the existing jitter tolerance) or alternatively rename the test to match the weaker assertion; specifically, locate the test block and either (A) find the selected entry for id 'b' (or from selected.map(s => s.track.title) locate the object) and add an expect on its jitteredScore to be close to 0.73 within the jitter delta, or (B) change the test title to something like "selects tracks allowing same-album when artist limit permits" if you prefer not to assert scores.packages/bot/src/utils/music/autoplay/sessionMood.ts (2)
9-9: 💤 Low valueConsider making
recentSkipCountnon-optional in the returned shape.
detectSessionMoodalways assignsrecentSkipCount(defaulting to0), so consumers never observeundefined. Marking the field optional in theSessionMoodinterface forces every reader to handle a| undefinedbranch that can't actually happen. Either keep the field optional and only set it when meaningful, or make it required (recentSkipCount: number) to match the runtime contract.🤖 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/sessionMood.ts` at line 9, The SessionMood shape currently declares recentSkipCount as optional but detectSessionMood always sets it (defaulting to 0); change the SessionMood type to make recentSkipCount required (recentSkipCount: number) and ensure detectSessionMood still assigns a numeric value, and update any callers or type assertions that expected undefined; locate the SessionMood interface and the detectSessionMood function to make these coordinated changes so the runtime contract and type signature match.
116-119: 💤 Low valueComment wording contradicts the behaviour.
The comment says "relax mood when user is skipping aggressively", but the branch actually forces
restless = true, which is a stricter/diversifying mood — not a relaxation. Reword to match (e.g., "force restless mood when user is skipping aggressively") to avoid future-reader confusion.🤖 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/sessionMood.ts` around lines 116 - 119, The comment for the aggressive-skip branch is misleading: it reads "relax mood when user is skipping aggressively" but the code sets restless = true; update the comment to match behavior (e.g., "force restless mood when user is skipping aggressively" or similar) near the conditional that checks recentSkipCount and sets restless in sessionMood.ts so the comment and the variables recentSkipCount and restless reflect the actual intent.packages/bot/src/utils/music/autoplay/spotifyRecommender.ts (1)
184-196: 💤 Low valueUse the already-imported
SessionMoodtype instead of an inlineimport(...)type.
SessionMoodis imported at the top of the file (line 21), so the inlineimport('./sessionMood').SessionMoodon line 189 is just redundant. Also, sincerestlessis the only flag consulted, the parameter could be narrowed accordingly without changing behavior.♻️ Suggested simplification
- sessionMood?: import('./sessionMood').SessionMood | null, + sessionMood?: SessionMood | null,🤖 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/spotifyRecommender.ts` around lines 184 - 196, In searchSeedCandidates, replace the inline type import "import('./sessionMood').SessionMood" with the already-imported SessionMood type and, since only the restless flag is used, narrow the parameter to either SessionMood | null or a smaller shape like { restless: boolean } | null; update the function signature parameter name (sessionMood) to use that type so the code continues to reference sessionMood.restless without the redundant inline import.packages/bot/src/utils/music/autoplay/candidateCollector.ts (1)
60-62: ⚡ Quick winLast fallback in
candidateKeyis dead code.
candidateKeyfalls back throughcandidate.id || candidate.url || normalizeTrackKey(candidate.title, candidate.author)only whennormalizedKey === '::'. ButnormalizeTrackKey(title, author)is what produced'::'in the first place, so calling it again as the final fallback is guaranteed to return'::'— i.e., the same key for every empty-title/empty-author candidate, defeating the purpose of falling back. Drop the third leg or replace it with a stable per-track key (e.g.,String(candidate.duration ?? '')or the candidate'ssource).♻️ Suggested cleanup
- const normalizedKey = normalizeTrackKey(candidate.title, candidate.author) - const candidateKey = - normalizedKey !== '::' ? normalizedKey : (candidate.id || candidate.url || normalizeTrackKey(candidate.title, candidate.author)) + const normalizedKey = normalizeTrackKey(candidate.title, candidate.author) + const candidateKey = + normalizedKey !== '::' + ? normalizedKey + : (candidate.id || candidate.url || `unknown::${candidates.size}`)🤖 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/candidateCollector.ts` around lines 60 - 62, The fallback in candidateKey is dead because normalizeTrackKey(title, author) already returned '::' and calling it again won't help; update the fallback to use a stable per-track identifier instead of re-calling normalizeTrackKey. In the candidateKey expression (variable candidateKey that currently uses normalizedKey !== '::' ? normalizedKey : (candidate.id || candidate.url || normalizeTrackKey(...))), replace the final normalizeTrackKey(...) leg with a stable value such as String(candidate.duration ?? '') or candidate.source (e.g., candidate.id || candidate.url || String(candidate.duration ?? candidate.source ?? '') ) so each track gets a meaningful fallback key.packages/bot/src/utils/music/queueManipulation.ts (2)
822-823: 💤 Low valueMove the canonical
calculateRecommendationScoreimport to the top.Mid-file
importis hoisted, so this is functionally fine, but it splits imports across the file (the rest are at lines 1–61) and makes the module's external surface harder to reason about. Either move it to the top import block or, since this file already re-exports it, drop the re-export and have callers import directly from./autoplay/candidateScorer.🤖 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.ts` around lines 822 - 823, The import of calculateRecommendationScore is mid-file which splits imports; either move the import statement for calculateRecommendationScore to the top-level import block alongside the other imports, or remove this re-export (export { calculateRecommendationScore }) and update callers to import calculateRecommendationScore directly from ./autoplay/candidateScorer; locate the calculateRecommendationScore symbol and the export line in queueManipulation.ts to apply the chosen change.
71-71: ⚡ Quick winRemove dead constants:
LASTFM_SCORE_BOOST,LASTFM_SEED_COUNT, andMAX_SIMILAR_LOOKUPS.These constants are no longer used in
queueManipulation.ts— they have been migrated to and actively used inpackages/bot/src/utils/music/autoplay/lastFmSeeder.ts. Keeping duplicate definitions risks accidental divergence when values need to be updated. Since no files import these constants fromqueueManipulation.ts, removal is safe.🤖 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.ts` at line 71, Remove the dead duplicated constants from queueManipulation.ts: delete LASTFM_SCORE_BOOST, LASTFM_SEED_COUNT, and MAX_SIMILAR_LOOKUPS (they were originally declared alongside other constants at the top of the file). Verify no functions in queueManipulation.ts (e.g., any functions that reference last.fm logic or seed generation) use these symbols and that lastFmSeeder.ts is the single source of truth; after removal, run a typecheck/build to ensure no remaining references and update any tests if they relied on the local definitions.packages/bot/src/utils/music/autoplay/candidateScorer.ts (1)
131-152: 💤 Low value
ScoringContext.genreContextduplicates the exportedGenreContextinterface.
GenreContextis exported on lines 57–68 with the exact same shape (candidateTags?,currentTrackTags?,sessionGenreFamilies?) thatScoringContext.genreContextredeclares inline. ReusingGenreContextkeeps the public surface in sync if a field is later added/renamed, and it's also what callers will likely want to import directly when they need to construct one.♻️ Suggested cleanup
sessionMood?: SessionMood | null skipNoveltyBoost?: boolean - genreContext?: { - candidateTags?: string[] - currentTrackTags?: string[] - sessionGenreFamilies?: Set<string> - } + genreContext?: GenreContext }🤖 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/candidateScorer.ts` around lines 131 - 152, ScoringContext currently redeclares the same shape inline as the exported GenreContext; change the ScoringContext property genreContext to use the existing exported GenreContext type instead of an inline object literal (keep it optional and nullable exactly as before), and remove the duplicated inline shape so callers use the single exported GenreContext definition; update any local imports/exports if needed to reference GenreContext and ensure TypeScript compiles.packages/bot/src/utils/music/autoplay/lastFmSeeder.ts (1)
100-249: 🏗️ Heavy liftThree near-identical scoring/upsert blocks in this function — extract a helper.
The seed loop (lines 106–139), similar-track loop (lines 145–187), and the new sparse-artist fallback (lines 199–247) all run essentially the same pipeline: filter → dedupe by
excludedUrls/excludedKeys→ reject bydislikedWeights > 0.5→ fetch artist tags → callcalculateRecommendationScore({...skipNoveltyBoost: true, genreContext})→upsertScoredCandidate. The differences are limited to (a) the score boost formula and (b) the reason suffix.This is mostly a maintainability concern, but it's also where bugs hide — e.g., the
LASTFM_SCORE_BOOSTadjustment in this PR has to be checked at three call sites, and only the first two apply the newlovedBoost. Worth lifting into a singlescoreAndUpsertLastFmTracks(tracks, { boost, reasonSuffix })helper before this grows further.🤖 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/lastFmSeeder.ts` around lines 100 - 249, Extract a helper (e.g., scoreAndUpsertLastFmTracks) that accepts (tracks, {boost, reasonSuffix, skipNoveltyBoost = true, applyLovedBoost = false, queue, requestedBy}) and performs the common pipeline: filter via shouldIncludeCandidate using excludedUrls/excludedKeys, dedupe/normalize with normalizeTrackKey, check dislikedWeights (>0.5), fetch artist tags via getArtistTags, call calculateRecommendationScore with skipNoveltyBoost and genreContext, then call upsertScoredCandidate on the shared candidates map with score computed as rec.score + boost (+ lovedBoost if applyLovedBoost) and reason concatenated with reasonSuffix; replace the three repeated blocks (the seed loop block that needs lovedBoost, the similar-track block that multiplies by s.match/100, and the sparse-artist fallback) to call this helper with the appropriate boost and reasonSuffix and preserve existing parameters like queue, requestedBy, currentTrack, recentArtists, likedWeights, preferredArtistKeys, blockedArtistKeys, autoplayMode, artistFrequency, implicitDislikeKeys, implicitLikeKeys, dislikedWeights, sessionMood, currentTrackTags, sessionGenreFamilies, and AUTOPLAY_BUFFER_SIZE checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.context/query-context.md:
- Around line 43-54: The committed .context/query-context.md contains local
worktree paths like ".worktrees/lucky-artists-fix/packages/..." which leak
developer-specific directories; update the file so all paths start from the
canonical workspace (e.g., "packages/...") by regenerating
.context/query-context.md from the root workspace, or if this file is intended
to be local-only, add ".context/query-context.md" to .gitignore and remove the
committed copy; locate references to ".worktrees/lucky-artists-fix" in
.context/query-context.md and replace/regenerate them accordingly.
In `@docs/specs/2026-04-25-backlog-map/spec.md`:
- Around line 29-34: The fenced code block containing the Unblocks dependency
graph (the block whose first line is "A.1 ─▶ A.4 (hono family bumps include
node-server in `#785/`#786)" and subsequent lines A.2, E.1, B.1) is missing a
language specifier; update the opening backticks to include a language such as
text or plaintext (e.g., change ``` to ```text) so markdownlint passes and
rendering is consistent.
In `@packages/bot/src/utils/music/autoplay/candidateScorer.spec.ts`:
- Around line 525-566: The two failing tests call calculateRecommendationScore
positionally but the function now accepts a single ScoringContext object; update
both calls to pass an object (ctx) with the same named properties used in the
other tests (e.g. candidateTrack: candidate, currentTrack: current,
recentArtists, likedWeights, dislikedCandidates: new Set(), likedCandidates: new
Set(), mode: 'similar' or 'popular', mood: moodWithSkips or null, and any extra
meta like candidateTags/currentTrackTags) so they match the new ScoringContext
shape and compile against calculateRecommendationScore.
In `@packages/bot/src/utils/music/autoplay/candidateScorer.ts`:
- Around line 35-36: Replace the hardcoded literal 40 in candidateScorer.ts with
the TEMPO_DELTA_LARGE constant so the code uses TEMPO_DELTA_LARGE alongside
TEMPO_DELTA_SMALL (i.e., change the occurrence of the literal 40 to
TEMPO_DELTA_LARGE), or if TEMPO_DELTA_LARGE is unnecessary remove its
declaration and consistently use TEMPO_DELTA_SMALL where intended; ensure
references to TEMPO_DELTA_LARGE and TEMPO_DELTA_SMALL are consistent so the two
values stay in sync.
- Around line 11-48: The SCORE_* block contains unused constants
(SCORE_SAME_GENRE_HINT, SCORE_POPULAR_RESTLESS, SCORE_BLOCKED_ARTIST_PREFERRED,
POPULAR_TRACK_THRESHOLD, TEMPO_DELTA_LARGE) and several constants being reused
for unrelated semantics (e.g., SCORE_SAME_ARTIST, SCORE_IMPLICIT_DISLIKE,
SCORE_DURATION_MATCH, SCORE_FREQUENT_ARTIST, SCORE_RECENT_ARTIST,
SCORE_ACOUSTICNESS_MATCH). Remove the dead constants, and for each reused
constant create semantically-named constants (e.g., FAVORITE_ARTIST_BOOST,
SAME_ARTIST_NOVELTY_PENALTY, SESSION_DURATION_BONUS, LONG_TRACK_PENALTY,
SAME_SOURCE_BOOST, PREFER_SHORT_ACOUSTICNESS) or inline literals at the
off-purpose call sites; update all references in candidateScorer (search for
usages of SCORE_SAME_ARTIST, SCORE_IMPLICIT_DISLIKE, SCORE_DURATION_MATCH,
SCORE_FREQUENT_ARTIST, SCORE_RECENT_ARTIST, SCORE_ACOUSTICNESS_MATCH,
TEMPO_DELTA_LARGE) to the new names or inlined values so each constant maps 1:1
to a single concept. Ensure tests/type checks still pass after renames.
---
Outside diff comments:
In `@packages/bot/src/utils/music/autoplay/candidateScorer.spec.ts`:
- Around line 841-935: There is a stray closing brace that prematurely ends the
describe('enrichWithAudioFeatures') block, which leaves the five
tempo/acousticness tests (e.g., "applies tempo drastic change penalty...",
"boosts track with high acousticness...", "penalizes extreme acousticness
swing...", "applies continuity bonus...", "penalizes acoustic candidate...") and
the "sorts results by score descending" test outside that suite; remove that
extra "})" that closes describe('enrichWithAudioFeatures') too early and instead
place the closing "})" for describe('enrichWithAudioFeatures') after the last of
those tests so all enrichWithAudioFeatures-related it() cases remain nested
inside the correct describe block.
---
Duplicate comments:
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 874-879: The URL comparison in the tracks.findIndex call can match
when both t.url and track.url are falsy; update the predicate used by trackIndex
so the URL branch is guarded by truthiness (e.g., only evaluate t.url ===
track.url when both track.url and t.url are truthy) while keeping the existing
reference and id checks (refer to tracks.findIndex, the predicate parameters t
and track, and fields track.id / t.id / t.url / track.url); ensure
short-circuiting order remains: identity check, id equality, then guarded URL
equality.
---
Nitpick comments:
In `@packages/bot/src/utils/music/autoplay/candidateCollector.ts`:
- Around line 60-62: The fallback in candidateKey is dead because
normalizeTrackKey(title, author) already returned '::' and calling it again
won't help; update the fallback to use a stable per-track identifier instead of
re-calling normalizeTrackKey. In the candidateKey expression (variable
candidateKey that currently uses normalizedKey !== '::' ? normalizedKey :
(candidate.id || candidate.url || normalizeTrackKey(...))), replace the final
normalizeTrackKey(...) leg with a stable value such as String(candidate.duration
?? '') or candidate.source (e.g., candidate.id || candidate.url ||
String(candidate.duration ?? candidate.source ?? '') ) so each track gets a
meaningful fallback key.
In `@packages/bot/src/utils/music/autoplay/candidateScorer.ts`:
- Around line 131-152: ScoringContext currently redeclares the same shape inline
as the exported GenreContext; change the ScoringContext property genreContext to
use the existing exported GenreContext type instead of an inline object literal
(keep it optional and nullable exactly as before), and remove the duplicated
inline shape so callers use the single exported GenreContext definition; update
any local imports/exports if needed to reference GenreContext and ensure
TypeScript compiles.
In `@packages/bot/src/utils/music/autoplay/diversitySelector.spec.ts`:
- Around line 457-518: The new describe block describe('same-album soft
penalty', ...) is over-indented compared to its sibling
describe('diversitySelector', ...) children (e.g.,
purgeDuplicatesOfCurrentTrack); adjust the leading spaces so the
describe('same-album soft penalty', ...) block and its inner tests are indented
to match the other sibling blocks (4 spaces from the parent) to restore
consistent file indentation.
- Around line 477-492: The test "second track from same album is penalised by
0.12" currently only checks selected titles but not the actual penalty; update
the test that calls selectDiverseCandidates (and uses makeCandidate) to assert
Track B's resulting jitteredScore is approximately 0.85 - 0.12 (allowing the
existing jitter tolerance) or alternatively rename the test to match the weaker
assertion; specifically, locate the test block and either (A) find the selected
entry for id 'b' (or from selected.map(s => s.track.title) locate the object)
and add an expect on its jitteredScore to be close to 0.73 within the jitter
delta, or (B) change the test title to something like "selects tracks allowing
same-album when artist limit permits" if you prefer not to assert scores.
In `@packages/bot/src/utils/music/autoplay/lastFmSeeder.ts`:
- Around line 100-249: Extract a helper (e.g., scoreAndUpsertLastFmTracks) that
accepts (tracks, {boost, reasonSuffix, skipNoveltyBoost = true, applyLovedBoost
= false, queue, requestedBy}) and performs the common pipeline: filter via
shouldIncludeCandidate using excludedUrls/excludedKeys, dedupe/normalize with
normalizeTrackKey, check dislikedWeights (>0.5), fetch artist tags via
getArtistTags, call calculateRecommendationScore with skipNoveltyBoost and
genreContext, then call upsertScoredCandidate on the shared candidates map with
score computed as rec.score + boost (+ lovedBoost if applyLovedBoost) and reason
concatenated with reasonSuffix; replace the three repeated blocks (the seed loop
block that needs lovedBoost, the similar-track block that multiplies by
s.match/100, and the sparse-artist fallback) to call this helper with the
appropriate boost and reasonSuffix and preserve existing parameters like queue,
requestedBy, currentTrack, recentArtists, likedWeights, preferredArtistKeys,
blockedArtistKeys, autoplayMode, artistFrequency, implicitDislikeKeys,
implicitLikeKeys, dislikedWeights, sessionMood, currentTrackTags,
sessionGenreFamilies, and AUTOPLAY_BUFFER_SIZE checks.
In `@packages/bot/src/utils/music/autoplay/lastFmSeeds.spec.ts`:
- Around line 560-581: The test currently only checks slice.length <= 3 which is
too weak; update the test that calls consumeBlendedSeedSlice(['heavy','light'],
3, weights) to assert the exact trimmed behavior by expecting slice.length === 3
and (optionally) verifying the per-seed composition—e.g., that 'heavy'
contributed 3 items before trimming or that the returned slice contains the
expected tracks from getTopTracksMock for 'heavy' and 'light'; locate and modify
the test around consumeBlendedSeedSlice, getByDiscordIdMock and getTopTracksMock
to add these stricter assertions so the overshoot trim branch is actually
exercised.
In `@packages/bot/src/utils/music/autoplay/sessionMood.ts`:
- Line 9: The SessionMood shape currently declares recentSkipCount as optional
but detectSessionMood always sets it (defaulting to 0); change the SessionMood
type to make recentSkipCount required (recentSkipCount: number) and ensure
detectSessionMood still assigns a numeric value, and update any callers or type
assertions that expected undefined; locate the SessionMood interface and the
detectSessionMood function to make these coordinated changes so the runtime
contract and type signature match.
- Around line 116-119: The comment for the aggressive-skip branch is misleading:
it reads "relax mood when user is skipping aggressively" but the code sets
restless = true; update the comment to match behavior (e.g., "force restless
mood when user is skipping aggressively" or similar) near the conditional that
checks recentSkipCount and sets restless in sessionMood.ts so the comment and
the variables recentSkipCount and restless reflect the actual intent.
In `@packages/bot/src/utils/music/autoplay/spotifyRecommender.ts`:
- Around line 184-196: In searchSeedCandidates, replace the inline type import
"import('./sessionMood').SessionMood" with the already-imported SessionMood type
and, since only the restless flag is used, narrow the parameter to either
SessionMood | null or a smaller shape like { restless: boolean } | null; update
the function signature parameter name (sessionMood) to use that type so the code
continues to reference sessionMood.restless without the redundant inline import.
In `@packages/bot/src/utils/music/queueManipulation.ts`:
- Around line 822-823: The import of calculateRecommendationScore is mid-file
which splits imports; either move the import statement for
calculateRecommendationScore to the top-level import block alongside the other
imports, or remove this re-export (export { calculateRecommendationScore }) and
update callers to import calculateRecommendationScore directly from
./autoplay/candidateScorer; locate the calculateRecommendationScore symbol and
the export line in queueManipulation.ts to apply the chosen change.
- Line 71: Remove the dead duplicated constants from queueManipulation.ts:
delete LASTFM_SCORE_BOOST, LASTFM_SEED_COUNT, and MAX_SIMILAR_LOOKUPS (they were
originally declared alongside other constants at the top of the file). Verify no
functions in queueManipulation.ts (e.g., any functions that reference last.fm
logic or seed generation) use these symbols and that lastFmSeeder.ts is the
single source of truth; after removal, run a typecheck/build to ensure no
remaining references and update any tests if they relied on the local
definitions.
🪄 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: 9138d975-b355-480c-8449-227e2926eb33
📒 Files selected for processing (20)
.claudeignore.context/lucky-rag-context.md.context/query-context.md.context/session.json.github/copilot-instructions.mddocs/specs/2026-04-25-backlog-map/spec.mddocs/specs/2026-04-25-backlog-map/tasks.mddocs/specs/2026-05-04-backlog-map/index.mdpackages/bot/src/functions/music/commands/stop.spec.tspackages/bot/src/utils/music/autoplay/candidateCollector.tspackages/bot/src/utils/music/autoplay/candidateScorer.spec.tspackages/bot/src/utils/music/autoplay/candidateScorer.tspackages/bot/src/utils/music/autoplay/diversitySelector.spec.tspackages/bot/src/utils/music/autoplay/lastFmSeeder.tspackages/bot/src/utils/music/autoplay/lastFmSeeds.spec.tspackages/bot/src/utils/music/autoplay/sessionMood.spec.tspackages/bot/src/utils/music/autoplay/sessionMood.tspackages/bot/src/utils/music/autoplay/spotifyRecommender.tspackages/bot/src/utils/music/queueManipulation.tspackages/shared/src/utils/error/logAndRethrow.spec.ts
✅ Files skipped from review due to trivial changes (3)
- .context/session.json
- .claudeignore
- docs/specs/2026-05-04-backlog-map/index.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Prisma models for database schema definitions and type safety — all database operations should leverage Prisma ORM types from
generated/prisma/clientUse TypeScript interfaces for data contracts and type definitions in service APIs — avoid using
anytypeFeature toggles should be checked using
FeatureToggleService.isEnabledForGuild()orisEnabledGlobal()before executing feature-specific codeUse
Result<T>type fromtypes/common/BaseResultfor all async operations returning success/failure outcomesImplement error handling using
errorHandler.tsutilities — usewrapError(),logAndRethrow(), orhandleError()for consistent error processingUse camelCase for variable and function names in TypeScript files
Use PascalCase for class names and type names in TypeScript
Use UPPER_SNAKE_CASE for constants in TypeScript files
Use async/await for all Promise-based operations — avoid
.then()callback chainsUse type guards from
utils/guards.tsfor runtime type validation — implement as pure functions returning boolean type predicates
Files:
packages/bot/src/utils/music/autoplay/sessionMood.tspackages/shared/src/utils/error/logAndRethrow.spec.tspackages/bot/src/utils/music/autoplay/candidateCollector.tspackages/bot/src/utils/music/autoplay/diversitySelector.spec.tspackages/bot/src/functions/music/commands/stop.spec.tspackages/bot/src/utils/music/autoplay/lastFmSeeds.spec.tspackages/bot/src/utils/music/autoplay/lastFmSeeder.tspackages/bot/src/utils/music/autoplay/candidateScorer.spec.tspackages/bot/src/utils/music/autoplay/sessionMood.spec.tspackages/bot/src/utils/music/autoplay/candidateScorer.tspackages/bot/src/utils/music/autoplay/spotifyRecommender.tspackages/bot/src/utils/music/queueManipulation.ts
packages/bot/src/utils/music/autoplay/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Music autoplay functions should enrich candidates with audio features before scoring — use
enrichWithAudioFeatures()to fetch Spotify audio dataDetect session mood using language heuristics and historical track genres — implement via
detectSessionMood()for context-aware recommendations
Files:
packages/bot/src/utils/music/autoplay/sessionMood.tspackages/bot/src/utils/music/autoplay/candidateCollector.tspackages/bot/src/utils/music/autoplay/diversitySelector.spec.tspackages/bot/src/utils/music/autoplay/lastFmSeeds.spec.tspackages/bot/src/utils/music/autoplay/lastFmSeeder.tspackages/bot/src/utils/music/autoplay/candidateScorer.spec.tspackages/bot/src/utils/music/autoplay/sessionMood.spec.tspackages/bot/src/utils/music/autoplay/candidateScorer.tspackages/bot/src/utils/music/autoplay/spotifyRecommender.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: LucasSantana-Dev/Lucky
Timestamp: 2026-05-05T21:07:43.299Z
Learning: Generate Prisma client code automatically via `node gen-context.js` before asking questions about code — use `sigmap ask` command to document dependencies
Learnt from: CR
Repo: LucasSantana-Dev/Lucky
Timestamp: 2026-05-05T21:07:43.299Z
Learning: Structure services with single responsibility — each service class should handle one domain entity (e.g., `AutoMessageService` for auto messages, `EmbedBuilderService` for embeds)
Learnt from: CR
Repo: LucasSantana-Dev/Lucky
Timestamp: 2026-05-05T21:07:43.299Z
Learning: Import shared utilities and services from `packages/shared` — use relative imports within a package, absolute imports for cross-package dependencies
Learnt from: CR
Repo: LucasSantana-Dev/Lucky
Timestamp: 2026-05-05T21:07:43.299Z
Learning: Use Redis operations for caching and temporary state — leverage `GuildSettingsService` for guild-level configuration caching
Learnt from: CR
Repo: LucasSantana-Dev/Lucky
Timestamp: 2026-05-05T21:07:43.299Z
Learning: Use SigMap `sigmap ask` command before answering questions about code dependencies — validate AI responses with `sigmap judge --response <file>`
🪛 LanguageTool
docs/specs/2026-04-25-backlog-map/tasks.md
[style] ~31-~31: To form a complete sentence, be sure to include a subject.
Context: ...no_ci.md. ## Notes - pnpm outdated` could not be run cleanly (root invocation def...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.22.1)
.context/lucky-rag-context.md
[warning] 22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 46-46: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 261-261: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 329-329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 384-384: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
.context/query-context.md
[warning] 5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/specs/2026-04-25-backlog-map/spec.md
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (5)
docs/specs/2026-04-25-backlog-map/spec.md (1)
1-47: Well-structured backlog map documentation.The spec provides a clear Top-10 prioritization with impact/effort scoring, dependency tracking, and a defined re-evaluation cadence. The structure follows the template parity noted in the Anchor section, and references to evidence and prior backlogs are properly documented.
docs/specs/2026-04-25-backlog-map/tasks.md (1)
1-39: Clear task tracking with proper status documentation.The tasks file effectively tracks the Top-10 backlog items with checkboxes, provides context in the Done and Notes sections, and properly references superseded documentation. The structure supports iterative backlog management and progress tracking.
packages/shared/src/utils/error/logAndRethrow.spec.ts (1)
1-92: LGTM.Comprehensive coverage of the three error helpers (rethrow vs. swallow vs. warn paths), including non-Error wrapping, data passthrough, and stack inclusion for
logAndWarn. Mocks are isolated correctly withjest.clearAllMocks()inbeforeEach.packages/bot/src/functions/music/commands/stop.spec.ts (2)
12-16: ⚡ Quick winThe mock path correctly matches the production import.
The import in
stop.tsline 12 isimport { musicSessionSnapshotService } from '../../../utils/music/sessionSnapshots', which matches the mocked module path exactly. The mock will apply correctly, and no changes are needed.
81-100: ⚡ Quick winThe production code correctly awaits
musicSessionSnapshotService.deleteSnapshot()before invokingqueue?.node.stop(), so the test's order assertion is valid and will not be flaky.
| ## .worktrees/lucky-artists-fix/packages/shared/src/services/database/DatabaseInitializationService.ts | ||
| ``` | ||
| export interface DatabaseInitializationResult | ||
| export interface DatabaseServiceStatus | ||
| export class DatabaseInitializationService | ||
| async initialize() → Promise<boolean> | ||
| async getServiceStatus() → Promise<DatabaseServ | ||
| getDatabaseService() → DatabaseService | nu | ||
| async shutdown() → Promise<void> | ||
| ``` | ||
|
|
||
| ## .worktrees/lucky-artists-fix/packages/shared/src/services/guildAutomation/types.ts |
There was a problem hiding this comment.
Committed paths leak a local worktree.
Lines 43 and 54 reference .worktrees/lucky-artists-fix/packages/..., which is a path inside a developer's local Git worktree (.worktrees/...). Other contributors and CI won't have that directory, so the doc points at non-existent files for everyone but the author. Regenerate .context/query-context.md from the canonical workspace (so paths start with packages/...) before committing, or add .context/query-context.md to .gitignore if it's intended to be developer-local SigMap output.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 44-44: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 @.context/query-context.md around lines 43 - 54, The committed
.context/query-context.md contains local worktree paths like
".worktrees/lucky-artists-fix/packages/..." which leak developer-specific
directories; update the file so all paths start from the canonical workspace
(e.g., "packages/...") by regenerating .context/query-context.md from the root
workspace, or if this file is intended to be local-only, add
".context/query-context.md" to .gitignore and remove the committed copy; locate
references to ".worktrees/lucky-artists-fix" in .context/query-context.md and
replace/regenerate them accordingly.
| ``` | ||
| A.1 ─▶ A.4 (hono family bumps include node-server in #785/#786) | ||
| A.2 ─▶ I.3 (release notes + top.gg credibility) | ||
| E.1 ─▶ B.1 (centralized veto removes one concern from the split review) | ||
| B.1 ─▶ B.4 (clear API for autoplay command surface to flatten against) | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to the fenced code block.
The Unblocks dependency graph is missing a language specifier. Add text or plaintext to satisfy markdownlint and improve rendering consistency.
📝 Proposed fix
-```
+```text
A.1 ─▶ A.4 (hono family bumps include node-server in `#785/`#786)
A.2 ─▶ I.3 (release notes + top.gg credibility)
E.1 ─▶ B.1 (centralized veto removes one concern from the split review)
B.1 ─▶ B.4 (clear API for autoplay command surface to flatten against)
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 29-29: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/specs/2026-04-25-backlog-map/spec.md` around lines 29 - 34, The fenced
code block containing the Unblocks dependency graph (the block whose first line
is "A.1 ─▶ A.4 (hono family bumps include node-server in `#785/`#786)" and
subsequent lines A.2, E.1, B.1) is missing a language specifier; update the
opening backticks to include a language such as text or plaintext (e.g., change
``` to ```text) so markdownlint passes and rendering is consistent.
| const SCORE_SAME_ARTIST = 0.3 | ||
| const SCORE_SAME_GENRE_HINT = 0.3 | ||
| const SCORE_POPULAR_ARTIST = 0.2 | ||
| const SCORE_PREFERRED_ARTIST = 0.3 | ||
| const SCORE_LIKED_WEIGHT_MULTIPLIER = 0.3 | ||
| const SCORE_DISLIKED_PENALTY = -0.3 | ||
| const SCORE_IMPLICIT_DISLIKE = -0.35 | ||
| const SCORE_IMPLICIT_LIKE = 0.25 | ||
| const SCORE_TITLE_SIM_BONUS = 0.12 | ||
| const TITLE_SIM_THRESHOLD = 0.4 | ||
| const SCORE_RECENT_ARTIST = -0.25 | ||
| const SCORE_DURATION_MATCH = 0.15 | ||
| const SCORE_DURATION_BONUS_THRESHOLD = 0.05 | ||
| const SCORE_ACOUSTICNESS_MATCH = 0.1 | ||
| const SCORE_POPULAR_RESTLESS = 0.08 | ||
| const SCORE_BLOCKED_ARTIST = -0.4 | ||
| const SCORE_BLOCKED_ARTIST_PREFERRED = 0.25 | ||
| const SCORE_FREQUENT_ARTIST = -0.2 | ||
| const SCORE_GENRE_TAG_MAX = 0.2 | ||
| const SCORE_GENRE_TAG_PER_MATCH = 0.05 | ||
| const SCORE_LIKED_ARTIST_WEIGHT = 0.2 | ||
| const SCORE_LIKED_ARTIST_TEMPO = 0.1 | ||
| const SCORE_TEMPO_PENALTY_LARGE = -0.15 | ||
| const SCORE_TEMPO_PENALTY_SMALL = -0.07 | ||
| const TEMPO_DELTA_LARGE = 40 | ||
| const TEMPO_DELTA_SMALL = 25 | ||
| const DURATION_RATIO_TIGHT_LOW = 0.8 | ||
| const DURATION_RATIO_TIGHT_HIGH = 1.2 | ||
| const DURATION_RATIO_LOOSE_LOW = 0.7 | ||
| const DURATION_RATIO_LOOSE_HIGH = 1.3 | ||
| const SCORE_DURATION_RATIO_TIGHT = 0.15 | ||
| const SCORE_DURATION_RATIO_LOOSE = 0.05 | ||
| const GENRE_PENALTY_STRONG = -0.6 | ||
| const GENRE_PENALTY_WEAK = -0.3 | ||
| const GENRE_PENALTY_UNKNOWN = -0.1 | ||
| const SCORE_SPOTIFY_PREFERRED = 0.4 | ||
| const DISLIKE_WEIGHT_THRESHOLD = 0.5 | ||
| const POPULAR_TRACK_THRESHOLD = 50 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Several extracted constants are dead, and several others are reused at semantically unrelated sites.
A quick audit of the new SCORE_* block versus its call sites:
Dead constants (declared, never referenced in this file):
SCORE_SAME_GENRE_HINT(line 12)SCORE_POPULAR_RESTLESS(line 25)SCORE_BLOCKED_ARTIST_PREFERRED(line 27)POPULAR_TRACK_THRESHOLD(line 48)TEMPO_DELTA_LARGE(line 35) — flagged separately
Misleadingly named (the constant is reused for the magnitude, not the concept):
SCORE_SAME_ARTIST = 0.3is used as the favourite-artist boost at line 251 (freq >= 5), not for same-artist matching.SCORE_IMPLICIT_DISLIKE = -0.35is used as the same-artist novelty penalty at line 287.SCORE_DURATION_MATCH = 0.15is used for session novelty (line 297), deep-dive (line 341), and energy/valence proximity (line 505) — three unrelated concepts.SCORE_FREQUENT_ARTIST = -0.2is used for long-track penalty (line 331), version variant penalty (line 395), and recent-artist penalty in discover mode (line 416).SCORE_RECENT_ARTIST = -0.25is used as a same-source boost at line 304.SCORE_ACOUSTICNESS_MATCH = 0.1is used for preferShort match (line 350), restless discovery (line 355), and acousticness scoring (lines 509/526/531).
The visible problem: tweaking, say, the same-artist novelty penalty by editing SCORE_IMPLICIT_DISLIKE will silently shift the implicit-dislike weight too. Either rename each constant after the dominant call site and inline a literal at the off-purpose sites, or split each concept into its own constant.
🤖 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/candidateScorer.ts` around lines 11 -
48, The SCORE_* block contains unused constants (SCORE_SAME_GENRE_HINT,
SCORE_POPULAR_RESTLESS, SCORE_BLOCKED_ARTIST_PREFERRED, POPULAR_TRACK_THRESHOLD,
TEMPO_DELTA_LARGE) and several constants being reused for unrelated semantics
(e.g., SCORE_SAME_ARTIST, SCORE_IMPLICIT_DISLIKE, SCORE_DURATION_MATCH,
SCORE_FREQUENT_ARTIST, SCORE_RECENT_ARTIST, SCORE_ACOUSTICNESS_MATCH). Remove
the dead constants, and for each reused constant create semantically-named
constants (e.g., FAVORITE_ARTIST_BOOST, SAME_ARTIST_NOVELTY_PENALTY,
SESSION_DURATION_BONUS, LONG_TRACK_PENALTY, SAME_SOURCE_BOOST,
PREFER_SHORT_ACOUSTICNESS) or inline literals at the off-purpose call sites;
update all references in candidateScorer (search for usages of
SCORE_SAME_ARTIST, SCORE_IMPLICIT_DISLIKE, SCORE_DURATION_MATCH,
SCORE_FREQUENT_ARTIST, SCORE_RECENT_ARTIST, SCORE_ACOUSTICNESS_MATCH,
TEMPO_DELTA_LARGE) to the new names or inlined values so each constant maps 1:1
to a single concept. Ensure tests/type checks still pass after renames.
| const TEMPO_DELTA_LARGE = 40 | ||
| const TEMPO_DELTA_SMALL = 25 |
There was a problem hiding this comment.
TEMPO_DELTA_LARGE is declared but never read — line 514 hardcodes 40 instead.
You added TEMPO_DELTA_LARGE = 40 (line 35) right next to TEMPO_DELTA_SMALL = 25, and TEMPO_DELTA_SMALL is correctly used at line 515. But line 514 still uses the literal 40. The two values now have to be kept in sync by hand — exactly what extracting the constant was supposed to prevent.
🛡️ Suggested fix
- if (tempoDelta > 40) track.score += SCORE_TEMPO_PENALTY_LARGE
+ if (tempoDelta > TEMPO_DELTA_LARGE) track.score += SCORE_TEMPO_PENALTY_LARGE
else if (tempoDelta > TEMPO_DELTA_SMALL) track.score += SCORE_TEMPO_PENALTY_SMALLAlso applies to: 512-516
🤖 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/candidateScorer.ts` around lines 35 -
36, Replace the hardcoded literal 40 in candidateScorer.ts with the
TEMPO_DELTA_LARGE constant so the code uses TEMPO_DELTA_LARGE alongside
TEMPO_DELTA_SMALL (i.e., change the occurrence of the literal 40 to
TEMPO_DELTA_LARGE), or if TEMPO_DELTA_LARGE is unnecessary remove its
declaration and consistently use TEMPO_DELTA_SMALL where intended; ensure
references to TEMPO_DELTA_LARGE and TEMPO_DELTA_SMALL are consistent so the two
values stay in sync.
…all sites in candidateFallback
…RE_BLOCKED_ARTIST_PREFERRED constants
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bot/src/utils/music/autoplay/candidateScorer.ts (1)
222-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip-storm relaxation never runs once the session-family veto is active.
The new
recentSkipCountrelaxation only affects the softfamilyPenalty, but the scorer hard-rejects cross-family candidates earlier at Lines 232-236. In the exact case wheresessionGenreFamiliesis populated, a skip storm still returns-Infinity, so autoplay never gets the broader candidate pool this branch is trying to unlock.Suggested fix
+ const inSkipStorm = (sessionMood?.recentSkipCount ?? 0) >= 3 + - if (sessionGenreFamilies.size > 0 && candidateTags.length > 0) { + if (!inSkipStorm && sessionGenreFamilies.size > 0 && candidateTags.length > 0) { const candidateFamilies = getGenreFamilies(candidateTags) if (candidateFamilies.size > 0) { let intersects = false for (const family of candidateFamilies) { if (sessionGenreFamilies.has(family)) { @@ - if (sessionMood?.recentSkipCount && sessionMood.recentSkipCount >= 3) { + if (inSkipStorm) { familyPenalty *= 0.5 }Also applies to: 375-378
🤖 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/candidateScorer.ts` around lines 222 - 239, The hard veto on cross-family candidates in the candidate scoring logic (the block that computes candidateFamilies = getGenreFamilies(candidateTags) and returns score: -Infinity with reason 'cross-genre: family drift from session' when no intersection with sessionGenreFamilies) prevents the skip-storm relaxation from ever running; instead relax that hard-reject to allow skip-storms to bypass it by checking recentSkipCount (the same relaxation applied to familyPenalty) before returning -Infinity: if recentSkipCount exceeds the skip-storm threshold, skip the early return and let scoring continue (apply a softened familyPenalty or mark candidate as allowed), and apply the same change to the duplicate block at the other location that also returns -Infinity (the second candidateFamilies/sessionGenreFamilies veto around lines noted in the review).
🤖 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/autoplay/candidateScorer.ts`:
- Around line 283-295: When sessionMood.deepDiveArtist equals the
candidateArtist, do not apply the same-artist implicit penalty
(SCORE_IMPLICIT_DISLIKE) and do not treat that candidate as novel (i.e. skip
adding SCORE_DURATION_MATCH / "session novelty"); instead award a dedicated
deep-dive boost (add a SCORE_DEEP_DIVE_BONUS constant or reuse/raise
SCORE_TITLE_SIM_BONUS) and push a corresponding reason into reasons; implement
this check around the candidateArtist === currentArtist branch and the later
novelty branch (where skipNoveltyBoost and recentArtists are consulted) so
deep-dive candidates are net-favored over novel-artist bonuses.
In `@packages/bot/src/utils/music/candidateFallback.ts`:
- Around line 51-64: The fallback scorers call calculateRecommendationScore
without genreContext, allowing cross-genre drift; update CandidateContext and
collectBroadFallbackCandidates to include the session/current-track genreContext
and thread that into the calculateRecommendationScore call(s) at lines shown,
and for addGenreTrackCandidate use the seed tag (candidateTags) as the
candidateTags/genreContext hint when constructing the CandidateContext before
scoring; ensure all calls to calculateRecommendationScore in this file now
receive the new genreContext field from CandidateContext so genre-based
vetoes/penalties are applied consistently.
In `@packages/bot/src/utils/music/queueEditOps.ts`:
- Around line 215-220: The current tracks.findIndex(...) in computing trackIndex
picks the first matching duplicate, which can select an older autoplay entry
instead of the newly queued manual track; change the logic in the trackIndex
computation (where tracks.findIndex is used) to find the newest matching entry
instead — e.g. use Array.prototype.findLastIndex with the same predicate
(matching by identity, track.id, or track.url) or, if findLastIndex isn't
available, scan tracks from the end toward the start and pick the first match so
the most recently queued copy is selected.
---
Outside diff comments:
In `@packages/bot/src/utils/music/autoplay/candidateScorer.ts`:
- Around line 222-239: The hard veto on cross-family candidates in the candidate
scoring logic (the block that computes candidateFamilies =
getGenreFamilies(candidateTags) and returns score: -Infinity with reason
'cross-genre: family drift from session' when no intersection with
sessionGenreFamilies) prevents the skip-storm relaxation from ever running;
instead relax that hard-reject to allow skip-storms to bypass it by checking
recentSkipCount (the same relaxation applied to familyPenalty) before returning
-Infinity: if recentSkipCount exceeds the skip-storm threshold, skip the early
return and let scoring continue (apply a softened familyPenalty or mark
candidate as allowed), and apply the same change to the duplicate block at the
other location that also returns -Infinity (the second
candidateFamilies/sessionGenreFamilies veto around lines noted in the review).
🪄 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: 323c5e18-4579-46d9-92ac-f6cb8b555cc8
📒 Files selected for processing (5)
packages/bot/src/utils/music/autoplay/candidateScorer.spec.tspackages/bot/src/utils/music/autoplay/candidateScorer.tspackages/bot/src/utils/music/candidateFallback.tspackages/bot/src/utils/music/queueEditOps.tssonar-project.properties
✅ Files skipped from review due to trivial changes (1)
- sonar-project.properties
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
interfacefor defining object shapes in TypeScript rather thantypealiasesUse PascalCase for class and interface names in TypeScript
Always use async/await for promise handling in TypeScript rather than .then() chains
Files:
packages/bot/src/utils/music/queueEditOps.tspackages/bot/src/utils/music/autoplay/candidateScorer.tspackages/bot/src/utils/music/autoplay/candidateScorer.spec.tspackages/bot/src/utils/music/candidateFallback.ts
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable, function, and property names in TypeScript/JavaScript
Use UPPER_SNAKE_CASE for exported constants
Files:
packages/bot/src/utils/music/queueEditOps.tspackages/bot/src/utils/music/autoplay/candidateScorer.tspackages/bot/src/utils/music/autoplay/candidateScorer.spec.tspackages/bot/src/utils/music/candidateFallback.ts
**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Result wrapper type for all operations that can fail, avoiding throw-based error handling
Feature toggle checks should use FeatureToggleService.isEnabledForGuild() or isEnabledGlobal() methods
Error handling should use type-safe error classes (MusicError, GuildAutomationError, etc.) with specific error codes
All async operations should include timeout handling to prevent indefinite waiting
Environment configuration should be loaded via loadEnvironment() or loadEnvironmentAsync() functions, not process.env directly
All user-facing error messages should be sanitized using sanitizeErrorMessage() to prevent information leakage
TypeScript files should have proper null safety checks using optional chaining (?.) and nullish coalescing (??)
Files:
packages/bot/src/utils/music/queueEditOps.tspackages/bot/src/utils/music/autoplay/candidateScorer.tspackages/bot/src/utils/music/autoplay/candidateScorer.spec.tspackages/bot/src/utils/music/candidateFallback.ts
packages/bot/src/utils/music/autoplay/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Autoplay queue management should use the diversitySelector to ensure variety in recommendations
Files:
packages/bot/src/utils/music/autoplay/candidateScorer.tspackages/bot/src/utils/music/autoplay/candidateScorer.spec.ts
packages/bot/src/utils/music/autoplay/candidateScorer.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Music track scores should be calculated using the candidateScorer with audio feature constraints
Files:
packages/bot/src/utils/music/autoplay/candidateScorer.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: LucasSantana-Dev/Lucky
Timestamp: 2026-05-05T23:01:59.458Z
Learning: Use the Repository pattern for data access in service layers
… fallback, findLastIndex
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
…ority + autoplay improvements * fix(music): fix artist routing, stop snapshot clear, manual track priority, and improve autoplay Bug fixes: - /artist: prefer exact/word-boundary artist match before substring; prevents "Prince" from routing to "Prince Royce" (#1) - /stop: delete Redis session snapshot on stop so autoplay tracks don't persist for the next session (#2) - moveUserTrackToPriority: add reference/ID fallback lookup so Spotify tracks (resolved to YouTube URLs at queue time) are correctly re-ordered (#3) Autoplay improvements (plan item execution): - LASTFM_SCORE_BOOST 0.0 → 0.20 so Last.fm candidates get meaningful lift over cold Spotify picks - Deep-dive sessions: raise per-artist cap to 5 (from 2) when session mood detects same-artist focus, so autoplay follows the user's intent - Cache sessionMood between replenish cycles; recompute only after 3+ new tracks to prevent per-cycle mood flips - Log hard-rejected autoplay candidates (score -Infinity/NaN) at debug level for visibility into genre-veto decisions * feat(autoplay): scoring improvements, loved seed boost, genre fallback, and skip-storm detection Plan items implemented: - 1.2: Last.fm loved seed boost — LASTFM_SCORE_BOOST 0.0→0.20; +0.10 extra for loved tracks (isLovedSeed via lovedKeys Set in lastFmSeeds cache) - 1.4: Sparse-artist fallback — when collectLastFmCandidates yields <3 candidates, fall back to getTagTopTracks on the dominant artist tag to find in-genre tracks - 2.1: Tempo continuity penalty — Δtempo>40 BPM: -0.15; >25 BPM: -0.07 in enrichWithAudioFeatures - 2.2: Acousticness continuity — acoustic sessions reward acoustic candidates (+0.10); penalise acoustic candidates in electric sessions (-0.10) - 2.3: Popularity boost in restless mood — track.raw.popularity>50 adds +0.08 when restless - 2.4: Same-album soft penalty in selectDiverseCandidates — -0.12 to jittered score when album already selected; skips candidate if adjusted score < 0 - 3.3: Duration sigmoid — 0.15*tanh((durationMs-300k)/60k) replaces flat +0.1 for preferLong - 3.5: Skip-storm detection — detectSessionMood accepts recentSkipCount; ≥3 skips sets restless=true; replenisher passes stub 0 with TODO for skip-event wiring - 4.2: Query modifier expansion — BASE_QUERY_MODIFIERS (5) vs FULL_QUERY_MODIFIERS (10); restless sessions use base-only to cycle faster through similar searches * feat(autoplay): add same-album soft penalty in diversity selection (plan 2.4) * test(bot): add lastFmSeeder.spec.ts to fix SonarCloud gate on PR #805 12 tests covering searchLastFmQuery (engine fallback, duration filter, result limit) and collectLastFmCandidates (seed selection, loved boost, dislike skip, shouldIncludeCandidate guard). * test(bot): expand lastFmSeeder coverage — similar tracks and sparse-artist fallback Adds tests for the similar-tracks loop (lines 150-192) and the sparse-artist genre fallback (lines 197-254) which were completely uncovered, bringing lastFmSeeder.ts above the 80% SonarCloud threshold. Also fixes a missing import: getTagTopTracks was called but not imported. * chore: remove unused audioFeatureCache from candidateScorer - Remove unused LRUCache import from 'lru-cache' - Remove unused AudioFeatureEntry interface - Remove duplicate audioFeatureCache definition The canonical audioFeatureCache instance lives in queueManipulation.ts and is actively used in getTrackAudioFeatures(). This removes the dead code duplicate in candidateScorer.ts that was never instantiated or used. Test Results: 202/204 test suites passed (2,855 tests) - Pre-existing failures in lifecycleHandlers and queueResolverWiring unrelated to this cache cleanup * test(bot): add queueStateManager.spec.ts with comprehensive coverage * test(bot): add youtubeErrorHandler/analyzer.spec.ts with 56 comprehensive test cases * test(bot): add titleComparison/service.spec.ts * test(bot): cover remaining lastFmSeeder branches (lines 85, 202, 215) * test(bot): add service.spec.ts for TrackManagementService (44 tests) * feat(lastfm): surface API errors at warn level instead of debug Replace logAndSwallow (debugLog) with logAndWarn (warnLog) in all Last.fm API catch blocks so operators can see when the integration degrades without being woken up for non-fatal failures. Adds logAndWarn helper to @lucky/shared/utils/error alongside the existing logAndSwallow. * refactor: remove duplicate normalizeText/normalizeTrackKey from candidateScorer Import canonical versions from queueManipulation instead of maintaining local copies. Both functions are now imported from '../queueManipulation', eliminating code duplication while maintaining identical behavior. - Remove local normalizeText() implementation (lines 54-58) - Remove local normalizeTrackKey() implementation (lines 60-66) - Add import: import { normalizeText, normalizeTrackKey } from '../queueManipulation' - candidateScorer tests: 36 passed This is part of the autoplay cleanup refactoring task. * test(shared): add logAndRethrow.spec.ts covering logAndRethrow, logAndSwallow, logAndWarn * refactor: consolidate ScoredTrack, extract scoring constants, add missing imports - Import ScoredTrack type from diversitySelector.ts in candidateScorer.ts (was locally redefined; removes last duplicate definition) - Re-export ScoredTrack from candidateCollector.ts (fixes index.ts re-export) - Add missing getTagTopTracks import in lastFmSeeder.ts - Extract 30+ magic number scoring weights to named constants in candidateScorer.ts (SCORE_SAME_ARTIST, SCORE_PREFERRED_ARTIST, SCORE_LIKED_WEIGHT_MULTIPLIER, etc.) - candidateScorer + diversitySelector + candidateCollector + lastFmSeeder tests: 49 passed * test(autoplay): add tests for recentSkipCount and same-album penalty - sessionMood.spec.ts: 4 new tests for recentSkipCount >= 3 → restless - diversitySelector.spec.ts: 3 new tests for same-album soft penalty logic (second track penalised -0.12, excluded if score < 0, no-album unaffected) - All 71 tests pass across sessionMood, diversitySelector, lastFmSeeder * test: cover candidateScorer audio-feature paths and isLovedSeed Add tests for: - enrichWithAudioFeatures tempo drastic-change penalty, acousticness bonus, acousticness swing penalty, and acoustic-session continuity paths - recentSkipCount >=3 skip-storm genre-penalty relaxation - autoplayMode=popular liked-weight boost - isLovedSeed (was not imported in spec at all — 5 uncovered lines) * refactor: replace 15-param calculateRecommendationScore with ScoringContext object * fix: address CodeRabbit review — recentSkipCount in SessionMood, deduplicate scorer, test assertions * fix: remove unused SCORE_SAME_GENRE_HINT, SCORE_POPULAR_RESTLESS, SCORE_BLOCKED_ARTIST_PREFERRED constants * chore: exclude lastFmSeeder.ts from CPD (repeated scoring loops by design) * fix: guard URL comparison in moveUserTrackToPriority; fix stray brace in scorer spec * fix: skip-storm bypasses genre veto, deep-dive boost, genreContext in fallback, findLastIndex * fix: replace findLastIndex with reverse loop (ES2021 compat)
This branch was successfully deployed
1 participant
Summary
Bug fixes:
/artist Prince→ Prince Royce: adds exact/word-boundary match pass before substring match so the user's named artist wins over partial-name matches/stopleaves autoplay queue alive: now deletes the Redis session snapshot on stop so autoplay tracks don't re-appear next sessionmoveUserTrackToPrioritynow uses reference/ID fallback in addition to URL comparison — Spotify tracks are resolved to YouTube URLs at queue time so URL-only matching was silently failingAutoplay improvements (Phase 1+3 from
.claude/plans/autoplay-improvements.md):LASTFM_SCORE_BOOSTraised from0.0→0.20— Last.fm candidates now score above cold Spotify pickssessionMood.deepDiveArtistmatches the current seed artist, so autoplay follows the user's intent instead of switching artists immediatelysessionMoodcached between replenish cycles; recomputed only after 3+ new tracks to prevent per-cycle mood flipsscore = -Infinity/NaN) now logged at debug level for visibility into genre-veto and locale-veto decisionsTest plan
/artist Princequeues Prince tracks, not Prince Royce/stopfollowed by/playstarts fresh with no leftover autoplay tracks/playqueued track appears before autoplay-seeded tracks in queueSummary by CodeRabbit
Release Notes
New Features
Bug Fixes