fix(catalog): address episode catalog review feedback
This commit is contained in:
@@ -512,6 +512,9 @@ For each case, capture:
|
||||
- API response changes around count modes may require Android and Apple follow-up. Keeping the existing `total_exact=false` behavior avoids most client churn.
|
||||
- Person filters may need fallback initially unless episode-level people data is available and indexed.
|
||||
- Array GIN filters should be checked with real plans. If separate GIN indexes do too much post-filtering by library, use `btree_gin` multi-column indexes or add selective partial indexes for common libraries.
|
||||
- Index maintenance, disk, and write amplification should be tracked explicitly. The btree indexes such as `idx_episode_catalog_entries_title`, `idx_episode_catalog_entries_added`, and `idx_episode_catalog_entries_air_date`, plus GIN indexes such as `idx_episode_catalog_entries_genres_gin`, trade scanner write cost and disk growth for browse latency. Measure `episode_catalog_entries` table and index size before rollout, and watch trigger update latency during large scanner runs.
|
||||
- Backfill safety needs a production runbook. Treat the `episode_catalog_entries` backfill as idempotent work that can be chunked or retried with low-locking settings, capture a production-sized duration estimate before enabling it broadly, and document rollback to dropping the read model plus its triggers.
|
||||
- Read-model health should be observable. Alert when `episode_catalog_entries.updated_at` lags source table changes beyond a stale threshold or refresh errors appear, fall back to the generic executor when health checks fail, and provide a remediation command to rebuild stale entries.
|
||||
|
||||
## Recommended Decision
|
||||
|
||||
|
||||
@@ -29,6 +29,7 @@ type episodeCatalogUserStatePlan struct {
|
||||
sortOrder string
|
||||
sortOnly bool
|
||||
requiresFullPageGate bool
|
||||
requireProgressRatio bool
|
||||
}
|
||||
|
||||
func (e *QueryExecutor) tryEpisodeCatalogUserStatePreviewPage(
|
||||
@@ -74,7 +75,7 @@ func (e *QueryExecutor) tryEpisodeCatalogUserStatePreviewPage(
|
||||
offset = 0
|
||||
}
|
||||
|
||||
ctes := []string{episodeCatalogUserStateCTE(plan.source)}
|
||||
ctes := []string{episodeCatalogUserStateCTE(plan)}
|
||||
args := []any{access.UserID, access.ProfileID, libraryID}
|
||||
argIdx := 4
|
||||
whereParts := []string{"ece.media_folder_id = $3"}
|
||||
@@ -375,6 +376,7 @@ func extractEpisodeCatalogUserStatePlan(def QueryDefinition) (episodeCatalogUser
|
||||
if plan.source == "" {
|
||||
return episodeCatalogUserStatePlan{}, false, nil
|
||||
}
|
||||
plan.requireProgressRatio = plan.source == "progress" && plan.sortOnly
|
||||
return plan, true, nil
|
||||
}
|
||||
if def.Match != "all" {
|
||||
@@ -423,6 +425,7 @@ func extractEpisodeCatalogUserStatePlan(def QueryDefinition) (episodeCatalogUser
|
||||
return episodeCatalogUserStatePlan{}, false, nil
|
||||
}
|
||||
plan.entryDef.Groups = filteredGroups
|
||||
plan.requireProgressRatio = plan.source == "progress" && plan.sortOnly
|
||||
return plan, true, nil
|
||||
}
|
||||
|
||||
@@ -513,24 +516,28 @@ func rebindUserStateClauses(clauses []string, argIdx int) []string {
|
||||
return out
|
||||
}
|
||||
|
||||
func episodeCatalogUserStateCTE(source string) string {
|
||||
switch source {
|
||||
func episodeCatalogUserStateCTE(plan episodeCatalogUserStatePlan) string {
|
||||
switch plan.source {
|
||||
case "progress":
|
||||
return `user_progress AS (
|
||||
SELECT uwp.media_item_id AS episode_id,
|
||||
uwp.position_seconds::double precision / NULLIF(uwp.duration_seconds, 0) AS progress_ratio
|
||||
FROM user_watch_progress uwp
|
||||
progressRatioGate := ""
|
||||
if plan.requireProgressRatio {
|
||||
progressRatioGate = `
|
||||
AND COALESCE(uwp.duration_seconds, 0) > 0`
|
||||
}
|
||||
return fmt.Sprintf(`user_progress AS (
|
||||
SELECT uwp.media_item_id AS episode_id,
|
||||
uwp.position_seconds::double precision / NULLIF(uwp.duration_seconds, 0) AS progress_ratio
|
||||
FROM user_watch_progress uwp
|
||||
LEFT JOIN user_history_hidden_items hhi
|
||||
ON hhi.user_id = $1
|
||||
AND hhi.profile_id = $2
|
||||
AND hhi.media_item_id = uwp.media_item_id
|
||||
WHERE uwp.user_id = $1
|
||||
AND uwp.profile_id = $2
|
||||
AND uwp.completed = FALSE
|
||||
AND uwp.position_seconds > 0
|
||||
AND COALESCE(uwp.duration_seconds, 0) > 0
|
||||
AND (hhi.media_item_id IS NULL OR uwp.updated_at > hhi.hidden_before)
|
||||
)`
|
||||
AND uwp.profile_id = $2
|
||||
AND uwp.completed = FALSE
|
||||
AND uwp.position_seconds > 0%s
|
||||
AND (hhi.media_item_id IS NULL OR uwp.updated_at > hhi.hidden_before)
|
||||
)`, progressRatioGate)
|
||||
default:
|
||||
return `user_viewed AS (
|
||||
SELECT src.episode_id,
|
||||
@@ -810,7 +817,7 @@ func buildEpisodeCatalogEntryRuleWhere(rule QueryRule, argIdx int) (string, []an
|
||||
case "content_rating":
|
||||
return buildEpisodeCatalogEqualityClause("ece.content_rating", rule, argIdx)
|
||||
case "added_at":
|
||||
return buildEpisodeCatalogComparisonClause("ece.added_at", rule, argIdx, "timestamptz")
|
||||
return buildEpisodeCatalogComparisonClause("ece.episode_created_at", rule, argIdx, "timestamptz")
|
||||
case "release_date":
|
||||
return buildEpisodeCatalogComparisonClause("ece.episode_air_date", rule, argIdx, "date")
|
||||
case "status":
|
||||
|
||||
@@ -80,6 +80,32 @@ func TestBuildEpisodeCatalogEntryQueryWhereUsesReadModelFilters(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestEpisodeCatalogAddedAtFilterUsesEpisodeCreatedAt(t *testing.T) {
|
||||
def := QueryDefinition{
|
||||
Match: "all",
|
||||
Groups: []QueryGroup{{
|
||||
Match: "all",
|
||||
Rules: []QueryRule{
|
||||
{Field: "added_at", Op: "in_last", Value: "30d"},
|
||||
},
|
||||
}},
|
||||
}.Normalize()
|
||||
|
||||
where, _, _, ok, err := buildEpisodeCatalogEntryQueryWhere(def, 2)
|
||||
if err != nil {
|
||||
t.Fatalf("buildEpisodeCatalogEntryQueryWhere returned error: %v", err)
|
||||
}
|
||||
if !ok {
|
||||
t.Fatal("buildEpisodeCatalogEntryQueryWhere unexpectedly fell back")
|
||||
}
|
||||
if !strings.Contains(where, "ece.episode_created_at >= NOW() - INTERVAL '30 days'") {
|
||||
t.Fatalf("expected added_at filter to use episode_created_at, got %q", where)
|
||||
}
|
||||
if strings.Contains(where, "ece.added_at") {
|
||||
t.Fatalf("added_at filter must not use library first_seen_at, got %q", where)
|
||||
}
|
||||
}
|
||||
|
||||
func TestEpisodeCatalogEntryQueryFallsBackForSameFileTechnicalAnd(t *testing.T) {
|
||||
group := QueryGroup{
|
||||
Match: "all",
|
||||
@@ -98,6 +124,56 @@ func TestEpisodeCatalogEntryQueryFallsBackForSameFileTechnicalAnd(t *testing.T)
|
||||
}
|
||||
}
|
||||
|
||||
func TestEpisodeCatalogInProgressPlanKeepsUnknownDurationRows(t *testing.T) {
|
||||
def := QueryDefinition{
|
||||
Match: "all",
|
||||
Groups: []QueryGroup{{
|
||||
Match: "all",
|
||||
Rules: []QueryRule{
|
||||
{Field: "in_progress", Op: "is", Value: true},
|
||||
},
|
||||
}},
|
||||
Sort: QuerySort{Field: "title", Order: "asc"},
|
||||
}.Normalize()
|
||||
|
||||
plan, ok, err := extractEpisodeCatalogUserStatePlan(def)
|
||||
if err != nil {
|
||||
t.Fatalf("extractEpisodeCatalogUserStatePlan returned error: %v", err)
|
||||
}
|
||||
if !ok {
|
||||
t.Fatal("expected in_progress filter to use a user-state plan")
|
||||
}
|
||||
sql := episodeCatalogUserStateCTE(plan)
|
||||
if strings.Contains(sql, "COALESCE(uwp.duration_seconds, 0) > 0") {
|
||||
t.Fatalf("in_progress filter must keep unknown-duration rows, got CTE:\n%s", sql)
|
||||
}
|
||||
}
|
||||
|
||||
func TestEpisodeCatalogProgressSortRequiresProgressRatio(t *testing.T) {
|
||||
def := QueryDefinition{
|
||||
Match: "all",
|
||||
Groups: []QueryGroup{{
|
||||
Match: "all",
|
||||
Rules: []QueryRule{
|
||||
{Field: "genre", Op: "is", Value: "Comedy"},
|
||||
},
|
||||
}},
|
||||
Sort: QuerySort{Field: "progress", Order: "desc"},
|
||||
}.Normalize()
|
||||
|
||||
plan, ok, err := extractEpisodeCatalogUserStatePlan(def)
|
||||
if err != nil {
|
||||
t.Fatalf("extractEpisodeCatalogUserStatePlan returned error: %v", err)
|
||||
}
|
||||
if !ok {
|
||||
t.Fatal("expected progress sort to use a user-state plan")
|
||||
}
|
||||
sql := episodeCatalogUserStateCTE(plan)
|
||||
if !strings.Contains(sql, "COALESCE(uwp.duration_seconds, 0) > 0") {
|
||||
t.Fatalf("progress sort must require a computable ratio before falling back, got CTE:\n%s", sql)
|
||||
}
|
||||
}
|
||||
|
||||
func TestExtractEpisodeCatalogUserStatePlanForLastWatched(t *testing.T) {
|
||||
def := QueryDefinition{
|
||||
Match: "all",
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_series ON public.media_items;
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_episodes ON public.episodes;
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files_update ON public.media_files;
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files_insert_delete ON public.media_files;
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files ON public.media_files;
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_episode_libraries ON public.episode_libraries;
|
||||
|
||||
|
||||
@@ -183,8 +183,8 @@ BEGIN
|
||||
) AS resolution_codes,
|
||||
MAX(mf.bitrate) FILTER (WHERE mf.bitrate IS NOT NULL AND mf.bitrate > 0) AS max_bitrate,
|
||||
MIN(mf.bitrate) FILTER (WHERE mf.bitrate IS NOT NULL AND mf.bitrate > 0) AS min_bitrate,
|
||||
BOOL_OR(COALESCE(mf.hdr, false)) AS has_hdr,
|
||||
BOOL_OR(NOT COALESCE(mf.hdr, false)) AS has_non_hdr,
|
||||
BOOL_OR(mf.hdr IS TRUE) AS has_hdr,
|
||||
BOOL_OR(mf.hdr IS FALSE) AS has_non_hdr,
|
||||
BOOL_OR(EXISTS (
|
||||
SELECT 1
|
||||
FROM jsonb_array_elements(COALESCE(mf.video_tracks, '[]'::jsonb)) AS vt
|
||||
@@ -376,7 +376,7 @@ WITH active_episode_files AS (
|
||||
public.episode_catalog_normalized_resolution(mf.resolution) AS normalized_resolution,
|
||||
public.episode_catalog_resolution_rank(mf.resolution) AS resolution_rank,
|
||||
mf.bitrate,
|
||||
COALESCE(mf.hdr, false) AS hdr,
|
||||
mf.hdr,
|
||||
EXISTS (
|
||||
SELECT 1
|
||||
FROM jsonb_array_elements(COALESCE(mf.video_tracks, '[]'::jsonb)) AS vt
|
||||
@@ -398,8 +398,8 @@ file_stats AS (
|
||||
FILTER (WHERE normalized_resolution IS NOT NULL) AS resolution_codes,
|
||||
MAX(bitrate) FILTER (WHERE bitrate IS NOT NULL AND bitrate > 0) AS max_bitrate,
|
||||
MIN(bitrate) FILTER (WHERE bitrate IS NOT NULL AND bitrate > 0) AS min_bitrate,
|
||||
BOOL_OR(hdr) AS has_hdr,
|
||||
BOOL_OR(NOT hdr) AS has_non_hdr,
|
||||
BOOL_OR(hdr IS TRUE) AS has_hdr,
|
||||
BOOL_OR(hdr IS FALSE) AS has_non_hdr,
|
||||
BOOL_OR(has_dolby_vision) AS has_dolby_vision,
|
||||
BOOL_OR(NOT has_dolby_vision) AS has_non_dolby_vision
|
||||
FROM active_episode_files
|
||||
@@ -595,14 +595,32 @@ AFTER INSERT OR UPDATE OR DELETE ON public.episode_libraries
|
||||
FOR EACH ROW EXECUTE FUNCTION public.episode_catalog_entries_episode_libraries_trigger();
|
||||
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files ON public.media_files;
|
||||
CREATE TRIGGER trg_episode_catalog_entries_media_files
|
||||
AFTER INSERT OR UPDATE OF episode_id, media_folder_id, missing_since, resolution, bitrate, hdr, video_tracks, audio_tracks, subtitle_tracks, external_subtitles, created_at OR DELETE
|
||||
ON public.media_files
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files_insert_delete ON public.media_files;
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_media_files_update ON public.media_files;
|
||||
CREATE TRIGGER trg_episode_catalog_entries_media_files_insert_delete
|
||||
AFTER INSERT OR DELETE ON public.media_files
|
||||
FOR EACH ROW EXECUTE FUNCTION public.episode_catalog_entries_media_files_trigger();
|
||||
|
||||
CREATE TRIGGER trg_episode_catalog_entries_media_files_update
|
||||
AFTER UPDATE ON public.media_files
|
||||
FOR EACH ROW
|
||||
WHEN (
|
||||
OLD.episode_id IS DISTINCT FROM NEW.episode_id OR
|
||||
OLD.media_folder_id IS DISTINCT FROM NEW.media_folder_id OR
|
||||
OLD.missing_since IS DISTINCT FROM NEW.missing_since OR
|
||||
OLD.resolution IS DISTINCT FROM NEW.resolution OR
|
||||
OLD.bitrate IS DISTINCT FROM NEW.bitrate OR
|
||||
OLD.hdr IS DISTINCT FROM NEW.hdr OR
|
||||
OLD.video_tracks IS DISTINCT FROM NEW.video_tracks OR
|
||||
OLD.external_subtitles IS DISTINCT FROM NEW.external_subtitles OR
|
||||
OLD.audio_language_codes IS DISTINCT FROM NEW.audio_language_codes OR
|
||||
OLD.subtitle_language_codes IS DISTINCT FROM NEW.subtitle_language_codes
|
||||
)
|
||||
EXECUTE FUNCTION public.episode_catalog_entries_media_files_trigger();
|
||||
|
||||
DROP TRIGGER IF EXISTS trg_episode_catalog_entries_episodes ON public.episodes;
|
||||
CREATE TRIGGER trg_episode_catalog_entries_episodes
|
||||
AFTER INSERT OR UPDATE OF content_id, series_id, title, air_date, runtime, rating_imdb, rating_tmdb, still_path, still_thumbhash, created_at OR DELETE
|
||||
AFTER INSERT OR UPDATE OF content_id, series_id, title, episode_number, air_date, runtime, rating_imdb, rating_tmdb, still_path, still_thumbhash, created_at OR DELETE
|
||||
ON public.episodes
|
||||
FOR EACH ROW EXECUTE FUNCTION public.episode_catalog_entries_episodes_trigger();
|
||||
|
||||
@@ -639,6 +657,15 @@ changed_entries AS (
|
||||
CROSS JOIN migration_clock mc
|
||||
WHERE si.updated_at >= mc.started_at
|
||||
AND si.type = 'series'
|
||||
UNION
|
||||
SELECT DISTINCT ece.episode_id, ece.media_folder_id
|
||||
FROM public.episode_catalog_entries ece
|
||||
WHERE NOT EXISTS (
|
||||
SELECT 1
|
||||
FROM public.episode_libraries el
|
||||
WHERE el.episode_id = ece.episode_id
|
||||
AND el.media_folder_id = ece.media_folder_id
|
||||
)
|
||||
)
|
||||
SELECT COUNT(*)
|
||||
FROM (
|
||||
|
||||
@@ -126,7 +126,14 @@ def getenv_required(name: str) -> str:
|
||||
return value
|
||||
|
||||
|
||||
def validate_base_url(base_url: str) -> None:
|
||||
parsed = urllib.parse.urlparse(base_url)
|
||||
if parsed.scheme not in {"http", "https"} or not parsed.netloc:
|
||||
raise ValueError("SILO_BASE_URL must be an http or https URL")
|
||||
|
||||
|
||||
def build_url(config: Config, params: dict[str, str], include_total: bool) -> str:
|
||||
validate_base_url(config.base_url)
|
||||
query = {
|
||||
"source": "query",
|
||||
"type": "episode",
|
||||
@@ -137,7 +144,8 @@ def build_url(config: Config, params: dict[str, str], include_total: bool) -> st
|
||||
}
|
||||
if not include_total:
|
||||
query["include_total"] = "false"
|
||||
return urllib.parse.urljoin(config.base_url.rstrip("/") + "/", "api/v1/catalog") + "?" + urllib.parse.urlencode(query)
|
||||
api_url = urllib.parse.urljoin(config.base_url.rstrip("/") + "/", "api/v1/catalog")
|
||||
return api_url + "?" + urllib.parse.urlencode(query)
|
||||
|
||||
|
||||
def fetch_case(config: Config, case: str, params: dict[str, str], include_total: bool) -> CaseResult:
|
||||
@@ -151,16 +159,31 @@ def fetch_case(config: Config, case: str, params: dict[str, str], include_total:
|
||||
},
|
||||
)
|
||||
started = time.perf_counter()
|
||||
status = 0
|
||||
try:
|
||||
with urllib.request.urlopen(request, timeout=config.timeout) as response:
|
||||
status = response.status
|
||||
body = response.read()
|
||||
elapsed_ms = (time.perf_counter() - started) * 1000
|
||||
payload = json.loads(body)
|
||||
items = payload.get("items")
|
||||
try:
|
||||
payload = json.loads(body)
|
||||
items = payload.get("items")
|
||||
except (json.JSONDecodeError, AttributeError, TypeError) as err:
|
||||
return CaseResult(
|
||||
case,
|
||||
include_total,
|
||||
status,
|
||||
elapsed_ms,
|
||||
None,
|
||||
None,
|
||||
None,
|
||||
None,
|
||||
str(err),
|
||||
)
|
||||
return CaseResult(
|
||||
case=case,
|
||||
include_total=include_total,
|
||||
status=response.status,
|
||||
status=status,
|
||||
elapsed_ms=elapsed_ms,
|
||||
total=payload.get("total"),
|
||||
total_exact=payload.get("total_exact"),
|
||||
@@ -225,6 +248,10 @@ def main() -> int:
|
||||
offset=args.offset,
|
||||
timeout=args.timeout,
|
||||
)
|
||||
try:
|
||||
validate_base_url(config.base_url)
|
||||
except ValueError as err:
|
||||
raise SystemExit(str(err)) from err
|
||||
|
||||
cases = DEFAULT_SORT_CASES + DEFAULT_FILTER_CASES
|
||||
include_total_values = [False, True] if args.include_exact else [False]
|
||||
|
||||
@@ -69,7 +69,7 @@ describe("LibraryBrowse", () => {
|
||||
expect(mocks.useCatalogWindow).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("shows the filtered item count from the catalog result", () => {
|
||||
it("does not show the estimated item count as an exact result count", () => {
|
||||
mocks.useCatalogWindow.mockReturnValue({
|
||||
data: {
|
||||
totalItems: 1234,
|
||||
@@ -95,10 +95,11 @@ describe("LibraryBrowse", () => {
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(markup).toContain("1,234 items");
|
||||
expect(markup).toContain("Filters");
|
||||
expect(markup).not.toContain("1,234 items");
|
||||
});
|
||||
|
||||
it("shows a loading state for the item count while catalog data loads", () => {
|
||||
it("does not show an item count loading state when exact totals are skipped", () => {
|
||||
mocks.useCatalogWindow.mockReturnValue({
|
||||
data: {
|
||||
totalItems: 0,
|
||||
@@ -124,7 +125,8 @@ describe("LibraryBrowse", () => {
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(markup).toContain("Loading item count");
|
||||
expect(markup).toContain("Filters");
|
||||
expect(markup).not.toContain("Loading item count");
|
||||
expect(markup).not.toContain("0 items");
|
||||
});
|
||||
|
||||
|
||||
@@ -38,10 +38,6 @@ function getLibrarySortRelevanceScope(
|
||||
return "all";
|
||||
}
|
||||
|
||||
function formatItemCount(count: number): string {
|
||||
return `${count.toLocaleString()} item${count === 1 ? "" : "s"}`;
|
||||
}
|
||||
|
||||
export default function LibraryBrowse({
|
||||
libraryId,
|
||||
libraryType,
|
||||
@@ -112,7 +108,6 @@ export default function LibraryBrowse({
|
||||
const totalItems = catalogQuery.data?.totalItems ?? 0;
|
||||
const pages = catalogQuery.data?.pages ?? new Map();
|
||||
const isLoading = catalogQuery.isLoading;
|
||||
const itemCountLabel = formatItemCount(totalItems);
|
||||
|
||||
return (
|
||||
<div className="space-y-5 py-2 sm:space-y-6">
|
||||
@@ -146,8 +141,6 @@ export default function LibraryBrowse({
|
||||
allowPersonalizedFilters
|
||||
allowPersonalizedSorts
|
||||
sortRelevanceScope={sortRelevanceScope}
|
||||
resultCountLabel={itemCountLabel}
|
||||
resultCountLoading={isLoading}
|
||||
/>
|
||||
<ItemGrid
|
||||
totalItems={totalItems}
|
||||
|
||||
Reference in New Issue
Block a user