Repository navigation
refactor(artists): extract ArtistSuggestionService from route handler - #980
Merged
Merged
Conversation
…vice - Create ArtistSuggestionService with three-tier lookup strategy: 1. User's saved preferred artists (database) 2. User's Spotify top artists (if linked) 3. Popular artists fallback (static list) - Move 80+ popular artist names to constants/popularArtists.ts - Add Redis caching for fallback and user top artists - Reduce artists.ts route handler from 586 to 298 LOC - Update existing tests to match new behavior - All 823 backend tests passing
Move all business logic from routes/artists.ts into services/artistSuggestion.ts: - Add handler methods: handleGetSuggestions, handleSearchArtists, etc. - Validate inputs and return errors in service layer - Trim routes file from 319 to 46 LOC (meets <100 requirement) - Create unit test suite with 33 tests (89.84% coverage, exceeds 80%) - All tests passing (856 tests in backend package)
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
github-actions
Bot
added
dependencies
|
Failed to generate code suggestions for PR |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 39 minutes and 12 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, 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 trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughA new ChangesArtist Suggestion Service Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/backend/tests/unit/routes/artists.test.ts (1)
53-56: ⚡ Quick winAssert the registered middleware chain, not just the terminal handler.
getRouteHandler()drops everything except the last arg, so these tests still pass if a route losesrequireAuthor swapsapiLimiter/writeLimiter. After this refactor, that wiring is most of what the route layer still owns.Possible direction
-function getRouteHandler(mockMethod: any, index = 0): any { +function getRouteRegistration(mockMethod: jest.Mock, index = 0) { const call = mockMethod.mock.calls[index] - return call[call.length - 1] + return { + path: call[0], + middlewares: call.slice(1, -1), + handler: call[call.length - 1], + } }Then assert
pathandmiddlewaresfor each route before invokinghandler.🤖 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/backend/tests/unit/routes/artists.test.ts` around lines 53 - 56, getRouteHandler currently returns only the last argument (handler) so tests miss verifying registered middleware and path; change getRouteHandler(mockMethod, index=0) to inspect the full call (const call = mockMethod.mock.calls[index]) and return an object with path = call[0], middlewares = call.slice(1, call.length - 1) and handler = call[call.length - 1]; update tests to assert the path and the ordered middlewares (e.g., requireAuth, apiLimiter/writeLimiter) before invoking/inspecting handler.
🤖 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/backend/src/routes/artists.ts`:
- Around line 54-57: The route currently passes r.params.artistId (type string |
string[]) directly to ArtistSuggestionService.handleGetRelatedArtists (expects
string | undefined); normalize it to a single string or undefined first — e.g.
compute artistId = Array.isArray(r.params.artistId) ? r.params.artistId[0] :
r.params.artistId and pass that to svc.handleGetRelatedArtists — and update the
wrap error handling to use catch (e: unknown) and narrow (e.g. instanceof Error
or checking 'status'/'error' props) instead of catch (e: any) to remove unsafe
any usage.
- Around line 10-19: The wrap helper currently uses Promise<any> and catch (e:
any); change the handler signature to (h: (r: AuthenticatedRequest) =>
Promise<unknown>) and catch (e: unknown), then introduce a small type guard
(e.g., isHttpError) that narrows unknown to { status?: number; error?: string }
and use it to safely read status and error (falling back to 500 and the provided
err message); update uses of e to go through the type guard and keep errorLog({
message: `${ctx} error`, error: e }) only when appropriate.
In `@packages/backend/src/services/artistSuggestion.ts`:
- Around line 2-8: There are two imports from '`@lucky/shared/utils`' in
artistSuggestion.ts (a separate type-only import for SpotifyArtist and a value
import for errorLog, getPrismaClient, searchSpotifyArtists,
getSpotifyRelatedArtists); consolidate them into a single import by adding "type
SpotifyArtist" into the existing named import (e.g. import { type SpotifyArtist,
errorLog, getPrismaClient, searchSpotifyArtists, getSpotifyRelatedArtists } from
'`@lucky/shared/utils`') and remove the separate import line.
- Around line 195-205: fetchSpotifyTopArtists issues parallel fetches for
timeRanges without any timeout/abort protection; update the code that builds
promises (where timeRanges is mapped and Promise.all is awaited) to use an
AbortController per request and a setTimeout to call controller.abort() after a
configurable timeout (e.g., 5–10s), ensure you clear the timeout on successful
response to avoid leaks, and handle abort errors so Promise.all rejects/handles
them cleanly; reference the mapping that creates promises and the Promise.all
that assigns responses to add this AbortController+timeout logic.
- Around line 451-457: The single-save Zod schema for preferred artists rejects
explicit null for imageUrl and spotifyId; update the schema in the object
defined around the validation used by handleSavePreferredArtist so both imageUrl
and spotifyId accept null as well as undefined by changing their types from
z.string().optional() to z.string().nullable().optional() (or
z.string().nullable() if you prefer to require presence but allow null),
ensuring parity with the DB nullable columns and the batch-save behavior for
imageUrl; keep the field names artistSuggestion schema: imageUrl and spotifyId
so the change is localized.
- Around line 521-550: handleBatchSavePreferences is doing await
db.userArtistPreference.upsert(...) inside a loop which can leave partial writes
if one upsert fails; wrap the whole loop in a single database transaction so all
upserts commit or all rollback. Modify handleBatchSavePreferences to call
db.$transaction(...) (or the equivalent transaction API your DB client exposes)
and perform the normalized artistKey computation and userArtistPreference.upsert
calls inside that transaction callback (referencing normalizeArtistKey and
userArtistPreference.upsert), collect results and return them; ensure any thrown
error propagates to abort the transaction so no partial updates persist.
In `@packages/backend/tests/unit/services/artistSuggestion.test.ts`:
- Line 1: In the "should cache user top artists after fetch" test in
artistSuggestion.test.ts you stubbed global.fetch but never restore it; save the
original (e.g. const originalFetch = global.fetch) before mocking or use
jest.spyOn(global, 'fetch') to create the mock, then restore global.fetch in an
afterEach/cleanup block (or a finally within the test) by reassigning
global.fetch = originalFetch or calling mockRestore()/jest.restoreAllMocks();
update the test to reference the specific test name and ensure the original
fetch is always restored to avoid leaking state to subsequent tests.
---
Nitpick comments:
In `@packages/backend/tests/unit/routes/artists.test.ts`:
- Around line 53-56: getRouteHandler currently returns only the last argument
(handler) so tests miss verifying registered middleware and path; change
getRouteHandler(mockMethod, index=0) to inspect the full call (const call =
mockMethod.mock.calls[index]) and return an object with path = call[0],
middlewares = call.slice(1, call.length - 1) and handler = call[call.length -
1]; update tests to assert the path and the ordered middlewares (e.g.,
requireAuth, apiLimiter/writeLimiter) before invoking/inspecting handler.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ecff51cd-ad33-4263-8141-3cf2592cc631
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (5)
packages/backend/src/constants/popularArtists.tspackages/backend/src/routes/artists.tspackages/backend/src/services/artistSuggestion.tspackages/backend/tests/unit/routes/artists.test.tspackages/backend/tests/unit/services/artistSuggestion.test.ts
|
Size Change: 0 B Total Size: 423 kB ℹ️ View Unchanged
|
…utes Move wrapHandler and ApiError interface to src/utils/routeUtils.ts. Redefine route handlers as module-level consts; setupArtistsRoutes becomes a compact registration block (≤100 LOC spec met). Also fix Zod schema nullable fields and test cleanup: - spotifyId/imageUrl: add .nullable() to match DB columns - Remove unused `suggestions` variable in cache test - Save/restore global.fetch around mock assignment
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
LucasSantana-Dev
added a commit
that referenced
this pull request
May 24, 2026
## Summary Replaces the 15+ positional arguments passed through the autoplay collector pipeline with a single `AutoplayContext` value object, improving call-site clarity and making the pipeline easier to extend. ### Changes - Introduce `AutoplayContext` interface in `autoplayContext.ts` - Update `candidateCollector`, `lastFmSeeder`, `spotifyRecommender`, `replenisher`, and `candidateFallback` to accept `AutoplayContext` instead of positional args - Fix missing `beforeEach` mock setup in `candidateCollector` tests - Remove leftover `.bak` file from refactor ### Why Positional argument lists of 15+ items are hard to read, easy to misorder, and brittle to extend. A named value object surfaces intent at every call site and isolates future additions to one interface definition. Part of the architecture refactor series (T1–T5): - T1 #979 ✅ — extract `MessagePipeline` - T2 #980 ✅ — introduce `IGuildAutomationRepository` - T3 #981 ✅ — `AutomationPlan` result type - T4 #982 ✅ — `GuildAutomationRepository` / `Orchestrator` split - T5 (this PR) — `AutoplayContext` value object
LucasSantana-Dev
added a commit
that referenced
this pull request
May 24, 2026
ADRs for the 5 refactors merged in PRs #979–#983 and the Phase 4 test reduction strategy. ## Decision records added - `2026-05-23-artist-suggestion-service.md` — ArtistSuggestionService extraction (#980) - `2026-05-23-autoplay-context-value-object.md` — AutoplayContext value object (#983) - `2026-05-23-bot-test-reduction-phase4-replacement-strategy.md` — Phase 4 test replacement strategy - `2026-05-23-guild-automation-orchestrator-repository-split.md` — GuildAutomationService split (#982) - `2026-05-23-message-pipeline-handler-chain.md` — MessagePipeline handler chain (#981) - `2026-05-23-recommendation-engine-single-entrypoint.md` — recommendTracks() single entrypoint (#979) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added architecture decision records documenting planned service refactoring, test optimization strategies, and API consolidation initiatives to improve system maintainability. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/LucasSantana-Dev/Lucky/pull/1040?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
LucasSantana-Dev
added a commit
that referenced
this pull request
May 25, 2026
## Summary - Bumps all package.json files from `2.14.1` → `2.15.0` - Populates `CHANGELOG.md` with everything since v2.14.1 - Adds ADR `docs/decisions/2026-05-24-ci-runtime-baseline-accepted.md` (CI runtime baseline: 3–4 min accepted, Jest sharding deferred) ### Changes included in this release **Added** - ServerLogs + ServerSettings UI pages (#965) - AutoMessages executor wiring into execution service (#950) **Changed** - 4 UI redesigns: Admin, Config, Login, ServersPage, CustomCommands, GuildAutomation, Spotify, LastFm (#967–#970) - 5 refactors: AutoplayContext VO (#983), GuildAutomationOrchestrator/Repository split (#982), MessagePipeline chain (#981), ArtistSuggestionService (#980), recommendTracks entrypoint (#979) **Fixed** — deploy CI gap sweep (A–F) - Gap A: hard-fail on OAuth 429 (#1045) - Gap B: surface async deploy via commit statuses (#1046) - Gap C: bot healthcheck polls Discord gateway, not Redis TCP (#1047) - Gap D: post error status on lock contention (#1052) - Gap E: add bot to required containers, remove dead unhealthy grep (#1054) - Gap F: wait for homelab-deploy completion on docker_rebuilt=true path (#1056) - lockfile-hash BuildKit cache key (#1016) - squash-merged release branch archive (#946) **Internal** - Phase 4 test cleanup — 93 tests removed (#956–#1035) - Pre-commit hooks: husky + lint-staged + tsc (#1007) - madge gate promoted to blocking - Dependabot routine bumps (#971–#977, #1042) > **Note:** #1054 (Gap E) may still be in CI — merge this PR after #1054 lands.
](https://app.coderabbit.ai/change-stack/LucasSantana-Dev/Lucky/pull/1040?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This was referenced Jun 12, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packages/backend/src/routes/artists.ts(586 LOC → 54 LOC) into a newArtistSuggestionServicepackages/backend/src/constants/popularArtists.tsTest plan
npm run test:ci --workspace=packages/backend— 856 tests passartists.tsroute handler is 54 LOC (≤100 requirement)ArtistSuggestionServiceunit tests mock only Prisma + Spotify (no Express)artistSuggestion.ts: 91.87% (≥80% requirement)Summary by CodeRabbit
Release Notes
New Features
Refactor
Tests