feat: made for you section#24
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Adds a new “Made For You” carousel to the Dashboard home view, alongside a refactor of “Recently Played” to focus on track-based cards and updated playback interactions. Also includes a few layout tweaks to support the new panel density.
Changes:
- Add
MadeForYouSectionand render it in the homeMainPanel. - Refactor
RecentlyPlayedSection/card + tests to dedupe and play individual tracks (vs album/artist cards). - Add Spotify browse query hooks for categories and category playlists; tweak layout widths/spacing.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/Dashboard.tsx | Adds min-w-0 to the main scroll area to prevent flex overflow. |
| src/hooks/useSpotifyQueries.ts | Adds useCategories and useCategoryPlaylists React Query hooks. |
| src/components/layout/MainPanel.tsx | Renders the new MadeForYouSection on the home view. |
| src/components/features/NowPlaying.tsx | Widens the Now Playing aside (w-60 → w-72). |
| src/components/features/home/RecentlyPlayedSection.tsx | Switches recently played items to track-based cards + track URI playback. |
| src/components/features/home/RecentlyPlayedSection.test.tsx | Updates tests to match new track-based recently played behavior. |
| src/components/features/home/RecentlyPlayedCard.tsx | Updates RecentItem type to track only. |
| src/components/features/home/RecentlyPlayedCard.test.tsx | Updates tests to match the new RecentItem shape and track URIs. |
| src/components/features/home/NavHeader.tsx | Adds mt-4 to the sticky filter header layout. |
| src/components/features/home/MadeForYouSection.tsx | Introduces the new “Made For You” playlist carousel with play/pause + scroll arrows. |
| src/components/features/home/HomeSection.tsx | Makes title optional and conditionally renders the heading. |
| imageUrl: track.album?.images[1]?.url ?? track.album?.images[0]?.url, | ||
| type: 'track', | ||
| uri: track.uri, | ||
| navigationPath: `/album/${track.album?.id}`, |
There was a problem hiding this comment.
navigationPath is built with track.album?.id, which will produce /album/undefined when track.album is missing (a case this component explicitly supports). This will navigate to a broken route when the card is clicked. Consider guarding navigation (e.g., make navigationPath optional and disable navigation when absent) or provide a valid fallback route.
| navigationPath: `/album/${track.album?.id}`, | |
| navigationPath: track.album ? `/album/${track.album.id}` : undefined, |
| } | ||
|
|
||
| export function HomeSection({ title, children }: Readonly<HomeSectionProps>) { | ||
| return ( | ||
| <section> |
There was a problem hiding this comment.
HomeSection now allows omitting title but still renders a <section> landmark. A <section> without a heading/accessible name can create unnamed landmarks for screen readers. Consider either keeping title required, or adding an aria-label/aria-labelledby prop (or rendering a <div> when no title is provided).
| } | |
| export function HomeSection({ title, children }: Readonly<HomeSectionProps>) { | |
| return ( | |
| <section> | |
| 'aria-label'?: string; | |
| 'aria-labelledby'?: string; | |
| } | |
| export function HomeSection({ title, children, ...sectionProps }: Readonly<HomeSectionProps>) { | |
| const hasAccessibleName = | |
| Boolean(title) || | |
| Boolean(sectionProps['aria-label']) || | |
| Boolean(sectionProps['aria-labelledby']); | |
| if (!hasAccessibleName) { | |
| return ( | |
| <div> | |
| {title && <h2 className="text-text-primary font-bold text-xl mb-3">{title}</h2>} | |
| {children} | |
| </div> | |
| ); | |
| } | |
| return ( | |
| <section {...sectionProps}> |
| return ( | ||
| <button | ||
| className="flex flex-col gap-2 w-40 shrink-0 text-left group focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-white rounded-md" | ||
| onClick={() => navigate(`/playlist/${playlist.id}`)} | ||
| > | ||
| <div className="relative w-40 h-40 rounded-md overflow-hidden bg-surface-hover"> | ||
| {image ? ( | ||
| <img | ||
| src={image} | ||
| alt={playlist.name} | ||
| className="w-full h-full object-cover group-hover:scale-105 transition-transform duration-300" | ||
| /> | ||
| ) : ( | ||
| <div className="w-full h-full flex items-center justify-center bg-accent/20 text-accent font-bold text-2xl"> | ||
| {initials} | ||
| </div> | ||
| )} | ||
|
|
||
| {/* Play/Pause button — bottom right corner */} | ||
| <div | ||
| className={`absolute bottom-2 right-2 transition-all duration-200 ${isActive ? 'opacity-100 translate-y-0' : 'opacity-0 translate-y-2 group-hover:opacity-100 group-hover:translate-y-0'}`} | ||
| onClick={handlePlayPause} | ||
| > |
There was a problem hiding this comment.
The play/pause control is a clickable <div> inside a clickable <button>. This makes the play/pause action non-focusable/non-keyboard accessible and mixes interactive semantics (nested interactive region). Consider restructuring so the card container is not a <button> (e.g., use a <div>/<Link> for navigation) and render the play/pause as a real <button type="button"> with an accessible label.
| const updateArrows = () => { | ||
| const el = scrollRef.current; | ||
| if (!el) return; | ||
| setCanScrollLeft(el.scrollLeft > 0); | ||
| setCanScrollRight(el.scrollLeft + el.clientWidth < el.scrollWidth - 1); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| updateArrows(); | ||
| const el = scrollRef.current; | ||
| if (!el) return; | ||
| el.addEventListener('scroll', updateArrows); | ||
| window.addEventListener('resize', updateArrows); | ||
| return () => { | ||
| el.removeEventListener('scroll', updateArrows); | ||
| window.removeEventListener('resize', updateArrows); | ||
| }; | ||
| }, [data]); |
There was a problem hiding this comment.
This effect registers event listeners using updateArrows, but updateArrows is re-created on every render and isn't included in the dependency list. With eslint-plugin-react-hooks enabled, this will trigger an exhaustive-deps warning and can lead to subtle listener/closure issues later. Consider moving updateArrows inside the effect or wrapping it in useCallback and including it in the deps array.
| className="flex gap-4 overflow-x-auto pb-2" | ||
| style={{ scrollbarWidth: 'none', msOverflowStyle: 'none' }} |
There was a problem hiding this comment.
The inline scrollbar-hiding styles (scrollbarWidth / msOverflowStyle) only cover Firefox/legacy IE; Chrome/Safari will still show a scrollbar unless ::-webkit-scrollbar is handled. If the intent is to hide the scrollbar cross-browser, consider adding a shared CSS utility (similar to scrollbar-thin in src/index.css) and applying it here instead of inline styles.
| className="flex gap-4 overflow-x-auto pb-2" | |
| style={{ scrollbarWidth: 'none', msOverflowStyle: 'none' }} | |
| className="flex gap-4 overflow-x-auto pb-2 scrollbar-thin" |
| export function MadeForYouSection() { | ||
| const { data, isLoading } = useUserPlaylists(50); | ||
| const { data: profile } = useCurrentUserProfile(); | ||
| const { data: playback } = useCurrentPlayback(); | ||
| const { play, pause } = usePlaybackControls(); | ||
| const { deviceId } = usePlayerStore(); | ||
|
|
||
| const sectionTitle = `Made For ${profile?.display_name ?? 'You'}`; | ||
| const activeContextUri = playback?.context?.uri ?? null; | ||
| const isPlaybackActive = playback?.is_playing ?? false; | ||
|
|
There was a problem hiding this comment.
MadeForYouSection introduces non-trivial UI logic (play/pause behavior tied to playback state + horizontal scrolling/arrow affordances) but has no accompanying tests. Since other home sections are covered by RTL/Vitest tests, adding a MadeForYouSection.test.tsx covering rendering, arrow visibility behavior, and play/pause click behavior would help prevent regressions.
|
No description provided.