Discover artwork + Library icon + notification tap routing #73

Merged

bvandeusen merged 5 commits from dev into main 2026-06-02 09:58:10 -04:00

Conversation 0 Commits 5 Files Changed 7

+274 -15

bvandeusen commented 2026-06-02 09:20:16 -04:00

Owner

Copy Link

Copy Source

Summary

• BaseUrlInterceptor scoped to placeholder host (fixes Discover artwork). Coil and Retrofit share the same OkHttp client; the interceptor was rewriting Lidarr's absolute external artwork URLs (artwork.musicbrainz.org, coverartarchive.org) into Minstrel-host 404s, which Coil fell back from silently. Added PLACEHOLDER_HOST constant and short-circuited the rewrite for non-placeholder hosts. Three new unit tests cover the placeholder rewrite, external pass-through, and unparseable-baseUrl fallthrough.
• Top-nav Library icon → Material Outlined LibraryMusic. Lucide has no music-library glyph and the literal library icons (Library / LibraryBig / SquareLibrary) don't read as "music collection" without a label. Material's Icons.Outlined.LibraryMusic is the canonical music-library symbol; outlined variant matches Lucide's stroke style closely enough to blend. Added compose-material-icons-extended (version pinned by compose-bom).
• Media-notification tap opens NowPlaying. Previously landed on whatever shell route MainActivity last rendered because the session-activity PendingIntent was never configured. MinstrelPlayerService now builds a PendingIntent with an EXTRA_OPEN_NOW_PLAYING flag and hands it to MediaSession.Builder.setSessionActivity. MainActivity reads the extra in onCreate AND onNewIntent (covers cold + warm starts), flips a StateFlow, then strips the extra. App composable runs a LaunchedEffect to navigate once the NavHost is mounted; launchSingleTop avoids stacking. Intentional improvement over Flutter audio_service default (which just opens the launcher activity).

Follow-up worth noting: AuthCookieInterceptor still attaches the Minstrel session cookie to external requests. External hosts ignore unrecognized cookies, so it's not breaking anything, but a parallel scope-to-placeholder cleanup would be tidy.

Test plan

• CI: new BaseUrlInterceptorTest passes; detekt + ktlint clean
• On device: Discover view shows artist artwork instead of fallback user icons
• Top nav: Library icon reads as books-with-music-note; no visible style clash next to House / Search / EllipsisVertical
• Tap the media notification while on Home (warm) and after a kill (cold) — both land on NowPlaying. Drag-down returns to the originating screen.
• Lock-screen card + Pixel Watch tile also open NowPlaying when tapped.

## Summary - **BaseUrlInterceptor scoped to placeholder host (fixes Discover artwork).** Coil and Retrofit share the same OkHttp client; the interceptor was rewriting Lidarr's absolute external artwork URLs (`artwork.musicbrainz.org`, `coverartarchive.org`) into Minstrel-host 404s, which Coil fell back from silently. Added `PLACEHOLDER_HOST` constant and short-circuited the rewrite for non-placeholder hosts. Three new unit tests cover the placeholder rewrite, external pass-through, and unparseable-baseUrl fallthrough. - **Top-nav Library icon → Material Outlined LibraryMusic.** Lucide has no music-library glyph and the literal library icons (Library / LibraryBig / SquareLibrary) don't read as "music collection" without a label. Material's `Icons.Outlined.LibraryMusic` is the canonical music-library symbol; outlined variant matches Lucide's stroke style closely enough to blend. Added `compose-material-icons-extended` (version pinned by compose-bom). - **Media-notification tap opens NowPlaying.** Previously landed on whatever shell route MainActivity last rendered because the session-activity PendingIntent was never configured. `MinstrelPlayerService` now builds a PendingIntent with an `EXTRA_OPEN_NOW_PLAYING` flag and hands it to `MediaSession.Builder.setSessionActivity`. MainActivity reads the extra in onCreate AND onNewIntent (covers cold + warm starts), flips a StateFlow, then strips the extra. App composable runs a `LaunchedEffect` to navigate once the NavHost is mounted; `launchSingleTop` avoids stacking. Intentional improvement over Flutter audio_service default (which just opens the launcher activity). Follow-up worth noting: `AuthCookieInterceptor` still attaches the Minstrel session cookie to external requests. External hosts ignore unrecognized cookies, so it's not breaking anything, but a parallel scope-to-placeholder cleanup would be tidy. ## Test plan - [ ] CI: new BaseUrlInterceptorTest passes; detekt + ktlint clean - [ ] On device: Discover view shows artist artwork instead of fallback user icons - [ ] Top nav: Library icon reads as books-with-music-note; no visible style clash next to House / Search / EllipsisVertical - [ ] Tap the media notification while on Home (warm) and after a kill (cold) — both land on NowPlaying. Drag-down returns to the originating screen. - [ ] Lock-screen card + Pixel Watch tile also open NowPlaying when tapped.

bvandeusen added 1 commit 2026-06-02 09:20:16 -04:00

fix(android): scope BaseUrlInterceptor to placeholder host only ... aec10ce787

Discover suggestion artist images failed to load on Android while
loading fine in the web client. Root cause: BaseUrlInterceptor
unconditionally rewrote every outgoing request's scheme/host/port
to AuthStore.baseUrl. That's correct for Minstrel-bound requests
built with the http://placeholder.invalid sentinel (Retrofit's
frozen baseUrl, every cover-URL builder, ServerImage's
resolveServerUrl). But Lidarr surfaces artist artwork as absolute
URLs to external hosts (artwork.musicbrainz.org,
coverartarchive.org); rewriting those to the Minstrel host
produced 404s that Coil silently fell back from to the User icon.

Web works because the browser fetches the URL as authored. Coil
on Android shares the OkHttp client (and so the interceptor chain)
with Retrofit, which is why the bug surfaced here only.

Add a PLACEHOLDER_HOST companion constant and short-circuit the
rewrite for non-placeholder hosts. Test coverage:
- placeholder host → rewritten to live baseUrl
- absolute external URL → host/scheme/path preserved
- unparseable baseUrl → falls through (no throw)

AuthCookieInterceptor still attaches the Minstrel session cookie
to external requests; external hosts ignore unrecognized cookies
so that's not breaking anything, but it's worth a follow-up DRY
pass to scope auth attachment the same way.

bvandeusen added 1 commit 2026-06-02 09:35:58 -04:00

feat(android): swap top-nav Library icon to Lucide.SquareLibrary ... 22dc343b39

LibraryBig (stacked book spines) didn't read as "Library" without
the label — easy to misread as a generic stack/columns icon.
SquareLibrary frames the same books-on-shelf glyph inside a
rounded square, matching the visual weight of the neighbouring
House and Search icons better and reading more clearly as a
distinct tappable destination.

Untouched: the RequestsScreen per-row "album"-kind avatar still
uses LibraryBig (parity with Flutter's lib/requests/requests_screen.dart).

bvandeusen added 1 commit 2026-06-02 09:40:50 -04:00

feat(android): use Material Outlined LibraryMusic for top-nav Library ... faf2cac0c9

Lucide has no music-library glyph — Library / LibraryBig /
SquareLibrary all read as a generic books-on-shelf icon without
a label. Operator picked Material's LibraryMusic (the canonical
"books + music note" symbol used by every major music app) as
the recognizable alternative.

Use the Outlined variant: filled icons would clash with the
neighbouring stroked Lucide icons (House, Search, EllipsisVertical),
but Outlined's stroke style matches Lucide closely enough that
the mix is subtle.

Adds the compose-material-icons-extended dependency (version
pinned by compose-bom). R8 strips unused icons in release builds
so the APK cost is just the ones we actually reference.

bvandeusen added 1 commit 2026-06-02 09:43:47 -04:00

feat(android): media-notification tap opens NowPlaying ... 438e81a117

Tapping the system media notification previously landed on
whatever shell route MainActivity last rendered (Home / Library /
Search) because MinstrelPlayerService never configured the
session-activity PendingIntent, so Media3 defaulted to the
launcher activity entry point. Operator request: tap should go
straight to the full player.

MinstrelPlayerService.onCreate now builds a PendingIntent
targeting MainActivity with an EXTRA_OPEN_NOW_PLAYING flag and
passes it to MediaSession.Builder.setSessionActivity. The flag
also covers the lock-screen card and the Pixel Watch tile —
both use the same session-activity PendingIntent.

MainActivity reads the extra in onCreate AND onNewIntent (so a
warm app gets the navigation too, not just cold launches), flips
a pendingOpenNowPlaying StateFlow, then strips the extra so a
config-change recreation doesn't re-trigger. The App composable
observes the flag and runs a LaunchedEffect to navigate once the
NavHost is mounted — handles both cold start (BootSplash →
resolved → navigate) and warm start. launchSingleTop avoids
stacking copies if NowPlaying is already on top, and the
onOpenedNowPlaying callback clears the flag post-navigation so
later recompositions don't re-fire.

Divergence from Flutter (intentional): audio_service's default
notification tap behavior just opens the launcher activity at
whatever screen it was on — exactly the behavior the operator
asked to improve.

bvandeusen changed title from Scope BaseUrlInterceptor to placeholder host (fixes Discover artwork) to Discover artwork + Library icon + notification tap routing 2026-06-02 09:44:12 -04:00

bvandeusen added 1 commit 2026-06-02 09:50:06 -04:00

fix(android): keep BaseUrlInterceptor.intercept under detekt ReturnCount ... 4cd38aa62f

aec10ce7 added a third early return (the placeholder-host bail)
on top of the existing unparseable-baseUrl elvis return, tripping
detekt's ReturnCount ceiling of 2 on the dev test workflow.

Refactor: keep the placeholder-host early bail (it preserves the
no-op cost for external URLs — no AuthStore read, no URL parse),
fold the unparseable-baseUrl case into a `?:` that falls back to
the original URL. Result is two returns and identical observable
behavior — placeholder hosts get rewritten when baseUrl parses,
fall through unchanged when it doesn't.

Existing unit tests cover all three paths and continue to assert
the same outputs.

bvandeusen merged commit b64965b38d into main 2026-06-02 09:58:10 -04:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

deps

Dependency update (Renovate)

deps:major

Major dependency bump — held for manual review

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

bvandeusen

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: bvandeusen/minstrel#73