From 24c72e05e615a3459561eade8a40a6bac39aa3f7 Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:27:45 +0000 Subject: [PATCH 01/11] Player core: keep the details of a playback failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A playback error was only a short message ("Couldn't play this track."), so there was nothing to go on when a track wouldn't play. It now carries what happened: when, the source the player was given (a server path without its origin, or a downloaded copy's URI), what the audio player said (the adapter's new optional errorDetail()), and what the server answered when asked for the stream again. The stream is now asked again for every streamed track, library files included, so a missing file shows as "HTTP 410 · file missing on disk". Only TIDAL's 502/503 reason still replaces the message on the bar. playbackErrorLines/playbackErrorText lay the details out for display and for copying into a bug report. Co-Authored-By: Claude Opus 5.5 --- core/src/player/audio-adapter.ts | 5 ++ core/src/player/index.ts | 7 +++ core/src/player/playback-error.ts | 42 ++++++++++++++++ core/src/player/player-core.ts | 17 +++++++ core/src/player/use-player-core.ts | 74 ++++++++++++++++++++-------- core/tests/playback-error.test.ts | 55 +++++++++++++++++++++ core/tests/use-player-core.test.tsx | 75 +++++++++++++++++++++++------ 7 files changed, 241 insertions(+), 34 deletions(-) create mode 100644 core/src/player/playback-error.ts create mode 100644 core/tests/playback-error.test.ts diff --git a/core/src/player/audio-adapter.ts b/core/src/player/audio-adapter.ts index 68b4eb35..6fecc58b 100644 --- a/core/src/player/audio-adapter.ts +++ b/core/src/player/audio-adapter.ts @@ -68,6 +68,11 @@ export interface AudioAdapter { duration(): number; /** Subscribe to an event. Returns an unsubscribe function. */ on(event: AudioAdapterEvent, handler: () => void): () => void; + /** + * What the platform said about the latest `error`, for the failure's + * details; null when it said nothing. + */ + errorDetail?(): string | null; /** Release any resources held by this adapter. */ dispose(): void; } diff --git a/core/src/player/index.ts b/core/src/player/index.ts index 764549a3..a0bf42e6 100644 --- a/core/src/player/index.ts +++ b/core/src/player/index.ts @@ -8,6 +8,8 @@ export { fisherYatesWithAnchor, repeatModeLabel, SMOOTH_MIX_SECONDS, + type PlaybackError, + type PlaybackErrorDetails, type PlayerControls, type PlayerState, type PlayOptions, @@ -20,6 +22,11 @@ export { type AudioAdapterEvent, } from "./audio-adapter"; export { usePlayerCore } from "./use-player-core"; +export { + playbackErrorLines, + playbackErrorText, + type PlaybackErrorLine, +} from "./playback-error"; export { activityTrack, playbackDeviceButtonLabel, diff --git a/core/src/player/playback-error.ts b/core/src/player/playback-error.ts new file mode 100644 index 00000000..52cb2454 --- /dev/null +++ b/core/src/player/playback-error.ts @@ -0,0 +1,42 @@ +import type { TrackListItem } from "../api"; +import { displayText } from "../format"; +import type { PlaybackError } from "./player-core"; + +/** One labelled line of a playback failure's details. */ +export interface PlaybackErrorLine { + label: string; + value: string; +} + +/** + * A playback failure's details as labelled lines, in the order they're shown + * and copied. `track` is the failed track, while it's still loaded. + */ +export function playbackErrorLines( + error: PlaybackError, + track: TrackListItem | null, +): PlaybackErrorLine[] { + const { at, source, player, server } = error.details; + const lines: PlaybackErrorLine[] = [{ label: "Error", value: error.message }]; + if (track?.id === error.trackId) { + const artist = displayText(track.artist); + const title = displayText(track.title, "Untitled"); + lines.push({ label: "Track", value: artist ? `${title} — ${artist}` : title }); + } + lines.push( + { label: "Track ID", value: error.trackId }, + { label: "Source", value: source }, + { label: "Player", value: player ?? "Gave no reason" }, + { + label: "Server", + value: server === undefined ? "Asking…" : (server ?? "Not asked: a downloaded copy"), + }, + { label: "Time", value: new Date(at).toISOString() }, + ); + return lines; +} + +/** The lines as plain text, to copy into a bug report. */ +export function playbackErrorText(lines: PlaybackErrorLine[]): string { + return lines.map(({ label, value }) => `${label}: ${value}`).join("\n"); +} diff --git a/core/src/player/player-core.ts b/core/src/player/player-core.ts index fda7b3d0..69a08aa4 100644 --- a/core/src/player/player-core.ts +++ b/core/src/player/player-core.ts @@ -7,6 +7,23 @@ export interface PlaybackError { trackId: string; /** Short and user-facing: the server's reason, or a generic one. */ message: string; + /** What happened, for a closer look or a bug report. */ + details: PlaybackErrorDetails; +} + +/** The facts behind a `PlaybackError`. */ +export interface PlaybackErrorDetails { + /** When it failed, in ms since the epoch. */ + at: number; + /** What the player was given: a server path, or a downloaded copy's URI. */ + source: string; + /** What the audio player said about it; null when it said nothing. */ + player: string | null; + /** + * What the server answered when asked for the stream again. Undefined + * while it's being asked; null when it wasn't (a downloaded copy). + */ + server?: string | null; } export interface PlayerState { diff --git a/core/src/player/use-player-core.ts b/core/src/player/use-player-core.ts index 448df427..09016eca 100644 --- a/core/src/player/use-player-core.ts +++ b/core/src/player/use-player-core.ts @@ -20,6 +20,7 @@ import { shouldReportPlay, withPlayNext, type PlaybackError, + type PlaybackErrorDetails, type PlayerControls, type PlayerState, type RepeatMode, @@ -91,29 +92,52 @@ function firstPlayableIndex( const PLAYBACK_ERROR_FALLBACK = "Couldn't play this track."; +/** What asking for a failed stream again found. */ +interface StreamProbe { + /** The server's reason it can't serve the stream, when it gave one. */ + reason: string | null; + /** The answer, for the failure's details. */ + answer: string; +} + /** * Why a stream failed, in the server's words: request it once more. The * backend answers a TIDAL stream it can't serve with a 502 or 503 and a short * plain-text reason (and remembers TIDAL's refusal, so this doesn't ask TIDAL - * again). Anything else, such as a stream that works now, a network failure, - * another error or a reverse proxy's HTML error page, gets the generic - * message. + * again). Any other answer, such as a stream that works now, a network + * failure, another error or a reverse proxy's HTML error page, gives no + * reason; it is only described. */ -async function streamFailureMessage(url: string): Promise { +async function probeStream(url: string): Promise { try { const response = await rawFetch(url); - // Nothing to explain; don't download the playlist. + // Nothing to explain; don't download the stream. void response.body?.cancel().catch(() => {}); + const type = response.headers.get("content-type"); + return { reason: null, answer: `HTTP ${response.status}${type ? ` · ${type}` : ""}` }; } catch (error) { - const text = - error instanceof ApiError && (error.status === 502 || error.status === 503) - ? error.message - : ""; - if (text && text.length <= 300 && !text.startsWith("<") && !text.includes("\n")) { - return text; + if (!(error instanceof ApiError)) { + return { reason: null, answer: `No answer: ${String(error)}` }; } + const text = error.message; + const readable = + text.length <= 300 && !text.startsWith("<") && !text.includes("\n") ? text : ""; + return { + reason: readable && (error.status === 502 || error.status === 503) ? readable : null, + answer: `HTTP ${error.status}${readable ? ` · ${readable}` : ""}`, + }; + } +} + +/** A failed source as the details show it: a server address without its origin. */ +function failedSource(uri: string): string { + if (!/^https?:\/\//i.test(uri)) return uri; + try { + const { pathname, search } = new URL(uri); + return pathname + search; + } catch { + return uri; } - return PLAYBACK_ERROR_FALLBACK; } /** @@ -964,16 +988,28 @@ export function usePlayerCore({ if (!track) return; const failure = { trackId: track.id }; failureRef.current = failure; - const report = (message: string) => { - if (failureRef.current === failure) setPlaybackError({ trackId: track.id, message }); + const uri = resolveUri(track.id); + const url = streamUrl(track.id); + // A downloaded copy just didn't play; only a stream can be asked again. + const streamed = uri === url; + const details: PlaybackErrorDetails = { + at: Date.now(), + source: failedSource(uri), + player: adapter.errorDetail?.() ?? null, + server: streamed ? undefined : null, + }; + const report = (message: string, server = details.server) => { + if (failureRef.current === failure) { + setPlaybackError({ trackId: track.id, message, details: { ...details, server } }); + } }; // Say so right away; the server's reason, if any, replaces it. report(PLAYBACK_ERROR_FALLBACK); - // Only the server can say why its TIDAL stream failed. A downloaded - // copy or a library file just didn't play. - const url = streamUrl(track.id); - if (isTidalTrack(track) && resolveUri(track.id) === url) { - void streamFailureMessage(url).then(report); + if (streamed) { + void probeStream(url).then(({ reason, answer }) => + // Only TIDAL streams fail for a reason worth showing on the bar. + report(isTidalTrack(track) && reason ? reason : PLAYBACK_ERROR_FALLBACK, answer), + ); } }); return () => { diff --git a/core/tests/playback-error.test.ts b/core/tests/playback-error.test.ts new file mode 100644 index 00000000..ee7308df --- /dev/null +++ b/core/tests/playback-error.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from "vitest"; +import type { TrackListItem } from "../src/api"; +import { playbackErrorLines, playbackErrorText } from "../src/player/playback-error"; +import type { PlaybackError } from "../src/player/player-core"; + +const track: TrackListItem = { + id: "tidal:1", + title: "Thrills (v2.2)", + artist: "Someone", + duration_ms: 180_000, +}; + +const failure = (details: Partial = {}): PlaybackError => ({ + trackId: "tidal:1", + message: "Couldn't play this track.", + details: { + at: Date.UTC(2026, 9, 29, 12, 0, 0), + source: "/api/tracks/tidal%3A1/stream", + player: "hls.js manifestLoadError: A network error (status 502) occurred while loading manifest", + server: "HTTP 502 · TIDAL refused to stream this track", + ...details, + }, +}); + +describe("playbackErrorLines", () => { + it("lists what happened, in order", () => { + expect(playbackErrorText(playbackErrorLines(failure(), track))).toBe( + [ + "Error: Couldn't play this track.", + "Track: Thrills (v2.2) — Someone", + "Track ID: tidal:1", + "Source: /api/tracks/tidal%3A1/stream", + "Player: hls.js manifestLoadError: A network error (status 502) occurred while loading manifest", + "Server: HTTP 502 · TIDAL refused to stream this track", + "Time: 2026-10-29T12:00:00.000Z", + ].join("\n"), + ); + }); + + it("says when nobody gave a reason, or the server wasn't asked", () => { + const lines = playbackErrorLines(failure({ player: null, server: null }), track); + expect(lines).toContainEqual({ label: "Player", value: "Gave no reason" }); + expect(lines).toContainEqual({ label: "Server", value: "Not asked: a downloaded copy" }); + }); + + it("says the server is still being asked", () => { + const lines = playbackErrorLines(failure({ server: undefined }), track); + expect(lines).toContainEqual({ label: "Server", value: "Asking…" }); + }); + + it("leaves the track out once another one is loaded", () => { + const lines = playbackErrorLines(failure(), { ...track, id: "tidal:2" }); + expect(lines.map((line) => line.label)).not.toContain("Track"); + }); +}); diff --git a/core/tests/use-player-core.test.tsx b/core/tests/use-player-core.test.tsx index ad444709..d3a0cf1c 100644 --- a/core/tests/use-player-core.test.tsx +++ b/core/tests/use-player-core.test.tsx @@ -105,6 +105,12 @@ const ids = (tracks: TrackListItem[]) => tracks.map((track) => track.id); beforeEach(() => { vi.clearAllMocks(); + // A failed stream is asked for again; unless a test answers, it never is. + vi.stubGlobal("fetch", vi.fn(() => new Promise(() => {}))); +}); + +afterEach(() => { + vi.unstubAllGlobals(); }); describe("usePlayerCore", () => { @@ -802,10 +808,6 @@ describe("usePlayerCore playback errors", () => { await new Promise((resolve) => setTimeout(resolve, 0)); }); - afterEach(() => { - vi.unstubAllGlobals(); - }); - it("stops on the failed track and shows the server's reason for a TIDAL stream", async () => { const fetchMock = vi.fn(async () => new Response(refusal, { status: 502 })); vi.stubGlobal("fetch", fetchMock); @@ -815,30 +817,41 @@ describe("usePlayerCore playback errors", () => { act(() => emit("error")); expect(result.current.state.isPlaying).toBe(false); await settle(); - expect(result.current.state.playbackError).toEqual({ trackId: "tidal:1", message: refusal }); + expect(result.current.state.playbackError).toEqual({ + trackId: "tidal:1", + message: refusal, + details: { + at: expect.any(Number), + source: "test://stream/tidal:1", + player: null, + server: `HTTP 502 · ${refusal}`, + }, + }); expect(result.current.state.current?.id).toBe("tidal:1"); expect(fetchMock).toHaveBeenCalledExactlyOnceWith("test://stream/tidal:1", expect.anything()); }); it.each([ - ["a library track", t("a"), undefined], - ["a reverse proxy's HTML page", tidal("1"), () => new Response("502 Bad Gateway", { status: 502 })], - ["a stream that answers now", tidal("1"), () => new Response("#EXTM3U\n", { status: 200 })], - ["an error that isn't TIDAL's", tidal("1"), () => new Response("internal error", { status: 500 })], - ["an unreachable server", tidal("1"), () => Promise.reject(new TypeError("Failed to fetch"))], - ])("shows a generic message for %s", async (_case, track, respond) => { - const fetchMock = vi.fn(async () => respond ? respond() : new Response(refusal, { status: 502 })); + ["a library track's refusal", t("a"), () => new Response(refusal, { status: 502 }), `HTTP 502 · ${refusal}`], + ["a library file gone from disk", t("a"), () => new Response("file missing on disk", { status: 410 }), "HTTP 410 · file missing on disk"], + ["a reverse proxy's HTML page", tidal("1"), () => new Response("502 Bad Gateway", { status: 502 }), "HTTP 502"], + ["a stream that answers now", tidal("1"), () => new Response("#EXTM3U\n", { status: 200, headers: { "Content-Type": "application/vnd.apple.mpegurl" } }), "HTTP 200 · application/vnd.apple.mpegurl"], + ["an error that isn't TIDAL's", tidal("1"), () => new Response("internal error", { status: 500 }), "HTTP 500 · internal error"], + ["an unreachable server", tidal("1"), () => Promise.reject(new TypeError("Failed to fetch")), "No answer: TypeError: Failed to fetch"], + ])("shows a generic message for %s, and the answer in the details", async (_case, track, respond, server) => { + const fetchMock = vi.fn(async () => respond()); vi.stubGlobal("fetch", fetchMock); const { result, emit } = await setup(); act(() => result.current.controls.play(track)); act(() => emit("error")); await settle(); - expect(result.current.state.playbackError).toEqual({ + expect(result.current.state.playbackError).toMatchObject({ trackId: track.id, message: "Couldn't play this track.", + details: { server }, }); - expect(fetchMock).toHaveBeenCalledTimes(respond ? 1 : 0); + expect(fetchMock).toHaveBeenCalledTimes(1); }); it("does not ask the server about a downloaded TIDAL track", async () => { @@ -849,10 +862,41 @@ describe("usePlayerCore playback errors", () => { act(() => emit("error")); await settle(); - expect(result.current.state.playbackError?.message).toBe("Couldn't play this track."); + expect(result.current.state.playbackError).toMatchObject({ + message: "Couldn't play this track.", + details: { source: "file:///offline.flac", server: null }, + }); expect(fetchMock).not.toHaveBeenCalled(); }); + it("keeps what the audio player said about the failure", async () => { + const fake = createFakeAdapter(); + fake.adapter.errorDetail = () => "MEDIA_ERR_DECODE: PIPELINE_ERROR_DECODE"; + const { result } = renderHook(() => + usePlayerCore({ + adapter: fake.adapter, + storage: createMemoryStorage(), + interpolateProgress: false, + }), + ); + act(() => result.current.controls.play(t("a"))); + + act(() => fake.emit("error")); + expect(result.current.state.playbackError?.details.player).toBe( + "MEDIA_ERR_DECODE: PIPELINE_ERROR_DECODE", + ); + }); + + it("shows a server address without its origin", async () => { + const { result, emit } = await setup({ + resolveTrackUri: (id) => `https://music.example/api/tracks/${id}/stream`, + }); + act(() => result.current.controls.play(t("a"))); + + act(() => emit("error")); + expect(result.current.state.playbackError?.details.source).toBe("/api/tracks/a/stream"); + }); + it("ignores a probe that answers after another track started", async () => { let answer!: (response: Response) => void; vi.stubGlobal("fetch", vi.fn(() => new Promise((resolve) => { answer = resolve; }))); @@ -903,6 +947,7 @@ describe("usePlayerCore playback errors", () => { act(() => emit("error")); expect(result.current.state.playbackError?.message).toBe("Couldn't play this track."); + expect(result.current.state.playbackError?.details.server).toBeUndefined(); answer(new Response(refusal, { status: 502 })); await settle(); expect(result.current.state.playbackError?.message).toBe(refusal); From d9c35543b77018c84442c36250768f60e5364bd3 Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:27:45 +0000 Subject: [PATCH 02/11] Player: click a playback error to see what happened and copy it The red error line in the player bar is now a button. It opens a popover above the track listing the failure's details (error, track, source, what the player and the server said, time) with a Copy details button. In the mini player, where a popover has no room, it opens the side panel instead, like the queue and lyrics. The details close when the failure clears: playing again or another track. The HTML audio adapter records what failed for errorDetail(): the element's MediaError (code name and Chromium's message) or the hls.js error it gave up on, never a segment URL, which carries TIDAL's signed address. Co-Authored-By: Claude Opus 5.5 --- frontend/src/adapters/html-audio-adapter.ts | 39 +++++- frontend/src/components/MiniPlayer.tsx | 10 ++ .../src/components/player/MiniPlayerPanel.tsx | 9 +- frontend/src/components/player/NowPlaying.tsx | 80 +++++++++--- .../player/PlaybackErrorPopover.tsx | 118 ++++++++++++++++++ .../components/player/useMiniPlayerMode.ts | 16 ++- frontend/src/styles/player-popovers.css | 44 +++++++ frontend/src/styles/player.css | 24 +++- frontend/tests/html-audio-adapter.test.tsx | 26 ++++ frontend/tests/player-display.test.tsx | 66 +++++++++- frontend/tests/ui/overlays.ui.ts | 23 ++++ 11 files changed, 425 insertions(+), 30 deletions(-) create mode 100644 frontend/src/components/player/PlaybackErrorPopover.tsx diff --git a/frontend/src/adapters/html-audio-adapter.ts b/frontend/src/adapters/html-audio-adapter.ts index e8513c41..d453e65a 100644 --- a/frontend/src/adapters/html-audio-adapter.ts +++ b/frontend/src/adapters/html-audio-adapter.ts @@ -51,6 +51,9 @@ export function useHtmlAudioAdapter(): { // One registry for the adapter's lifetime: useState never replaces it, so // listing it as a dependency below re-runs nothing. const [listeners] = useState(createAudioAdapterEmitter); + // What the element or hls.js said about the latest failure; set right + // before each "error" is emitted. + const errorDetailRef = useRef(null); // Wire native events → listener registry once the element mounts. useEffect(() => { @@ -75,7 +78,9 @@ export function useHtmlAudioAdapter(): { // (it may still report the missing src), and hls.js reports and // recovers the media it drives itself. error: () => { - if (a.getAttribute("src") && !hlsRef.current) dispatchError(); + if (!a.getAttribute("src") || hlsRef.current) return; + errorDetailRef.current = describeMediaError(a.error); + dispatchError(); }, }; for (const [event, handler] of Object.entries(handlers)) { @@ -237,6 +242,7 @@ export function useHtmlAudioAdapter(): { if (!data.fatal || !giveUpOnFatalHlsError(HlsRuntime, hls, data)) return; if (hlsRef.current === hls) { hlsRef.current = null; + errorDetailRef.current = describeHlsError(data); listeners.emit("error"); } }); @@ -318,6 +324,7 @@ export function useHtmlAudioAdapter(): { preparedUrlRef.current = null; } else if (hlsRef.current === hls) { hlsRef.current = null; + errorDetailRef.current = describeHlsError(data); listeners.emit("error"); } }); @@ -409,6 +416,9 @@ export function useHtmlAudioAdapter(): { return activeAudioRef.current?.duration ?? 0; }, on: listeners.on, + errorDetail() { + return errorDetailRef.current; + }, dispose() { finishFade(); loadGenerationRef.current += 1; @@ -440,6 +450,33 @@ export function useHtmlAudioAdapter(): { return { adapter, audioRefs }; } +const MEDIA_ERROR_NAMES: Record = { + 1: "MEDIA_ERR_ABORTED", + 2: "MEDIA_ERR_NETWORK", + 3: "MEDIA_ERR_DECODE", + 4: "MEDIA_ERR_SRC_NOT_SUPPORTED", +}; + +/** + * The element's own account of a failure, e.g. "MEDIA_ERR_SRC_NOT_SUPPORTED: + * DEMUXER_ERROR_COULD_NOT_OPEN: FFmpegDemuxer: open context failed". + */ +function describeMediaError(error: MediaError | null): string | null { + if (!error) return null; + const name = MEDIA_ERROR_NAMES[error.code] ?? `MediaError ${error.code}`; + return error.message ? `${name}: ${error.message}` : name; +} + +/** + * hls.js's account of the error it gave up on, e.g. "hls.js manifestLoadError: + * A network error (status 502) occurred while loading manifest". Never the + * URL: a segment's carries TIDAL's signed address. + */ +function describeHlsError(data: ErrorData): string { + const message = data.error?.message; + return `hls.js ${data.details}${message ? `: ${message}` : ""}`; +} + /** * Recover from a fatal hls.js error where hls.js can, else destroy it. * Returns true when it gave up and the source is gone. A playlist that never diff --git a/frontend/src/components/MiniPlayer.tsx b/frontend/src/components/MiniPlayer.tsx index b607f323..e1ff2a6c 100644 --- a/frontend/src/components/MiniPlayer.tsx +++ b/frontend/src/components/MiniPlayer.tsx @@ -1,3 +1,4 @@ +import { useEffect } from "react"; import RemoteControlIndicator from "./RemoteControlIndicator"; import { Minimize2 as ArrowsPointingInIcon, @@ -49,9 +50,17 @@ export default function MiniPlayer() { anchor, barHeight, panelToggle, + closePanel, } = useMiniPlayerMode(); const canResizeWindow = canSetMiniPlayer(); const lyricsToggle = miniPlayerMode ? panelToggle("lyrics") : null; + const errorToggle = miniPlayerMode ? panelToggle("error") : undefined; + // The details go with the failure: playing again, or another track, shuts + // them. + const errorPanelOpen = !!errorToggle?.open; + useEffect(() => { + if (errorPanelOpen && !displayError) closePanel("error"); + }, [errorPanelOpen, displayError, closePanel]); return (
@@ -74,6 +83,7 @@ export default function MiniPlayer() { title={displayTitle} artist={displayArtist} error={displayError} + errorPanel={errorToggle} onContextMenu={ displayCurrent && !isRemoteMode ? bindCtx(displayCurrent) diff --git a/frontend/src/components/player/MiniPlayerPanel.tsx b/frontend/src/components/player/MiniPlayerPanel.tsx index 2f9e10fd..6c5d7b4b 100644 --- a/frontend/src/components/player/MiniPlayerPanel.tsx +++ b/frontend/src/components/player/MiniPlayerPanel.tsx @@ -1,6 +1,7 @@ import { usePlayer, usePlayerTime, useRemotePlayback } from "../../context/Player"; import { lyricsDurationSeconds, useTrackLyrics } from "../../lib/useTrackLyrics"; import { DeviceConnectionStatus, DeviceList } from "../PlaybackDevicePopover"; +import { PlaybackErrorDetails } from "./PlaybackErrorPopover"; import PlayerLyricsLine from "../PlayerLyricsLine"; import { QueueList, useQueuePosition } from "../QueuePopover"; import { @@ -10,20 +11,22 @@ import { import { MINI_PANEL_ID, type MiniPanel } from "./useMiniPlayerMode"; const TITLES: Record = { + error: "Playback error", devices: "Play on", lyrics: "Lyrics", queue: "Queue", }; const LABELS: Record = { + error: "Playback error", devices: "Playback device", lyrics: "Lyrics", queue: "Play queue", }; /** - * The mini player's side panel: the queue, lyrics or device list the utility - * buttons used to open as popovers. Renders two items of the player grid, a + * The mini player's side panel: the queue, lyrics, device list or playback + * error details that open as popovers outside mini mode. Renders two items of the player grid, a * heading beside the utility row and the body under it. `enter` slides a * panel swapped in for another from the side its button sits on. */ @@ -60,6 +63,8 @@ export default function MiniPlayerPanel({ ) : panel === "lyrics" ? ( + ) : panel === "error" ? ( + ) : ( )} diff --git a/frontend/src/components/player/NowPlaying.tsx b/frontend/src/components/player/NowPlaying.tsx index fb2f96a0..97416a17 100644 --- a/frontend/src/components/player/NowPlaying.tsx +++ b/frontend/src/components/player/NowPlaying.tsx @@ -1,43 +1,85 @@ +import { useState } from "react"; import type { TrackListItem } from "@music-library/core"; import { trackCoverUrl } from "../../api"; import { displayText } from "../../lib/format"; import CoverArt from "../CoverArt"; +import PlaybackErrorPopover from "./PlaybackErrorPopover"; +import { usePopoverOpen, type MiniPanelToggle } from "./useMiniPlayerMode"; export default function NowPlaying({ track, title, artist, error, + errorPanel, onContextMenu, }: { track: TrackListItem | null; title: string; artist: string; - /** Why the track stopped playing; shown in place of the artist. */ + /** + * Why the track stopped playing; shown in place of the artist. Clicking it + * shows what happened. + */ error?: string | null; + /** In the mini player the details open in its panel instead of a popover. */ + errorPanel?: MiniPanelToggle; onContextMenu?: React.MouseEventHandler; }) { const coverSrc = track ? trackCoverUrl(track) : null; + // Held in state, as in DevicePickerButton: the popover reads them during + // render. + const [block, setBlock] = useState(null); + const [errorButton, setErrorButton] = useState(null); + const [detailsOpen, setDetailsOpen] = usePopoverOpen(!!errorPanel); + // A cleared failure closes its details, so the next one doesn't open them. + if (!error && detailsOpen) setDetailsOpen(false); + const open = errorPanel ? errorPanel.open : detailsOpen; + return ( -
- -
-
{title}
-
- {/* Always rendered, so screen readers announce a failure as it - appears rather than only when they reach the line. */} - {error} - {!error && artist} + <> +
+ +
+
{title}
+
+ {/* Always rendered, so screen readers announce a failure as it + appears rather than only when they reach the line. */} + + {error && ( + + )} + + {!error && artist} +
-
+ {/* Outside the block, so its context menu doesn't reach the popover. */} + setDetailsOpen(false)} + /> + ); } diff --git a/frontend/src/components/player/PlaybackErrorPopover.tsx b/frontend/src/components/player/PlaybackErrorPopover.tsx new file mode 100644 index 00000000..91c83c15 --- /dev/null +++ b/frontend/src/components/player/PlaybackErrorPopover.tsx @@ -0,0 +1,118 @@ +import { useRef, useState } from "react"; +import { createPortal } from "react-dom"; +import { + Check as CheckIcon, + Clipboard as ClipboardIcon, + X as XMarkIcon, +} from "lucide-react"; +import { playbackErrorLines, playbackErrorText } from "@music-library/core"; +import { usePlayer } from "../../context/Player"; +import { copyText } from "../../lib/clipboard"; +import { useCopiedFlag } from "../../lib/useCopiedFlag"; +import { useDismiss } from "../../lib/useDismiss"; +import { useTransitionMount } from "../../lib/useTransitionMount"; +import { Button } from "../Button"; + +/** The playback error line's popover: what happened, above the track. */ +export default function PlaybackErrorPopover({ + open, + anchor, + toggle, + onClose, +}: { + open: boolean; + /** The now-playing block; the popover opens above it. */ + anchor: HTMLElement | null; + /** The error line that opened it, which toggles it rather than dismissing. */ + toggle: HTMLElement | null; + onClose: () => void; +}) { + const ref = useRef(null); + + useDismiss(ref, { + onDismiss: onClose, + enabled: open, + capture: true, + ignore: (target) => !!toggle?.contains(target), + }); + + const { mounted, visible } = useTransitionMount(open, 180); + + if (!mounted || !anchor) return null; + + const rect = anchor.getBoundingClientRect(); + const width = Math.min(440, window.innerWidth - 24); + const bottom = Math.max(12, window.innerHeight - rect.top + 8); + const left = Math.max(12, Math.min(rect.left, window.innerWidth - width - 12)); + + return createPortal( +
event.stopPropagation()} + onMouseDown={(event) => event.stopPropagation()} + > +
+ Playback error + + +
+ +
, + document.body, + ); +} + +/** + * What happened to the current track, line by line, and a button that copies + * it for a bug report. Shared by the popover and the mini player's panel. + */ +export function PlaybackErrorDetails() { + const { playbackError, current } = usePlayer(); + // Keeps the last failure on screen while the popover or panel closes after + // it clears. + const [last, setLast] = useState(playbackError); + if (playbackError && playbackError !== last) setLast(playbackError); + const error = playbackError ?? last; + const { copied, flash } = useCopiedFlag(2000); + if (!error) return null; + + const lines = playbackErrorLines(error, current); + return ( +
+
+ {lines.map(({ label, value }) => ( +
+
{label}
+
{value}
+
+ ))} +
+
+ +
+
+ ); +} diff --git a/frontend/src/components/player/useMiniPlayerMode.ts b/frontend/src/components/player/useMiniPlayerMode.ts index d4e025bc..bec787f7 100644 --- a/frontend/src/components/player/useMiniPlayerMode.ts +++ b/frontend/src/components/player/useMiniPlayerMode.ts @@ -9,8 +9,11 @@ import { setMiniPlayerPanel, } from "../../lib/platform"; -/** What the mini player's side panel can show, in utility-row order. */ -export const MINI_PANELS = ["devices", "lyrics", "queue"] as const; +/** + * What the mini player's side panel can show, in the order their controls + * sit in the bar: the playback error line, then the utility row. + */ +export const MINI_PANELS = ["error", "devices", "lyrics", "queue"] as const; export type MiniPanel = (typeof MINI_PANELS)[number]; export const MINI_PANEL_ID = "mini-player-panel"; @@ -168,6 +171,14 @@ export function useMiniPlayerMode() { [requestPanel], ); + // Shuts `which` if it's the panel asked for, as when what it shows goes. + const closePanel = useCallback( + (which: MiniPanel) => { + if (wanted.current === which) requestPanel(null); + }, + [requestPanel], + ); + const panelToggle = (which: MiniPanel): MiniPanelToggle => ({ open: requested === which, controls: MINI_PANEL_ID, @@ -215,6 +226,7 @@ export function useMiniPlayerMode() { anchor, barHeight, panelToggle, + closePanel, }; } diff --git a/frontend/src/styles/player-popovers.css b/frontend/src/styles/player-popovers.css index d99dc7e0..41a045fd 100644 --- a/frontend/src/styles/player-popovers.css +++ b/frontend/src/styles/player-popovers.css @@ -246,6 +246,50 @@ z-index: 1; } +/* The playback error's details (PlaybackErrorPopover.tsx): above the + now-playing block, so it grows from the left. The body is shared with the + mini player's error panel. */ +.error-pop { + transform-origin: bottom left; +} +.error-pop-body { + padding: 12px 14px 14px; + display: flex; + flex-direction: column; + gap: 14px; +} +.error-pop-lines { + margin: 0; + display: grid; + grid-template-columns: auto minmax(0, 1fr); + gap: 6px 14px; + font-size: 12px; + line-height: 1.45; +} +.error-pop-line { + display: contents; +} +.error-pop-line dt { + color: var(--muted-foreground); +} +/* Selectable, though the desktop app turns selection off: a part is worth + copying on its own. */ +.error-pop-line dd { + margin: 0; + min-width: 0; + color: var(--foreground); + overflow-wrap: anywhere; + -webkit-user-select: text; + user-select: text; +} +.error-pop-line:first-child dd { + color: var(--destructive); +} +.error-pop-actions { + display: flex; + justify-content: flex-end; +} + .queue-pop[data-closed], .device-pop[data-closed] { opacity: 0; diff --git a/frontend/src/styles/player.css b/frontend/src/styles/player.css index 7fa12015..f3d58cdf 100644 --- a/frontend/src/styles/player.css +++ b/frontend/src/styles/player.css @@ -121,10 +121,32 @@ text-overflow: ellipsis; } /* A playback failure in place of the artist. Specific enough to beat the - mini player's own artist color. */ + mini player's own artist color. The line is a button that shows what + happened; it truncates itself, so the line lets its focus ring show. */ .np-text .np-artist.np-error { + overflow: visible; color: var(--destructive); } +.np-error-btn { + display: inline-block; + max-width: 100%; + padding: 0; + border: 0; + border-radius: var(--radius-sm); + background: none; + color: inherit; + font: inherit; + vertical-align: top; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + cursor: pointer; +} +.np-error-btn:hover, +.np-error-btn[aria-expanded="true"] { + text-decoration: underline; + text-underline-offset: 2px; +} .transport { display: flex; diff --git a/frontend/tests/html-audio-adapter.test.tsx b/frontend/tests/html-audio-adapter.test.tsx index 15f9e0c8..aac2e9d2 100644 --- a/frontend/tests/html-audio-adapter.test.tsx +++ b/frontend/tests/html-audio-adapter.test.tsx @@ -121,6 +121,32 @@ it("gives up on a playlist or segment the server refused", async () => { expect(error).toHaveBeenCalledOnce(); }); +it("says what the element or hls.js reported about the failure", async () => { + const { adapter, active, error } = setup(); + act(() => adapter.load("/api/tracks/one/stream")); + Object.defineProperty(active, "error", { + configurable: true, + value: { code: 4, message: "DEMUXER_ERROR_COULD_NOT_OPEN: FFmpegDemuxer: open context failed" }, + }); + active.dispatchEvent(new Event("error")); + expect(error).toHaveBeenCalledOnce(); + expect(adapter.errorDetail?.()).toBe( + "MEDIA_ERR_SRC_NOT_SUPPORTED: DEMUXER_ERROR_COULD_NOT_OPEN: FFmpegDemuxer: open context failed", + ); + + act(() => adapter.load("/api/tracks/tidal%3Atwo/stream")); + await vi.waitFor(() => expect(mock.instances).toHaveLength(1)); + mock.instances[0].fail({ + ...manifestFailed, + url: "https://cdn.example/signed?token=secret", + error: new Error("A network error (status 502) occurred while loading manifest"), + }); + expect(error).toHaveBeenCalledTimes(2); + expect(adapter.errorDetail?.()).toBe( + "hls.js manifestLoadError: A network error (status 502) occurred while loading manifest", + ); +}); + // jsdom never plays an element; mark it playing for a crossfade to start. function playing(audio: HTMLAudioElement) { Object.defineProperty(audio, "paused", { configurable: true, get: () => false }); diff --git a/frontend/tests/player-display.test.tsx b/frontend/tests/player-display.test.tsx index e9f1a081..f3e8c149 100644 --- a/frontend/tests/player-display.test.tsx +++ b/frontend/tests/player-display.test.tsx @@ -1,5 +1,5 @@ -import { render, renderHook, screen } from "@testing-library/react"; -import { beforeEach, expect, it, vi } from "vitest"; +import { cleanup, fireEvent, render, renderHook, screen, waitFor, within } from "@testing-library/react"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; import type { PlaybackDevice, PlayerState, TrackListItem } from "@music-library/core"; import NowPlaying from "../src/components/player/NowPlaying"; import { usePlayerDisplay } from "../src/components/player/usePlayerDisplay"; @@ -44,6 +44,7 @@ beforeEach(() => { }; mock.remote = { targetDevice: desktop, commandPending: false }; }); +afterEach(cleanup); it("shows the provider's displayed state while controlling another device", () => { const { result } = renderHook(() => usePlayerDisplay()); @@ -75,15 +76,22 @@ it("keeps transport enabled while a remote command is pending", () => { expect(result.current.transportDisabled).toBe(false); }); +const details = { + at: Date.UTC(2026, 9, 29, 12, 0, 0), + source: "/api/tracks/t1/stream", + player: "MEDIA_ERR_SRC_NOT_SUPPORTED: DEMUXER_ERROR_COULD_NOT_OPEN", + server: "HTTP 200 · audio/flac", +}; + it("shows the current track's playback error in place of its artist", () => { const message = "TIDAL refused to stream this track: Not available in your region"; mock.remote = { targetDevice: null, commandPending: false }; - mock.player = { ...mock.player, isPlaying: false, playbackError: { trackId: "t1", message } }; + mock.player = { ...mock.player, isPlaying: false, playbackError: { trackId: "t1", message, details } }; const { result, rerender } = renderHook(() => usePlayerDisplay()); expect(result.current.displayError).toBe(message); expect(result.current.displayArtist).toBe("Artist · Album"); - mock.player = { ...mock.player, playbackError: { trackId: "t0", message } }; + mock.player = { ...mock.player, playbackError: { trackId: "t0", message, details } }; rerender(); expect(result.current.displayError).toBeNull(); }); @@ -101,7 +109,55 @@ it("renders the error as the artist line and announces it", () => { , ); expect(line.textContent).toBe(message); - expect(line.getAttribute("title")).toBe(message); + expect(screen.getByRole("button", { name: message }).getAttribute("title")).toBe(message); expect(line.classList.contains("np-error")).toBe(true); expect(screen.getByRole("status").textContent).toBe(message); }); + +it("shows what happened when the error is clicked, and copies it", async () => { + const message = "Couldn't play this track."; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + const writeText = vi.fn(async () => {}); + Object.defineProperty(navigator, "clipboard", { configurable: true, value: { writeText } }); + render(); + + const line = screen.getByRole("button", { name: message }); + expect(line.getAttribute("aria-expanded")).toBe("false"); + fireEvent.click(line); + expect(line.getAttribute("aria-expanded")).toBe("true"); + const dialog = await screen.findByRole("dialog", { name: "Playback error" }); + expect(dialog.textContent).toContain("DEMUXER_ERROR_COULD_NOT_OPEN"); + expect(dialog.textContent).toContain("HTTP 200 · audio/flac"); + + fireEvent.click(within(dialog).getByRole("button", { name: "Copy details" })); + await waitFor(() => expect(writeText).toHaveBeenCalledOnce()); + expect(writeText.mock.calls[0]).toEqual([ + [ + "Error: Couldn't play this track.", + "Track: Song — Artist", + "Track ID: t1", + "Source: /api/tracks/t1/stream", + "Player: MEDIA_ERR_SRC_NOT_SUPPORTED: DEMUXER_ERROR_COULD_NOT_OPEN", + "Server: HTTP 200 · audio/flac", + "Time: 2026-10-29T12:00:00.000Z", + ].join("\n"), + ]); + expect(await within(dialog).findByRole("button", { name: "Copied" })).toBeTruthy(); +}); + +it("opens the mini player's panel instead of a popover", () => { + const message = "Couldn't play this track."; + const onToggle = vi.fn(); + render( + , + ); + fireEvent.click(screen.getByRole("button", { name: message })); + expect(onToggle).toHaveBeenCalledOnce(); + expect(screen.queryByRole("dialog")).toBeNull(); +}); diff --git a/frontend/tests/ui/overlays.ui.ts b/frontend/tests/ui/overlays.ui.ts index 0527fcf1..6d7d77b5 100644 --- a/frontend/tests/ui/overlays.ui.ts +++ b/frontend/tests/ui/overlays.ui.ts @@ -137,6 +137,29 @@ test("settings playback section", async ({ page }, testInfo) => { await checkUI(page, testInfo); }); +test("playback error details", async ({ page }, testInfo) => { + // A library file the server no longer has. + await page.route(`**/api/tracks/${menuTrack.id}/stream`, (route) => + route.fulfill({ status: 410, contentType: "text/plain", body: "file missing on disk" }), + ); + await page.locator(".track-title").and(page.getByTitle(menuTrack.title, { exact: true })).dispatchEvent("click"); + await page.locator(".player-bar").getByRole("button", { name: "Couldn't play this track." }).dispatchEvent("click"); + const dialog = page.getByRole("dialog", { name: "Playback error" }); + await ready(page, dialog.getByText("HTTP 410 · file missing on disk")); + await checkUI(page, testInfo, { + exceptions: [ + { + // axe compares the stack under each line of text, the page beneath + // the popover included. The track's two lines sit over the sidebar + // and the list's rows, so it gives up. + selector: ".error-pop-line:nth-child(2) dd", + rules: ["text-contrast"], + reason: "A wrapped line of popover text over the sidebar and the track list, which axe can't resolve. The popover's other lines, in the same style, are measured.", + }, + ], + }); +}); + test("queue popover", async ({ page }, testInfo) => { await page.locator(".player-bar").getByRole("button", { name: "Queue", exact: true }).dispatchEvent("click"); await ready(page, page.locator(".queue-pop").getByText(loadedTrack.title).first()); From 3ed54e19c7423cd41f59e0fffde7916f639a713c Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:39:23 +0000 Subject: [PATCH 03/11] Playback error details: review fixes - What the audio player said is kept to one line of at most 300 characters, with any address replaced (hls.js quotes a failed key's URI, which may be signed). - The details keep the failed track while they close over the next one, so the Track line no longer drops out mid-fade. - The mini player's error panel shuts from NowPlaying, beside the popover's own close, through a close() on the panel toggle. Tests cover both closing when the failure clears, not reopening for the next one, and Escape / outside-click dismissal. - The lint:ui exception names the Track line by data-label rather than its position. Co-Authored-By: Claude Opus 5.5 --- core/src/player/use-player-core.ts | 15 +++- core/tests/use-player-core.test.tsx | 27 +++++++ frontend/src/components/MiniPlayer.tsx | 11 +-- frontend/src/components/player/NowPlaying.tsx | 10 ++- .../player/PlaybackErrorPopover.tsx | 28 +++++--- .../components/player/useMiniPlayerMode.ts | 14 ++-- frontend/tests/player-display.test.tsx | 72 ++++++++++++++++++- frontend/tests/ui/overlays.ui.ts | 2 +- 8 files changed, 146 insertions(+), 33 deletions(-) diff --git a/core/src/player/use-player-core.ts b/core/src/player/use-player-core.ts index 09016eca..8478c385 100644 --- a/core/src/player/use-player-core.ts +++ b/core/src/player/use-player-core.ts @@ -129,6 +129,19 @@ async function probeStream(url: string): Promise { } } +/** + * What the audio player said, fit for a bug report: on one line, capped, and + * without addresses (hls.js quotes a failed key's URI, which may be signed). + */ +function playerDetail(text: string | null | undefined): string | null { + const plain = text + ?.replace(/[a-z][a-z0-9+.-]*:\/\/[^\s"'<>]+/gi, "
") + .replace(/\s+/g, " ") + .trim(); + if (!plain) return null; + return plain.length > 300 ? `${plain.slice(0, 299)}…` : plain; +} + /** A failed source as the details show it: a server address without its origin. */ function failedSource(uri: string): string { if (!/^https?:\/\//i.test(uri)) return uri; @@ -995,7 +1008,7 @@ export function usePlayerCore({ const details: PlaybackErrorDetails = { at: Date.now(), source: failedSource(uri), - player: adapter.errorDetail?.() ?? null, + player: playerDetail(adapter.errorDetail?.()), server: streamed ? undefined : null, }; const report = (message: string, server = details.server) => { diff --git a/core/tests/use-player-core.test.tsx b/core/tests/use-player-core.test.tsx index d3a0cf1c..c3d83065 100644 --- a/core/tests/use-player-core.test.tsx +++ b/core/tests/use-player-core.test.tsx @@ -887,6 +887,33 @@ describe("usePlayerCore playback errors", () => { ); }); + it("keeps no address from what the audio player said, and caps it", async () => { + const fake = createFakeAdapter(); + let said = 'hls.js keyLoadError: Invalid key URI: "https://cdn.example/key?token=secret"'; + fake.adapter.errorDetail = () => said; + const { result } = renderHook(() => + usePlayerCore({ + adapter: fake.adapter, + storage: createMemoryStorage(), + interpolateProgress: false, + }), + ); + act(() => result.current.controls.play(t("a"))); + + act(() => fake.emit("error")); + expect(result.current.state.playbackError?.details.player).toBe( + 'hls.js keyLoadError: Invalid key URI: "
"', + ); + + said = `MEDIA_ERR_DECODE:\n${"x".repeat(400)}`; + act(() => result.current.controls.toggle()); + act(() => fake.emit("error")); + const player = result.current.state.playbackError?.details.player ?? ""; + expect(player).toHaveLength(300); + expect(player.startsWith("MEDIA_ERR_DECODE: xxx")).toBe(true); + expect(player.endsWith("…")).toBe(true); + }); + it("shows a server address without its origin", async () => { const { result, emit } = await setup({ resolveTrackUri: (id) => `https://music.example/api/tracks/${id}/stream`, diff --git a/frontend/src/components/MiniPlayer.tsx b/frontend/src/components/MiniPlayer.tsx index e1ff2a6c..39d53b70 100644 --- a/frontend/src/components/MiniPlayer.tsx +++ b/frontend/src/components/MiniPlayer.tsx @@ -1,4 +1,3 @@ -import { useEffect } from "react"; import RemoteControlIndicator from "./RemoteControlIndicator"; import { Minimize2 as ArrowsPointingInIcon, @@ -50,17 +49,9 @@ export default function MiniPlayer() { anchor, barHeight, panelToggle, - closePanel, } = useMiniPlayerMode(); const canResizeWindow = canSetMiniPlayer(); const lyricsToggle = miniPlayerMode ? panelToggle("lyrics") : null; - const errorToggle = miniPlayerMode ? panelToggle("error") : undefined; - // The details go with the failure: playing again, or another track, shuts - // them. - const errorPanelOpen = !!errorToggle?.open; - useEffect(() => { - if (errorPanelOpen && !displayError) closePanel("error"); - }, [errorPanelOpen, displayError, closePanel]); return (
@@ -83,7 +74,7 @@ export default function MiniPlayer() { title={displayTitle} artist={displayArtist} error={displayError} - errorPanel={errorToggle} + errorPanel={miniPlayerMode ? panelToggle("error") : undefined} onContextMenu={ displayCurrent && !isRemoteMode ? bindCtx(displayCurrent) diff --git a/frontend/src/components/player/NowPlaying.tsx b/frontend/src/components/player/NowPlaying.tsx index 97416a17..a91a3095 100644 --- a/frontend/src/components/player/NowPlaying.tsx +++ b/frontend/src/components/player/NowPlaying.tsx @@ -1,4 +1,4 @@ -import { useState } from "react"; +import { useEffect, useState } from "react"; import type { TrackListItem } from "@music-library/core"; import { trackCoverUrl } from "../../api"; import { displayText } from "../../lib/format"; @@ -32,8 +32,14 @@ export default function NowPlaying({ const [block, setBlock] = useState(null); const [errorButton, setErrorButton] = useState(null); const [detailsOpen, setDetailsOpen] = usePopoverOpen(!!errorPanel); - // A cleared failure closes its details, so the next one doesn't open them. + // The details go with the failure: playing again, or another track, shuts + // them, so the next failure doesn't open them by itself. if (!error && detailsOpen) setDetailsOpen(false); + const panelShowsGoneError = !error && !!errorPanel?.open; + const closePanel = errorPanel?.close; + useEffect(() => { + if (panelShowsGoneError) closePanel?.(); + }, [panelShowsGoneError, closePanel]); const open = errorPanel ? errorPanel.open : detailsOpen; return ( diff --git a/frontend/src/components/player/PlaybackErrorPopover.tsx b/frontend/src/components/player/PlaybackErrorPopover.tsx index 91c83c15..4e61e9ad 100644 --- a/frontend/src/components/player/PlaybackErrorPopover.tsx +++ b/frontend/src/components/player/PlaybackErrorPopover.tsx @@ -5,7 +5,12 @@ import { Clipboard as ClipboardIcon, X as XMarkIcon, } from "lucide-react"; -import { playbackErrorLines, playbackErrorText } from "@music-library/core"; +import { + playbackErrorLines, + playbackErrorText, + type PlaybackError, + type TrackListItem, +} from "@music-library/core"; import { usePlayer } from "../../context/Player"; import { copyText } from "../../lib/clipboard"; import { useCopiedFlag } from "../../lib/useCopiedFlag"; @@ -81,20 +86,25 @@ export default function PlaybackErrorPopover({ */ export function PlaybackErrorDetails() { const { playbackError, current } = usePlayer(); - // Keeps the last failure on screen while the popover or panel closes after - // it clears. - const [last, setLast] = useState(playbackError); - if (playbackError && playbackError !== last) setLast(playbackError); - const error = playbackError ?? last; + // Keeps the last failure, and the track it was on, on screen while the + // popover or panel closes after it clears (another track may be loaded). + const [last, setLast] = useState<{ + error: PlaybackError; + track: TrackListItem | null; + } | null>(null); + if (playbackError && playbackError !== last?.error) { + setLast({ error: playbackError, track: current }); + } + const shown = playbackError ? { error: playbackError, track: current } : last; const { copied, flash } = useCopiedFlag(2000); - if (!error) return null; + if (!shown) return null; - const lines = playbackErrorLines(error, current); + const lines = playbackErrorLines(shown.error, shown.track); return (
{lines.map(({ label, value }) => ( -
+
{label}
{value}
diff --git a/frontend/src/components/player/useMiniPlayerMode.ts b/frontend/src/components/player/useMiniPlayerMode.ts index bec787f7..84f081b6 100644 --- a/frontend/src/components/player/useMiniPlayerMode.ts +++ b/frontend/src/components/player/useMiniPlayerMode.ts @@ -23,6 +23,8 @@ export interface MiniPanelToggle { open: boolean; controls: string; onToggle: () => void; + /** Shuts the panel if it's this one, as when what it shows goes. */ + close: () => void; } /** @@ -171,18 +173,13 @@ export function useMiniPlayerMode() { [requestPanel], ); - // Shuts `which` if it's the panel asked for, as when what it shows goes. - const closePanel = useCallback( - (which: MiniPanel) => { - if (wanted.current === which) requestPanel(null); - }, - [requestPanel], - ); - const panelToggle = (which: MiniPanel): MiniPanelToggle => ({ open: requested === which, controls: MINI_PANEL_ID, onToggle: () => togglePanel(which), + close: () => { + if (wanted.current === which) requestPanel(null); + }, }); // Escape closes the panel, without the morph: a keyboard dismissal should @@ -226,7 +223,6 @@ export function useMiniPlayerMode() { anchor, barHeight, panelToggle, - closePanel, }; } diff --git a/frontend/tests/player-display.test.tsx b/frontend/tests/player-display.test.tsx index f3e8c149..cc8875c3 100644 --- a/frontend/tests/player-display.test.tsx +++ b/frontend/tests/player-display.test.tsx @@ -2,6 +2,7 @@ import { cleanup, fireEvent, render, renderHook, screen, waitFor, within } from import { afterEach, beforeEach, expect, it, vi } from "vitest"; import type { PlaybackDevice, PlayerState, TrackListItem } from "@music-library/core"; import NowPlaying from "../src/components/player/NowPlaying"; +import { PlaybackErrorDetails } from "../src/components/player/PlaybackErrorPopover"; import { usePlayerDisplay } from "../src/components/player/usePlayerDisplay"; const mock = vi.hoisted(() => ({ @@ -145,6 +146,57 @@ it("shows what happened when the error is clicked, and copies it", async () => { expect(await within(dialog).findByRole("button", { name: "Copied" })).toBeTruthy(); }); +it("closes the details on Escape or a click outside", async () => { + const message = "Couldn't play this track."; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + render(); + const line = screen.getByRole("button", { name: message }); + + fireEvent.click(line); + await screen.findByRole("dialog", { name: "Playback error" }); + fireEvent.keyDown(window, { key: "Escape" }); + expect(line.getAttribute("aria-expanded")).toBe("false"); + + fireEvent.click(line); + expect(line.getAttribute("aria-expanded")).toBe("true"); + fireEvent.mouseDown(document.body); + expect(line.getAttribute("aria-expanded")).toBe("false"); +}); + +it("shuts the details when the failure clears, and doesn't reopen them for the next", async () => { + const message = "Couldn't play this track."; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + const { rerender } = render( + , + ); + fireEvent.click(screen.getByRole("button", { name: message })); + await screen.findByRole("dialog", { name: "Playback error" }); + + // Played again: the failure is gone, and so are its details. + mock.player = { ...mock.player, playbackError: null }; + rerender(); + await waitFor(() => expect(screen.queryByRole("dialog")).toBeNull()); + + mock.player = { ...mock.player, playbackError: { trackId: "t1", message, details: { ...details } } }; + rerender(); + expect(screen.getByRole("button", { name: message }).getAttribute("aria-expanded")).toBe("false"); + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(screen.queryByRole("dialog")).toBeNull(); +}); + +it("keeps the failed track in the details while they close over the next one", () => { + const message = "Couldn't play this track."; + const next: TrackListItem = { ...track, id: "t2", title: "Next song" }; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + const { container, rerender } = render(); + expect(container.textContent).toContain("Song — Artist"); + + mock.player = { ...mock.player, current: next, playbackError: null }; + rerender(); + expect(container.textContent).toContain("Song — Artist"); + expect(container.textContent).toContain(message); +}); + it("opens the mini player's panel instead of a popover", () => { const message = "Couldn't play this track."; const onToggle = vi.fn(); @@ -154,10 +206,28 @@ it("opens the mini player's panel instead of a popover", () => { title="Song" artist="Artist · Album" error={message} - errorPanel={{ open: false, controls: "mini-player-panel", onToggle }} + errorPanel={{ open: false, controls: "mini-player-panel", onToggle, close: vi.fn() }} />, ); fireEvent.click(screen.getByRole("button", { name: message })); expect(onToggle).toHaveBeenCalledOnce(); expect(screen.queryByRole("dialog")).toBeNull(); }); + +it("shuts the mini player's error panel when the failure clears", () => { + const message = "Couldn't play this track."; + const close = vi.fn(); + const panel = (open: boolean) => ({ open, controls: "mini-player-panel", onToggle: vi.fn(), close }); + const { rerender } = render( + , + ); + expect(close).not.toHaveBeenCalled(); + + // Another panel is up: nothing to shut. + rerender(); + expect(close).not.toHaveBeenCalled(); + + rerender(); + rerender(); + expect(close).toHaveBeenCalled(); +}); diff --git a/frontend/tests/ui/overlays.ui.ts b/frontend/tests/ui/overlays.ui.ts index 6d7d77b5..f7f56923 100644 --- a/frontend/tests/ui/overlays.ui.ts +++ b/frontend/tests/ui/overlays.ui.ts @@ -152,7 +152,7 @@ test("playback error details", async ({ page }, testInfo) => { // axe compares the stack under each line of text, the page beneath // the popover included. The track's two lines sit over the sidebar // and the list's rows, so it gives up. - selector: ".error-pop-line:nth-child(2) dd", + selector: '.error-pop-line[data-label="Track"] dd', rules: ["text-contrast"], reason: "A wrapped line of popover text over the sidebar and the track list, which axe can't resolve. The popover's other lines, in the same style, are measured.", }, From eeb027e76f4d9e256a28477ce11a0f8be6a65db6 Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 14:53:02 +0000 Subject: [PATCH 04/11] Playback error details: second review fixes - The server's answer gets the same treatment as the player's: one line, capped, addresses replaced (a proxy's error page may name its upstream). - A library file is probed for its first byte (Range: bytes=0-0), so a stream that works now answers 206 without starting a whole download. A TIDAL stream is still asked exactly as the player asks it, since the backend passes the request on to TIDAL. - A closing popover keeps the failure it opened for, even if the next track fails during the fade. - Closing the popover with its button sends focus back to the error line rather than dropping it. Co-Authored-By: Claude Opus 5.5 --- core/src/player/use-player-core.ts | 21 ++++++++------ core/tests/use-player-core.test.tsx | 29 +++++++++++++++++++ .../player/PlaybackErrorPopover.tsx | 22 +++++++++----- frontend/tests/player-display.test.tsx | 26 +++++++++++++++++ 4 files changed, 82 insertions(+), 16 deletions(-) diff --git a/core/src/player/use-player-core.ts b/core/src/player/use-player-core.ts index 8478c385..2cc8bd5c 100644 --- a/core/src/player/use-player-core.ts +++ b/core/src/player/use-player-core.ts @@ -108,32 +108,35 @@ interface StreamProbe { * failure, another error or a reverse proxy's HTML error page, gives no * reason; it is only described. */ -async function probeStream(url: string): Promise { +async function probeStream(url: string, { firstByte }: { firstByte: boolean }): Promise { try { - const response = await rawFetch(url); + // A library file is served with ranges, so one byte answers as well as + // the whole file. A TIDAL stream is asked exactly as the player asks it. + const response = await rawFetch(url, firstByte ? { headers: { Range: "bytes=0-0" } } : {}); // Nothing to explain; don't download the stream. void response.body?.cancel().catch(() => {}); const type = response.headers.get("content-type"); return { reason: null, answer: `HTTP ${response.status}${type ? ` · ${type}` : ""}` }; } catch (error) { if (!(error instanceof ApiError)) { - return { reason: null, answer: `No answer: ${String(error)}` }; + return { reason: null, answer: detailLine(`No answer: ${String(error)}`) ?? "No answer" }; } const text = error.message; const readable = text.length <= 300 && !text.startsWith("<") && !text.includes("\n") ? text : ""; return { reason: readable && (error.status === 502 || error.status === 503) ? readable : null, - answer: `HTTP ${error.status}${readable ? ` · ${readable}` : ""}`, + answer: detailLine(`HTTP ${error.status}${readable ? ` · ${readable}` : ""}`) ?? "", }; } } /** - * What the audio player said, fit for a bug report: on one line, capped, and - * without addresses (hls.js quotes a failed key's URI, which may be signed). + * A failure's account fit for a bug report: on one line, capped, and without + * addresses (hls.js quotes a failed key's URI, which may be signed, and a + * proxy's error page may name its upstream). */ -function playerDetail(text: string | null | undefined): string | null { +function detailLine(text: string | null | undefined): string | null { const plain = text ?.replace(/[a-z][a-z0-9+.-]*:\/\/[^\s"'<>]+/gi, "
") .replace(/\s+/g, " ") @@ -1008,7 +1011,7 @@ export function usePlayerCore({ const details: PlaybackErrorDetails = { at: Date.now(), source: failedSource(uri), - player: playerDetail(adapter.errorDetail?.()), + player: detailLine(adapter.errorDetail?.()), server: streamed ? undefined : null, }; const report = (message: string, server = details.server) => { @@ -1019,7 +1022,7 @@ export function usePlayerCore({ // Say so right away; the server's reason, if any, replaces it. report(PLAYBACK_ERROR_FALLBACK); if (streamed) { - void probeStream(url).then(({ reason, answer }) => + void probeStream(url, { firstByte: !isTidalTrack(track) }).then(({ reason, answer }) => // Only TIDAL streams fail for a reason worth showing on the bar. report(isTidalTrack(track) && reason ? reason : PLAYBACK_ERROR_FALLBACK, answer), ); diff --git a/core/tests/use-player-core.test.tsx b/core/tests/use-player-core.test.tsx index c3d83065..196e792b 100644 --- a/core/tests/use-player-core.test.tsx +++ b/core/tests/use-player-core.test.tsx @@ -854,6 +854,35 @@ describe("usePlayerCore playback errors", () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); + it("asks for a library file's first byte, and a TIDAL stream as the player does", async () => { + const fetchMock = vi.fn(async (_url: string, _init?: RequestInit) => new Response("x", { status: 206 })); + vi.stubGlobal("fetch", fetchMock); + const { result, emit } = await setup(); + const range = (call: number) => + (fetchMock.mock.calls[call][1]?.headers as Record | undefined)?.Range; + + act(() => result.current.controls.play(t("a"), [t("a"), tidal("1")])); + act(() => emit("error")); + await settle(); + expect(range(0)).toBe("bytes=0-0"); + + act(() => result.current.controls.next()); + act(() => emit("error")); + await settle(); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(range(1)).toBeUndefined(); + }); + + it("keeps no address from the server's answer", async () => { + vi.stubGlobal("fetch", vi.fn(async () => new Response("upstream http://10.0.0.5:8080/x unreachable", { status: 502 }))); + const { result, emit } = await setup(); + act(() => result.current.controls.play(t("a"))); + + act(() => emit("error")); + await settle(); + expect(result.current.state.playbackError?.details.server).toBe("HTTP 502 · upstream
unreachable"); + }); + it("does not ask the server about a downloaded TIDAL track", async () => { const fetchMock = vi.fn(async () => new Response(refusal, { status: 502 })); vi.stubGlobal("fetch", fetchMock); diff --git a/frontend/src/components/player/PlaybackErrorPopover.tsx b/frontend/src/components/player/PlaybackErrorPopover.tsx index 4e61e9ad..5d855667 100644 --- a/frontend/src/components/player/PlaybackErrorPopover.tsx +++ b/frontend/src/components/player/PlaybackErrorPopover.tsx @@ -69,12 +69,16 @@ export default function PlaybackErrorPopover({ type="button" className="iconbtn device-pop-close" aria-label="Close playback error" - onClick={onClose} + onClick={() => { + onClose(); + // This button is going; focus goes back to the line it came from. + toggle?.focus(); + }} >
- +
, document.body, ); @@ -84,7 +88,12 @@ export default function PlaybackErrorPopover({ * What happened to the current track, line by line, and a button that copies * it for a bug report. Shared by the popover and the mini player's panel. */ -export function PlaybackErrorDetails() { +export function PlaybackErrorDetails({ + frozen = false, +}: { + /** Closing: keep what was shown, even if another failure comes in. */ + frozen?: boolean; +}) { const { playbackError, current } = usePlayer(); // Keeps the last failure, and the track it was on, on screen while the // popover or panel closes after it clears (another track may be loaded). @@ -92,10 +101,9 @@ export function PlaybackErrorDetails() { error: PlaybackError; track: TrackListItem | null; } | null>(null); - if (playbackError && playbackError !== last?.error) { - setLast({ error: playbackError, track: current }); - } - const shown = playbackError ? { error: playbackError, track: current } : last; + const live = frozen ? null : playbackError; + if (live && live !== last?.error) setLast({ error: live, track: current }); + const shown = live ? { error: live, track: current } : last; const { copied, flash } = useCopiedFlag(2000); if (!shown) return null; diff --git a/frontend/tests/player-display.test.tsx b/frontend/tests/player-display.test.tsx index cc8875c3..81f63a83 100644 --- a/frontend/tests/player-display.test.tsx +++ b/frontend/tests/player-display.test.tsx @@ -197,6 +197,32 @@ it("keeps the failed track in the details while they close over the next one", ( expect(container.textContent).toContain(message); }); +it("keeps the closing details on the failure they opened for", () => { + const first = { trackId: "t1", message: "Couldn't play this track.", details }; + const second = { trackId: "t2", message: "Couldn't play this track.", details: { ...details, server: "HTTP 404 · not found" } }; + mock.player = { ...mock.player, current: track, playbackError: first }; + const { container, rerender } = render(); + + // The next track failed while the popover fades out. + mock.player = { ...mock.player, current: { ...track, id: "t2" }, playbackError: second }; + rerender(); + expect(container.textContent).toContain("HTTP 200 · audio/flac"); + expect(container.textContent).not.toContain("HTTP 404"); +}); + +it("sends focus back to the error line when closed with its button", async () => { + const message = "Couldn't play this track."; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + render(); + const line = screen.getByRole("button", { name: message }); + fireEvent.click(line); + const dialog = await screen.findByRole("dialog", { name: "Playback error" }); + + fireEvent.click(within(dialog).getByRole("button", { name: "Close playback error" })); + expect(line.getAttribute("aria-expanded")).toBe("false"); + expect(document.activeElement).toBe(line); +}); + it("opens the mini player's panel instead of a popover", () => { const message = "Couldn't play this track."; const onToggle = vi.fn(); From bf9e373bdd22bd7f59c1969c424af4f6408686a6 Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:03:02 +0000 Subject: [PATCH 05/11] Playback error details: third review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Escape sends focus back to the error line too, when focus was inside the popover, as its close button already did. An outside close leaves focus where it went. - The probe's success answer goes through the same one-line, capped, address-free cleanup as its error answers. - A downloaded copy's source names only its file ("Downloaded copy · x.flac"), not the device's container path. - Tests for the panel toggle's close() (only its own panel, idempotent) and the mini player's error panel. Co-Authored-By: Claude Opus 5.5 --- core/src/player/use-player-core.ts | 11 +++++- core/tests/use-player-core.test.tsx | 13 +++++-- .../player/PlaybackErrorPopover.tsx | 15 +++++--- frontend/tests/mini-player-mode.test.tsx | 37 +++++++++++++++++++ frontend/tests/player-display.test.tsx | 37 ++++++++++++++++++- 5 files changed, 99 insertions(+), 14 deletions(-) create mode 100644 frontend/tests/mini-player-mode.test.tsx diff --git a/core/src/player/use-player-core.ts b/core/src/player/use-player-core.ts index 2cc8bd5c..f9ab8895 100644 --- a/core/src/player/use-player-core.ts +++ b/core/src/player/use-player-core.ts @@ -116,7 +116,10 @@ async function probeStream(url: string, { firstByte }: { firstByte: boolean }): // Nothing to explain; don't download the stream. void response.body?.cancel().catch(() => {}); const type = response.headers.get("content-type"); - return { reason: null, answer: `HTTP ${response.status}${type ? ` · ${type}` : ""}` }; + return { + reason: null, + answer: detailLine(`HTTP ${response.status}${type ? ` · ${type}` : ""}`) ?? "", + }; } catch (error) { if (!(error instanceof ApiError)) { return { reason: null, answer: detailLine(`No answer: ${String(error)}`) ?? "No answer" }; @@ -145,8 +148,12 @@ function detailLine(text: string | null | undefined): string | null { return plain.length > 300 ? `${plain.slice(0, 299)}…` : plain; } -/** A failed source as the details show it: a server address without its origin. */ +/** + * A failed source as the details show it: a server address without its + * origin, or a downloaded copy by its file name (its folder is the device's). + */ function failedSource(uri: string): string { + if (/^file:/i.test(uri)) return `Downloaded copy · ${uri.slice(uri.lastIndexOf("/") + 1)}`; if (!/^https?:\/\//i.test(uri)) return uri; try { const { pathname, search } = new URL(uri); diff --git a/core/tests/use-player-core.test.tsx b/core/tests/use-player-core.test.tsx index 196e792b..d146c679 100644 --- a/core/tests/use-player-core.test.tsx +++ b/core/tests/use-player-core.test.tsx @@ -873,14 +873,19 @@ describe("usePlayerCore playback errors", () => { expect(range(1)).toBeUndefined(); }); - it("keeps no address from the server's answer", async () => { - vi.stubGlobal("fetch", vi.fn(async () => new Response("upstream http://10.0.0.5:8080/x unreachable", { status: 502 }))); + it.each([ + ["an error", () => new Response("upstream http://10.0.0.5:8080/x unreachable", { status: 502 }), "HTTP 502 · upstream
unreachable"], + ["a success", () => new Response("x", { status: 200, headers: { "Content-Type": `audio/flac; see=https://proxy.example/why ${"x".repeat(400)}` } }), "HTTP 200 · audio/flac; see=
x"], + ])("keeps no address from the server's answer to %s, and caps it", async (_case, respond, start) => { + vi.stubGlobal("fetch", vi.fn(async () => respond())); const { result, emit } = await setup(); act(() => result.current.controls.play(t("a"))); act(() => emit("error")); await settle(); - expect(result.current.state.playbackError?.details.server).toBe("HTTP 502 · upstream
unreachable"); + const server = result.current.state.playbackError?.details.server ?? ""; + expect(server.startsWith(start)).toBe(true); + expect(server.length).toBeLessThanOrEqual(300); }); it("does not ask the server about a downloaded TIDAL track", async () => { @@ -893,7 +898,7 @@ describe("usePlayerCore playback errors", () => { await settle(); expect(result.current.state.playbackError).toMatchObject({ message: "Couldn't play this track.", - details: { source: "file:///offline.flac", server: null }, + details: { source: "Downloaded copy · offline.flac", server: null }, }); expect(fetchMock).not.toHaveBeenCalled(); }); diff --git a/frontend/src/components/player/PlaybackErrorPopover.tsx b/frontend/src/components/player/PlaybackErrorPopover.tsx index 5d855667..2ce26dc8 100644 --- a/frontend/src/components/player/PlaybackErrorPopover.tsx +++ b/frontend/src/components/player/PlaybackErrorPopover.tsx @@ -34,8 +34,15 @@ export default function PlaybackErrorPopover({ }) { const ref = useRef(null); + // Closing from inside (its button, Escape) sends focus back to the line + // it came from, rather than dropping it with the popover. + const close = () => { + if (ref.current?.contains(document.activeElement)) toggle?.focus(); + onClose(); + }; + useDismiss(ref, { - onDismiss: onClose, + onDismiss: close, enabled: open, capture: true, ignore: (target) => !!toggle?.contains(target), @@ -69,11 +76,7 @@ export default function PlaybackErrorPopover({ type="button" className="iconbtn device-pop-close" aria-label="Close playback error" - onClick={() => { - onClose(); - // This button is going; focus goes back to the line it came from. - toggle?.focus(); - }} + onClick={close} > diff --git a/frontend/tests/mini-player-mode.test.tsx b/frontend/tests/mini-player-mode.test.tsx new file mode 100644 index 00000000..fefadbfd --- /dev/null +++ b/frontend/tests/mini-player-mode.test.tsx @@ -0,0 +1,37 @@ +import { act, cleanup, renderHook, waitFor } from "@testing-library/react"; +import { afterEach, expect, it, vi } from "vitest"; +import { useMiniPlayerMode } from "../src/components/player/useMiniPlayerMode"; + +const platform = vi.hoisted(() => ({ + canSetMiniPlayer: () => true, + getMiniPlayerPanelAnchor: vi.fn(async () => "top" as const), + setMiniPlayerMode: vi.fn(async () => ({ ok: true })), + setMiniPlayerPanel: vi.fn(async (open: boolean) => ({ ok: true, open, anchor: "top" as const })), +})); +vi.mock("../src/lib/platform", () => platform); +vi.mock("../src/context/LyricsPanel", () => ({ + useLyricsPanel: () => ({ open: false, setOpen: () => {} }), +})); + +afterEach(cleanup); + +it("closes a panel through its own toggle only", async () => { + const { result } = renderHook(() => useMiniPlayerMode()); + act(() => result.current.panelToggle("error").onToggle()); + await waitFor(() => expect(result.current.panel).toBe("error")); + + // Another panel's close leaves this one up. + act(() => result.current.panelToggle("queue").close()); + expect(result.current.panelToggle("error").open).toBe(true); + + act(() => result.current.panelToggle("error").close()); + expect(result.current.panelToggle("error").open).toBe(false); + await waitFor(() => expect(result.current.panel).toBeNull()); + expect(platform.setMiniPlayerPanel).toHaveBeenLastCalledWith(false); + + // Again once it's shut: nothing to do. + platform.setMiniPlayerPanel.mockClear(); + act(() => result.current.panelToggle("error").close()); + expect(result.current.panelToggle("error").open).toBe(false); + expect(platform.setMiniPlayerPanel).not.toHaveBeenCalled(); +}); diff --git a/frontend/tests/player-display.test.tsx b/frontend/tests/player-display.test.tsx index 81f63a83..ed3d4a2c 100644 --- a/frontend/tests/player-display.test.tsx +++ b/frontend/tests/player-display.test.tsx @@ -1,6 +1,7 @@ import { cleanup, fireEvent, render, renderHook, screen, waitFor, within } from "@testing-library/react"; import { afterEach, beforeEach, expect, it, vi } from "vitest"; import type { PlaybackDevice, PlayerState, TrackListItem } from "@music-library/core"; +import MiniPlayerPanel from "../src/components/player/MiniPlayerPanel"; import NowPlaying from "../src/components/player/NowPlaying"; import { PlaybackErrorDetails } from "../src/components/player/PlaybackErrorPopover"; import { usePlayerDisplay } from "../src/components/player/usePlayerDisplay"; @@ -210,19 +211,51 @@ it("keeps the closing details on the failure they opened for", () => { expect(container.textContent).not.toContain("HTTP 404"); }); -it("sends focus back to the error line when closed with its button", async () => { +it.each(["its button", "Escape"])("sends focus back to the error line when closed with %s", async (how) => { const message = "Couldn't play this track."; mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; render(); const line = screen.getByRole("button", { name: message }); fireEvent.click(line); const dialog = await screen.findByRole("dialog", { name: "Playback error" }); + const copy = within(dialog).getByRole("button", { name: "Copy details" }); + copy.focus(); - fireEvent.click(within(dialog).getByRole("button", { name: "Close playback error" })); + if (how === "Escape") fireEvent.keyDown(copy, { key: "Escape" }); + else fireEvent.click(within(dialog).getByRole("button", { name: "Close playback error" })); expect(line.getAttribute("aria-expanded")).toBe("false"); expect(document.activeElement).toBe(line); }); +it("leaves focus alone when the details close from outside", async () => { + const message = "Couldn't play this track."; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + render( + <> + + + , + ); + fireEvent.click(screen.getByRole("button", { name: message })); + await screen.findByRole("dialog", { name: "Playback error" }); + const search = screen.getByRole("textbox", { name: "Search" }); + search.focus(); + + fireEvent.keyDown(search, { key: "Escape" }); + expect(document.activeElement).toBe(search); +}); + +it("shows the details in the mini player's error panel", () => { + const message = "Couldn't play this track."; + mock.remote = { targetDevice: null, commandPending: false }; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + render(); + expect(screen.getByText("Playback error", { selector: ".mini-panel-title" })).toBeTruthy(); + const region = screen.getByRole("region", { name: "Playback error" }); + expect(region.textContent).toContain("HTTP 200 · audio/flac"); + expect(within(region).getByRole("button", { name: "Copy details" })).toBeTruthy(); +}); + it("opens the mini player's panel instead of a popover", () => { const message = "Couldn't play this track."; const onToggle = vi.fn(); From d8c77afd1b196ab963e86f1324faea555c16761f Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:09:44 +0000 Subject: [PATCH 06/11] Playback error details: fourth review fixes - The mini player's error panel keeps the failure it showed while it morphs away, as the popover does while it fades. - The popover's focus comment says what happens on a click elsewhere. Co-Authored-By: Claude Opus 5.5 --- frontend/src/components/MiniPlayer.tsx | 6 +++++- .../src/components/player/MiniPlayerPanel.tsx | 5 ++++- .../components/player/PlaybackErrorPopover.tsx | 5 +++-- frontend/tests/player-display.test.tsx | 17 +++++++++++++++++ 4 files changed, 29 insertions(+), 4 deletions(-) diff --git a/frontend/src/components/MiniPlayer.tsx b/frontend/src/components/MiniPlayer.tsx index 39d53b70..10bd93f8 100644 --- a/frontend/src/components/MiniPlayer.tsx +++ b/frontend/src/components/MiniPlayer.tsx @@ -154,7 +154,11 @@ export default function MiniPlayer() {
{miniPlayerMode && panel && ( - + )} diff --git a/frontend/src/components/player/MiniPlayerPanel.tsx b/frontend/src/components/player/MiniPlayerPanel.tsx index 6c5d7b4b..ca4840d5 100644 --- a/frontend/src/components/player/MiniPlayerPanel.tsx +++ b/frontend/src/components/player/MiniPlayerPanel.tsx @@ -33,9 +33,12 @@ const LABELS: Record = { export default function MiniPlayerPanel({ panel, enter, + closing = false, }: { panel: MiniPanel; enter?: "forward" | "back"; + /** Asked to go: the panel keeps what it showed while it morphs away. */ + closing?: boolean; }) { const position = useQueuePosition(); @@ -64,7 +67,7 @@ export default function MiniPlayerPanel({ ) : panel === "lyrics" ? ( ) : panel === "error" ? ( - + ) : ( )} diff --git a/frontend/src/components/player/PlaybackErrorPopover.tsx b/frontend/src/components/player/PlaybackErrorPopover.tsx index 2ce26dc8..ba00b8dc 100644 --- a/frontend/src/components/player/PlaybackErrorPopover.tsx +++ b/frontend/src/components/player/PlaybackErrorPopover.tsx @@ -34,8 +34,9 @@ export default function PlaybackErrorPopover({ }) { const ref = useRef(null); - // Closing from inside (its button, Escape) sends focus back to the line - // it came from, rather than dropping it with the popover. + // Closing while focus is inside sends it back to the line the popover came + // from, rather than dropping it with the popover. A click elsewhere closes + // it too, and that click's own focus then wins. const close = () => { if (ref.current?.contains(document.activeElement)) toggle?.focus(); onClose(); diff --git a/frontend/tests/player-display.test.tsx b/frontend/tests/player-display.test.tsx index ed3d4a2c..4e8336d3 100644 --- a/frontend/tests/player-display.test.tsx +++ b/frontend/tests/player-display.test.tsx @@ -256,6 +256,23 @@ it("shows the details in the mini player's error panel", () => { expect(within(region).getByRole("button", { name: "Copy details" })).toBeTruthy(); }); +it("keeps the mini player's error panel on its failure while it closes", () => { + const message = "Couldn't play this track."; + mock.remote = { targetDevice: null, commandPending: false }; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + const { rerender } = render(); + + mock.player = { + ...mock.player, + current: { ...track, id: "t2" }, + playbackError: { trackId: "t2", message, details: { ...details, server: "HTTP 404 · not found" } }, + }; + rerender(); + const region = screen.getByRole("region", { name: "Playback error" }); + expect(region.textContent).toContain("HTTP 200 · audio/flac"); + expect(region.textContent).not.toContain("HTTP 404"); +}); + it("opens the mini player's panel instead of a popover", () => { const message = "Couldn't play this track."; const onToggle = vi.fn(); From abf50aad4f588eebf491a7711661a03dd0a50d3e Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:18:11 +0000 Subject: [PATCH 07/11] Playback error details: fifth review fixes - The details freeze only once they've shown a failure: a mini panel whose open morph lands after another panel was asked for no longer opens on an empty body. - describeHlsError's comment says where a quoted address goes (core replaces it), rather than that there is none. Co-Authored-By: Claude Opus 5.5 --- frontend/src/adapters/html-audio-adapter.ts | 5 +++-- frontend/src/components/player/PlaybackErrorPopover.tsx | 4 +++- frontend/tests/player-display.test.tsx | 9 +++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/frontend/src/adapters/html-audio-adapter.ts b/frontend/src/adapters/html-audio-adapter.ts index d453e65a..3c69ab1e 100644 --- a/frontend/src/adapters/html-audio-adapter.ts +++ b/frontend/src/adapters/html-audio-adapter.ts @@ -469,8 +469,9 @@ function describeMediaError(error: MediaError | null): string | null { /** * hls.js's account of the error it gave up on, e.g. "hls.js manifestLoadError: - * A network error (status 502) occurred while loading manifest". Never the - * URL: a segment's carries TIDAL's signed address. + * A network error (status 502) occurred while loading manifest". Not its + * `url`: a segment's carries TIDAL's signed address. The few messages that + * quote one lose it in core, which replaces every address in the detail. */ function describeHlsError(data: ErrorData): string { const message = data.error?.message; diff --git a/frontend/src/components/player/PlaybackErrorPopover.tsx b/frontend/src/components/player/PlaybackErrorPopover.tsx index ba00b8dc..1093a0b4 100644 --- a/frontend/src/components/player/PlaybackErrorPopover.tsx +++ b/frontend/src/components/player/PlaybackErrorPopover.tsx @@ -105,7 +105,9 @@ export function PlaybackErrorDetails({ error: PlaybackError; track: TrackListItem | null; } | null>(null); - const live = frozen ? null : playbackError; + // Frozen only once there's something to keep: a panel that opens already + // on its way out still shows the failure. + const live = frozen && last ? null : playbackError; if (live && live !== last?.error) setLast({ error: live, track: current }); const shown = live ? { error: live, track: current } : last; const { copied, flash } = useCopiedFlag(2000); diff --git a/frontend/tests/player-display.test.tsx b/frontend/tests/player-display.test.tsx index 4e8336d3..8d6cad6e 100644 --- a/frontend/tests/player-display.test.tsx +++ b/frontend/tests/player-display.test.tsx @@ -256,6 +256,15 @@ it("shows the details in the mini player's error panel", () => { expect(within(region).getByRole("button", { name: "Copy details" })).toBeTruthy(); }); +it("shows the failure in a mini panel that opens already on its way out", () => { + const message = "Couldn't play this track."; + mock.remote = { targetDevice: null, commandPending: false }; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + render(); + const region = screen.getByRole("region", { name: "Playback error" }); + expect(region.textContent).toContain("HTTP 200 · audio/flac"); +}); + it("keeps the mini player's error panel on its failure while it closes", () => { const message = "Couldn't play this track."; mock.remote = { targetDevice: null, commandPending: false }; From 3318f325cc7fdc06f8ea36805f72285d5eba94c1 Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:26:29 +0000 Subject: [PATCH 08/11] Playback error details: CodeRabbit review fixes - A failure describes the source the adapter was actually given (loaded or activated), not what the track resolves to now: a download removed while its file plays no longer reads as a failed stream and sends a probe. - The popover takes focus as it opens; portaled to the end of the page, it was otherwise the last thing Tab reached. Closing from inside still sends focus back to the error line. - The popover's height is capped to the room above the bar; in a short window the details scroll under the heading. - The lint:ui spec opens it with a real click, so the line must take the pointer; a test title says what it checks. Co-Authored-By: Claude Opus 5.5 --- core/src/player/use-player-core.ts | 14 ++++++++++--- core/tests/use-player-core.test.tsx | 20 +++++++++++++++++++ .../player/PlaybackErrorPopover.tsx | 13 ++++++++++-- frontend/src/styles/player-popovers.css | 14 +++++++++++++ frontend/tests/player-display.test.tsx | 14 +++++++++++-- frontend/tests/ui/overlays.ui.ts | 3 ++- 6 files changed, 70 insertions(+), 8 deletions(-) diff --git a/core/src/player/use-player-core.ts b/core/src/player/use-player-core.ts index f9ab8895..7bbf9104 100644 --- a/core/src/player/use-player-core.ts +++ b/core/src/player/use-player-core.ts @@ -212,6 +212,10 @@ export function usePlayerCore({ const listenedSecondsRef = useRef(0); const listeningTickRef = useRef(null); const loadedTrackIdRef = useRef(null); + // The source the adapter was last given, as loaded or activated. A failure + // is about it, whatever the track would resolve to now (a download removed + // while it plays still played from the file). + const loadedUriRef = useRef(null); const preparedNextRef = useRef<{ trackId: string; uri: string } | null>(null); const playbackAttemptRef = useRef(0); const handoffRef = useRef<{ track: TrackListItem; attempt: number } | null>(null); @@ -352,7 +356,8 @@ export function usePlayerCore({ setDuration(0); anchorRef.current = { audioTime: 0, wallTime: performance.now() }; loadedTrackIdRef.current = track.id; - adapter.load(resolvePlayableUri(track.id)); + loadedUriRef.current = resolvePlayableUri(track.id); + adapter.load(loadedUriRef.current); startPlayback(); handoffRef.current = { track, attempt: playbackAttemptRef.current }; setIsPlaying(true); @@ -565,6 +570,7 @@ export function usePlayerCore({ crossfade || (isPrepared && adapter.activatePrepared?.(nextUri) === true); if (activated) { loadedTrackIdRef.current = nextTrack.id; + loadedUriRef.current = nextUri; } else { adapter.clearPrepared?.(); } @@ -579,6 +585,7 @@ export function usePlayerCore({ adapter.seek(0); } else { loadedTrackIdRef.current = nextTrack.id; + loadedUriRef.current = nextUri; adapter.load(nextUri); } startPlayback(); @@ -772,7 +779,8 @@ export function usePlayerCore({ if (!current) return; if (loadedTrackIdRef.current === current.id) return; loadedTrackIdRef.current = current.id; - adapter.load(resolvePlayableUri(current.id)); + loadedUriRef.current = resolvePlayableUri(current.id); + adapter.load(loadedUriRef.current); }, [adapter, current, resolvePlayableUri]); const currentId = current?.id; @@ -1011,7 +1019,7 @@ export function usePlayerCore({ if (!track) return; const failure = { trackId: track.id }; failureRef.current = failure; - const uri = resolveUri(track.id); + const uri = loadedUriRef.current ?? resolveUri(track.id); const url = streamUrl(track.id); // A downloaded copy just didn't play; only a stream can be asked again. const streamed = uri === url; diff --git a/core/tests/use-player-core.test.tsx b/core/tests/use-player-core.test.tsx index d146c679..812689c3 100644 --- a/core/tests/use-player-core.test.tsx +++ b/core/tests/use-player-core.test.tsx @@ -903,6 +903,26 @@ describe("usePlayerCore playback errors", () => { expect(fetchMock).not.toHaveBeenCalled(); }); + it("describes the source that was loaded, even if the track resolves elsewhere now", async () => { + const fetchMock = vi.fn(async () => new Response(refusal, { status: 502 })); + vi.stubGlobal("fetch", fetchMock); + let downloaded = true; + const { result, emit } = await setup({ + resolveTrackUri: () => (downloaded ? "file:///offline.flac" : undefined), + }); + act(() => result.current.controls.play(t("a"))); + + // The download is removed while its file plays; then the file fails. + downloaded = false; + act(() => emit("error")); + await settle(); + expect(result.current.state.playbackError?.details).toMatchObject({ + source: "Downloaded copy · offline.flac", + server: null, + }); + expect(fetchMock).not.toHaveBeenCalled(); + }); + it("keeps what the audio player said about the failure", async () => { const fake = createFakeAdapter(); fake.adapter.errorDetail = () => "MEDIA_ERR_DECODE: PIPELINE_ERROR_DECODE"; diff --git a/frontend/src/components/player/PlaybackErrorPopover.tsx b/frontend/src/components/player/PlaybackErrorPopover.tsx index 1093a0b4..e1da6a0f 100644 --- a/frontend/src/components/player/PlaybackErrorPopover.tsx +++ b/frontend/src/components/player/PlaybackErrorPopover.tsx @@ -1,4 +1,4 @@ -import { useRef, useState } from "react"; +import { useEffect, useRef, useState } from "react"; import { createPortal } from "react-dom"; import { Check as CheckIcon, @@ -51,22 +51,31 @@ export default function PlaybackErrorPopover({ const { mounted, visible } = useTransitionMount(open, 180); + // Portaled to the end of the page, it would be the last thing Tab reaches: + // focus moves in as it opens, and goes back to the line when it closes. + useEffect(() => { + if (visible) ref.current?.focus(); + }, [visible]); + if (!mounted || !anchor) return null; const rect = anchor.getBoundingClientRect(); const width = Math.min(440, window.innerWidth - 24); const bottom = Math.max(12, window.innerHeight - rect.top + 8); const left = Math.max(12, Math.min(rect.left, window.innerWidth - width - 12)); + // In a short window the details scroll under the heading, which stays put. + const maxHeight = Math.max(120, window.innerHeight - bottom - 12); return createPortal(
event.stopPropagation()} onMouseDown={(event) => event.stopPropagation()} > diff --git a/frontend/src/styles/player-popovers.css b/frontend/src/styles/player-popovers.css index 41a045fd..9f54a424 100644 --- a/frontend/src/styles/player-popovers.css +++ b/frontend/src/styles/player-popovers.css @@ -250,8 +250,22 @@ now-playing block, so it grows from the left. The body is shared with the mini player's error panel. */ .error-pop { + display: flex; + flex-direction: column; transform-origin: bottom left; } +/* Focus lands on the popover itself as it opens; its controls show focus as + Tab reaches them. */ +.error-pop:focus-visible { + outline: none; +} +.error-pop > .device-pop-head { + flex: none; +} +.error-pop > .error-pop-body { + min-height: 0; + overflow-y: auto; +} .error-pop-body { padding: 12px 14px 14px; display: flex; diff --git a/frontend/tests/player-display.test.tsx b/frontend/tests/player-display.test.tsx index 8d6cad6e..80db35c5 100644 --- a/frontend/tests/player-display.test.tsx +++ b/frontend/tests/player-display.test.tsx @@ -211,6 +211,15 @@ it("keeps the closing details on the failure they opened for", () => { expect(container.textContent).not.toContain("HTTP 404"); }); +it("moves focus into the details as they open", async () => { + const message = "Couldn't play this track."; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + render(); + fireEvent.click(screen.getByRole("button", { name: message })); + const dialog = await screen.findByRole("dialog", { name: "Playback error" }); + await waitFor(() => expect(document.activeElement).toBe(dialog)); +}); + it.each(["its button", "Escape"])("sends focus back to the error line when closed with %s", async (how) => { const message = "Couldn't play this track."; mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; @@ -237,7 +246,8 @@ it("leaves focus alone when the details close from outside", async () => { , ); fireEvent.click(screen.getByRole("button", { name: message })); - await screen.findByRole("dialog", { name: "Playback error" }); + const dialog = await screen.findByRole("dialog", { name: "Playback error" }); + await waitFor(() => expect(document.activeElement).toBe(dialog)); const search = screen.getByRole("textbox", { name: "Search" }); search.focus(); @@ -282,7 +292,7 @@ it("keeps the mini player's error panel on its failure while it closes", () => { expect(region.textContent).not.toContain("HTTP 404"); }); -it("opens the mini player's panel instead of a popover", () => { +it("asks the mini player for its error panel instead of opening a popover", () => { const message = "Couldn't play this track."; const onToggle = vi.fn(); render( diff --git a/frontend/tests/ui/overlays.ui.ts b/frontend/tests/ui/overlays.ui.ts index f7f56923..257bfd84 100644 --- a/frontend/tests/ui/overlays.ui.ts +++ b/frontend/tests/ui/overlays.ui.ts @@ -143,7 +143,8 @@ test("playback error details", async ({ page }, testInfo) => { route.fulfill({ status: 410, contentType: "text/plain", body: "file missing on disk" }), ); await page.locator(".track-title").and(page.getByTitle(menuTrack.title, { exact: true })).dispatchEvent("click"); - await page.locator(".player-bar").getByRole("button", { name: "Couldn't play this track." }).dispatchEvent("click"); + // A real click: the line must take the pointer, not just the event. + await page.locator(".player-bar").getByRole("button", { name: "Couldn't play this track." }).click(); const dialog = page.getByRole("dialog", { name: "Playback error" }); await ready(page, dialog.getByText("HTTP 410 · file missing on disk")); await checkUI(page, testInfo, { From d126aa065818291b587bed7c9c35369328b28c06 Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:34:33 +0000 Subject: [PATCH 09/11] Playback error details: focus lands on Copy details - The popover focuses its Copy details button as it opens, rather than itself. On the container, Space reached the player's play/pause key, which retried the failed track and dropped focus; on the button Space and Enter copy, and keyboard users see where focus is. - Clicking the error line to close the popover takes focus to the line itself, for browsers that don't focus a clicked button (Safari), so it isn't left inside the closing popover. Co-Authored-By: Claude Opus 5.5 --- frontend/src/components/player/NowPlaying.tsx | 11 ++++++++- .../player/PlaybackErrorPopover.tsx | 15 ++++++++---- frontend/src/styles/player-popovers.css | 5 ---- frontend/tests/player-display.test.tsx | 24 ++++++++++++++++--- 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/frontend/src/components/player/NowPlaying.tsx b/frontend/src/components/player/NowPlaying.tsx index a91a3095..ef9ba9e1 100644 --- a/frontend/src/components/player/NowPlaying.tsx +++ b/frontend/src/components/player/NowPlaying.tsx @@ -69,7 +69,16 @@ export default function NowPlaying({ aria-haspopup="dialog" aria-expanded={open} aria-controls={errorPanel?.controls} - onClick={errorPanel ? errorPanel.onToggle : () => setDetailsOpen((v) => !v)} + onClick={ + errorPanel + ? errorPanel.onToggle + : (event) => { + // Closing: focus may be inside the popover, and not + // every browser moves it to a clicked button. + if (detailsOpen) event.currentTarget.focus(); + setDetailsOpen((v) => !v); + } + } > {error} diff --git a/frontend/src/components/player/PlaybackErrorPopover.tsx b/frontend/src/components/player/PlaybackErrorPopover.tsx index e1da6a0f..6c29f473 100644 --- a/frontend/src/components/player/PlaybackErrorPopover.tsx +++ b/frontend/src/components/player/PlaybackErrorPopover.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useEffect, useRef, useState, type Ref } from "react"; import { createPortal } from "react-dom"; import { Check as CheckIcon, @@ -33,6 +33,7 @@ export default function PlaybackErrorPopover({ onClose: () => void; }) { const ref = useRef(null); + const copyRef = useRef(null); // Closing while focus is inside sends it back to the line the popover came // from, rather than dropping it with the popover. A click elsewhere closes @@ -52,9 +53,11 @@ export default function PlaybackErrorPopover({ const { mounted, visible } = useTransitionMount(open, 180); // Portaled to the end of the page, it would be the last thing Tab reaches: - // focus moves in as it opens, and goes back to the line when it closes. + // focus moves in as it opens, to its action, so Space and Enter stay there + // rather than reaching the player's own keys. It goes back to the line + // when the popover closes. useEffect(() => { - if (visible) ref.current?.focus(); + if (visible) copyRef.current?.focus(); }, [visible]); if (!mounted || !anchor) return null; @@ -70,7 +73,6 @@ export default function PlaybackErrorPopover({
- +
, document.body, ); @@ -103,9 +105,11 @@ export default function PlaybackErrorPopover({ */ export function PlaybackErrorDetails({ frozen = false, + copyRef, }: { /** Closing: keep what was shown, even if another failure comes in. */ frozen?: boolean; + copyRef?: Ref; }) { const { playbackError, current } = usePlayer(); // Keeps the last failure, and the track it was on, on screen while the @@ -135,6 +139,7 @@ export function PlaybackErrorDetails({
- +
, document.body, ); @@ -106,10 +110,13 @@ export default function PlaybackErrorPopover({ export function PlaybackErrorDetails({ frozen = false, copyRef, + linesId, }: { /** Closing: keep what was shown, even if another failure comes in. */ frozen?: boolean; copyRef?: Ref; + /** An id for the lines, for whatever they describe. */ + linesId?: string; }) { const { playbackError, current } = usePlayer(); // Keeps the last failure, and the track it was on, on screen while the @@ -129,7 +136,7 @@ export function PlaybackErrorDetails({ const lines = playbackErrorLines(shown.error, shown.track); return (
-
+
{lines.map(({ label, value }) => (
{label}
diff --git a/frontend/tests/player-display.test.tsx b/frontend/tests/player-display.test.tsx index 61362ec2..6c95290c 100644 --- a/frontend/tests/player-display.test.tsx +++ b/frontend/tests/player-display.test.tsx @@ -219,6 +219,10 @@ it("moves focus to the details' action as they open", async () => { const dialog = await screen.findByRole("dialog", { name: "Playback error" }); const copy = within(dialog).getByRole("button", { name: "Copy details" }); await waitFor(() => expect(document.activeElement).toBe(copy)); + // Focus skips the lines, so they describe the dialog for screen readers. + const lines = document.getElementById(dialog.getAttribute("aria-describedby") ?? ""); + expect(lines?.tagName).toBe("DL"); + expect(lines?.textContent).toContain("HTTP 200 · audio/flac"); }); it("sends focus back to the error line when the line closes the details", async () => { From 86cb68ab4008e92220530c3e7638ee60394ca4d1 Mon Sep 17 00:00:00 2001 From: esson <55195336+githubesson@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:51:40 +0000 Subject: [PATCH 11/11] Playback error details: second CodeRabbit review fixes - The loaded source describes a failure only when it is the reported track's: between a track change and its commit, the next track's may be loaded already, so the reported one is resolved again. - The server stops being asked once the failure is gone (retry, another track, unmount): the probe is aborted, and an aborted one reports nothing. - TIDAL's reason gets the same address scrubbing before it's shown. - In the mini player the error line opens a region, so it no longer says it opens a dialog. - A window resize places the popover again. - The panel toggle's close() is tested with mini mode on. Co-Authored-By: Claude Opus 5.5 --- core/src/player/use-player-core.ts | 38 +++++++++++++--- core/tests/use-player-core.test.tsx | 43 +++++++++++++++++++ frontend/src/components/player/NowPlaying.tsx | 3 +- .../player/PlaybackErrorPopover.tsx | 10 +++++ frontend/tests/mini-player-mode.test.tsx | 4 +- frontend/tests/player-display.test.tsx | 25 ++++++++++- 6 files changed, 112 insertions(+), 11 deletions(-) diff --git a/core/src/player/use-player-core.ts b/core/src/player/use-player-core.ts index 7bbf9104..d8fa2261 100644 --- a/core/src/player/use-player-core.ts +++ b/core/src/player/use-player-core.ts @@ -108,11 +108,17 @@ interface StreamProbe { * failure, another error or a reverse proxy's HTML error page, gives no * reason; it is only described. */ -async function probeStream(url: string, { firstByte }: { firstByte: boolean }): Promise { +async function probeStream( + url: string, + { firstByte, signal }: { firstByte: boolean; signal: AbortSignal }, +): Promise { try { // A library file is served with ranges, so one byte answers as well as // the whole file. A TIDAL stream is asked exactly as the player asks it. - const response = await rawFetch(url, firstByte ? { headers: { Range: "bytes=0-0" } } : {}); + const response = await rawFetch(url, { + signal, + ...(firstByte ? { headers: { Range: "bytes=0-0" } } : {}), + }); // Nothing to explain; don't download the stream. void response.body?.cancel().catch(() => {}); const type = response.headers.get("content-type"); @@ -128,7 +134,8 @@ async function probeStream(url: string, { firstByte }: { firstByte: boolean }): const readable = text.length <= 300 && !text.startsWith("<") && !text.includes("\n") ? text : ""; return { - reason: readable && (error.status === 502 || error.status === 503) ? readable : null, + reason: + readable && (error.status === 502 || error.status === 503) ? detailLine(readable) : null, answer: detailLine(`HTTP ${error.status}${readable ? ` · ${readable}` : ""}`) ?? "", }; } @@ -202,6 +209,8 @@ export function usePlayerCore({ // message may still be on the way): playing that track again retries it, // and a message only lands while this is still the same failure. const failureRef = useRef<{ trackId: string } | null>(null); + // The server being asked why the current failure happened. + const probeRef = useRef(null); const playbackReportedRef = useRef(null); const lastFMScrobbledRef = useRef(null); // Lazily seeded (see the track-change effect below) rather than @@ -333,6 +342,9 @@ export function usePlayerCore({ }, [adapter]); const clearPlaybackError = useCallback(() => { + // A failure that's gone has nothing left to ask about. + probeRef.current?.abort(); + probeRef.current = null; if (!failureRef.current) return; failureRef.current = null; setPlaybackError(null); @@ -1019,7 +1031,10 @@ export function usePlayerCore({ if (!track) return; const failure = { trackId: track.id }; failureRef.current = failure; - const uri = loadedUriRef.current ?? resolveUri(track.id); + // The source loaded for this track. Until a track change commits, the + // next track's may be loaded already; this one is resolved again then. + const uri = + (loadedTrackIdRef.current === track.id && loadedUriRef.current) || resolveUri(track.id); const url = streamUrl(track.id); // A downloaded copy just didn't play; only a stream can be asked again. const streamed = uri === url; @@ -1036,10 +1051,18 @@ export function usePlayerCore({ }; // Say so right away; the server's reason, if any, replaces it. report(PLAYBACK_ERROR_FALLBACK); + probeRef.current?.abort(); + probeRef.current = null; if (streamed) { - void probeStream(url, { firstByte: !isTidalTrack(track) }).then(({ reason, answer }) => - // Only TIDAL streams fail for a reason worth showing on the bar. - report(isTidalTrack(track) && reason ? reason : PLAYBACK_ERROR_FALLBACK, answer), + const probe = new AbortController(); + probeRef.current = probe; + void probeStream(url, { firstByte: !isTidalTrack(track), signal: probe.signal }).then( + ({ reason, answer }) => { + if (probe.signal.aborted) return; + if (probeRef.current === probe) probeRef.current = null; + // Only TIDAL streams fail for a reason worth showing on the bar. + report(isTidalTrack(track) && reason ? reason : PLAYBACK_ERROR_FALLBACK, answer); + }, ); } }); @@ -1055,6 +1078,7 @@ export function usePlayerCore({ }, [adapter, startPlayback]); useEffect(() => () => adapter.clearPrepared?.(), [adapter]); + useEffect(() => () => probeRef.current?.abort(), []); // The exposed clock is quantized to 250 ms. Sample at that cadence while // retaining the audio/wall-clock anchor between native updates. diff --git a/core/tests/use-player-core.test.tsx b/core/tests/use-player-core.test.tsx index 812689c3..78b078b9 100644 --- a/core/tests/use-player-core.test.tsx +++ b/core/tests/use-player-core.test.tsx @@ -923,6 +923,49 @@ describe("usePlayerCore playback errors", () => { expect(fetchMock).not.toHaveBeenCalled(); }); + it("stops asking the server once the failure is gone", async () => { + const fetchMock = vi.fn((_url: string, _init?: RequestInit) => new Promise(() => {})); + vi.stubGlobal("fetch", fetchMock); + const { result, emit } = await setup(); + act(() => result.current.controls.play(t("a"), [t("a"), t("b")])); + act(() => emit("error")); + const signal = fetchMock.mock.calls[0][1]?.signal; + expect(signal?.aborted).toBe(false); + + act(() => result.current.controls.next()); + expect(signal?.aborted).toBe(true); + expect(result.current.state.playbackError).toBeNull(); + }); + + it("keeps no address from TIDAL's reason either", async () => { + vi.stubGlobal("fetch", vi.fn(async () => + new Response("TIDAL refused to stream this track: see https://cdn.example/x?sig=abc", { status: 502 }), + )); + const { result, emit } = await setup(); + act(() => result.current.controls.play(tidal("1"))); + + act(() => emit("error")); + await settle(); + const message = result.current.state.playbackError?.message ?? ""; + expect(message).toBe("TIDAL refused to stream this track: see
"); + }); + + it("describes the failed track's own source while the next one is loading", async () => { + const { result, emit } = await setup(); + act(() => result.current.controls.play(t("a"), [t("a"), t("b")])); + + // The next track's source is loaded before its change commits; a failure + // reported then is still the playing track's. + act(() => { + result.current.controls.next(); + emit("error"); + }); + expect(result.current.state.playbackError).toMatchObject({ + trackId: "a", + details: { source: "test://stream/a" }, + }); + }); + it("keeps what the audio player said about the failure", async () => { const fake = createFakeAdapter(); fake.adapter.errorDetail = () => "MEDIA_ERR_DECODE: PIPELINE_ERROR_DECODE"; diff --git a/frontend/src/components/player/NowPlaying.tsx b/frontend/src/components/player/NowPlaying.tsx index ef9ba9e1..91b31999 100644 --- a/frontend/src/components/player/NowPlaying.tsx +++ b/frontend/src/components/player/NowPlaying.tsx @@ -66,7 +66,8 @@ export default function NowPlaying({ type="button" className="np-error-btn" title={error} - aria-haspopup="dialog" + // The mini player's panel is a region, not a dialog. + aria-haspopup={errorPanel ? undefined : "dialog"} aria-expanded={open} aria-controls={errorPanel?.controls} onClick={ diff --git a/frontend/src/components/player/PlaybackErrorPopover.tsx b/frontend/src/components/player/PlaybackErrorPopover.tsx index 79d43ddb..c26e6046 100644 --- a/frontend/src/components/player/PlaybackErrorPopover.tsx +++ b/frontend/src/components/player/PlaybackErrorPopover.tsx @@ -63,6 +63,16 @@ export default function PlaybackErrorPopover({ if (visible) copyRef.current?.focus(); }, [visible]); + // Placed from the line's position at render; a window resize places it + // again. + const [, setResized] = useState(0); + useEffect(() => { + if (!open) return; + const onResize = () => setResized((n) => n + 1); + window.addEventListener("resize", onResize); + return () => window.removeEventListener("resize", onResize); + }, [open]); + if (!mounted || !anchor) return null; const rect = anchor.getBoundingClientRect(); diff --git a/frontend/tests/mini-player-mode.test.tsx b/frontend/tests/mini-player-mode.test.tsx index fefadbfd..d9eb569f 100644 --- a/frontend/tests/mini-player-mode.test.tsx +++ b/frontend/tests/mini-player-mode.test.tsx @@ -5,7 +5,7 @@ import { useMiniPlayerMode } from "../src/components/player/useMiniPlayerMode"; const platform = vi.hoisted(() => ({ canSetMiniPlayer: () => true, getMiniPlayerPanelAnchor: vi.fn(async () => "top" as const), - setMiniPlayerMode: vi.fn(async () => ({ ok: true })), + setMiniPlayerMode: vi.fn(async (on: boolean) => ({ ok: true, miniPlayer: on })), setMiniPlayerPanel: vi.fn(async (open: boolean) => ({ ok: true, open, anchor: "top" as const })), })); vi.mock("../src/lib/platform", () => platform); @@ -17,6 +17,8 @@ afterEach(cleanup); it("closes a panel through its own toggle only", async () => { const { result } = renderHook(() => useMiniPlayerMode()); + await act(() => result.current.toggleMiniPlayerMode()); + expect(result.current.miniPlayerMode).toBe(true); act(() => result.current.panelToggle("error").onToggle()); await waitFor(() => expect(result.current.panel).toBe("error")); diff --git a/frontend/tests/player-display.test.tsx b/frontend/tests/player-display.test.tsx index 6c95290c..0ae4a40d 100644 --- a/frontend/tests/player-display.test.tsx +++ b/frontend/tests/player-display.test.tsx @@ -1,4 +1,4 @@ -import { cleanup, fireEvent, render, renderHook, screen, waitFor, within } from "@testing-library/react"; +import { act, cleanup, fireEvent, render, renderHook, screen, waitFor, within } from "@testing-library/react"; import { afterEach, beforeEach, expect, it, vi } from "vitest"; import type { PlaybackDevice, PlayerState, TrackListItem } from "@music-library/core"; import MiniPlayerPanel from "../src/components/player/MiniPlayerPanel"; @@ -326,11 +326,32 @@ it("asks the mini player for its error panel instead of opening a popover", () = errorPanel={{ open: false, controls: "mini-player-panel", onToggle, close: vi.fn() }} />, ); - fireEvent.click(screen.getByRole("button", { name: message })); + const line = screen.getByRole("button", { name: message }); + expect(line.hasAttribute("aria-haspopup")).toBe(false); + fireEvent.click(line); expect(onToggle).toHaveBeenCalledOnce(); expect(screen.queryByRole("dialog")).toBeNull(); }); +it("places the details again when the window is resized", async () => { + const message = "Couldn't play this track."; + mock.player = { ...mock.player, current: track, playbackError: { trackId: "t1", message, details } }; + const { container } = render(); + const block = container.querySelector(".np")!; + let top = 700; + vi.spyOn(block, "getBoundingClientRect").mockImplementation(() => ({ top, left: 20 }) as DOMRect); + fireEvent.click(screen.getByRole("button", { name: message })); + const dialog = await screen.findByRole("dialog", { name: "Playback error" }); + const bottom = () => dialog.style.bottom; + const before = bottom(); + + top = 500; + act(() => { + window.dispatchEvent(new Event("resize")); + }); + expect(bottom()).not.toBe(before); +}); + it("shuts the mini player's error panel when the failure clears", () => { const message = "Couldn't play this track."; const close = vi.fn();