Skip to content

feat(catalog): list an item's collections on its detail page - #239

Merged
randrini merged 5 commits into
mainfrom
fix/v1-item-collections
Oct 7, 2026
Merged

randrini merged 5 commits into
mainfrom
fix/v1-item-collections

Conversation

@randrini

@randrini randrini commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Related issue: #230
Validation tasks: none found covering collection membership on detail pages.

Movie/series detail pages show no collection membership — collections are only reachable from library browse, disconnected from their items.

Approach

Server: reverse lookup on the existing item↔collection mapping, exposed as an additive collections array (id, title, poster_url, poster_thumbhash, item_count) on the v2 item detail response, with existing collection authorization mirrored. Metadata-save responses populate the same row. Web: Collections row of poster chips on the title page (renders only when non-empty), linking to each collection's browse view, following neighboring row conventions.

Stored server-collection memberships only (no smart/live-query, no personal collections); item_count is the global total; lookup failure yields [].

Validation

  • Service tests (member of 2 / member of none / inaccessible excluded, season/series mapping, empty fallback, access-filter recording) + real Postgres reverse-lookup test + metadata-save response test with restricted policy + handler tests + mapper pins
  • Web: 43 row tests + full ItemDetail suite (425) + full web suite green; tsc/eslint/prettier clean
  • Contract: openapi/fixtures/route manifest regenerated, route inventory current, additive-only changes
  • Full CI green on the final head

Risks

Additive API surface only; web row hidden when empty. Android/Apple: field ships regardless; native clients defer UI (no native work in this PR).

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.

AI Disclosure

  • Harness: OpenCode
  • Tool(s): OpenCode (fixer, designer subagents)
  • Model(s): opencode-go/muse-spark-1.3-contributor
  • Involvement: AI-assisted

randrini added 2 commits October 7, 2026 11:20
Add the additive `collections` member to the v2 item detail: the visible
server collections a movie or series belongs to, each with id, title,
poster_url, and item_count. A dedicated `getItemCollectionsCapability`
document reports whether the row is wired.

The lookup reuses `LibraryCollectionRepository.ListContainingItem` through
a narrow `ItemCollectionIndex` seam on the existing collection handler, so
the item detail never widens `LibraryCollectionService` for every fake.
It filters by `CanAccessLibraryCollection` and resolves posters through the
same viewer-aware path as the library Collections tab, so a hidden or
out-of-scope collection never leaks. An episode or season answers its parent
series' collections, because only movies and series can be members.

The row is best-effort, mirroring the theme lookup: an unwired index or a
lookup failure leaves the detail response a success with an empty row.

Contract artifacts are regenerated, not hand-edited. New fixture cases append
at the end of the positional list so committed request ids stay stable.
The v2 item detail now carries a `collections` array — the visible
collections a movie or series belongs to, each with id, title, poster_url
and item_count. Render it as a Collections row on the movie and series
detail pages: one poster chip per collection (poster, title, item count)
linking to that collection's browse view, using the shared MediaCarousel
so the row scrolls and sizes like the neighboring poster rows.

The membership list arrives on the detail payload itself, so the row has
no fetch of its own: the page's existing skeleton covers loading and its
error state covers a failed read, and a title in no collections renders
no row at all. The chip links through buildLibraryCollectionCatalogHref,
the same href the sidebar and Collections tab use for a library
collection.

Regenerate the committed v2 web bindings so the new detail member and
the item-collections capability are typed, and pin the mapper with a
test: catalogItemDetailFromV2 builds ItemDetail field by field, so a
member it forgets to copy would drop silently and the row would never
render.
@randrini
randrini requested a review from drondeseries October 7, 2026 10:48
The #230 item-collections capability route was added to the v2 router without
regenerating internal/api/testdata/media_routes.txt, so TestMediaRouteManifest
failed in CI. Regenerate it: the only change is the new
GET /api/v2/capabilities/item-collections entry, once for each fixture router.
@drondeseries

Copy link
Copy Markdown
Collaborator

Review — COMMENT (no auth bypass found, 1 medium to settle)

Main GET → mapper → movie/series UI path looks sound. Verified at 20e19ee: one parameterized reverse-lookup query, auth via the existing authorized detail service then CanAccessLibraryCollection (same predicate as browse), viewer-aware poster resolution, additive v2 field + capability op, no /api/v1 change, openapi/fixtures/route manifest regenerated. All 8 checks green.

1. Medium — metadata-update response reports no memberships

internal/apiv2/catalog_items.go:1396 inits Collections to []; getCatalogItem populates it but updateAdminItemMetadata (internal/apiv2/admin_catalog_item_metadata.go:80-91) returns the mapper default directly. Editing an item in collections returns "collections":[] until refetch — contradicts "every v2 item detail" (docs/catalog-api.md:581-583). Web is saved by its invalidate/refetch (web/src/hooks/queries/items.ts:334-349), other clients aren't.
Ask: populate memberships on that path with viewer access + add a response test for an item with a collection.

2. Low — document the narrower semantics

Stored server-collection memberships only (no smart/live-query, no personal collections), item_count is the global total not viewer-visible count, lookup failure → []. Defensible, but undocumented. Please note it in docs/catalog-api.md:566-587 (and include poster_thumbhash in the field list).

3. Low — test wording overstates coverage

Member-of-2 / none / inaccessible-excluded cases exist, but on stub indexes not SQL; the HTTP fake discards the access filter; the 200 + [] fallback is untested; season/series mapping untested. "43 row tests" reads like file totals, not new cases (5 in CollectionsSection.test.tsx). Also confirm internal/catalog/jellyfin12_compat_db_test.go:41-84 actually ran against Postgres (skips without SILO_TEST_DATABASE_URL).

Before merge: settle #1, clarify #2 in docs, correct the validation wording, add the Related issue: / Validation tasks: line with the native-client deferral, and smoke-test restricted profiles + large memberships (unpaginated by design — don't silently truncate).

The metadata editor's PATCH response built its detail through the shared
mapper, whose `collections` member defaults to empty, so editing an item that
belongs to a collection returned `"collections":[]` until the client refetched
the read detail. Populate the row on that path with the access scope the
permission gate resolved, the same lookup the read detail uses.

Correct the documented and commented semantics: the field reports stored
server-collection memberships only (a smart collection derives its members from
its query and stores no rows; personal collections are a separate surface), and
`item_count` is the collection's total, not the viewer-visible count. Document
`poster_thumbhash` and the metadata-save row.

Strengthen the tests where the prior coverage was thin: assert the resolved
access filter reaches the reverse lookup, cover season and series mapping, and
cover the lookup-failure fallback. Add a Postgres-backed handler test for the
real SQL that skips without SILO_TEST_DATABASE_URL.
@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Review addressed and pushed (8da71b62): metadata-save responses now populate memberships with the resolved access scope (response test with restricted policy included; fixture regenerated); docs record the narrower semantics (stored-only, global item_count, [] on failure, poster_thumbhash listed); test gaps closed (access-filter recording, season/series mapping, empty fallback, real Postgres reverse-lookup test passing against migrated pgvector). Overstated membership claims corrected in touched files. Full suites green. Related issue: #230. Validation tasks: none found covering this.

@drondeseries

Copy link
Copy Markdown
Collaborator

Re-check at 8da71b62 — all 3 prior findings addressed, 2 gates left

New commit directly answers the last review. No new code blockers found.

Prior findings → verified fixed

1. Metadata-save [] → fixed. internal/apiv2/admin_catalog_item_metadata.go now does out.Collections = NonNil(reg.itemCollections(ctx, detail, handlers.AccessFilterFromContext(ctx, ""))) — same lookup as the read path, gate-resolved scope, non-fatal nil → [] preserved. Pinned by TestUpdateAdminItemMetadataCarriesCollections (edited-item id, gate scope [1 2] reaches the lookup, row on the save body) + regenerated admin_item_metadata_updated.json fixture. The "" deviceID is consistent with the headerless v2 read path — no device-scoped divergence.

2. Docs scope → fixed. docs/catalog-api.md now says stored server-memberships only (smart excluded — no rows; personal excluded — separate surface), item_count is the stored total not viewer-visible, poster_thumbhash documented, failure → [], PATCH-save row documented, episode/season keying via parent series with audiobook/ebook empty. Matches internal/apiv2/catalog_items.go:1154-1180.

3. Thin tests → fixed. Read test now asserts the resolved access filter ([1 2]), season + series mapping, and lookup-failure → 200 + []. Fake records lastCollectionsAccess. New TestItemCollectionsReverseLookupDB exercises the real SQL (visible kept with global item_count: 2, hidden + off-scope dropped under allowlist, hidden still dropped unrestricted, smart never listed, no-member → empty non-nil) with the standard SILO_TEST_DATABASE_URL skip + test/purge-DB refusal. Cleanup order is LIFO-correct.

Remaining before merge (no new review round needed after these)

  1. CI not green yet. Go test, changed-lines lint, router-recovery lint still IN_PROGRESS (lint/contract/web SUCCESS). Do not merge until all complete on this head.
  2. PR body is stale (still the pre-fix text): Approach lists (id, title, poster_url, item_count) — omits poster_thumbhash and the PATCH-save row; Validation doesn't mention the new save-response/DB tests; still no Related issue: / Validation tasks: line and no explicit native-client deferral record. Per repo rules that line is required — add it plus the Apple/Android disposition.

@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Re-check items closed: full CI green on this head (all 8 checks), and the PR body refreshed (poster_thumbhash + PATCH-save row documented, new save-response/DB tests listed, Related issue + Validation tasks lines present, native-client deferral recorded). Ready for merge decision.

@drondeseries

Copy link
Copy Markdown
Collaborator

Final: APPROVE — ready to merge

Head 8da71b62 unchanged since the last re-check: same 4 commits, 8/8 CI green, MERGEABLE/CLEAN. The two gates from the re-check are closed (CI green on this head; body refreshed with Related issue: #230, Validation tasks:, poster_thumbhash + PATCH-save row, and native-client deferral). No new findings.

@randrini

randrini commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Synced with current main (includes #242 CI and prior merges): one digest-fixture conflict resolved by regeneration; CI workflow files identical to main. No code changes in this sync. Awaiting review — no merge until approved.

@randrini
randrini merged commit 27d5fda into main Oct 7, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants