diff --git a/docs/api-v2-guide.md b/docs/api-v2-guide.md index 043a0682..f09116d6 100644 --- a/docs/api-v2-guide.md +++ b/docs/api-v2-guide.md @@ -777,10 +777,25 @@ deployment setting and never a member's, so a client never sends one. { "provider": "maloja", "destination": "alice", "available": true }, { "provider": "maloja", "destination": "bob", "available": true }, { "provider": "lastfm", "destination": "default", "available": false, - "unavailable": "last.fm needs WAVEFLOW_PUBLIC_URL to be an https address for the browser journey; an operator can still link an account from this server's command line" } + "unavailable": "browser_journey_needs_https" } ] ``` +`unavailable` is a **case, not a sentence**, and it is present only when +`available` is `false`. It was finished English prose before 2026-09-15, +which every client printed verbatim — including one that ships in two +languages, so a French reader was told in English what to change in a +configuration file. The wording belongs to whoever is doing the telling. Two +cases, and both name something an operator can change: + +| Case | What is missing | +| --- | --- | +| `no_application_configured` | `WAVEFLOW_SCROBBLE_LASTFM_API_KEY` and `_SECRET` are unset, so there is no Last.fm application to speak for. | +| `browser_journey_needs_https` | `WAVEFLOW_PUBLIC_URL` is not an `https` address, so there is nowhere to bring a browser back to. The command-line journey below needs no public address and still works. | + +Treat an unrecognised case as "this server did not say why" rather than as a +blank: a server is free to be newer than the client reading it. + Several instances of one recipient are ordinary — Maloja self-hosts, and on a household server everybody has their own. Addresses are never published: a member does not need one, and no link stores one. @@ -948,7 +963,30 @@ unaffected. Creating a library starts its first scan and returns both `library_id` and `scan_id`. The scan event route uses Server-Sent Events and still requires the -Bearer token. +Bearer token — so `EventSource`, which sends no headers, cannot open it; read +the response body of a `fetch` instead. + +**Both of its frames carry the same object**, the one +`GET /api/v2/scans/{scan_id}` answers: an opening `snapshot` event, then a +`progress` event per step. A watcher replaces what it holds and never has to +tell the two apart. + +```text +event: snapshot +data: {"id":"…","library_id":"…","status":"running","total_files":164,"processed_files":0, + "added":0,"updated":0,"moved":0,"skipped":0,"unavailable":0,"errors":0, + "current_path":null,"message":null} + +event: progress +data: {"id":"…","library_id":"…","status":"running","total_files":164,"processed_files":61, …} +``` + +Builds before 2026-09-15 sent a different object for `progress`, naming +those same three fields `scan_id`, `total` and `processed` while the ten +counters beside them kept their names. A client bound to the snapshot therefore +showed its totals as absent from the first progress frame on, and its +per-outcome counters correctly — which reads as a display bug rather than as +two shapes on one stream. There is one shape now. `GET /api/v2/libraries` answers, for each library the account belongs to, its `role` and `accepts_uploads`. The flag says whether the library takes files at diff --git a/src/api/libraries.rs b/src/api/libraries.rs index 666ef6bb..f0a51692 100644 --- a/src/api/libraries.rs +++ b/src/api/libraries.rs @@ -202,12 +202,16 @@ pub async fn scan_events( .map_err(db_error)? .ok_or(ApiError::NotFound)?; let mut receiver = state.scanner.subscribe(scan_id); + // Both events carry a `ScanJobRecord`, so a watcher replaces what it holds + // and never has to tell the two frames apart. `progress` used to carry the + // scanner's own `ScanProgress`, which names three of the same fields + // differently — see `ScanProgress::record`. let output = async_stream::stream! { yield Ok(Event::default().event("snapshot").json_data(initial).expect("scan snapshot serializes")); if let Some(ref mut receiver) = receiver { loop { match receiver.recv().await { - Ok(progress) => yield Ok(Event::default().event("progress").json_data(progress).expect("scan progress serializes")), + Ok(progress) => yield Ok(Event::default().event("progress").json_data(progress.record()).expect("scan progress serializes")), Err(tokio::sync::broadcast::error::RecvError::Lagged(_)) => continue, Err(tokio::sync::broadcast::error::RecvError::Closed) => break, } diff --git a/src/lib.rs b/src/lib.rs index a5a31605..aaba035e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -242,6 +242,7 @@ fn chunk_body_limit(limits: &config::UploadLimits) -> usize { services::ScrobbleProvider, services::ScrobbleLinkState, services::ScrobbleDestinationName, + services::ScrobbleUnavailable, api::LastFmAuthorizationResponse, services::UncertainScrobble, api::LinkScrobbleRequest, @@ -289,8 +290,7 @@ fn chunk_body_limit(limits: &config::UploadLimits) -> usize { sync::SyncPage, media::StreamTicketResponse, media::CanvasResponse, - api::LibraryEventAckRequest, - scanner::ScanProgress + api::LibraryEventAckRequest )), modifiers(&SecurityAddon), tags( diff --git a/src/scanner.rs b/src/scanner.rs index ba5d8769..1ebe6c56 100644 --- a/src/scanner.rs +++ b/src/scanner.rs @@ -15,21 +15,31 @@ use lofty::{ file::TaggedFileExt, prelude::{Accessor, AudioFile, ItemKey}, }; -use serde::Serialize; use tokio::sync::{broadcast, Mutex}; -use utoipa::ToSchema; use uuid::Uuid; use walkdir::WalkDir; use crate::{ - catalog::{ApplyOutcome, ArtworkInput, CatalogApply, CatalogTrackInput, LibraryRecord}, + catalog::{ + ApplyOutcome, ArtworkInput, CatalogApply, CatalogTrackInput, LibraryRecord, ScanJobRecord, + }, database::Database, lyrics::{self, LyricsInput}, }; const MAX_LYRICS_BYTES: u64 = 1024 * 1024; -#[derive(Debug, Clone, Serialize, ToSchema)] +/// One reading of a scan in flight, as it travels between the scanning task and +/// whoever is watching. +/// +/// Deliberately **not** `Serialize`. It used to be, and `/api/v2/scans/{id}/events` +/// yielded it verbatim for every `progress` event while the opening `snapshot` +/// carried a [`ScanJobRecord`] — two shapes of the same reading on one stream, +/// disagreeing on three names out of thirteen. A watcher that bound `total_files` +/// showed `undefined` from the first progress frame on. The stream now converts +/// through [`ScanProgress::record`], and dropping the derive is what stops the +/// other shape coming back: there is no way to put this type on a wire. +#[derive(Debug, Clone)] pub struct ScanProgress { pub scan_id: Uuid, pub library_id: Uuid, @@ -46,6 +56,28 @@ pub struct ScanProgress { pub message: Option, } +impl ScanProgress { + /// The same reading in the shape `GET /api/v2/scans/{id}` answers, which is + /// the one shape the event stream speaks. + pub fn record(&self) -> ScanJobRecord { + ScanJobRecord { + id: self.scan_id, + library_id: self.library_id, + status: self.status.clone(), + total_files: self.total as i64, + processed_files: self.processed as i64, + added: self.added, + updated: self.updated, + moved: self.moved, + skipped: self.skipped, + unavailable: self.unavailable, + errors: self.errors, + current_path: self.current_path.clone(), + message: self.message.clone(), + } + } +} + #[derive(Clone)] pub struct ScanManager { db: Database, diff --git a/src/services/lastfm.rs b/src/services/lastfm.rs index beaabf77..8e9a5d67 100644 --- a/src/services/lastfm.rs +++ b/src/services/lastfm.rs @@ -84,15 +84,16 @@ impl LastFmJourney { } } -/// Why Last.fm cannot be linked on this server, in words an operator can act -/// on. +/// Why Last.fm cannot be linked on this server. /// /// Published beside the destination rather than discovered when somebody tries: /// a link that fails later without explanation is the silent failure this whole /// RFC spends itself preventing. -pub(super) fn lastfm_unavailability(config: &crate::config::Config) -> Option<&'static str> { +pub(super) fn lastfm_unavailability( + config: &crate::config::Config, +) -> Option { if config.lastfm.is_none() { - return Some("no last.fm application is configured on this server"); + return Some(super::ScrobbleUnavailable::NoApplicationConfigured); } if !crate::api::public_url_is_https(config.public_url.as_deref()) { // Naming the other way out, since 2026-09-15. This said last.fm @@ -101,10 +102,7 @@ pub(super) fn lastfm_unavailability(config: &crate::config::Config) -> Option<&' // all — it is true of *this* journey. A reason an operator can act on // has to name the action that still exists, or it reads as a dead end // where there is a door. - return Some( - "last.fm needs WAVEFLOW_PUBLIC_URL to be an https address for the browser journey; \ - an operator can still link an account from this server's command line", - ); + return Some(super::ScrobbleUnavailable::BrowserJourneyNeedsHttps); } None } diff --git a/src/services/mod.rs b/src/services/mod.rs index e80e96fb..8c72e1fc 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -995,9 +995,9 @@ pub struct DomainServices { /// What one Last.fm journey needs beyond an account and a destination, or /// `None` when this server cannot carry one at all. lastfm_journey: Option, - /// Why not, in words an operator can act on. Published beside the + /// Why not, as a code the caller words. Published beside the /// destination rather than discovered when somebody tries to link. - lastfm_unavailable: Option<&'static str>, + lastfm_unavailable: Option, /// How a request token becomes a session key, by destination name. Filled /// after construction like [`Self::register_scrobble_target`], and for the /// same reason. @@ -1077,7 +1077,7 @@ pub use lastfm::{ }; pub use scrobbling::{ ScrobbleDestinationName, ScrobbleDrain, ScrobbleEnvelope, ScrobbleLinkState, ScrobbleProvider, - ScrobbleTarget, ScrobbleVerdict, UncertainScrobble, + ScrobbleTarget, ScrobbleUnavailable, ScrobbleVerdict, UncertainScrobble, }; impl DomainServices { diff --git a/src/services/scrobbling.rs b/src/services/scrobbling.rs index b2b5e7ef..978c3058 100644 --- a/src/services/scrobbling.rs +++ b/src/services/scrobbling.rs @@ -148,7 +148,7 @@ pub struct ScrobbleDestinationName { pub destination: String, /// Whether this server can actually link it right now. pub available: bool, - /// And when it cannot, why — in words an operator can act on. + /// And when it cannot, why. /// /// Published here rather than left to be discovered: a link that fails /// later with no explanation is the silent failure this whole RFC spends @@ -156,7 +156,27 @@ pub struct ScrobbleDestinationName { /// still unusable, because its journey needs an application the operator /// registered and an `https` address to bring a person back to. #[serde(skip_serializing_if = "Option::is_none")] - pub unavailable: Option<&'static str>, + pub unavailable: Option, +} + +/// Why a declared instance cannot be linked — a code, not a sentence. +/// +/// This was an English sentence until 2026-09-15, printed verbatim into a web +/// client that ships in two languages: a French reader was told, in English, +/// what to change in their configuration. A reason only helps whoever can read +/// it, so what crosses the wire is the case and the wording belongs to whoever +/// is doing the telling. The two cases are the two things an operator can +/// actually change. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, ToSchema)] +#[serde(rename_all = "snake_case")] +pub enum ScrobbleUnavailable { + /// No Last.fm application is declared on this server — + /// `WAVEFLOW_SCROBBLE_LASTFM_API_KEY` and `_SECRET`. + NoApplicationConfigured, + /// `WAVEFLOW_PUBLIC_URL` is not an `https` address, so there is nowhere to + /// bring a browser back to. The command-line journey needs no public + /// address at all and still works. + BrowserJourneyNeedsHttps, } /// What one link's queue looks like from outside: counters, never content. diff --git a/tests/scanner.rs b/tests/scanner.rs index 77ff8f26..ef814efa 100644 --- a/tests/scanner.rs +++ b/tests/scanner.rs @@ -921,3 +921,91 @@ async fn an_unreferenced_cover_and_its_thumbnails_go_once_they_are_old_enough() "the dead link is reported, not repaired" ); } + +/// The event stream speaks one shape, and every counter lands where it belongs. +/// +/// `/api/v2/scans/{id}/events` opens with a `snapshot` carrying a +/// `ScanJobRecord` and then sends a `progress` per step. Those frames used to +/// be two different types: `progress` carried the scanner's own `ScanProgress`, +/// which names `id`, `total_files` and `processed_files` as `scan_id`, `total` +/// and `processed`. A watcher bound to the snapshot read `undefined` from the +/// first progress frame on, while the ten counters that happen to share a name +/// kept working — which is why it read as a display bug rather than a shape. +/// +/// That the frames agree is now a compile-time fact: `ScanProgress` is no +/// longer `Serialize`, so the handler cannot yield it and has to convert. What +/// the compiler cannot check is that the conversion puts each number where it +/// belongs — eleven of the thirteen fields are `i64` or `Uuid`, so a transposed +/// pair type-checks perfectly. That is what this pins. +#[test] +fn scan_progress_converts_into_the_shape_the_stream_sends() { + let scan = uuid::Uuid::new_v4(); + let library = uuid::Uuid::new_v4(); + let record = waveflow_server::scanner::ScanProgress { + scan_id: scan, + library_id: library, + status: "running".into(), + total: 97, + processed: 61, + added: 1, + updated: 2, + moved: 3, + skipped: 4, + unavailable: 5, + errors: 6, + current_path: Some("Rue Delacour/Nocturne.flac".into()), + message: Some("still reading".into()), + } + .record(); + + // Distinct values throughout, so a swap between any two shows up here + // rather than in a client six months later. + assert_eq!( + record.id, scan, + "the scan's own id, under the snapshot's name" + ); + assert_eq!(record.library_id, library); + assert_eq!(record.status, "running"); + assert_eq!(record.total_files, 97); + assert_eq!(record.processed_files, 61); + assert_eq!(record.added, 1); + assert_eq!(record.updated, 2); + assert_eq!(record.moved, 3); + assert_eq!(record.skipped, 4); + assert_eq!(record.unavailable, 5); + assert_eq!(record.errors, 6); + assert_eq!( + record.current_path.as_deref(), + Some("Rue Delacour/Nocturne.flac") + ); + assert_eq!(record.message.as_deref(), Some("still reading")); + + // And the names themselves, since they are the whole point: a client binds + // these thirteen and nothing else. + let json = serde_json::to_value(&record).unwrap(); + let mut keys: Vec<&str> = json + .as_object() + .unwrap() + .keys() + .map(String::as_str) + .collect(); + keys.sort_unstable(); + assert_eq!( + keys, + [ + "added", + "current_path", + "errors", + "id", + "library_id", + "message", + "moved", + "processed_files", + "skipped", + "status", + "total_files", + "unavailable", + "updated", + ] + ); +} diff --git a/tests/scrobbling.rs b/tests/scrobbling.rs index 9d2bd16b..ed3b3238 100644 --- a/tests/scrobbling.rs +++ b/tests/scrobbling.rs @@ -41,7 +41,8 @@ use waveflow_server::catalog::LibraryRecord; use waveflow_server::config::ScrobbleLimits; use waveflow_server::database::LibraryVisibility; use waveflow_server::services::{ - ScrobbleEnvelope, ScrobbleProvider, ScrobbleTarget, ScrobbleVerdict, ServiceError, + ScrobbleEnvelope, ScrobbleProvider, ScrobbleTarget, ScrobbleUnavailable, ScrobbleVerdict, + ServiceError, }; use waveflow_server::AppState; @@ -3821,7 +3822,10 @@ async fn last_fm_says_why_it_is_unavailable_instead_of_failing_later() { let declared = state.services.scrobble_destinations(); assert_eq!(declared.len(), 1); assert!(!declared[0].available); - assert!(declared[0].unavailable.unwrap().contains("application")); + assert_eq!( + declared[0].unavailable, + Some(ScrobbleUnavailable::NoApplicationConfigured) + ); assert!(matches!( state .services @@ -3838,7 +3842,10 @@ async fn last_fm_says_why_it_is_unavailable_instead_of_failing_later() { let state = waveflow_server::initialize(&plaintext).await.unwrap(); let declared = state.services.scrobble_destinations(); assert!(!declared[0].available); - assert!(declared[0].unavailable.unwrap().contains("https")); + assert_eq!( + declared[0].unavailable, + Some(ScrobbleUnavailable::BrowserJourneyNeedsHttps) + ); assert!(matches!( state .services diff --git a/webapp/e2e/studio-nocturne.spec.ts b/webapp/e2e/studio-nocturne.spec.ts index 6dba02ca..1c73011c 100644 --- a/webapp/e2e/studio-nocturne.spec.ts +++ b/webapp/e2e/studio-nocturne.spec.ts @@ -214,10 +214,14 @@ let loseAcknowledgementOf: number | null = null; */ type Recipient = "listenbrainz" | "maloja" | "lastfm"; -/** Copied from `lastfm_unavailability`, which is what a real server answers. */ -const NO_PUBLIC_URL = - "last.fm needs WAVEFLOW_PUBLIC_URL to be an https address for the browser journey; " + - "an operator can still link an account from this server's command line"; +/** + * What `lastfm_unavailability` answers — a case, not a sentence. + * + * It used to be the server's own English prose, copied here and asserted + * verbatim below, so the test agreed with the server about a string neither of + * them should have been sending to a client that ships in two languages. + */ +const NO_PUBLIC_URL = "browser_journey_needs_https"; const freshDestinations = () => [ { provider: "listenbrainz" as const, destination: "default", available: true }, @@ -274,6 +278,25 @@ let scrobbleWrites: Array<{ method: string; path: string; body?: unknown }> = [] /** Where the Last.fm journey says to send the browser. */ let lastFmAuthorizeUrl = "https://www.last.fm/api/auth/?api_key=k&cb=back"; +/** + * The words the mock holds for each track. Absent is a track without any. + * + * Shaped like `src/lyrics.rs` answers, `camelCase` envelope included — written + * from the server rather than from `LyricsList` in `webapp/src/api.ts`, which + * is how that type spent a release claiming `structured_lyrics` while every + * read of it threw. + */ +let lyricSheets = new Map< + string, + Array<{ + displayArtist: string | null; + displayTitle: string; + lang: string; + synced: boolean; + line: Array<{ start?: number; value: string }>; + }> +>(); + /** The loop the mock holds for each track, as bytes. Absent is none. */ let canvases = new Map(); /** Every placement and removal the mock received, in order. */ @@ -542,11 +565,15 @@ async function mockAuthenticatedApi(page: Page) { url.pathname.startsWith("/api/v2/tracks/") && url.pathname.endsWith("/lyrics") ) { - // A track without words. The playing page asks for them, and the - // catch-all below would answer with a track, which is not a list of - // lyrics — no test visited that page before the canvas put a loop on it. + // The playing page asks for these, and the catch-all below would answer + // with a track, which is not a list of lyrics — no test visited that page + // before the canvas put a loop on it. Empty unless a test says otherwise. + const trackId = url.pathname.split("/")[4] as string; await route.fulfill({ - json: { track_id: url.pathname.split("/")[4], structured_lyrics: [] }, + json: { + trackId, + structuredLyrics: lyricSheets.get(trackId) ?? [], + }, }); return; } @@ -683,6 +710,7 @@ test.beforeEach(async ({ page }) => { uploadCommits = []; uploadSession = freshUploadSession(); loseAcknowledgementOf = null; + lyricSheets = new Map(); canvases = new Map(); canvasWrites = []; refuseCanvasWith = null; @@ -801,6 +829,59 @@ test("sorts through the server and filters in the browser", async ({ await expect(page.getByText("Nothing matches that filter.")).toBeVisible(); }); +/** + * Covers are asked for when they are about to be seen, not when the page mounts. + * + * `/api/v2/artwork/{hash}` needs an `Authorization` header, so each thumbnail + * costs a `fetch` of its own. A library of 164 tracks opened with 127 requests + * in half a second against a connection limit of six, and the covers below the + * fold were queued ahead of the ones being looked at. + * + * Asserted on one card at the far end rather than on a count: a count depends + * on the viewport and would have to be loosened until it stopped meaning + * anything. What this pins is the property — off screen is not asked for, and + * scrolling to it asks. + */ +test("asks for a cover when it comes into view, and not before", async ({ + page, +}) => { + const asked: string[] = []; + const shelf = Array.from({ length: 60 }, (_, index) => ({ + id: `album-lazy-${index}`, + library_id: "library-1", + title: `Shelf ${String(index).padStart(2, "0")}`, + artist: "Rue Delacour", + artist_id: "artist-1", + artwork_hash: `hash-${index}`, + year: 2000 + index, + starred_at: null, + user_rating: null, + })); + + // Registered after `mockAuthenticatedApi`, so these win: Playwright tries + // handlers newest first. + await page.route("**/api/v2/albums*", async (route) => { + await route.fulfill({ json: shelf }); + }); + await page.route("**/api/v2/artwork/*", async (route) => { + asked.push(new URL(route.request().url()).pathname.split("/").pop() ?? ""); + // A 404 is enough: this is about which requests leave, not what comes back. + await route.fulfill({ status: 404, body: "" }); + }); + + await page.goto("/"); + const last = page.getByText("Shelf 59"); + await expect(last).toBeVisible(); + + // The top of the shelf is on screen and has been asked for; the bottom is + // sixty cards away and has not. + await expect.poll(() => asked).toContain("hash-0"); + expect(asked).not.toContain("hash-59"); + + await last.scrollIntoViewIfNeeded(); + await expect.poll(() => asked).toContain("hash-59"); +}); + /** * The card actions guard themselves while their album's tracks are being * fetched. The guard was one album id, which meant two cards in flight shared @@ -1569,6 +1650,33 @@ test("offers removal but no new canvas where the library takes none", async ({ * name. It steps aside when motion is reduced, and when the browser cannot play * it — the cover shows then, never an empty frame. */ +/** + * The playing page, reading what the server actually sends. + * + * Until 2026-09-15 `LyricsList` declared a `snake_case` envelope the server has + * never sent, so this page threw on its first property and showed nothing at + * all. Nothing caught it because the mock above was written from that type. An + * assertion on the words themselves is what tells the two shapes apart: an + * empty sheet renders the same "no lyrics" line under either name. + */ +test("shows the words a track travels with", async ({ page }) => { + lyricSheets.set("song-1", [ + { + displayArtist: "Rue Delacour", + displayTitle: "Nocturne", + lang: "eng", + synced: false, + line: [{ value: "the lamps come on along the quay" }], + }, + ]); + await page.goto("/playing"); + await expect( + page.getByRole("listitem").filter({ + hasText: "the lamps come on along the quay", + }), + ).toBeVisible(); +}); + test("plays a track's canvas over its cover, and steps aside for it", async ({ page, }) => { @@ -1629,10 +1737,14 @@ test("offers each instance by name, and says why one cannot be linked", async ({ ).toBeVisible(); // Declared and unusable is a real shape, and the reason is published so the - // person is not left to discover it when a link fails later. + // person is not left to discover it when a link fails later. Asserted on the + // words this client chose for `NO_PUBLIC_URL`, not on the code it was given: + // a case printed raw would satisfy any assertion made on the code itself. await expect( page.getByText( - `Unavailable here: ${NO_PUBLIC_URL}`, + "Unavailable here: the browser journey needs this server’s public " + + "address to be an https one — an operator can still link an account " + + "from the command line", ), ).toBeVisible(); await expect( @@ -1645,6 +1757,52 @@ test("offers each instance by name, and says why one cannot be linked", async ({ expect(results.violations).toEqual([]); }); +/** + * A server newer than the client reading it. + * + * `unavailable` carries a case rather than a sentence, so this client has a + * table of words for the cases it knows — and a case it does not know must not + * become a blank where a reason should be. The type is open for the same + * reason: a union naming only today's cases would be an assertion about the + * wire that the wire never made, which is the fault this whole change set + * exists to remove. + */ +test("says a reason it does not recognise is a reason, not a blank", async ({ + page, +}) => { + const lastfm = destinations.find((row) => row.provider === "lastfm"); + if (lastfm) lastfm.unavailable = "a_case_from_a_later_release"; + + await page.goto("/settings/scrobbling"); + + await expect( + page.getByText("Unavailable here: this server did not say why"), + ).toBeVisible(); +}); + +/** + * `toString` is not a translation key. + * + * Opening the union widened the lookup key to `string`, and a plain object + * answers for names nobody put in it: `toString` and `constructor` come back + * off the prototype, truthy, and would reach `t()` as if they were keys. Every + * inherited name is a case this client does not know, and must read as one. + */ +for (const inherited of ["toString", "constructor", "hasOwnProperty"]) { + test(`treats the inherited name ${inherited} as a case it does not know`, async ({ + page, + }) => { + const lastfm = destinations.find((row) => row.provider === "lastfm"); + if (lastfm) lastfm.unavailable = inherited; + + await page.goto("/settings/scrobbling"); + + await expect( + page.getByText("Unavailable here: this server did not say why"), + ).toBeVisible(); + }); +} + test("links one instance by its key, and leaves the other alone", async ({ page, }) => { diff --git a/webapp/src/api.ts b/webapp/src/api.ts index f19d6e05..81314f38 100644 --- a/webapp/src/api.ts +++ b/webapp/src/api.ts @@ -909,9 +909,18 @@ export type StructuredLyrics = { line: LyricsLine[]; }; +/** + * What `GET /api/v2/tracks/{id}/lyrics` answers. + * + * `camelCase`, because `src/lyrics.rs` renames the whole struct that way — the + * envelope as much as the `StructuredLyrics` it carries. This declared + * `track_id`/`structured_lyrics` until 2026-09-15, so every read of it threw on + * the first property and the playing page was unreachable. `StructuredLyrics` + * above was already right: the inside was converted and the envelope forgotten. + */ export type LyricsList = { - track_id: string; - structured_lyrics: StructuredLyrics[]; + trackId: string; + structuredLyrics: StructuredLyrics[]; }; export const getLyrics = (trackId: string) => @@ -1005,17 +1014,42 @@ export const scrobble = (trackId: string, submission: boolean) => */ export type ScrobbleProvider = "listenbrainz" | "maloja" | "lastfm"; +/** + * The cases this build has been taught to word. + * + * Exhaustive over what the server sends *today*, which is what a screen's + * translation table must cover. + */ +export type KnownScrobbleUnavailable = + | "no_application_configured" + | "browser_journey_needs_https"; + +/** + * Why a declared instance cannot be linked — a case, not a sentence. + * + * The server sent its own English prose here until 2026-09-15, which this + * client printed verbatim: a French reader was told in English what to change + * in a configuration file. The wording belongs to whoever is doing the + * telling, so the wire carries only the case. + * + * Open on purpose. A server is free to be newer than the client reading it, so + * a type naming only today's cases would be the same untruth this file has + * just been cleared of: an assertion about the wire that the wire never made. + * `string & {}` keeps the known cases in autocomplete while admitting the rest, + * so handling an unrecognised one needs no cast to express. + */ +export type ScrobbleUnavailable = KnownScrobbleUnavailable | (string & {}); + /** One instance a member may link, and whether this server can link it now. */ export type ScrobbleDestination = { provider: ScrobbleProvider; destination: string; available: boolean; /** - * Why not, when it is not — in words an operator can act on. Present only - * for an unavailable one, which is why a screen must not read it as the - * reason a link failed. + * Why not, when it is not. Present only for an unavailable one, which is why + * a screen must not read it as the reason a link failed. */ - unavailable?: string; + unavailable?: ScrobbleUnavailable; }; /** diff --git a/webapp/src/artwork.tsx b/webapp/src/artwork.tsx index 661f7000..127784a5 100644 --- a/webapp/src/artwork.tsx +++ b/webapp/src/artwork.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import { useCallback, useEffect, useState } from "react"; import { artworkUrl } from "./api"; @@ -8,24 +8,94 @@ type ArtworkProps = { className?: string; }; +/** + * How far ahead of the viewport a cover starts loading. + * + * Enough that scrolling at a normal speed meets an image already there, and + * little enough that a long list still asks for a fraction of itself. One + * row's height, roughly, on every layout this client has. + */ +const AHEAD = "300px"; + +/** + * Whether this browser can tell us what is on screen. + * + * Where it cannot — an old engine, or a test environment that stubs the DOM — + * every cover loads at once, which is exactly what this component did before. + * Degrading to the previous behaviour is the only safe direction: the other + * one leaves a page of empty squares that nothing will ever fill. + */ +const observable = typeof IntersectionObserver !== "undefined"; + +/** + * A cover, fetched when it is about to be seen. + * + * `/api/v2/artwork/{hash}` requires an `Authorization` header, so `` + * cannot fetch it and every thumbnail costs this client a `fetch` and an + * object URL of its own. Rendering an album page used to start all of them on + * mount — 127 requests in half a second, measured on a library of 164 tracks, + * against a connection limit of six. The covers below the fold were queued + * ahead of the ones being looked at, so the visible page filled last. + * + * Asking only for what is on screen does not make the requests cheaper; it + * stops making the ones nobody asked for. A ticket like the stream's would + * make `` work and let the HTTP cache serve between sessions, which + * is the deeper fix and is not this one. + */ export function Artwork({ artworkId, title, className = "" }: ArtworkProps) { const [src, setSrc] = useState(null); + const [wanted, setWanted] = useState(!observable); + + // A callback ref rather than a `useRef`: this component renders a different + // element depending on whether the image has arrived, so there is no one node + // to observe for its lifetime. React hands the new node here on every swap. + const observe = useCallback( + (node: HTMLElement | null) => { + if (!node || !observable || wanted) return; + const observer = new IntersectionObserver( + (entries) => { + if (entries.some((entry) => entry.isIntersecting)) { + setWanted(true); + observer.disconnect(); + } + }, + { rootMargin: AHEAD }, + ); + observer.observe(node); + return () => observer.disconnect(); + }, + [wanted], + ); + + // A new id is a new question, asked again when that cover is next on screen. + // Adjusted while rendering rather than in an effect: an effect would paint + // the previous album's cover once under the new one's title first. + const [asked, setAsked] = useState(artworkId); + if (asked !== artworkId) { + setAsked(artworkId); + setSrc(null); + setWanted(!observable); + } useEffect(() => { + if (!wanted) return; let cancelled = false; - setSrc(null); void artworkUrl(artworkId).then((url) => { if (!cancelled) setSrc(url); }); return () => { cancelled = true; }; - }, [artworkId]); + }, [artworkId, wanted]); const classes = `cover ${className}`.trim(); - if (src) return ; + if (src) return ; return ( -