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/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/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/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/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( 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) }, }) diff --git a/src/frontend-box/src/app/media.service.ts b/src/frontend-box/src/app/media.service.ts index e4452f2f..913d8a27 100644 --- a/src/frontend-box/src/app/media.service.ts +++ b/src/frontend-box/src/app/media.service.ts @@ -1,13 +1,13 @@ 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' 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' @@ -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 }), ) @@ -283,18 +310,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 +487,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 +528,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)), @@ -699,7 +720,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/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/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, ) }), ) 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/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 }), ) } 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/spotify-player.service.ts b/src/frontend-box/src/app/spotify-player.service.ts index 0f4ec67d..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) @@ -41,6 +50,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 +400,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 +445,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 +492,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) }) } diff --git a/src/frontend-box/src/app/spotify.service.ts b/src/frontend-box/src/app/spotify.service.ts index 8d053250..16acf4c6 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), @@ -200,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' @@ -243,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' @@ -292,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, @@ -322,11 +337,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 })) }), ) } @@ -344,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, @@ -373,11 +393,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 })) }), ) } @@ -395,10 +415,16 @@ 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, - 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', @@ -425,11 +451,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 })) }), ) } @@ -449,10 +475,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 @@ -483,9 +505,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 // ============================================================================ @@ -520,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, @@ -562,10 +640,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, @@ -588,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, @@ -618,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, 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) + } } }) } 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)) }