feat: per-user favorites — asset_favorite overlay, viewer hearts, cross-scope filters (#763) - #819
Open
Deeds67 wants to merge 67 commits into
Open
feat: per-user favorites — asset_favorite overlay, viewer hearts, cross-scope filters (#763)#819Deeds67 wants to merge 67 commits into
Deeds67 wants to merge 67 commits into
Conversation
github-actions
Bot
added
documentation
Deeds67
force-pushed
the
worktree-fix-763-space-member-favorite
branch
2 times, most recently
from
July 22, 2026 18:14
927665b to
06e9601
Compare
Deeds67
force-pushed
the
worktree-fix-763-space-member-favorite
branch
from
July 30, 2026 07:30
97ccb12 to
9f81e09
Compare
- §5.1: shared-link rationale was wrong (auth.user is non-optional); requires an explicit auth.sharedLink guard or a link visitor writes as the link owner - §4.2: revert-to-immich.spec.ts filters asset_favorite out; slice 0 must widen the filter or the table ships with zero revert coverage - Slice 4: removing the guard alone is inert — timeline.service.ts:164 owner-scoping must be replaced with readable-asset scoping - §9: slices are review units within one PR, not independently shippable - Slice 1: no longer claims to be a pure refactor - Add E16b, E27-E32 (isFavorite:false, concurrency, bulk limit, album/partner revocation, stacks, mobile toggle direction) - Slice 6: favorite.action.dart:11 computes direction over the unfiltered selection - §10: performance budget, shared-link and cross-scope criteria
Slice-0 planning refuted §4's no-audit-table assumption. Sync streams page on updateId (sync.repository.ts:136-143); a favorite write that touches only asset_favorite never advances asset.updateId, so the change never reaches mobile via incremental sync — and a full resync masks it. Rejected bumping asset.updateId: a viewer favoriting would mutate the owner's row and re-sync it to every member. Chosen: asset_favorite is its own synced entity with createId/updateId + asset_favorite_audit, mirroring album_space_asset. Both tables land in slice 0, before the irreversible drop.
E10's leak test cannot go red in slice 1: isFavorite + withSharedSpaces is a hard 400 (timeline.service.ts:177-200) and plain /favorites is owner-scoped (asset.repository.ts:421-423), so no non-owned favorite can appear in any listing until slice 4 removes both. No access filter in slice 1.
Threads an explicit authUserId (the caller, distinct from userIds — the timeline target, which differs on space/album browse paths) through TimeBucketOptions so withTimeBucketAssetFilters, getTimeBucket's masked select, and getStatistics resolve isFavorite against the asset_favorite overlay instead of the ownership-masked asset.isFavorite column. mapAsset (asset-response.dto.ts:228) is deliberately NOT changed here — see report to caller for why (B finding): virtually every query feeding it (getById, getByIds, update, search/map, ...) still selects the raw column, so flipping the mapper now would leak raw isFavorite to non- owners on those paths. Left for a follow-up that also migrates those queries. Also dedupes a repeated asset.isFavorite entry in database.ts's workflowAssetV1 column list.
…rlay (#763) Threads an explicit authUserId through searchAssetBuilder, buildFilteredAssetIds and getFilteredMapMarkers rather than reusing userIds[0], which is the timeline target and not the caller on space/album paths. SQL snapshots are NOT regenerated here — see follow-up.
… filters (#763) Follow-up to the previous commit's note ("SQL snapshots are NOT regenerated here"). Running the SQL generator against a freshly migrated DB surfaced a real bug it caught: searchMetadata, searchStatistics, searchRandom, searchLargeAssets and searchSmart's @GenerateSql fixtures set isFavorite: true without an authUserId, so favoriteExistsFor's asUuid(undefined) crashed the postgres.js driver (UNDEFINED_VALUE) instead of producing a query to capture. Added authUserId: DummyValue.UUID to each fixture and regenerated src/queries/*.sql. asset.repository.sql also picks up the previous slice's (#763 timeline reads) already-landed EXISTS substitution, whose snapshot was left stale. search.repository.sql now reflects the same asset.isFavorite -> EXISTS (asset_favorite ...) substitution for the search/facet paths. map.repository.sql and shared.space.repository.sql are unchanged, as expected: their @GenerateSql fixtures never set isFavorite, so the branch isn't exercised by the snapshot generator (verified by direct medium tests instead).
Threads the caller's id (authUserId) into every auth-bearing query that feeds mapAsset/mapMemory/mapStack, projecting favoriteExistsFor(...) as isFavoriteForUser so GET /assets/:id, search, memories, stacks, and the folder view resolve isFavorite from the per-user asset_favorite overlay instead of the fail-safe false left by slice 1b Task 1. Four call sites are deliberately left unprojected (mapSharedLink, the background job/notification websocket pushes, and duplicate resolution) with comments explaining why, so they aren't "fixed" back into a leak.
addAll/removeAll write the asset_favorite overlay for a given user, mirroring the tag.repository.ts bulk-array idiom (Chunked + onConflict doNothing). Registers the repository in BaseService's three positional sites and the medium/unit test factories (test/medium.factory.ts, test/utils.ts) that construct BaseService positionally.
…te alias (#763) Canonical per-user favorite write path gated on Permission.AssetRead (space viewers included), with an explicit auth.sharedLink rejection so an anonymous share-link visitor can never write a favorite attributed to the link owner. The existing isFavorite field on PUT /assets/:id and PUT /assets becomes a deprecated alias that routes into the same AssetService.updateFavorites method, behind its own stricter Permission.AssetUpdate guard so it can never be more permissive than the canonical endpoint. Also fixes a boot-blocking gap from the prior commit: AssetFavoriteRepository was wired into BaseService but never registered as a real Nest provider in src/repositories/index.ts, so the server threw UnknownDependenciesException on startup. Threads the per-user favorite overlay through the workflow/plugin engine's AssetV1 read (workflow-execution.service.ts), which previously read the legacy asset.isFavorite column and would have made an assetFavorite plugin toggle step never converge. Inverts a bug-encoding e2e assertion in shared-space-album.e2e-spec.ts: a space editor's bulk-favorite of another member's asset now only creates the editor's own row, leaving the asset owner's favorite state untouched.
Upload (asset-media.service.ts:352), copy (asset.service.ts:557) and duplicate-merge (duplicate.service.ts:308) still write/read asset.isFavorite. Slice 3 drops that column, so the original 3-before-7 ordering would have broken the build at the one irreversible step.
Catches up overlay-projection snapshots that slices 1/1b/7 changed without regenerating (asset, memory, search, stack, view isFavoriteForUser blocks), converts the sync stream blocks to recipient-resolved overlay joins (slice 6 task 1), adds the new SyncRepository.assetFavorite.getUpserts/getDeletes blocks (task 2), and renames asset-favorite.repository.sql to the generator-canonical asset.favorite.repository.sql (content identical).
…tion direction, canonical endpoint (#763)
…erlay (#763) job.service.ts's AssetEditReadyV2 / AssetUploadReadyV2 payloads mapped isFavorite off the raw asset.isFavorite column via assetRepository.getById / getByIdsWithAllRelationsButStacks called with no authUserId -- a column the overlay write path has stopped updating since slice 2, so these two owner-scoped events went stale. Both repository methods now project isFavorite unconditionally via favoriteExistsForOwner, restoring the raw column's exact pre-#763 owner semantics for every consumer (only job.service.ts reads .isFavorite directly off these methods; every other consumer routes through mapAsset's isFavoriteForUser instead).
… the single source of truth (#763) asset_favorite has been the write path since slice 1 and the read path (mapAsset's isFavoriteForUser, the plugin-facing workflowAssetV1 projection, the owner-scoped job.service.ts websocket payloads) since slice 1b/2. This removes the column it replaced: a new migration (1784100000000-DropAssetIsFavoriteColumn) drops it, with a down() that restores it and backfills from the overlay for already-deployed DBs and the revert-to-immich path. Fallout from the field removal, fixed by overlay conversion or dead-code deletion (never by re-adding the field): - MapAsset.isFavorite (dtos/asset-response.dto.ts) deleted outright — dead since mapAsset already reads isFavoriteForUser exclusively. - Test factories/fixtures that seeded the raw column (test/factories/asset.factory.ts, test/medium.factory.ts assetInsert, test/mappers.ts getForAsset) switch to asset_favorite inserts or a synthesized isFavorite default matching the new unconditional repository projection. - Sync-spec "bump asset.updateId" helpers that repurposed a raw-column UPDATE as a no-op metadata change now touch originalFileName instead. - asset-favorite.repository.spec.ts's two tests exercising the slice-0 migration's backfill SQL against a live schema are removed — that schema no longer exists to run them against; the migration file's own historical correctness is untouched. - scripts/revert-to-immich.sql step 8 lists the new migration name (revert-to-immich.spec.ts's step-8 test went red on the bare migration file before this, confirming the gap).
The revert-validation workflow runs the script against the released main image, whose schema predates the asset_favorite overlay — the unguarded UPDATE ... FROM asset_favorite aborted the transaction there. to_regclass guard skips the backfill when the overlay never existed (the raw column still carries the favorites in that state).
…763) The web writes favorites via the canonical per-user endpoint since slice 5; the ui-project mocks still intercepted the old bulk PUT /assets route, so the awaited favorite request never matched and the three favorite flows timed out in CI.
#763) Rebase fallout from #813. That PR added `recently-added-filter-options`, whose spec pinned the pre-#763 rule that a Favorites filter drops the partner scope (a favorite used to be the owner's flag on the asset, so a partner's favorites meant nothing to the viewer). #763 makes favorites per-user via the `asset_favorite` overlay, so `buildPhotosTimelineOptions` no longer strips `withPartners` / `withSharedSpaces` when filtering by favorite — the viewer's favorites on partner assets are their own and must stay in scope. Recently Added still strips `withSharedSpaces` unconditionally (its own+partner invariant), so only the partner expectation changes. Updates the assertion plus the two now-stale comments describing the old scoping (the module doc and the e2e scope-invariant note).
…troduce drift (#763) Migration 1784000000000 creates the asset_favorite_delete_audit function, its statement-level AFTER DELETE trigger, and both migration_overrides rows — but nothing declared them in functions.ts or on AssetFavoriteTable. That is exactly the drift class main just eliminated in #827 for album_space_asset_delete_audit: getSchemaDrift runs with `functions: { ignoreExtra: false }`, so a function that exists only in the database reports FunctionDrop plus two OverrideDrops, every instance boots with "Detected schema drift", and `migrations:generate` against a real database emits DROP TRIGGER + DROP FUNCTION — which executes cleanly and would silently remove the favorite delete sync stream. CI cannot catch this: SQL Schema Checks runs `schema:reset` (rebuilding the DB from code) before `migrations:generate`, so a migration-vs-code divergence is invisible there. Verified with the real generator that schemaFromCode({ overrides: true }) emitted zero asset_favorite artefacts before this change. Declares both halves byte-identically to the migration's DDL, mirroring AlbumSpaceAssetTable, and extends the two parity specs with the asset_favorite cases so the decorator/function can't rot away from the migration again.
…ine (#763) Two fallouts from rebasing onto main, both in tests: - selection-capabilities.spec.ts: the E1 space-viewer case asserts the full capability object via an ALL_FALSE spread, so it needed canFavorite: true explicitly now that favoriting is ownership-agnostic. - search.repository.spec.ts (medium): main's #858 filter-narrowing tests seeded favorites with `ctx.newAsset({ isFavorite: true })`. That column is dropped in this branch, and git auto-merged both sides with no conflict, so the breakage only surfaced in tsc. Reseeded through the asset_favorite overlay, matching the pattern the other medium specs already use.
v3.1.0 introduced three new reads of the `asset.isFavorite` column this branch drops, plus two toolbar assertions written against the ownership gate #763 removes. - `columns.searchAsset` (database.ts, new upstream explicit column list replacing `selectAll('asset')`) listed `asset.isFavorite`. Removed, with the same note as `syncAsset` / `workflowAssetV1`: the five consumers project the per-user overlay separately via `favoriteExistsFor(eb, options.authUserId)`. - The dormant Search V3 `branchPredicates` compared `asset.isFavorite` directly. Ported onto `favoriteExistsFor`, resolved for the CALLER via a new optional `AssetSearchBuilderV3Options.authUserId` mirroring the legacy builder. V3 stays unwired; whoever wires it must thread the id. - SQL snapshots: dropped the `asset.isFavorite` select entry from the 25 affected generated select lists, replaced the three V3 `isFavorite` predicates with the overlay EXISTS form (param positions unchanged), and regenerated the stale `getAssetsByCity (with authUserId)` section that still carried the pre-v3.1.0 `asset.*`. All 30 touched sections verified byte-identical against an offline Kysely compile of the same builders. - Flipped the two `space-album-detail-page.spec.ts` and the one `spaces-selection-toolbar-person.e2e-spec.ts` Favorite-absence assertions that v3.1.0 still gates on ownership, matching the already-flipped -album/-timeline siblings: `canFavorite` is unconditional, so a non-owned selection — including a read-only space viewer's — keeps the heart.
…s enum shape (#763) Two v3.1.0 renames the rebase auto-merged around: - openapi-generator v7.24 emits `SyncRequestType` as a real Dart enum, so the M14 capability filter reads `type.toJson()`, not `type.value`. The `_spaceAlbumSyncTypes` sibling line auto-merged onto the new form; the `_assetFavoriteSyncTypes` line added by #763 did not, and `.value` no longer exists — `flutter test` failed to compile five files. tsc/dart analyze on the pre-fix tree caught it only via the failing compile. - `FavoriteAction.shouldFavorite` was renamed to `favorite` upstream; `favorite_action_test.dart` still read the old name.
… upstream did not sweep (#763)
Deeds67
force-pushed
the
worktree-fix-763-space-member-favorite
branch
from
July 30, 2026 08:45
9f81e09 to
0a2e7e1
Compare
…tes (#763) `asset_favorite` rows are deliberately never cleaned up when a user loses access (design §5.2): visibility is re-derived from live membership on read. That makes the invariant a pure query-scoping property, so it is covered where the scoping lives. Medium spec drives the real TimelineService and SearchService so both halves run for real — `getSpaceIdsForTimeline` resolving current membership, and the favorite predicate being ANDed with the RBAC scope in `withTimeBucketAssetFilters` / `searchAssetBuilderLegacy`. Two cases: the member's last space is revoked (queries fall back to `asset.ownerId = caller`), and a second membership survives (the space arm stays active but narrows). Every absence assertion is paired with a must-remain control asset in the same response, so a query that returned nothing could not read as green. Adds matching compile-only assertions to the two query-builder specs, which run in the unit job without a database: the favorite predicate must always sit alongside either the ownerId scope or the timelineSpaceIds arm. Both go red if the ownerId arm is removed. E10 was already covered end-to-end in asset-favorite.e2e-spec.ts, but only via bucket counts on the timeline path and with no control asset.
…voriting (#763) `AssetService.update` / `AssetApiRepository.update` still declare an `isFavorite` parameter (upstream Immich shapes, kept as-is for rebase hygiene) that posts an `AssetBulkUpdateDto` to the owner-only `PUT /assets`. Favorites are now a per-user overlay written through `PUT /assets/favorites`, so a favorite routed that way would be owner-gated again. Nothing passes it today — `AssetService.update` has no callers — but it is the obvious thing to reach for when wiring the heart icon to the consolidated action. Adds a policy test with two frozen inventories: the `AssetBulkUpdateDto` construction sites carrying `isFavorite` (the choke point every route to the bulk endpoint must pass through — sanctioned sites are the dead consolidated `update` and the deliberate pre-5.2.0 `updateFavorite` fallback), and the `.update(...)` call sites passing `isFavorite:`. Failures name the offending member and point at `updateFavorite`. Placed in the mobile suite rather than extending the server grep gate because CI gates the server unit job on `server/**`, so a mobile-only change would not run it; `mobile-unit-tests` is gated on `mobile/**`. Also adds pointer comments at both `update({isFavorite})` declarations.
…filter (#763) The dedicated /favorites page requested only { isFavorite, withStacked }, so it took the owner-only arm of searchAssetBuilderLegacy. The Photos timeline favorites filter, by contrast, sets withPartners/withSharedSpaces (buildPhotosTimelineOptions, slice 4). That asymmetry meant a favorite a member placed on another member's Space asset — which the per-user overlay explicitly permits — appeared in the Photos filter but never on /favorites, so it was effectively unreachable. Access is still enforced per request: the server recomputes timelineSpaceIds from current shared_space_member rows, so losing Space access drops the asset from results even though the overlay row survives. That invariant is covered by favorite-access-revocation.medium.spec.ts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #763.
What
Favoriting becomes a per-user fact — a
(userId, assetId)overlay table (asset_favorite) replaces the globalasset.isFavoriteboolean, which is dropped. Any user who can read an asset can favorite it (space viewers included); nobody's favorite is visible to, or mutable by, anyone else. Design:docs/superpowers/specs/2026-07-20-per-user-favorites-design.md.Highlights per layer
asset_favorite+asset_favorite_audit(delete tombstones), owner-backfilled from the old column;revert-to-immich.sqlgains its first column-restore step (non-owner rows are intentionally discarded on revert — documented).favoriteExistsFor(caller)overlay join;AssetResponseDtoshape unchanged.PUT /assets/favoritesgated onPermission.AssetRead, explicit shared-link rejection; the oldisFavoriteDTO fields remain as a strictly-narrower deprecated alias into the same service method.isFavorite × withSharedSpaces/withPartners400-guards are gone (favorite arm only — archive/trash guards stay); favorited stack children surface standalone (spec §5.4); map + filtered-map compose. Perf budget on 10 spaces / 10k cross-owner favorites: buckets ~45-70ms, first bucket ~0.9-1.9s vs 5000ms ceiling (favorite-cross-scope.spec.ts).fshortcut, multi-select bar, cmdk, 7 route surfaces) with an explicit shared-link guard; writes go to the canonical endpoint; Playwright proves the literal [bug] Favorite action missing from photo action menu for non admin Space members #763 scenario.asset_favoriteis its own synced entity (AssetFavoritesV1, upserts + tombstones — favorite writes never bumpasset.updateId, so no cross-member re-sync amplification); the heart un-gates with the E32 mixed-selection direction bug fixed; both stream request AND write path are version-gated (> 5.2.0) with a legacy-endpoint fallback so app↔server skew degrades gracefully.workflowAssetV1.isFavoriteand job websocket payloads resolve the owner's overlay (fixing a latent staleness bug found in review); a grep-gate unit test permanently forbids raw column references.Review posture
Every task passed an independent spec+quality review; slices 4/6 and the whole branch got additional cross-cutting reviews (RBAC leak analysis, sync two-device story, version-skew matrix, §10 acceptance sweep). Full gates at HEAD: server unit 5107, medium 543 (incl. 65-file sync suite), e2e API 339 across 9 files, web 3669 + tsc/lint, mobile 2824 +
dart analyze --fatal-infos, full lint/prettier sweeps clean.Decisions worth reviewer attention
/favoritesnamed web route unchanged: it is archive-inclusive by design (default visibility), whilewithSharedSpacesrequires explicitvisibility=timeline(retained guard) — wiring spaces in would silently drop archived favorites. Cross-scope favorites surface via the photos-page favorite filter, search, and map. Follow-up decision documented in the slice-4 plan.Open item before merge