Frontend Reliability Polish — API-correctness, SDK single-flight, F5-survival

AdminInterface/www/logout.php

@@ -1,6 +1,31 @@
 <?php
 
+// R3-B-4: previous code only called session_destroy(), which clears
+// server-side session data — but the client still holds the PHPSESSID
+// cookie. On next request the browser sends the old cookie, PHP
+// re-creates a session under the same id (now without `logged_in`),
+// and the user gets sent through the login flow as expected. Mostly
+// works, but leaves a stale session id alive in the browser and
+// produces inconsistent state across multi-tab use (one tab logs out,
+// other tabs still hold the cookie until refreshed).
+//
+// Best practice: also expire the cookie client-side via setcookie()
+// with an empty value and a past expiry, AND clear $_SESSION first so
+// session_destroy doesn't leave any data behind even briefly.
 session_start();
+$_SESSION = [];
+if (ini_get('session.use_cookies')) {
+    $params = session_get_cookie_params();
+    setcookie(
+        session_name(),
+        '',
+        time() - 42000,
+        $params['path'],
+        $params['domain'],
+        $params['secure'],
+        $params['httponly']
+    );
+}
 session_destroy();
 header('Location: index.php');
 exit;

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");

src/backend-player/src/parsers.js

@@ -1,24 +1,58 @@
 const id = (val) => 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,<v>,Artist,<v>,…`.
+// 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
 }
 

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'

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<string> {
     const coverUrl = media.cover || '../assets/images/nocover_mupi.png'
-
-    return new Observable((observer) => {
-      observer.next(coverUrl)
-    })
+    return of(coverUrl)
   }
 
   getArtistArtwork(media: Media): Observable<string> {
     const coverUrl = media.artistcover || media.cover || '../assets/images/nocover_mupi.png'
-
-    return new Observable((observer) => {
-      observer.next(coverUrl)
-    })
+    return of(coverUrl)
   }
 }

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.
 }

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(

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)
         },
       })