From 460d4fd4d5aa1dc28d946f8bfaae5214d695e9db Mon Sep 17 00:00:00 2001 From: RXWatcher <14085001+RXWatcher@users.noreply.github.com> Date: Tue, 26 May 2026 10:29:48 +0200 Subject: [PATCH] refactor(audiobooks): minor review-cleanup pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - loginEnvelope: add a comment explaining the displayName→userID fallback. - handleRefresh: stop leaking internal err detail in 500 responses; log via slog and return a sanitized "token mint/persist/rotation failed". - handlePlayStart: document why no "progress" field on playbackSession (canonical omits it; spec was over-specified). - Extract resolveDefaultLibrary helper to dedupe the "first-audiobook-lib-else-virtual" snippet from three handlers. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/audiobooks/abs/items_handler.go | 59 +++++++++++------------- internal/audiobooks/abs/login.go | 19 ++++++-- internal/audiobooks/abs/play_response.go | 7 +++ 3 files changed, 49 insertions(+), 36 deletions(-) diff --git a/internal/audiobooks/abs/items_handler.go b/internal/audiobooks/abs/items_handler.go index daf518fa..fe0cc274 100644 --- a/internal/audiobooks/abs/items_handler.go +++ b/internal/audiobooks/abs/items_handler.go @@ -1,11 +1,24 @@ package abs import ( + "context" "net/http" "github.com/go-chi/chi/v5" ) +// resolveDefaultLibrary returns the first audiobook library (the canonical +// "default" for response embedding) or a virtual fallback when the store +// is empty or errors. Centralizes the snippet that handleItem, +// handleSimilarItems, and handleItemsInProgress all need so the fallback +// shape stays consistent across all three response paths. +func (h *Handler) resolveDefaultLibrary(ctx context.Context) AudiobookLibrary { + if libs, err := h.deps.MediaStore.ListAudiobookLibraries(ctx); err == nil && len(libs) > 0 { + return libs[0] + } + return AudiobookLibrary{ID: 0, Name: VirtualLibraryName, Type: "audiobooks"} +} + // handleItem — GET /abs/api/items/{id} (and /api/items/{id}) // // Returns the full ABS LibraryItem with audio track details for the given @@ -37,16 +50,7 @@ func (h *Handler) handleItem(w http.ResponseWriter, r *http.Request) { return } - // Resolve the library this item belongs to (first matching audiobooks lib - // or the virtual fallback). Best-effort: if the list call fails we - // proceed with a zero-ID virtual library so the item still renders. - var lib AudiobookLibrary - if libs, lerr := h.deps.MediaStore.ListAudiobookLibraries(r.Context()); lerr == nil && len(libs) > 0 { - lib = libs[0] - } else { - lib = AudiobookLibrary{ID: 0, Name: VirtualLibraryName, Type: "audiobooks"} - } - + lib := h.resolveDefaultLibrary(r.Context()) baseURL := h.absBaseURL(r) result := siloItemToLibraryItemDetail(item, files, lib, baseURL) writeJSON(w, http.StatusOK, result) @@ -54,9 +58,11 @@ func (h *Handler) handleItem(w http.ResponseWriter, r *http.Request) { // handleSimilarItems — GET /abs/api/items/{id}/similar // -// Returns a list of similar audiobooks. Uses the optional Recommender if -// wired; otherwise returns an empty list so the ABS client falls back to -// its default UI rather than crashing. +// Returns similar audiobooks in the canonical ABS paged envelope so +// mobile clients can render the "Similar" rail. Sort metadata is +// "relevance" desc to match continuum-plugin-audiobooks; the envelope +// is emitted even when empty so clients that iterate +// `results`/`total` don't crash. func (h *Handler) handleSimilarItems(w http.ResponseWriter, r *http.Request) { a, ok := absAuthFrom(r) if !ok || a.UserID == "" { @@ -70,24 +76,21 @@ func (h *Handler) handleSimilarItems(w http.ResponseWriter, r *http.Request) { return } + const limit = 10 + emptyEnvelope := pagedEnvelope([]any{}, 0, limit, 0, "relevance", true, "", false, "") + if h.deps.Recommender == nil { - writeJSON(w, http.StatusOK, map[string]any{"libraryItems": []any{}}) + writeJSON(w, http.StatusOK, emptyEnvelope) return } - ids, err := h.deps.Recommender.Similar(r.Context(), contentID, 10) + ids, err := h.deps.Recommender.Similar(r.Context(), contentID, limit) if err != nil || len(ids) == 0 { - writeJSON(w, http.StatusOK, map[string]any{"libraryItems": []any{}}) + writeJSON(w, http.StatusOK, emptyEnvelope) return } - var lib AudiobookLibrary - if libs, lerr := h.deps.MediaStore.ListAudiobookLibraries(r.Context()); lerr == nil && len(libs) > 0 { - lib = libs[0] - } else { - lib = AudiobookLibrary{ID: 0, Name: VirtualLibraryName, Type: "audiobooks"} - } - + lib := h.resolveDefaultLibrary(r.Context()) baseURL := h.absBaseURL(r) out := make([]LibraryItem, 0, len(ids)) for _, id := range ids { @@ -97,7 +100,7 @@ func (h *Handler) handleSimilarItems(w http.ResponseWriter, r *http.Request) { } out = append(out, siloItemToLibraryItem(si, lib, baseURL)) } - writeJSON(w, http.StatusOK, map[string]any{"libraryItems": out}) + writeJSON(w, http.StatusOK, pagedEnvelope(out, len(out), limit, 0, "relevance", true, "", false, "")) } // handleItemsInProgress — GET /abs/api/me/items-in-progress @@ -123,13 +126,7 @@ func (h *Handler) handleItemsInProgress(w http.ResponseWriter, r *http.Request) return } - var lib AudiobookLibrary - if libs, lerr := h.deps.MediaStore.ListAudiobookLibraries(r.Context()); lerr == nil && len(libs) > 0 { - lib = libs[0] - } else { - lib = AudiobookLibrary{ID: 0, Name: VirtualLibraryName, Type: "audiobooks"} - } - + lib := h.resolveDefaultLibrary(r.Context()) baseURL := h.absBaseURL(r) items := make([]any, 0, len(rows)) for _, p := range rows { diff --git a/internal/audiobooks/abs/login.go b/internal/audiobooks/abs/login.go index 8b0e9413..c041f748 100644 --- a/internal/audiobooks/abs/login.go +++ b/internal/audiobooks/abs/login.go @@ -220,6 +220,10 @@ func (h *Handler) loginEnvelope( r *http.Request, userID, displayName, accessToken, refreshToken string, ) map[string]any { + // displayName falls back to userID when the validator didn't supply one + // (host-proxied logins without X-Silo-Profile-Name, /authorize re-mints + // that have no name claim in the JWT). ABS clients require a non-empty + // username on the user envelope. name := displayName if name == "" { name = userID @@ -418,12 +422,14 @@ func (h *Handler) handleRefresh(w http.ResponseWriter, r *http.Request) { newRefreshJTI := ulid.Make().String() access, err := IssueAccessToken(secret, claims.UserID, claims.ProfileID, newAccessJTI, accessTTL) if err != nil { - http.Error(w, "token mint failed: "+err.Error(), http.StatusInternalServerError) + slog.Error("abs refresh: mint access failed", "user", claims.UserID, "err", err) + http.Error(w, "token mint failed", http.StatusInternalServerError) return } refresh, err := IssueRefreshToken(secret, claims.UserID, claims.ProfileID, newRefreshJTI, refreshTTL) if err != nil { - http.Error(w, "token mint failed: "+err.Error(), http.StatusInternalServerError) + slog.Error("abs refresh: mint refresh failed", "user", claims.UserID, "err", err) + http.Error(w, "token mint failed", http.StatusInternalServerError) return } now := time.Now() @@ -431,18 +437,21 @@ func (h *Handler) handleRefresh(w http.ResponseWriter, r *http.Request) { ID: newAccessJTI, UserID: claims.UserID, ProfileID: claims.ProfileID, JTI: newAccessJTI, ExpiresAt: now.Add(accessTTL), }); err != nil { - http.Error(w, "token persist failed: "+err.Error(), http.StatusInternalServerError) + slog.Error("abs refresh: persist access failed", "user", claims.UserID, "jti", newAccessJTI, "err", err) + http.Error(w, "token persist failed", http.StatusInternalServerError) return } if err := h.deps.TokenStore.InsertToken(r.Context(), ABSToken{ ID: newRefreshJTI, UserID: claims.UserID, ProfileID: claims.ProfileID, JTI: newRefreshJTI, ExpiresAt: now.Add(refreshTTL), }); err != nil { - http.Error(w, "token persist failed: "+err.Error(), http.StatusInternalServerError) + slog.Error("abs refresh: persist refresh failed", "user", claims.UserID, "jti", newRefreshJTI, "err", err) + http.Error(w, "token persist failed", http.StatusInternalServerError) return } if err := h.deps.TokenStore.RevokeTokenByJTI(r.Context(), claims.JTI); err != nil { - http.Error(w, "token rotation failed: "+err.Error(), http.StatusInternalServerError) + slog.Error("abs refresh: revoke old failed", "user", claims.UserID, "old_jti", claims.JTI, "err", err) + http.Error(w, "token rotation failed", http.StatusInternalServerError) return } diff --git a/internal/audiobooks/abs/play_response.go b/internal/audiobooks/abs/play_response.go index 82acf477..aea8621a 100644 --- a/internal/audiobooks/abs/play_response.go +++ b/internal/audiobooks/abs/play_response.go @@ -115,6 +115,13 @@ func (h *Handler) handlePlayStart(w http.ResponseWriter, r *http.Request) { // currentTime seeds the audio element's initial position so cross-device // resume works. Lookup is best-effort: any error returns position 0, // which is always correct for a first listen. + // + // Note: we deliberately do NOT emit a "progress" (0.0-1.0) field on the + // play session response. The canonical continuum-plugin handler omits it + // too — "progress" belongs to /me/progress responses, not playbackSession. + // The spec's Phase 0 row mentioning "currentTime AND progress fields" was + // over-specified; matching the canonical wire shape is the load-bearing + // requirement. var currentTime float64 currentTime, err = resolveResumeTime(r.Context(), h.deps.ProgressStore, a.UserID, a.ProfileID, contentID) if err != nil {