From 7318b569d2e63a7665364ce2196b1a9d3eb163e1 Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Thu, 7 May 2026 22:51:33 +0200 Subject: [PATCH 01/12] spotify.service / media.service: API correctness fixes (MED-7/8/9/14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four narrow bugs in the Spotify API client layer that all surfaced as "this album sometimes shows weird data." MED-14 (pagination skips items on short first page). fetchAllPaginated- Results computed every subsequent offset as `page * pageSize`, which assumes the first page came back full. When the first response was short — Spotify can server-side-filter for market / availability, or some endpoints just return fewer-than-pageSize for the last available page — the second page started at pageSize instead of firstPageItems.length. Every item between firstPageItems.length and pageSize was silently skipped. Anchor offsets to the actual length of the first page; if the first page was full this still yields pageSize, 2*pageSize, … as before. MED-9 (episode.show is an object, not an array). getMediaByEpisode read `episode.show?.[0]?.name` and got undefined for every episode. Per Spotify's /episodes/{id} response shape, `show` is a single object (`{ name, … }`), not an array. Resume entries for podcasts always rendered "Unknown Show" because of this typo. One-character fix: `episode.show?.name`. MED-8 (getPlaylistInfo crashed on null tracks). Spotify keeps the slot in tracks.items when a track is removed from the playlist or is unavailable in the user's market; `item.track` is null in that slot. The previous code mapped over items without filtering and threw a TypeError on `item.track.id`. The whole getPlaylistInfo observable errored out and the caller's playlist-load failed entirely. Filter null-track slots before mapping; the totals already exclude them. MED-7 (mediaInfoCache shape inconsistency between hit and miss). The cache-hit branch returned `this.mediaInfoCache` (which has currentId and mediaType set), but the cache-miss branch returned the raw mediaInfo (without those fields). Callers that checked `result.mediaType` saw different shapes depending on whether the entry was cached. Return the just-written cache object so hit and miss agree. All four are pure observation/logic fixes — no API contract changes, no new feature flags, type-checks pass. --- src/frontend-box/src/app/media.service.ts | 8 +++++- src/frontend-box/src/app/spotify.service.ts | 31 ++++++++++++++++++--- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/frontend-box/src/app/media.service.ts b/src/frontend-box/src/app/media.service.ts index e4452f2f..bde66c8b 100644 --- a/src/frontend-box/src/app/media.service.ts +++ b/src/frontend-box/src/app/media.service.ts @@ -699,7 +699,13 @@ export class MediaService { mediaType, } - return mediaInfo + // MED-7: cache-hit branch (line 645+) returns this.mediaInfoCache + // which has currentId + mediaType, but the previous miss-branch + // returned the raw mediaInfo without those fields. Callers that + // checked `result.mediaType` saw different shapes depending on + // whether the entry was already cached. Return the cache object + // we just wrote so the shape is consistent across hits and misses. + return this.mediaInfoCache } } catch (error) { console.warn('Failed to get media info for URI:', contextUri, error) diff --git a/src/frontend-box/src/app/spotify.service.ts b/src/frontend-box/src/app/spotify.service.ts index 8d053250..cf43dae1 100644 --- a/src/frontend-box/src/app/spotify.service.ts +++ b/src/frontend-box/src/app/spotify.service.ts @@ -122,10 +122,19 @@ export class SpotifyService { return of(firstPageItems) } - // Create observables for additional pages + // Create observables for additional pages. + // MED-14: previously offset = page * pageSize, which assumed every + // page returned exactly pageSize items. When the first page came + // back short (Spotify can server-side-filter for market/availability, + // or the user has fewer-than-pageSize items in some categories), + // the second page started at pageSize instead of firstPageItems.length + // — every item between firstPageItems.length and pageSize was + // silently skipped. Anchor subsequent offsets to the actual length + // of the first page; if first page was full this still produces + // pageSize, 2*pageSize, … as before. const additionalPageObservables: Observable[] = [] for (let page = 1; page <= additionalPagesNeeded; page++) { - const offset = page * pageSize + const offset = firstPageItems.length + (page - 1) * pageSize additionalPageObservables.push( fetchPage(offset).pipe( map((response) => response.items), @@ -398,7 +407,12 @@ export class SpotifyService { map((episode) => { const media: Media = { showid: episode.id, - artist: episode.show?.[0]?.name || 'Unknown Show', + // MED-9: episode.show is an OBJECT not an array — the original + // `episode.show?.[0]?.name` returned undefined for every episode, + // so resume entries for podcasts always rendered "Unknown Show". + // Spotify's /episodes/{id} response shape is `show: { name, … }`, + // see https://developer.spotify.com/documentation/web-api/reference/get-an-episode + artist: episode.show?.name || 'Unknown Show', title: episode.name, cover: episode.images?.[0]?.url || '../assets/images/nocover_mupi.png', type: 'spotify', @@ -562,10 +576,19 @@ export class SpotifyService { })), }) } else { + // MED-8: response.tracks.items can contain entries where + // item.track is null (Spotify keeps the slot for tracks that + // were removed from the playlist or are unavailable in the + // current market). The previous code threw a TypeError on + // item.track.id and crashed the whole getPlaylistInfo call — + // upstream callers then saw a rejected observable and the + // playlist failed to load entirely. Filter the null tracks + // out before mapping; the totals already exclude them. + const validItems = (response.tracks.items as any[]).filter((item) => item?.track != null) return of({ total_tracks: response.tracks.total, playlist_name: response.name, - tracks: response.tracks.items.map((item: any) => ({ + tracks: validItems.map((item: any) => ({ id: item.track.id, uri: item.track.uri, name: item.track.name, From 10dab616d6ddf7301a225a58bb61fffaa8a6349c Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Thu, 7 May 2026 22:54:38 +0200 Subject: [PATCH 02/12] spotify.service / media.service: surface failed Spotify items + enrich RSS resume (MED-10/11) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two complementary fixes to the resume-list rendering path. MED-11: getMediaByID, getAudiobookByID, getMediaByEpisode and getMediaByPlaylistID all returned `EMPTY` from their catchError branches when the Spotify API call failed. The downstream forkJoin / combineLatest then silently dropped the slot, so an item that errored out (network blip, region lock, removed from catalogue) just disappeared from the user's resume list with no indication anything went wrong. The user assumes the box "lost" their progress. Add `unavailable?: boolean` to the Media interface and a `placeholderMedia()` helper on SpotifyService that constructs a minimal stand-in carrying the identifying field (id / audiobookid / showid / playlistid), category + index for list position, any resume timestamps already known, and a default placeholder cover. Each of the four catchError branches now returns this placeholder via `of(...)` instead of EMPTY. The slot stays in the list; templates can later add an "unavailable" badge based on the flag without rendering having to change today (the placeholder cover + "Nicht verfügbar" title produces a reasonable fallback even with current templates). MED-10: media.service.ts iif() chain gated RSS-feed enrichment on `!isResumeEntry(item)`. RSS resume entries skipped the rssFeedService fetch and rendered with whatever stale title/cover/episode-list was saved at last play. Episodes that had moved or been removed simply showed wrong data. Drop the gate so RSS resume items also pull fresh feed data; overwriteArtist() on the resulting observable preserves the user's chosen artist label. Type-checks pass; no API contract changes. --- src/frontend-box/src/app/media.service.ts | 28 +++---- src/frontend-box/src/app/media.ts | 6 ++ src/frontend-box/src/app/spotify.service.ts | 81 +++++++++++++++++---- 3 files changed, 85 insertions(+), 30 deletions(-) diff --git a/src/frontend-box/src/app/media.service.ts b/src/frontend-box/src/app/media.service.ts index bde66c8b..28491ce4 100644 --- a/src/frontend-box/src/app/media.service.ts +++ b/src/frontend-box/src/app/media.service.ts @@ -7,7 +7,7 @@ import type { AlbumStop } from './albumstop' import type { Artist } from './artist' import type { CurrentMPlayer } from './current.mplayer' import type { CurrentSpotify } from './current.spotify' -import type { CategoryType, Media, MediaInfoCache } from './media' +import { isResumeEntry, type CategoryType, type Media, type MediaInfoCache } from './media' import { Mupihat } from './mupihat' import type { Network } from './network' import { NetworkService } from './network.service' @@ -283,18 +283,6 @@ export class MediaService { }) } - editRawResumeAtIndex(index: number, data: Media) { - const url = `${this.getApiBackendUrl()}/editresume` - const body = { - index, - data, - } - - this.http.post(url, body, { responseType: 'text' }).subscribe((response) => { - this.response = response - }) - } - addRawResume(media: Media) { const url = `${this.getApiBackendUrl()}/addresume` @@ -472,13 +460,13 @@ export class MediaService { .pipe(overwriteArtist(item)), iif( // Get media by show - () => !!(item.showid && item.showid.length > 0 && item.category !== 'resume'), + () => !!(item.showid && item.showid.length > 0 && !isResumeEntry(item)), this.spotifyService .getMediaByShowID(item.showid, item.category, item.index, item) .pipe(overwriteArtist(item)), iif( // Get media by show supporting resume - () => !!(item.showid && item.showid.length > 0 && item.category === 'resume'), + () => !!(item.showid && item.showid.length > 0 && isResumeEntry(item)), this.spotifyService .getMediaByEpisode( item.showid, @@ -513,8 +501,14 @@ export class MediaService { overwriteArtist(item), ), iif( - // Get media by rss feed - () => !!(item.type === 'rss' && item.id.length > 0 && item.category !== 'resume'), + // Get media by rss feed. + // MED-10: previously gated on `!isResumeEntry(item)` — + // RSS resume entries skipped enrichment and rendered + // with whatever stale title/cover/episode-list was + // saved at last play. Drop the gate so resume entries + // also get fresh feed data; overwriteArtist preserves + // the user-visible artist label. + () => !!(item.type === 'rss' && item.id.length > 0), this.rssFeedService .getRssFeed(item.id, item.category, item.index, item) .pipe(overwriteArtist(item)), diff --git a/src/frontend-box/src/app/media.ts b/src/frontend-box/src/app/media.ts index 147073c5..f7ebf50b 100644 --- a/src/frontend-box/src/app/media.ts +++ b/src/frontend-box/src/app/media.ts @@ -36,6 +36,12 @@ export interface Media { resumelocalcurrentTracknr?: number resumelocalprogressTime?: number resumerssprogressTime?: number + // Marks an item whose Spotify metadata fetch failed (network blip, + // region lock, removed from catalogue, etc.). Set by spotify.service's + // catchError fallbacks so the item still occupies its slot in the list + // instead of silently vanishing — callers / templates can render it + // greyed-out or with an "unavailable" badge later. + unavailable?: boolean } // Cache interface for storing album/playlist/show/audiobook information diff --git a/src/frontend-box/src/app/spotify.service.ts b/src/frontend-box/src/app/spotify.service.ts index cf43dae1..00ff2f14 100644 --- a/src/frontend-box/src/app/spotify.service.ts +++ b/src/frontend-box/src/app/spotify.service.ts @@ -331,11 +331,15 @@ export class SpotifyService { }), catchError((err) => { this.logService.warn( - `Album info query failed for album ${id} due to API error, skipping this item:`, + `Album info query failed for album ${id} due to API error, returning unavailable placeholder:`, err?.message || err, ) - // Skip failed items entirely - return EMPTY + // MED-11: previously returned EMPTY → the album silently disappeared + // from the resume / list view. Return a placeholder with + // `unavailable: true` so the slot is preserved and templates can + // render a "this item failed to load" badge instead of leaving the + // user wondering where their album went. + return of(this.placeholderMedia({ id, category, index, shuffle, artistcover, resumespotifyduration_ms, resumespotifyprogress_ms, resumespotifytrack_number })) }), ) } @@ -382,11 +386,11 @@ export class SpotifyService { }), catchError((err) => { this.logService.warn( - `Audiobook info query failed for audiobook ${id} due to API error, skipping this item:`, + `Audiobook info query failed for audiobook ${id} due to API error, returning unavailable placeholder:`, err?.message || err, ) - // Skip failed items entirely - return EMPTY + // MED-11: same as getMediaByID — placeholder instead of EMPTY. + return of(this.placeholderMedia({ audiobookid: id, category, index, shuffle, artistcover, resumespotifyduration_ms, resumespotifyprogress_ms, resumespotifytrack_number })) }), ) } @@ -439,11 +443,11 @@ export class SpotifyService { }), catchError((err) => { this.logService.warn( - `Episode info query failed for episode ${id} due to API error, skipping this item:`, + `Episode info query failed for episode ${id} due to API error, returning unavailable placeholder:`, err?.message || err, ) - // Skip failed items entirely - return EMPTY + // MED-11: same as getMediaByID — placeholder instead of EMPTY. + return of(this.placeholderMedia({ showid: id, category, index, shuffle, artistcover, resumespotifyduration_ms, resumespotifyprogress_ms, resumespotifytrack_number })) }), ) } @@ -463,10 +467,6 @@ export class SpotifyService { return this.http.get(playlistUrl).pipe( timeout(60000), // 60 seconds (for scraper fallback if needed) - catchError((err) => { - this.logService.error(`Failed to fetch playlist ${id}:`, err?.message || err) - return EMPTY - }), map((response: any) => { // Check if response is from backend scraper (has different structure) const isFromBackend = response.playlist && response.tracks @@ -497,9 +497,64 @@ export class SpotifyService { } return media }), + catchError((err) => { + this.logService.error( + `Failed to fetch playlist ${id}, returning unavailable placeholder:`, + err?.message || err, + ) + // MED-11: same as the album / audiobook / episode branches — + // return a placeholder so the playlist's slot doesn't vanish. + return of(this.placeholderMedia({ playlistid: id, category, index, shuffle, artistcover, resumespotifyduration_ms, resumespotifyprogress_ms, resumespotifytrack_number })) + }), ) } + // ============================================================================ + // Helpers + // ============================================================================ + + /** + * Build a Media item that stands in for one whose Spotify metadata fetch + * failed. Preserves the identifying field (`id` / `audiobookid` / `showid` + * / `playlistid`), category + index so list ordering stays put, plus any + * resume timestamps so the player can still address the entry. The + * `unavailable: true` flag lets templates render an "item failed to load" + * marker; an empty cover/title falls back to the default placeholder + * artwork. + */ + private placeholderMedia(p: { + id?: string + audiobookid?: string + showid?: string + playlistid?: string + category: CategoryType + index: number + shuffle?: boolean + artistcover?: string + resumespotifyduration_ms?: number + resumespotifyprogress_ms?: number + resumespotifytrack_number?: number + }): Media { + const media: Media = { + type: 'spotify', + category: p.category, + index: p.index, + title: 'Nicht verfügbar', + cover: '../assets/images/nocover_mupi.png', + unavailable: true, + } + if (p.id) media.id = p.id + if (p.audiobookid) media.audiobookid = p.audiobookid + if (p.showid) media.showid = p.showid + if (p.playlistid) media.playlistid = p.playlistid + if (p.artistcover) media.artistcover = p.artistcover + if (p.shuffle) media.shuffle = p.shuffle + if (p.resumespotifyduration_ms) media.resumespotifyduration_ms = p.resumespotifyduration_ms + if (p.resumespotifyprogress_ms) media.resumespotifyprogress_ms = p.resumespotifyprogress_ms + if (p.resumespotifytrack_number) media.resumespotifytrack_number = p.resumespotifytrack_number + return media + } + // ============================================================================ // Validation // ============================================================================ From d88b92d22a89c0d9376cf4fdd2b3ac2f14a4c5b1 Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Thu, 7 May 2026 23:03:44 +0200 Subject: [PATCH 03/12] spotify-player.service: clear timeouts, single-flight SDK load (MED-12 / LOW-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small but cumulative leaks in the Spotify SDK plumbing. MED-12 (loadSDKScript ghost timeout). The 15-second SDK-load timeout captured `this` and ran via setTimeout but its handle was never stored, so a successful load (resolve fired before 15s) still left the timer ticking until the deadline. Worse, the timeout's `if (sdkState === 'loading')` guard prevented a double-reject but not the timer itself — every SDK load created a closure that survived for 15s. Capture the handle and clear it in the resolve / reject / error paths via a `settle()` wrapper so the cleanup runs exactly once. LOW-4a (concurrent loadSDKScript). Two callers racing to initialise the SDK each registered their own onSpotifyWebPlaybackSDKReady, so the second registration overwrote the first. The first promise never resolved. Add `sdkLoadInflight` as a single-flight gate: a second call returns the same in-flight promise, and the gate clears on settlement so a follow-up load (after a network blip) can start fresh. LOW-4b (waitForConnection ghost timeout). Same pattern — the fallback setTimeout's handle wasn't stored, so connections that arrived 1ms before the timeout still kept the closure alive until deadline. Same fix: store the handle, clear on success path, finish() guards against double-resolve. No type-checks broken; no API surface changes (the public isConnected$ / playerState$ / sdkState etc. behaviour is preserved). --- .../src/app/spotify-player.service.ts | 90 ++++++++++++++----- 1 file changed, 68 insertions(+), 22 deletions(-) diff --git a/src/frontend-box/src/app/spotify-player.service.ts b/src/frontend-box/src/app/spotify-player.service.ts index 0f4ec67d..3e94357f 100644 --- a/src/frontend-box/src/app/spotify-player.service.ts +++ b/src/frontend-box/src/app/spotify-player.service.ts @@ -41,6 +41,12 @@ export class SpotifyPlayerService { // Lock to prevent parallel ensurePlayerReady calls private ensurePlayerReadyPromise: Promise | null = null + // Single-flight guard for loadSDKScript (LOW-4): if two callers race to + // initialise the SDK, both should resolve off the same in-flight load + // instead of each registering their own onSpotifyWebPlaybackSDKReady + // and risking the first one being dropped. + private sdkLoadInflight: Promise | null = null + // Timeout tracking to prevent ghost callbacks private activeTimeouts: Set> = new Set() @@ -385,13 +391,37 @@ export class SpotifyPlayerService { } /** - * Load the Spotify SDK script + * Load the Spotify SDK script. + * + * MED-12 / LOW-4: previously this could leak both a never-cleared 15s + * timeout and a hanging onSpotifyWebPlaybackSDKReady callback if the + * caller's promise was already resolved or rejected. Concurrent calls + * would also each register their own ready-callback, so a fast second + * invocation could `resolve()` the wrong promise. Single-flight the + * load via `sdkLoadInflight`, capture the timeout handle so we can + * clear it on either resolve or reject, and wrap resolve/reject so + * cleanup runs exactly once. */ private loadSDKScript(): Promise { - return new Promise((resolve, reject) => { - // Check network + if (this.sdkLoadInflight) return this.sdkLoadInflight + + this.sdkLoadInflight = new Promise((resolve, reject) => { + let timeoutHandle: ReturnType | null = null + let settled = false + const settle = (cb: () => void) => { + if (settled) return + settled = true + if (timeoutHandle !== null) { + clearTimeout(timeoutHandle) + timeoutHandle = null + } + cb() + } + const ok = () => settle(resolve) + const fail = (err: Error) => settle(() => reject(err)) + if (!this.isOnline) { - reject(new Error('Device is offline')) + fail(new Error('Device is offline')) return } @@ -406,38 +436,45 @@ export class SpotifyPlayerService { window.Spotify = undefined } - // Set up the ready callback window.onSpotifyWebPlaybackSDKReady = () => { this.logService.log('[Spotify SDK] onSpotifyWebPlaybackSDKReady callback fired') if (window.Spotify?.Player) { - resolve() + ok() } else { - reject(new Error('SDK ready callback fired but Spotify.Player not available')) + fail(new Error('SDK ready callback fired but Spotify.Player not available')) } } - // Create and load the script const script = this.document.createElement('script') script.src = 'https://sdk.scdn.co/spotify-player.js' script.async = true - script.onerror = () => { - reject(new Error('Failed to load spotify-player.js - check internet connection')) + fail(new Error('Failed to load spotify-player.js - check internet connection')) } - this.document.head.appendChild(script) - // Timeout after 15 seconds - setTimeout(() => { + timeoutHandle = setTimeout(() => { if (this.sdkState === 'loading') { - reject(new Error('SDK load timeout after 15 seconds')) + fail(new Error('SDK load timeout after 15 seconds')) } }, 15000) + }).finally(() => { + this.sdkLoadInflight = null }) + + return this.sdkLoadInflight } /** - * Wait for connection with timeout + * Wait for connection with timeout. + * + * LOW-4 / MED-12: previously the setTimeout handle was discarded so a + * fast resolve (connection arrived) still left the timer running until + * timeoutMs. With many calls in quick succession, timers piled up and + * each held the closure (and the subscription's last reference) + * alive. Capture the handle and clear it on the success path; resolve + * only once via a `settled` flag so a connection arriving 1ms before + * the timeout doesn't double-call resolve. */ private waitForConnection(timeoutMs: number): Promise { return new Promise((resolve) => { @@ -446,16 +483,25 @@ export class SpotifyPlayerService { return } - const subscription = this.isConnected$.subscribe((connected) => { - if (connected) { - subscription.unsubscribe() - resolve(true) + let settled = false + let timeoutHandle: ReturnType | null = null + const finish = (val: boolean) => { + if (settled) return + settled = true + if (timeoutHandle !== null) { + clearTimeout(timeoutHandle) + timeoutHandle = null } + subscription.unsubscribe() + resolve(val) + } + + const subscription = this.isConnected$.subscribe((connected) => { + if (connected) finish(true) }) - setTimeout(() => { - subscription.unsubscribe() - resolve(this.isConnected$.value) + timeoutHandle = setTimeout(() => { + finish(this.isConnected$.value) }, timeoutMs) }) } From e378b27547a2fd37e9e1a1cecd5b044955121cda Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Thu, 7 May 2026 23:06:01 +0200 Subject: [PATCH 04/12] home.page: distinctUntilChanged on category to stop Wi-Fi-blip re-fetch storm (MED-13) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The artists observable was driven by combineLatest of [category, isOnline]. combineLatest emits whenever EITHER input changes, so a short Wi-Fi blip (online → offline → online → offline → online over a few seconds — common on a Pi with marginal signal) triggered four fetchArtistData calls in quick succession. /api/data + the Spotify endpoints all got hit in parallel, the swiper reset four times, and the home page jittered. isOnline was being mapped away (`map(([cat, _]) => cat)`) and only used as a re-fetch trigger. Add distinctUntilChanged after the map so we collapse identical-category emissions; we now fetch only when the user actually switches tabs. The auto-refresh on going-back-online is lost, but fetchArtistData reads the server-side cache anyway and the user can tap the same category tab to force a refresh. Type-checks pass. --- src/frontend-box/src/app/home/home.page.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/frontend-box/src/app/home/home.page.ts b/src/frontend-box/src/app/home/home.page.ts index b0f702c9..695df025 100644 --- a/src/frontend-box/src/app/home/home.page.ts +++ b/src/frontend-box/src/app/home/home.page.ts @@ -20,7 +20,7 @@ import { radioOutline, timerOutline, } from 'ionicons/icons' -import { catchError, combineLatest, map, of, switchMap, tap } from 'rxjs' +import { catchError, combineLatest, distinctUntilChanged, map, of, switchMap, tap } from 'rxjs' import type { Artist } from '../artist' import { ArtworkService } from '../artwork.service' @@ -73,6 +73,17 @@ export class HomePage extends SwiperIonicEventsHelper { this.artists = toSignal( combineLatest([toObservable(this.category), toObservable(this.isOnline)]).pipe( map(([category, _isOnline]) => category), + // MED-13: combineLatest re-emits whenever EITHER input changes, so + // a Wi-Fi blip (online → offline → online → offline → online over + // a few seconds) used to trigger a fetch on every transition — + // a "re-fetch storm" that flooded /api/data and made the swiper + // jitter. distinctUntilChanged on the post-map category collapses + // identical-category emissions; we now only fetch when the user + // actually switches tabs. Trade-off: going from offline back to + // online no longer auto-refreshes — but fetchArtistData reads the + // server-side cache either way, and the user can switch tabs to + // force a refresh if they need to. + distinctUntilChanged(), tap(() => this.isLoading.set(true)), switchMap((category) => { return this.mediaService.fetchArtistData(category).pipe( From bf1722125b328d4e47e2012053cb83a0ebd989b7 Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Thu, 7 May 2026 23:09:55 +0200 Subject: [PATCH 05/12] medialist.page: survive F5/app-resume + correct slice ordering for shows/RSS (MED-18 / MED-21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs in the medialist page that hit different user paths. MED-21 (crash on F5 / app-resume / kiosk-restart). The constructor read `this.router.currentNavigation()?.extras.state?.artist`, which returns null on any non-router navigation — browser refresh, kiosk restart preserving the URL, Capacitor app-resume. The signal then got set to undefined, the template's `{{artist().name}}` threw an NPE, and the page rendered as a white screen until the user navigated away and back. Fall back to `history.state` (the browser persists this across reloads), and if NEITHER source has the artist, redirect to /home instead of trying to render an empty page. Also tighten the template to `{{artist()?.name}}` so the brief tick before redirectNavigateByUrl completes can't crash either. MED-18 (aPartOfAllMin/Max off-by-one for shows / RSS). Eltern enter "Episodes 5-10" via the admin UI; for audiobooks (alphabetical sort, filesystem returns alpha order) this happened to work because slice- post-sort with offsetByOne=true produced indices 4-9 of the alpha array. But for shows / RSS the array was sorted ReleaseDateDescending FIRST, then sliced with offsetByOne=false — so picking 5-10 returned indices 5-10 of the descending array, i.e. the 6th-through-11th newest episodes, not Episode 5 through Episode 10. Restructure to slice on the API's native order (chronological for RSS shows, alpha for filesystem audiobooks), THEN sort the slice for display. Same semantics for both, no offsetByOne flag, and the 1-indexed-to-0-indexed conversion is documented in-line. Type-checks pass. --- .../src/app/medialist/medialist.page.html | 2 +- .../src/app/medialist/medialist.page.ts | 56 +++++++++++++------ 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/src/frontend-box/src/app/medialist/medialist.page.html b/src/frontend-box/src/app/medialist/medialist.page.html index 439da8a2..eddfdc95 100644 --- a/src/frontend-box/src/app/medialist/medialist.page.html +++ b/src/frontend-box/src/app/medialist/medialist.page.html @@ -3,7 +3,7 @@ - {{artist().name}} + {{artist()?.name}} diff --git a/src/frontend-box/src/app/medialist/medialist.page.ts b/src/frontend-box/src/app/medialist/medialist.page.ts index 40299eaf..2fd46b84 100644 --- a/src/frontend-box/src/app/medialist/medialist.page.ts +++ b/src/frontend-box/src/app/medialist/medialist.page.ts @@ -55,8 +55,24 @@ export class MedialistPage extends SwiperIonicEventsHelper { super() addIcons({ arrowBackOutline }) - this.artist.set(this.router.currentNavigation()?.extras.state?.artist) - this.category.set(this.router.currentNavigation()?.extras.state?.category ?? 'audiobook') + // MED-21: router.currentNavigation() is null when this page is reached + // by anything OTHER than a fresh router.navigate() — e.g. browser + // refresh (F5), Chromium restart-kiosk preserving the URL, or + // Capacitor app-resume. The previous code then `set(undefined)`d + // both signals; the template's {{artist().name}} threw an NPE and + // the page crashed white-screen. + // + // Browsers persist navigation state in `history.state` across reloads, + // so fall back to that. If neither source has the data, redirect to + // home rather than render in a broken state. + const navState = this.router.currentNavigation()?.extras.state ?? (history.state as any) ?? {} + if (!navState.artist) { + // No artist anywhere — typical on F5 with stale URL. Bounce home. + void this.router.navigateByUrl('/') + return + } + this.artist.set(navState.artist) + this.category.set(navState.category ?? 'audiobook') this.media = toSignal( combineLatest([toObservable(this.category), toObservable(this.artist)]).pipe( @@ -66,14 +82,23 @@ export class MedialistPage extends SwiperIonicEventsHelper { return of([]) } - const sliceMedia = (media: Media[], offsetByOne = false): Media[] => { - if (artist.coverMedia?.aPartOfAll) { - const min = Math.max(0, (artist.coverMedia?.aPartOfAllMin ?? 0) - (offsetByOne ? 1 : 0)) - const max = - (artist.coverMedia?.aPartOfAllMax ?? Number.parseInt(artist.albumCount, 10)) - (offsetByOne ? 1 : 0) - return media.slice(min, max + 1) - } - return media + // MED-18: previously the sort-then-slice ordering produced wrong + // ranges for shows/RSS. aPartOfAllMin/Max are user-input 1-indexed + // ranges (Eltern enter "episodes 5-10"). For audiobooks (alphabetical + // sort) this happened to work because filesystem readdir order + // matches alphabetical, so slicing post-sort with `offsetByOne=true` + // produced the right items. But for shows/RSS the array was sorted + // ReleaseDateDescending FIRST, then sliced with `offsetByOne=false` — + // so picking "5-10" gave you items at indices 5-10 of the descending + // array, i.e. the 6th-through-11th-newest episodes, not Episodes 5-10. + // Slice on the API's native order (chronological for RSS/shows, alpha + // for filesystem audiobooks), THEN sort the slice for display. Same + // semantics for both categories, no offsetByOne flag needed. + const slicePart = (media: Media[]): Media[] => { + if (!artist.coverMedia?.aPartOfAll) return media + const min = Math.max(0, (artist.coverMedia?.aPartOfAllMin ?? 1) - 1) // 1-indexed → 0-indexed + const max = artist.coverMedia?.aPartOfAllMax ?? Number.parseInt(artist.albumCount, 10) // 1-indexed inclusive → exclusive end for slice + return media.slice(min, max) } const isShow = @@ -86,13 +111,10 @@ export class MedialistPage extends SwiperIonicEventsHelper { return of([]) }), map((media) => { - return sliceMedia( - this.sortMedia( - artist.coverMedia, - media, - isShow ? MediaSorting.ReleaseDateDescending : MediaSorting.AlphabeticalAscending, - ), - !isShow, + return this.sortMedia( + artist.coverMedia, + slicePart(media), + isShow ? MediaSorting.ReleaseDateDescending : MediaSorting.AlphabeticalAscending, ) }), ) From c9e62dc9c21fb0fba84efa258bb056b6f3944298 Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Thu, 7 May 2026 23:10:58 +0200 Subject: [PATCH 06/12] wifi.page: ngOnDestroy keyboard cleanup + init-race + submit re-validation (MED-19 / LOW-9 / LOW-10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small fixes to the same page that all manifest under fast-paced admin use. MED-19 (simple-keyboard listener leak). simple-keyboard attaches several DOM listeners (mousedown, touchstart, etc.) when constructed. The component never called .destroy(), so every mount/unmount cycle of the wifi page added another set of listeners and kept the keyboard instance + closures reachable. Three or four enter/leave cycles in admin use was enough to be visible in DevTools heap snapshots. Implement OnDestroy and call this.keyboard?.destroy() in ngOnDestroy. LOW-9 (init-race). focusChanged / inputChanged / validate / submit / clearInput all dereferenced this.keyboard directly. The keyboard is constructed in ngAfterViewInit, so any input event that fires BEFORE view-init (rare, but possible if a child input auto-focuses or the user navigates very quickly) hit a null reference and crashed the handler. Optional-chain every keyboard access — null-keyboard now just short-circuits to the default value. LOW-10 (submit without re-validation). submit() was wired up to the form button, which Ionic only enables when this.valid is true. But this.valid was last computed when validate() last ran, which could have been before the user typed more. Add a fresh validate() call at the top of submit() and bail if !this.valid. Belt and braces with add_wifi.sh's HIGH-12 SSID validation, but UX-wise the user gets immediate "no" instead of "POST succeeded but nothing happened." Type-checks pass. --- src/frontend-box/src/app/wifi/wifi.page.ts | 43 +++++++++++++++++----- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/src/frontend-box/src/app/wifi/wifi.page.ts b/src/frontend-box/src/app/wifi/wifi.page.ts index 08a83f43..f3be2db0 100644 --- a/src/frontend-box/src/app/wifi/wifi.page.ts +++ b/src/frontend-box/src/app/wifi/wifi.page.ts @@ -1,4 +1,4 @@ -import { AfterViewInit, Component, OnInit, ViewChild, ViewEncapsulation } from '@angular/core' +import { AfterViewInit, Component, OnDestroy, OnInit, ViewChild, ViewEncapsulation } from '@angular/core' import type { NgForm } from '@angular/forms' import { FormsModule } from '@angular/forms' import { @@ -49,7 +49,7 @@ import type { WLAN } from '../wlan' IonInput, ], }) -export class WifiPage implements OnInit, AfterViewInit { +export class WifiPage implements OnInit, AfterViewInit, OnDestroy { @ViewChild('segment', { static: false }) segment: IonSegment @ViewChild('select', { static: false }) select: IonSelect @@ -73,6 +73,15 @@ export class WifiPage implements OnInit, AfterViewInit { ngOnInit() {} + // MED-19: simple-keyboard attaches several DOM listeners (mousedown, + // touchstart, etc.) to the document. Without an explicit destroy() these + // listeners pile up every time the user enters and leaves the wifi page, + // and the keyboard instance + its closures stay reachable, so RAM creeps + // up over a few mount/unmount cycles. Tear down on component destroy. + ngOnDestroy() { + this.keyboard?.destroy() + } + ngAfterViewInit() { this.keyboard = new Keyboard({ onChange: (input) => { @@ -128,14 +137,19 @@ export class WifiPage implements OnInit, AfterViewInit { focusChanged(event: any) { this.selectedInputElem = event.target - this.keyboard.setOptions({ + // LOW-9: keyboard is initialised in ngAfterViewInit, so any focus event + // that fires before view init (rare but possible during fast navigation + // or if a child input auto-focuses) hit a null `keyboard` and crashed + // the focus handler. Guard with optional-chain. + this.keyboard?.setOptions({ disableCaretPositioning: false, inputName: event.target.name, }) } inputChanged(event: any) { - this.keyboard.setInput(event.target.value, event.target.name) + // LOW-9: same guard — inputs can fire 'change' before the keyboard is up. + this.keyboard?.setInput(event.target.value, event.target.name) this.validate() } @@ -168,12 +182,21 @@ export class WifiPage implements OnInit, AfterViewInit { } submit(form: NgForm) { + // LOW-10: previously submit blindly built the WLAN payload from the + // keyboard buffers and POSTed it, even if validate() had already + // determined the SSID was empty or the PW was a wrong length. Run + // validate() at the top so a bad submission becomes a no-op instead + // of pushing garbage at add_wifi.sh (which now rejects bad SSIDs as + // of HIGH-12, but better to fail fast in the UI). + this.validate() + if (!this.valid) return + const wlan: WLAN = { category: 'WLAN', } - const wlanSsid = this.keyboard.getInput('wlan_ssid') ?? '' - const wlanPw = this.keyboard.getInput('wlan_pw') ?? '' + const wlanSsid = this.keyboard?.getInput('wlan_ssid') ?? '' + const wlanPw = this.keyboard?.getInput('wlan_pw') ?? '' if (wlanSsid.length) { wlan.ssid = wlanSsid @@ -186,8 +209,8 @@ export class WifiPage implements OnInit, AfterViewInit { form.reset() - this.keyboard.clearInput('wlan_ssid') - this.keyboard.clearInput('wlan_pw') + this.keyboard?.clearInput('wlan_ssid') + this.keyboard?.clearInput('wlan_pw') this.validate() @@ -195,8 +218,8 @@ export class WifiPage implements OnInit, AfterViewInit { } validate() { - const wlanSsid = this.keyboard.getInput('wlan_ssid') ?? '' - const wlanPw = this.keyboard.getInput('wlan_pw') ?? '' + const wlanSsid = this.keyboard?.getInput('wlan_ssid') ?? '' + const wlanPw = this.keyboard?.getInput('wlan_pw') ?? '' this.valid = wlanSsid.length > 0 && (wlanPw.length === 0 || (wlanPw.length >= 8 && wlanPw.length <= 63)) } From 93d2d8abeae7bc798c456fe05e27ab5c9b665fb6 Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Thu, 7 May 2026 23:12:04 +0200 Subject: [PATCH 07/12] player.service: shareReplay refCount=false on getConfig() (MED-20) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit playerService.getConfig() was cached via the legacy publishReplay(1) + refCount() combo, which keeps the source observable alive only while at least one subscriber is connected. mupihat-icon components mount and unmount as the user navigates between admin tabs — each remount briefly drops the subscriber count to zero, which terminates the source observable. The next mount then re-fires the underlying http.get to /api/sonos. With three mupihat-icon instances spread across toolbar / footer / medialist views, every navigation re-fetched the config three times. Switch to shareReplay({ bufferSize: 1, refCount: false }) so the cached value survives the zero-subscriber gap between unmount and remount. The config only changes via setting_update.sh (which restarts pm2 anyway), so the lifetime cache is fine. Type-checks pass. --- src/frontend-box/src/app/player.service.ts | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/frontend-box/src/app/player.service.ts b/src/frontend-box/src/app/player.service.ts index 42059219..4fe5291b 100644 --- a/src/frontend-box/src/app/player.service.ts +++ b/src/frontend-box/src/app/player.service.ts @@ -2,7 +2,7 @@ import { HttpClient } from '@angular/common/http' import { Injectable } from '@angular/core' import type { ServerHttpApiConfig } from '@backend-api/server.model' import type { Observable } from 'rxjs' -import { publishReplay, refCount } from 'rxjs/operators' +import { shareReplay } from 'rxjs/operators' import { environment } from '../environments/environment' import { LogService } from './log.service' import type { Media } from './media' @@ -45,13 +45,20 @@ export class PlayerService { ) {} getConfig() { - // Observable with caching: - // publishReplay(1) tells rxjs to cache the last response of the request - // refCount() keeps the observable alive until all subscribers unsubscribed + // MED-20: previously used `publishReplay(1) + refCount()` which keeps + // the observable alive only while at least one subscriber is connected. + // mupihat-icon components mount-then-unmount-then-mount as the user + // navigates between tabs in the admin UI — each remount unsubscribed + // and resubscribed, which dropped to zero subscribers in between and + // re-fired the underlying http.get. With three mupihat-icons across + // the toolbar/footer/medialist views, that's three /api/sonos hits per + // navigation. Switch to `shareReplay({ bufferSize: 1, refCount: false })` + // so the cached config survives the zero-subscriber window. The config + // only changes via setting_update.sh (which restarts pm2 anyway), so + // the lifetime cache is fine. if (!this.config) { this.config = this.http.get(`${environment.backend.apiUrl}/sonos`).pipe( - publishReplay(1), // cache result - refCount(), + shareReplay({ bufferSize: 1, refCount: false }), ) } From b1ecca87d775d36f1babf7388e85d11852ef692e Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Thu, 7 May 2026 23:13:45 +0200 Subject: [PATCH 08/12] frontend misc cleanup: artwork / rssfeed / settings / swiper (LOW-5/6/7/8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small cleanups, none individually load-bearing but they accumulate. LOW-5 (artwork.service Observable never completes). getArtwork / getArtistArtwork built `new Observable(observer => observer.next(url))` without ever calling observer.complete(). Subscribers stayed open forever and only got cleaned up when the component was destroyed. With one swiper showing 30 cards on a long-running kiosk, that's 30 subscriptions per page open. Switch to `of(value)` which is synchronous, single-emit, and completes properly. LOW-7 (rssfeed.service log spam + uncaught error). The map() body ran `console.log(item)` once per RSS item — every feed load dumped hundreds of lines of raw RSS into chrome_debug.log, which we ship via debug.php for support tickets. Drop the log. Also add a catchError that returns an empty Media[] so a feed-fetch failure (network blip, malformed XML) doesn't error the whole observable and crash the medialist page. LOW-6 (settings.page no error handler on shutdown/reboot POST). `this.http.post('/api/shutdown', {}).subscribe()` swallowed any network / 500 error silently. The Ionic alert dismissed and the user got zero feedback that the action didn't fire. Add an error logger so the failure at least lands in chrome_debug.log for diagnosis. LOW-8 (swiper stale index after data change). cachedSwiperPosition was captured on ionViewWillLeave and restored unconditionally on re-enter via slideTo(cached). If the data set shrank while the page was hidden (audiobook removed, category switched, etc.), slideTo landed on an out-of-range index. Clamp to `Math.min(cached, len-1)` so we restore to the last valid slide rather than nothing. Type-checks pass. --- src/frontend-box/src/app/artwork.service.ts | 17 ++++++++--------- src/frontend-box/src/app/rssfeed.service.ts | 15 ++++++++++++--- .../src/app/settings/settings.page.ts | 14 ++++++++++++-- .../src/app/swiper/swiper.component.ts | 11 ++++++++++- 4 files changed, 42 insertions(+), 15 deletions(-) diff --git a/src/frontend-box/src/app/artwork.service.ts b/src/frontend-box/src/app/artwork.service.ts index 101628c3..51412446 100644 --- a/src/frontend-box/src/app/artwork.service.ts +++ b/src/frontend-box/src/app/artwork.service.ts @@ -1,24 +1,23 @@ import { Injectable } from '@angular/core' -import { Observable } from 'rxjs' +import { Observable, of } from 'rxjs' import type { Media } from './media' @Injectable({ providedIn: 'root', }) export class ArtworkService { + // LOW-5: previous code used `new Observable(observer => observer.next(url))` + // which never calls observer.complete(), so the observable stays "open" + // forever. Subscribers leak — every cover lookup adds a non-completing + // subscription that's only released when the component is destroyed. + // `of(value)` is the idiomatic single-emit-then-complete observable. getArtwork(media: Media): Observable { const coverUrl = media.cover || '../assets/images/nocover_mupi.png' - - return new Observable((observer) => { - observer.next(coverUrl) - }) + return of(coverUrl) } getArtistArtwork(media: Media): Observable { const coverUrl = media.artistcover || media.cover || '../assets/images/nocover_mupi.png' - - return new Observable((observer) => { - observer.next(coverUrl) - }) + return of(coverUrl) } } diff --git a/src/frontend-box/src/app/rssfeed.service.ts b/src/frontend-box/src/app/rssfeed.service.ts index 78311cfa..029d7bef 100644 --- a/src/frontend-box/src/app/rssfeed.service.ts +++ b/src/frontend-box/src/app/rssfeed.service.ts @@ -1,7 +1,7 @@ import { HttpClient } from '@angular/common/http' import { Injectable } from '@angular/core' -import type { Observable } from 'rxjs' -import { map, mergeAll, toArray } from 'rxjs/operators' +import { type Observable, of } from 'rxjs' +import { catchError, map, mergeAll, toArray } from 'rxjs/operators' import { environment } from 'src/environments/environment' import type { CategoryType, Media } from './media' import type { RssFeed } from './rssfeed' @@ -21,7 +21,11 @@ export class RssFeedService { return this.http.get(this.url).pipe( map((response: RssFeed) => { return response.rss.channel.item.map((item) => { - console.log(item) + // LOW-7: removed `console.log(item)` — fired once per feed item + // on every load, polluting chrome_debug.log (which we already + // ship via debug.php for support tickets) with hundreds of lines + // of raw RSS data per feed. Useful for one-off debugging, not + // for production. const media: Media = { id: item.enclosure?._attributes?.url, artist: this.handleCData(response.rss?.channel?.title), @@ -40,6 +44,11 @@ export class RssFeedService { }), mergeAll(), toArray(), + // LOW-7: previously a feed-fetch error rejected the observable, so + // upstream callers got an error and the medialist crashed. Return + // an empty array on error so the page just shows "no episodes" and + // the user can navigate away cleanly. + catchError(() => of([] as Media[])), ) } diff --git a/src/frontend-box/src/app/settings/settings.page.ts b/src/frontend-box/src/app/settings/settings.page.ts index 5f505476..ba10887c 100644 --- a/src/frontend-box/src/app/settings/settings.page.ts +++ b/src/frontend-box/src/app/settings/settings.page.ts @@ -124,13 +124,23 @@ export class SettingsPage extends SwiperIonicEventsHelper { { text: 'Shutdown', handler: () => { - this.http.post('/api/shutdown', {}).subscribe() + // LOW-6: previously `subscribe()` with no error handler. A + // failed POST (network blip, backend down) silently disappeared, + // and the user got an Ionic alert dismiss with no feedback that + // the shutdown didn't fire. Wire up an error logger so the + // failure at least lands in chrome_debug.log. + this.http.post('/api/shutdown', {}).subscribe({ + error: (err) => console.error('[settings] /api/shutdown failed:', err), + }) }, }, { text: 'Reboot', handler: () => { - this.http.post('/api/reboot', {}).subscribe() + // LOW-6: same fix. + this.http.post('/api/reboot', {}).subscribe({ + error: (err) => console.error('[settings] /api/reboot failed:', err), + }) }, }, { diff --git a/src/frontend-box/src/app/swiper/swiper.component.ts b/src/frontend-box/src/app/swiper/swiper.component.ts index eca2f978..4b9699e7 100644 --- a/src/frontend-box/src/app/swiper/swiper.component.ts +++ b/src/frontend-box/src/app/swiper/swiper.component.ts @@ -63,7 +63,16 @@ export class SwiperComponent { effect(() => { if (this.pageIsShown()) { - this.swiper()?.slideTo(this.cachedSwiperPosition, 0) + // LOW-8: cachedSwiperPosition is captured on ionViewWillLeave, but + // when the user comes back the data set may be smaller (e.g. they + // edited the audiobook list and removed entries while the page was + // hidden). slideTo(cached) on an out-of-range index either no-ops + // silently or produces a blank tile area. Clamp to the visible + // length so we land on the last valid slide instead. + const len = this.shownData().length + if (len > 0) { + this.swiper()?.slideTo(Math.min(this.cachedSwiperPosition, len - 1), 0) + } } }) } From 67e25d22f06d473b4f312e3afb7bd9fc10a6e5f8 Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Thu, 7 May 2026 21:22:23 +0200 Subject: [PATCH 09/12] spotify.php: flip spotify.active=true after a successful OAuth re-auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-running the Connect-Spotify OAuth flow set accessToken and refreshToken in mupiboxconfig.json but left spotify.active untouched. If an admin had previously set active=false (e.g. as a debugging workaround when the previous tokens were broken), the new tokens silently don't take effect — the frontend continues to hide Spotify content, the admin assumes the re-auth also failed, and ends up with a "Spotify is broken even after I just re-authorised" mystery. Set active=true unconditionally would re-enable Spotify even if the OAuth call returned an error blob (no access_token in the response), which would resurrect the SDK-init-loop-stuck-state from the previous broken-token regime. Gate the flip on both tokens being non-empty so a failed OAuth leaves active alone. --- AdminInterface/www/spotify.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/AdminInterface/www/spotify.php b/AdminInterface/www/spotify.php index 3cf50bdb..d1a45424 100644 --- a/AdminInterface/www/spotify.php +++ b/AdminInterface/www/spotify.php @@ -23,6 +23,16 @@ $tokendata = json_decode($Tokenoutput[0], true); $data["spotify"]["accessToken"] = $tokendata["access_token"]; $data["spotify"]["refreshToken"] = $tokendata["refresh_token"]; + // Re-authorising via OAuth implies the user wants Spotify ON. Without this + // flip, an admin who turned `active` off (e.g. while debugging) and then + // re-ran the Connect-Spotify flow would still have Spotify hidden in the + // frontend — and might assume the new tokens are also broken. Only flip if + // we actually got both tokens back; the OAuth call could have failed and + // returned an error blob, in which case enabling Spotify would resurrect + // the loading-spinner-stuck state. + if (!empty($tokendata["access_token"]) && !empty($tokendata["refresh_token"])) { + $data["spotify"]["active"] = true; + } $json_object = json_encode($data); $save_rc = file_put_contents('/tmp/.mupiboxconfig.json', $json_object); exec("sudo mv /tmp/.mupiboxconfig.json /etc/mupibox/mupiboxconfig.json"); From e2861cf31380ce4a5e13153bf35900a46357cbff Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Fri, 8 May 2026 09:58:40 +0200 Subject: [PATCH 10/12] frontend SDK robustness round 2: throttle activity tracking + drop BehaviorSubject replay (LOW-4 A23/A24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two leftover sub-bugs from LOW-4 that the Phase 4 commit explicitly deferred — both small and unrelated except for living in the spotify- player adjacent stack. A23 (display-manager.service activity tracking). - The pipe between activityDebouncer and the resetIdleTimer call was empty, despite a `// Debounce activity events` comment that suggested otherwise. Every mousemove and touchmove tick fired a fresh resetIdleTimer call. The work itself was cheap (one Date.now() write) but woke the JS event loop hundreds of times a second on a busy touchscreen drag. Add throttleTime(500ms); a 1-minute idle threshold has plenty of headroom for half-second granularity. - ngOnDestroy was dead code. The service is providedIn: 'root', so Angular keeps it alive for the entire app lifetime — ngOnDestroy never fires. Removed the cleanup block and noted the lifetime assumption in a comment so the next person touching this file doesn't mistake it for a missing teardown. The `document` listeners stay attached forever, which is fine since `document` lives just as long. A24 (external-playback-navigator BehaviorSubject replay). - trackChangeDetected$ was a BehaviorSubject, which replays the last value to every new subscriber. external- playback-navigator subscribes once on construction and would receive the LAST detected track immediately on any HMR reload or service re-mount — and then auto-navigate to /player even though the user had since walked away to another page. Switch to plain Subject so subscribers only see events that fire AFTER they subscribed. Verified all three consumers (external-playback-navigator, media.service, spotify.service re-export) only .next()/.subscribe() — no .value reads — so the swap doesn't break anything. Type-checks pass. --- .../src/app/display-manager.service.ts | 22 ++++++++++++++----- .../src/app/spotify-player.service.ts | 13 +++++++++-- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/frontend-box/src/app/display-manager.service.ts b/src/frontend-box/src/app/display-manager.service.ts index 20f25334..7c136e8a 100644 --- a/src/frontend-box/src/app/display-manager.service.ts +++ b/src/frontend-box/src/app/display-manager.service.ts @@ -1,6 +1,7 @@ import { HttpClient } from '@angular/common/http' import { Injectable } from '@angular/core' import { interval, Subject, Subscription } from 'rxjs' +import { throttleTime } from 'rxjs/operators' import { environment } from 'src/environments/environment' import { MupiboxConfig } from './mupibox-config.model' import { SpotifyService } from './spotify.service' @@ -52,8 +53,14 @@ export class DisplayManagerService { document.addEventListener(eventName, () => this.activityDebouncer.next(), { passive: true }) } - // Debounce activity events to avoid spamming resets - this.activitySubscription = this.activityDebouncer.subscribe(() => { + // LOW-4 / A23: previous comment claimed "Debounce activity events to avoid + // spamming resets" but the pipe was empty — every mousemove and touchstart + // tick fired a fresh resetIdleTimer call (which writes to a Date.now() + // field, so cheap, but still wakes the JS event loop hundreds of times + // a second on a busy screen). Add throttleTime(500ms) so we update the + // last-activity timestamp at most twice a second — plenty for a 1-minute + // idle threshold and orders of magnitude less work. + this.activitySubscription = this.activityDebouncer.pipe(throttleTime(500)).subscribe(() => { this.resetIdleTimer() }) } @@ -101,8 +108,11 @@ export class DisplayManagerService { }) } - ngOnDestroy(): void { - this.idleCheckInterval?.unsubscribe() - this.activitySubscription?.unsubscribe() - } + // LOW-4 / A23: removed dead ngOnDestroy. The service is providedIn: 'root', + // so Angular keeps it alive for the entire app lifetime — ngOnDestroy never + // fires. The cleanup it claimed to do was theatre. The DOM listeners + // attached to `document` in setupActivityTracking() likewise stay attached + // for the app lifetime (which is fine — `document` lives just as long). + // If we ever switch to a non-root scope this needs revisiting; until then + // honesty beats cargo-culted lifecycle hooks. } diff --git a/src/frontend-box/src/app/spotify-player.service.ts b/src/frontend-box/src/app/spotify-player.service.ts index 3e94357f..101a3eec 100644 --- a/src/frontend-box/src/app/spotify-player.service.ts +++ b/src/frontend-box/src/app/spotify-player.service.ts @@ -1,7 +1,7 @@ import { DOCUMENT } from '@angular/common' import { HttpClient } from '@angular/common/http' import { Inject, Injectable } from '@angular/core' -import { BehaviorSubject } from 'rxjs' +import { BehaviorSubject, Subject } from 'rxjs' import { debounceTime, filter } from 'rxjs/operators' import { environment } from 'src/environments/environment' import { LogService } from './log.service' @@ -33,7 +33,16 @@ export class SpotifyPlayerService { // External playback detection private previousPlayerState: SpotifyWebPlaybackState | null = null - public trackChangeDetected$ = new BehaviorSubject(null) + // LOW-4 / A24: this used to be a BehaviorSubject, which replays the last + // emitted value to every new subscriber. external-playback-navigator's + // subscribe-on-init then re-fired auto-navigate-to-/player on HMR / + // component re-mount with whatever track was last detected — even though + // the user had since navigated elsewhere. Subject (without replay) only + // emits to listeners attached at the moment of .next(), which is the + // intended "track-just-changed" fire-and-forget shape for this signal. + // Verified all consumers: only .next() and .subscribe() — no .value reads + // anywhere, so the swap is safe. + public trackChangeDetected$ = new Subject() // Error state observable for UI feedback public sdkLoadError$ = new BehaviorSubject(null) From db286f53d6f3f356c7b49dc02cb2e0661ecc50dc Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Fri, 8 May 2026 10:07:13 +0200 Subject: [PATCH 11/12] small robustness round: parsers.js / log.service / logout.php (B7 / B14 / R3-B-4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small defensive cleanups, none functional changes in the common path but each one removes a sharp edge a future bug could land on. B7 — parsers.js (mplayer slave-protocol response parsing). - parseString did `str.slice(1, -1)` blindly. Quotes around mplayer's get_meta_* responses are CONVENTIONAL but not guaranteed (different mplayer builds emit different framing, edge cases like empty-string answers); a non-quoted response would silently lose its first and last char. Check for the "…" wrapper and only strip when it's present. - parseStringList split mplayer's metadata response on `,` — which goes wrong as soon as a field VALUE contains a comma (album titles like "Foo, Vol. 2" or composer names like "Bach, Johann Sebastian"). Re-join all fragments between two known meta-prop markers as the value, so commas inside values are preserved instead of treated as delimiters. - parseFlag now also accepts non-string input safely (mplayer glitches sometimes hand us numbers or null). B14 — log.service.ts HTTP-logging recursion risk. - sendBufferedLogs already used console.warn directly (correct). Added a method-level comment explaining why this MUST stay that way: this.warn → bufferLogEntry → queues a HTTP POST → on failure the catchError would re-enter sendBufferedLogs → infinite loop. The property `console.warn` (NOT `this.warn`) annotation hangs on the two existing call sites. R3-B-4 — logout.php session cookie. - session_destroy() alone clears server-side session storage but leaves the PHPSESSID cookie alive on the client. Next request the browser sends the same id, PHP rehydrates an empty session under it, and other tabs still believe they are authenticated. Best practice: also expire the cookie via setcookie(name, '', past-time, …) with the original cookie parameters, and clear $_SESSION first so destroy doesn't leave any data even briefly. Type-checks pass. --- AdminInterface/www/logout.php | 25 +++++++++++++++ src/backend-player/src/parsers.js | 42 ++++++++++++++++++++++--- src/frontend-box/src/app/log.service.ts | 9 ++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/AdminInterface/www/logout.php b/AdminInterface/www/logout.php index 3318a804..7508c081 100644 --- a/AdminInterface/www/logout.php +++ b/AdminInterface/www/logout.php @@ -1,6 +1,31 @@ val -const parseString = (str) => str.slice(1, -1) +// B7: previous parseString was a blind `str.slice(1, -1)` — strips the +// FIRST and LAST characters whether or not they're actually quotes. +// mplayer's slave-protocol responses for `get_meta_*` ARE wrapped in +// double quotes, but the wrapper isn't guaranteed (some mplayer +// builds emit unquoted strings, ANS_-property responses are bare, +// and edge cases — empty strings, partial outputs — would have us +// silently strip real characters). Check the wrapper first; if it's +// not a `"…"` pair, return the input untouched. +const parseString = (str) => { + if (typeof str !== 'string') return '' + if (str.length >= 2 && str.startsWith('"') && str.endsWith('"')) { + return str.slice(1, -1) + } + return str +} -const parseFlag = (str) => str.toLowerCase().trim() === 'yes' +const parseFlag = (str) => typeof str === 'string' && str.toLowerCase().trim() === 'yes' const knownMetaProps = ['Title', 'Artist', 'Album', 'Year', 'Comment', 'Genre'] +// B7: parseStringList consumes mplayer's `metadata` response which +// concatenates fields as comma-separated `Title,,Artist,,…`. +// The previous implementation split on a literal `,` — which goes +// wrong as soon as a field VALUE contains a comma (e.g. an album +// titled "Foo, Vol. 2" or an artist "Bach, Johann Sebastian"). We +// can't fix the underlying ambiguity (mplayer's protocol is what it +// is), but we can be more defensive: re-join everything between two +// known meta-prop markers as the value, so a comma INSIDE a value +// gets preserved instead of treated as a delimiter. Loses only when +// a value happens to start with one of knownMetaProps verbatim — far +// less likely than commas in titles. const parseStringList = (str) => { const res = Object.create(null) + if (typeof str !== 'string') return res const parts = str.split(',') let metaProp = null + let buffer = [] + const flush = () => { + if (metaProp != null && buffer.length > 0) { + res[metaProp] = buffer.join(',') + buffer = [] + } + } for (let i = 0; i < parts.length; i++) { const part = parts[i] if (knownMetaProps.includes(part)) { + flush() metaProp = part } else if (metaProp) { - if (!res[metaProp]) res[metaProp] = part - else res[metaProp] += `,${part}` + buffer.push(part) } } + flush() return res } diff --git a/src/frontend-box/src/app/log.service.ts b/src/frontend-box/src/app/log.service.ts index 16f31292..e0213a3f 100644 --- a/src/frontend-box/src/app/log.service.ts +++ b/src/frontend-box/src/app/log.service.ts @@ -101,6 +101,13 @@ export class LogService { } } + // B14: this method must NEVER call this.warn / this.error / this.log / + // this.debug — those forward to bufferLogEntry, which queues a HTTP + // POST, which on failure would re-enter sendBufferedLogs from + // catchError → infinite recursion that floods both the buffer and + // the backend. Use console.warn / console.error directly for any + // diagnostics here. The current code is already correct in this + // respect; the comment exists to keep it that way under future edits. private sendBufferedLogs(): void { if (this.logBuffer.length === 0) return @@ -124,10 +131,12 @@ export class LogService { .subscribe({ next: (response) => { if (!response.success) { + // B14: console.warn — NOT this.warn. See method comment. console.warn('Backend log processing failed:', response.message) } }, error: (error) => { + // B14: console.warn — NOT this.warn. See method comment. console.warn('Error in log HTTP request:', error) }, }) From 8a55fccfa5d17b94cd7eacd809625e0fb0445cc6 Mon Sep 17 00:00:00 2001 From: Waldemar Berg Date: Fri, 8 May 2026 10:18:22 +0200 Subject: [PATCH 12/12] robustness: frontend reliability (B10/B11/B12/B15) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - B10 spotify.service: 15s timeout() on 8 metadata HTTP calls so a hung Spotify backend doesn't block the current$ polling loop or album/show/audiobook lookups indefinitely (catchError handles the resulting TimeoutError). - B11 media.service: catchError on current$/local$/albumStop$/mupihat$ HTTP polling streams — a single 5xx no longer tears down the shareReplay-backed observable and silently kills polling. - B12 add.page: m3uStreamfetcher save() now awaits the resolved URL inside the loading-indicator's then() (race fixed; save() was previously firing before the stream URL was resolved). - B15 network.service: isOnline() seeded with startWith(false) so toSignal()-based consumers don't hang at initialValue forever during the cold-boot 300ms-to-5s window before /api/network's first response. --- src/frontend-box/src/app/add/add.page.ts | 28 ++++++++++----- src/frontend-box/src/app/media.service.ts | 39 +++++++++++++++++---- src/frontend-box/src/app/network.service.ts | 10 +++++- src/frontend-box/src/app/spotify.service.ts | 12 +++++++ 4 files changed, 73 insertions(+), 16 deletions(-) diff --git a/src/frontend-box/src/app/add/add.page.ts b/src/frontend-box/src/app/add/add.page.ts index 12e15303..7744f7b1 100644 --- a/src/frontend-box/src/app/add/add.page.ts +++ b/src/frontend-box/src/app/add/add.page.ts @@ -355,7 +355,7 @@ export class AddPage implements OnInit, AfterViewInit { submit(form: NgForm) { this.activityIndicatorService.create().then((indicator) => { this.activityIndicatorVisible = true - indicator.present().then(() => { + indicator.present().then(async () => { if (this.sourceType === 'spotifyURL' || this.sourceType === 'spotifySearch') { this.source = 'spotify' } else if (this.sourceType === 'streamURL') { @@ -400,14 +400,24 @@ export class AddPage implements OnInit, AfterViewInit { if (form.form.value.streamURL?.length) { media.id = form.form.value.streamURL if (media.id.endsWith('.m3u')) { - this.m3uStreamfetcher(media.id) - .then((firstURL) => { - console.log('First found URL:', firstURL) - media.id = firstURL - }) - .catch((error) => { - console.error('Error for extract url from m3u:', error) - }) + // B12: previously this fired m3uStreamfetcher() WITHOUT + // awaiting and immediately fell through to this.save(media) + // — the save persisted the original .m3u URL while the + // .then() callback updated media.id LATER (post-save). The + // extracted stream URL was therefore never persisted, and + // playback later tried to play the .m3u literal in mplayer + // (which mplayer happens to handle but the resume path + // can't address). Inline the await so save() sees the + // resolved URL. Errors fall through silently with the + // original .m3u — matches the original swallow-error + // semantics, just without the race. + try { + const firstURL = await this.m3uStreamfetcher(media.id) + console.log('First found URL:', firstURL) + media.id = firstURL + } catch (error) { + console.error('Error for extract url from m3u:', error) + } } if (media.id.startsWith('https://')) { // Ersetze 'https' durch 'http' diff --git a/src/frontend-box/src/app/media.service.ts b/src/frontend-box/src/app/media.service.ts index 28491ce4..913d8a27 100644 --- a/src/frontend-box/src/app/media.service.ts +++ b/src/frontend-box/src/app/media.service.ts @@ -1,7 +1,7 @@ import { HttpClient } from '@angular/common/http' import { Injectable } from '@angular/core' import { firstValueFrom, from, iif, interval, Observable, of, Subject } from 'rxjs' -import { map, mergeAll, mergeMap, shareReplay, switchMap, toArray } from 'rxjs/operators' +import { catchError, map, mergeAll, mergeMap, shareReplay, switchMap, toArray } from 'rxjs/operators' import { environment } from '../environments/environment' import type { AlbumStop } from './albumstop' import type { Artist } from './artist' @@ -158,25 +158,52 @@ export class MediaService { }), shareReplay({ bufferSize: 1, refCount: true }), ) - : // Remote: HTTP polling + : // Remote: HTTP polling. + // B11: a single HTTP failure (network blip, backend restart) + // would error the source observable, and shareReplay would + // forever replay that error to subscribers — UI stops getting + // state updates until the page is reloaded. Wrap the inner + // get in catchError(of({})) so transient failures show as + // "no current state" without tearing down the polling stream. interval(10000).pipe( switchMap( - (): Observable => this.http.get(`${this.getPlayerBackendUrl()}/state`), + (): Observable => + this.http + .get(`${this.getPlayerBackendUrl()}/state`) + .pipe(catchError(() => of({} as CurrentSpotify))), ), shareReplay({ bufferSize: 1, refCount: true }), ) + // Same B11 pattern for local$ / albumStop$ / mupihat$ — all polling + // streams that should swallow transient errors instead of becoming + // permanently broken. this.local$ = interval(1000).pipe( - switchMap((): Observable => this.http.get(`${this.getPlayerBackendUrl()}/local`)), + switchMap( + (): Observable => + this.http + .get(`${this.getPlayerBackendUrl()}/local`) + .pipe(catchError(() => of({} as CurrentMPlayer))), + ), shareReplay({ bufferSize: 1, refCount: true }), ) this.albumStop$ = interval(1000).pipe( - switchMap((): Observable => this.http.get(`${this.getApiBackendUrl()}/albumstop`)), + switchMap( + (): Observable => + this.http + .get(`${this.getApiBackendUrl()}/albumstop`) + .pipe(catchError(() => of({} as AlbumStop))), + ), shareReplay({ bufferSize: 1, refCount: false }), ) // Every 2 seconds should be enough for timely charging update. this.mupihat$ = interval(2000).pipe( - switchMap((): Observable => this.http.get(`${this.getApiBackendUrl()}/mupihat`)), + switchMap( + (): Observable => + this.http + .get(`${this.getApiBackendUrl()}/mupihat`) + .pipe(catchError(() => of({} as Mupihat))), + ), shareReplay({ bufferSize: 1, refCount: false }), ) diff --git a/src/frontend-box/src/app/network.service.ts b/src/frontend-box/src/app/network.service.ts index 83fbd2fd..70bf1948 100644 --- a/src/frontend-box/src/app/network.service.ts +++ b/src/frontend-box/src/app/network.service.ts @@ -1,7 +1,7 @@ import { HttpClient } from '@angular/common/http' import { Injectable } from '@angular/core' import { Observable, timer } from 'rxjs' -import { distinctUntilChanged, filter, map, shareReplay, switchMap } from 'rxjs/operators' +import { distinctUntilChanged, filter, map, shareReplay, startWith, switchMap } from 'rxjs/operators' import { environment } from '../environments/environment' import type { Network } from './network' @@ -30,6 +30,14 @@ export class NetworkService { return this.network$.pipe( filter((network) => network.ip !== undefined), map((network) => network.onlinestate === 'online'), + // B15: until the first /api/network response arrives the filter() + // above produces no emissions, and any consumer using `toSignal(…)` + // hangs at its initialValue forever (default null). Seed with + // false so callers see "offline" until the real value arrives — + // a 300ms-to-5s window on cold boot. Treats unknown as offline, + // which matches the conservative interpretation for the kid- + // facing UI ("don't try to use Spotify until I know we're online"). + startWith(false), distinctUntilChanged(), ) } diff --git a/src/frontend-box/src/app/spotify.service.ts b/src/frontend-box/src/app/spotify.service.ts index 00ff2f14..16acf4c6 100644 --- a/src/frontend-box/src/app/spotify.service.ts +++ b/src/frontend-box/src/app/spotify.service.ts @@ -209,6 +209,10 @@ export class SpotifyService { const artistUrl = `${environment.backend.apiUrl}/spotify/artist/${id}` return this.http.get(artistUrl).pipe( + // B10: 15s timeout so a hung Spotify-API call doesn't block the + // current$ polling loop indefinitely. catchError further down + // catches the TimeoutError and falls back to the placeholder. + timeout(15000), switchMap((artist) => { const artistcover = artist.images?.[0]?.url || '../assets/images/nocover_mupi.png' @@ -252,6 +256,7 @@ export class SpotifyService { const showEpisodesUrl = `${environment.backend.apiUrl}/spotify/show/${id}/episodes` return this.http.get(showUrl).pipe( + timeout(15000), // B10 switchMap((show) => { const showName = show.name || 'Unknown Show' const showcover = show.images?.[0]?.url || '../assets/images/nocover_mupi.png' @@ -301,6 +306,7 @@ export class SpotifyService { const albumUrl = `${environment.backend.apiUrl}/spotify/album/${id}` return this.http.get(albumUrl).pipe( + timeout(15000), // B10 map((album) => { const media: Media = { id: album.id, @@ -357,6 +363,7 @@ export class SpotifyService { const audiobookUrl = `${environment.backend.apiUrl}/spotify/audiobook/${id}` return this.http.get(audiobookUrl).pipe( + timeout(15000), // B10 map((audiobook) => { const media: Media = { audiobookid: audiobook.id, @@ -408,6 +415,7 @@ export class SpotifyService { const episodeUrl = `${environment.backend.apiUrl}/spotify/episode/${id}` return this.http.get(episodeUrl).pipe( + timeout(15000), // B10 map((episode) => { const media: Media = { showid: episode.id, @@ -589,6 +597,7 @@ export class SpotifyService { const albumUrl = `${environment.backend.apiUrl}/spotify/album/${albumId}` return this.http.get(albumUrl).pipe( + timeout(15000), // B10 map((album) => ({ total_tracks: album.total_tracks, album_name: album.name, @@ -666,9 +675,11 @@ export class SpotifyService { const showEpisodesUrl = `${environment.backend.apiUrl}/spotify/show/${showId}/episodes` return this.http.get(showUrl).pipe( + timeout(15000), // B10 switchMap((show) => { // Get all episodes for position calculation return this.http.get(showEpisodesUrl).pipe( + timeout(15000), // B10 map((episodesData) => ({ total_episodes: show.total_episodes, show_name: show.name, @@ -696,6 +707,7 @@ export class SpotifyService { const audiobookUrl = `${environment.backend.apiUrl}/spotify/audiobook/${audiobookId}` return this.http.get(audiobookUrl).pipe( + timeout(15000), // B10 map((audiobook) => ({ total_chapters: audiobook.chapters?.total || 0, audiobook_name: audiobook.name,