Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
167 changes: 158 additions & 9 deletions webapp/e2e/studio-nocturne.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2094,6 +2094,153 @@ test("puts the songs playback action beside the sort and names its scope", async
await expect(page.locator(".player-track strong")).toHaveText("Army of Me");
});

/**
* Every credited artist in a song list is a link to its own page.
*
* The column used to be the display string as plain text, so a playlist or a
* mix offered no way to reach an artist. A track credited to two artists names
* two pages; a track whose credits the server did not list still links through
* its primary credit; an artist the catalogue does not know stays text.
*/
test("links each credited artist in a song list to its page", async ({
page,
}, testInfo) => {
await page.route("**/api/v2/songs?*", (route) =>
route.fulfill({
json: [
{
...track,
id: "song-duo",
title: "Soleil Bleu",
artist: "Bleu Soleil, LUIZA",
artist_id: "artist-1",
artists: [
{ id: "artist-1", name: "Bleu Soleil" },
{ id: "artist-2", name: "LUIZA" },
],
},
{
...track,
id: "song-primary",
title: "Second track",
artist: "Björk",
artist_id: "artist-1",
artists: [],
},
{
...track,
id: "song-unknown",
title: "Third track",
artist: "Nobody We Know",
artist_id: null,
artists: [],
},
],
}),
);
await page.goto("/songs");
const rows = page.locator(".songs tbody tr");
await expect(rows).toHaveCount(3);

// Every layout: the track's menu names each credited artist, which is how a
// phone, whose song table hides the artist column, reaches one.
await rows.nth(0).locator("button.song-title").focus();
await page.keyboard.press("Shift+F10");
const menu = page.getByRole("menu", { name: "Actions for Soleil Bleu" });
await expect(menu.getByRole("menuitem", { name: /^Go to / })).toHaveText([
"Go to album",
"Go to Bleu Soleil",
"Go to LUIZA",
]);
await menu.getByRole("menuitem", { name: "Go to LUIZA" }).click();
await expect(page).toHaveURL(/\/artists\/artist-2$/);
await page.goBack();
await expect(rows).toHaveCount(3);

// The column itself is drawn on a wide screen only.
if (testInfo.project.name !== "desktop") return;
const duo = rows.nth(0).locator(".song-artists");
await expect(duo.getByRole("link")).toHaveCount(2);
await expect(duo).toHaveText("Bleu Soleil, LUIZA");
await expect(duo.getByRole("link", { name: "LUIZA" })).toHaveAttribute(
"href",
"/artists/artist-2",
);

await expect(
rows.nth(1).locator(".song-artists").getByRole("link", { name: "Björk" }),
).toHaveAttribute("href", "/artists/artist-1");
await expect(rows.nth(2).locator(".song-artists").getByRole("link")).toHaveCount(
0,
);
await expect(rows.nth(2).locator(".song-artists")).toHaveText("Nobody We Know");

await expectNoWcagViolations(page);

await duo.getByRole("link", { name: "Bleu Soleil" }).click();
await expect(page).toHaveURL(/\/artists\/artist-1$/);
});

/**
* A track menu taller than the window stays on screen and scrolls itself.
*
* One "Go to" entry per credited artist means a compilation track can carry a
* menu longer than a short window. It is capped at the window and scrolls on
* its own; the wheel over it scrolls it rather than closing it, and the wheel
* over the page still closes it.
*/
test("keeps a long track menu on screen and lets it scroll", async ({
page,
}) => {
await page.setViewportSize({ width: 900, height: 420 });
const many = Array.from({ length: 30 }, (_, index) => ({
id: `artist-${index + 1}`,
name: `Guest ${index + 1}`,
}));
await page.route("**/api/v2/songs?*", (route) =>
route.fulfill({
json: [
{
...track,
id: "song-crowd",
title: "Crowded",
artist: many.map((artist) => artist.name).join(", "),
artist_id: "artist-1",
artists: many,
},
],
}),
);
await page.goto("/songs");
await page.locator(".songs tbody tr button.song-title").first().focus();
await page.keyboard.press("Shift+F10");
const menu = page.getByRole("menu", { name: "Actions for Crowded" });
await expect(menu).toBeVisible();

const box = await menu.boundingBox();
expect(box).not.toBeNull();
expect((box?.y ?? 0) + (box?.height ?? 0)).toBeLessThanOrEqual(420);
expect(await menu.evaluate((node) => node.scrollHeight > node.clientHeight)).toBe(
true,
);

// The wheel over the menu scrolls it and leaves it open.
await menu.hover();
await page.mouse.wheel(0, 400);
await expect
.poll(() => menu.evaluate((node) => node.scrollTop))
.toBeGreaterThan(0);
await expect(menu).toBeVisible();
const last = menu.getByRole("menuitem", { name: "Go to Guest 30" });
await last.scrollIntoViewIfNeeded();
await expect(last).toBeInViewport();

// The wheel anywhere else still closes it.
await page.mouse.move(880, 400);
await page.mouse.wheel(0, 100);
await expect(menu).toHaveCount(0);
});

/**
* The right click on a track, and the properties it opens (RFC-012 decision 4).
*
Expand Down Expand Up @@ -2141,6 +2288,9 @@ test("opens a track's properties from its context menu", async ({ page }) => {
"Download",
// RFC-022: this browser supports Cache Storage, so it offers the copy.
"Keep for offline",
// Where the track belongs, reachable from any list and on a phone.
"Go to album",
"Go to Skunk Anansie",
"Properties",
"Edit tags",
"Move to folder…",
Expand All @@ -2151,11 +2301,11 @@ test("opens a track's properties from its context menu", async ({ page }) => {
"outline-style",
"solid",
);
// Past "Keep for offline" (RFC-022), the fourth item.
await page.keyboard.press("ArrowDown");
await page.keyboard.press("ArrowDown");
await page.keyboard.press("ArrowDown");
await page.keyboard.press("ArrowDown");
// Past "Keep for offline" (RFC-022) and the album and artist entries, the
// seventh item.
for (let press = 0; press < 6; press += 1) {
await page.keyboard.press("ArrowDown");
}
await expect(menu.getByRole("menuitem", { name: "Properties" })).toBeFocused();
await page.keyboard.press("Enter");

Expand All @@ -2182,10 +2332,9 @@ test("opens a track's properties from its context menu", async ({ page }) => {

// The backdrop, outside the panel's box, does close it.
await page.keyboard.press("Shift+F10");
await page.keyboard.press("ArrowDown");
await page.keyboard.press("ArrowDown");
await page.keyboard.press("ArrowDown");
await page.keyboard.press("ArrowDown");
for (let press = 0; press < 6; press += 1) {
await page.keyboard.press("ArrowDown");
}
await page.keyboard.press("Enter");
await expect(panel).toBeVisible();
await page.mouse.click(2, 2);
Expand Down
7 changes: 7 additions & 0 deletions webapp/src/api/catalog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ export type Song = {
album: string | null;
artist: string | null;
artist_id: string | null;
/**
* Every credited artist, in tag order: the structured form of `artist`,
* which stays the display string. On the wire since `SongItem` gained it;
* declared here so each name can link to its own page. Optional, so a mock
* that leaves it out is a track credited by its display string alone.
*/
artists?: Array<{ id: string; name: string }>;
artwork_hash: string | null;
duration_ms: number;
track: number | null;
Expand Down
30 changes: 29 additions & 1 deletion webapp/src/components/song-table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { Link } from "@tanstack/react-router";
import { type ReactNode, useState } from "react";

import { formatDuration, type Song, setFavorite, setRating } from "../api";
import { creditedArtists } from "../lib/artist-credits";
import { usePlayer } from "../player/player";
import { useI18n } from "../state/i18n";
import { mayCorrectTracks, useLibraryScope } from "../state/library-scope";
Expand All @@ -12,6 +13,31 @@ import { useTrackMenu } from "./track-menu";

const RATING_STARS = [1, 2, 3, 4, 5];

/**
* A track's artists, each a link to its own page.
*
* A track credited to two artists ("Bleu Soleil, LUIZA") names two pages, so
* the structured list is what is linked, in tag order, rather than the display
* string. Where the list is empty, the primary credit still links if it has an
* id, and an artist the catalogue does not know stays plain text.
*/
function ArtistLinks({ song }: { song: Song }) {
const credited = creditedArtists(song);
if (credited.length === 0) return <>{song.artist}</>;
return (
<>
{credited.map((artist, index) => (
<span key={artist.id}>
{index > 0 ? ", " : null}
<Link to="/artists/$artistId" params={{ artistId: artist.id }}>
{artist.name}
</Link>
</span>
))}
</>
);
}

/**
* What a dragged row carries: the track ids, as JSON. A type of its own, so a
* folder takes a drop from a song row and nothing else.
Expand Down Expand Up @@ -224,7 +250,9 @@ export function SongTable({
</button>
</td>
{variant === "default" ? (
<td className="muted">{song.artist}</td>
<td className="muted song-artists">
<ArtistLinks song={song} />
</td>
) : null}
<td className="muted song-duration">
{formatDuration(song.duration_ms)}
Expand Down
43 changes: 39 additions & 4 deletions webapp/src/components/track-menu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
import { createPortal } from "react-dom";

import { placeTracks, type Song, streamUrl } from "../api";
import { creditedArtists } from "../lib/artist-credits";
import { downloadUrl, saveFrom } from "../lib/download";
import {
forgetOffline,
Expand Down Expand Up @@ -341,10 +342,17 @@ function TrackMenu({
const away = (event: Event) => {
if (!list.current?.contains(event.target as Node)) closeRef.current();
};
// The wheel closes it wherever it turns, over the menu too: the menu does
// not scroll, so a wheel over it scrolls the page beneath and leaves the
// menu where its row used to be.
const wheel = () => closeRef.current();
// The wheel closes it, over the menu too, unless the menu has rows to
// scroll: a track credited to many artists makes it taller than a short
// window, and it is then capped and scrolls on its own. A menu with nothing
// to scroll would hand the wheel to the page beneath and leave the menu
// where its row used to be, so that one still closes.
const wheel = (event: Event) => {
const menu = list.current;
const inside = menu?.contains(event.target as Node) ?? false;
if (inside && menu && menu.scrollHeight > menu.clientHeight) return;
closeRef.current();
};
const leave = () => closeRef.current();
// A long press opens the menu with the finger still down, and that finger
// drifts as it lifts: its moves are the gesture that opened the menu, not
Expand Down Expand Up @@ -429,6 +437,33 @@ function TrackMenu({
},
]
: []),
// Where the track belongs, from every list it appears in. The artist
// column of a song table is hidden on a narrow screen, so this is how a
// phone reaches an artist from a playlist or a mix.
...(song.album_id
? [
{
key: "album",
icon: "albums" as const,
label: t("trackMenu.goToAlbum"),
run: () =>
void navigate({
to: "/albums/$albumId",
params: { albumId: song.album_id as string },
}),
},
]
: []),
...creditedArtists(song).map((artist) => ({
key: `artist-${artist.id}`,
icon: "artists" as const,
label: t("trackMenu.goToArtist", { name: artist.name }),
run: () =>
void navigate({
to: "/artists/$artistId",
params: { artistId: artist.id },
}),
})),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
{
key: "properties",
icon: "info",
Expand Down
42 changes: 42 additions & 0 deletions webapp/src/lib/artist-credits.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { describe, expect, it } from "vitest";

import { creditedArtists } from "./artist-credits";

describe("creditedArtists", () => {
it("uses the structured list, in tag order, when the server sends one", () => {
expect(
creditedArtists({
artist: "Bleu Soleil, LUIZA",
artist_id: "artist-1",
artists: [
{ id: "artist-1", name: "Bleu Soleil" },
{ id: "artist-2", name: "LUIZA" },
],
}),
).toEqual([
{ id: "artist-1", name: "Bleu Soleil" },
{ id: "artist-2", name: "LUIZA" },
]);
});

it("falls back to the primary credit when the list is missing or empty", () => {
const primary = [{ id: "artist-1", name: "Björk" }];
expect(creditedArtists({ artist: "Björk", artist_id: "artist-1" })).toEqual(
primary,
);
expect(
creditedArtists({ artist: "Björk", artist_id: "artist-1", artists: [] }),
).toEqual(primary);
});

it("links nothing for an artist the catalogue does not know", () => {
expect(
creditedArtists({
artist: "Nobody We Know",
artist_id: null,
artists: [],
}),
).toEqual([]);
expect(creditedArtists({ artist: null, artist_id: null })).toEqual([]);
});
});
21 changes: 21 additions & 0 deletions webapp/src/lib/artist-credits.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import type { Song } from "../api";

/** One artist a track can link to. */
export type CreditedArtist = { id: string; name: string };

/**
* The artists a track can link to, in tag order.
*
* The structured list the server sends, where it has one. Otherwise the
* primary credit, if the catalogue knows it by an id, and nothing at all for
* an artist it does not know: a name with no page behind it is not a link.
*/
export function creditedArtists(
song: Pick<Song, "artist" | "artist_id" | "artists">,
): CreditedArtist[] {
if (song.artists && song.artists.length > 0) return song.artists;
if (song.artist_id && song.artist) {
return [{ id: song.artist_id, name: song.artist }];
}
return [];
}
Loading
Loading