Repository navigation
Redesign Preferred Artists page with default suggestions and batch save - #630
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 59 seconds. ⌛ 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 (2)
📝 WalkthroughWalkthroughAdded a backend route for Spotify artist suggestions with OAuth token validation and fallback to popular artists. Updated frontend to display suggestions on the preferences page and batch preference changes before saving. Added API client method and expanded test coverage for new suggestion flows. Changes
Sequence DiagramsequenceDiagram
actor User
participant Frontend
participant Backend API
participant Spotify OAuth
participant Spotify API
User->>Frontend: Load Preferences Page
Frontend->>Backend API: GET /api/artists/suggestions
Backend API->>Spotify OAuth: getValidAccessToken(user)
Spotify OAuth-->>Backend API: access_token
Backend API->>Spotify API: GET /v1/me/top/artists?limit=12
Spotify API-->>Backend API: top artists (may be < 12)
Backend API->>Spotify API: searchSpotifyArtists(popular artist queries)
Spotify API-->>Backend API: additional artists
Backend API-->>Frontend: up to 12 unique suggestions
Frontend->>Frontend: Display suggestions
User->>Frontend: Click Prefer/Block on artists
Frontend->>Frontend: Store in unsavedChanges (local)
Frontend->>Frontend: Show "Save Preferences (N)" button
User->>Frontend: Click Save Preferences
Frontend->>Backend API: POST /api/artists/savePreference (batch)
Backend API-->>Frontend: Success
Frontend->>Frontend: Clear unsavedChanges
Frontend->>User: Preferences saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Size Change: +179 B (+0.05%) Total Size: 330 kB 📦 View Changed
ℹ️ View Unchanged
|
…tch save - Load suggested artists on page mount (user's top Spotify artists or popular artists) - Show artist grid without requiring search input - Click to expand related artists panel with add buttons - Batch save button instead of auto-save on each click - Fixed sidebar active link highlighting for /music vs /music/artists
- PreferredArtists.test.tsx: add mockGetSuggestions and fix tests to match redesigned grid view with batch save button, detail panel, and related artists - Sidebar.tsx: simplify isActive() logic to properly support sub-route activation for all paths including /music sub-routes All 517 tests pass.
- Backend: 22 unit tests for artists.ts (suggestions, search, related, preferences CRUD) * Covers OAuth-linked top artists path, fallback to search, error cases (401, 503, 500) * Tests both success and error flows for all 5 endpoints - Frontend: 14 additional tests for PreferredArtists.tsx (26 total) * Coverage: 93.44% statements, 82.11% branches, 90% functions * Covers save/error handling, loading states, related panel, unsaved changes * Tests duplicate prevention, genre rendering, preference state in detail panel - API client: artistsApi.ts inherently covered via frontend integration tests Fixes PR #630 SonarCloud coverage requirement (41.8% → >80%)
LucasSantana-Dev
force-pushed
the
feature/preferred-artists-ux
branch
from
April 15, 2026 03:40
b5886c6 to
7702f61
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/backend/tests/unit/routes/artists.test.ts (1)
105-107: Handler extraction relies on route registration order.Extracting handlers via
mock.calls[0][2],mock.calls[1][2], etc. couples tests to the order routes are registered insetupArtistsRoutes. If routes are reordered, these indices will break silently. Consider extracting handlers by matching the route path instead.♻️ Example approach
function findHandler(mockApp: Express, method: 'get' | 'post' | 'delete', path: string) { const calls = (mockApp[method] as jest.Mock).mock.calls const call = calls.find(([p]) => p === path) return call?.[2] } // Usage: const handler = findHandler(mockApp, 'get', '/api/artists/suggestions')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/tests/unit/routes/artists.test.ts` around lines 105 - 107, Tests for route handlers currently grab handlers by call index (e.g., (mockApp.get as jest.Mock).mock.calls[0][2]), which couples them to registration order; update the tests that call setupArtistsRoutes and read handlers from mockApp (mockApp.get/mockApp.post/etc.) to instead search the mock calls for the desired route path and HTTP method and return the handler (e.g., implement a small helper used by the test to scan (mockApp.get as jest.Mock).mock.calls for the entry whose first argument equals the route string and then use its third element as the handler), ensuring references to setupArtistsRoutes and mockApp.get/mockApp.post identify where to change.packages/frontend/src/pages/PreferredArtists.tsx (2)
445-447: Silent error handling provides no user feedback.When
savePreferencefails, the error is caught but users receive no indication of failure. WhileunsavedChangescorrectly remains populated (allowing retry), consider adding a toast or error state as noted in the TODO comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/pages/PreferredArtists.tsx` around lines 445 - 447, The catch block in the savePreference flow (inside PreferredArtists.tsx around the savePreference call) swallows errors and provides no user feedback; update the catch to surface failures by invoking the app's toast/notification mechanism (or set an error state) and include the error message so users know the save failed, while keeping unsavedChanges intact for retry; specifically modify the catch handling that currently comments "// error handling could be improved with a toast" to call the existing toast/snackbar helper (or setError / setToast) and ensure any cleanup or UI state (e.g., disabling spinner) is handled consistently.
416-442: Sequential API calls could be parallelized for better performance.The batch save iterates through
unsavedChangesand awaits eachsavePreferencecall sequentially. For users with many pending changes, this could be slow. Consider usingPromise.allfor parallel execution if the backend supports concurrent requests.♻️ Parallel save approach
const savePromises = [...unsavedChanges.entries()].map(async ([key, pref]) => { const artist = suggestedArtists.find(a => normalizeArtistKey(a.name) === key) // ... build payload return api.artists.savePreference(payload) }) await Promise.all(savePromises)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/pages/PreferredArtists.tsx` around lines 416 - 442, The loop over unsavedChanges is calling api.artists.savePreference sequentially which is slow; change it to build an array of promises and await them in parallel using Promise.all. For each entry in unsavedChanges (use unsavedChanges.entries() or [...unsavedChanges.entries()]) compute the artist via suggestedArtists.find(a => normalizeArtistKey(a.name) === key) or fallback to savedPreferences.get(key), construct the same payload you currently pass to api.artists.savePreference, push the returned promise into an array, and then await Promise.all(promises) so savePreference calls run concurrently; keep use of normalizeArtistKey, suggestedArtists, savedPreferences, and api.artists.savePreference intact.packages/backend/src/routes/artists.ts (1)
60-67: Consider adding a timeout to the Spotify API fetch call.The
fetchcall to Spotify's top artists endpoint has no timeout configured. If the Spotify API is slow or unresponsive, this could block the request handler indefinitely. Consider usingAbortSignal.timeout()for resilience.♻️ Suggested fix
const userTopRes = await fetch( 'https://api.spotify.com/v1/me/top/artists?limit=12&time_range=medium_term', { headers: { Authorization: `Bearer ${link}`, }, + signal: AbortSignal.timeout(5000), }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/backend/src/routes/artists.ts` around lines 60 - 67, The Spotify fetch that assigns userTopRes has no timeout; wrap the request with an AbortSignal (e.g., use AbortSignal.timeout(ms) or create an AbortController) and pass signal to fetch so the call to fetch('https://api.spotify.com/v1/me/top/artists...') will be aborted after a reasonable timeout (e.g., 3-10s); update the surrounding async handler to catch the abort/timeout error (AbortError) and return/throw an appropriate error response instead of letting the request hang.packages/frontend/src/pages/PreferredArtists.test.tsx (1)
512-549: Test validates API call but not error feedback to user.The test confirms
mockSavePreferenceis called when an error occurs, but doesn't assert any user-visible error feedback. This aligns with the implementation's TODO comment about improving error handling with a toast. Consider adding error UI assertions when that improvement is made.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/frontend/src/pages/PreferredArtists.test.tsx` around lines 512 - 549, The test "handles save preference error gracefully" currently only asserts mockSavePreference was called; update it to also assert the user-visible error feedback is shown after mockSavePreference rejects (e.g., check for the toast/alert message or error UI that the component displays on failure). Locate the test by its name and the references to mockSavePreference, renderPage, and the Save Preferences button, then add a waitFor/assertion that the expected error element (toast/alert text or role) appears in the DOM.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/frontend/src/components/Layout/Sidebar.tsx`:
- Around line 489-491: The current active-link logic still treats a parent like
"/music" as active for "/music/artists" because withChild uses startsWith(path +
'/'); change the logic in the Sidebar link activation to only use exact match
(remove/stop using withChild) so the function returns only exact (i.e., return
exact) and ensure any aria-current computation relies on that exact variable
instead of withChild (update uses of exact and withChild in this block
accordingly).
In `@packages/frontend/src/pages/PreferredArtists.tsx`:
- Around line 412-450: The save loop in handleSavePreferences only looks up
artist metadata in suggestedArtists and savedPreferences so preferences toggled
from searchResults get lost; modify how unsavedChanges is populated (update
handleTogglePreference) to store an object containing both preference and the
artist metadata (e.g., { pref, artist }) or include searchResults lookup, then
update handleSavePreferences to read artist metadata from unsavedChanges first
(falling back to suggestedArtists or savedPreferences only if missing) before
calling api.artists.savePreference; reference handleSavePreferences,
handleTogglePreference, unsavedChanges, suggestedArtists, savedPreferences, and
searchResults when making this change.
---
Nitpick comments:
In `@packages/backend/src/routes/artists.ts`:
- Around line 60-67: The Spotify fetch that assigns userTopRes has no timeout;
wrap the request with an AbortSignal (e.g., use AbortSignal.timeout(ms) or
create an AbortController) and pass signal to fetch so the call to
fetch('https://api.spotify.com/v1/me/top/artists...') will be aborted after a
reasonable timeout (e.g., 3-10s); update the surrounding async handler to catch
the abort/timeout error (AbortError) and return/throw an appropriate error
response instead of letting the request hang.
In `@packages/backend/tests/unit/routes/artists.test.ts`:
- Around line 105-107: Tests for route handlers currently grab handlers by call
index (e.g., (mockApp.get as jest.Mock).mock.calls[0][2]), which couples them to
registration order; update the tests that call setupArtistsRoutes and read
handlers from mockApp (mockApp.get/mockApp.post/etc.) to instead search the mock
calls for the desired route path and HTTP method and return the handler (e.g.,
implement a small helper used by the test to scan (mockApp.get as
jest.Mock).mock.calls for the entry whose first argument equals the route string
and then use its third element as the handler), ensuring references to
setupArtistsRoutes and mockApp.get/mockApp.post identify where to change.
In `@packages/frontend/src/pages/PreferredArtists.test.tsx`:
- Around line 512-549: The test "handles save preference error gracefully"
currently only asserts mockSavePreference was called; update it to also assert
the user-visible error feedback is shown after mockSavePreference rejects (e.g.,
check for the toast/alert message or error UI that the component displays on
failure). Locate the test by its name and the references to mockSavePreference,
renderPage, and the Save Preferences button, then add a waitFor/assertion that
the expected error element (toast/alert text or role) appears in the DOM.
In `@packages/frontend/src/pages/PreferredArtists.tsx`:
- Around line 445-447: The catch block in the savePreference flow (inside
PreferredArtists.tsx around the savePreference call) swallows errors and
provides no user feedback; update the catch to surface failures by invoking the
app's toast/notification mechanism (or set an error state) and include the error
message so users know the save failed, while keeping unsavedChanges intact for
retry; specifically modify the catch handling that currently comments "// error
handling could be improved with a toast" to call the existing toast/snackbar
helper (or setError / setToast) and ensure any cleanup or UI state (e.g.,
disabling spinner) is handled consistently.
- Around line 416-442: The loop over unsavedChanges is calling
api.artists.savePreference sequentially which is slow; change it to build an
array of promises and await them in parallel using Promise.all. For each entry
in unsavedChanges (use unsavedChanges.entries() or
[...unsavedChanges.entries()]) compute the artist via suggestedArtists.find(a =>
normalizeArtistKey(a.name) === key) or fallback to savedPreferences.get(key),
construct the same payload you currently pass to api.artists.savePreference,
push the returned promise into an array, and then await Promise.all(promises) so
savePreference calls run concurrently; keep use of normalizeArtistKey,
suggestedArtists, savedPreferences, and api.artists.savePreference intact.
🪄 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: 927d58f4-fbf4-41ec-8a68-b40676b2d113
📒 Files selected for processing (6)
packages/backend/src/routes/artists.tspackages/backend/tests/unit/routes/artists.test.tspackages/frontend/src/components/Layout/Sidebar.tsxpackages/frontend/src/pages/PreferredArtists.test.tsxpackages/frontend/src/pages/PreferredArtists.tsxpackages/frontend/src/services/artistsApi.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: compressed-size
- GitHub Check: Quality Gates
- GitHub Check: SonarCloud Scan
🔇 Additional comments (8)
packages/frontend/src/services/artistsApi.ts (1)
26-28: LGTM!The new
getSuggestionsmethod correctly matches the backend endpoint response shape{ artists: SpotifyArtist[] }and follows the established patterns of other methods in this API client.packages/backend/src/routes/artists.ts (1)
31-145: LGTM - well-structured suggestions endpoint.The new endpoint handles authentication, Spotify configuration checks, user's top artists with OAuth, and fallback to popular artists search. The deduplication via Map and proper error handling with
errorLogare good practices.packages/frontend/src/pages/PreferredArtists.test.tsx (2)
16-28: LGTM - comprehensive test mock setup.The mock for
getSuggestionsis properly added alongside existing API mocks, and the default return value inbeforeEach(Line 92) ensures tests start with a clean state.
551-725: LGTM - thorough edge case coverage.Good test coverage for no genres, loading states, unsaved changes removal, duplicate artists, API errors, and preference state in related artists panel. The tests follow consistent patterns and properly await async operations.
packages/backend/tests/unit/routes/artists.test.ts (2)
75-226: LGTM - comprehensive test coverage for suggestions endpoint.Tests cover the OAuth top artists flow, fallback to popular artists search, and all error cases (401, 503, 500). The fetch spy is properly restored after each test.
228-608: LGTM - consistent test coverage for all routes.Tests for search, related artists, and preference CRUD operations follow a consistent pattern with proper mock setup and assertions for success and error cases.
packages/frontend/src/pages/PreferredArtists.tsx (2)
612-633: LGTM - well-implemented save button with loading state.The save button correctly shows the pending changes count, disables during save, and displays a loading spinner. Good UX pattern for batch operations.
346-349: LGTM - appropriate initial data loading.Both preferences and suggestions are loaded on mount. The effect correctly depends on both callback references, ensuring data refreshes when the guild changes.
LucasSantana-Dev
enabled auto-merge (squash)
April 15, 2026 03:48
|
This was referenced Apr 18, 2026
LucasSantana-Dev
added a commit
that referenced
this pull request
May 13, 2026
…ve (#630) * fix: sidebar active link - only highlight exact /music path, not /music/artists * feat: add /api/artists/suggestions endpoint for default artist grid * feat: add getSuggestions method to artistsApi * feat: redesign preferred artists page with default suggestions and batch save - Load suggested artists on page mount (user's top Spotify artists or popular artists) - Show artist grid without requiring search input - Click to expand related artists panel with add buttons - Batch save button instead of auto-save on each click - Fixed sidebar active link highlighting for /music vs /music/artists * fix(backend): replace any type with SpotifyArtist in artists suggestions endpoint * fix: update PreferredArtists and Sidebar tests for redesigned UX - PreferredArtists.test.tsx: add mockGetSuggestions and fix tests to match redesigned grid view with batch save button, detail panel, and related artists - Sidebar.tsx: simplify isActive() logic to properly support sub-route activation for all paths including /music sub-routes All 517 tests pass. * test: comprehensive coverage for artists feature - Backend: 22 unit tests for artists.ts (suggestions, search, related, preferences CRUD) * Covers OAuth-linked top artists path, fallback to search, error cases (401, 503, 500) * Tests both success and error flows for all 5 endpoints - Frontend: 14 additional tests for PreferredArtists.tsx (26 total) * Coverage: 93.44% statements, 82.11% branches, 90% functions * Covers save/error handling, loading states, related panel, unsaved changes * Tests duplicate prevention, genre rendering, preference state in detail panel - API client: artistsApi.ts inherently covered via frontend integration tests Fixes PR #630 SonarCloud coverage requirement (41.8% → >80%) * fix(frontend): exact /music match + preserve search-result artist metadata on save
4 tasks
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Backend Changes
Frontend Changes
Tests
Summary by CodeRabbit