Restore Plex sign-in + library auto-populate for all 4 destination backends#7
Merged
Merged
Conversation
Replaces the paste-the-UUID UX with a button. The destination form's
ABS arm gets a "Discover libraries" button next to the Library ID input;
clicking it POSTs the URL + token to /destinations/discover/abs, which
calls GET /api/libraries and returns an HTML fragment with a <select>.
Picking an entry fills the existing library_id input via an inline
onchange handler.
The existing curl/UI manual instructions stay below the button as a
<details>, so the documented fallback path keeps working.
Adds an exported mediaserver.ListLibraries(ctx, url, token) helper
that handles both the {libraries:[...]} (older ABS) and bare-array
response shapes via a first-byte sniff, and treats an empty body as
an explicit error rather than letting the JSON decoder surface
"unexpected end of JSON input".
The discover handler validates the URL up front: scheme must be http
or https, host must be non-empty. RFC1918 / loopback addresses are
intentionally NOT blocked — Audplexus is built to talk to LAN media
servers and the entire point of the picker is to make that easier.
The scheme allowlist keeps the endpoint from being turned into a
generic fetcher for file:// / gopher:// / etc.
Non-book libraries (podcasts) are hidden from the picker, with a
count surfaced in the success line. If the token can see the server
but no book libraries are visible, the response is a warning rather
than a "Connected." with an empty picker.
Tested with httptest covering both response shapes, empty/whitespace
bodies, non-2xx responses, and malformed JSON.
ABS reads series/series-part/asin freeform atoms via ffprobe to auto-group books into series. With the global tag-profile setting defaulting to Basic, ABS users get downloads without those atoms until they discover the setting — bad UX for an opt-in profile that ABS objectively needs. Override at runtime in metadataWithOptionalCover: if any enabled library_destinations row is type=abs, force the profile to Audiobook-rich regardless of the resolved default. The user-set default still applies for Plex-only setups, preserving the historical album-collapse workaround. DB errors fall back to the resolved default rather than failing the download. Extracted to resolveTagProfileForDownload + a destinationLister interface so the override is unit-testable without faking the whole DownloadManager. Eight table-driven subtests cover: no destinations, plex-only, abs-present, mixed, already-rich, DB error (twice), and the "all abs disabled at DB layer" case that documents reliance on ListEnabledLibraryDestinations's filter.
…h/plex/* handlers The Plex PIN sign-in UI was removed by 4c5c8ee but the backend code was left in place: 7 unreachable POST /auth/plex/* routes, their handlers in plex_auth.go, and an orphaned ~140-line JS block in settings.html that targeted DOM elements that no longer exist (#plex-auth-state, #plex-start-btn, #plex_section_title). This commit deletes the dead surfaces ahead of re-introducing Plex sign-in as a per-destination affordance in a follow-up: - Routes removed from server.go (handlePlexStart/Poll/Complete/Select/ SectionSelect/Scan/Check). - Handler functions deleted along with completePlexLogin, wantsJSON, mustPlexToken, and plexSearchCount — none of these have non-handler callers. - File renamed plex_auth.go -> plex_client.go to reflect that it's now just an HTTP client for plex.tv + a Plex server, not auth-page glue. - Settings.html: dead Plex-auth IIFE removed (~140 lines). The Plex sign-in flow will return on the destination form, not the global settings page. Helpers kept (still called by live code paths in server.go for reconcile / diagnostics / dashboard): plexClientID, plexAuthURL, plexCreatePin, plexGetPin, plexListServerOptions, plexListSections, plexTriggerSectionScan, plexSectionItemCount, plexSectionLocation, plexGetActivities, plexListSectionItems, addPlexHeaders, buildPlexURL, getPlexSettings, and the response struct types. No functional change — every helper that survives kept its signature. go build ./... passes.
Restores the auto-population UX that the multi-destination refactor (commit 4c5c8ee) inadvertently regressed. Plex destinations now have three new affordances on the per-destination form, mirroring the ABS "Discover libraries" pattern (commit 96eda9b): 1. Sign in with Plex — POST /destinations/plex/pin/start mints a PIN on plex.tv and returns an HTML fragment with an "Open plex.tv" button (target=_blank) plus a hidden HTMX poller. The poller hits POST /destinations/plex/pin/poll every 2s; on approval the response is a <script> fragment that fills the plex_token input and clicks the Discover servers button to chain into URL discovery. 2. Discover servers — POST /destinations/plex/discover/servers calls plex.tv /api/v2/resources for the user's owned/shared servers and renders a <select> of connection URIs. Picking one fills the URL field. LAN connections are flagged in the label. 3. Discover sections — POST /destinations/plex/discover/sections calls the configured Plex server's /library/sections and lists all sections (not filtered to type=artist — some audiobook libraries are misconfigured as type=movie, hiding them would strand users). Picking one fills the section_id field. Manual paste still works — token and section_id inputs remain editable, and the existing "How to find your token / section ID" details blocks are kept (re-labelled "Manual:") for users who already have credentials. All discover endpoints have :id variants (POST /destinations/:id/plex/discover/...) that resolve the saved row's plex_token when the edit form leaves it blank, mirroring the secret-carry-over trick destinationForTest uses. All fragments render with role="status" aria-live="polite" so SR users hear discovery outcomes. The PIN-success <script> uses encoding/json for safe inline embedding plus < > & replacements to close the </script> injection path. Reuses the surviving helpers in plex_client.go (plexCreatePin, plexGetPin, plexListServerOptions, plexListSections, plexAuthURL, addPlexHeaders, buildPlexURL) — no new HTTP client code. go build ./... + existing tests pass.
Adds EmbyListLibraries(ctx, baseURL, apiKey) — exported helper that hits
GET /emby/Library/MediaFolders and returns every library visible to the
credentials. Mirrors abs.ListLibraries.
New handler handleDestinationsDiscoverEmby renders a <select> picker:
POST /destinations/discover/emby — form values
POST /destinations/:id/discover/emby — saved row, api_key carried
The picker filters to CollectionType="audiobooks", but falls back to
showing all libraries (with a warning banner) when none match — a
misconfigured server shouldn't strand the user.
Render logic is factored into renderEmbyLikeDiscover() + a generic
mediaServerLibrary{ID, Name, Kind, Path} row shape so the Jellyfin
slice can share it without copy-paste. Picker fills #library_id via
inline onchange, matching the ABS pattern.
Saved-row api_key carry-over via new resolveAPIKeyFromForm() helper
(mirror of resolvePlexTokenFromForm) lets discover work from the edit
form without re-pasting the key.
Template: Emby destinations now have a "Discover libraries" button +
result div, parallel to the existing ABS one. role="status"
aria-live="polite" for SR parity.
go build ./... passes.
Adds JellyfinListLibraries(ctx, baseURL, apiKey) — exported helper that hits GET /Library/VirtualFolders with the canonical `Authorization: MediaBrowser Token="..."` header (not the deprecated X-Emby-Token) and returns every library visible to the credentials. Mirror of EmbyListLibraries. New handler handleDestinationsDiscoverJellyfin reuses the renderEmbyLikeDiscover() helper from the Emby slice — same mediaServer- Library shape, same picker UI, filter is CollectionType="books" (Jellyfin's audiobook collection type) instead of "audiobooks". POST /destinations/discover/jellyfin — form values POST /destinations/:id/discover/jellyfin — saved row, api_key carried Template: Jellyfin destinations now have a "Discover libraries" button parallel to Emby and ABS. Help text for the manual library-ID lookup also corrected to use the modern Authorization: MediaBrowser header (the previous X-Emby-Token instructions were deprecated and will stop working on Jellyfin 12.0). go build ./... + go test ./... pass.
Two-pronged fix for the "two cards both labeled Plex" problem when a user adds a second destination of the same type: 1. Plex server picker now autofills display_name with the discovered server name (e.g. "Living Room Plex") on selection, but only when the display_name field is empty — so a user who typed their own name doesn't get it overwritten every time they change the picker. The server name is carried on data-server-name and is stripped of the trailing " (Plex Media Server)" product suffix that plexListServerOptions adds for the visible disambiguation label. 2. Server-side uniqueDisplayName() appends " (2)", " (3)" … to the candidate when another destination already uses the same name (case-insensitive). Runs on both Create and Update, with excludeID on Update so renaming a destination back to its current value is a no-op instead of bumping it. Bounded at 999 attempts with a uuid-suffix fallback so a pathological collision storm can't infinite-loop; on a ListLibraryDestinations error we return the candidate as-is rather than block creation. Result: a user adding two Plex servers via the sign-in flow gets "Living Room Plex" + "Bedroom Plex" automatically. Falling back to manual configuration on the same type yields "Plex" + "Plex (2)" instead of two identical labels. Emby/Jellyfin/ABS only get the auto-numbering fallback today. Per- backend server-info discovery (e.g. Emby /System/Info → ServerName) is a follow-up if it matters. go build ./... + go test ./... pass.
Independent review of feat/destination-discovery surfaced one broken
edit-path, one leak vector, and a few correctness/cleanup issues.
High:
- ABS Discover button was broken on /destinations/:id/edit — the button
always posted to /destinations/discover/abs (no :id variant) and the
handler read api_key directly via c.PostForm with no saved-row fallback.
Result: edit form started with api_key blank (sensitive fields are
never prefilled) and Discover surfaced "Enter both URL and API token"
unless the user re-pasted the key. Fix: register
POST /destinations/:id/discover/abs, switch handler to
resolveAPIKeyFromForm, and add {{if $isEdit}} URL switch in the
template. Emby/Jellyfin/Plex already did this correctly; ABS was the
outlier.
- Plex PIN-poll success response inlines the auth token verbatim in a
<script> with no cache headers. Any cache (CDN, reverse proxy, browser
bfcache) sitting in front of the response would store the token and
potentially serve it to the wrong user. Added writeSensitiveHTML
helper that sets Cache-Control: no-store + Pragma: no-cache, used on
every Plex PIN response and every discover-result fragment for
symmetry.
- jsString() comment promised <, >, & escaping on top of JSON.
Misleading: Go's json.Marshal defaults to SetEscapeHTML(true), which
already escapes those three to </>/& — the manual
ReplaceAll calls were operating on chars that no longer existed.
Output was safe, code was confusing. Dropped the dead replace calls
and rewrote the comment to explain what's actually defending here.
Medium:
- validateRemoteURL accepted http://user:pass@host. The userinfo
serves no legitimate purpose (the user is about to type the API key
into a dedicated field below) and confuses naive readers about which
host is contacted. Reject with a clear error.
Low:
- stripPlexProductSuffix was a regex-style hack that over-stripped
legitimately parenthesized server names (a server literally named
"Plex (Living Room)" would become "Plex"). Replaced with a clean
carry-through: plexServerOption now has a separate DeviceName field
populated from dev.Name pre-concatenation, used directly for the
display_name autofill. The visible picker label keeps the product
suffix for disambiguation.
- renderEmbyLikeDiscover had a `_ /*unused*/ string` trailing parameter
from earlier scaffolding. Dropped, callers updated.
- Inlined a single-use `token := pin.AuthToken` variable in
handleDestinationsPlexPinPoll.
Build + tests pass.
… click Previously the "Sign in with Plex" button returned a fragment containing an "Open plex.tv sign-in" link the user had to click. Two clicks for one intent is the kind of friction this PR was supposed to remove. Now the response includes an inline <script> that calls window.open directly. Browsers allow popups without a blocker prompt when they happen inside an active user gesture; the user's click on "Sign in with Plex" is still considered active when the HTMX response is parsed (Chrome/Edge/Firefox honor this; Safari is stricter about network-response popups). A visible "If no window opened, open plex.tv sign-in" fallback link covers the case where a popup-blocker intervenes anyway. Push and pop in noopener mode so the popup can't navigate this tab.
http://abs.lan:13378/ and http://abs.lan:13378 used to land in the DB as different strings, even though every backend's buildURL strips the trailing slash before issuing requests. Stored values are now canonical (no trailing slash on the path) so dedup checks and reconcile diffs can rely on string equality. normalizeURL is applied at the five user-facing entry points: destinationFromForm (the save path) and the four discover handlers (Plex sections, ABS, Emby, Jellyfin). Uses url.Parse so a path like /api/ is preserved as /api but trailing slashes alone are stripped.
There was a problem hiding this comment.
Pull request overview
This PR reintroduces “discovery” UX on the per-destination form across all supported media-server backends (Plex/Emby/Jellyfin/ABS), restoring Plex PIN sign-in and adding library/section pickers so users don’t have to manually extract IDs/tokens. It also improves destination naming behavior for multi-destination setups and adds a download-time tag-profile override to ensure ABS users get series-friendly tagging.
Changes:
- Add Plex PIN sign-in (plex.tv) plus server and section discovery directly in the destination form (HTMX fragments).
- Add Emby/Jellyfin “Discover libraries” pickers and fix ABS discover behavior on edit forms.
- Make destination display names optional with automatic disambiguation, and override tag profile to “audiobook-rich” when any ABS destination is enabled.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/web/templates/settings.html | Removes legacy Plex auth JS that was tied to deleted /auth/plex/* routes. |
| internal/web/templates/destinations_form.html | Adds per-backend discovery/sign-in UI elements (HTMX) and makes display name optional. |
| internal/web/server.go | Removes legacy /auth/plex/* routing; adds per-destination discover/sign-in endpoints. |
| internal/web/plex_client.go | Strips legacy Gin handlers; keeps Plex client utilities and expands server option metadata for UI. |
| internal/web/destinations_handlers.go | Implements discovery/sign-in handlers, URL normalization/validation helpers, and display-name deduping. |
| internal/mediaserver/abs.go | Adds ListLibraries helper + ABSLibrary projection for ABS library discovery. |
| internal/mediaserver/abs_list_libraries_test.go | Unit tests for ABS ListLibraries supporting multiple response shapes and error cases. |
| internal/mediaserver/emby.go | Adds EmbyListLibraries + EmbyLibrary for Emby library discovery. |
| internal/mediaserver/jellyfin.go | Adds JellyfinListLibraries + JellyfinLibrary using the MediaBrowser Authorization header. |
| internal/library/tag_profile_override.go | Adds helper to force audiobook-rich tags when any ABS destination is enabled. |
| internal/library/tag_profile_override_test.go | Unit tests covering override behavior and DB error fallback. |
| internal/library/download.go | Uses the new tag-profile override during download metadata generation. |
Comments suppressed due to low confidence (1)
internal/web/destinations_handlers.go:1092
destinationFromFormnormalizes and accepts any non-empty URL, but it doesn’t validate scheme/host or rejectuser:pass@hosteven thoughvalidateRemoteURLexists and the new discover endpoints rely on it. This allows saving malformed/unsupported URLs (or URLs with userinfo) that will later break discover/reconcile paths with less clear errors; consider callingvalidateRemoteURLhere (before persisting) so create/update enforce the same URL constraints.
d := &database.LibraryDestination{
Type: database.LibraryDestinationType(t),
DisplayName: displayName,
URL: normalizeURL(c.PostForm("url")),
AudiobookPath: strings.TrimSpace(c.PostForm("audiobook_path")),
DestinationPath: strings.TrimSpace(c.PostForm("destination_path")),
}
if d.URL == "" {
return nil, errors.New("URL is required")
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+787
to
+792
| func renderDiscoverResult(c *gin.Context, libs []mediaserver.ABSLibrary, errMsg string) { | ||
| c.Header("Content-Type", "text/html; charset=utf-8") | ||
| if errMsg != "" { | ||
| c.String(http.StatusOK, | ||
| `<div class="info-box" style="border-color:var(--error);margin:.5rem 0" role="status" aria-live="polite">`+ | ||
| `<strong>Failed.</strong> `+htmlEscape(errMsg)+`</div>`) |
Comment on lines
+572
to
+574
| // SetEscapeHTML(true) for json.Marshal, which already escapes <, >, | ||
| // and & as < / > / & — so a token containing "</script>" | ||
| // becomes "</script>" and cannot close the script tag. |
Owner
|
Thanks for this btw, I was working on it but you beat me to it lmao. |
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
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.
The multi-destination refactor (3fa9ebe) → (4c5c8ee) dropped the Plex.tv PIN sign-in, the Plex server picker, and the Plex section picker — and Emby/Jellyfin never got the auto-populate treatment that ABS got in (96eda9b). Users were left hand-crafting
curlcommands and pasting UUIDs the app already knows how to fetch.This PR brings discovery back, on the per-destination form, for every backend.
What changes
/api/v2/resources), "Discover sections" (/library/sectionson the chosen server)./emby/Library/MediaFolders, filtered toCollectionType=audiobookswith a fallback to all libraries when none match./Library/VirtualFoldersusing the canonicalAuthorization: MediaBrowser Token="…"header (not the deprecatedX-Emby-Token).:idvariant) so the saved api_key carries over correctly.Manual paste still works on every backend — the discovery buttons sit alongside the existing inputs, and the "How to find your token / section ID / library ID" details blocks remain for users who already have credentials in hand.
Display name disambiguation
Adding two destinations of the same type used to give you two cards both labeled "Plex". Now:
display_namefrom the discovered server name (e.g. "Living Room Plex") when the field is empty — typical sign-in flow never hits the fallback.uniqueDisplayName()appends(2),(3), … on Create and Update when another row already has the same name (case-insensitive). Renaming a destination back to its current value is a no-op viaexcludeID.Cleanup
The 7 unreachable
/auth/plex/*routes and ~140 lines of orphaned JS insettings.htmlare removed.plex_auth.gois renamed toplex_client.go— what survives is just the HTTP client for plex.tv + a Plex server, still used by reconcile, diagnostics, and the dashboard.Commits
plex_auth.go→plex_client.go, drop dead/auth/plex/*handlers and orphaned JS.mediaServerLibrary{ID,Name,Kind,Path}shape +renderEmbyLikeDiscoverhelper.X-Emby-Tokenhelp text to useAuthorization: MediaBrowser.Cache-Control: no-storeto PIN/discover responses, rejectuser:pass@hostURLs, clean up over-zealous suffix stripping, drop an unused parameter.Implementation notes
outerHTML); on plex.tv approval the response is a small<script>that fills#plex_tokenand clicks the Discover servers button to chain into URL discovery.writeSensitiveHTMLhelper that setsCache-Control: no-store+Pragma: no-cache— defense against any cache (CDN, reverse proxy, browser bfcache) storing tokens minted for one user and serving them to another.jsStringuses Go'sjson.Marshalfor safe JS-literal embedding;SetEscapeHTML(true)(the default) handles<>&so a token containing</script>becomes\u003c/script\u003eand cannot close the script tag.role="status" aria-live="polite"for SR parity. The Plex sign-in popup opens viatarget="_blank" rel="noopener noreferrer".type=artist) for the same reason.resolvePlexTokenFromForm/resolveAPIKeyFromFormmirror the existingdestinationForTestpattern so Discover works without re-pasting the token.validateRemoteURLis reused across every new discover endpoint — http(s) schemes only, non-empty host, nouser:pass@userinfo. RFC1918 / loopback is explicitly allowed (LAN media servers are the use case).