diff --git a/internal/api/convert.go b/internal/api/convert.go index 9560a679..a7148825 100644 --- a/internal/api/convert.go +++ b/internal/api/convert.go @@ -183,3 +183,13 @@ func parsePaging(raw url.Values) (limit, offset int, err error) { } return limit, offset, nil } + +// nonNilStrings guarantees a JSON array rather than null. The clients iterate +// these without a null check, matching how every other list field in this +// package is emitted. +func nonNilStrings(in []string) []string { + if in == nil { + return []string{} + } + return in +} diff --git a/internal/api/library.go b/internal/api/library.go index cc919a81..a4e2c99c 100644 --- a/internal/api/library.go +++ b/internal/api/library.go @@ -82,9 +82,17 @@ func (h *handlers) handleGetAlbum(w http.ResponseWriter, r *http.Request) { refs = append(refs, ref) durSec += ref.DurationSec } + // Genre chips are a navigation nicety, so a failure here must not 404 an + // album that loaded fine. Log and ship the detail without them. + genres, err := q.ListGenresForAlbum(r.Context(), album.ID) + if err != nil { + h.logger.Warn("api: list album genres failed", "err", err, "album_id", uuidToString(album.ID)) + genres = nil + } detail := AlbumDetail{ AlbumRef: albumRefFrom(album, artistName, len(tracks), durSec), Tracks: refs, + Genres: nonNilStrings(genres), } writeJSON(w, http.StatusOK, detail) } @@ -114,9 +122,15 @@ func (h *handlers) handleGetArtist(w http.ResponseWriter, r *http.Request) { // durationSec=0: not aggregated for nested album lists per spec data flow. refs = append(refs, albumRefFrom(row.Album, artist.Name, int(row.TrackCount), 0)) } + genres, err := q.ListGenresForArtist(r.Context(), artist.ID) + if err != nil { + h.logger.Warn("api: list artist genres failed", "err", err, "artist_id", uuidToString(artist.ID)) + genres = nil + } detail := ArtistDetail{ ArtistRef: artistRefFrom(artist, len(rows)), Albums: refs, + Genres: nonNilStrings(genres), } writeJSON(w, http.StatusOK, detail) } diff --git a/internal/api/types.go b/internal/api/types.go index bc9f98d1..53e872f6 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -89,12 +89,19 @@ type TrackRef struct { type ArtistDetail struct { ArtistRef Albums []AlbumRef `json:"albums"` + // Genres carried by this artist's tracks, for quick-jump chips (#367). + // Always non-nil at JSON so the client can iterate without a null check. + Genres []string `json:"genres"` } // AlbumDetail is the response body of GET /api/albums/{id}. type AlbumDetail struct { AlbumRef Tracks []TrackRef `json:"tracks"` + // Genres carried by this album's tracks, for quick-jump chips (#367). + // Derived from the tracks rather than stored on the album, because genre + // lives on tracks and an album's tracks can disagree. Non-nil at JSON. + Genres []string `json:"genres"` } // SearchResponse is the body of GET /api/search. Each facet carries its own diff --git a/internal/db/dbq/browse.sql.go b/internal/db/dbq/browse.sql.go index bbb7fc8e..be5de0f4 100644 --- a/internal/db/dbq/browse.sql.go +++ b/internal/db/dbq/browse.sql.go @@ -7,6 +7,8 @@ package dbq import ( "context" + + "github.com/jackc/pgx/v5/pgtype" ) const countAlbumsByGenre = `-- name: CountAlbumsByGenre :one @@ -212,6 +214,69 @@ func (q *Queries) ListAlbumsByYearRangeWithArtist(ctx context.Context, arg ListA return items, nil } +const listGenresForAlbum = `-- name: ListGenresForAlbum :many +SELECT DISTINCT trim(g.genre) AS genre +FROM tracks +JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true +WHERE tracks.album_id = $1 AND trim(g.genre) <> '' +ORDER BY trim(g.genre) +` + +// Distinct genres carried by an album's tracks, for the album detail page's +// quick-jump chips. Split and trimmed identically to ListGenresWithCount, so a +// chip always leads to a page that actually contains this album — the two +// diverging is exactly the bug #367 had to fix in ListAlbumsByGenre. +func (q *Queries) ListGenresForAlbum(ctx context.Context, albumID pgtype.UUID) ([]string, error) { + rows, err := q.db.Query(ctx, listGenresForAlbum, albumID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []string + for rows.Next() { + var genre string + if err := rows.Scan(&genre); err != nil { + return nil, err + } + items = append(items, genre) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + +const listGenresForArtist = `-- name: ListGenresForArtist :many +SELECT DISTINCT trim(g.genre) AS genre +FROM tracks +JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true +WHERE tracks.artist_id = $1 AND trim(g.genre) <> '' +ORDER BY trim(g.genre) +` + +// Same, across everything by one artist. Alphabetical rather than by count: +// an artist's genre set is small, and a stable order reads better than a +// frequency ranking nobody asked about. +func (q *Queries) ListGenresForArtist(ctx context.Context, artistID pgtype.UUID) ([]string, error) { + rows, err := q.db.Query(ctx, listGenresForArtist, artistID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []string + for rows.Next() { + var genre string + if err := rows.Scan(&genre); err != nil { + return nil, err + } + items = append(items, genre) + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const listGenresWithCount = `-- name: ListGenresWithCount :many SELECT trim(g.genre) AS genre, COUNT(DISTINCT tracks.id)::bigint AS track_count FROM tracks diff --git a/internal/db/queries/browse.sql b/internal/db/queries/browse.sql index 58002299..3a1e3385 100644 --- a/internal/db/queries/browse.sql +++ b/internal/db/queries/browse.sql @@ -86,3 +86,24 @@ SELECT COUNT(*) FROM albums WHERE release_date IS NOT NULL AND EXTRACT(YEAR FROM release_date)::int BETWEEN sqlc.arg(year_from)::int AND sqlc.arg(year_to)::int; + +-- name: ListGenresForAlbum :many +-- Distinct genres carried by an album's tracks, for the album detail page's +-- quick-jump chips. Split and trimmed identically to ListGenresWithCount, so a +-- chip always leads to a page that actually contains this album — the two +-- diverging is exactly the bug #367 had to fix in ListAlbumsByGenre. +SELECT DISTINCT trim(g.genre) AS genre +FROM tracks +JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true +WHERE tracks.album_id = $1 AND trim(g.genre) <> '' +ORDER BY trim(g.genre); + +-- name: ListGenresForArtist :many +-- Same, across everything by one artist. Alphabetical rather than by count: +-- an artist's genre set is small, and a stable order reads better than a +-- frequency ranking nobody asked about. +SELECT DISTINCT trim(g.genre) AS genre +FROM tracks +JOIN LATERAL regexp_split_to_table(coalesce(tracks.genre, ''), '[;,]') AS g(genre) ON true +WHERE tracks.artist_id = $1 AND trim(g.genre) <> '' +ORDER BY trim(g.genre); diff --git a/web/src/lib/api/types.ts b/web/src/lib/api/types.ts index 17a7f791..bb1645c3 100644 --- a/web/src/lib/api/types.ts +++ b/web/src/lib/api/types.ts @@ -45,10 +45,14 @@ export type TrackRef = { export type ArtistDetail = ArtistRef & { albums: AlbumRef[]; + // Genres across this artist's tracks (#367). Server guarantees an array. + genres: string[]; }; export type AlbumDetail = AlbumRef & { tracks: TrackRef[]; + // Genres across this album's tracks (#367). Server guarantees an array. + genres: string[]; }; export type Playlist = { diff --git a/web/src/lib/components/AlbumCard.test.ts b/web/src/lib/components/AlbumCard.test.ts index 79d55b20..0c2ad29f 100644 --- a/web/src/lib/components/AlbumCard.test.ts +++ b/web/src/lib/components/AlbumCard.test.ts @@ -91,7 +91,7 @@ describe('AlbumCard', () => { duration_sec: 545 }) ]; - const detail: AlbumDetail = { ...album, tracks }; + const detail: AlbumDetail = { ...album, tracks, genres: [] }; (api.get as ReturnType).mockResolvedValueOnce(detail); render(AlbumCard, { props: { album } }); @@ -112,7 +112,7 @@ describe('AlbumCard', () => { duration_sec: 545 }) ]; - const detail: AlbumDetail = { ...album, tracks }; + const detail: AlbumDetail = { ...album, tracks, genres: [] }; (api.get as ReturnType).mockResolvedValueOnce(detail); render(AlbumCard, { props: { album } }); diff --git a/web/src/routes/albums/[id]/+page.svelte b/web/src/routes/albums/[id]/+page.svelte index 355265be..b2ff3dc3 100644 --- a/web/src/routes/albums/[id]/+page.svelte +++ b/web/src/routes/albums/[id]/+page.svelte @@ -126,8 +126,31 @@

{album.artist_name}

+ {#if album.year} -

{album.year}

+

+ + {album.year} + +

+ {/if} + {#if album.genres?.length} + {/if}

{album.track_count} {album.track_count === 1 ? 'track' : 'tracks'} diff --git a/web/src/routes/albums/[id]/album.test.ts b/web/src/routes/albums/[id]/album.test.ts index 23c1427f..005a388d 100644 --- a/web/src/routes/albums/[id]/album.test.ts +++ b/web/src/routes/albums/[id]/album.test.ts @@ -49,7 +49,8 @@ describe('album detail page', () => { year: 1959, track_count: 2, duration_sec: 544 + 565, cover_url: '/api/albums/xyz/cover', cover_art_source: null, - tracks: [track('t1', 'So What', 1, 544), track('t2', 'Freddie Freeloader', 2, 565)] + tracks: [track('t1', 'So What', 1, 544), track('t2', 'Freddie Freeloader', 2, 565)], + genres: ['Jazz'] }; (createAlbumQuery as ReturnType).mockReturnValue(mockQuery({ data: detail })); render(AlbumPage); @@ -77,7 +78,8 @@ describe('album detail page', () => { track_count: 0, duration_sec: 0, cover_url: '/api/albums/xyz/cover', cover_art_source: null, - tracks: [] + tracks: [], + genres: [] }; (createAlbumQuery as ReturnType).mockReturnValue(mockQuery({ data: detail })); render(AlbumPage); @@ -119,4 +121,51 @@ describe('album detail page', () => { vi.useRealTimers(); } }); + + // Quick-jumps into the browse axes (#367). The genre href must be encoded: + // a slash-bearing tag is why browse selection lives in the query string. + test('year and genre are quick-jump links into the browse axes', () => { + const detail: AlbumDetail = { + id: 'xyz', title: 'Kind of Blue', sort_title: 'Kind of Blue', + artist_id: 'md', artist_name: 'Miles Davis', + year: 1959, track_count: 0, duration_sec: 0, + cover_url: '/api/albums/xyz/cover', + cover_art_source: null, + tracks: [], + genres: ['Jazz', 'Rock/Pop'] + }; + (createAlbumQuery as ReturnType).mockReturnValue(mockQuery({ data: detail })); + render(AlbumPage); + + expect(screen.getByRole('link', { name: '1959' })).toHaveAttribute( + 'href', + '/library/years?y=1959' + ); + expect(screen.getByRole('link', { name: 'Jazz' })).toHaveAttribute( + 'href', + '/library/genres?g=Jazz' + ); + expect(screen.getByRole('link', { name: 'Rock/Pop' })).toHaveAttribute( + 'href', + '/library/genres?g=Rock%2FPop' + ); + }); + + test('no genre chips when the album carries no genre tags', () => { + const detail: AlbumDetail = { + id: 'xyz', title: 'Untagged', sort_title: 'Untagged', + artist_id: 'md', artist_name: 'Miles Davis', + track_count: 0, duration_sec: 0, + cover_url: '/api/albums/xyz/cover', + cover_art_source: null, + tracks: [], + genres: [] + }; + (createAlbumQuery as ReturnType).mockReturnValue(mockQuery({ data: detail })); + render(AlbumPage); + + expect(screen.queryByRole('link', { name: /library\/genres/ })).toBeNull(); + // No year on this fixture either, so no year jump. + expect(screen.queryByRole('link', { name: /^\d{4}$/ })).toBeNull(); + }); }); diff --git a/web/src/routes/artists/[id]/+page.svelte b/web/src/routes/artists/[id]/+page.svelte index 3f829273..8da5a2e9 100644 --- a/web/src/routes/artists/[id]/+page.svelte +++ b/web/src/routes/artists/[id]/+page.svelte @@ -90,6 +90,24 @@

{detail.album_count} {detail.album_count === 1 ? 'album' : 'albums'}

+ + {#if detail.genres?.length} + + {/if}