Skip to content

feat(collections): merge the collections and Home rows revamp into main - #2013

Merged
Quick104 merged 451 commits into
mainfrom
feat/collections-revamp
Oct 8, 2026
Merged

Quick104 merged 451 commits into
mainfrom
feat/collections-revamp

Conversation

@Quick104

@Quick104 Quick104 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Closes #1608
Closes #1612
Closes #1614
Closes #1616
Closes #1639
Related issue: #192, #1137, #1615, #990, #717
Validation tasks: changes #193 S1, S2, S3 (recorded Pass) and S4 (recorded Partial); unblocks #193 S6 (#1616), S7 and #195 W7 (#1615); reaches #193 S5 with no change to its outcome. Every other case these changes reach has no recorded result and needs a first walk on the merged build: #192 AC1–AC7, X1–X5 (the native part of X5 also waits on Silo-Server/silo-apple#534 and Silo-Server/silo-android#406); #194 A1–A5; #195 W1–W6; #1137 AC1–AC3, AC5, X1–X3; #1140 C1, C2, C4; #1141 C1, C2; #1142 C1–C3; #1181 C1; #1252 C1; #1256 C1; the #199 Browse criterion; Silo-Server/silo-apple#20 and Silo-Server/silo-apple#21 C1–C3, C7; Silo-Server/silo-android#5 C1–C3, C6, C7; Silo-Server/silo-android#6 C1–C3, C7; Silo-Server/silo-apple#305, Silo-Server/silo-apple#306, Silo-Server/silo-android#315 and Silo-Server/silo-android#316 C1, C3.

This merges feat/collections-revamp into main. The branch reworks collections and Home rows in the web app and their APIs. It holds 66 pull requests (#1851 to #1993), each reviewed and merged into the branch on its own.

Before the revamp, one profile could reorder or regroup another profile's personal collections. The web also offered Delete and Sync on collections the server would then refuse to change. Collections were built in several different editors, Home rows were managed from an older Sections page, and some setups dead-ended: the TMDB franchise placeholder could not be configured, and Add to collection had nothing to add to when no manual collection existed.

After this merge:

  • A personal collection belongs to the profile that created it, and only that profile changes it. One Show to other profiles switch shows it read-only to every profile on the login (Personal collections: owner-only management and a single share-with-all-profiles switch #1615 model). A shared collection shows only titles that both the owner and the viewer can access, and imports fill their item limit the same way.
  • The Collections page splits into Your collections, Shared with me and Server collections. One New collection button opens a type picker.
  • Manual, Smart and Synced list collections, server and personal, are created and edited on one collection editor page. Smart collections get a live poster preview, and the rule editor picks values from your libraries.
  • Admins get a List view for server collections with bulk sync, show, hide and delete; Arrange for shelves, including pinning; and Starter packs in place of template bundles.
  • Admin > Sections becomes Home rows: one list with switches and a row menu, one Add/Edit row dialog, poster peeks, and copying a row to several library pages. Settings > Home Screen uses the same parts for profiles.
  • Collections show which Home and library rows use them, and you can add one to a row from the collection.

Each sub-PR's description has its design, decisions and validation notes.

The #193 results recorded before the revamp change on purpose, and each needs a re-test on a build that includes this merge:

Approach

On top of the reviewed sub-PRs, this PR adds three merges of main, the follow-ups they needed, and two rounds of fixes from review on this PR (listed after the merges).

The first merge (32 commits of main). Most of the conflict resolution was routine. The parts worth a reviewer's look:

#862 (Jellyfin BoxSets for opted-in personal collections) also targets this branch and is still open. It is not part of this merge.

The second merge (97 commits of main) changed behavior in four places:

The third merge (21 commits of main) kept both new catalog search capability flags, facet_value_search and video_with_episodes_scope, and regenerated the OpenAPI document, fixtures, and web types.

Fixes from the first review on this PR, each with a test that fails without it:

  • The personal collection editor body no longer carries a background collage's thumbhash behind its strong ETag.
  • A row scoped to an empty library list no longer queries every library.
  • Collection editor: Create keeps the list it was opened from. The library untick warning counts every title, not the first 200. Quick successive adds keep their order. Reordering skips titles that failed to add. Artwork changed during a save survives it. A conflict stays until Keep mine or Use theirs.
  • Home rows: a dialog closed mid-save no longer closes the next one. Holiday row names keep typed spaces. Rule rows offer only the sorts their media kind supports. Switching profiles clears poster peeks.
  • Add to collection restores a failed tick after a search hides the row. A sort picked in the Advanced filter sheet replaces collection order. The narrow New collection dock sits above the background playback bar.

Fixes from the second review, on the merged build:

  • The collection editor asked the catalog for 200 titles a page, over its 100 maximum, so the request failed: saved titles showed no poster or year, and the warning about titles only in an unticked library never appeared. It now reads pages of 100 through the cursor, keeping one page size, since a cursor is bound to it.
  • Show only can be set back to All on a personal collection.
  • The admin List and bulk delete recognize the 409 that v2 sends for a server collection a row still shows; their tests had mocked the v1 code collection_in_use.
  • An update with a null group_id is accepted again (a named group still answers 501). The v1 removals row and docs/collections-api.md record it, along with the order's own-collections rule.
  • web/src/lib/collectionGroups.ts is removed; nothing imports it.
  • A smart collection with no poster among a viewer's matches no longer holds a collage build-queue slot for ten minutes. Enough of them on a busy server filled the node's queue and stopped every other personal collage build there.

The test cleanup removes 38 obsolete or redundant web cases and five inherited Postgres subcases for retired personal groups. It also removes unused mutation hooks, old Home-row payload builders, and unreachable personal-group SQL. Useful assertions now exercise the current save paths. The cleanup removes 2,207 net lines, including 1,643 lines of tests and fixtures.

Validation

At 2762dbade and the commits before it:

  • A sandbox started on main with three profiles on one login (one a child profile limited to Movies and PG), personal collections with groups and full and partial allow lists, and a server collection, then rebuilt from this branch. The migration kept the fully shared collection shared, made the one shared with a single profile private, removed the group, and gave each profile one order. Writes from another profile answer 403, a private collection is 404 to other profiles, counts and collages follow the owner's and the viewer's access, and group operations answer 501 on v1 and v2.
  • Walked on the web: the Collections page, the type picker, the smart editor with live preview, Add to collection (including a profile with no collection), the admin List, select mode, Arrange and Starter packs, Admin > Home rows and the /admin/sections redirect, Add as a row from a server and a personal collection and the rows each lists, Settings > Home Screen, and the admin per-profile Home sections card. Before the fixes the editor logged two 422s per open; after them it logs none.
  • go build ./..., go vet, make lint-changed (0 issues), and make test-go: everything passes except TestRunStatsWithRealFFmpeg, which fails the same way on main with the local ffmpeg. Database-backed tests skip without a test database; CI runs them.
  • Web: make test-web (834 files, 7,175 tests, run in four shards), pnpm run lint (0 errors), format:check, build, and budget:check (325,136 bytes Brotli, within the budget): pass.
  • verify-apiv2-openapi, verify-apiv2-contract, verify-apiv2-fixtures, verify-apiv2-web-types, verify-route-inventory, verify-migration-ledger, verify-offline-routes, verify-settings-bindings-all, verify-scenario-catalogs, verify-playback-fixtures, verify-local-paths: pass.
  • Earlier, at cca184d2c: 8,256 comparisons of the old and simplified Home-row builders produce equivalent payloads.
  • Not run: the native apps against this build, and the validation cases listed above, which need a walk by their validators on a build that includes this merge.

Evidence

Before and after captures of the Collections page, a shared collection from other profiles, the new collection flow, Admin > Collections, Admin > Sections and Home rows, and Settings > Home Screen, on desktop and phone width, with recordings of the walk and of the review fixes.

Evidence: https://evidence.siloserver.org/r/silo-server/pr-2013/

Risks

Checklist

  • I read and can explain the complete diff.
  • This pull request addresses one concern.
  • The Evidence section shows every change a user can see, or says there is none.

AI Disclosure

  • Harness: Claude Code and Codex (running in T3 Code)
  • Tool(s): Claude Code, Codex
  • Model(s): claude-opus-5-5[1m], gpt-6-astra, gpt-6.1-sol; review subagents also ran on Claude Sonnet through Claude Code's sonnet model alias
  • Involvement: AI-assisted. The merges of main, the conflict resolution, the follow-up commits, the fixes from the second review, and this description are AI-generated. The test cleanup and its validation summary were generated with gpt-6-astra through Codex. Each sub-PR records its own disclosure.
  • Adversarial review: the merged build had a multi-agent code review (Claude finders and verifiers), two rounds of cross-provider review with gpt-6.1-sol through Codex, and a live sandbox walk. Their findings are posted as review comments on this PR. Seven were fixed here (the SQLite v30 collision, the editor's catalog page size and the untick warning, the in-use 409, Show only, a null group_id, and the collage build queue), and the cross-provider round's cursor page-size finding was fixed too. v1 sharing is kept as documented; the rest are left as suggestions. Each sub-PR records its own review.

🤖 Generated with Claude Code

Note

Merge collections and Home rows revamp into main

  • Adds the new Admin Home rows page (/admin/home-rows) with the full row add/edit/picker dialogs, and redirects the legacy admin Sections route to it in App.tsx. The retired allow_profile_custom_sections setting now always returns true.
  • Replaces per-profile collection allow lists with a single login-wide shared flag. Migration 20261003235347 (schema v31) converts partially-shared collections to private and removes personal collection groups in migrate.go; collection-group operations now return HTTP 501.
  • Adds viewer-specific personal collection poster collages: PersonalCollectionCollages serves stored collages and builds missing ones in the background, keyed by owner access intersection (personal_collection_collages.go, collages.go).
  • Hardens collection access: reading another profile's shared collection now intersects viewer and owner library/maturity access (personal_collection_access.go); scheduler syncs use atomic DB-clock claims so only one node syncs a due collection (scheduler.go).
  • Rewrites the web collection editor into a routed CollectionEditorPage with manual, smart, and synced collection flows, plus new collection action menus, starter packs, and shelf arrangement UI.
  • Risk: breaking API v2 changes (approved in breaking-approvals.json) — collection create/update responses drop allowed_profile_ids, group-scoped ordering returns 501/422, and the schema v31 migration irreversibly flattens collection groups; check userdb.applyCollectionLoginSharing before deploying.

Macroscope summarized a40299f.

Quick104 and others added 30 commits October 4, 2026 10:32
…ule-rows

feat(web): pick collections and build rule rows in the Home row dialog
… into feat/api-collection-row-references

Brings in the base's personal login sharing (#1875), the scrobble lock
fix (#1887), and the phase-1 API branches. The only conflict was the
contract digest in get_system_info_ok.json; the OpenAPI document,
fixtures, and web types were regenerated from the merged router.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The two tests that tick 100 library pages queried each checkbox by role
and name. Role queries recompute every accessible name in the group, so
the tests ran past CI's 30-second timeout. Label lookups find the same
checkboxes far faster.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e with content

The page placement rose by a hard-coded 8rem/9rem, which overlapped the
taller stacked watch bar on phones, and in the admin shell at lg its scroll
room left the end of the page under the raised bar. It now rises by
--playback-bar-clearance, derived on :root from the playback bar heights
usePlaybackBarHeight already measures, and its spacer and scrim include
that clearance. Both shells publish --page-gutter and pad by it, so the bar
lines up with the content column at every width.

The editor's create mode needs the bar before anything is staged, so
visible, tone and label are opt-in props. The page status stays mounted,
empty, while the bar is hidden, so the first change is announced.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…es' into fix/sections-missing-collection-log

Brings in the moved feat/collections-revamp base (#1875 personal login
sharing, #1887 scrobble lock fix) through the row-references branch.
The merge applied cleanly; the missing-collection logging tests pass
under the login-wide sharing model without changes.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
feat(web): add a Home row to several library pages at once
…rged

An approval lives only on the pull request that needs it. The #1615
removals merged into feat/collections-revamp with #1875, so these entries
now match no change and fail the contract gate. The record stays in the
file's history; the final merge into main carries them again.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…log' into feat/api-collections-contains-item

Brings in login-wide personal collection sharing (#1875), the scrobble
lock fix (#1887), and the earlier phase-1 collection PRs. The contains_item
tests now create shared collections with is_shared alone, since
allowed_profile_ids is gone. The capability test expects both
login_sharing and contains_item. The fixture list carries the shared
collection, which gets no contains flag. Contract artifacts were
regenerated.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-switch

feat(web): let admins allow rule rows on profiles from Home rows
A collection page offered no way to change the collection, so owners and
admins had to find it again on the Collections page or the admin board.

Every collection page now puts Edit and a ⋯ menu (Open in, Sync now for
synced lists, Delete…) top-right for the collection's owner, and for server
collections while acting as admin, never by account role alone. Another
profile's shared collection says whose it is and that it's read-only, and
`?notice=read-only` explains why an editor link landed on the page. A
library's Collections tab links acting admins to Arrange shelves.

Delete sends the ETag the page read; a 412 refreshes the collection so the
next try sends its current version. Personal sync and delete now share the
scope hooks the page uses.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The collection page now caches the same snapshot the editor reads. With
the app's two-minute staleTime, clicking Edit soon after opening the page
started the editor on that cached copy, so a change made elsewhere in the
meantime surfaced only as a 412 on save. The editor now refetches on mount
and keeps the read it made, not the cached copy.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The SQLite user store has no description column, so a description sent on
create was dropped while getCollectionCapabilities still reported
create_description. Stores now report description support through
CollectionFeatures: Postgres reports it, SQLite does not. The capability
follows the acting account's store, and a create carrying a description on
a store without support answers 501 capability_unsupported, now declared on
createCollection.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…at/api-profile-section-default-title

# Conflicts:
#	contracts/api/v2/fixtures/get_system_info_ok.json
The #1615 approvals came in with the feat/collections-revamp merge. This
branch's contract diff against that base no longer contains those
removals, so the contract check reports every approval as stale.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…scription

feat(api): accept a description when creating a personal collection
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…at/api-profile-section-default-title

# Conflicts:
#	contracts/api/v2/fixtures/get_system_info_ok.json
…osters

feat(api): include poster URLs in personal smart previews
…at/api-profile-section-default-title

# Conflicts:
#	contracts/api/v2/fixtures/get_system_info_ok.json
# Conflicts:
#	contracts/api/v2/fixtures/get_system_info_ok.json
…eb-base

# Conflicts:
#	contracts/api/v2/fixtures/get_system_info_ok.json
The scope adapter moved isListBackedCollectionType to lib/collections/types.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…at/api-bundle-template-summaries

# Conflicts:
#	contracts/api/v2/fixtures/get_system_info_ok.json
A client newer than the server could only tell whether bundles carry
templates by inspecting the response. The template_summaries flag on
getAdminCollectionCapabilities lets it feature-detect the summaries
before enabling the starter-packs flow.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…fault-title

feat(api): return a profile row's server title in profile section settings
The collections redesign reuses the Home rows list rows, page pills, row and
More menus, select mode bar, poster peeks and two-step dialog. They move to
components/calm and take plain props instead of Home rows types, so
collection pages can render them without depending on HomeRow or PageRef.
Home rows imports them from there and renders the same DOM.

ActionMenu gains optional help lines and submenus, and ConfirmDialog gains
bullets and the "This can't be undone." line, for the collection menus and
confirms that follow. The Home rows collection kind label now comes from
collectionKindOf so both features name types the same way.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
refactor(web): move Home Screen saves into one hook
feat(web): build Settings > Home Screen from the Home rows components
The bullets and the "This can't be undone." line sat outside the alert
dialog's description, so screen readers announced only the first
sentence and skipped the consequences of a destructive action.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@macroscopeapp

macroscopeapp Bot commented Oct 6, 2026

Copy link
Copy Markdown

The Validation tasks: line still omits unrun cross-client checks reached by these changes: #192 X1–X4 (collection creation, access, admin updates, and restricted members) and #1137 X1–X3 (row-set propagation, profile ordering, and restricted rows). It also lists #1181 C1, which tests progress/preferences and kids filtering; this diff does not change those paths. A complete #192 X5 walk also depends on the open Apple/Android sharing follow-ups, since this diff contains no native sharing UI.

Suggested fix (no diff):

Validation tasks: changes #193 S1, S2, S3 (recorded Pass) and S4 (recorded Partial); unblocks #193 S6 (#1616), S7 and #195 W7 (#1615); reaches #193 S5 with no change to its outcome. Every other case these changes reach has no recorded result and needs a first walk on the merged build: #192 AC2, AC5–AC7, X1–X4; X5 after Silo-Server/silo-apple#534 and Silo-Server/silo-android#406; #194 A1–A5; #195 W1–W6; #1137 AC1–AC3, X1–X3; #1140 C1, C2, C4; #1141 C1–C3; #1142 C1–C3; #1252 C1; #1256 C1; the #199 Browse criterion; Silo-Server/silo-apple#20 and Silo-Server/silo-apple#21 C1–C3, C6, C7; Silo-Server/silo-android#5 and Silo-Server/silo-android#6 C1–C3, C6, C7; Silo-Server/silo-apple#305, Silo-Server/silo-apple#306, Silo-Server/silo-android#315 and Silo-Server/silo-android#316 C1, C3.

Automated check: Macroscope check run agent (gpt-6-luna). No validation was performed.

Posted via Macroscope — v1 validation impact

@silo-kody

silo-kody Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Silo Kody — review complete

Review finished. Check the inline comments for findings and verify each suggestion against the code and tests.

Reviewing changes in Silo
  • Include the related issue, expected behavior, and validation steps in the PR description.
  • For API changes, describe the effect on Apple and Android clients and Jellyfin compatibility.
  • For plugin changes, identify the affected SDK contract, plugin, and catalog entry.
  • Follow this repository's AGENTS.md and CONTRIBUTING.md.
  • Request another review with @kody start-review in a PR comment.
  • React with 👍 or 👎 to give feedback on individual suggestions.
Review settings
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug ✅
Performance ✅
Security ✅
Business Logic ❌

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cca184d2c1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +416 to +418
count := "count(*)"
if distinctTitles {
count = "count(DISTINCT cid)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Deduplicate titles when counting array facets

When a media item contains the same genre, studio, network, or country more than once, the later UNNEST emits duplicate (cid, name) rows, but this path uses count(*) unless the request has a person scope. Because these arrays have no database uniqueness constraint, one title can therefore contribute multiple times to the advertised title count and incorrectly change ranked facet results. Count distinct content IDs for array facets as well.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same point as the earlier thread on this line (Kody's), where the author measured the count(DISTINCT cid) version against a cold cache and kept the current count. It only affects the picker's count hint, not which titles a rule matches.

Quick104 commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Silo's pull request guidance changed today (#2016). A PR that changes what a user sees now needs before-and-after evidence in its Evidence section, and a PR with no visible change says Evidence: none, no user-visible change. This PR was opened before the rule existed, so it hasn't had a chance to meet it yet.

Missing here: Before-and-after screenshots of the web collections pages, the collection editor and the Home rows settings on the merged build, or an evidence.siloserver.org link. Pointing to the sub-PRs doesn't cover the integrated result.

The rules are in Show visible changes, and the PR template shows the Evidence section layout. I've converted this PR to draft for now. Mark it ready for review once the Evidence section is filled in.


Generated by Claude Code

@Quick104
Quick104 marked this pull request as draft October 6, 2026 19:25
Quick104 and others added 9 commits October 7, 2026 21:01
Conflict resolution:

- internal/userdb: main took SQLite schema v30 for the profile PIN
  revision (#2020). The login-wide sharing migration moves to v31, so a
  store already at main's v30 still runs it.
- internal/catalog, internal/sections: main's fail-closed library scope
  (#1953, #2005, #2006) and this branch fixed the same empty-scope bug.
  The executor and section fetcher keep main's AccessFilter.LibraryScope;
  library collections keep this branch's ResolveLibraryCollectionMembership,
  which already returns nothing when the saved libraries miss the
  collection's own. main's scope test now exercises that resolver.
  narrowLibraryScope no longer needs the helper main removed.
- internal/policy: main's hidden-library output (#2058) and this branch's
  ContentAccessOnly input both declared `hidden`; the input side is
  renamed.
- Home sections (#2043): the admin per-profile card keeps main's row
  list but saves with this branch's override builder and a baseline, so
  like Settings > Home Screen it stores only what changed and untouched
  rows keep following the server. main's duplicate lib/sectionOverrides.ts
  is dropped; EditableSectionRows stays for that card and labels rows with
  rowKindLabel. Settings > Home Screen keeps this branch's page.
- apiv2: keep both sides' services and fixtures; the admin profile
  section settings fixture gains default_title.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
v2 answers a DELETE of a server collection that a row still shows with
409 and problem type "conflict"; collection_in_use is the v1 error code.
The List and bulk delete only matched collection_in_use, so their in-use
message, the fallback to the rows list, and bulk delete's "kept" count
never ran: the admin got the raw conflict text, and bulk delete counted
the collection as failed. Both now match the 409, and their tests use
the problem the server sends.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
POST /api/v2/catalog/query accepts at most 100 items a page, but the
collection editor asked for 200. Both requests failed with 422: saved
titles in a manual collection's editor showed no posters or years, and
the warning about titles only in an unticked library never appeared, so
unticking a library dropped titles without saying so.

fetchCatalogItems reads a source a page of at most 100 at a time through
the response cursor, up to an optional maximum. The untick warning reads
every page with it, and the editor reads the posters for its one page of
titles (COLLECTION_ITEMS_PAGE, 200) with it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Setting Show only back to All left display_query_definition out of the
PATCH, and the server keeps the saved filter when the field is absent.
The save reported success, then the reread brought the filter back.
An update that clears a saved filter now sends an empty one, which the
server stores as none.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
With collection groups retired, any update that carried a group_id
answered 501, even null, which asks for no group. A v1 client that sends
"group_id": null with a rename lost the whole update. A null or blank
group_id is now ignored on a store without groups; naming a group still
answers 501.

The v1 removals table and the collections API doc now also say that the
order's ordered_ids names only the acting profile's own collections.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Nothing imports lib/collectionGroups.ts since the grouped collection
boards were removed with personal collection groups.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Before it merged main, the collections revamp recorded its login-sharing
migration as SQLite schema v30, and main's v30 adds profiles.pin_revision.
A store a revamp build opened is at v30 without that column, so the merged
ladder skipped the column, ran the sharing conversion again, and installed
a PIN trigger that names a missing column: profile reads then fail, and
collections shared since lose their sharing because they carry no allow
list. A v30 store without pin_revision now gains the column and keeps its
sharing.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Since main's #2019, the admin area sends a session with no chosen profile
to the profile picker, so these route tests rendered the picker instead
of Home rows. Their signed-in admin now has a profile, as the other App
route tests do.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A catalog cursor is bound to the limit it was issued for, and a page can
come back short with more to follow (the SQLite store's bounded scan
does). fetchCatalogItems asked for a smaller last page to stop at its
maximum, so such a read ended in 400 invalid_cursor. It now asks for the
same limit on every page and trims the result to the maximum.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@Quick104 Quick104 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of cca184d2c merged with current main, with a live walk on a sandbox built from the merge: a login with three profiles (one a child profile limited to Movies and PG), personal collections created on main with groups and full and partial allow lists, then the upgrade. The migration did what the description says (fully shared stays shared, partly shared becomes private, groups emptied, one flat order per profile), writes from another profile answer 403, and shared collections, counts, and per-viewer collages follow the owner's and the viewer's access. Findings are on the lines below; three more don't fit a line.

High: the branch conflicts with main. GitHub reports 15 conflicting files (main is 97 commits ahead), and three changes on main collide with this branch's behavior, not just its text:

  • SQLite schema v30 on both sides (see the comment on internal/userdb/migrate.go).
  • main's per-profile Home sections card in Admin > Users > Preferences (#2043) was built on EditableSectionRows.tsx, which this branch deletes, and on a copy of the old override builder, which writes every field of every row. Merged as is, an admin's edit there would pin titles and limits that Settings > Home Screen now leaves to follow the server.
  • main and this branch each fixed "an empty library scope matches every library" (#1953, #2005, #2006); the query executor and section fetcher must take one version, and library collections keep ResolveLibraryCollectionMembership.

Since #2019 the admin area also needs a chosen profile, which fails this branch's admin route tests, and two helpers this branch uses were renamed or removed on main.

I'm merging main on this branch and resolving these. The admin card keeps main's row list but saves through lib/homeRows/profileOverrides.ts with a baseline, so it stores only what changed, and main's duplicate lib/sectionOverrides.ts goes. There's no need to change anything yourself.

Low: unreachable group code. Both user stores now report no collection groups, so everything after collectionFeatureError(store, "groups") can't run: the group service bodies in collections_service.go and collections_editor_service.go, the EnsureCollectionGroup branch of UpdatePersonalCollection, and the group_id branch of the Postgres updateCollectionAttempt, whose "next position in this group" logic contradicts the flat per-profile order if a store ever reported groups again. The guards and the store stubs are still needed for the frozen routes. web/src/lib/collectionGroups.ts has no importers left; I'm removing that file. The Go cleanup is yours to take or leave.

Low: a pre-existing rail mismatch, for a follow-up. A personal smart collection without a display filter is shown as a row through fetchFiltered, which replaces the collection's own limit with the row's item_limit, and a library page replaces the collection's library list instead of intersecting it. The row can then show titles the collection page doesn't. This is the same at the merge base, so it isn't from this PR.

Not raised: the open threads already cover position collisions past 200 titles in the manual editor (still reproduces) and the server collections cap on the Collections page.

Comment thread internal/userdb/migrate.go Outdated
Comment thread web/src/components/collections/editor/ManualContentsPanel.tsx Outdated
Comment thread web/src/components/collections/editor/CollectionEditor.tsx Outdated
Comment thread web/src/pages/AdminCollections.tsx Outdated
Comment thread web/src/lib/collections/scope.ts
/>
) : null}
</CollectionEditorShell>
{view && editor.etag && isServer ? (

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low: Delete does nothing after a create whose read-back failed. The fallback in collectionScope.ts sets the id without a view or ETag, the header still offers Delete, and both dialogs need view && editor.etag, so nothing opens. confirmDelete stays true, so the dialog opens on its own after the next reread (for example after adding a title).

Suggested repair: reread before opening the dialog when there's no ETag, or disable Delete until the collection has been read, and reset confirmDelete when no dialog can render. Not changed here.

const run = ++syncRun.current;
const fresh = await readFresh(id, false);
// A later read answers for both.
if (run !== syncRun.current) return { conflicts: current.current.conflicts };

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low, plausible: a save retry after a 412 can send a stale ETag. An overtaken syncWithServer returns before the newer read stores its ETag, and save then retries with current.current.etag. A second 412 marks every changed field as a conflict. The common case (a concurrent reread sharing one fetch) settles in the right order; it takes a newer run that starts its own fetch after this one's settles. A unit test with two separately resolvable snapshot fetches would confirm it.

Suggested repair: keep the newest run's promise and have an overtaken call await it. Not changed here.

if reads.unavailable[c.ID] {
continue
}
if n, ok := reads.counts[c.ID]; ok {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low, plausible: a failed owner-scoped count shows the stored count. When the grouped count query fails, the collection has no entry in reads.counts and keeps the stored item_count, which a sync wrote with the owner's access. A restricted viewer then sees a number that includes titles they can't open (no titles, just the number). An unresolvable owner is dropped instead, so the two failures differ. Not new: the base had the same fallback with no access filter at all.

Suggested repair: when the viewer isn't the owner and the count is missing, mark the collection unavailable or count 0. Not changed here.

updated.NextSyncAt = nextSyncAt
// Read the row back: a schedule edited while the sync ran, on any node,
// kept its own next run, and the caller renders what was stored.
updated, err := store.GetCollection(ctx, collection.ID)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low, plausible: a sync that succeeded can be recorded as failed. The read-back runs after ReplaceCollectionItems and UpdateCollectionSyncState have committed. If it fails (a pool timeout, a cancelled context), RunSync returns an error, the import handler then writes status failed with item_count 0 over the stored success, and a manual sync answers 500 for a sync that worked. Before this PR the view was built in memory, so this path couldn't fail after the write.

Suggested repair: on a read-back error, log it and render the values just written instead of failing. Not changed here.

if collectionID == "" {
return key, nil
}
revision, err := f.CollectionRepo.CollectionRevision(ctx, collectionID)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low, plausible (not measured): a cache hit still costs a query per collection row. resolvedListKey reads CollectionRevision before every lookup, so a Home render with N collection rows makes N primary-key queries even when every list is cached. The base built the key without a query.

Suggested repair: read the revisions of a render's collection rows in one query before the fan-out, or cache them per node for a few seconds. Not changed here.

@silo-kody

silo-kody Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Silo Kody — review complete

Review finished. Check the inline comments for findings and verify each suggestion against the code and tests.

Reviewing changes in Silo
  • Include the related issue, expected behavior, and validation steps in the PR description.
  • For API changes, describe the effect on Apple and Android clients and Jellyfin compatibility.
  • For plugin changes, identify the affected SDK contract, plugin, and catalog entry.
  • Follow this repository's AGENTS.md and CONTRIBUTING.md.
  • Request another review with @kody start-review in a PR comment.
  • React with 👍 or 👎 to give feedback on individual suggestions.
Review settings
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug ✅
Performance ✅
Security ✅
Business Logic ❌

@macroscopeapp

macroscopeapp Bot commented Oct 8, 2026

Copy link
Copy Markdown

The PR body has no ## Evidence section or Evidence: link. This change is visible, and the description and diff show these effects without evidence:

  • The personal Collections page and unified collection editor change their layout, categories, creation flow, sharing controls, and collection cards (web/src/pages/Collections.tsx, web/src/pages/CollectionEditorPage.tsx, web/src/components/collections/editor/CollectionEditor.tsx). Add before-and-after captures of the same data at desktop and phone widths; include a short recording of the multi-step create/edit and reorder flow.
  • Admin Collections changes to List and Arrange views, bulk actions, and starter packs (web/src/pages/AdminCollections.tsx, web/src/components/collections/admin/GroupsBoard.tsx, web/src/components/collections/StarterPacksDialog.tsx). Add desktop and phone-width before-and-after captures, plus a recording for the arrange or starter-pack flow.
  • Admin Home rows and profile Home Screen settings use new row lists, dialogs, and copy actions (web/src/pages/AdminHomeRows.tsx, web/src/pages/settings/HomeScreenSettings.tsx, web/src/components/homeRows/HomeRowsPage.tsx). Add desktop and phone-width before-and-after captures, plus a recording of the add/edit or copy flow.
  • Shared collection contents, counts, and artwork can differ by owner/viewer access, and collection rows can show different items or ordering (internal/api/handlers/collections_service.go, internal/sections/fetcher.go, internal/catalog/personal_collection_collages.go). Show before and after for the same collection or row with the same query and access, including any changed titles, count, or artwork; use UI captures where rendered and same-request response excerpts only for behavior no client displays.

Name the surface and build or commit for each capture. For web surfaces also include mobile evidence where the change is visible at phone width. A link to the private evidence page is sufficient.

Automated check: Macroscope check run agent (gpt-6-luna). Evidence was not reviewed for correctness.

Posted via Macroscope — Visible change evidence

Conflict resolution: main's video_with_episodes search scope (#1995) and
this branch's facet value search each added a flag to the catalog search
capabilities; both are kept, and the OpenAPI document, fixtures, and web
types are regenerated.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@Quick104

Quick104 commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

Review summary

Reviewed head: 839f62ba1 (the review started at cca184d2c; this branch now also carries two merges of main and the fixes below). Findings are in the review.

Fixed on this branch

  • The branch conflicted with main. Merged twice; the parts that changed behavior (SQLite schema v30, the admin per-profile Home sections card from feat(admin): let admins edit one profile's home section order #2043, empty library scopes, the admin area's profile requirement) are described under Approach in the PR body. 3dfc62f, e716be9, 0d3a309, 2762dba.
  • The collection editor's catalog reads went over the 100-item page limit, so saved titles had no posters or years and the library untick warning never showed. 50ffbd8, 2316cb1 (the second from the cross-provider round: keep one page size through a cursor).
  • The admin List and bulk delete never matched v2's in-use 409. 71b91f4.
  • Show only couldn't be set back to All. d62c333.
  • An update with "group_id": null failed as a whole. aa6006a.
  • An unused groups helper on the web. 2f9cf28.
  • Empty smart collections held collage build-queue slots and could fill the node's queue. 839f62b, with a database subtest that passes on a migrated Postgres and fails with the old behavior (that test isn't in CI's database lists).

Open

  • Decided: /api/v1 create and update keep ignoring allowed_profile_ids, as documented, since v1 is being retired. A v1 client that shares with some profiles shares with every profile on the login.
  • Low, left as suggestions on their lines: description on create with the SQLite user store, Add as a row landing on Home when the libraries read fails, Delete after a failed create read-back, a stale ETag on a save retry, the stored count shown when the owner-scoped count fails, a sync recorded as failed when its read-back fails, and a revision query per collection row on cached Home renders.
  • Follow-up, not from this PR: a personal smart collection shown as a row uses the row's limit and the page's library instead of the collection's own; the same at the merge base.
  • Existing threads: position collisions past 200 titles in the manual editor still reproduce; the server collections cap on the Collections page is still open.

Validated features

Validation through the board script was not available (the token lacks read:project), so this comes from the validation issues' results tables. No passed case regresses. The cases the PR body lists as changed (#193 S1–S3 Pass, S4 Partial) change as it says. One addition for S4: a query_definition on a manual or synced server collection no longer makes it live; only collection_type smart does. The admin per-profile Home sections card has no validation case. The Validation tasks: line in the body stands.

Verification

A sandbox upgraded from main with three profiles and pre-revamp sharing and groups, then walked on the web; the migration, the 403s and 404s, owner and viewer counts and collages, group 501s, the editor, Add to collection, the admin collection pages, and Home rows behaved as described, and the fixes were checked on a rebuild. Go and web suites, lint, build, the bundle budget, and the contract checks pass locally; only TestRunStatsWithRealFFmpeg fails, as on main. CI passed on 2762dbade, including the database jobs; 839f62ba1 adds only the collage fix.

Evidence: https://evidence.siloserver.org/r/silo-server/pr-2013/

User manual

Filed as Silo-Server/siloserver.org#121, with draft docs PR Silo-Server/siloserver.org#122 for the collections pages, Curate the home screen, and Change your home rows. It stays a draft until this PR merges. The Admin > Users Home sections card is tracked separately in siloserver.org#119.

Apple and Android

Follow-ups are open: Silo-Server/silo-apple#534 and Silo-Server/silo-android#406. silo-apple hides its group controls on groups: false or a 501; silo-android omits allowed_profile_ids when null. Not checked: how either app handles the own-collections rule on reorder before those land.

Cross-provider review

Two rounds with gpt-6.1-sol through Codex on the merged build and on the fixes. Round one found the SQLite v30 store case (fixed), a race between the admin card's two reads (judged no worse than before, since neither builder has a revision guard), and the pre-existing rail mismatch above. Round two found the cursor page-size problem (fixed) and nothing else.

Fit: 7/10 · Readiness: 7/10 — conditional

Fit 7. It fixes a real ownership defect (one profile could reorder or regroup another's collections), puts each decision on the server, removes parallel editors, and stops offering actions the server refuses, which is what AGENTS.md asks for in this QA era. It also adds a lot of new surface in that era: per-viewer collages with a background build queue, sync schedules, starter packs, and new contract fields. The v1 sharing behavior is kept as documented because v1 is being retired.

Readiness 7. The approach holds up on a real upgrade, and the defects found are fixed with tests. What's left: the validators' walks of the changed and not-yet-run cases on a build with this merge, and the native follow-ups.

@Quick104
Quick104 marked this pull request as ready for review October 8, 2026 02:34

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2762dbade6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const claimed = useRef(0);

async function afterWrite() {
await scope.invalidate(queryClient, collectionId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset the paged item view after membership writes

When the editor is on the second or a later page, adding a title or restoring one via Undo calls afterWrite while retaining the current cursor. Each membership write advances the collection revision encoded in that cursor, and this invalidation immediately refetches the active query with the stale cursor, so the API returns an invalid-cursor conflict and the panel replaces the list with its error state. Reset page to the first page before invalidating, as the remove path already does.

Useful? React with 👍 / 👎.

…fresh

A smart collection with no poster among the viewer's matches recorded
that it has no collage and then returned ErrNotEnoughImages, so the build
queue kept a ten-minute retry marker for that viewer and collection. The
markers count toward the queue's 256-entry cap, which is shared by every
account on the node, so enough empty smart collections read at once made
it refuse every other personal collage build there. The recorded empty
state already holds off the next refresh until SmartRefreshInterval, so
the refresh now succeeds and frees its slot.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@silo-kody

silo-kody Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Silo Kody — review complete

Review finished. Check the inline comments for findings and verify each suggestion against the code and tests.

Reviewing changes in Silo
  • Include the related issue, expected behavior, and validation steps in the PR description.
  • For API changes, describe the effect on Apple and Android clients and Jellyfin compatibility.
  • For plugin changes, identify the affected SDK contract, plugin, and catalog entry.
  • Follow this repository's AGENTS.md and CONTRIBUTING.md.
  • Request another review with @kody start-review in a PR comment.
  • React with 👍 or 👎 to give feedback on individual suggestions.
Review settings
Review Options

The following review options are enabled or disabled:

Options Enabled
Bug ✅
Performance ✅
Security ✅
Business Logic ❌

@macroscopeapp

macroscopeapp Bot commented Oct 8, 2026

Copy link
Copy Markdown

The evidence link is present, but the description does not identify evidence for these affected surfaces:

  • Library > Collections: shared personal collections now appear there with owner bylines. Please ensure the linked page includes before-and-after desktop and mobile captures of this tab.
  • Rendered Home/library pages: collection rows can now be added and show collection content. Please include before-and-after captures of the same row with the same profile and library/query, on desktop and mobile where visible at phone width; a recording can cover the add-to-row flow.

Automated check: Macroscope check run agent (gpt-6-luna). Evidence was not reviewed for correctness.

Posted via Macroscope — Visible change evidence

@Quick104
Quick104 merged commit 0beaf66 into main Oct 8, 2026
20 checks passed
@Quick104
Quick104 deleted the feat/collections-revamp branch October 8, 2026 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant