Skip to content

Add album link detection and lyrics list UI with loading/partial-error handling#3

Open
spacegrey23 wants to merge 1 commit intomainfrom
codex/update-uselyrics-to-handle-album-urls
Open

Add album link detection and lyrics list UI with loading/partial-error handling#3
spacegrey23 wants to merge 1 commit intomainfrom
codex/update-uselyrics-to-handle-album-urls

Conversation

@spacegrey23
Copy link
Copy Markdown
Owner

Motivation

  • Detect when a pasted URL is an album (contains /album/) and fetch per-track data instead of a single track.
  • Return a normalized list of lyric items from the hook so the UI can render multiple tracks for albums.
  • Surface loading states and partial errors so users can see progress and which tracks failed.
  • Keep backward compatibility for single-track links while supporting album responses.

Description

  • Update useLyrics in client/src/hooks/useLyrics.js and client/src/features/lyrics/useLyrics.js to detect album links via isAlbumLink, call getAlbumData for albums or getSongData for songs, and select/normalize the response into items (list of normalized track objects).
  • Add normalization helpers extractItems, normalizeItem, and normalizeResults to support varied backend payload shapes and to produce id, songName, artistName, albumName, albumImage, syncedLyrics, plainLyrics, and errorMessage per item.
  • Replace single-LyricsCard rendering in client/src/App.jsx with list-aware rendering that shows a SongCard for the primary item and maps lyricsItems to one LyricsCard per track with per-item handleCopy/handleDownload wiring, list-level loading banner, and partial-error banner.
  • Enhance client/src/components/LyricsCard/LyricsCard.jsx to support title, subtitle, isLoading, and errorMessage props, render loading skeletons, show an explicit empty state, and disable copy/download when no lyrics exist.

Testing

  • Ran npm install in the client directory which completed successfully.
  • Started the dev server with npm run dev and Vite started and served the app successfully.
  • Captured a UI screenshot via a Playwright script to verify the new list UI, which executed and produced an artifact successfully.
  • No automated unit tests were run as part of this change (UI/workflow verification only).

Codex Task

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c537e64d98

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +61 to +63
const albumLink = isAlbumLink(songLink)
const endpoint = albumLink ? 'getAlbumData' : 'getSongData'
const response = await fetch(`${import.meta.env.VITE_MY_BACKEND_LINK}${endpoint}?url=${encodeURIComponent(songLink)}`)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Route album links to a non-existent backend endpoint

When a pasted URL contains /album/, the client now switches to getAlbumData, but the backend in this repo only registers /getSongData (see backend/api/routes/routes.mjs). With no getAlbumData route, album URLs will always hit a 404 and surface an error, so the new album flow never succeeds unless a matching backend endpoint is added.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant