Frontend Reliability Polish — API-correctness, SDK single-flight, F5-survival#149
Open
wowa1990 wants to merge 12 commits into
Open
Frontend Reliability Polish — API-correctness, SDK single-flight, F5-survival#149wowa1990 wants to merge 12 commits into
wowa1990 wants to merge 12 commits into
Conversation
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.
…h RSS resume (MED-10/11) 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.
…2 / LOW-4) 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).
…ch storm (MED-13) 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.
…ows/RSS (MED-18 / MED-21)
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.
…ation (MED-19 / LOW-9 / LOW-10) 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.
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.
…/7/8)
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.
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.
…haviorSubject replay (LOW-4 A23/A24)
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<Track|null>, 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.
…14 / R3-B-4)
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.
- 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Was es macht
home.pagedistinctUntilChanged (MED-13)medialistüberlebt F5 und App-Resume (MED-18/21)wifi.pageKeyboard-Cleanup + Race-Window (MED-19 / LOW-9/10)player.serviceshareReplay refCount (MED-20)spotify.phpOAuth-Re-Auth flag-flipparsers.js,log.service,logout.phpCleanupArchitektur (kurz)
Keine API-Änderungen, kein Migration-Layer. Patches sind individuell klein und thematisch nicht in den anderen PRs reingerutscht — bewusst als „Sammlung restlicher Polish-Patches" gebündelt damit du sie en bloc sichten kannst, statt als Einzel-PRs.
Test in Codespaces / lokal
Hinweis: App-Resume-Test (Android/iOS PWA) nur auf echtem Device möglich.
Offene Punkte
activity throttle,BehaviorSubjectohne replay) ändern subtil das Timing — wenn dir das zu „untestbar" ist, gerne explizit ausnehmen.