From f10536165842f231577dc8fbf8eeb654d02c8d55 Mon Sep 17 00:00:00 2001 From: Quick <31828688+Quick104@users.noreply.github.com> Date: Fri, 29 May 2026 11:28:59 -0400 Subject: [PATCH] fix(sections): harden trending refresher per PR review - Interleave Trakt movies/shows by rank so the mixed row shows both types instead of burying all series past the display limit. - Treat any Trakt sub-fetch failure as fatal (errors.Join) so a partial result never overwrites the last-good snapshot with a media type missing. - Skip non-title entries (TMDB trending/all returns media_type "person") in both ID batching and ordering so they can't match an unrelated library title. - Guard the refresh task against a nil refresher. - Tests: person skip, Trakt interleave, Trakt partial-failure preserves last-good, snapshot read error propagation. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/sections/fetcher.go | 11 ++- internal/sections/trending_read_test.go | 13 +++ internal/sections/trending_refresher.go | 49 +++++++--- internal/sections/trending_refresher_test.go | 96 +++++++++++++++++++ .../tasks/refresh_trending_discover.go | 5 + 5 files changed, 160 insertions(+), 14 deletions(-) diff --git a/internal/sections/fetcher.go b/internal/sections/fetcher.go index 80bf526c..279f809f 100644 --- a/internal/sections/fetcher.go +++ b/internal/sections/fetcher.go @@ -2432,10 +2432,17 @@ func orderedTrendingContentIDs(entries []trendingDiscoverEntry, movieLookup, ser seen := make(map[string]struct{}, len(entries)) out := make([]string, 0, len(entries)) for _, e := range entries { - lookup := movieLookup + // Only movies and series are resolvable; skip anything else (TMDB + // trending/all also returns media_type "person"). + var lookup *catalog.ExternalIDLookup isSeries := e.mediaType == "tv" - if isSeries { + switch e.mediaType { + case "movie": + lookup = movieLookup + case "tv": lookup = seriesLookup + default: + continue } if lookup == nil { continue diff --git a/internal/sections/trending_read_test.go b/internal/sections/trending_read_test.go index 6c1e97a7..5743405d 100644 --- a/internal/sections/trending_read_test.go +++ b/internal/sections/trending_read_test.go @@ -2,6 +2,7 @@ package sections import ( "context" + "errors" "testing" ) @@ -50,3 +51,15 @@ func TestLoadTrendingDiscoverContentIDsNotFound(t *testing.T) { t.Fatalf("ids = %v; want nil when no snapshot exists", ids) } } + +func TestLoadTrendingDiscoverContentIDsPropagatesError(t *testing.T) { + boom := errors.New("boom") + f := &Fetcher{TrendingSnapshots: fakeSnapshotGetter{err: boom}} + ids, err := f.loadTrendingDiscoverContentIDs(context.Background(), "tmdb", "week") + if !errors.Is(err, boom) { + t.Fatalf("err = %v; want boom", err) + } + if ids != nil { + t.Fatalf("ids = %v; want nil on error", ids) + } +} diff --git a/internal/sections/trending_refresher.go b/internal/sections/trending_refresher.go index 245aa859..88a00892 100644 --- a/internal/sections/trending_refresher.go +++ b/internal/sections/trending_refresher.go @@ -3,6 +3,7 @@ package sections import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "time" @@ -189,19 +190,18 @@ func (r *TrendingRefresher) fetchEntries(ctx context.Context, source, window str if r.TraktTrending == nil { return nil, nil } + // Trakt has no mixed endpoint; fetch movies + shows separately. Treat ANY + // failure as fatal so a partial result never overwrites the last-good + // snapshot with one media type missing. movies, movieErr := r.TraktTrending.GetCollectionPreset(ctx, "trending", "movie", fetchLimit, "") shows, showErr := r.TraktTrending.GetCollectionPreset(ctx, "trending", "tv", fetchLimit, "") - if movieErr != nil && showErr != nil { - return nil, fmt.Errorf("trakt trending: %w / %w", movieErr, showErr) + if movieErr != nil || showErr != nil { + return nil, fmt.Errorf("trakt trending: %w", errors.Join(movieErr, showErr)) } - out := make([]trendingDiscoverEntry, 0, len(movies)+len(shows)) - for _, e := range movies { - out = append(out, newTrendingEntry(e.TMDBID, e.TVDBID, e.IMDbID, e.MediaType)) - } - for _, e := range shows { - out = append(out, newTrendingEntry(e.TMDBID, e.TVDBID, e.IMDbID, e.MediaType)) - } - return out, nil + // Interleave by rank so the mixed row actually shows both movies and + // series; plain concatenation would bury all series past the display + // limit whenever enough movies match the library. + return interleaveTraktEntries(movies, shows), nil } if r.TMDBTrending == nil { @@ -218,6 +218,24 @@ func (r *TrendingRefresher) fetchEntries(ctx context.Context, source, window str return out, nil } +// interleaveTraktEntries alternates rank-ordered movies and shows so the mixed +// trending row surfaces both media types. Each input list is already in trending +// order; alternating preserves that order within each type while mixing them. +func interleaveTraktEntries(movies, shows []catalog.TraktCollectionEntry) []trendingDiscoverEntry { + out := make([]trendingDiscoverEntry, 0, len(movies)+len(shows)) + for i := 0; i < len(movies) || i < len(shows); i++ { + if i < len(movies) { + m := movies[i] + out = append(out, newTrendingEntry(m.TMDBID, m.TVDBID, m.IMDbID, m.MediaType)) + } + if i < len(shows) { + s := shows[i] + out = append(out, newTrendingEntry(s.TMDBID, s.TVDBID, s.IMDbID, s.MediaType)) + } + } + return out +} + // resolveIDs matches trending entries to library content IDs via two batched // external-ID lookups (movies, series), preserving trending order. func (r *TrendingRefresher) resolveIDs(ctx context.Context, entries []trendingDiscoverEntry) ([]string, error) { @@ -226,9 +244,16 @@ func (r *TrendingRefresher) resolveIDs(ctx context.Context, entries []trendingDi } var movieBatch, seriesBatch catalog.ExternalIDBatch for _, e := range entries { - batch := &movieBatch - if e.mediaType == "tv" { + var batch *catalog.ExternalIDBatch + switch e.mediaType { + case "movie": + batch = &movieBatch + case "tv": batch = &seriesBatch + default: + // Skip non-title entries (TMDB trending/all can return "person") so + // they never match an unrelated library title by shared external ID. + continue } if e.tmdbID != "" { batch.TMDBIDs = append(batch.TMDBIDs, e.tmdbID) diff --git a/internal/sections/trending_refresher_test.go b/internal/sections/trending_refresher_test.go index 4ef6bb89..2f9bef86 100644 --- a/internal/sections/trending_refresher_test.go +++ b/internal/sections/trending_refresher_test.go @@ -59,6 +59,18 @@ func (f fakeTMDB) GetCollectionPreset(context.Context, string, string, string, i return f.entries, f.err } +type fakeTrakt struct { + byMediaType map[string][]catalog.TraktCollectionEntry + errByType map[string]error +} + +func (f fakeTrakt) GetCollectionPreset(_ context.Context, _, mediaType string, _ int, _ string) ([]catalog.TraktCollectionEntry, error) { + if err := f.errByType[mediaType]; err != nil { + return nil, err + } + return f.byMediaType[mediaType], nil +} + type fakeResolver struct { byType map[string]*catalog.ExternalIDLookup } @@ -176,6 +188,90 @@ func TestRefresherEmptyProviderPreservesLastGood(t *testing.T) { } } +func TestRefresherSkipsPersonEntries(t *testing.T) { + store := newFakeSnapshotStore() + r := &TrendingRefresher{ + Sections: fakeSectionLister{configs: []json.RawMessage{tmdbConfig(t, "tmdb", "week")}}, + Snapshots: store, + Resolver: fakeResolver{byType: map[string]*catalog.ExternalIDLookup{ + // "99" is present in the movie lookup to simulate a person ID that + // collides with an unrelated library movie's TMDB ID. + "movie": {ByTMDB: map[string]string{"10": "c-movie", "99": "c-person-collision"}, ByIMDb: map[string]string{}, ByTVDB: map[string]string{}}, + "series": {ByTMDB: map[string]string{"20": "c-series"}, ByIMDb: map[string]string{}, ByTVDB: map[string]string{}}, + }}, + TMDBTrending: fakeTMDB{entries: []catalog.TMDBCollectionEntry{ + {ID: 10, MediaType: "movie"}, + {ID: 99, MediaType: "person"}, + {ID: 20, MediaType: "tv"}, + }}, + Clock: recipes.FixedClock(time.Date(2026, 5, 29, 12, 0, 0, 0, time.UTC)), + } + + if _, err := r.RunOnce(context.Background()); err != nil { + t.Fatalf("RunOnce: %v", err) + } + + got := store.saved["tmdb|week"].contentIDs + want := []string{"c-movie", "c-series"} + if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] { + t.Fatalf("content IDs = %v; want %v (person entry must be skipped)", got, want) + } +} + +func TestRefresherTraktInterleavesMoviesAndShows(t *testing.T) { + store := newFakeSnapshotStore() + r := &TrendingRefresher{ + Sections: fakeSectionLister{configs: []json.RawMessage{tmdbConfig(t, "trakt", "week")}}, + Snapshots: store, + Resolver: fakeResolver{byType: map[string]*catalog.ExternalIDLookup{ + "movie": {ByTMDB: map[string]string{"1": "m1", "2": "m2"}, ByIMDb: map[string]string{}, ByTVDB: map[string]string{}}, + "series": {ByTMDB: map[string]string{"3": "s1"}, ByIMDb: map[string]string{}, ByTVDB: map[string]string{}}, + }}, + TraktTrending: fakeTrakt{byMediaType: map[string][]catalog.TraktCollectionEntry{ + "movie": {{TMDBID: 1, MediaType: "movie"}, {TMDBID: 2, MediaType: "movie"}}, + "tv": {{TMDBID: 3, MediaType: "tv"}}, + }}, + Clock: recipes.FixedClock(time.Date(2026, 5, 29, 12, 0, 0, 0, time.UTC)), + } + + if _, err := r.RunOnce(context.Background()); err != nil { + t.Fatalf("RunOnce: %v", err) + } + + // Interleaved order: movie[0], show[0], movie[1] => m1, s1, m2. A plain + // concat would have buried s1 after all movies. + got := store.saved["trakt|week"].contentIDs + want := []string{"m1", "s1", "m2"} + if len(got) != len(want) || got[0] != want[0] || got[1] != want[1] || got[2] != want[2] { + t.Fatalf("content IDs = %v; want %v (interleaved)", got, want) + } +} + +func TestRefresherTraktPartialFailurePreservesLastGood(t *testing.T) { + store := newFakeSnapshotStore() + r := &TrendingRefresher{ + Sections: fakeSectionLister{configs: []json.RawMessage{tmdbConfig(t, "trakt", "week")}}, + Snapshots: store, + Resolver: fakeResolver{}, + TraktTrending: fakeTrakt{ + byMediaType: map[string][]catalog.TraktCollectionEntry{"movie": {{TMDBID: 1, MediaType: "movie"}}}, + errByType: map[string]error{"tv": errors.New("trakt shows 500")}, + }, + Clock: recipes.FixedClock(time.Date(2026, 5, 29, 12, 0, 0, 0, time.UTC)), + } + + if _, err := r.RunOnce(context.Background()); err != nil { + t.Fatalf("RunOnce: %v", err) + } + + if _, ok := store.saved["trakt|week"]; ok { + t.Fatal("SaveSuccess must not run when one Trakt sub-fetch fails (would drop a media type)") + } + if store.attempts["trakt|week"].status != "error" { + t.Fatalf("attempt status = %q; want error", store.attempts["trakt|week"].status) + } +} + func TestDistinctTrendingCombosCollapsesTrakt(t *testing.T) { configs := []json.RawMessage{ tmdbConfig(t, "trakt", "day"), diff --git a/internal/taskmanager/tasks/refresh_trending_discover.go b/internal/taskmanager/tasks/refresh_trending_discover.go index 7324d517..3d9a3941 100644 --- a/internal/taskmanager/tasks/refresh_trending_discover.go +++ b/internal/taskmanager/tasks/refresh_trending_discover.go @@ -3,6 +3,7 @@ package tasks import ( "context" "encoding/json" + "errors" "fmt" "github.com/Silo-Server/silo-server/internal/taskmanager" @@ -47,6 +48,10 @@ func (t *RefreshTrendingDiscoverTask) DefaultTriggers() []taskmanager.TriggerCon func (t *RefreshTrendingDiscoverTask) Execute(ctx context.Context, progress taskmanager.ProgressReporter) error { progress.Report(0, "Refreshing trending discover") + if t.refresher == nil { + return errors.New("trending discover refresh: refresher not configured") + } + resultData, err := t.refresher.RunOnce(ctx) if err != nil { return fmt.Errorf("trending discover refresh: %w", err)