From 3353456d469b73b8933bde4ee4aae6ddb78cfdce Mon Sep 17 00:00:00 2001 From: d3v1l1989 <64855033+d3v1l1989@users.noreply.github.com> Date: Sun, 7 Jun 2026 17:42:44 +0200 Subject: [PATCH] fix(jellycompat): aggregate series watch state from episode progress (#70) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A series never has a user_watch_progress row of its own, so series items always rendered Played:false with no UnplayedItemCount — every show looked unwatched in compat clients' library views, and the IsPlayed browse filter treated all series as unplayed. Roll episode progress up to series rows in the browse/search/detail paths, mirroring applySeasonUserData and the native API's rollup semantics (WatchedCount/UnplayedCount/InProgressCount). Progress lookups are chunked at 500 ids (per-user stores may be SQLite-backed) and the rollup is capped at 250 series per page so max-limit browses of series-only libraries stay bounded. --- internal/jellycompat/content_direct.go | 146 ++++++++++++++++++ .../jellycompat/list_seasons_batch_test.go | 4 + internal/jellycompat/series_userdata_test.go | 111 +++++++++++++ 3 files changed, 261 insertions(+) create mode 100644 internal/jellycompat/series_userdata_test.go diff --git a/internal/jellycompat/content_direct.go b/internal/jellycompat/content_direct.go index 79bfb9a2..415fed95 100644 --- a/internal/jellycompat/content_direct.go +++ b/internal/jellycompat/content_direct.go @@ -61,6 +61,7 @@ type seasonListSource interface { type episodeListSource interface { ListBySeason(ctx context.Context, seriesID string, seasonNum int) ([]*models.Episode, error) ListBySeriesGroupedBySeason(ctx context.Context, seriesID string) (map[int][]*models.Episode, error) + ListBySeriesIDs(ctx context.Context, seriesIDs []string) (map[string][]*models.Episode, error) } // directContentService implements ContentService by calling catalog repos directly. @@ -265,6 +266,18 @@ func (s *directContentService) BrowseItems(ctx context.Context, session *Session if len(collected) > requestedLimit { collected = collected[:requestedLimit] } + + // The handler's resolveUserStateForContentIDs overlay only covers items + // with their own progress rows, which a series never has — aggregate + // series watch state here so library views carry Played/UnplayedItemCount. + // The is_played path already did this via enrichListItemsUserData. + // That same absence is what keeps this safe from the handler overlay: + // userDataDTO only overrides item-level UserData when a direct progress + // row exists, and one never does for a series id. + if isPlayedFilter == "" { + s.enrichSeriesUserData(ctx, session, collected) + } + if totalKnown { hasMore = totalFromCatalog > requestedOffset+requestedLimit } @@ -347,6 +360,17 @@ func (s *directContentService) GetItemDetail(ctx context.Context, session *Sessi IsInProgress: progress.PositionSeconds > 0 && !progress.Completed, } } + + // A series never has a progress row of its own, so roll watch + // state up from its episodes (mirrors applySeasonUserData) to + // give clients Played/UnplayedItemCount at the series level. + if result.UserData == nil && strings.EqualFold(result.Type, "series") && s.episodeRepo != nil { + if episodesBySeries, epErr := s.episodeRepo.ListBySeriesIDs(ctx, []string{contentID}); epErr == nil { + episodes := episodesBySeries[contentID] + progressMap := chunkedProgressByMediaItems(ctx, store, session.ProfileID, modelEpisodeContentIDs(episodes)) + result.UserData = seriesUserDataFromEpisodes(episodes, progressMap) + } + } } } @@ -545,6 +569,128 @@ func (s *directContentService) enrichListItemsUserData(ctx context.Context, sess items[i].UserData = seasonUserDataFromProgress(progress) } } + + s.enrichSeriesListUserData(ctx, session, store, items) +} + +// enrichSeriesUserData is enrichSeriesListUserData with store acquisition, +// for callers that haven't already resolved the user store. +func (s *directContentService) enrichSeriesUserData(ctx context.Context, session *Session, items []upstreamListItem) { + if s.storeProvider == nil || len(items) == 0 { + return + } + store, err := s.userStore(ctx, session) + if err != nil { + return + } + s.enrichSeriesListUserData(ctx, session, store, items) +} + +// enrichSeriesListUserData fills aggregated user data for series rows in a +// list response. A series never has a progress row of its own, so Played and +// UnplayedItemCount are rolled up from episode progress, mirroring +// applySeasonUserData semantics. +func (s *directContentService) enrichSeriesListUserData(ctx context.Context, session *Session, store userstore.UserStore, items []upstreamListItem) { + if s.episodeRepo == nil { + return + } + seriesIDs := make([]string, 0, len(items)) + for i := range items { + if items[i].UserData == nil && items[i].ContentID != "" && strings.EqualFold(items[i].Type, "series") { + seriesIDs = append(seriesIDs, items[i].ContentID) + } + } + if len(seriesIDs) == 0 { + return + } + // Cap the rollup so a max-limit browse (compatBrowseMaxLimit) of a + // series-only library can't materialize hundreds of thousands of episode + // rows in one request. Series past the cap keep a nil UserData — the same + // shape they had before series aggregation existed. Real clients page at + // 20-100 items, well under the cap. + const maxSeriesUserDataRollups = 250 + if len(seriesIDs) > maxSeriesUserDataRollups { + seriesIDs = seriesIDs[:maxSeriesUserDataRollups] + } + episodesBySeries, err := s.episodeRepo.ListBySeriesIDs(ctx, seriesIDs) + if err != nil { + return + } + episodeIDs := make([]string, 0, 256) + for _, episodes := range episodesBySeries { + episodeIDs = append(episodeIDs, modelEpisodeContentIDs(episodes)...) + } + progressMap := chunkedProgressByMediaItems(ctx, store, session.ProfileID, episodeIDs) + for i := range items { + if items[i].UserData != nil || !strings.EqualFold(items[i].Type, "series") { + continue + } + if episodes, ok := episodesBySeries[items[i].ContentID]; ok { + items[i].UserData = seriesUserDataFromEpisodes(episodes, progressMap) + } + } +} + +// seriesUserDataFromEpisodes computes WatchedCount/UnplayedCount/ +// InProgressCount/Played for a whole series from a pre-fetched progressMap. +// Pure function — no I/O. Counting semantics match the native API's series +// rollup (all episodes including specials; in-progress = started but not +// completed). +func seriesUserDataFromEpisodes(episodes []*models.Episode, progressMap map[string]userstore.WatchProgress) *catalog.SeasonUserData { + watched := 0 + unplayed := 0 + inProgress := 0 + for _, ep := range episodes { + if ep == nil { + continue + } + progress, ok := progressMap[ep.ContentID] + if ok && progress.Completed { + watched++ + continue + } + if ok && progress.PositionSeconds > 0 { + inProgress++ + } + unplayed++ + } + return &catalog.SeasonUserData{ + WatchedCount: watched, + UnplayedCount: unplayed, + InProgressCount: inProgress, + Played: unplayed == 0 && len(episodes) > 0, + } +} + +// modelEpisodeContentIDs returns the non-empty content ids of the given episodes. +func modelEpisodeContentIDs(episodes []*models.Episode) []string { + ids := make([]string, 0, len(episodes)) + for _, ep := range episodes { + if ep != nil && ep.ContentID != "" { + ids = append(ids, ep.ContentID) + } + } + return ids +} + +// chunkedProgressByMediaItems batches ListProgressByMediaItems calls when a +// series list expands to thousands of episode ids: per-user stores may be +// SQLite-backed (999 bind-variable default), and chunking also keeps +// Postgres IN-lists at a sane size. Returns an empty map (never nil); chunks +// that error are skipped. +func chunkedProgressByMediaItems(ctx context.Context, store userstore.UserStore, profileID string, mediaItemIDs []string) map[string]userstore.WatchProgress { + const chunkSize = 500 + result := make(map[string]userstore.WatchProgress, len(mediaItemIDs)) + for start := 0; start < len(mediaItemIDs); start += chunkSize { + chunk, err := store.ListProgressByMediaItems(ctx, profileID, mediaItemIDs[start:min(start+chunkSize, len(mediaItemIDs))]) + if err != nil { + continue + } + for id, progress := range chunk { + result[id] = progress + } + } + return result } // enrichSeasonUserData adds aggregated user data for a season. Used by diff --git a/internal/jellycompat/list_seasons_batch_test.go b/internal/jellycompat/list_seasons_batch_test.go index 33cd5629..9a84a444 100644 --- a/internal/jellycompat/list_seasons_batch_test.go +++ b/internal/jellycompat/list_seasons_batch_test.go @@ -65,6 +65,10 @@ func (c *countingEpisodeListSource) ListBySeriesGroupedBySeason(_ context.Contex return out, nil } +func (c *countingEpisodeListSource) ListBySeriesIDs(_ context.Context, _ []string) (map[string][]*models.Episode, error) { + return map[string][]*models.Episode{}, nil +} + // TestListSeasons_UsesBatchEpisodeFetch verifies ListSeasons makes exactly // one ListBySeriesGroupedBySeason call (replacing N per-season ListBySeason // calls) and exactly one ListProgressByMediaItems call (replacing N diff --git a/internal/jellycompat/series_userdata_test.go b/internal/jellycompat/series_userdata_test.go new file mode 100644 index 00000000..557bbaa6 --- /dev/null +++ b/internal/jellycompat/series_userdata_test.go @@ -0,0 +1,111 @@ +package jellycompat + +import ( + "testing" + + "github.com/Silo-Server/silo-server/internal/models" + "github.com/Silo-Server/silo-server/internal/userstore" +) + +func ep(id string) *models.Episode { + return &models.Episode{ContentID: id} +} + +// TestSeriesUserDataFromEpisodes covers the series watch-state rollup: a +// series has no progress row of its own, so Played/UnplayedCount/ +// InProgressCount are aggregated from per-episode progress. +func TestSeriesUserDataFromEpisodes(t *testing.T) { + cases := []struct { + name string + episodes []*models.Episode + progress map[string]userstore.WatchProgress + wantWatched int + wantUnplayed int + wantInProgress int + wantPlayed bool + }{ + { + name: "no episodes", + episodes: nil, + progress: map[string]userstore.WatchProgress{}, + // Played must be false for an empty series — unplayed==0 alone + // must not flip it. + wantPlayed: false, + }, + { + name: "all watched", + episodes: []*models.Episode{ep("a"), ep("b")}, + progress: map[string]userstore.WatchProgress{ + "a": {Completed: true}, + "b": {Completed: true}, + }, + wantWatched: 2, + wantPlayed: true, + }, + { + name: "mixed watched, in-progress, untouched", + episodes: []*models.Episode{ep("a"), ep("b"), ep("c")}, + progress: map[string]userstore.WatchProgress{ + "a": {Completed: true}, + "b": {PositionSeconds: 120}, + }, + wantWatched: 1, + wantUnplayed: 2, + wantInProgress: 1, + wantPlayed: false, + }, + { + name: "no progress at all", + episodes: []*models.Episode{ep("a")}, + progress: map[string]userstore.WatchProgress{}, + wantUnplayed: 1, + wantPlayed: false, + }, + { + name: "nil episodes skipped", + episodes: []*models.Episode{nil, ep("a"), nil}, + progress: map[string]userstore.WatchProgress{ + "a": {Completed: true}, + }, + wantWatched: 1, + wantPlayed: true, + }, + { + name: "zero-position progress row is not in-progress", + episodes: []*models.Episode{ep("a")}, + progress: map[string]userstore.WatchProgress{ + "a": {PositionSeconds: 0}, + }, + wantUnplayed: 1, + wantInProgress: 0, + wantPlayed: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := seriesUserDataFromEpisodes(tc.episodes, tc.progress) + if got.WatchedCount != tc.wantWatched { + t.Errorf("WatchedCount = %d, want %d", got.WatchedCount, tc.wantWatched) + } + if got.UnplayedCount != tc.wantUnplayed { + t.Errorf("UnplayedCount = %d, want %d", got.UnplayedCount, tc.wantUnplayed) + } + if got.InProgressCount != tc.wantInProgress { + t.Errorf("InProgressCount = %d, want %d", got.InProgressCount, tc.wantInProgress) + } + if got.Played != tc.wantPlayed { + t.Errorf("Played = %v, want %v", got.Played, tc.wantPlayed) + } + }) + } +} + +// TestModelEpisodeContentIDs verifies nil episodes and empty content ids are +// dropped — these ids feed SQL IN-lists. +func TestModelEpisodeContentIDs(t *testing.T) { + got := modelEpisodeContentIDs([]*models.Episode{nil, ep(""), ep("a"), ep("b")}) + if len(got) != 2 || got[0] != "a" || got[1] != "b" { + t.Errorf("modelEpisodeContentIDs = %v, want [a b]", got) + } +}