Repository navigation
fix(bot): resolve /album text queries to an album url - #2053
Conversation
/album with a free-text query never worked. Not intermittently: no text query could ever succeed. The command pinned QueryType.SPOTIFY_SEARCH for non-URL input, but every search-type QueryType hits a *track* search. The Spotify extractor's text branch ends in `createResponse(null, tracks)`, and createResponse takes the playlist as its first argument, so `searchResult.playlist` was always null. album.ts then hit its `if (!searchResult.playlist)` guard and answered "No album found. Please provide a Spotify album URL or a more specific album search." That advice was unreachable, since no amount of specificity turns a track search into a playlist. Only a Spotify album URL ever worked. Apple Music's search branch has the same shape, so adding fallback arms would not have helped. Resolve the query to an album URL first via Spotify's search API with type=album, then feed that URL through the URL path that already works. The artist option folds into the lookup query instead of the track search. Both failure points now warn-log; previously neither emitted anything. The client-credentials token fetcher moves from backend's SpotifyAuthService into @lucky/shared so the bot and backend share one implementation rather than drifting on cache and expiry handling; SpotifyAuthService re-exports it, so its callers are unchanged. Adds album.spec.ts, the command's first tests: 4 cases, 3 of which fail against the pre-fix code (the 4th covers the URL path, which worked). Closes #2050.
|
Warning Review limit reached
Next review available in: 32 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Failed to generate code suggestions for PR |
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
Consolidates the Spotify client-credentials token into a single shared getSpotifyClientToken implementation, removing the duplicate (and its cache) from SpotifyAuthService and re-exporting the shared one; adds searchSpotifyAlbums/albumApi helpers in shared for album resolution. Rewrites the /album command to resolve free-text queries to a Spotify album URL via searchSpotifyAlbums before searching, since text queries always hit track search and left searchResult.playlist null (#2050), and replaces the dead-end "more specific album search" copy with an actionable no-match message. Adds album.spec.ts covering text-to-URL resolution, the artist option folding into the lookup, direct URL passthrough, and the no-match path.
Worth a look
- Concurrent token cache misses all request a Spotify token —
packages/shared/src/utils/spotify/clientToken.ts:14· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 29 functions depend on the 20 functions this change touches.
Health — this change adds coupling hotspots:
- new:
setupSpotifyRoutes()— 2 callers, 10 callees - new:
PreferredArtistsPage()— 0 callers, 7 callees
Verification — 29 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 29 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
At startup and at expiry every concurrent caller misses the cache before any of them stores a token, so each opened its own client-credentials exchange against accounts.spotify.com. Pre-existing in the backend copy this PR moved, but promoting it to shared doubles the caller count, so it is fixed here rather than left to drift. Callers now share one in-flight promise, cleared on settlement so a failed exchange is retried instead of cached. Adds clientToken.test.ts, 4 cases: concurrent callers open one exchange (fails without this change), a cached token is not refetched, a failed exchange is retried, and unset credentials short-circuit with no request. Raised by cubic on #2053.
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Moves the Spotify client-credentials token logic out of SpotifyAuthService into @lucky/shared/utils (new spotify/clientToken and spotify/albumApi modules), re-exporting getSpotifyClientToken for backend callers. Reworks the /album command to resolve free-text queries to a Spotify album URL via searchSpotifyAlbums before searching, replacing the broken SPOTIFY_SEARCH path that could never return an album and always reported "No album found" (#2050). Adds album.spec.ts covering URL passthrough, text-to-album resolution, the artist filter, and the no-match message, plus tests for the shared client-token cache.
No blocking issues surfaced. 11 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 32 functions depend on the 23 functions this change touches.
Health — this change adds coupling hotspots:
- new:
setupSpotifyRoutes()— 2 callers, 10 callees - new:
PreferredArtistsPage()— 0 callers, 7 callees
Verification — 32 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 32 function(s) in the blast radius were not formally verified this run
· 2 more finding(s) on lines outside this diff (see the check run).
|
🤖 I have created a release *beep* *boop* --- <details><summary>2.39.6</summary> ## [2.39.6](v2.39.5...v2.39.6) (2026-08-22) ### Bug Fixes * **bot:** drop the audio-features scoring path, spotify returns 403 ([#2074](#2074)) ([11891bf](11891bf)) * **bot:** drop the autoplay arm that calls a removed spotify endpoint ([#2071](#2071)) ([682a795](682a795)) * **bot:** fall back past a dead spotify arm in /artist ([#2052](#2052)) ([45dc998](45dc998)) * **bot:** make three silent /play failures observable ([#2062](#2062)) ([e6b2810](e6b2810)) * **bot:** pick closest-duration soundcloud fallback match ([#2049](#2049)) ([738efb8](738efb8)) * **bot:** report a dead autoplay replenish at error level ([#2063](#2063)) ([2c5962e](2c5962e)) * **bot:** report spotify extractor health instead of failing silently ([#2060](#2060)) ([ddaa287](ddaa287)) * **bot:** rerank search results toward exact artist/title match ([#2045](#2045)) ([0c719d5](0c719d5)) * **bot:** resolve /album text queries to an album url ([#2053](#2053)) ([4498299](4498299)) * **bot:** restore youtube-dl-exec, discord-player-youtubei needs it undeclared ([#2040](#2040)) ([c75475b](c75475b)) * **bot:** stop reporting an outage when the fallbacks found nothing ([#2069](#2069)) ([37412e2](37412e2)) * **bot:** surface dead last.fm env session key to sentry ([#2047](#2047)) ([3c97291](3c97291)) * **bot:** update a case reason through the service layer ([#2066](#2066)) ([187d783](187d783)) * **bot:** use metadata setter instead of direct property assignment ([#2042](#2042)) ([740d53e](740d53e)) ### Performance Improvements * **bot:** cut yt-dlp timeout from 15s to 6s ([#2044](#2044)) ([39e9e3c](39e9e3c)) </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
## Summary packages/bot/src/functions/music/commands/session.ts (258 LOC) had no spec while every sibling music command does. /album shipped 100% broken for every text query and stayed that way until #2053 precisely because it had no spec either (#2050) - same risk class here. ## Coverage added - save: no-active-queue warning, name lowercasing, the service's duplicate/invalid/limit refusal, and success - list: empty state and the formatted per-session line - delete: found/not-found, and that it skips the voice-channel guard restore needs - restore: voice-channel guard, reusing vs creating a queue, the queueConnect failure path, the zero-restored-tracks not-found case, and success ## Test plan - [x] `npx jest src/functions/music/commands/session.spec.ts` - 16/16 pass - [x] Verified each assertion actually fails against a mutated implementation (forced branches to always/never take the no-queue and delete-found paths, forced restore to always create a fresh queue) before confirming it passes against the real code - the same check that would have caught #2050 - [x] `npx tsc -p packages/bot --noEmit` clean - [x] No production code changed - test-only PR Closes #2057 <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds the missing spec for the `/session` music command so it has the same regression protection as sibling commands, closing #2057. Test-only PR with no production code changes. **Coverage** - `save`: warns without an active queue, lowercases the session name, handles the service's duplicate/invalid/limit refusal, and reports success. - `list`: covers the empty state and the formatted per-session line. - `delete`: covers found, not-found, and that it skips the voice-channel guard `restore` needs. - `restore`: covers the voice-channel guard, queue creation vs reuse, the `queueConnect` failure path, the zero-restored-tracks not-found case, and success. - Every assertion was verified to fail against a mutated implementation before passing against the real code. <sup>Written for commit f97d9ae. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2263?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
## Summary packages/bot/src/functions/music/commands/effects.ts (182 LOC) had no spec while every sibling music command does. /album shipped 100% broken for every text query and stayed that way until #2053 precisely because it had no spec either (#2050) - same risk class here. ## Coverage added - Guard chain: guild / DJ-role / active-queue / is-playing early returns - bassboost: all 3 filter tiers (low/mid/high), the 0/6 out-of-range rejections, the disable-at-0 double-toggle, and the catch-and-report-error path - nightcore: enabled / disabled / no-resampler branches, and the catch-and-report-error path - reset: filter-clear + resampler-toggle, no-resampler safety, and the catch-and-report-error path ## Test plan - [x] `npx jest src/functions/music/commands/effects.spec.ts` - 20/20 pass - [x] Verified each assertion actually fails against a mutated implementation (swapped a filter tier, changed a success message, dropped a filter-clear call) before confirming it passes against the real code - the same check that would have caught #2050 - [x] `npx tsc -p packages/bot --noEmit` clean - [x] No production code changed - test-only PR Closes #2058 <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds the missing spec for `/effects` so its behavior is tested like every sibling music command. Test-only; no production code changes. Closes #2058. **Coverage** - Early-return guard chain for guild, DJ role, active queue, and is-playing. - Bassboost's three filter tiers, the 0/6 out-of-range rejections, the disable-at-0 double-toggle, and error reporting. - Nightcore's enabled, disabled, and no-resampler branches, the `nightcore` filter name, and error reporting. - Reset's filter-clear/resampler-toggle, no-resampler safety, and error reporting. - Mutated-implementation checks confirm each assertion fails on wrong code, covering the check that would have caught #2050. <sup>Written for commit ddcf125. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2262?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
…2264) ## Summary lyrics, seek, shuffle, skipto, leavecleanup, songinfo, and the music command-loader index.ts had no spec while every other command in packages/bot/src/functions/music/commands/ does. /album shipped 100% broken for every text query and stayed that way until #2053 precisely because it had no spec either (#2050) - same risk class here. music.ts, session.ts, and effects.ts (the three largest, per #2055's own scope split) already got their specs in separate prior PRs (#2056, #2263, #2262); this closes out the rest of that parent issue's inventory in one batch since none of them touch production code or depend on each other. ## Coverage added - **lyrics**: feature-toggle gate, blank-query-outside-guild rejection, falling back to the now-playing track (including its unknown-title/no-author case), the not-found and thrown-error branches, and paging multi-page results across follow-ups - **seek**: every mm:ss/ss parse-failure shape, the no-duration and beyond-duration rejections, and the parse/format round-trip on success - **shuffle**: the <2-tracks rejection, the random-vs-smart branch (including the no-subcommand default), and that smart shuffle clears and rebuilds the queue in the shuffled order - **skipto**: the beyond-queue-size rejection vs the exactly-at-boundary accept, and the generic-vs-track-embed reply depending on whether a track exists at the target index - **leavecleanup**: no-voice-channel guard, removing only tracks whose requester left, singular/plural wording, and that a throw during one track's removal doesn't stop the rest - **songinfo**: the null-vs-present progress bar branch - **index.ts**: mirrors the existing source-inspection pattern already used for the automod/management/moderation command loaders, since the real function can't run under jest's CJS transform of `import.meta.url` ## Test plan - [x] All 7 new spec files pass (55 tests total) - [x] Verified each new assertion actually fails against a mutated implementation of its command (one targeted mutation per file: a flipped/loosened guard condition, a dropped filter, a hardcoded default) before confirming it passes against the real code - the same check that would have caught #2050 - [x] `npx tsc -p packages/bot --noEmit` clean - [x] No production code changed - test-only PR Closes #2055 <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Adds spec coverage for the six untested music commands in `packages/bot`, closing the coverage gap from #2055 so regressions like the one that broke `/album` are caught. No production code changed. **Coverage added** - `lyrics`: toggle gate, blank-query rejection, now-playing fallback, error branches, and multi-page follow-ups. - `seek`: parse failures, duration rejections, and success round-trip. - `shuffle`: minimum track count, random vs. smart (including default), and smart queue rebuild. - `skipto`: boundary validation and generic vs. track-embed replies. - `leavecleanup`: guards, requester filter, pluralization, and error resilience. - `songinfo`: progress bar present/null branches. A spec for the command-loader `index.ts` was attempted and dropped: `import.meta.url` can't be parsed under the repo's CJS jest transform, and the loader logic is already covered by `getCommandsFromDirectory.spec.ts`. Every assertion was verified to fail against a mutated implementation, and `tsc` is clean. <sup>Written for commit 83433c3. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/2264?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
1 participant
Closes #2050.
The bug is bigger than #2050 described
/albumwith a free-text query never worked. Not intermittently: no text query could ever succeed.Trace for
/album abbey road:searchEngine = QueryType.SPOTIFY_SEARCH(album.ts:113-117, pre-fix)this.createResponse(null, tracks)—discord-player-spotify/dist/index.js:823createResponse(playlist?, tracks?)takes the playlist first —discord-player/dist/index.d.ts:3333. SosearchResult.playlistis alwaysnullfor a text query.album.ts:139checksif (!searchResult.playlist)and answers:That advice is unreachable. No amount of specificity turns a track search into a playlist. Only a Spotify album URL ever worked.
This is why the
/artistfallback fix (#2052) does not port here. Adding YouTube/SoundCloud arms would change nothing: every search-typeQueryTypereturns tracks.SPOTIFY_ALBUM,APPLE_MUSIC_ALBUM,SOUNDCLOUD_PLAYLISTandYOUTUBE_PLAYLISTare all URL types, and Apple Music'sAPPLE_MUSIC_SEARCHbranch has the samecreateResponse(null, tracks)shape. Three arms would have failed the same playlist gate.CHANGELOG line 1009 shipped this as "search and browse albums with track listings". There was no
album.spec.ts, which is why it went unnoticed.Change
type=album, then feed that URL through the URL path that already works. Reuses the working branch instead of adding a parallel one.artistoption folds into the album lookup rather than the track search, so it narrows the album rather than the tracks.warnLog. Previously neither emitted anything, the same blind spot documented in fix(bot): Spotify extractor registration failure is invisible — no degraded gauge, no Sentry #2051.Shared token fetcher
getSpotifyClientToken()(client-credentials, cached to 30s before expiry) moves frompackages/backend/src/services/SpotifyAuthService.tsinto@lucky/shared/utils/spotify/clientToken.ts. The bot needs the same token, and a second copy would drift on the caching and expiry-margin details.SpotifyAuthServicere-exports it, so its three existing callers inartistSuggestion.tsare untouched. Backend's localfetchJsonwrapper is inlined in the moved copy to keep shared free of a backend-only helper.Tests
album.spec.tsis new: the command had no tests at all. 4 cases, verified 3 fail against the pre-fix code:Full suites green: 376 bot command tests, 45 backend Spotify tests, 11 shared Spotify tests. Typecheck clean across shared, bot, backend, frontend.
Note on degradation
searchSpotifyAlbumsreturns[]whenSPOTIFY_CLIENT_ID/SPOTIFY_CLIENT_SECRETare unset, so text queries then report "no album found" instead of throwing. URL queries keep working through the extractor's own credentials, which is the same behavior as before this PR.Summary by cubic
Fixes /album free‑text queries by resolving them to a Spotify album URL before playback, and deduplicates concurrent Spotify client‑credentials requests. Previously any non‑URL input used a track‑only search and always replied “No album found.”
@lucky/shared/utils/spotify/albumApisearchSpotifyAlbums, then searches the resolved album URL withQueryType.AUTO.@lucky/shared/utils/spotify/clientToken, re‑exported by backendSpotifyAuthService; shares the in‑flight token exchange across callers and adds tests for cache/in‑flight/credential paths.album.spec.tscovering URL resolution, artist narrowing, URL passthrough, and actionable failure messaging.Rollout
SpotifyAuthServicere‑export.SPOTIFY_CLIENT_ID/SPOTIFY_CLIENT_SECRETare unset, text queries now degrade to “No album found”; direct album URLs still work.Written for commit 33cb770. Summary will update on new commits.