From a3ede69530b3632fe8c0aaa3cf1b302f4710bd7d Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Tue, 15 Sep 2026 13:18:31 +0200 Subject: [PATCH 1/6] fix(webapp): read the lyrics envelope the server actually sends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `src/lyrics.rs` renames the whole `LyricsList` struct to camelCase, envelope included. `api.ts` declared `track_id` / `structured_lyrics`, so `value?.structuredLyrics[0]` — written as `structured_lyrics` — threw on the first property and took the whole "now playing" page down with it. The `StructuredLyrics` type beside it was already correct: the inside was converted and the envelope forgotten. Nothing caught it because the e2e mock was written from that type rather than from the server, so the suite confirmed the client's account of the wire instead of testing it. The mock now answers the server's shape, and it can carry real words: an empty sheet renders the same "no lyrics" line under either name, so only an assertion on the words themselves tells the two apart. Claude-Session: https://claude.ai/code/session_01HreLtK4rFopmncpZEHzp59 Signed-off-by: InstaZDLL --- webapp/e2e/studio-nocturne.spec.ts | 59 ++++++++++++++++++++++++++++-- webapp/src/api.ts | 13 ++++++- webapp/src/pages.tsx | 2 +- 3 files changed, 67 insertions(+), 7 deletions(-) diff --git a/webapp/e2e/studio-nocturne.spec.ts b/webapp/e2e/studio-nocturne.spec.ts index 6dba02ca..ae128154 100644 --- a/webapp/e2e/studio-nocturne.spec.ts +++ b/webapp/e2e/studio-nocturne.spec.ts @@ -274,6 +274,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 +561,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 +706,7 @@ test.beforeEach(async ({ page }) => { uploadCommits = []; uploadSession = freshUploadSession(); loseAcknowledgementOf = null; + lyricSheets = new Map(); canvases = new Map(); canvasWrites = []; refuseCanvasWith = null; @@ -1569,6 +1593,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, }) => { diff --git a/webapp/src/api.ts b/webapp/src/api.ts index f19d6e05..d85886b2 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) => diff --git a/webapp/src/pages.tsx b/webapp/src/pages.tsx index 07b1cd92..2b413a78 100644 --- a/webapp/src/pages.tsx +++ b/webapp/src/pages.tsx @@ -1168,7 +1168,7 @@ function Lyrics({ trackId }: { trackId: string }) { () => getLyrics(trackId), [trackId], ); - const sheet = value?.structured_lyrics[0]; + const sheet = value?.structuredLyrics[0]; const active = sheet?.synced ? currentLyricLine(sheet.line, progress.position) : -1; From 9b3d5f81fc1218f67d1297dd8a6bb5a826a4880b Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Tue, 15 Sep 2026 13:18:54 +0200 Subject: [PATCH 2/6] fix(api): send one shape on the scan event stream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/api/v2/scans/{scan_id}/events` opened with a `snapshot` carrying a `ScanJobRecord` and then sent a `progress` per step carrying the scanner's own `ScanProgress`. The two agree on ten fields out of thirteen and disagree on three: `id`/`scan_id`, `total_files`/`total`, `processed_files`/`processed`. A watcher that bound the snapshot therefore showed its totals as absent from the first progress frame on while its per-outcome counters kept working — which reads as a display bug rather than as two shapes on one stream. The web client showed "undefined files of undefined" above a correct "164 added". The stream converts through `ScanProgress::record` now, and `ScanProgress` is no longer `Serialize`: the other shape cannot come back, because there is no way left to put that type on a wire. `ScanProgress` also leaves the OpenAPI components, where it described nothing any route answers. 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 the new test pins, with thirteen distinct values. Claude-Session: https://claude.ai/code/session_01HreLtK4rFopmncpZEHzp59 Signed-off-by: InstaZDLL --- docs/api-v2-guide.md | 25 ++++++++++++- src/api/libraries.rs | 6 ++- src/lib.rs | 3 +- src/scanner.rs | 40 ++++++++++++++++++-- tests/scanner.rs | 88 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 154 insertions(+), 8 deletions(-) diff --git a/docs/api-v2-guide.md b/docs/api-v2-guide.md index 043a0682..ba10d03a 100644 --- a/docs/api-v2-guide.md +++ b/docs/api-v2-guide.md @@ -948,7 +948,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..f55bbd67 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -289,8 +289,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/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", + ] + ); +} From 186c37d7933ab64bbe3f19bb70619595e6480fc6 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Tue, 15 Sep 2026 13:19:09 +0200 Subject: [PATCH 3/6] fix(scrobbling): say why an instance is unavailable as a case, not a sentence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /api/v2/scrobble-destinations` published finished English prose in `unavailable`, and every client printed it verbatim — including this one, which ships in English and French. A French reader was told, in English, what to change in a configuration file. A reason only helps whoever can read it. What crosses the wire is the case now and the wording belongs to whoever is doing the telling: `no_application_configured` and `browser_journey_needs_https`, both naming something an operator can change. **This changes the API shape**: the field is still `unavailable` on an unavailable destination, and its value is no longer a sentence. An unrecognised case must not become a blank — a server is free to be newer than the client reading it — so the screen falls back to saying the server did not say why, the way it already treats a queue failure cause it has not been taught. The e2e test asserted the server's own sentence, copied into the spec, so it agreed with the server about a string neither should have been sending. It asserts the words this client chose now: asserting on the code would be satisfied by a page printing the code raw. Claude-Session: https://claude.ai/code/session_01HreLtK4rFopmncpZEHzp59 Signed-off-by: InstaZDLL --- docs/api-v2-guide.md | 17 ++++++++++++++++- src/lib.rs | 1 + src/services/lastfm.rs | 14 ++++++-------- src/services/mod.rs | 6 +++--- src/services/scrobbling.rs | 24 ++++++++++++++++++++++-- tests/scrobbling.rs | 13 ++++++++++--- webapp/e2e/studio-nocturne.spec.ts | 20 ++++++++++++++------ webapp/src/api.ts | 19 +++++++++++++++---- webapp/src/i18n.tsx | 10 ++++++++++ webapp/src/scrobbling-page.tsx | 27 ++++++++++++++++++++++++++- webapp/src/scrobbling.test.ts | 13 +++++++++---- webapp/src/scrobbling.ts | 3 ++- 12 files changed, 134 insertions(+), 33 deletions(-) diff --git a/docs/api-v2-guide.md b/docs/api-v2-guide.md index ba10d03a..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. diff --git a/src/lib.rs b/src/lib.rs index f55bbd67..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, 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/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 ae128154..6aef115d 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 }, @@ -1680,10 +1684,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( diff --git a/webapp/src/api.ts b/webapp/src/api.ts index d85886b2..5d7fa99d 100644 --- a/webapp/src/api.ts +++ b/webapp/src/api.ts @@ -1014,17 +1014,28 @@ export const scrobble = (trackId: string, submission: boolean) => */ export type ScrobbleProvider = "listenbrainz" | "maloja" | "lastfm"; +/** + * 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. + */ +export type ScrobbleUnavailable = + | "no_application_configured" + | "browser_journey_needs_https"; + /** 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/i18n.tsx b/webapp/src/i18n.tsx index b371ec1e..56b55e74 100644 --- a/webapp/src/i18n.tsx +++ b/webapp/src/i18n.tsx @@ -360,6 +360,11 @@ const en = { "scrobbling.provider.lastfm": "Last.fm", "scrobbling.instance": "instance “{name}”", "scrobbling.unavailable": "Unavailable here: {reason}", + "scrobbling.unavailable.no_application_configured": + "no Last.fm application is declared on this server", + "scrobbling.unavailable.browser_journey_needs_https": + "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", + "scrobbling.unavailable.unknown": "this server did not say why", "scrobbling.retired": "This server no longer offers this instance. Nothing more will leave for it — you can still withdraw the authorisation.", "scrobbling.notLinked": "Not linked.", @@ -780,6 +785,11 @@ const fr: Record = { "scrobbling.provider.lastfm": "Last.fm", "scrobbling.instance": "instance « {name} »", "scrobbling.unavailable": "Indisponible ici : {reason}", + "scrobbling.unavailable.no_application_configured": + "aucune application Last.fm n’est déclarée sur ce serveur", + "scrobbling.unavailable.browser_journey_needs_https": + "le parcours par le navigateur exige que l’adresse publique de ce serveur soit en https — un exploitant peut encore lier un compte en ligne de commande", + "scrobbling.unavailable.unknown": "ce serveur n’a pas dit pourquoi", "scrobbling.retired": "Ce serveur ne propose plus cette instance. Plus rien ne partira vers elle — vous pouvez encore retirer l’autorisation.", "scrobbling.notLinked": "Non liée.", diff --git a/webapp/src/scrobbling-page.tsx b/webapp/src/scrobbling-page.tsx index 9675532a..d31ed89d 100644 --- a/webapp/src/scrobbling-page.tsx +++ b/webapp/src/scrobbling-page.tsx @@ -12,6 +12,7 @@ import { retryUncertainScrobble, type ScrobbleLink, type ScrobbleProvider, + type ScrobbleUnavailable, type UncertainScrobble, unlinkScrobble, } from "./api"; @@ -78,6 +79,30 @@ const REASONS = new Set([ "interrupted", ]); +/** + * Why an instance cannot be linked, put into words. + * + * The server sent finished English prose here until 2026-09-15 and this page + * printed it, so a French reader was told in English what to change in a + * configuration file. It sends the case now and the wording is ours — which + * also means a case this client has not been taught must not become a blank: + * `unknown` says that much rather than nothing. + */ +const UNAVAILABLE_REASON = { + no_application_configured: "scrobbling.unavailable.no_application_configured", + browser_journey_needs_https: + "scrobbling.unavailable.browser_journey_needs_https", +} as const satisfies Record; + +function unavailableReason( + code: ScrobbleUnavailable | undefined, +): TranslationKey { + // Read through a wider type on purpose: the union above says what this build + // knows, and a server is free to be newer than the client reading it. + const known: Partial> = UNAVAILABLE_REASON; + return (code && known[code]) || "scrobbling.unavailable.unknown"; +} + function when(instant: number, locale: Locale): string { return new Intl.DateTimeFormat(locale, { dateStyle: "medium", @@ -241,7 +266,7 @@ function DestinationRow({ ) : ( {t("scrobbling.unavailable", { - reason: row.unavailable ?? "", + reason: t(unavailableReason(row.unavailable)), })} )} diff --git a/webapp/src/scrobbling.test.ts b/webapp/src/scrobbling.test.ts index 5a2d8aca..acabd0b6 100644 --- a/webapp/src/scrobbling.test.ts +++ b/webapp/src/scrobbling.test.ts @@ -1,6 +1,11 @@ import { describe, expect, it } from "vitest"; -import type { Play, ScrobbleDestination, ScrobbleLink } from "./api"; +import type { + Play, + ScrobbleDestination, + ScrobbleLink, + ScrobbleUnavailable, +} from "./api"; import { NAMED_UNCERTAIN, playsByInstant, @@ -13,7 +18,7 @@ function offered( provider: ScrobbleDestination["provider"], destination: string, available = true, - unavailable?: string, + unavailable?: ScrobbleUnavailable, ): ScrobbleDestination { return { provider, destination, available, unavailable }; } @@ -67,10 +72,10 @@ describe("scrobbleRows", () => { it("carries the reason an offered instance cannot be linked", () => { const rows = scrobbleRows( - [offered("lastfm", "default", false, "needs an https public URL")], + [offered("lastfm", "default", false, "browser_journey_needs_https")], [], ); - expect(rows[0].unavailable).toBe("needs an https public URL"); + expect(rows[0].unavailable).toBe("browser_journey_needs_https"); expect(rows[0].link).toBeNull(); }); diff --git a/webapp/src/scrobbling.ts b/webapp/src/scrobbling.ts index 45c98233..e7bb4ff7 100644 --- a/webapp/src/scrobbling.ts +++ b/webapp/src/scrobbling.ts @@ -3,6 +3,7 @@ import type { ScrobbleDestination, ScrobbleLink, ScrobbleProvider, + ScrobbleUnavailable, UncertainScrobble, } from "./api"; @@ -20,7 +21,7 @@ export type ScrobbleRow = { destination: string; /** `null` when the operator no longer declares this instance. */ available: boolean | null; - unavailable?: string; + unavailable?: ScrobbleUnavailable; link: ScrobbleLink | null; }; From 62bf3f90dc3510885fa55c8dce03b3429236c680 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Tue, 15 Sep 2026 13:19:26 +0200 Subject: [PATCH 4/6] perf(webapp): ask for a cover when it comes into view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/api/v2/artwork/{hash}` requires an `Authorization` header, so `` cannot fetch it and each thumbnail costs a `fetch` and an object URL of its own. Every `Artwork` started its own on mount: opening the album page of a 164-track library sent 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, so the visible page filled last. An `IntersectionObserver` with 300px of lead asks only for what is about to be seen. This does not make the requests cheaper; it stops making the ones nobody asked for. Where the browser has no observer — an old engine, a stubbed DOM — every cover loads at once, which is exactly the previous behaviour: the other direction would leave a page of empty squares nothing will ever fill. A ticket like the stream's would let `` work and the HTTP cache serve between sessions. That is the deeper fix and it is not this one. The test asserts the property rather than a count: a count depends on the viewport and would have to be loosened until it stopped meaning anything. The card sixty rows down is not asked for, and scrolling to it asks. No e2e fixture had ever carried an `artwork_hash`, so none of this was exercised at all. Claude-Session: https://claude.ai/code/session_01HreLtK4rFopmncpZEHzp59 Signed-off-by: InstaZDLL --- webapp/e2e/studio-nocturne.spec.ts | 53 ++++++++++++++++++++ webapp/src/artwork.tsx | 80 ++++++++++++++++++++++++++++-- 2 files changed, 128 insertions(+), 5 deletions(-) diff --git a/webapp/e2e/studio-nocturne.spec.ts b/webapp/e2e/studio-nocturne.spec.ts index 6aef115d..94d2e14b 100644 --- a/webapp/e2e/studio-nocturne.spec.ts +++ b/webapp/e2e/studio-nocturne.spec.ts @@ -829,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 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 ( -