feat(server): resolve duplicates#25316
Merged
jrasm91 merged 33 commits intoimmich-app:mainfrom Mar 26, 2026
Merged
Conversation
Phlogi
changed the title
immich-push-o-matic
bot
added
the
documentation
jrasm91
force-pushed
the
deduplicate-sync-album
branch
2 times, most recently
from
76849ac to
e57288f
Compare
jrasm91
reviewed
Feb 2, 2026
jrasm91
force-pushed
the
deduplicate-sync-album
branch
from
7718399 to
6a86465
Compare
* Added new settings menu to the the deduplication tab. * The toggable options in the settings are synchronization of: albums, favorites, ratings, description, visibility and location. * When synchronizing the albums, the resolved images will be added to all albums of the duplicates. * When synchronizing the favorite status, the resolved images will be marked as favorite, if at least one selectable image is marked as favorite. * When synchronizing the ratings, the highest rating from the selectable images will be applied to the resolved image. * When synchronizing the description, all descriptions from the selectable images will be merged into one description for the resolved image. * When synchronizing the visibility, the most restrictive visibility setting from the selectable images will be applied to the resolved image. * When synchronizing the location, if exactly one unique location exists among the selectable images, this location will be applied to the resolved image. * There is no additional UI for these settings to keep the visual clutter minimal. The settings are applied automatically based on the user's preferences.
This update introduces tag synchronization for duplicate resolution, ensuring all unique tag IDs from duplicates are applied to kept assets. The visibility sync logic is updated to use a simplified ordering, as the hidden status items will never show up in a duplicate set. Album synchronization now merges albums directly via addAssetsToAlbums; as the approach with copyAsset API endpoint was ineffiecient. Description, rating, and location sync logic is improved for correctness. and deduplication. i18n strings were added / updated.
…king Moves duplicate metadata synchronization from frontend to backend, enabling robust batch operations and proper validation. This is an improved refactor of PR immich-app#13851. New endpoints: - POST /duplicates/resolve - batch resolve with configurable metadata sync - POST /duplicates/stack - create stacks from duplicate groups - GET /duplicates - now includes suggestedKeepAssetIds based on file size and EXIF Key changes: - Move sync logic (albums, tags, favorites, ratings, descriptions, location, visibility) to server - Add server-side metadata merge policies with proper conflict resolution - Replace client-side resolution logic with new backend endpoints - Add comprehensive E2E tests (70+ test cases) and unit tests - Update OpenAPI specs and TypeScript SDK No breaking changes - only additions to existing API.
jrasm91
force-pushed
the
deduplicate-sync-album
branch
from
6a86465 to
b6d4fa6
Compare
mertalev
reviewed
Feb 2, 2026
14 tasks
jrasm91
force-pushed
the
deduplicate-sync-album
branch
from
10c6603 to
00cbfeb
Compare
jrasm91
approved these changes
Mar 20, 2026
Member
|
@Phlogi I think this is ready to go in the next release. If you have time, do you mind doing another round of testing to make sure it still works correctly? |
jrasm91
changed the title
Contributor
Author
Great, ran my test sets and it looks good. One thing I noticed about the rating value: File: server/src/services/duplicate.service.ts (lines 256-263) Small issue: The merge always sets exifUpdate.rating = 0, even when no duplicate has a rating. Fix: Guard the rating assignment the same way description and latitude/longitude are guarded right below it: @jrasm91 it's almost cosmetic, but could be implemented, let me know. |
Member
|
Yeah feel free to push that change. Thanks for testing! |
Contributor
Author
Done, also added a missing type cast. |
jrasm91
force-pushed
the
deduplicate-sync-album
branch
from
8f3a2e7 to
7607161
Compare
oPeras1
pushed a commit
to oPeras1/immich
that referenced
this pull request
Mar 27, 2026
* feat(web): Synchronize information from deduplicated images * Added new settings menu to the the deduplication tab. * The toggable options in the settings are synchronization of: albums, favorites, ratings, description, visibility and location. * When synchronizing the albums, the resolved images will be added to all albums of the duplicates. * When synchronizing the favorite status, the resolved images will be marked as favorite, if at least one selectable image is marked as favorite. * When synchronizing the ratings, the highest rating from the selectable images will be applied to the resolved image. * When synchronizing the description, all descriptions from the selectable images will be merged into one description for the resolved image. * When synchronizing the visibility, the most restrictive visibility setting from the selectable images will be applied to the resolved image. * When synchronizing the location, if exactly one unique location exists among the selectable images, this location will be applied to the resolved image. * There is no additional UI for these settings to keep the visual clutter minimal. The settings are applied automatically based on the user's preferences. * Replace addAssetToAlbums with copyAsset * fix linter * feat(web): add duplicate sync fields and fix typo * feat(web): add tag sync and enhance duplicate resolution This update introduces tag synchronization for duplicate resolution, ensuring all unique tag IDs from duplicates are applied to kept assets. The visibility sync logic is updated to use a simplified ordering, as the hidden status items will never show up in a duplicate set. Album synchronization now merges albums directly via addAssetsToAlbums; as the approach with copyAsset API endpoint was ineffiecient. Description, rating, and location sync logic is improved for correctness. and deduplication. i18n strings were added / updated. * feat(server): move duplicate resolution to backend with sync and stacking Moves duplicate metadata synchronization from frontend to backend, enabling robust batch operations and proper validation. This is an improved refactor of PR immich-app#13851. New endpoints: - POST /duplicates/resolve - batch resolve with configurable metadata sync - POST /duplicates/stack - create stacks from duplicate groups - GET /duplicates - now includes suggestedKeepAssetIds based on file size and EXIF Key changes: - Move sync logic (albums, tags, favorites, ratings, descriptions, location, visibility) to server - Add server-side metadata merge policies with proper conflict resolution - Replace client-side resolution logic with new backend endpoints - Add comprehensive E2E tests (70+ test cases) and unit tests - Update OpenAPI specs and TypeScript SDK No breaking changes - only additions to existing API. * feat(preferences): enable all duplicate sync settings by default * chore: clean up * chore: clean up * refactor: rename & clean up * fix: preference upgrade * chore: linting * refactor(e2e): use updateAssets API for setAssetDuplicateId * fix: visibility sync logic in duplicate resolution * fix(duplicate): write description to exifUpdate Previously the duplicate resolution populated assetUpdate.description even though description belongs to exif info. * fix(duplicate): remove redundant updateLockedColumns wrapper updateAllExif already computes lockedProperties via distinctLocked using Object.keys(options). The wrapper added a lockedProperties key to the options object, causing the spurious string 'lockedProperties' to be stored in the lockedProperties array. * fix(duplicate): write merged tags to asset_exif to survive metadata re-extraction During duplicate resolution, replaceAssetTags correctly wrote merged tag IDs to the tag_asset table, but never updated asset_exif.tags or locked the tags property. The subsequent SidecarWrite → AssetExtractMetadata chain calls applyTagList, which destructively replaces tag_asset rows with whatever is in asset_exif.tags — still the original per-asset tags, not the merged set. Write merged tag values to asset_exif.tags via updateAllExif (which also locks the property via distinctLocked), and queue SidecarWrite when tags change so they persist to the sidecar file. * docs(duplicates): clarify location and tag sync behavior * refactor(duplicate): remove sync settings, always sync all metadata on resolve Remove DuplicateSyncSettingsDto and the per-field sync toggles (albums, favorites, rating, description, visibility, location, tags). Duplicate resolution now unconditionally syncs all metadata from trashed assets to kept assets. - Remove DuplicateSyncSettingsDto and settings field from DuplicateResolveDto - Update DuplicateService to always run all sync logic without conditionals - Delete DuplicateSettingsModal.svelte and settings gear button from UI - Remove DuplicateSettings type and duplicateSettings persisted store - Update unit and e2e tests to remove settings from resolve requests * docs: update duplicates utility to reflect automatic metadata sync * docs(web): replace duplicates info modal with link to documentation * chore: clean up * fix: add missing type cast to jsonAgg in duplicate repository getAll * fix: skip persisting rating=0 in duplicate merge to avoid unnecessary sidecar write --------- Co-authored-by: Toni <51962051+EinToni@users.noreply.github.com> Co-authored-by: Jason Rasmussen <jason@rasm.me> Co-authored-by: Jason Rasmussen <jrasm91@gmail.com>
eleboucher
pushed a commit
to eleboucher/homelab
that referenced
this pull request
Apr 7, 2026
….0) (#101) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [immich-app/immich](https://github.com/immich-app/immich) | minor | `v2.6.3` → `v2.7.0` | --- ### Release Notes <details> <summary>immich-app/immich (immich-app/immich)</summary> ### [`v2.7.0`](https://github.com/immich-app/immich/releases/tag/v2.7.0) [Compare Source](immich-app/immich@v2.6.3...v2.7.0) ### v2.7.0 Welcome to Immich `v2.7.0`! This release includes enhancements to the asset viewer, security improvements, changes to the duplicate APIs and viewer, and a bunch of bug fixes. Keep reading below for the complete highlights and a note on the upcoming `v3.0.0` release. > \[!NOTE]\ > We're working on a managed backup service for Immich with end-to-end encrypted backups of your library to a remote datacentre where only you hold the keys. > > We've put together a quick survey (\~5 mins) to get a better idea of how you're backing things up today and what you'd actually want from something like this. Your answers help us figure out what to prioritise, so we'd really appreciate it if you took a few minutes to fill it out. > > Leave your email at the end if you're interested in joining our free closed beta when it's ready. > > <https://futo-backups-survey.immich.app/> #### Known limitations - The machine learning service on `amd64` currently requires the `>= x86-64-v2` microarchitecture. This will be patched in an upcoming patch release for backward compatibility with very old processors (before \~2010), but it will become a minimum requirement in 3.0. `arm64` is not affected by this change. #### Highlights - Remove from album (asset viewer) - Move to locked folder (folder page) - Editor shortcuts - Create a new face on-the-fly in the face tag editor - Resolve duplicates - Helmet configuration - Version check infrastructure - Notable fix: live photo and video download in Safari - Notable fix: escape HTML in the Panorama Photo Viewer ##### Remove from album The web has a new action, "Remove from album," available in the asset viewer that makes it easier to remove an asset from an album. This action is available to both album and asset owners. <img width="360" height="202" alt="image" src="https://github.com/user-attachments/assets/e5facb24-ed10-4adc-957a-37147cca5634" /> ##### Move to locked folder in the Folder view Similarly, the folder view now includes the "Move to locked folder" action. <img width="1900" height="762" alt="image" src="https://github.com/user-attachments/assets/c39e792b-81da-4c31-a23f-03f96853fe8e" /> ##### Editor shortcuts Users on the web can now edit with keyboard shortcuts. Press `e` to open the editor. Once in the editor, press `[` or `]` to rotate the asset +/- 90 degrees. Finally, save any changes and close the editor with `ENTER`. <https://github.com/user-attachments/assets/969de104-b02d-41a6-830b-3e1a49541d14> ##### Create a new face on-the-fly in the face tag editor You can now create a new face/person on the fly from the face tagging editor interface <img width="350" alt="image" src="https://github.com/user-attachments/assets/c39db0e3-da47-4421-9040-5f51650deee9" /> <img width="350" alt="image" src="https://github.com/user-attachments/assets/0c81a1ee-c54e-4167-9dff-95719fe44595" /> ##### Deduplication improvements The duplicate screen has gone through a bunch of iterations since it was first introduced all the way back in May, 2024. The latest release moves a bunch of logic from the client to the server, which now automatically suggests which asset to keep based on image size and EXIF data. Additionally, the new server implementation will automatically synchronize metadata, including albums, favorite status, rating, description, visibility, location, and tags. For more information about this process, see the new [documentation](https://docs.immich.app/features/duplicates-utility). ##### Helmet configuration You can now opt in to using a [Content Security Policy (CSP)](https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CSP) in Immich. The new environment variant `IMMICH_HELMET_FILE` accepts a boolean or a path to a [helmet](https://helmetjs.github.io/) configuration file. **Recommend action:** The team recommends setting `IMMICH_HELMET_FILE=true` to enable the default policy. Then, please let us know if you run into any issues with it. ##### Background and details Since Immich is deployed in so many different ways, it has been hard to figure out how to enable a CSP that would not conflict with or break existing installs that might use 3rd party map providers, custom CSS, embed Immich in an iframe, or other such features. In this release, we have added the ability to both opt in to a default policy and configure a custom one. To use the default policy, simply set the environment variable `IMMICH_HELMET_FILE=true`. To use a custom policy, set the environment variable to a path on disk (within the immich-server) that contains a valid helmet configuration (e.g. `IMMICH_HELMET_FILE=/opt/immich/helmet.json`). CSP can be used to control what scripts are allowed to run on the page, which domains to load images from, etc. Additionally, it can be used to configure headers for [Referrer-Policy](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Referrer-Policy), [X-Powered-By](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/X-Powered-By), [X-Frame-Options](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/X-Frame-Options), and others. ##### New version check infrastructure Prior to this release, instances that used the automatic version check feature would send HTTP requests to `github.com`. Now, we have set up a small service at `version.immich.cloud` to handle these types of requests. This avoids any privacy implications of connecting to `github.com` , as well as moves the request load to our own infrastructure. ##### Notable fix: live photo and video download in Safari When downloading files in Safari with the same name, it will simply overwrite the file instead of automatically renaming it. In this release, the still and motion parts of a live photo are now named differently to prevent this from happening. ##### Notable fix: escape HTML in panorama photo viewer In `v2.6.0`, we added the ability to show/view clip text in the panorama viewer, but introduced an XSS vulnerability, which has been fixed in this release. Interestingly, this was XSS using text in the image, which would then get read by OCR. ##### Notable fix: Immich User Agent for external requests Similar to the mobile app, the server now sends a custom [User Agent](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/User-Agent) header. The format for the User Agent is `immich-server/{version}`. For example, `immich-server/2.7.0`. #### `v3.0.0` Just a heads up that this is the likely to be the last release before `v3.0.0`. Being a major release there will be a handful of breaking changes, *although it's worth noting that nothing is currently planned that requires user intervention*. It is mainly changes that impact 3rd party developers. More information and details should be available in the coming weeks. #### Support Immich <p align="center"> <img src="https://media.giphy.com/media/v1.Y2lkPTc5MGI3NjExbjY2eWc5Y2F0ZW56MmR4aWE0dDhzZXlidXRmYWZyajl1bWZidXZpcyZlcD12MV9pbnRlcm5hbF9naWZfYnlfaWQmY3Q9Zw/87CKDqErVfMqY/giphy.gif" width="450" title="SUPPORT THE PROJECT!"> </p> If you find the project helpful, you can support Immich by purchasing a product key at <https://buy.immich.app> or our merchandise at <https://immich.store> *** <!-- Release notes generated using configuration in .github/release.yml at v2.7.0 --> #### What's Changed ##### 🚀 Features - feat: add support for helmet configuration by [@​jrasm91](https://github.com/jrasm91) in [#​27058](immich-app/immich#27058) - feat: create new person in face editor by [@​alextran1502](https://github.com/alextran1502) in [#​27364](immich-app/immich#27364) ##### 🌟 Enhancements - feat(web): add a seperate tooltip for switching from dark to light mode by [@​Vogeluff](https://github.com/Vogeluff) in [#​27297](immich-app/immich#27297) - feat(web): focus on face-editor search input by [@​cratoo](https://github.com/cratoo) in [#​27136](immich-app/immich#27136) - feat(web): add RemoveFromAlbumAction to asset viewer nav bar by [@​timonrieger](https://github.com/timonrieger) in [#​27000](immich-app/immich#27000) - feat(web): add shortcuts to rotate images by [@​squishykid](https://github.com/squishykid) in [#​26927](immich-app/immich#26927) - feat(server): add checksum algorithm field by [@​etnoy](https://github.com/etnoy) in [#​26573](immich-app/immich#26573) - feat(server): resolve duplicates by [@​Phlogi](https://github.com/Phlogi) in [#​25316](immich-app/immich#25316) - chore(mobile): reduce spacing on video controls by [@​uhthomas](https://github.com/uhthomas) in [#​27313](immich-app/immich#27313) - perf(server): optimize people page query by [@​ffchung](https://github.com/ffchung) in [#​27346](immich-app/immich#27346) - feat(web): dim photo outside hovered face bounding box by [@​midzelis](https://github.com/midzelis) in [#​27402](immich-app/immich#27402) - feat(web): OCR overlay interactivity during zoom by [@​midzelis](https://github.com/midzelis) in [#​27039](immich-app/immich#27039) - feat: add move to lock folder in folder view by [@​alextran1502](https://github.com/alextran1502) in [#​27384](immich-app/immich#27384) - feat(web): highlight active person thumbnail in detail panel and edit faces panel by [@​midzelis](https://github.com/midzelis) in [#​27401](immich-app/immich#27401) - feat: move version checks to our own infrastructure by [@​zackpollard](https://github.com/zackpollard) in [#​27450](immich-app/immich#27450) - feat: add preview button when selecting images by [@​johnmaguire](https://github.com/johnmaguire) in [#​27305](immich-app/immich#27305) - fix: user-agent format by [@​jrasm91](https://github.com/jrasm91) in [#​27488](immich-app/immich#27488) - chore(mobile): reduce buffering timer duration by [@​uhthomas](https://github.com/uhthomas) in [#​27111](immich-app/immich#27111) - fix(mobile): use key on video controls by [@​uhthomas](https://github.com/uhthomas) in [#​27512](immich-app/immich#27512) - feat(server): Add support for .ts files by [@​ray](https://github.com/ray) in [#​27529](immich-app/immich#27529) ##### 🐛 Bug fixes - fix(server): refresh unedited asset dimensions on metadata extraction by [@​michelheusschen](https://github.com/michelheusschen) in [#​27220](immich-app/immich#27220) - fix(server): memory fragmentation by [@​mertalev](https://github.com/mertalev) in [#​27027](immich-app/immich#27027) - fix(database restores): don't assume onboarding has completed by [@​insertish](https://github.com/insertish) in [#​27052](immich-app/immich#27052) - fix(web): preserve timezone when changing timestamp (Closes [#​25354](immich-app/immich#25354)) by [@​updatemike](https://github.com/updatemike) in [#​27095](immich-app/immich#27095) - fix: various command palette usages by [@​danieldietzler](https://github.com/danieldietzler) in [#​27304](immich-app/immich#27304) - fix(web): keep map view open after closing asset viewer by [@​diiogofer](https://github.com/diiogofer) in [#​26980](immich-app/immich#26980) - fix(web): prevent Safari from overwriting live photo image with video by [@​saurav61091](https://github.com/saurav61091) in [#​26898](immich-app/immich#26898) - fix(mobile): video icon not showing on memories by [@​YarosMallorca](https://github.com/YarosMallorca) in [#​27311](immich-app/immich#27311) - fix(mobile): mismatch between system and app color when using low-chroma system color scheme by [@​putuprema](https://github.com/putuprema) in [#​27282](immich-app/immich#27282) - fix(mobile): images loads sometimes cancel too early by [@​LeLunZ](https://github.com/LeLunZ) in [#​27067](immich-app/immich#27067) - fix(mobile): streamline error handling for live photo saving by [@​LeLunZ](https://github.com/LeLunZ) in [#​27337](immich-app/immich#27337) - fix(web): keep upload totals stable when dismissing items ([#​27247](immich-app/immich#27247)) by [@​Nicolas-micuda-becker](https://github.com/Nicolas-micuda-becker) in [#​27354](immich-app/immich#27354) - fix(mobile): low upload timeout on android by [@​mertalev](https://github.com/mertalev) in [#​27399](immich-app/immich#27399) - fix(web): add drop shadow to asset viewer nav bar and prevent button shrinking by [@​midzelis](https://github.com/midzelis) in [#​27404](immich-app/immich#27404) - fix(mobile): favorite button not updating state by [@​YarosMallorca](https://github.com/YarosMallorca) in [#​27271](immich-app/immich#27271) - fix: detection of WebM container by [@​chanb22](https://github.com/chanb22) in [#​24182](immich-app/immich#24182) - fix(web): prevent AssetUpdate from adding unrelated timeline assets by [@​michelheusschen](https://github.com/michelheusschen) in [#​27369](immich-app/immich#27369) - fix: withFilePath select edited or unedited file by [@​bwees](https://github.com/bwees) in [#​27328](immich-app/immich#27328) - fix(web): Enable stack selector in shared album view by [@​timonrieger](https://github.com/timonrieger) in [#​24641](immich-app/immich#24641) - fix(server): use substring matching for person name search by [@​okxint](https://github.com/okxint) in [#​26903](immich-app/immich#26903) - fix: escape html by [@​jrasm91](https://github.com/jrasm91) in [#​27469](immich-app/immich#27469) - fix(mobile): ignore pointer events on toasts by [@​uhthomas](https://github.com/uhthomas) in [#​26990](immich-app/immich#26990) - fix(mobile): reset video controls hide timer when showing controls ch… by [@​uhthomas](https://github.com/uhthomas) in [#​26985](immich-app/immich#26985) - fix(mobile): don't update search filters in-place by [@​uhthomas](https://github.com/uhthomas) in [#​26965](immich-app/immich#26965) - fix(web): isFullScreen initial value check is incorrect by [@​midzelis](https://github.com/midzelis) in [#​27520](immich-app/immich#27520) - fix(mobile): transparent system navbar when trash bottom bar is visible by [@​putuprema](https://github.com/putuprema) in [#​27093](immich-app/immich#27093) - fix: timestamp handling for database backup in Web UI by [@​AfonsoMendoncaRibeiro](https://github.com/AfonsoMendoncaRibeiro) in [#​27359](immich-app/immich#27359) - fix: allow bots to access /s/ urls by [@​domints](https://github.com/domints) in [#​27579](immich-app/immich#27579) ##### 📚 Documentation - feat(docs): add keycloack example to oauth docs by [@​robson90](https://github.com/robson90) in [#​27425](immich-app/immich#27425) ##### 🌐 Translations - chore(web): update translations by [@​weblate](https://github.com/weblate) in [#​27029](immich-app/immich#27029) - chore(web): update translations by [@​weblate](https://github.com/weblate) in [#​27483](immich-app/immich#27483) #### New Contributors - [@​Vogeluff](https://github.com/Vogeluff) made their first contribution in [#​27297](immich-app/immich#27297) - [@​updatemike](https://github.com/updatemike) made their first contribution in [#​27095](immich-app/immich#27095) - [@​diiogofer](https://github.com/diiogofer) made their first contribution in [#​26980](immich-app/immich#26980) - [@​squishykid](https://github.com/squishykid) made their first contribution in [#​26927](immich-app/immich#26927) - [@​Phlogi](https://github.com/Phlogi) made their first contribution in [#​25316](immich-app/immich#25316) - [@​saurav61091](https://github.com/saurav61091) made their first contribution in [#​26898](immich-app/immich#26898) - [@​putuprema](https://github.com/putuprema) made their first contribution in [#​27282](immich-app/immich#27282) - [@​ffchung](https://github.com/ffchung) made their first contribution in [#​27346](immich-app/immich#27346) - [@​chanb22](https://github.com/chanb22) made their first contribution in [#​24182](immich-app/immich#24182) - [@​robson90](https://github.com/robson90) made their first contribution in [#​27425](immich-app/immich#27425) - [@​okxint](https://github.com/okxint) made their first contribution in [#​26903](immich-app/immich#26903) - [@​johnmaguire](https://github.com/johnmaguire) made their first contribution in [#​27305](immich-app/immich#27305) - [@​ray](https://github.com/ray) made their first contribution in [#​27529](immich-app/immich#27529) - [@​AfonsoMendoncaRibeiro](https://github.com/AfonsoMendoncaRibeiro) made their first contribution in [#​27359](immich-app/immich#27359) - [@​domints](https://github.com/domints) made their first contribution in [#​27579](immich-app/immich#27579) **Full Changelog**: <immich-app/immich@v2.6.3...v2.7.0> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDEuMSIsInVwZGF0ZWRJblZlciI6IjQzLjEwMS4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJyZW5vdmF0ZS9naXRodWItcmVsZWFzZSIsInR5cGUvbWlub3IiXX0=--> Reviewed-on: https://git.erwanleboucher.dev/eleboucher/homelab/pulls/101 Co-authored-by: bot-owl <bot@erwanleboucher.dev> Co-committed-by: bot-owl <bot@erwanleboucher.dev>
Deeds67
added a commit
to open-noodle/gallery
that referenced
this pull request
Apr 8, 2026
…r upstream Revert the file move from T01 (f19eb16). The original task moved e2e/src/api/specs/duplicate.e2e-spec.ts → e2e/src/specs/server/api/ so the vitest glob would pick it up. T01's commit message said "audit found zero violations" — but the audit was for waitForQueueFinish, NOT spec correctness. I never actually ran the spec to verify it works after the move. The full Phase 1 review surfaced that the spec is upstream-broken: - Added in upstream PR immich-app#25316 (2026-03-26) by @Phlogi as part of the "feat(server): resolve duplicates" feature. - Upstream PR immich-app#25856 (Feb 10, 2026, by @minidzelis) had restructured e2e/src/api/specs/ → e2e/src/specs/server/api/ six WEEKS earlier. - PR immich-app#25316 didn't notice and added the new spec in the OLD path. The file went into the wrong directory and was never picked up by the vitest glob in upstream's CI either. - A subsequent upstream PR changed the response shape of POST /duplicates/resolve from `{status, results: [{duplicateId, status}]}` to a bare `BulkIdResponseDto[]`. The never-running spec didn't get updated. 18 of 21 tests now fail against the current API. - The 3 tests that DO pass are access-matrix style assertions that are robust to body shape changes. This is upstream's bug. Maintaining a fork-local fix would generate merge conflicts every rebase. The right fix lives upstream — file an issue / PR there. Reverting the move puts the file back where upstream put it. Our vitest glob doesn't pick it up, so our CI is unaffected by upstream's broken tests. When upstream eventually fixes the spec (either fixes the assertions or re-locates the file or both), we inherit the fix cleanly via the next rebase. Backlog updates: - T01 row rewritten to reflect that only the audit landed (not the file move) and to point at the new T-cleanup-01 follow-up. - New "Upstream cleanup tasks" section before the Decision log, with T-cleanup-01: bring upstream duplicate.e2e-spec.ts up to date with the current /duplicates/resolve API and PR upstream. Estimated 2-4 hours of mechanical assertion rewriting. Verified locally: the 4 working specs (helpers + timeline + face + shared-space) still pass 205/205 across 3 consecutive runs after the revert. tsc --noEmit clean. No regressions to our coverage.
This was referenced Apr 8, 2026
Deeds67
added a commit
to open-noodle/gallery
that referenced
this pull request
Apr 8, 2026
* docs(plans): research e2e API test coverage gaps and backlog
Catalogues the 282-endpoint server surface against the 29 existing
e2e API specs, identifies the 8 zero-coverage controllers and the
endpoint holes inside heavily-tested specs (shared-space /people and
/libraries, asset metadata/edits/copy, library->space link side
effects). Defines a reusable Permission/Actor matrix and proposes
~14 follow-up PRs prioritised by bug-catching ROI.
* docs(plans): break e2e API coverage research into per-task backlog
Adds a working backlog of 39 PR-sized tasks (T01–T39) derived from the
e2e API coverage research doc, plus full design docs for the three
upfront tasks: helpers (T02), timeline access matrix (T03), and the
shared-space people listing (T09).
The backlog records architectural decisions (helper API shape, fixture
lifetime, extending utils.createSpacePerson over duplication) separately
from observed server invariants (400 vs 403 split between requireAccess
and requireMembership, the listing's thumbnailPath gate, the metadata vs
thumbnail role split on PATCH /shared-spaces). Each task row hard-pins
its dependencies so most of Phase 1+ can run in parallel after T02.
Four code-reviewer passes caught and fixed ~15 substantive issues across
the drafts; the docs as committed here are the result.
* test(e2e): T01 move stray duplicate spec into the discovered glob
The vitest config at e2e/vitest.config.ts uses
`src/specs/server/**/*.e2e-spec.ts` for spec discovery. The file at
e2e/src/api/specs/duplicate.e2e-spec.ts was outside that glob and has
never been running in CI — it was added during the duplicate-detection
work but landed in the wrong directory.
Move it to e2e/src/specs/server/api/duplicate.e2e-spec.ts (where every
other server API spec lives), remove the now-empty src/api/specs and
src/api directories, and tick T01 in the coverage backlog.
The companion audit of waitForQueueFinish vs expect.poll for non-admin
specs (per the e2e-coverage research doc and the
feedback_e2e_admin_only_queues memory) found zero violations: all 38
existing callers correctly pass admin.accessToken. Result documented
inline in the backlog row.
Imports in the moved file use the `src/*` path alias, so file depth is
irrelevant — no code changes were needed beyond `git mv`.
* test(e2e): T02 add Actor / SpaceContext / forEachActor helpers
Adds e2e/src/actors.ts with Actor + ActorId + SpaceContext types and
the buildSpaceContext / forEachActor helpers, and extends the existing
utils.createSpacePerson to insert the shared_space_person_face junction
row, accept a type parameter, and return {globalPersonId, spacePersonId,
faceId} instead of just the space person ID.
These exist to turn the Permission/Actor matrix from §3 of the e2e
coverage research doc into a one-liner per endpoint, so downstream
specs (T03+) can write `forEachActor(...)` instead of hand-rolling six
describe blocks per endpoint. See docs/plans/2026-04-06-e2e-T02-helpers-design.md
for the rationale and decision log.
Key design points (all captured in the design doc):
- buildSpaceContext composes utils.adminSetup / userSetup / createSpace /
addSpaceMember / addSpaceAssets / createAsset — no parallel
implementations of any existing helper.
- forEachActor throws Error (not expect.toBe) so the failure message
names the actor that failed; without that, debugging an actor matrix
is needlessly painful.
- Sequential, not parallel — tests share a database and parallel actor
runs would race on the same fixtures.
- Fixture lifetime contract: ctx is read-only in beforeAll; mutating
tests own their cleanup via try/finally or nested describes.
- ActorId starts minimal (8 actors). partner / libraryOwner / apiKey* /
sharedLink land with their first consumer task.
Smoke tests in e2e/src/specs/server/api/_helpers.e2e-spec.ts validate
all three behaviours that downstream PRs depend on:
1. Auth threading — bearer token reaches the server for every actor.
2. Anon split — /users/me requires auth (anon: 401, members: 200).
3. createSpacePerson extension — returns three IDs and inserts the
shared_space_person_face junction row (verified via direct DB query).
4. Role assignment — PATCH /shared-spaces/:id with {thumbnailCropY: 0}
distinguishes Owner/Editor from Viewer. Uses thumbnailCropY (Editor-
level) rather than name (Owner-level per shared-space.service.ts:197-203)
so Editor and Viewer get distinct status codes.
Implemented test-first: each smoke test was written and run failing
before the corresponding helper code was added.
Verified locally against the e2e stack: all 4 smoke tests green
(537ms total), server.e2e-spec.ts unchanged (24/24 still pass), tsc
--noEmit clean. createSpacePerson currently has zero callers in e2e/
(verified via grep) so the signature change is risk-free.
T01 ticked in the backlog along with T02. The cleanup audit found zero
non-admin waitForQueueFinish callers — the rule from the
feedback_e2e_admin_only_queues memory is currently held everywhere.
* test(e2e): T03 add timeline /buckets and /bucket access matrix
Adds e2e/src/specs/server/api/timeline.e2e-spec.ts with 9 tests
covering the access matrix for both timeline endpoints. This is the
first real consumer of the actor / forEachActor helpers from T02.
Tests on GET /timeline/buckets:
1. Auth required (anon: 401, owner: 200)
2. Owner sees own assets without filter (count == 2)
3. spaceId access matrix — status codes for owner/editor/viewer/non-member/anon.
Non-member returns **400** (not 403), pinned because timeline uses requireAccess
which throws BadRequestException (src/utils/access.ts:37-42), distinct from the
shared-space-family endpoints which use requireMembership and return 403.
4. spaceId scopes assets to the space, not the requesting user. spaceOwner with
spaceId sees the 1 space asset, NOT the 2 they own. Catches the bug shape where
the implementation would `WHERE asset.ownerId = auth.user.id` instead of joining
through shared_space_asset.
5. Non-owner space members (editor, viewer) actually see space content via spaceId.
This is the PR #163 / #202 bug shape — it returned 200 with empty body, so pure
status-code testing would have missed it.
Tests on GET /timeline/bucket (singular):
6. Auth required.
7. spaceId access matrix mirroring test 3 — same shape, distinct endpoint. The
risk being probed: forgetting to apply the same scoping check on the singular
endpoint. PR #260 is in this family.
8. Non-owner space members see the space asset via /bucket. Same bug class as
test 5, applied to the asset-list endpoint. Asserts the actual asset ID is in
the response, not just the status code.
9. /bucket returns the parallel-array TimeBucketAssetResponseDto shape, not
bucket counts. Sanity check that the two endpoints aren't confused.
Coverage matches the 10 tests enumerated in the T03 design doc; design tests 3
and 6 are merged into one access-matrix test (the matrix already pins
spaceNonMember: 400, so a separate test would assert the same thing).
Verified locally against the e2e stack: 9 tests green (580ms total). Combined
with the 4 helper smoke tests, total: 13 tests, 1.06s. tsc --noEmit clean.
T03 ticked in the backlog.
* test(e2e): apply post-T03 review nits
Three nits from the post-T01-T03 implementation review:
1. Add `authHeaders(actor)` helper to actors.ts and use it in
_helpers.e2e-spec.ts and timeline.e2e-spec.ts. The previous
`actor.token ? asBearerAuth(actor.token) : {}` pattern at every
forEachActor call site was unprecedented in the e2e suite —
localizing the conditional inside actors.ts keeps the call sites
consistent with idiomatic supertest usage.
2. Drop the redundant `utils.initSdk()` calls in the new spec files'
beforeAll. utils.ts:809 already invokes it at module load, and no
other spec under specs/server/api calls it explicitly.
3. Drop the try/finally + utils.disconnectDatabase() in
_helpers.e2e-spec.ts smoke test 3. Existing specs that use the
raw pg client (shared-space.e2e-spec.ts, etc.) don't disconnect —
they rely on worker-process exit. Disconnecting mid-spec would
break any later test in the file that uses utils.createSpacePerson
or any other client-using helper. Add an inline comment explaining
why.
`asBearerAuth(actor.token!)` is left in place at non-forEachActor
call sites where the actor is always authenticated — that's the
existing convention across the e2e suite.
Verified locally: 13 tests still green (4 helpers + 9 timeline,
1.03s), tsc --noEmit clean.
* test(e2e): T04 add timeline withSharedSpaces / withPartners semantics
Extends actors.ts and timeline.e2e-spec.ts to cover the two flags
that gate cross-user content on the timeline endpoints.
Helper changes (actors.ts):
- Add `partner` to ActorId.
- Add optional `partner` and `partnerAssetId` fields to SpaceContext.
- Add `BuildSpaceContextOptions` with `withPartner?: boolean`. When
set, buildSpaceContext creates an extra user, has them share their
library with spaceOwner, and uploads one asset for them.
- Add `addPartner({token, userId}, {token, userId})` helper. The
default `partner.inTimeline` column is **false** (verified at
server/src/schema/tables/partner.table.ts:46), so a fresh
`createPartner` call is invisible to `withPartners=true` until the
recipient enables it. The helper auto-enables inTimeline by having
the recipient call PUT /partners/:id, so test call sites don't have
to remember the two-step dance. This default-false behaviour is
exactly the kind of footgun an integration test would have caught
if anyone had ever written one before today.
New tests in timeline.e2e-spec.ts (`describe('GET /timeline/buckets — withSharedSpaces and withPartners')`):
1. withSharedSpaces=true makes a non-owner member see space content
on their own timeline. spaceEditor owns 1 asset (editorAssetId);
with withSharedSpaces, the union picks up spaceAssetId via the
membership default of showInTimeline=true, so total goes 1 -> 2.
2. Toggling showInTimeline=false drops the space out of
getSpaceIdsForTimeline. PATCH /shared-spaces/:id/members/me/timeline
with {showInTimeline: false}; verify total goes back to 1; restore
in try/finally per the fixture lifetime contract.
3. withPartners=true makes spaceOwner see partner-shared assets.
Total = 2 own + 1 partner = 3.
4. Default (no withPartners) excludes partner assets. Total = 2.
5. Combining withSharedSpaces and withPartners doesn't double-count —
spaceAssetId is already counted in spaceOwner's own 2, so the union
stays at 3.
All calls pass `visibility=timeline` explicitly because timeline.service.ts:91-113
treats `visibility === undefined` as `requestedArchived = true` and throws 400 when
either flag is set. This invariant is now pinned in the backlog.
Backlog updates:
- T04 row ticked (5 tests).
- New "Observed invariants" entries: partner.inTimeline default-false,
withSharedSpaces/withPartners visibility requirement.
Library-linked space asset visibility (mentioned in the original T04
design) is deferred to T17, which is the right home — it requires the
linkLibrary helper that doesn't exist yet.
Verified locally: 18 tests green (4 helpers + 14 timeline, 1.11s),
tsc --noEmit clean.
* test(e2e): T05 add timeline visibility filter tests
Adds 7 new tests to timeline.e2e-spec.ts: 5 for the visibility filter
behaviour itself, plus 2 invariant pins folded in from the T04 review
NIT (visibility-undefined-throws-400).
The visibility tests use a dedicated user (visibilityUser) with 4
assets in different states: timeline, archive, hidden, and trashed.
A separate user keeps the assertions deterministic — using spaceOwner
would have to subtract the existing space-related assets from every
expected count.
Tests in `describe('GET /timeline/buckets — visibility filters')`:
1. **default visibility (no param) returns Timeline AND Archive (2)** —
pins the non-obvious server behaviour at server/src/utils/database.ts:79-81.
`withDefaultVisibility` is `where('asset.visibility', 'in', [Archive, Timeline])`,
NOT just Timeline. The web UI's main timeline view must pass
`visibility=timeline` explicitly to exclude archived assets. This was a
real surprise during implementation — the first version of the test
expected count=1 and failed with count=2 against the live server.
2. visibility=timeline returns only the strict timeline assets (1).
3. visibility=archive returns only archived assets (1).
4. visibility=hidden returns only hidden assets (1). Hidden visibility is
normally used for the video part of live photos / motion photos
(per the AssetVisibility enum docstring); pinned here to protect that
path from a future refactor.
5. trashed assets are excluded regardless of visibility filter. Asserts
the count under both default and explicit visibility=timeline so a
trash regression would inflate either independently.
Tests appended to the withSharedSpaces/withPartners describe (folded
from T04 review NIT):
6. withSharedSpaces=true without explicit visibility returns 400.
7. withPartners=true without explicit visibility returns 400.
Backlog updates:
- T05 ticked.
- New "Observed invariant" entry: default visibility filter is
permissive (Timeline + Archive) — this is exactly the kind of fact
that bites every author who hasn't read database.ts.
Verified locally: 21 tests green (4 helpers + 17 timeline, 812ms),
tsc --noEmit clean.
* test(e2e): apply post-T05 review nits
Three nits from the post-T05 implementation review:
1. Visibility-400 invariant tests now also assert on the error message
(`/withSharedSpaces/` and `/withPartners/`). Without that, a future
unrelated 400 (e.g. a DTO validation change) would silently still
satisfy the test.
2. Renamed test 5 to "soft-deleted (trashed) assets are excluded
regardless of visibility filter". Both soft-delete and force-delete
set `deletedAt`, so the test characterises the deletedAt-based
exclusion which is what the timeline query actually depends on.
Comment now spells this out.
3. Refactored the `Promise.all` 4-element destructure into a parallel
block of 3 + a sequential trash creation. The previous
`const [, , , trashedAsset] = await Promise.all([...])` was uncommon
in the e2e suite. Pulling trashed out of the parallel block is
slightly slower (4 sequential RTTs vs 3 parallel + 1) but more
readable, and the cost is negligible at fixture-setup time.
Verified locally: 21 tests still green, tsc --noEmit clean.
* test(e2e): T06 add timeline filter passthrough tests with spaceId
Adds 5 tests to timeline.e2e-spec.ts in a new describe block, probing
how the various filter parameters (`spacePersonId`, `personId`, `tagIds`)
interact with `spaceId` scoping. This is the PR #260 bug shape pin —
the fork has a *dedicated* `spacePersonId` DTO field separate from
`personId`, and the original PR #260 bug was matching a global `personId`
against a `shared_space_person.id`.
Setup creates two new fixtures attached to ctx (without mutating any
existing state):
- `spacePerson` via utils.createSpacePerson — adds Alice as a face on
spaceAssetId, with the shared_space_person_face junction row that
the timeline filter joins through.
- `spaceTagId` — a tag owned by spaceOwner, applied to spaceAssetId.
Tests:
1. **spacePersonId + spaceId** restricts to assets containing that
space person. Joins through shared_space_person_face → asset_face
→ asset; only spaceAssetId qualifies, total=1.
2. **GLOBAL personId + spaceId** does not cross-pollute. Pinned at
total=1 (which here is *legitimate* because the global person row
is the same one createSpacePerson set up under the hood — not a
bug). The test exists so a future change that decouples the two
joins or breaks the spaceId restriction is caught.
3. **spacePersonId without spaceId** falls back to owner-scoped and
still filters correctly (total=1, just spaceAssetId).
4. **tagIds + spaceId** returns the tagged subset of space content.
5. **non-owner space member sees tagged space content via
spaceId+tagIds** — spaceEditor querying with spaceOwner's tag
returns the tagged space asset. Pins that the timeline tag filter
doesn't enforce per-user tag ownership for space-scoped queries,
which is the expected UX for shared spaces (one member labels a
photo, others see the label).
Coverage matches 5 of the ~8 tests in the original T06 design. EXIF
filter passthrough (country/make/rating) is deferred because it requires
fixture images with extracted metadata (the e2e suite uses generated
PNGs for most tests). When a follow-up wants EXIF coverage it can be
appended to this describe block.
Verified locally: 26 tests green (4 helpers + 22 timeline, 868ms),
tsc --noEmit clean.
T06 ticked. All four Phase 1 timeline subtasks (T03–T06) are now done.
* test(e2e): apply post-T06 review nits
The post-T06 review caught a real coverage gap: the original test 2
("global personId does NOT cross-pollute") asserted total=1, which
was a coincidence — the global person from createSpacePerson is
legitimately attached to spaceAssetId via asset_face, so the join
hits it whether or not the spaceId restriction is working. A
regression that removed the spaceId scoping entirely would still
return 1 and the test would pass.
To actually pin the boundary:
1. beforeAll now also creates a decoy global person ("Decoy Bob")
attached only to ownerAssetId (NOT in the space). No
shared_space_person row, no junction.
2. Test 2 is now the load-bearing boundary test: query with
personId=<decoy>&spaceId=<spaceId> and assert total=0. The
decoy's asset is NOT in the space; if the spaceId scoping is
working, the join must not return it. If a future regression
breaks the scoping, this test goes from 0 to 1.
3. Added a new test 3 for the other half of the boundary: querying
with personId=<spacePerson.globalPersonId>&spaceId=<spaceId> still
returns 1 because that global person IS attached to a space asset
via asset_face. Together, tests 2 and 3 pin both sides of the
spaceId boundary on the global personId join path.
Plus the IMPORTANT and NIT comment improvements:
- Test "spacePersonId without spaceId" comment now spells out that
the join is NOT spaceId-restricted — a future test that puts the
same spacePersonId on a second space's asset would observe
count > 1.
- Test "non-owner space member sees tagged content" comment now
spells out the actual invariant being pinned: hasTags has zero
per-user check, tag IDs are universally addressable on the
timeline filter. The shared-spaces UX consequence (one member
labels, all members can filter) is documented as the load-bearing
property — a future refactor that adds an owner check to hasTags
would silently break the UX unless this test catches it.
Verified locally: 27 tests green (4 helpers + 23 timeline, 914ms),
tsc --noEmit clean.
* test(e2e): T07 add face CRUD access matrix
Adds e2e/src/specs/server/api/face.e2e-spec.ts with 10 tests covering
the four /faces endpoints. Also extends utils.createFace to return the
inserted face id (was Promise<void>) so PUT/DELETE tests can address
specific faces — backwards-compatible because all existing callers
ignore the return value.
Tests:
POST /faces (3):
1. Access matrix on the asset side. Owner can; spaceNonMember 400;
anon 401. Same Immich-wide bulk-access pattern as timeline (400
not 403 for non-owner).
2. Cross-owner asset rejected even when the person belongs to caller.
3. Cross-owner person rejected even when the asset belongs to caller.
GET /faces (2):
4. Access matrix on the asset side.
5. Owner gets the face row back with the linked person populated.
Asserts presence of the specific face we inserted (not a count) to
stay robust against unrelated faces accumulating on the asset.
PUT /faces/:personId — reassign (2):
6. Access matrix when reassigning a face to a new person owned by
the same user (Alice → Anne).
7. Reassigning to a cross-owner target person is rejected — the
person-access check on the target fires before any state mutation.
DELETE /faces/:faceId (3):
8. Owner can soft-delete (force=false).
9. Owner can force-delete (force=true).
10. Access matrix for non-owner / anon. Each test creates its own
scratch face so the access matrix doesn't permanently mutate
state.
The non-obvious API shapes are documented in the spec file header
and pinned in the backlog "Observed invariants" section:
- POST /faces returns void (no face id)
- PUT /faces/:id has path=target-person, body=face — the FaceDto is
reused with different meanings on different endpoints
- GET /faces takes ?id=<assetId> (the FaceDto field is named `id`
but represents the asset)
The first attempt at the spec hit four cascading failures from my
incorrect API understanding (POST returning void, PUT semantics
backwards, count assertion fragile to other tests). The corrected
version pins the actual server behaviour and is robust to test
isolation.
Verified locally: 41 tests green across 3 spec files
(4 helpers + 27 timeline + 10 faces, 1.82s test runtime),
tsc --noEmit clean.
T07 ticked.
* test(e2e): T08 add face deletion side effect tests + post-T07 nit fix
Adds 6 tests to face.e2e-spec.ts in a new "face deletion side effects (T08)"
describe block, plus a one-line comment fix from the post-T07 review.
T08 tests:
1. Soft-deleted face is excluded from GET /faces?id=<assetId> via the
`asset_face.deletedAt IS NULL` filter (person.repository.ts:229).
2. Hard-deleted face is excluded from GET (deleted row, same observable
result).
3. Soft-deleting the only face on a person preserves the person row.
Global persons are NOT cascade-deleted when their last face goes away;
GET /people/:id still returns the person. This matters for the
shared-spaces UX where the person row outlives any individual face
attachment.
4. Soft-deleting a face decreases the person's getStatistics asset count.
getStatistics joins through asset_face filtering on `deletedAt IS NULL`
AND `isVisible IS true` (person.repository.ts:335-352), so soft-delete
drops the row out of the count.
5. Hard-delete decreases the count via the same mechanism (row removal).
6. Re-attaching the same (assetId, personId) after a soft-delete inserts
a NEW asset_face row. There's no UNIQUE constraint blocking this;
pinned so a future schema change is caught.
Two scope adjustments documented in the backlog row:
- "Below-minFaces faces unaddressable" was a *space-person* concern from
PR #139 (space person thumbnail 404s), not a global face concern.
Moved to T10/T11.
- "Space-person dedup queue jobId dedup" from PR #292 requires probing
queue state that isn't exposed via the face controller. Moved to T14
which owns the deduplicate endpoint.
Post-T07 review NIT fix:
- POST /faces access matrix comment said "write access to BOTH the asset
and the person", but person.service.ts:641-642 actually uses
`Permission.AssetRead` + `Permission.PersonRead` (not write). Comment
reworded.
Verified locally: 47 tests green across 3 spec files (4 helpers + 27
timeline + 16 faces, 1.88s test runtime), tsc --noEmit clean.
T08 ticked. Faces group complete (T07 + T08).
* test(e2e): apply post-T08 review nit — strengthen the re-attach test
Test 6 ("re-attaching a face after a soft-delete") originally asserted
both faces on the same asset, which made the stats `count(distinct
asset.id)` = 1 with or without the deletedAt filter — the assertion
passed by accident, not because of the filter.
Strengthen by:
1. Putting the soft-deleted face on assetA and the new face on assetB.
The stats count now actually distinguishes the two semantics:
- With deletedAt filter (correct): 1 (only assetB)
- Without it (broken): 2 (assetA + assetB)
2. Adding a third face on assetA (re-attach with the existing
soft-deleted row in place). Asserts no UNIQUE constraint blocks the
second insert AND that count goes 1 → 2 to confirm the new face is
counted.
The "no UNIQUE constraint" pin is preserved; the deletedAt filter is
now genuinely exercised by the count assertion.
Verified locally: 16 tests still green, tsc --noEmit clean.
* test(e2e): T09 add shared-space people listing tests
Adds an 11-test describe block to shared-space.e2e-spec.ts covering
the GET /shared-spaces/:id/people endpoint per the T09 design doc
(docs/plans/2026-04-06-e2e-T09-space-people-listing-design.md).
Setup: dedicated owner/editor/viewer/non-member users with their
own space and 5 space-people via the extended utils.createSpacePerson
helper from T02 (which inserts the four-table chain including the
shared_space_person_face junction).
Tests:
1. Access matrix — owner/editor/viewer 200, non-member 403, anon 401.
shared-space endpoints use requireMembership → ForbiddenException,
distinct from timeline's requireAccess → BadRequestException.
2. Listing returns space person IDs (NOT global person IDs). The
canonical assertion for the whole T09–T14 sub-tree.
3. Hidden persons excluded by default.
4. ?withHidden=true includes hidden persons.
5. Unnamed persons included by default.
6. ?named=true returns only persons with non-empty names (on either
shared_space_person.name OR person.name per the OR clause in
shared-space.repository.ts:514-521).
7. Toggling shared_space.petsEnabled=false hides pets, restored in
try/finally per the fixture lifetime contract.
Pagination tests in a nested describe (own beforeAll/afterAll for the
15 extra rows so they don't leak into sibling tests):
8. ?limit=10 caps the response.
9. ?offset paginates without overlap.
10. Sort order is stable across calls.
Final test in the parent describe:
11. Empty thumbnailPath on the underlying global person excludes the
space person from the listing. Pins the fork's "minFaces gate"
mechanism (shared-space.repository.ts:512-513) — pinned now so a
future query refactor that drops the filter would be caught.
Mutates via direct DB and restores in try/finally.
All assertions match the design doc decisions in the backlog
"Decision log" section: space person ID is canonical, stable sort,
extended createSpacePerson is the helper, listing query params are
limit/offset/withHidden/named/takenAfter/takenBefore (no `top`, no
text-based name search).
Backlog updates:
- T09 ticked.
- New "Known flaky-spec footgun" section: utils.createPerson +
Promise.all is unsafe with the shared pg.Client — observed once
as an FK violation in T09 setup, didn't reproduce in 3 follow-up
runs. Latent since T07. Not blocking; documented for follow-up.
Verified locally: 110 tests green in shared-space.e2e-spec.ts
(99 existing + 11 new T09), 157 tests across 4 spec files including
helpers/timeline/face/shared-space (4 + 27 + 16 + 110), 6.78s test
runtime, tsc --noEmit clean.
* test(e2e): apply post-T09 review nits
The post-T09 review caught one IMPORTANT and two NIT issues — all
fixed in this commit.
1. **IMPORTANT — missing afterAll in pagination describe.** The T09
commit message claimed "own beforeAll/afterAll" but the
implementation only had beforeAll. The 15 extra space-people rows
would leak into test 11 (the thumbnailPath gate) and any future
sibling test added to the parent describe. T11 still passes (it
asserts NOT contain), but the leak violates the T02 fixture
lifetime contract and would bite T10+. Added afterAll with
`DELETE FROM shared_space_person WHERE id = ANY($1::uuid[])` over
the captured ids.
2. **NIT — test 11 used a JOIN query for globalPersonId.** The
extended createSpacePerson helper from T02 already returns
`{globalPersonId, spacePersonId, faceId}` — no need to re-query
the database. Stored zeroThumbGlobalId in the describe scope and
dropped the 4-line JOIN.
3. **NIT — test 1 used a manual for-loop instead of forEachActor.**
The design doc explicitly called for forEachActor. The manual loop
worked but diverged from the T03+ pattern. Switched to forEachActor
with proper Actor objects, importing from src/actors. This also
sets the precedent for T10-T14 to use the helper consistently.
Verified locally: 110 tests still green in shared-space.e2e-spec.ts,
tsc --noEmit clean.
* test(e2e): T10 single space-person + thumbnail + assets
Adds 9 tests in a new nested describe inside T09's parent block,
sharing the T09 fixture setup. Covers the three read-only sub-endpoints:
- GET /shared-spaces/:id/people/:personId
- GET /shared-spaces/:id/people/:personId/thumbnail
- GET /shared-spaces/:id/people/:personId/assets
GET /people/:personId (5 tests):
1. Access matrix (owner/editor/viewer 200, non-member 403, anon 401).
2. Returns the canonical space person ID and name.
3. Hidden person IS fetchable directly — confirms half of the T09
open hypothesis: hidden filter is listing-only.
4. **Pet person is NOT fetchable directly when petsEnabled=false** —
DISPROVES the other half of the T09 hypothesis. The pet filter
applies BOTH to the listing and to the single-fetch endpoint.
This asymmetry vs hidden is real and worth pinning. UX
consequence: turning pets off in a space hides the entire pet
sub-graph, even from members who know the pet's ID.
5. **Non-existent personId returns 400 (not 404)** — bulk-access
pattern via requireAccess uniformly returns BadRequestException
for "not found OR no access" to avoid leaking existence. Same
taxonomic split as timeline. T11+ inherit this convention.
GET /people/:personId/thumbnail (2 tests):
6. Access matrix. **Member success-case is 500**, not 200, because
the fixture thumbnailPath ('/my/awesome/thumbnail.jpg' set by
utils.createSpacePerson) doesn't exist on disk. The access check
passes; the file resolution then fails and returns 500. Pinned
as a known footgun in the backlog — fixable later but out of
scope for T10. The 500 vs 404 distinction is a small server-side
bug independent of the access path.
7. Non-member 403 fires before file resolution (sanity check that
the 500 path isn't somehow leaking access).
GET /people/:personId/assets (2 tests):
8. Access matrix.
9. Returns the asset IDs containing the person — Alice is on
spaceAssetId so the response contains it.
Backlog updates:
- T10 ticked.
- The "open hypothesis" about hidden/pets at listing only is moved
to a new "Resolved hypotheses" section with the asymmetric finding
documented.
- New "Observed invariant": pet filter asymmetry (listing AND single
fetch) vs hidden filter (listing only).
- New "Observed invariant": single-person endpoints return 400 for
unknown IDs (bulk-access pattern), not 404.
- New "Known footguns" section: the thumbnail-500 issue.
Verified locally: 119 tests green in shared-space.e2e-spec.ts
(110 from T09 + 9 new T10), tsc --noEmit clean.
* test(e2e): apply post-T10 review fixes — service mechanism + thumbnail strategy
Two IMPORTANT findings from the post-T10 review, both about explanation
correctness rather than test logic.
1. **Backlog "bulk-access pattern via requireAccess" claim was wrong.**
`getSpacePerson` at shared-space.service.ts:625-636 calls
`requireMembership` (ForbiddenException for non-member, that part was
right), then runs `getPersonById` and manually
`throw new BadRequestException('Person not found')` for both the
missing-person case AND the pet-when-disabled case. The 400s the
T10 tests observe are real and load-bearing, but they come from
manual throws inside the service handler, NOT from the
`requireAccess` bulk pattern that timeline uses. Backlog
"Observed invariants" rewritten to cite the correct mechanism with
line references.
2. **Thumbnail 500 was a fixture wart, not a server bug.**
`getSpacePersonThumbnail` at shared-space.service.ts:643-657 has
THREE return paths once the access check passes:
- person not found / wrong space → NotFoundException → 404
- thumbnailPath null/empty → NotFoundException → 404
- thumbnailPath set → serveFromBackend → 200 (or 500 if missing)
The 500 in the previous T10 commit was triggered because
utils.createSpacePerson uses '/my/awesome/thumbnail.jpg' (a
non-empty path that doesn't exist on disk), which trips
serveFromBackend. The service actually has graceful 404 handling.
Restructured the thumbnail tests to exercise the **graceful 404
path**: transiently blank `person.thumbnailPath` via DB, assert
`{member: 404, non-member: 403, anon: 401}`. This pins the layered
ordering 401 < 403 < 404 — the correct member-success path for a
person with no thumbnail. Restore in try/finally.
The 200 path is not exercised because it would require pointing
the fixture at a real file in the upload location. That's a
reasonable follow-up but out of scope here.
Merged the two thumbnail tests into one (the matrix already
covers the access ordering, the second test was redundant). T10
is now 8 tests in the file (was 9), all assertions correct.
Backlog updates:
- Two "Observed invariants" rewritten to cite the manual
BadRequestException mechanism with the correct line numbers.
- New "Observed invariant" describing the three return paths of
getSpacePersonThumbnail.
- "Known footguns" entry rewritten: it's a fixture issue, not a
server bug. Mentions the follow-up to make createSpacePerson
accept a real fixture file.
Verified locally: 118 tests green in shared-space.e2e-spec.ts
(110 from T09 + 8 from T10), tsc --noEmit clean.
* test(e2e): T11 PUT/DELETE space person — rename, hide, delete
Adds 7 tests in a new nested describe inside the T09/T10 block,
covering mutation of a single space person via PUT and DELETE.
Both endpoints route through `requireRole(SharedSpaceRole.Editor)`
(verified at shared-space.service.ts:665, 704), so:
- Owner + Editor can mutate (200/204)
- Viewer is rejected (403)
- non-member is rejected (403, via requireMembership inside requireRole)
- anon (401, auth middleware)
PUT tests (4):
1. Access matrix for rename — owner+editor 200, viewer/non-member 403,
anon 401.
2. Actually renames the person — sends `{name: 'AfterRename'}` and
asserts the response body reflects the new name.
3. Marking isHidden=true hides the person from the default listing
but the direct fetch still returns 200 — pairs with T10's listing-only
hidden invariant. The PUT path is the supported way to set isHidden,
complementing T09's direct-DB-mutation pattern.
4. Non-existent personId returns 400 (manual BadRequestException at
shared-space.service.ts:668-669, same shape as T10).
DELETE tests (3):
5. Access matrix — owner+editor 204, viewer/non-member 403, anon 401.
Uses 5 different scratch persons (one per actor) to avoid
"delete-then-delete" race conditions in the matrix.
6. Preserves the underlying global person row — verifies via direct
DB query that `person` table row stays after `shared_space_person`
delete. The shared_space_person delete is correctly scoped and
does NOT cascade to the global person table.
7. Non-existent personId returns 400.
All scratch persons are created fresh per `it()` block via
utils.createSpacePerson, so the mutations are fully isolated and
don't affect T09's listing assertions or T10's read-only fixtures.
Verified locally: 125 tests green in shared-space.e2e-spec.ts
(110 from T09 + 8 from T10 + 7 from T11, 5.15s test runtime),
tsc --noEmit clean.
T11 ticked. Space-people sub-tree progress: 3/6 (T09/T10/T11 done,
T12 merge / T13 alias / T14 deduplicate remaining).
* test(e2e): T12 POST merge space persons
Adds 6 tests in a new nested describe inside the T09/T10/T11 block,
covering POST /shared-spaces/:id/people/:personId/merge.
Service shape (shared-space.service.ts:730-778): path :personId is the
*target*, body `{ids: string[]}` lists the *sources*. Requires Editor.
Validates both sides in the same space and the same type, reassigns
the source's junction rows to the target, deletes the source rows,
recounts the target's denormalised faceCount/assetCount, and queues
a dedup pass.
Tests:
1. Access matrix — owner+editor 204, viewer 403, non-member 403, anon 401.
Uses 5 separate scratch source persons (one per actor) so the
matrix doesn't try to merge the same source twice.
2. Merge reassigns the source's junction rows and deletes the source.
Verified via direct DB queries: source row gone, target now has 2
junction rows (its own face + the inherited one).
3. After merge, target's denormalised faceCount=2 and assetCount=1
(both faces are on the same asset, so distinct asset count is 1).
Pins recountPersons (shared-space.repository.ts:686+).
4. Cannot merge a person into themselves — 400 with message
matching /themselves/ from shared-space.service.ts:743-745.
5. Cannot merge across types (person target + pet source) — 400
with message matching /different types/ from line 754-756.
Pins the type-segregation invariant: pets and persons stay
separate even within the same space.
6. Missing target OR missing source returns 400. Two requests in
one test, both pinned.
Each test creates fresh scratch persons via utils.createSpacePerson
inside the `it()` block — fully isolated from T09/T10/T11 fixtures
and from sibling T12 tests.
Verified locally: 131 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12, 6.20s test runtime),
tsc --noEmit clean.
T12 ticked. Space-people sub-tree: 4/6 done (T09/T10/T11/T12). T13
alias and T14 deduplicate remaining.
* test(e2e): T13 space person alias — per-user, viewer-allowed
Adds 5 tests in a new nested describe inside the T09/T10/T11/T12 block,
covering PUT and DELETE alias.
Three critical invariants pinned by this block — the original T13
backlog row was wrong about both:
1. **Aliases are PER-USER, not visible to all members.** The service
stores `(personId, userId, alias)` and `getAlias(personId, auth.user.id)`
returns only the caller's row (shared-space.service.ts:780-798).
Owner setting "Mom" as Alice's alias is invisible to editor and
viewer. The original backlog row claimed "visible to all members"
which is the opposite of reality.
2. **Aliases require `requireMembership`, NOT `requireRole(Editor)`.**
Viewers CAN set their own aliases. Logical: aliases are personal
metadata, not space state — a read-only viewer should still be able
to label people for themselves.
3. **DELETE has no person existence check; it's idempotent on missing
personId.** Asymmetric vs PUT (which validates and returns 400).
Service code at lines 800-803.
Tests:
1. Access matrix — owner+editor+viewer 204 (all members can set their
own alias), non-member 403, anon 401.
2. Per-user persistence + isolation — owner sets "Mom" alias, owner GET
sees "Mom", editor GET sees null alias and the original 'PerUserAlice'
name. Pins both halves of the per-user invariant.
3. Alias does NOT modify global `person.name` — direct DB query
confirms the underlying `person.name` row stays at the original
value after alias is set.
4. DELETE removes the alias AND is idempotent on missing personId
(single test asserts both) — covers the asymmetry vs PUT.
5. PUT alias on missing personId returns 400 — pinned for symmetry
with T11/T12's missing-personId convention.
Backlog updates:
- T13 ticked with the corrected row description.
- New "Observed invariant" entry documenting the per-user + viewer-
allowed + idempotent-DELETE quirks.
Verified locally: 136 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13, 6.32s test
runtime), tsc --noEmit clean.
T13 ticked. Space-people sub-tree: 5/6 done. T14 (deduplicate) is
the last one before T15-T17 finish the space-libraries sub-tree.
* test(e2e): apply post-T13 review nit — tighten alias null assertions
Tests 2 and 4 used a defensive `alias === null || alias === undefined`
check, but `mapSpacePerson` always sets `alias: string | null` and never
omits the field. Tightened both assertions to `toBeNull()` so a future
regression that drops the field would be caught (the disjunction would
silently still pass on a missing field).
Other NITs from the post-T13 review (split the two-in-one DELETE test,
trim the verbose backlog row) were judgment calls — leaving as-is.
Verified locally: 136 tests still green, tsc --noEmit clean.
* test(e2e): T14 deduplicate space people — Owner-only + jobId dedup pin
Adds 4 tests in a new nested describe inside the T09/T10/T11/T12/T13
block, covering POST /shared-spaces/:id/people/deduplicate.
Service shape (shared-space.service.ts:721-728): the manual dedup
trigger requires `Owner` role (NOT Editor — distinct from PUT/DELETE/
merge), then queues a SharedSpacePersonDedup job on the
FacialRecognition queue with jobId `space-dedup-${spaceId}`
(job.repository.ts:239-241).
Tests:
1. **Owner-only access matrix** — owner 204, editor 403 (this is the
distinguishing test from T11/T12 which all only required Editor),
viewer 403, non-member 403, anon 401. Pins the role distinction.
2. Owner happy path returns 204 — sanity check on the success shape.
3. Two consecutive owner calls both return 204 — pins HTTP-level
idempotency, independent of whether BullMQ deduplicates underneath.
4. **PR #292 jobId dedup verification.** The load-bearing test for the
whole T14 task. Strategy: pause the FacialRecognition queue, empty
it, trigger dedup twice, count jobs whose data matches the test
space, restore in try/finally. The two triggers should produce
exactly ONE queued job — BullMQ's queue() with a duplicate jobId
is a no-op, so PR #292's behaviour is preserved.
Queue manipulation requires admin token (queue.controller.ts:23 —
admin: true). The `admin` token is already set up in the outer
beforeAll. The pause/restore is bracketed in try/finally so a test
failure doesn't leave the queue in a paused state and break the
rest of the suite.
Backlog updates:
- T14 ticked.
- New "Observed invariant" pinning the Owner-only role and the
jobId-based queue dedup, with the verification mechanism documented.
Verified locally: 140 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14, 5.29s
test runtime), tsc --noEmit clean.
T14 ticked. **Space-people sub-tree COMPLETE** (T09-T14, all 6 tasks).
Phase 1 progress: T01-T14 done, only T15-T17 (space-libraries
sub-tree) remaining before Phase 1 closes.
* test(e2e): apply post-T14 review fixes — DTO body + tightened assertions
Four post-T14 review findings, all addressed in this commit. The test
behaviour is unchanged (still 140/140 green) but the test logic now
matches the actual server contract instead of relying on dead body
fields and over-permissive predicates.
1. **BLOCKING — DTO body shape was wrong.**
The test's `DELETE /queues/:name/jobs` calls sent
`{statuses: [...]}`, but `QueueDeleteDto` only has `failed?: boolean`
(queue.dto.ts:24-31). The `statuses` field was silently dropped by
NestJS's whitelist validator and the underlying
`jobRepository.empty(name)` drained the queue unconditionally
anyway, so the test passed — but the body was misleading: it
implied a statuses-based filter that doesn't exist. Removed both
sends in try and finally; the empty call is now bodyless and the
intent is clear.
2. **Comment misleading.** The GET /queues/:name/jobs call applied no
filter, so it returned ALL jobs in any state, not just waiting/paused.
Updated the comment to spell that out and removed the misleading
"waiting/paused" language.
3. **NIT — try/finally hole.** The test now asserts `pauseRes.status
=== 200` immediately after the PUT pause, so a pause failure fails
loudly instead of silently letting the worker race the assertion.
4. **NIT — over-permissive filter.** The original predicate was
`j.data?.spaceId === spaceId || j.id === space-dedup-${spaceId}` —
the `||` would only widen if the jobId encoded something OTHER than
spaceId, which is exactly the kind of refactor we want to catch.
Tightened to a strict `j.data?.spaceId === spaceId` check on the
job's data payload, which is the load-bearing field we care about.
Verified locally: 140 tests still green in shared-space.e2e-spec.ts,
tsc --noEmit clean.
* test(e2e): T15 PUT space libraries — link library to space
Adds 7 tests in a new nested describe inside the T09-T14 block,
covering PUT /shared-spaces/:id/libraries.
Service shape (shared-space.service.ts:449-477) has a TWO-step gate:
1. `if (!auth.user.isAdmin)` → ForbiddenException 'Only admins can
link libraries to spaces' — admin gate, fires FIRST.
2. `requireRole(Editor)` — must be a space member with Editor or
Owner role.
Then library existence is checked (400 if missing). On success,
addLibrary returns null for duplicate (spaceId, libraryId) and skips
the face-sync queue — making the operation idempotent at HTTP level.
Tests:
1. **Non-admin owner of the space cannot link** — pins the admin
gate firing BEFORE role check. Even the space owner gets 403 if
they're not also a global admin. Asserts the message matches /admins/.
2. Non-admin editor cannot link — same admin gate.
3. Non-admin viewer cannot link — same.
4. Anon → 401.
5. **Admin who is an Editor in the space CAN link** — happy path. The
block's beforeAll adds the global `admin` user as an Editor in the
T09 test space.
6. **Idempotent on duplicate link** — calling link twice with the
same library returns 204 both times, and the shared_space_library
table has exactly 1 row for the (spaceId, libraryId) pair. Pins
the "204, not 409" behaviour explicitly.
7. **Library not found returns 400** — pins the existence check
message.
Setup creates two libraries via utils.createLibrary (admin token,
admin.userId as owner). The second library is used for the success
test so the idempotency test has a clean slate on the first library.
Backlog updates:
- T15 ticked with the corrected description (was "409 on duplicate" —
the actual behaviour is 204 idempotent).
- New "Observed invariant" pinning the two-step gate ordering and the
idempotent duplicate behaviour, with line references.
Verified locally: 147 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15,
5.35s test runtime), tsc --noEmit clean.
T15 ticked. Space-libraries sub-tree: 1/3 done. T16 (unlink) and T17
(link side effects) remaining before Phase 1 closes.
* test(e2e): T16 DELETE space libraries — unlink, idempotent
Adds 5 tests in a new nested describe inside the T09-T15 block,
covering DELETE /shared-spaces/:id/libraries/:libraryId.
Service shape (shared-space.service.ts:479-487) — same two-step gate
as T15:
1. `if (!auth.user.isAdmin)` → ForbiddenException 'Only admins...'
2. `requireRole(Editor)`
Then repository.removeLibrary(spaceId, libraryId) is a plain DELETE
on the (spaceId, libraryId) pair (shared-space.repository.ts:220-226)
with NO row-existence check. So unlink is idempotent at the HTTP
level: deleting an already-unlinked link, or deleting with a bogus
libraryId, both return 204.
Tests:
1. Non-admin owner cannot unlink (admin gate fires first, message
matches /admins/i).
2. Anon → 401.
3. **Admin Editor CAN unlink + DB row count goes 1 → 0.** Pre-checks
the row exists, calls unlink, then verifies the row is gone via
direct DB query. Pins the actual mutation.
4. **Unlinking an already-unlinked library is idempotent.** Reuses
scratchLibrary which test 3 just unlinked, calls again, expects
204.
5. **Unlinking with a non-existent libraryId returns 204.** Pins the
"no existence check" behaviour — even a bogus UUID is a no-op
DELETE → 204, NOT 404 (the original design row was wrong about
this).
Setup creates `scratchLibrary` per beforeAll and pre-links it via
the API so test 3 has a real link row to remove.
Backlog updates:
- T16 ticked with the corrected description (was "non-existent link
→ 404" — actual is 204 idempotent, no existence check).
Verified locally: 152 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15
+ 5 T16, 5.31s test runtime), tsc --noEmit clean.
T16 ticked. Space-libraries sub-tree: 2/3 done. T17 (link side
effects) is the last Phase 1 task before the entire phase closes.
* test(e2e): T17 library link side effects — closes Phase 1
Adds 6 tests in a new nested describe inside the T09-T16 block, the
final task of the space-libraries sub-tree and the last task of the
entire Phase 1 of the e2e coverage backlog.
T17 exercises the cross-table query path that link/unlink enable:
library assets becoming visible to space members via /timeline/bucket
?spaceId=. PR #163 was specifically about this code path. Setup
uploads an admin asset normally and UPDATEs its libraryId via direct
DB to associate it with a fresh library — bypassing the fragile
library scan path while exercising the same JOIN.
Tests:
1. **After link, a non-owner space member sees the library asset via
/timeline/bucket?spaceId=** — the load-bearing PR #163 invariant.
Editor (who owns no assets) calls the bucket query for the space
and the library asset appears via the shared_space_library JOIN.
2. **Viewer sees it too** — symmetric assertion for the read-only role.
3. **After unlink, library assets are no longer visible** — round-trip
pin: link → see → unlink → don't see.
4. **Soft-deleted library asset is hidden** via the `deletedAt IS NULL`
filter on the timeline query. Mutate `asset.deletedAt` to NOW(),
verify hidden, restore.
5. **Offline library asset IS still visible (NOT hidden)** — SURPRISING
FINDING. asset.repository.ts:835-849 joins shared_space_library on
(libraryId, spaceId) but does NOT filter on asset.isOffline. So a
library asset whose underlying file went offline is still listed in
the space's timeline bucket. The asymmetry vs `deletedAt` filtering
is real. Test pins the actual behavior — if a future change adds
the missing isOffline filter, the test fails and forces a deliberate
update. The access.repository's checkSpaceAccess in a different
code path DOES filter on isOffline=false, so the timeline path is
the inconsistent one.
6. **Library delete eventually cascades to shared_space_library** —
library.service.ts:370-379 is a SOFT delete: it sets `deletedAt`,
queues a LibraryDelete job, returns 204 immediately. The cascade
happens async when the job processes. Test calls
waitForQueueFinish('library') after the DELETE before asserting
the FK row is gone.
Setup uses a DB-direct approach (createAsset + UPDATE libraryId)
instead of the library scan helper because the scan path was hitting
unexplained timing/timeout issues — bypassing the scan keeps the
test focused on the JOIN behavior, which is what T17 actually probes.
Backlog updates:
- T17 ticked.
- New "Observed invariant": timeline spaceId query lacks the
isOffline filter on library assets — pinned with file:line and
the asymmetry called out.
- New "Observed invariant": library delete is soft + async cascade —
tests must wait for the LibraryDelete job to drain before asserting
the FK cascade.
Verified locally: 158 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15
+ 5 T16 + 6 T17, 5.50s test runtime), tsc --noEmit clean.
T17 ticked. **Phase 1 COMPLETE** — all 17 tasks (T01-T17) done.
* test(e2e): revert T01 duplicate spec move — upstream-broken, leave for upstream
Revert the file move from T01 (f19eb16bc). The original task moved
e2e/src/api/specs/duplicate.e2e-spec.ts → e2e/src/specs/server/api/
so the vitest glob would pick it up. T01's commit message said
"audit found zero violations" — but the audit was for waitForQueueFinish,
NOT spec correctness. I never actually ran the spec to verify it works
after the move.
The full Phase 1 review surfaced that the spec is upstream-broken:
- Added in upstream PR #25316 (2026-03-26) by @Phlogi as part of the
"feat(server): resolve duplicates" feature.
- Upstream PR #25856 (Feb 10, 2026, by @minidzelis) had restructured
e2e/src/api/specs/ → e2e/src/specs/server/api/ six WEEKS earlier.
- PR #25316 didn't notice and added the new spec in the OLD path.
The file went into the wrong directory and was never picked up by
the vitest glob in upstream's CI either.
- A subsequent upstream PR changed the response shape of
POST /duplicates/resolve from `{status, results: [{duplicateId, status}]}`
to a bare `BulkIdResponseDto[]`. The never-running spec didn't get
updated. 18 of 21 tests now fail against the current API.
- The 3 tests that DO pass are access-matrix style assertions that
are robust to body shape changes.
This is upstream's bug. Maintaining a fork-local fix would generate
merge conflicts every rebase. The right fix lives upstream — file an
issue / PR there.
Reverting the move puts the file back where upstream put it. Our
vitest glob doesn't pick it up, so our CI is unaffected by upstream's
broken tests. When upstream eventually fixes the spec (either fixes
the assertions or re-locates the file or both), we inherit the fix
cleanly via the next rebase.
Backlog updates:
- T01 row rewritten to reflect that only the audit landed (not the
file move) and to point at the new T-cleanup-01 follow-up.
- New "Upstream cleanup tasks" section before the Decision log,
with T-cleanup-01: bring upstream duplicate.e2e-spec.ts up to date
with the current /duplicates/resolve API and PR upstream.
Estimated 2-4 hours of mechanical assertion rewriting.
Verified locally: the 4 working specs (helpers + timeline + face +
shared-space) still pass 205/205 across 3 consecutive runs after the
revert. tsc --noEmit clean. No regressions to our coverage.
* test(e2e): T18 gallery-map filter access matrix + filters
Adds e2e/src/specs/server/api/gallery-map.e2e-spec.ts with 12 tests
covering the fork-only `/gallery/map/markers` controller. This is the
filtered map endpoint distinct from `/map/markers` — accepts a rich
query (people, tags, EXIF, dates, favorite, country/city) used by the
web map view's filter panel.
Service shape (shared-space.service.ts:561-585):
- Without spaceId: scoped to auth.user.id
- With spaceId: requireAccess(SharedSpaceRead) → 400 for non-member
- personIds re-route to spacePersonIds when spaceId is set (same DTO
field, different semantics)
- Always filters visibility=Timeline regardless of client input
Tests:
1. Auth required (anon → 401)
2. Authenticated user with no filters returns their geotagged assets
3. Empty array for a fresh user with no uploads
4. country filter narrows correctly (matching + non-matching cases)
5. city filter narrows correctly (matching + non-matching cases)
6. isFavorite=true excludes non-favorite assets
7. takenAfter cutoff in the future excludes the asset
8. takenBefore cutoff in the past excludes the asset
9. rating outside 1-5 returns 400 (DTO @Min/@Max validation)
10. type with invalid enum value returns 400
11. Archived assets are excluded — service hardcodes visibility=Timeline.
Test toggles the asset to archive and verifies the marker disappears.
12. Cross-user isolation — another user does not see this user's markers
(without spaceId, the service scopes to auth.user.id).
Setup uses the existing thompson-springs.jpg fixture (real GPS in
Colorado, USA, with camera EXIF) so the metadata-based filters have
real data to match against. Pattern matches the existing /map e2e
spec for fixture upload + websocket wait.
T19 will cover spaceId scoping and the personIds → spacePersonIds
re-routing as a separate task per the backlog.
Verified locally: 12 tests green (986ms test runtime), tsc --noEmit
clean.
T18 ticked. Phase 2: 1/5 done.
* test(e2e): T19 gallery-map spaceId scoping
Adds 6 tests in a new nested describe inside the T18 block, covering
the spaceId code path in /gallery/map/markers.
Setup creates a fresh space owned by `user`, adds a second member
(spaceMember), and adds the geotagged fixture asset to the space so
it should appear in space-scoped queries. spaceNonMember is created
but never added to the space.
Tests:
1. Non-member gets 400 (requireAccess BadRequestException at
shared-space.service.ts:563). Same taxonomy as T03 timeline —
the bulk-access pattern returns 400 not 403.
2. Anon → 401.
3. Space member sees the space asset via spaceId — the load-bearing
read invariant for space-scoped map queries.
4. Space owner with spaceId sees space-scoped content (the assertion
would catch a regression that returned the full owner library
instead of the space subset).
5. Non-existent spaceId returns 400 — bulk-access pattern (no 404).
6. country filter composes with spaceId — `country=Antarctica`
returns empty even though the asset would otherwise be visible.
PR #202's "hidden persons exclusion on gallery-map" angle is deferred
to a follow-up — it would need a shared_space_person fixture with
isHidden=true similar to T09's setup, which is more involved than the
T19 scope.
Verified locally: 18 tests green in gallery-map.e2e-spec.ts (12 T18
+ 6 T19, 898ms test runtime), tsc --noEmit clean.
T19 ticked. Phase 2: 2/5 done.
* docs(plans): mark T20 N/A — /map/markers has no spaceId support
T20 was originally planned as "space scoping extension to map.e2e-spec.ts"
but the upstream /map/markers endpoint has no spaceId support at all:
- server/src/dtos/map.dto.ts:29-47 — MapMarkerDto has no spaceId field
- server/src/services/map.service.ts:9-27 — getMapMarkers has no spaceId
branch (only userIds + albumIds)
- server/src/repositories/map.repository.ts — no spaceId references
Space-scoped map queries are entirely on the fork-only /gallery/map/markers
endpoint that we just covered in T18+T19. T20 is moot.
Marked the row as N/A in the backlog with the verification line refs so
a future maintainer doesn't reattempt the task.
Phase 2 is now effectively a 4-task group (T18/T19/T21/T22), with T20
crossed out.
* test(e2e): T21 view folder browsing tests
Adds e2e/src/specs/server/api/view.e2e-spec.ts with 9 tests covering
the /view controller's two folder-browsing endpoints.
Service shape (view.service.ts:7-16): both endpoints strictly scope
to auth.user.id. No partner sharing, no space scoping, no library
joins. The folder browse is owner-only.
Tests:
GET /view/folder/unique-paths (3):
1. Auth required (anon → 401)
2. Authenticated user gets their unique folder paths (non-empty array)
3. Cross-user isolation — userB's response does not contain any of
userA's paths that include userA's userId. The actual upload paths
include the user UUID, so this is a structural assertion that no
leak path exists between users.
GET /view/folder (5):
4. Auth required
5. Returns assets when given a known path from the user's folder list
(resolves a path via /unique-paths first, then queries /folder).
6. Empty array for a non-existent path
7. Cross-user isolation — userB calling /folder with userA's path
does NOT see userA's asset in the result. Pins that the service
really does scope by auth.user.id even when the caller-supplied
path matches another user's folder structure.
8. **Missing `path` query param returns 500** — REAL FINDING. The
controller at view.controller.ts:33 declares `@Query('path') path:
string` with no validation pipe and no default. The service passes
undefined to the repository which trips with a 500. Pinning the
actual behavior so a future server-side fix forces a deliberate
update. Worth filing upstream as a small server bug — should be
400 with a clear validation message.
Plus a small sanity assertion (9) that touches the userBAssetId
fixture so the linter doesn't flag it as unused.
T20 was N/A so this is the next Phase 2 task. T22 (workflow) is the
last one before Phase 3.
Verified locally: 9 tests green (404ms test runtime), tsc --noEmit
clean.
T21 ticked. Phase 2: 4/5 done (T20 was N/A).
* test(e2e): T22 workflow CRUD access matrix — closes Phase 2
Adds e2e/src/specs/server/api/workflow.e2e-spec.ts with 15 tests
covering the fork-only /workflows controller. Last task in Phase 2.
Service shape (workflow.service.ts):
- create: validates triggerType + per-plugin filter/action IDs (400 on bad ID)
- getAll: scoped to auth.user.id (owner-only)
- get/update/delete: requireAccess(WorkflowRead/Update/Delete) → 400 for non-owner
- update with no fields → BadRequestException('No fields to update')
- getAll cross-user returns empty array (not 403) — owner-scoped query
Workflows are per-user with no sharing concept. Cross-user access is
uniformly rejected at the access layer.
Tests:
POST /workflows (5):
1. Auth required (anon → 401)
2. Create with empty filters and actions succeeds (201, returns
workflow with ownerId, default enabled=true, AssetCreate trigger)
3. Invalid trigger type returns 400
4. Invalid pluginFilterId returns 400 with /filter/i message
5. Empty name returns 400 (DTO @IsNotEmpty)
GET /workflows (2):
6. Auth required
7. Owner-scoped — userA's workflows visible to userA, userB sees
empty array
GET /workflows/:id (3):
8. Owner can fetch their own workflow
9. Cross-user GET returns 400 (requireAccess bulk-access)
10. Non-existent workflow ID returns 400 (not 404)
PUT /workflows/:id (3):
11. Owner can rename
12. Empty PUT body returns 400 with /no fields/i message — pins the
explicit "No fields to update" service guard
13. Cross-user PUT returns 400
DELETE /workflows/:id (2):
14. Owner can delete + subsequent GET returns 400 (workflow gone)
15. Cross-user DELETE returns 400
Verified locally: 15 tests green (489ms test runtime), tsc --noEmit
clean.
T22 ticked. **Phase 2 COMPLETE** — T18/T19/T21/T22 done; T20 was N/A.
Phase 2 added 42 tests across 3 new spec files (gallery-map, view,
workflow). Total branch coverage: 247 e2e tests (Phase 1 = 205 + Phase
2 = 42).
* test(e2e): apply post-Phase-2 review nits
Three NITs from the Phase 2 review, all addressed:
1. **gallery-map T19 — missing personIds re-routing test.** The
service at shared-space.service.ts:569-570 re-routes
`dto.personIds → spacePersonIds` when `spaceId` is set, making
the same DTO field mean different things in different contexts.
Added a 7th test in the T19 nested describe that probes the
re-routing: passing a bogus person UUID with spaceId returns
empty (the lookup goes through shared_space_person, not asset_face).
Not the strongest possible assertion, but pins the behavioural
shape so a future refactor that changes the re-routing direction
would be caught.
2. **view T21 — dead sanity-check test.** The original spec had
`expect(typeof userBAssetId).toBe('string')` only to satisfy
the linter about the unused `userBAssetId` fixture. Removed both
the variable assignment and the dead test. The cross-user-isolation
tests already exercise userB by querying with their token; we
don't need their asset id specifically. Drop both, keep the
second createAsset call so userB has folder content.
3. **gallery-map backlog row count.** T19 said "6 tests" but the
actual count is 7 with the new re-routing test. Updated the row.
Verified locally: 27 tests green across gallery-map (19) + view (8),
tsc --noEmit clean.
* test(e2e): T23 asset metadata K/V CRUD
Adds e2e/src/specs/server/api/asset-metadata.e2e-spec.ts with 13
tests covering the asset metadata K/V endpoints (PUT /:id/metadata,
GET /:id/metadata, GET/DELETE /:id/metadata/:key, bulk PUT/DELETE
/assets/metadata).
All routes use Permission.AssetRead/AssetUpdate which routes through
requireAccess (bulk-access pattern → 400 for non-owner).
Tests:
GET /assets/:id/metadata (3):
1. Auth required (anon → 401)
2. Owner can list (empty initially)
3. Non-owner returns 400 (bulk-access)
PUT /assets/:id/metadata (3):
4. Owner upsert + value is queryable via the listing
5. Upsert overwrites an existing key value (set v=1, set v=2,
verify single-key fetch returns v=2)
6. Non-owner upsert returns 400
GET /assets/:id/metadata/:key (2):
7. Owner can fetch a single key set in test 4
8. Missing key returns 400 or 404 (test pins both — the actual
path uses requireAccess + service throw, behavior pinned)
DELETE /assets/:id/metadata/:key (2):
9. Owner can delete a key and it's removed from the listing
10. Non-owner delete returns 400
PUT /assets/metadata (bulk) (2):
11. Owner can upsert across multiple of their own assets in one call
12. Bulk upsert with a non-owner asset id mixed in is rejected (400)
DELETE /assets/metadata (bulk) (1):
13. Owner can bulk-delete keys across multiple of their own assets
Verified locally: 13 tests green (554ms test runtime), tsc --noEmit
clean.
T23 ticked. Phase 3: 1/6 done.
* test(e2e): T24 asset OCR endpoint access mat…
Deeds67
added a commit
to open-noodle/gallery
that referenced
this pull request
Apr 9, 2026
* docs(plans): research e2e API test coverage gaps and backlog
Catalogues the 282-endpoint server surface against the 29 existing
e2e API specs, identifies the 8 zero-coverage controllers and the
endpoint holes inside heavily-tested specs (shared-space /people and
/libraries, asset metadata/edits/copy, library->space link side
effects). Defines a reusable Permission/Actor matrix and proposes
~14 follow-up PRs prioritised by bug-catching ROI.
* docs(plans): break e2e API coverage research into per-task backlog
Adds a working backlog of 39 PR-sized tasks (T01–T39) derived from the
e2e API coverage research doc, plus full design docs for the three
upfront tasks: helpers (T02), timeline access matrix (T03), and the
shared-space people listing (T09).
The backlog records architectural decisions (helper API shape, fixture
lifetime, extending utils.createSpacePerson over duplication) separately
from observed server invariants (400 vs 403 split between requireAccess
and requireMembership, the listing's thumbnailPath gate, the metadata vs
thumbnail role split on PATCH /shared-spaces). Each task row hard-pins
its dependencies so most of Phase 1+ can run in parallel after T02.
Four code-reviewer passes caught and fixed ~15 substantive issues across
the drafts; the docs as committed here are the result.
* test(e2e): T01 move stray duplicate spec into the discovered glob
The vitest config at e2e/vitest.config.ts uses
`src/specs/server/**/*.e2e-spec.ts` for spec discovery. The file at
e2e/src/api/specs/duplicate.e2e-spec.ts was outside that glob and has
never been running in CI — it was added during the duplicate-detection
work but landed in the wrong directory.
Move it to e2e/src/specs/server/api/duplicate.e2e-spec.ts (where every
other server API spec lives), remove the now-empty src/api/specs and
src/api directories, and tick T01 in the coverage backlog.
The companion audit of waitForQueueFinish vs expect.poll for non-admin
specs (per the e2e-coverage research doc and the
feedback_e2e_admin_only_queues memory) found zero violations: all 38
existing callers correctly pass admin.accessToken. Result documented
inline in the backlog row.
Imports in the moved file use the `src/*` path alias, so file depth is
irrelevant — no code changes were needed beyond `git mv`.
* test(e2e): T02 add Actor / SpaceContext / forEachActor helpers
Adds e2e/src/actors.ts with Actor + ActorId + SpaceContext types and
the buildSpaceContext / forEachActor helpers, and extends the existing
utils.createSpacePerson to insert the shared_space_person_face junction
row, accept a type parameter, and return {globalPersonId, spacePersonId,
faceId} instead of just the space person ID.
These exist to turn the Permission/Actor matrix from §3 of the e2e
coverage research doc into a one-liner per endpoint, so downstream
specs (T03+) can write `forEachActor(...)` instead of hand-rolling six
describe blocks per endpoint. See docs/plans/2026-04-06-e2e-T02-helpers-design.md
for the rationale and decision log.
Key design points (all captured in the design doc):
- buildSpaceContext composes utils.adminSetup / userSetup / createSpace /
addSpaceMember / addSpaceAssets / createAsset — no parallel
implementations of any existing helper.
- forEachActor throws Error (not expect.toBe) so the failure message
names the actor that failed; without that, debugging an actor matrix
is needlessly painful.
- Sequential, not parallel — tests share a database and parallel actor
runs would race on the same fixtures.
- Fixture lifetime contract: ctx is read-only in beforeAll; mutating
tests own their cleanup via try/finally or nested describes.
- ActorId starts minimal (8 actors). partner / libraryOwner / apiKey* /
sharedLink land with their first consumer task.
Smoke tests in e2e/src/specs/server/api/_helpers.e2e-spec.ts validate
all three behaviours that downstream PRs depend on:
1. Auth threading — bearer token reaches the server for every actor.
2. Anon split — /users/me requires auth (anon: 401, members: 200).
3. createSpacePerson extension — returns three IDs and inserts the
shared_space_person_face junction row (verified via direct DB query).
4. Role assignment — PATCH /shared-spaces/:id with {thumbnailCropY: 0}
distinguishes Owner/Editor from Viewer. Uses thumbnailCropY (Editor-
level) rather than name (Owner-level per shared-space.service.ts:197-203)
so Editor and Viewer get distinct status codes.
Implemented test-first: each smoke test was written and run failing
before the corresponding helper code was added.
Verified locally against the e2e stack: all 4 smoke tests green
(537ms total), server.e2e-spec.ts unchanged (24/24 still pass), tsc
--noEmit clean. createSpacePerson currently has zero callers in e2e/
(verified via grep) so the signature change is risk-free.
T01 ticked in the backlog along with T02. The cleanup audit found zero
non-admin waitForQueueFinish callers — the rule from the
feedback_e2e_admin_only_queues memory is currently held everywhere.
* test(e2e): T03 add timeline /buckets and /bucket access matrix
Adds e2e/src/specs/server/api/timeline.e2e-spec.ts with 9 tests
covering the access matrix for both timeline endpoints. This is the
first real consumer of the actor / forEachActor helpers from T02.
Tests on GET /timeline/buckets:
1. Auth required (anon: 401, owner: 200)
2. Owner sees own assets without filter (count == 2)
3. spaceId access matrix — status codes for owner/editor/viewer/non-member/anon.
Non-member returns **400** (not 403), pinned because timeline uses requireAccess
which throws BadRequestException (src/utils/access.ts:37-42), distinct from the
shared-space-family endpoints which use requireMembership and return 403.
4. spaceId scopes assets to the space, not the requesting user. spaceOwner with
spaceId sees the 1 space asset, NOT the 2 they own. Catches the bug shape where
the implementation would `WHERE asset.ownerId = auth.user.id` instead of joining
through shared_space_asset.
5. Non-owner space members (editor, viewer) actually see space content via spaceId.
This is the PR #163 / #202 bug shape — it returned 200 with empty body, so pure
status-code testing would have missed it.
Tests on GET /timeline/bucket (singular):
6. Auth required.
7. spaceId access matrix mirroring test 3 — same shape, distinct endpoint. The
risk being probed: forgetting to apply the same scoping check on the singular
endpoint. PR #260 is in this family.
8. Non-owner space members see the space asset via /bucket. Same bug class as
test 5, applied to the asset-list endpoint. Asserts the actual asset ID is in
the response, not just the status code.
9. /bucket returns the parallel-array TimeBucketAssetResponseDto shape, not
bucket counts. Sanity check that the two endpoints aren't confused.
Coverage matches the 10 tests enumerated in the T03 design doc; design tests 3
and 6 are merged into one access-matrix test (the matrix already pins
spaceNonMember: 400, so a separate test would assert the same thing).
Verified locally against the e2e stack: 9 tests green (580ms total). Combined
with the 4 helper smoke tests, total: 13 tests, 1.06s. tsc --noEmit clean.
T03 ticked in the backlog.
* test(e2e): apply post-T03 review nits
Three nits from the post-T01-T03 implementation review:
1. Add `authHeaders(actor)` helper to actors.ts and use it in
_helpers.e2e-spec.ts and timeline.e2e-spec.ts. The previous
`actor.token ? asBearerAuth(actor.token) : {}` pattern at every
forEachActor call site was unprecedented in the e2e suite —
localizing the conditional inside actors.ts keeps the call sites
consistent with idiomatic supertest usage.
2. Drop the redundant `utils.initSdk()` calls in the new spec files'
beforeAll. utils.ts:809 already invokes it at module load, and no
other spec under specs/server/api calls it explicitly.
3. Drop the try/finally + utils.disconnectDatabase() in
_helpers.e2e-spec.ts smoke test 3. Existing specs that use the
raw pg client (shared-space.e2e-spec.ts, etc.) don't disconnect —
they rely on worker-process exit. Disconnecting mid-spec would
break any later test in the file that uses utils.createSpacePerson
or any other client-using helper. Add an inline comment explaining
why.
`asBearerAuth(actor.token!)` is left in place at non-forEachActor
call sites where the actor is always authenticated — that's the
existing convention across the e2e suite.
Verified locally: 13 tests still green (4 helpers + 9 timeline,
1.03s), tsc --noEmit clean.
* test(e2e): T04 add timeline withSharedSpaces / withPartners semantics
Extends actors.ts and timeline.e2e-spec.ts to cover the two flags
that gate cross-user content on the timeline endpoints.
Helper changes (actors.ts):
- Add `partner` to ActorId.
- Add optional `partner` and `partnerAssetId` fields to SpaceContext.
- Add `BuildSpaceContextOptions` with `withPartner?: boolean`. When
set, buildSpaceContext creates an extra user, has them share their
library with spaceOwner, and uploads one asset for them.
- Add `addPartner({token, userId}, {token, userId})` helper. The
default `partner.inTimeline` column is **false** (verified at
server/src/schema/tables/partner.table.ts:46), so a fresh
`createPartner` call is invisible to `withPartners=true` until the
recipient enables it. The helper auto-enables inTimeline by having
the recipient call PUT /partners/:id, so test call sites don't have
to remember the two-step dance. This default-false behaviour is
exactly the kind of footgun an integration test would have caught
if anyone had ever written one before today.
New tests in timeline.e2e-spec.ts (`describe('GET /timeline/buckets — withSharedSpaces and withPartners')`):
1. withSharedSpaces=true makes a non-owner member see space content
on their own timeline. spaceEditor owns 1 asset (editorAssetId);
with withSharedSpaces, the union picks up spaceAssetId via the
membership default of showInTimeline=true, so total goes 1 -> 2.
2. Toggling showInTimeline=false drops the space out of
getSpaceIdsForTimeline. PATCH /shared-spaces/:id/members/me/timeline
with {showInTimeline: false}; verify total goes back to 1; restore
in try/finally per the fixture lifetime contract.
3. withPartners=true makes spaceOwner see partner-shared assets.
Total = 2 own + 1 partner = 3.
4. Default (no withPartners) excludes partner assets. Total = 2.
5. Combining withSharedSpaces and withPartners doesn't double-count —
spaceAssetId is already counted in spaceOwner's own 2, so the union
stays at 3.
All calls pass `visibility=timeline` explicitly because timeline.service.ts:91-113
treats `visibility === undefined` as `requestedArchived = true` and throws 400 when
either flag is set. This invariant is now pinned in the backlog.
Backlog updates:
- T04 row ticked (5 tests).
- New "Observed invariants" entries: partner.inTimeline default-false,
withSharedSpaces/withPartners visibility requirement.
Library-linked space asset visibility (mentioned in the original T04
design) is deferred to T17, which is the right home — it requires the
linkLibrary helper that doesn't exist yet.
Verified locally: 18 tests green (4 helpers + 14 timeline, 1.11s),
tsc --noEmit clean.
* test(e2e): T05 add timeline visibility filter tests
Adds 7 new tests to timeline.e2e-spec.ts: 5 for the visibility filter
behaviour itself, plus 2 invariant pins folded in from the T04 review
NIT (visibility-undefined-throws-400).
The visibility tests use a dedicated user (visibilityUser) with 4
assets in different states: timeline, archive, hidden, and trashed.
A separate user keeps the assertions deterministic — using spaceOwner
would have to subtract the existing space-related assets from every
expected count.
Tests in `describe('GET /timeline/buckets — visibility filters')`:
1. **default visibility (no param) returns Timeline AND Archive (2)** —
pins the non-obvious server behaviour at server/src/utils/database.ts:79-81.
`withDefaultVisibility` is `where('asset.visibility', 'in', [Archive, Timeline])`,
NOT just Timeline. The web UI's main timeline view must pass
`visibility=timeline` explicitly to exclude archived assets. This was a
real surprise during implementation — the first version of the test
expected count=1 and failed with count=2 against the live server.
2. visibility=timeline returns only the strict timeline assets (1).
3. visibility=archive returns only archived assets (1).
4. visibility=hidden returns only hidden assets (1). Hidden visibility is
normally used for the video part of live photos / motion photos
(per the AssetVisibility enum docstring); pinned here to protect that
path from a future refactor.
5. trashed assets are excluded regardless of visibility filter. Asserts
the count under both default and explicit visibility=timeline so a
trash regression would inflate either independently.
Tests appended to the withSharedSpaces/withPartners describe (folded
from T04 review NIT):
6. withSharedSpaces=true without explicit visibility returns 400.
7. withPartners=true without explicit visibility returns 400.
Backlog updates:
- T05 ticked.
- New "Observed invariant" entry: default visibility filter is
permissive (Timeline + Archive) — this is exactly the kind of fact
that bites every author who hasn't read database.ts.
Verified locally: 21 tests green (4 helpers + 17 timeline, 812ms),
tsc --noEmit clean.
* test(e2e): apply post-T05 review nits
Three nits from the post-T05 implementation review:
1. Visibility-400 invariant tests now also assert on the error message
(`/withSharedSpaces/` and `/withPartners/`). Without that, a future
unrelated 400 (e.g. a DTO validation change) would silently still
satisfy the test.
2. Renamed test 5 to "soft-deleted (trashed) assets are excluded
regardless of visibility filter". Both soft-delete and force-delete
set `deletedAt`, so the test characterises the deletedAt-based
exclusion which is what the timeline query actually depends on.
Comment now spells this out.
3. Refactored the `Promise.all` 4-element destructure into a parallel
block of 3 + a sequential trash creation. The previous
`const [, , , trashedAsset] = await Promise.all([...])` was uncommon
in the e2e suite. Pulling trashed out of the parallel block is
slightly slower (4 sequential RTTs vs 3 parallel + 1) but more
readable, and the cost is negligible at fixture-setup time.
Verified locally: 21 tests still green, tsc --noEmit clean.
* test(e2e): T06 add timeline filter passthrough tests with spaceId
Adds 5 tests to timeline.e2e-spec.ts in a new describe block, probing
how the various filter parameters (`spacePersonId`, `personId`, `tagIds`)
interact with `spaceId` scoping. This is the PR #260 bug shape pin —
the fork has a *dedicated* `spacePersonId` DTO field separate from
`personId`, and the original PR #260 bug was matching a global `personId`
against a `shared_space_person.id`.
Setup creates two new fixtures attached to ctx (without mutating any
existing state):
- `spacePerson` via utils.createSpacePerson — adds Alice as a face on
spaceAssetId, with the shared_space_person_face junction row that
the timeline filter joins through.
- `spaceTagId` — a tag owned by spaceOwner, applied to spaceAssetId.
Tests:
1. **spacePersonId + spaceId** restricts to assets containing that
space person. Joins through shared_space_person_face → asset_face
→ asset; only spaceAssetId qualifies, total=1.
2. **GLOBAL personId + spaceId** does not cross-pollute. Pinned at
total=1 (which here is *legitimate* because the global person row
is the same one createSpacePerson set up under the hood — not a
bug). The test exists so a future change that decouples the two
joins or breaks the spaceId restriction is caught.
3. **spacePersonId without spaceId** falls back to owner-scoped and
still filters correctly (total=1, just spaceAssetId).
4. **tagIds + spaceId** returns the tagged subset of space content.
5. **non-owner space member sees tagged space content via
spaceId+tagIds** — spaceEditor querying with spaceOwner's tag
returns the tagged space asset. Pins that the timeline tag filter
doesn't enforce per-user tag ownership for space-scoped queries,
which is the expected UX for shared spaces (one member labels a
photo, others see the label).
Coverage matches 5 of the ~8 tests in the original T06 design. EXIF
filter passthrough (country/make/rating) is deferred because it requires
fixture images with extracted metadata (the e2e suite uses generated
PNGs for most tests). When a follow-up wants EXIF coverage it can be
appended to this describe block.
Verified locally: 26 tests green (4 helpers + 22 timeline, 868ms),
tsc --noEmit clean.
T06 ticked. All four Phase 1 timeline subtasks (T03–T06) are now done.
* test(e2e): apply post-T06 review nits
The post-T06 review caught a real coverage gap: the original test 2
("global personId does NOT cross-pollute") asserted total=1, which
was a coincidence — the global person from createSpacePerson is
legitimately attached to spaceAssetId via asset_face, so the join
hits it whether or not the spaceId restriction is working. A
regression that removed the spaceId scoping entirely would still
return 1 and the test would pass.
To actually pin the boundary:
1. beforeAll now also creates a decoy global person ("Decoy Bob")
attached only to ownerAssetId (NOT in the space). No
shared_space_person row, no junction.
2. Test 2 is now the load-bearing boundary test: query with
personId=<decoy>&spaceId=<spaceId> and assert total=0. The
decoy's asset is NOT in the space; if the spaceId scoping is
working, the join must not return it. If a future regression
breaks the scoping, this test goes from 0 to 1.
3. Added a new test 3 for the other half of the boundary: querying
with personId=<spacePerson.globalPersonId>&spaceId=<spaceId> still
returns 1 because that global person IS attached to a space asset
via asset_face. Together, tests 2 and 3 pin both sides of the
spaceId boundary on the global personId join path.
Plus the IMPORTANT and NIT comment improvements:
- Test "spacePersonId without spaceId" comment now spells out that
the join is NOT spaceId-restricted — a future test that puts the
same spacePersonId on a second space's asset would observe
count > 1.
- Test "non-owner space member sees tagged content" comment now
spells out the actual invariant being pinned: hasTags has zero
per-user check, tag IDs are universally addressable on the
timeline filter. The shared-spaces UX consequence (one member
labels, all members can filter) is documented as the load-bearing
property — a future refactor that adds an owner check to hasTags
would silently break the UX unless this test catches it.
Verified locally: 27 tests green (4 helpers + 23 timeline, 914ms),
tsc --noEmit clean.
* test(e2e): T07 add face CRUD access matrix
Adds e2e/src/specs/server/api/face.e2e-spec.ts with 10 tests covering
the four /faces endpoints. Also extends utils.createFace to return the
inserted face id (was Promise<void>) so PUT/DELETE tests can address
specific faces — backwards-compatible because all existing callers
ignore the return value.
Tests:
POST /faces (3):
1. Access matrix on the asset side. Owner can; spaceNonMember 400;
anon 401. Same Immich-wide bulk-access pattern as timeline (400
not 403 for non-owner).
2. Cross-owner asset rejected even when the person belongs to caller.
3. Cross-owner person rejected even when the asset belongs to caller.
GET /faces (2):
4. Access matrix on the asset side.
5. Owner gets the face row back with the linked person populated.
Asserts presence of the specific face we inserted (not a count) to
stay robust against unrelated faces accumulating on the asset.
PUT /faces/:personId — reassign (2):
6. Access matrix when reassigning a face to a new person owned by
the same user (Alice → Anne).
7. Reassigning to a cross-owner target person is rejected — the
person-access check on the target fires before any state mutation.
DELETE /faces/:faceId (3):
8. Owner can soft-delete (force=false).
9. Owner can force-delete (force=true).
10. Access matrix for non-owner / anon. Each test creates its own
scratch face so the access matrix doesn't permanently mutate
state.
The non-obvious API shapes are documented in the spec file header
and pinned in the backlog "Observed invariants" section:
- POST /faces returns void (no face id)
- PUT /faces/:id has path=target-person, body=face — the FaceDto is
reused with different meanings on different endpoints
- GET /faces takes ?id=<assetId> (the FaceDto field is named `id`
but represents the asset)
The first attempt at the spec hit four cascading failures from my
incorrect API understanding (POST returning void, PUT semantics
backwards, count assertion fragile to other tests). The corrected
version pins the actual server behaviour and is robust to test
isolation.
Verified locally: 41 tests green across 3 spec files
(4 helpers + 27 timeline + 10 faces, 1.82s test runtime),
tsc --noEmit clean.
T07 ticked.
* test(e2e): T08 add face deletion side effect tests + post-T07 nit fix
Adds 6 tests to face.e2e-spec.ts in a new "face deletion side effects (T08)"
describe block, plus a one-line comment fix from the post-T07 review.
T08 tests:
1. Soft-deleted face is excluded from GET /faces?id=<assetId> via the
`asset_face.deletedAt IS NULL` filter (person.repository.ts:229).
2. Hard-deleted face is excluded from GET (deleted row, same observable
result).
3. Soft-deleting the only face on a person preserves the person row.
Global persons are NOT cascade-deleted when their last face goes away;
GET /people/:id still returns the person. This matters for the
shared-spaces UX where the person row outlives any individual face
attachment.
4. Soft-deleting a face decreases the person's getStatistics asset count.
getStatistics joins through asset_face filtering on `deletedAt IS NULL`
AND `isVisible IS true` (person.repository.ts:335-352), so soft-delete
drops the row out of the count.
5. Hard-delete decreases the count via the same mechanism (row removal).
6. Re-attaching the same (assetId, personId) after a soft-delete inserts
a NEW asset_face row. There's no UNIQUE constraint blocking this;
pinned so a future schema change is caught.
Two scope adjustments documented in the backlog row:
- "Below-minFaces faces unaddressable" was a *space-person* concern from
PR #139 (space person thumbnail 404s), not a global face concern.
Moved to T10/T11.
- "Space-person dedup queue jobId dedup" from PR #292 requires probing
queue state that isn't exposed via the face controller. Moved to T14
which owns the deduplicate endpoint.
Post-T07 review NIT fix:
- POST /faces access matrix comment said "write access to BOTH the asset
and the person", but person.service.ts:641-642 actually uses
`Permission.AssetRead` + `Permission.PersonRead` (not write). Comment
reworded.
Verified locally: 47 tests green across 3 spec files (4 helpers + 27
timeline + 16 faces, 1.88s test runtime), tsc --noEmit clean.
T08 ticked. Faces group complete (T07 + T08).
* test(e2e): apply post-T08 review nit — strengthen the re-attach test
Test 6 ("re-attaching a face after a soft-delete") originally asserted
both faces on the same asset, which made the stats `count(distinct
asset.id)` = 1 with or without the deletedAt filter — the assertion
passed by accident, not because of the filter.
Strengthen by:
1. Putting the soft-deleted face on assetA and the new face on assetB.
The stats count now actually distinguishes the two semantics:
- With deletedAt filter (correct): 1 (only assetB)
- Without it (broken): 2 (assetA + assetB)
2. Adding a third face on assetA (re-attach with the existing
soft-deleted row in place). Asserts no UNIQUE constraint blocks the
second insert AND that count goes 1 → 2 to confirm the new face is
counted.
The "no UNIQUE constraint" pin is preserved; the deletedAt filter is
now genuinely exercised by the count assertion.
Verified locally: 16 tests still green, tsc --noEmit clean.
* test(e2e): T09 add shared-space people listing tests
Adds an 11-test describe block to shared-space.e2e-spec.ts covering
the GET /shared-spaces/:id/people endpoint per the T09 design doc
(docs/plans/2026-04-06-e2e-T09-space-people-listing-design.md).
Setup: dedicated owner/editor/viewer/non-member users with their
own space and 5 space-people via the extended utils.createSpacePerson
helper from T02 (which inserts the four-table chain including the
shared_space_person_face junction).
Tests:
1. Access matrix — owner/editor/viewer 200, non-member 403, anon 401.
shared-space endpoints use requireMembership → ForbiddenException,
distinct from timeline's requireAccess → BadRequestException.
2. Listing returns space person IDs (NOT global person IDs). The
canonical assertion for the whole T09–T14 sub-tree.
3. Hidden persons excluded by default.
4. ?withHidden=true includes hidden persons.
5. Unnamed persons included by default.
6. ?named=true returns only persons with non-empty names (on either
shared_space_person.name OR person.name per the OR clause in
shared-space.repository.ts:514-521).
7. Toggling shared_space.petsEnabled=false hides pets, restored in
try/finally per the fixture lifetime contract.
Pagination tests in a nested describe (own beforeAll/afterAll for the
15 extra rows so they don't leak into sibling tests):
8. ?limit=10 caps the response.
9. ?offset paginates without overlap.
10. Sort order is stable across calls.
Final test in the parent describe:
11. Empty thumbnailPath on the underlying global person excludes the
space person from the listing. Pins the fork's "minFaces gate"
mechanism (shared-space.repository.ts:512-513) — pinned now so a
future query refactor that drops the filter would be caught.
Mutates via direct DB and restores in try/finally.
All assertions match the design doc decisions in the backlog
"Decision log" section: space person ID is canonical, stable sort,
extended createSpacePerson is the helper, listing query params are
limit/offset/withHidden/named/takenAfter/takenBefore (no `top`, no
text-based name search).
Backlog updates:
- T09 ticked.
- New "Known flaky-spec footgun" section: utils.createPerson +
Promise.all is unsafe with the shared pg.Client — observed once
as an FK violation in T09 setup, didn't reproduce in 3 follow-up
runs. Latent since T07. Not blocking; documented for follow-up.
Verified locally: 110 tests green in shared-space.e2e-spec.ts
(99 existing + 11 new T09), 157 tests across 4 spec files including
helpers/timeline/face/shared-space (4 + 27 + 16 + 110), 6.78s test
runtime, tsc --noEmit clean.
* test(e2e): apply post-T09 review nits
The post-T09 review caught one IMPORTANT and two NIT issues — all
fixed in this commit.
1. **IMPORTANT — missing afterAll in pagination describe.** The T09
commit message claimed "own beforeAll/afterAll" but the
implementation only had beforeAll. The 15 extra space-people rows
would leak into test 11 (the thumbnailPath gate) and any future
sibling test added to the parent describe. T11 still passes (it
asserts NOT contain), but the leak violates the T02 fixture
lifetime contract and would bite T10+. Added afterAll with
`DELETE FROM shared_space_person WHERE id = ANY($1::uuid[])` over
the captured ids.
2. **NIT — test 11 used a JOIN query for globalPersonId.** The
extended createSpacePerson helper from T02 already returns
`{globalPersonId, spacePersonId, faceId}` — no need to re-query
the database. Stored zeroThumbGlobalId in the describe scope and
dropped the 4-line JOIN.
3. **NIT — test 1 used a manual for-loop instead of forEachActor.**
The design doc explicitly called for forEachActor. The manual loop
worked but diverged from the T03+ pattern. Switched to forEachActor
with proper Actor objects, importing from src/actors. This also
sets the precedent for T10-T14 to use the helper consistently.
Verified locally: 110 tests still green in shared-space.e2e-spec.ts,
tsc --noEmit clean.
* test(e2e): T10 single space-person + thumbnail + assets
Adds 9 tests in a new nested describe inside T09's parent block,
sharing the T09 fixture setup. Covers the three read-only sub-endpoints:
- GET /shared-spaces/:id/people/:personId
- GET /shared-spaces/:id/people/:personId/thumbnail
- GET /shared-spaces/:id/people/:personId/assets
GET /people/:personId (5 tests):
1. Access matrix (owner/editor/viewer 200, non-member 403, anon 401).
2. Returns the canonical space person ID and name.
3. Hidden person IS fetchable directly — confirms half of the T09
open hypothesis: hidden filter is listing-only.
4. **Pet person is NOT fetchable directly when petsEnabled=false** —
DISPROVES the other half of the T09 hypothesis. The pet filter
applies BOTH to the listing and to the single-fetch endpoint.
This asymmetry vs hidden is real and worth pinning. UX
consequence: turning pets off in a space hides the entire pet
sub-graph, even from members who know the pet's ID.
5. **Non-existent personId returns 400 (not 404)** — bulk-access
pattern via requireAccess uniformly returns BadRequestException
for "not found OR no access" to avoid leaking existence. Same
taxonomic split as timeline. T11+ inherit this convention.
GET /people/:personId/thumbnail (2 tests):
6. Access matrix. **Member success-case is 500**, not 200, because
the fixture thumbnailPath ('/my/awesome/thumbnail.jpg' set by
utils.createSpacePerson) doesn't exist on disk. The access check
passes; the file resolution then fails and returns 500. Pinned
as a known footgun in the backlog — fixable later but out of
scope for T10. The 500 vs 404 distinction is a small server-side
bug independent of the access path.
7. Non-member 403 fires before file resolution (sanity check that
the 500 path isn't somehow leaking access).
GET /people/:personId/assets (2 tests):
8. Access matrix.
9. Returns the asset IDs containing the person — Alice is on
spaceAssetId so the response contains it.
Backlog updates:
- T10 ticked.
- The "open hypothesis" about hidden/pets at listing only is moved
to a new "Resolved hypotheses" section with the asymmetric finding
documented.
- New "Observed invariant": pet filter asymmetry (listing AND single
fetch) vs hidden filter (listing only).
- New "Observed invariant": single-person endpoints return 400 for
unknown IDs (bulk-access pattern), not 404.
- New "Known footguns" section: the thumbnail-500 issue.
Verified locally: 119 tests green in shared-space.e2e-spec.ts
(110 from T09 + 9 new T10), tsc --noEmit clean.
* test(e2e): apply post-T10 review fixes — service mechanism + thumbnail strategy
Two IMPORTANT findings from the post-T10 review, both about explanation
correctness rather than test logic.
1. **Backlog "bulk-access pattern via requireAccess" claim was wrong.**
`getSpacePerson` at shared-space.service.ts:625-636 calls
`requireMembership` (ForbiddenException for non-member, that part was
right), then runs `getPersonById` and manually
`throw new BadRequestException('Person not found')` for both the
missing-person case AND the pet-when-disabled case. The 400s the
T10 tests observe are real and load-bearing, but they come from
manual throws inside the service handler, NOT from the
`requireAccess` bulk pattern that timeline uses. Backlog
"Observed invariants" rewritten to cite the correct mechanism with
line references.
2. **Thumbnail 500 was a fixture wart, not a server bug.**
`getSpacePersonThumbnail` at shared-space.service.ts:643-657 has
THREE return paths once the access check passes:
- person not found / wrong space → NotFoundException → 404
- thumbnailPath null/empty → NotFoundException → 404
- thumbnailPath set → serveFromBackend → 200 (or 500 if missing)
The 500 in the previous T10 commit was triggered because
utils.createSpacePerson uses '/my/awesome/thumbnail.jpg' (a
non-empty path that doesn't exist on disk), which trips
serveFromBackend. The service actually has graceful 404 handling.
Restructured the thumbnail tests to exercise the **graceful 404
path**: transiently blank `person.thumbnailPath` via DB, assert
`{member: 404, non-member: 403, anon: 401}`. This pins the layered
ordering 401 < 403 < 404 — the correct member-success path for a
person with no thumbnail. Restore in try/finally.
The 200 path is not exercised because it would require pointing
the fixture at a real file in the upload location. That's a
reasonable follow-up but out of scope here.
Merged the two thumbnail tests into one (the matrix already
covers the access ordering, the second test was redundant). T10
is now 8 tests in the file (was 9), all assertions correct.
Backlog updates:
- Two "Observed invariants" rewritten to cite the manual
BadRequestException mechanism with the correct line numbers.
- New "Observed invariant" describing the three return paths of
getSpacePersonThumbnail.
- "Known footguns" entry rewritten: it's a fixture issue, not a
server bug. Mentions the follow-up to make createSpacePerson
accept a real fixture file.
Verified locally: 118 tests green in shared-space.e2e-spec.ts
(110 from T09 + 8 from T10), tsc --noEmit clean.
* test(e2e): T11 PUT/DELETE space person — rename, hide, delete
Adds 7 tests in a new nested describe inside the T09/T10 block,
covering mutation of a single space person via PUT and DELETE.
Both endpoints route through `requireRole(SharedSpaceRole.Editor)`
(verified at shared-space.service.ts:665, 704), so:
- Owner + Editor can mutate (200/204)
- Viewer is rejected (403)
- non-member is rejected (403, via requireMembership inside requireRole)
- anon (401, auth middleware)
PUT tests (4):
1. Access matrix for rename — owner+editor 200, viewer/non-member 403,
anon 401.
2. Actually renames the person — sends `{name: 'AfterRename'}` and
asserts the response body reflects the new name.
3. Marking isHidden=true hides the person from the default listing
but the direct fetch still returns 200 — pairs with T10's listing-only
hidden invariant. The PUT path is the supported way to set isHidden,
complementing T09's direct-DB-mutation pattern.
4. Non-existent personId returns 400 (manual BadRequestException at
shared-space.service.ts:668-669, same shape as T10).
DELETE tests (3):
5. Access matrix — owner+editor 204, viewer/non-member 403, anon 401.
Uses 5 different scratch persons (one per actor) to avoid
"delete-then-delete" race conditions in the matrix.
6. Preserves the underlying global person row — verifies via direct
DB query that `person` table row stays after `shared_space_person`
delete. The shared_space_person delete is correctly scoped and
does NOT cascade to the global person table.
7. Non-existent personId returns 400.
All scratch persons are created fresh per `it()` block via
utils.createSpacePerson, so the mutations are fully isolated and
don't affect T09's listing assertions or T10's read-only fixtures.
Verified locally: 125 tests green in shared-space.e2e-spec.ts
(110 from T09 + 8 from T10 + 7 from T11, 5.15s test runtime),
tsc --noEmit clean.
T11 ticked. Space-people sub-tree progress: 3/6 (T09/T10/T11 done,
T12 merge / T13 alias / T14 deduplicate remaining).
* test(e2e): T12 POST merge space persons
Adds 6 tests in a new nested describe inside the T09/T10/T11 block,
covering POST /shared-spaces/:id/people/:personId/merge.
Service shape (shared-space.service.ts:730-778): path :personId is the
*target*, body `{ids: string[]}` lists the *sources*. Requires Editor.
Validates both sides in the same space and the same type, reassigns
the source's junction rows to the target, deletes the source rows,
recounts the target's denormalised faceCount/assetCount, and queues
a dedup pass.
Tests:
1. Access matrix — owner+editor 204, viewer 403, non-member 403, anon 401.
Uses 5 separate scratch source persons (one per actor) so the
matrix doesn't try to merge the same source twice.
2. Merge reassigns the source's junction rows and deletes the source.
Verified via direct DB queries: source row gone, target now has 2
junction rows (its own face + the inherited one).
3. After merge, target's denormalised faceCount=2 and assetCount=1
(both faces are on the same asset, so distinct asset count is 1).
Pins recountPersons (shared-space.repository.ts:686+).
4. Cannot merge a person into themselves — 400 with message
matching /themselves/ from shared-space.service.ts:743-745.
5. Cannot merge across types (person target + pet source) — 400
with message matching /different types/ from line 754-756.
Pins the type-segregation invariant: pets and persons stay
separate even within the same space.
6. Missing target OR missing source returns 400. Two requests in
one test, both pinned.
Each test creates fresh scratch persons via utils.createSpacePerson
inside the `it()` block — fully isolated from T09/T10/T11 fixtures
and from sibling T12 tests.
Verified locally: 131 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12, 6.20s test runtime),
tsc --noEmit clean.
T12 ticked. Space-people sub-tree: 4/6 done (T09/T10/T11/T12). T13
alias and T14 deduplicate remaining.
* test(e2e): T13 space person alias — per-user, viewer-allowed
Adds 5 tests in a new nested describe inside the T09/T10/T11/T12 block,
covering PUT and DELETE alias.
Three critical invariants pinned by this block — the original T13
backlog row was wrong about both:
1. **Aliases are PER-USER, not visible to all members.** The service
stores `(personId, userId, alias)` and `getAlias(personId, auth.user.id)`
returns only the caller's row (shared-space.service.ts:780-798).
Owner setting "Mom" as Alice's alias is invisible to editor and
viewer. The original backlog row claimed "visible to all members"
which is the opposite of reality.
2. **Aliases require `requireMembership`, NOT `requireRole(Editor)`.**
Viewers CAN set their own aliases. Logical: aliases are personal
metadata, not space state — a read-only viewer should still be able
to label people for themselves.
3. **DELETE has no person existence check; it's idempotent on missing
personId.** Asymmetric vs PUT (which validates and returns 400).
Service code at lines 800-803.
Tests:
1. Access matrix — owner+editor+viewer 204 (all members can set their
own alias), non-member 403, anon 401.
2. Per-user persistence + isolation — owner sets "Mom" alias, owner GET
sees "Mom", editor GET sees null alias and the original 'PerUserAlice'
name. Pins both halves of the per-user invariant.
3. Alias does NOT modify global `person.name` — direct DB query
confirms the underlying `person.name` row stays at the original
value after alias is set.
4. DELETE removes the alias AND is idempotent on missing personId
(single test asserts both) — covers the asymmetry vs PUT.
5. PUT alias on missing personId returns 400 — pinned for symmetry
with T11/T12's missing-personId convention.
Backlog updates:
- T13 ticked with the corrected row description.
- New "Observed invariant" entry documenting the per-user + viewer-
allowed + idempotent-DELETE quirks.
Verified locally: 136 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13, 6.32s test
runtime), tsc --noEmit clean.
T13 ticked. Space-people sub-tree: 5/6 done. T14 (deduplicate) is
the last one before T15-T17 finish the space-libraries sub-tree.
* test(e2e): apply post-T13 review nit — tighten alias null assertions
Tests 2 and 4 used a defensive `alias === null || alias === undefined`
check, but `mapSpacePerson` always sets `alias: string | null` and never
omits the field. Tightened both assertions to `toBeNull()` so a future
regression that drops the field would be caught (the disjunction would
silently still pass on a missing field).
Other NITs from the post-T13 review (split the two-in-one DELETE test,
trim the verbose backlog row) were judgment calls — leaving as-is.
Verified locally: 136 tests still green, tsc --noEmit clean.
* test(e2e): T14 deduplicate space people — Owner-only + jobId dedup pin
Adds 4 tests in a new nested describe inside the T09/T10/T11/T12/T13
block, covering POST /shared-spaces/:id/people/deduplicate.
Service shape (shared-space.service.ts:721-728): the manual dedup
trigger requires `Owner` role (NOT Editor — distinct from PUT/DELETE/
merge), then queues a SharedSpacePersonDedup job on the
FacialRecognition queue with jobId `space-dedup-${spaceId}`
(job.repository.ts:239-241).
Tests:
1. **Owner-only access matrix** — owner 204, editor 403 (this is the
distinguishing test from T11/T12 which all only required Editor),
viewer 403, non-member 403, anon 401. Pins the role distinction.
2. Owner happy path returns 204 — sanity check on the success shape.
3. Two consecutive owner calls both return 204 — pins HTTP-level
idempotency, independent of whether BullMQ deduplicates underneath.
4. **PR #292 jobId dedup verification.** The load-bearing test for the
whole T14 task. Strategy: pause the FacialRecognition queue, empty
it, trigger dedup twice, count jobs whose data matches the test
space, restore in try/finally. The two triggers should produce
exactly ONE queued job — BullMQ's queue() with a duplicate jobId
is a no-op, so PR #292's behaviour is preserved.
Queue manipulation requires admin token (queue.controller.ts:23 —
admin: true). The `admin` token is already set up in the outer
beforeAll. The pause/restore is bracketed in try/finally so a test
failure doesn't leave the queue in a paused state and break the
rest of the suite.
Backlog updates:
- T14 ticked.
- New "Observed invariant" pinning the Owner-only role and the
jobId-based queue dedup, with the verification mechanism documented.
Verified locally: 140 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14, 5.29s
test runtime), tsc --noEmit clean.
T14 ticked. **Space-people sub-tree COMPLETE** (T09-T14, all 6 tasks).
Phase 1 progress: T01-T14 done, only T15-T17 (space-libraries
sub-tree) remaining before Phase 1 closes.
* test(e2e): apply post-T14 review fixes — DTO body + tightened assertions
Four post-T14 review findings, all addressed in this commit. The test
behaviour is unchanged (still 140/140 green) but the test logic now
matches the actual server contract instead of relying on dead body
fields and over-permissive predicates.
1. **BLOCKING — DTO body shape was wrong.**
The test's `DELETE /queues/:name/jobs` calls sent
`{statuses: [...]}`, but `QueueDeleteDto` only has `failed?: boolean`
(queue.dto.ts:24-31). The `statuses` field was silently dropped by
NestJS's whitelist validator and the underlying
`jobRepository.empty(name)` drained the queue unconditionally
anyway, so the test passed — but the body was misleading: it
implied a statuses-based filter that doesn't exist. Removed both
sends in try and finally; the empty call is now bodyless and the
intent is clear.
2. **Comment misleading.** The GET /queues/:name/jobs call applied no
filter, so it returned ALL jobs in any state, not just waiting/paused.
Updated the comment to spell that out and removed the misleading
"waiting/paused" language.
3. **NIT — try/finally hole.** The test now asserts `pauseRes.status
=== 200` immediately after the PUT pause, so a pause failure fails
loudly instead of silently letting the worker race the assertion.
4. **NIT — over-permissive filter.** The original predicate was
`j.data?.spaceId === spaceId || j.id === space-dedup-${spaceId}` —
the `||` would only widen if the jobId encoded something OTHER than
spaceId, which is exactly the kind of refactor we want to catch.
Tightened to a strict `j.data?.spaceId === spaceId` check on the
job's data payload, which is the load-bearing field we care about.
Verified locally: 140 tests still green in shared-space.e2e-spec.ts,
tsc --noEmit clean.
* test(e2e): T15 PUT space libraries — link library to space
Adds 7 tests in a new nested describe inside the T09-T14 block,
covering PUT /shared-spaces/:id/libraries.
Service shape (shared-space.service.ts:449-477) has a TWO-step gate:
1. `if (!auth.user.isAdmin)` → ForbiddenException 'Only admins can
link libraries to spaces' — admin gate, fires FIRST.
2. `requireRole(Editor)` — must be a space member with Editor or
Owner role.
Then library existence is checked (400 if missing). On success,
addLibrary returns null for duplicate (spaceId, libraryId) and skips
the face-sync queue — making the operation idempotent at HTTP level.
Tests:
1. **Non-admin owner of the space cannot link** — pins the admin
gate firing BEFORE role check. Even the space owner gets 403 if
they're not also a global admin. Asserts the message matches /admins/.
2. Non-admin editor cannot link — same admin gate.
3. Non-admin viewer cannot link — same.
4. Anon → 401.
5. **Admin who is an Editor in the space CAN link** — happy path. The
block's beforeAll adds the global `admin` user as an Editor in the
T09 test space.
6. **Idempotent on duplicate link** — calling link twice with the
same library returns 204 both times, and the shared_space_library
table has exactly 1 row for the (spaceId, libraryId) pair. Pins
the "204, not 409" behaviour explicitly.
7. **Library not found returns 400** — pins the existence check
message.
Setup creates two libraries via utils.createLibrary (admin token,
admin.userId as owner). The second library is used for the success
test so the idempotency test has a clean slate on the first library.
Backlog updates:
- T15 ticked with the corrected description (was "409 on duplicate" —
the actual behaviour is 204 idempotent).
- New "Observed invariant" pinning the two-step gate ordering and the
idempotent duplicate behaviour, with line references.
Verified locally: 147 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15,
5.35s test runtime), tsc --noEmit clean.
T15 ticked. Space-libraries sub-tree: 1/3 done. T16 (unlink) and T17
(link side effects) remaining before Phase 1 closes.
* test(e2e): T16 DELETE space libraries — unlink, idempotent
Adds 5 tests in a new nested describe inside the T09-T15 block,
covering DELETE /shared-spaces/:id/libraries/:libraryId.
Service shape (shared-space.service.ts:479-487) — same two-step gate
as T15:
1. `if (!auth.user.isAdmin)` → ForbiddenException 'Only admins...'
2. `requireRole(Editor)`
Then repository.removeLibrary(spaceId, libraryId) is a plain DELETE
on the (spaceId, libraryId) pair (shared-space.repository.ts:220-226)
with NO row-existence check. So unlink is idempotent at the HTTP
level: deleting an already-unlinked link, or deleting with a bogus
libraryId, both return 204.
Tests:
1. Non-admin owner cannot unlink (admin gate fires first, message
matches /admins/i).
2. Anon → 401.
3. **Admin Editor CAN unlink + DB row count goes 1 → 0.** Pre-checks
the row exists, calls unlink, then verifies the row is gone via
direct DB query. Pins the actual mutation.
4. **Unlinking an already-unlinked library is idempotent.** Reuses
scratchLibrary which test 3 just unlinked, calls again, expects
204.
5. **Unlinking with a non-existent libraryId returns 204.** Pins the
"no existence check" behaviour — even a bogus UUID is a no-op
DELETE → 204, NOT 404 (the original design row was wrong about
this).
Setup creates `scratchLibrary` per beforeAll and pre-links it via
the API so test 3 has a real link row to remove.
Backlog updates:
- T16 ticked with the corrected description (was "non-existent link
→ 404" — actual is 204 idempotent, no existence check).
Verified locally: 152 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15
+ 5 T16, 5.31s test runtime), tsc --noEmit clean.
T16 ticked. Space-libraries sub-tree: 2/3 done. T17 (link side
effects) is the last Phase 1 task before the entire phase closes.
* test(e2e): T17 library link side effects — closes Phase 1
Adds 6 tests in a new nested describe inside the T09-T16 block, the
final task of the space-libraries sub-tree and the last task of the
entire Phase 1 of the e2e coverage backlog.
T17 exercises the cross-table query path that link/unlink enable:
library assets becoming visible to space members via /timeline/bucket
?spaceId=. PR #163 was specifically about this code path. Setup
uploads an admin asset normally and UPDATEs its libraryId via direct
DB to associate it with a fresh library — bypassing the fragile
library scan path while exercising the same JOIN.
Tests:
1. **After link, a non-owner space member sees the library asset via
/timeline/bucket?spaceId=** — the load-bearing PR #163 invariant.
Editor (who owns no assets) calls the bucket query for the space
and the library asset appears via the shared_space_library JOIN.
2. **Viewer sees it too** — symmetric assertion for the read-only role.
3. **After unlink, library assets are no longer visible** — round-trip
pin: link → see → unlink → don't see.
4. **Soft-deleted library asset is hidden** via the `deletedAt IS NULL`
filter on the timeline query. Mutate `asset.deletedAt` to NOW(),
verify hidden, restore.
5. **Offline library asset IS still visible (NOT hidden)** — SURPRISING
FINDING. asset.repository.ts:835-849 joins shared_space_library on
(libraryId, spaceId) but does NOT filter on asset.isOffline. So a
library asset whose underlying file went offline is still listed in
the space's timeline bucket. The asymmetry vs `deletedAt` filtering
is real. Test pins the actual behavior — if a future change adds
the missing isOffline filter, the test fails and forces a deliberate
update. The access.repository's checkSpaceAccess in a different
code path DOES filter on isOffline=false, so the timeline path is
the inconsistent one.
6. **Library delete eventually cascades to shared_space_library** —
library.service.ts:370-379 is a SOFT delete: it sets `deletedAt`,
queues a LibraryDelete job, returns 204 immediately. The cascade
happens async when the job processes. Test calls
waitForQueueFinish('library') after the DELETE before asserting
the FK row is gone.
Setup uses a DB-direct approach (createAsset + UPDATE libraryId)
instead of the library scan helper because the scan path was hitting
unexplained timing/timeout issues — bypassing the scan keeps the
test focused on the JOIN behavior, which is what T17 actually probes.
Backlog updates:
- T17 ticked.
- New "Observed invariant": timeline spaceId query lacks the
isOffline filter on library assets — pinned with file:line and
the asymmetry called out.
- New "Observed invariant": library delete is soft + async cascade —
tests must wait for the LibraryDelete job to drain before asserting
the FK cascade.
Verified locally: 158 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15
+ 5 T16 + 6 T17, 5.50s test runtime), tsc --noEmit clean.
T17 ticked. **Phase 1 COMPLETE** — all 17 tasks (T01-T17) done.
* test(e2e): revert T01 duplicate spec move — upstream-broken, leave for upstream
Revert the file move from T01 (f19eb16bc). The original task moved
e2e/src/api/specs/duplicate.e2e-spec.ts → e2e/src/specs/server/api/
so the vitest glob would pick it up. T01's commit message said
"audit found zero violations" — but the audit was for waitForQueueFinish,
NOT spec correctness. I never actually ran the spec to verify it works
after the move.
The full Phase 1 review surfaced that the spec is upstream-broken:
- Added in upstream PR #25316 (2026-03-26) by @Phlogi as part of the
"feat(server): resolve duplicates" feature.
- Upstream PR #25856 (Feb 10, 2026, by @minidzelis) had restructured
e2e/src/api/specs/ → e2e/src/specs/server/api/ six WEEKS earlier.
- PR #25316 didn't notice and added the new spec in the OLD path.
The file went into the wrong directory and was never picked up by
the vitest glob in upstream's CI either.
- A subsequent upstream PR changed the response shape of
POST /duplicates/resolve from `{status, results: [{duplicateId, status}]}`
to a bare `BulkIdResponseDto[]`. The never-running spec didn't get
updated. 18 of 21 tests now fail against the current API.
- The 3 tests that DO pass are access-matrix style assertions that
are robust to body shape changes.
This is upstream's bug. Maintaining a fork-local fix would generate
merge conflicts every rebase. The right fix lives upstream — file an
issue / PR there.
Reverting the move puts the file back where upstream put it. Our
vitest glob doesn't pick it up, so our CI is unaffected by upstream's
broken tests. When upstream eventually fixes the spec (either fixes
the assertions or re-locates the file or both), we inherit the fix
cleanly via the next rebase.
Backlog updates:
- T01 row rewritten to reflect that only the audit landed (not the
file move) and to point at the new T-cleanup-01 follow-up.
- New "Upstream cleanup tasks" section before the Decision log,
with T-cleanup-01: bring upstream duplicate.e2e-spec.ts up to date
with the current /duplicates/resolve API and PR upstream.
Estimated 2-4 hours of mechanical assertion rewriting.
Verified locally: the 4 working specs (helpers + timeline + face +
shared-space) still pass 205/205 across 3 consecutive runs after the
revert. tsc --noEmit clean. No regressions to our coverage.
* test(e2e): T18 gallery-map filter access matrix + filters
Adds e2e/src/specs/server/api/gallery-map.e2e-spec.ts with 12 tests
covering the fork-only `/gallery/map/markers` controller. This is the
filtered map endpoint distinct from `/map/markers` — accepts a rich
query (people, tags, EXIF, dates, favorite, country/city) used by the
web map view's filter panel.
Service shape (shared-space.service.ts:561-585):
- Without spaceId: scoped to auth.user.id
- With spaceId: requireAccess(SharedSpaceRead) → 400 for non-member
- personIds re-route to spacePersonIds when spaceId is set (same DTO
field, different semantics)
- Always filters visibility=Timeline regardless of client input
Tests:
1. Auth required (anon → 401)
2. Authenticated user with no filters returns their geotagged assets
3. Empty array for a fresh user with no uploads
4. country filter narrows correctly (matching + non-matching cases)
5. city filter narrows correctly (matching + non-matching cases)
6. isFavorite=true excludes non-favorite assets
7. takenAfter cutoff in the future excludes the asset
8. takenBefore cutoff in the past excludes the asset
9. rating outside 1-5 returns 400 (DTO @Min/@Max validation)
10. type with invalid enum value returns 400
11. Archived assets are excluded — service hardcodes visibility=Timeline.
Test toggles the asset to archive and verifies the marker disappears.
12. Cross-user isolation — another user does not see this user's markers
(without spaceId, the service scopes to auth.user.id).
Setup uses the existing thompson-springs.jpg fixture (real GPS in
Colorado, USA, with camera EXIF) so the metadata-based filters have
real data to match against. Pattern matches the existing /map e2e
spec for fixture upload + websocket wait.
T19 will cover spaceId scoping and the personIds → spacePersonIds
re-routing as a separate task per the backlog.
Verified locally: 12 tests green (986ms test runtime), tsc --noEmit
clean.
T18 ticked. Phase 2: 1/5 done.
* test(e2e): T19 gallery-map spaceId scoping
Adds 6 tests in a new nested describe inside the T18 block, covering
the spaceId code path in /gallery/map/markers.
Setup creates a fresh space owned by `user`, adds a second member
(spaceMember), and adds the geotagged fixture asset to the space so
it should appear in space-scoped queries. spaceNonMember is created
but never added to the space.
Tests:
1. Non-member gets 400 (requireAccess BadRequestException at
shared-space.service.ts:563). Same taxonomy as T03 timeline —
the bulk-access pattern returns 400 not 403.
2. Anon → 401.
3. Space member sees the space asset via spaceId — the load-bearing
read invariant for space-scoped map queries.
4. Space owner with spaceId sees space-scoped content (the assertion
would catch a regression that returned the full owner library
instead of the space subset).
5. Non-existent spaceId returns 400 — bulk-access pattern (no 404).
6. country filter composes with spaceId — `country=Antarctica`
returns empty even though the asset would otherwise be visible.
PR #202's "hidden persons exclusion on gallery-map" angle is deferred
to a follow-up — it would need a shared_space_person fixture with
isHidden=true similar to T09's setup, which is more involved than the
T19 scope.
Verified locally: 18 tests green in gallery-map.e2e-spec.ts (12 T18
+ 6 T19, 898ms test runtime), tsc --noEmit clean.
T19 ticked. Phase 2: 2/5 done.
* docs(plans): mark T20 N/A — /map/markers has no spaceId support
T20 was originally planned as "space scoping extension to map.e2e-spec.ts"
but the upstream /map/markers endpoint has no spaceId support at all:
- server/src/dtos/map.dto.ts:29-47 — MapMarkerDto has no spaceId field
- server/src/services/map.service.ts:9-27 — getMapMarkers has no spaceId
branch (only userIds + albumIds)
- server/src/repositories/map.repository.ts — no spaceId references
Space-scoped map queries are entirely on the fork-only /gallery/map/markers
endpoint that we just covered in T18+T19. T20 is moot.
Marked the row as N/A in the backlog with the verification line refs so
a future maintainer doesn't reattempt the task.
Phase 2 is now effectively a 4-task group (T18/T19/T21/T22), with T20
crossed out.
* test(e2e): T21 view folder browsing tests
Adds e2e/src/specs/server/api/view.e2e-spec.ts with 9 tests covering
the /view controller's two folder-browsing endpoints.
Service shape (view.service.ts:7-16): both endpoints strictly scope
to auth.user.id. No partner sharing, no space scoping, no library
joins. The folder browse is owner-only.
Tests:
GET /view/folder/unique-paths (3):
1. Auth required (anon → 401)
2. Authenticated user gets their unique folder paths (non-empty array)
3. Cross-user isolation — userB's response does not contain any of
userA's paths that include userA's userId. The actual upload paths
include the user UUID, so this is a structural assertion that no
leak path exists between users.
GET /view/folder (5):
4. Auth required
5. Returns assets when given a known path from the user's folder list
(resolves a path via /unique-paths first, then queries /folder).
6. Empty array for a non-existent path
7. Cross-user isolation — userB calling /folder with userA's path
does NOT see userA's asset in the result. Pins that the service
really does scope by auth.user.id even when the caller-supplied
path matches another user's folder structure.
8. **Missing `path` query param returns 500** — REAL FINDING. The
controller at view.controller.ts:33 declares `@Query('path') path:
string` with no validation pipe and no default. The service passes
undefined to the repository which trips with a 500. Pinning the
actual behavior so a future server-side fix forces a deliberate
update. Worth filing upstream as a small server bug — should be
400 with a clear validation message.
Plus a small sanity assertion (9) that touches the userBAssetId
fixture so the linter doesn't flag it as unused.
T20 was N/A so this is the next Phase 2 task. T22 (workflow) is the
last one before Phase 3.
Verified locally: 9 tests green (404ms test runtime), tsc --noEmit
clean.
T21 ticked. Phase 2: 4/5 done (T20 was N/A).
* test(e2e): T22 workflow CRUD access matrix — closes Phase 2
Adds e2e/src/specs/server/api/workflow.e2e-spec.ts with 15 tests
covering the fork-only /workflows controller. Last task in Phase 2.
Service shape (workflow.service.ts):
- create: validates triggerType + per-plugin filter/action IDs (400 on bad ID)
- getAll: scoped to auth.user.id (owner-only)
- get/update/delete: requireAccess(WorkflowRead/Update/Delete) → 400 for non-owner
- update with no fields → BadRequestException('No fields to update')
- getAll cross-user returns empty array (not 403) — owner-scoped query
Workflows are per-user with no sharing concept. Cross-user access is
uniformly rejected at the access layer.
Tests:
POST /workflows (5):
1. Auth required (anon → 401)
2. Create with empty filters and actions succeeds (201, returns
workflow with ownerId, default enabled=true, AssetCreate trigger)
3. Invalid trigger type returns 400
4. Invalid pluginFilterId returns 400 with /filter/i message
5. Empty name returns 400 (DTO @IsNotEmpty)
GET /workflows (2):
6. Auth required
7. Owner-scoped — userA's workflows visible to userA, userB sees
empty array
GET /workflows/:id (3):
8. Owner can fetch their own workflow
9. Cross-user GET returns 400 (requireAccess bulk-access)
10. Non-existent workflow ID returns 400 (not 404)
PUT /workflows/:id (3):
11. Owner can rename
12. Empty PUT body returns 400 with /no fields/i message — pins the
explicit "No fields to update" service guard
13. Cross-user PUT returns 400
DELETE /workflows/:id (2):
14. Owner can delete + subsequent GET returns 400 (workflow gone)
15. Cross-user DELETE returns 400
Verified locally: 15 tests green (489ms test runtime), tsc --noEmit
clean.
T22 ticked. **Phase 2 COMPLETE** — T18/T19/T21/T22 done; T20 was N/A.
Phase 2 added 42 tests across 3 new spec files (gallery-map, view,
workflow). Total branch coverage: 247 e2e tests (Phase 1 = 205 + Phase
2 = 42).
* test(e2e): apply post-Phase-2 review nits
Three NITs from the Phase 2 review, all addressed:
1. **gallery-map T19 — missing personIds re-routing test.** The
service at shared-space.service.ts:569-570 re-routes
`dto.personIds → spacePersonIds` when `spaceId` is set, making
the same DTO field mean different things in different contexts.
Added a 7th test in the T19 nested describe that probes the
re-routing: passing a bogus person UUID with spaceId returns
empty (the lookup goes through shared_space_person, not asset_face).
Not the strongest possible assertion, but pins the behavioural
shape so a future refactor that changes the re-routing direction
would be caught.
2. **view T21 — dead sanity-check test.** The original spec had
`expect(typeof userBAssetId).toBe('string')` only to satisfy
the linter about the unused `userBAssetId` fixture. Removed both
the variable assignment and the dead test. The cross-user-isolation
tests already exercise userB by querying with their token; we
don't need their asset id specifically. Drop both, keep the
second createAsset call so userB has folder content.
3. **gallery-map backlog row count.** T19 said "6 tests" but the
actual count is 7 with the new re-routing test. Updated the row.
Verified locally: 27 tests green across gallery-map (19) + view (8),
tsc --noEmit clean.
* test(e2e): T23 asset metadata K/V CRUD
Adds e2e/src/specs/server/api/asset-metadata.e2e-spec.ts with 13
tests covering the asset metadata K/V endpoints (PUT /:id/metadata,
GET /:id/metadata, GET/DELETE /:id/metadata/:key, bulk PUT/DELETE
/assets/metadata).
All routes use Permission.AssetRead/AssetUpdate which routes through
requireAccess (bulk-access pattern → 400 for non-owner).
Tests:
GET /assets/:id/metadata (3):
1. Auth required (anon → 401)
2. Owner can list (empty initially)
3. Non-owner returns 400 (bulk-access)
PUT /assets/:id/metadata (3):
4. Owner upsert + value is queryable via the listing
5. Upsert overwrites an existing key value (set v=1, set v=2,
verify single-key fetch returns v=2)
6. Non-owner upsert returns 400
GET /assets/:id/metadata/:key (2):
7. Owner can fetch a single key set in test 4
8. Missing key returns 400 or 404 (test pins both — the actual
path uses requireAccess + service throw, behavior pinned)
DELETE /assets/:id/metadata/:key (2):
9. Owner can delete a key and it's removed from the listing
10. Non-owner delete returns 400
PUT /assets/metadata (bulk) (2):
11. Owner can upsert across multiple of their own assets in one call
12. Bulk upsert with a non-owner asset id mixed in is rejected (400)
DELETE /assets/metadata (bulk) (1):
13. Owner can bulk-delete keys across multiple of their own assets
Verified locally: 13 tests green (554ms test runtime), tsc --noEmit
clean.
T23 ticked. Phase 3: 1/6 done.
* test(e2e): T24 asset OCR endpoint access mat…
Deeds67
added a commit
to open-noodle/gallery
that referenced
this pull request
Apr 11, 2026
* docs(plans): research e2e API test coverage gaps and backlog
Catalogues the 282-endpoint server surface against the 29 existing
e2e API specs, identifies the 8 zero-coverage controllers and the
endpoint holes inside heavily-tested specs (shared-space /people and
/libraries, asset metadata/edits/copy, library->space link side
effects). Defines a reusable Permission/Actor matrix and proposes
~14 follow-up PRs prioritised by bug-catching ROI.
* docs(plans): break e2e API coverage research into per-task backlog
Adds a working backlog of 39 PR-sized tasks (T01–T39) derived from the
e2e API coverage research doc, plus full design docs for the three
upfront tasks: helpers (T02), timeline access matrix (T03), and the
shared-space people listing (T09).
The backlog records architectural decisions (helper API shape, fixture
lifetime, extending utils.createSpacePerson over duplication) separately
from observed server invariants (400 vs 403 split between requireAccess
and requireMembership, the listing's thumbnailPath gate, the metadata vs
thumbnail role split on PATCH /shared-spaces). Each task row hard-pins
its dependencies so most of Phase 1+ can run in parallel after T02.
Four code-reviewer passes caught and fixed ~15 substantive issues across
the drafts; the docs as committed here are the result.
* test(e2e): T01 move stray duplicate spec into the discovered glob
The vitest config at e2e/vitest.config.ts uses
`src/specs/server/**/*.e2e-spec.ts` for spec discovery. The file at
e2e/src/api/specs/duplicate.e2e-spec.ts was outside that glob and has
never been running in CI — it was added during the duplicate-detection
work but landed in the wrong directory.
Move it to e2e/src/specs/server/api/duplicate.e2e-spec.ts (where every
other server API spec lives), remove the now-empty src/api/specs and
src/api directories, and tick T01 in the coverage backlog.
The companion audit of waitForQueueFinish vs expect.poll for non-admin
specs (per the e2e-coverage research doc and the
feedback_e2e_admin_only_queues memory) found zero violations: all 38
existing callers correctly pass admin.accessToken. Result documented
inline in the backlog row.
Imports in the moved file use the `src/*` path alias, so file depth is
irrelevant — no code changes were needed beyond `git mv`.
* test(e2e): T02 add Actor / SpaceContext / forEachActor helpers
Adds e2e/src/actors.ts with Actor + ActorId + SpaceContext types and
the buildSpaceContext / forEachActor helpers, and extends the existing
utils.createSpacePerson to insert the shared_space_person_face junction
row, accept a type parameter, and return {globalPersonId, spacePersonId,
faceId} instead of just the space person ID.
These exist to turn the Permission/Actor matrix from §3 of the e2e
coverage research doc into a one-liner per endpoint, so downstream
specs (T03+) can write `forEachActor(...)` instead of hand-rolling six
describe blocks per endpoint. See docs/plans/2026-04-06-e2e-T02-helpers-design.md
for the rationale and decision log.
Key design points (all captured in the design doc):
- buildSpaceContext composes utils.adminSetup / userSetup / createSpace /
addSpaceMember / addSpaceAssets / createAsset — no parallel
implementations of any existing helper.
- forEachActor throws Error (not expect.toBe) so the failure message
names the actor that failed; without that, debugging an actor matrix
is needlessly painful.
- Sequential, not parallel — tests share a database and parallel actor
runs would race on the same fixtures.
- Fixture lifetime contract: ctx is read-only in beforeAll; mutating
tests own their cleanup via try/finally or nested describes.
- ActorId starts minimal (8 actors). partner / libraryOwner / apiKey* /
sharedLink land with their first consumer task.
Smoke tests in e2e/src/specs/server/api/_helpers.e2e-spec.ts validate
all three behaviours that downstream PRs depend on:
1. Auth threading — bearer token reaches the server for every actor.
2. Anon split — /users/me requires auth (anon: 401, members: 200).
3. createSpacePerson extension — returns three IDs and inserts the
shared_space_person_face junction row (verified via direct DB query).
4. Role assignment — PATCH /shared-spaces/:id with {thumbnailCropY: 0}
distinguishes Owner/Editor from Viewer. Uses thumbnailCropY (Editor-
level) rather than name (Owner-level per shared-space.service.ts:197-203)
so Editor and Viewer get distinct status codes.
Implemented test-first: each smoke test was written and run failing
before the corresponding helper code was added.
Verified locally against the e2e stack: all 4 smoke tests green
(537ms total), server.e2e-spec.ts unchanged (24/24 still pass), tsc
--noEmit clean. createSpacePerson currently has zero callers in e2e/
(verified via grep) so the signature change is risk-free.
T01 ticked in the backlog along with T02. The cleanup audit found zero
non-admin waitForQueueFinish callers — the rule from the
feedback_e2e_admin_only_queues memory is currently held everywhere.
* test(e2e): T03 add timeline /buckets and /bucket access matrix
Adds e2e/src/specs/server/api/timeline.e2e-spec.ts with 9 tests
covering the access matrix for both timeline endpoints. This is the
first real consumer of the actor / forEachActor helpers from T02.
Tests on GET /timeline/buckets:
1. Auth required (anon: 401, owner: 200)
2. Owner sees own assets without filter (count == 2)
3. spaceId access matrix — status codes for owner/editor/viewer/non-member/anon.
Non-member returns **400** (not 403), pinned because timeline uses requireAccess
which throws BadRequestException (src/utils/access.ts:37-42), distinct from the
shared-space-family endpoints which use requireMembership and return 403.
4. spaceId scopes assets to the space, not the requesting user. spaceOwner with
spaceId sees the 1 space asset, NOT the 2 they own. Catches the bug shape where
the implementation would `WHERE asset.ownerId = auth.user.id` instead of joining
through shared_space_asset.
5. Non-owner space members (editor, viewer) actually see space content via spaceId.
This is the PR #163 / #202 bug shape — it returned 200 with empty body, so pure
status-code testing would have missed it.
Tests on GET /timeline/bucket (singular):
6. Auth required.
7. spaceId access matrix mirroring test 3 — same shape, distinct endpoint. The
risk being probed: forgetting to apply the same scoping check on the singular
endpoint. PR #260 is in this family.
8. Non-owner space members see the space asset via /bucket. Same bug class as
test 5, applied to the asset-list endpoint. Asserts the actual asset ID is in
the response, not just the status code.
9. /bucket returns the parallel-array TimeBucketAssetResponseDto shape, not
bucket counts. Sanity check that the two endpoints aren't confused.
Coverage matches the 10 tests enumerated in the T03 design doc; design tests 3
and 6 are merged into one access-matrix test (the matrix already pins
spaceNonMember: 400, so a separate test would assert the same thing).
Verified locally against the e2e stack: 9 tests green (580ms total). Combined
with the 4 helper smoke tests, total: 13 tests, 1.06s. tsc --noEmit clean.
T03 ticked in the backlog.
* test(e2e): apply post-T03 review nits
Three nits from the post-T01-T03 implementation review:
1. Add `authHeaders(actor)` helper to actors.ts and use it in
_helpers.e2e-spec.ts and timeline.e2e-spec.ts. The previous
`actor.token ? asBearerAuth(actor.token) : {}` pattern at every
forEachActor call site was unprecedented in the e2e suite —
localizing the conditional inside actors.ts keeps the call sites
consistent with idiomatic supertest usage.
2. Drop the redundant `utils.initSdk()` calls in the new spec files'
beforeAll. utils.ts:809 already invokes it at module load, and no
other spec under specs/server/api calls it explicitly.
3. Drop the try/finally + utils.disconnectDatabase() in
_helpers.e2e-spec.ts smoke test 3. Existing specs that use the
raw pg client (shared-space.e2e-spec.ts, etc.) don't disconnect —
they rely on worker-process exit. Disconnecting mid-spec would
break any later test in the file that uses utils.createSpacePerson
or any other client-using helper. Add an inline comment explaining
why.
`asBearerAuth(actor.token!)` is left in place at non-forEachActor
call sites where the actor is always authenticated — that's the
existing convention across the e2e suite.
Verified locally: 13 tests still green (4 helpers + 9 timeline,
1.03s), tsc --noEmit clean.
* test(e2e): T04 add timeline withSharedSpaces / withPartners semantics
Extends actors.ts and timeline.e2e-spec.ts to cover the two flags
that gate cross-user content on the timeline endpoints.
Helper changes (actors.ts):
- Add `partner` to ActorId.
- Add optional `partner` and `partnerAssetId` fields to SpaceContext.
- Add `BuildSpaceContextOptions` with `withPartner?: boolean`. When
set, buildSpaceContext creates an extra user, has them share their
library with spaceOwner, and uploads one asset for them.
- Add `addPartner({token, userId}, {token, userId})` helper. The
default `partner.inTimeline` column is **false** (verified at
server/src/schema/tables/partner.table.ts:46), so a fresh
`createPartner` call is invisible to `withPartners=true` until the
recipient enables it. The helper auto-enables inTimeline by having
the recipient call PUT /partners/:id, so test call sites don't have
to remember the two-step dance. This default-false behaviour is
exactly the kind of footgun an integration test would have caught
if anyone had ever written one before today.
New tests in timeline.e2e-spec.ts (`describe('GET /timeline/buckets — withSharedSpaces and withPartners')`):
1. withSharedSpaces=true makes a non-owner member see space content
on their own timeline. spaceEditor owns 1 asset (editorAssetId);
with withSharedSpaces, the union picks up spaceAssetId via the
membership default of showInTimeline=true, so total goes 1 -> 2.
2. Toggling showInTimeline=false drops the space out of
getSpaceIdsForTimeline. PATCH /shared-spaces/:id/members/me/timeline
with {showInTimeline: false}; verify total goes back to 1; restore
in try/finally per the fixture lifetime contract.
3. withPartners=true makes spaceOwner see partner-shared assets.
Total = 2 own + 1 partner = 3.
4. Default (no withPartners) excludes partner assets. Total = 2.
5. Combining withSharedSpaces and withPartners doesn't double-count —
spaceAssetId is already counted in spaceOwner's own 2, so the union
stays at 3.
All calls pass `visibility=timeline` explicitly because timeline.service.ts:91-113
treats `visibility === undefined` as `requestedArchived = true` and throws 400 when
either flag is set. This invariant is now pinned in the backlog.
Backlog updates:
- T04 row ticked (5 tests).
- New "Observed invariants" entries: partner.inTimeline default-false,
withSharedSpaces/withPartners visibility requirement.
Library-linked space asset visibility (mentioned in the original T04
design) is deferred to T17, which is the right home — it requires the
linkLibrary helper that doesn't exist yet.
Verified locally: 18 tests green (4 helpers + 14 timeline, 1.11s),
tsc --noEmit clean.
* test(e2e): T05 add timeline visibility filter tests
Adds 7 new tests to timeline.e2e-spec.ts: 5 for the visibility filter
behaviour itself, plus 2 invariant pins folded in from the T04 review
NIT (visibility-undefined-throws-400).
The visibility tests use a dedicated user (visibilityUser) with 4
assets in different states: timeline, archive, hidden, and trashed.
A separate user keeps the assertions deterministic — using spaceOwner
would have to subtract the existing space-related assets from every
expected count.
Tests in `describe('GET /timeline/buckets — visibility filters')`:
1. **default visibility (no param) returns Timeline AND Archive (2)** —
pins the non-obvious server behaviour at server/src/utils/database.ts:79-81.
`withDefaultVisibility` is `where('asset.visibility', 'in', [Archive, Timeline])`,
NOT just Timeline. The web UI's main timeline view must pass
`visibility=timeline` explicitly to exclude archived assets. This was a
real surprise during implementation — the first version of the test
expected count=1 and failed with count=2 against the live server.
2. visibility=timeline returns only the strict timeline assets (1).
3. visibility=archive returns only archived assets (1).
4. visibility=hidden returns only hidden assets (1). Hidden visibility is
normally used for the video part of live photos / motion photos
(per the AssetVisibility enum docstring); pinned here to protect that
path from a future refactor.
5. trashed assets are excluded regardless of visibility filter. Asserts
the count under both default and explicit visibility=timeline so a
trash regression would inflate either independently.
Tests appended to the withSharedSpaces/withPartners describe (folded
from T04 review NIT):
6. withSharedSpaces=true without explicit visibility returns 400.
7. withPartners=true without explicit visibility returns 400.
Backlog updates:
- T05 ticked.
- New "Observed invariant" entry: default visibility filter is
permissive (Timeline + Archive) — this is exactly the kind of fact
that bites every author who hasn't read database.ts.
Verified locally: 21 tests green (4 helpers + 17 timeline, 812ms),
tsc --noEmit clean.
* test(e2e): apply post-T05 review nits
Three nits from the post-T05 implementation review:
1. Visibility-400 invariant tests now also assert on the error message
(`/withSharedSpaces/` and `/withPartners/`). Without that, a future
unrelated 400 (e.g. a DTO validation change) would silently still
satisfy the test.
2. Renamed test 5 to "soft-deleted (trashed) assets are excluded
regardless of visibility filter". Both soft-delete and force-delete
set `deletedAt`, so the test characterises the deletedAt-based
exclusion which is what the timeline query actually depends on.
Comment now spells this out.
3. Refactored the `Promise.all` 4-element destructure into a parallel
block of 3 + a sequential trash creation. The previous
`const [, , , trashedAsset] = await Promise.all([...])` was uncommon
in the e2e suite. Pulling trashed out of the parallel block is
slightly slower (4 sequential RTTs vs 3 parallel + 1) but more
readable, and the cost is negligible at fixture-setup time.
Verified locally: 21 tests still green, tsc --noEmit clean.
* test(e2e): T06 add timeline filter passthrough tests with spaceId
Adds 5 tests to timeline.e2e-spec.ts in a new describe block, probing
how the various filter parameters (`spacePersonId`, `personId`, `tagIds`)
interact with `spaceId` scoping. This is the PR #260 bug shape pin —
the fork has a *dedicated* `spacePersonId` DTO field separate from
`personId`, and the original PR #260 bug was matching a global `personId`
against a `shared_space_person.id`.
Setup creates two new fixtures attached to ctx (without mutating any
existing state):
- `spacePerson` via utils.createSpacePerson — adds Alice as a face on
spaceAssetId, with the shared_space_person_face junction row that
the timeline filter joins through.
- `spaceTagId` — a tag owned by spaceOwner, applied to spaceAssetId.
Tests:
1. **spacePersonId + spaceId** restricts to assets containing that
space person. Joins through shared_space_person_face → asset_face
→ asset; only spaceAssetId qualifies, total=1.
2. **GLOBAL personId + spaceId** does not cross-pollute. Pinned at
total=1 (which here is *legitimate* because the global person row
is the same one createSpacePerson set up under the hood — not a
bug). The test exists so a future change that decouples the two
joins or breaks the spaceId restriction is caught.
3. **spacePersonId without spaceId** falls back to owner-scoped and
still filters correctly (total=1, just spaceAssetId).
4. **tagIds + spaceId** returns the tagged subset of space content.
5. **non-owner space member sees tagged space content via
spaceId+tagIds** — spaceEditor querying with spaceOwner's tag
returns the tagged space asset. Pins that the timeline tag filter
doesn't enforce per-user tag ownership for space-scoped queries,
which is the expected UX for shared spaces (one member labels a
photo, others see the label).
Coverage matches 5 of the ~8 tests in the original T06 design. EXIF
filter passthrough (country/make/rating) is deferred because it requires
fixture images with extracted metadata (the e2e suite uses generated
PNGs for most tests). When a follow-up wants EXIF coverage it can be
appended to this describe block.
Verified locally: 26 tests green (4 helpers + 22 timeline, 868ms),
tsc --noEmit clean.
T06 ticked. All four Phase 1 timeline subtasks (T03–T06) are now done.
* test(e2e): apply post-T06 review nits
The post-T06 review caught a real coverage gap: the original test 2
("global personId does NOT cross-pollute") asserted total=1, which
was a coincidence — the global person from createSpacePerson is
legitimately attached to spaceAssetId via asset_face, so the join
hits it whether or not the spaceId restriction is working. A
regression that removed the spaceId scoping entirely would still
return 1 and the test would pass.
To actually pin the boundary:
1. beforeAll now also creates a decoy global person ("Decoy Bob")
attached only to ownerAssetId (NOT in the space). No
shared_space_person row, no junction.
2. Test 2 is now the load-bearing boundary test: query with
personId=<decoy>&spaceId=<spaceId> and assert total=0. The
decoy's asset is NOT in the space; if the spaceId scoping is
working, the join must not return it. If a future regression
breaks the scoping, this test goes from 0 to 1.
3. Added a new test 3 for the other half of the boundary: querying
with personId=<spacePerson.globalPersonId>&spaceId=<spaceId> still
returns 1 because that global person IS attached to a space asset
via asset_face. Together, tests 2 and 3 pin both sides of the
spaceId boundary on the global personId join path.
Plus the IMPORTANT and NIT comment improvements:
- Test "spacePersonId without spaceId" comment now spells out that
the join is NOT spaceId-restricted — a future test that puts the
same spacePersonId on a second space's asset would observe
count > 1.
- Test "non-owner space member sees tagged content" comment now
spells out the actual invariant being pinned: hasTags has zero
per-user check, tag IDs are universally addressable on the
timeline filter. The shared-spaces UX consequence (one member
labels, all members can filter) is documented as the load-bearing
property — a future refactor that adds an owner check to hasTags
would silently break the UX unless this test catches it.
Verified locally: 27 tests green (4 helpers + 23 timeline, 914ms),
tsc --noEmit clean.
* test(e2e): T07 add face CRUD access matrix
Adds e2e/src/specs/server/api/face.e2e-spec.ts with 10 tests covering
the four /faces endpoints. Also extends utils.createFace to return the
inserted face id (was Promise<void>) so PUT/DELETE tests can address
specific faces — backwards-compatible because all existing callers
ignore the return value.
Tests:
POST /faces (3):
1. Access matrix on the asset side. Owner can; spaceNonMember 400;
anon 401. Same Immich-wide bulk-access pattern as timeline (400
not 403 for non-owner).
2. Cross-owner asset rejected even when the person belongs to caller.
3. Cross-owner person rejected even when the asset belongs to caller.
GET /faces (2):
4. Access matrix on the asset side.
5. Owner gets the face row back with the linked person populated.
Asserts presence of the specific face we inserted (not a count) to
stay robust against unrelated faces accumulating on the asset.
PUT /faces/:personId — reassign (2):
6. Access matrix when reassigning a face to a new person owned by
the same user (Alice → Anne).
7. Reassigning to a cross-owner target person is rejected — the
person-access check on the target fires before any state mutation.
DELETE /faces/:faceId (3):
8. Owner can soft-delete (force=false).
9. Owner can force-delete (force=true).
10. Access matrix for non-owner / anon. Each test creates its own
scratch face so the access matrix doesn't permanently mutate
state.
The non-obvious API shapes are documented in the spec file header
and pinned in the backlog "Observed invariants" section:
- POST /faces returns void (no face id)
- PUT /faces/:id has path=target-person, body=face — the FaceDto is
reused with different meanings on different endpoints
- GET /faces takes ?id=<assetId> (the FaceDto field is named `id`
but represents the asset)
The first attempt at the spec hit four cascading failures from my
incorrect API understanding (POST returning void, PUT semantics
backwards, count assertion fragile to other tests). The corrected
version pins the actual server behaviour and is robust to test
isolation.
Verified locally: 41 tests green across 3 spec files
(4 helpers + 27 timeline + 10 faces, 1.82s test runtime),
tsc --noEmit clean.
T07 ticked.
* test(e2e): T08 add face deletion side effect tests + post-T07 nit fix
Adds 6 tests to face.e2e-spec.ts in a new "face deletion side effects (T08)"
describe block, plus a one-line comment fix from the post-T07 review.
T08 tests:
1. Soft-deleted face is excluded from GET /faces?id=<assetId> via the
`asset_face.deletedAt IS NULL` filter (person.repository.ts:229).
2. Hard-deleted face is excluded from GET (deleted row, same observable
result).
3. Soft-deleting the only face on a person preserves the person row.
Global persons are NOT cascade-deleted when their last face goes away;
GET /people/:id still returns the person. This matters for the
shared-spaces UX where the person row outlives any individual face
attachment.
4. Soft-deleting a face decreases the person's getStatistics asset count.
getStatistics joins through asset_face filtering on `deletedAt IS NULL`
AND `isVisible IS true` (person.repository.ts:335-352), so soft-delete
drops the row out of the count.
5. Hard-delete decreases the count via the same mechanism (row removal).
6. Re-attaching the same (assetId, personId) after a soft-delete inserts
a NEW asset_face row. There's no UNIQUE constraint blocking this;
pinned so a future schema change is caught.
Two scope adjustments documented in the backlog row:
- "Below-minFaces faces unaddressable" was a *space-person* concern from
PR #139 (space person thumbnail 404s), not a global face concern.
Moved to T10/T11.
- "Space-person dedup queue jobId dedup" from PR #292 requires probing
queue state that isn't exposed via the face controller. Moved to T14
which owns the deduplicate endpoint.
Post-T07 review NIT fix:
- POST /faces access matrix comment said "write access to BOTH the asset
and the person", but person.service.ts:641-642 actually uses
`Permission.AssetRead` + `Permission.PersonRead` (not write). Comment
reworded.
Verified locally: 47 tests green across 3 spec files (4 helpers + 27
timeline + 16 faces, 1.88s test runtime), tsc --noEmit clean.
T08 ticked. Faces group complete (T07 + T08).
* test(e2e): apply post-T08 review nit — strengthen the re-attach test
Test 6 ("re-attaching a face after a soft-delete") originally asserted
both faces on the same asset, which made the stats `count(distinct
asset.id)` = 1 with or without the deletedAt filter — the assertion
passed by accident, not because of the filter.
Strengthen by:
1. Putting the soft-deleted face on assetA and the new face on assetB.
The stats count now actually distinguishes the two semantics:
- With deletedAt filter (correct): 1 (only assetB)
- Without it (broken): 2 (assetA + assetB)
2. Adding a third face on assetA (re-attach with the existing
soft-deleted row in place). Asserts no UNIQUE constraint blocks the
second insert AND that count goes 1 → 2 to confirm the new face is
counted.
The "no UNIQUE constraint" pin is preserved; the deletedAt filter is
now genuinely exercised by the count assertion.
Verified locally: 16 tests still green, tsc --noEmit clean.
* test(e2e): T09 add shared-space people listing tests
Adds an 11-test describe block to shared-space.e2e-spec.ts covering
the GET /shared-spaces/:id/people endpoint per the T09 design doc
(docs/plans/2026-04-06-e2e-T09-space-people-listing-design.md).
Setup: dedicated owner/editor/viewer/non-member users with their
own space and 5 space-people via the extended utils.createSpacePerson
helper from T02 (which inserts the four-table chain including the
shared_space_person_face junction).
Tests:
1. Access matrix — owner/editor/viewer 200, non-member 403, anon 401.
shared-space endpoints use requireMembership → ForbiddenException,
distinct from timeline's requireAccess → BadRequestException.
2. Listing returns space person IDs (NOT global person IDs). The
canonical assertion for the whole T09–T14 sub-tree.
3. Hidden persons excluded by default.
4. ?withHidden=true includes hidden persons.
5. Unnamed persons included by default.
6. ?named=true returns only persons with non-empty names (on either
shared_space_person.name OR person.name per the OR clause in
shared-space.repository.ts:514-521).
7. Toggling shared_space.petsEnabled=false hides pets, restored in
try/finally per the fixture lifetime contract.
Pagination tests in a nested describe (own beforeAll/afterAll for the
15 extra rows so they don't leak into sibling tests):
8. ?limit=10 caps the response.
9. ?offset paginates without overlap.
10. Sort order is stable across calls.
Final test in the parent describe:
11. Empty thumbnailPath on the underlying global person excludes the
space person from the listing. Pins the fork's "minFaces gate"
mechanism (shared-space.repository.ts:512-513) — pinned now so a
future query refactor that drops the filter would be caught.
Mutates via direct DB and restores in try/finally.
All assertions match the design doc decisions in the backlog
"Decision log" section: space person ID is canonical, stable sort,
extended createSpacePerson is the helper, listing query params are
limit/offset/withHidden/named/takenAfter/takenBefore (no `top`, no
text-based name search).
Backlog updates:
- T09 ticked.
- New "Known flaky-spec footgun" section: utils.createPerson +
Promise.all is unsafe with the shared pg.Client — observed once
as an FK violation in T09 setup, didn't reproduce in 3 follow-up
runs. Latent since T07. Not blocking; documented for follow-up.
Verified locally: 110 tests green in shared-space.e2e-spec.ts
(99 existing + 11 new T09), 157 tests across 4 spec files including
helpers/timeline/face/shared-space (4 + 27 + 16 + 110), 6.78s test
runtime, tsc --noEmit clean.
* test(e2e): apply post-T09 review nits
The post-T09 review caught one IMPORTANT and two NIT issues — all
fixed in this commit.
1. **IMPORTANT — missing afterAll in pagination describe.** The T09
commit message claimed "own beforeAll/afterAll" but the
implementation only had beforeAll. The 15 extra space-people rows
would leak into test 11 (the thumbnailPath gate) and any future
sibling test added to the parent describe. T11 still passes (it
asserts NOT contain), but the leak violates the T02 fixture
lifetime contract and would bite T10+. Added afterAll with
`DELETE FROM shared_space_person WHERE id = ANY($1::uuid[])` over
the captured ids.
2. **NIT — test 11 used a JOIN query for globalPersonId.** The
extended createSpacePerson helper from T02 already returns
`{globalPersonId, spacePersonId, faceId}` — no need to re-query
the database. Stored zeroThumbGlobalId in the describe scope and
dropped the 4-line JOIN.
3. **NIT — test 1 used a manual for-loop instead of forEachActor.**
The design doc explicitly called for forEachActor. The manual loop
worked but diverged from the T03+ pattern. Switched to forEachActor
with proper Actor objects, importing from src/actors. This also
sets the precedent for T10-T14 to use the helper consistently.
Verified locally: 110 tests still green in shared-space.e2e-spec.ts,
tsc --noEmit clean.
* test(e2e): T10 single space-person + thumbnail + assets
Adds 9 tests in a new nested describe inside T09's parent block,
sharing the T09 fixture setup. Covers the three read-only sub-endpoints:
- GET /shared-spaces/:id/people/:personId
- GET /shared-spaces/:id/people/:personId/thumbnail
- GET /shared-spaces/:id/people/:personId/assets
GET /people/:personId (5 tests):
1. Access matrix (owner/editor/viewer 200, non-member 403, anon 401).
2. Returns the canonical space person ID and name.
3. Hidden person IS fetchable directly — confirms half of the T09
open hypothesis: hidden filter is listing-only.
4. **Pet person is NOT fetchable directly when petsEnabled=false** —
DISPROVES the other half of the T09 hypothesis. The pet filter
applies BOTH to the listing and to the single-fetch endpoint.
This asymmetry vs hidden is real and worth pinning. UX
consequence: turning pets off in a space hides the entire pet
sub-graph, even from members who know the pet's ID.
5. **Non-existent personId returns 400 (not 404)** — bulk-access
pattern via requireAccess uniformly returns BadRequestException
for "not found OR no access" to avoid leaking existence. Same
taxonomic split as timeline. T11+ inherit this convention.
GET /people/:personId/thumbnail (2 tests):
6. Access matrix. **Member success-case is 500**, not 200, because
the fixture thumbnailPath ('/my/awesome/thumbnail.jpg' set by
utils.createSpacePerson) doesn't exist on disk. The access check
passes; the file resolution then fails and returns 500. Pinned
as a known footgun in the backlog — fixable later but out of
scope for T10. The 500 vs 404 distinction is a small server-side
bug independent of the access path.
7. Non-member 403 fires before file resolution (sanity check that
the 500 path isn't somehow leaking access).
GET /people/:personId/assets (2 tests):
8. Access matrix.
9. Returns the asset IDs containing the person — Alice is on
spaceAssetId so the response contains it.
Backlog updates:
- T10 ticked.
- The "open hypothesis" about hidden/pets at listing only is moved
to a new "Resolved hypotheses" section with the asymmetric finding
documented.
- New "Observed invariant": pet filter asymmetry (listing AND single
fetch) vs hidden filter (listing only).
- New "Observed invariant": single-person endpoints return 400 for
unknown IDs (bulk-access pattern), not 404.
- New "Known footguns" section: the thumbnail-500 issue.
Verified locally: 119 tests green in shared-space.e2e-spec.ts
(110 from T09 + 9 new T10), tsc --noEmit clean.
* test(e2e): apply post-T10 review fixes — service mechanism + thumbnail strategy
Two IMPORTANT findings from the post-T10 review, both about explanation
correctness rather than test logic.
1. **Backlog "bulk-access pattern via requireAccess" claim was wrong.**
`getSpacePerson` at shared-space.service.ts:625-636 calls
`requireMembership` (ForbiddenException for non-member, that part was
right), then runs `getPersonById` and manually
`throw new BadRequestException('Person not found')` for both the
missing-person case AND the pet-when-disabled case. The 400s the
T10 tests observe are real and load-bearing, but they come from
manual throws inside the service handler, NOT from the
`requireAccess` bulk pattern that timeline uses. Backlog
"Observed invariants" rewritten to cite the correct mechanism with
line references.
2. **Thumbnail 500 was a fixture wart, not a server bug.**
`getSpacePersonThumbnail` at shared-space.service.ts:643-657 has
THREE return paths once the access check passes:
- person not found / wrong space → NotFoundException → 404
- thumbnailPath null/empty → NotFoundException → 404
- thumbnailPath set → serveFromBackend → 200 (or 500 if missing)
The 500 in the previous T10 commit was triggered because
utils.createSpacePerson uses '/my/awesome/thumbnail.jpg' (a
non-empty path that doesn't exist on disk), which trips
serveFromBackend. The service actually has graceful 404 handling.
Restructured the thumbnail tests to exercise the **graceful 404
path**: transiently blank `person.thumbnailPath` via DB, assert
`{member: 404, non-member: 403, anon: 401}`. This pins the layered
ordering 401 < 403 < 404 — the correct member-success path for a
person with no thumbnail. Restore in try/finally.
The 200 path is not exercised because it would require pointing
the fixture at a real file in the upload location. That's a
reasonable follow-up but out of scope here.
Merged the two thumbnail tests into one (the matrix already
covers the access ordering, the second test was redundant). T10
is now 8 tests in the file (was 9), all assertions correct.
Backlog updates:
- Two "Observed invariants" rewritten to cite the manual
BadRequestException mechanism with the correct line numbers.
- New "Observed invariant" describing the three return paths of
getSpacePersonThumbnail.
- "Known footguns" entry rewritten: it's a fixture issue, not a
server bug. Mentions the follow-up to make createSpacePerson
accept a real fixture file.
Verified locally: 118 tests green in shared-space.e2e-spec.ts
(110 from T09 + 8 from T10), tsc --noEmit clean.
* test(e2e): T11 PUT/DELETE space person — rename, hide, delete
Adds 7 tests in a new nested describe inside the T09/T10 block,
covering mutation of a single space person via PUT and DELETE.
Both endpoints route through `requireRole(SharedSpaceRole.Editor)`
(verified at shared-space.service.ts:665, 704), so:
- Owner + Editor can mutate (200/204)
- Viewer is rejected (403)
- non-member is rejected (403, via requireMembership inside requireRole)
- anon (401, auth middleware)
PUT tests (4):
1. Access matrix for rename — owner+editor 200, viewer/non-member 403,
anon 401.
2. Actually renames the person — sends `{name: 'AfterRename'}` and
asserts the response body reflects the new name.
3. Marking isHidden=true hides the person from the default listing
but the direct fetch still returns 200 — pairs with T10's listing-only
hidden invariant. The PUT path is the supported way to set isHidden,
complementing T09's direct-DB-mutation pattern.
4. Non-existent personId returns 400 (manual BadRequestException at
shared-space.service.ts:668-669, same shape as T10).
DELETE tests (3):
5. Access matrix — owner+editor 204, viewer/non-member 403, anon 401.
Uses 5 different scratch persons (one per actor) to avoid
"delete-then-delete" race conditions in the matrix.
6. Preserves the underlying global person row — verifies via direct
DB query that `person` table row stays after `shared_space_person`
delete. The shared_space_person delete is correctly scoped and
does NOT cascade to the global person table.
7. Non-existent personId returns 400.
All scratch persons are created fresh per `it()` block via
utils.createSpacePerson, so the mutations are fully isolated and
don't affect T09's listing assertions or T10's read-only fixtures.
Verified locally: 125 tests green in shared-space.e2e-spec.ts
(110 from T09 + 8 from T10 + 7 from T11, 5.15s test runtime),
tsc --noEmit clean.
T11 ticked. Space-people sub-tree progress: 3/6 (T09/T10/T11 done,
T12 merge / T13 alias / T14 deduplicate remaining).
* test(e2e): T12 POST merge space persons
Adds 6 tests in a new nested describe inside the T09/T10/T11 block,
covering POST /shared-spaces/:id/people/:personId/merge.
Service shape (shared-space.service.ts:730-778): path :personId is the
*target*, body `{ids: string[]}` lists the *sources*. Requires Editor.
Validates both sides in the same space and the same type, reassigns
the source's junction rows to the target, deletes the source rows,
recounts the target's denormalised faceCount/assetCount, and queues
a dedup pass.
Tests:
1. Access matrix — owner+editor 204, viewer 403, non-member 403, anon 401.
Uses 5 separate scratch source persons (one per actor) so the
matrix doesn't try to merge the same source twice.
2. Merge reassigns the source's junction rows and deletes the source.
Verified via direct DB queries: source row gone, target now has 2
junction rows (its own face + the inherited one).
3. After merge, target's denormalised faceCount=2 and assetCount=1
(both faces are on the same asset, so distinct asset count is 1).
Pins recountPersons (shared-space.repository.ts:686+).
4. Cannot merge a person into themselves — 400 with message
matching /themselves/ from shared-space.service.ts:743-745.
5. Cannot merge across types (person target + pet source) — 400
with message matching /different types/ from line 754-756.
Pins the type-segregation invariant: pets and persons stay
separate even within the same space.
6. Missing target OR missing source returns 400. Two requests in
one test, both pinned.
Each test creates fresh scratch persons via utils.createSpacePerson
inside the `it()` block — fully isolated from T09/T10/T11 fixtures
and from sibling T12 tests.
Verified locally: 131 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12, 6.20s test runtime),
tsc --noEmit clean.
T12 ticked. Space-people sub-tree: 4/6 done (T09/T10/T11/T12). T13
alias and T14 deduplicate remaining.
* test(e2e): T13 space person alias — per-user, viewer-allowed
Adds 5 tests in a new nested describe inside the T09/T10/T11/T12 block,
covering PUT and DELETE alias.
Three critical invariants pinned by this block — the original T13
backlog row was wrong about both:
1. **Aliases are PER-USER, not visible to all members.** The service
stores `(personId, userId, alias)` and `getAlias(personId, auth.user.id)`
returns only the caller's row (shared-space.service.ts:780-798).
Owner setting "Mom" as Alice's alias is invisible to editor and
viewer. The original backlog row claimed "visible to all members"
which is the opposite of reality.
2. **Aliases require `requireMembership`, NOT `requireRole(Editor)`.**
Viewers CAN set their own aliases. Logical: aliases are personal
metadata, not space state — a read-only viewer should still be able
to label people for themselves.
3. **DELETE has no person existence check; it's idempotent on missing
personId.** Asymmetric vs PUT (which validates and returns 400).
Service code at lines 800-803.
Tests:
1. Access matrix — owner+editor+viewer 204 (all members can set their
own alias), non-member 403, anon 401.
2. Per-user persistence + isolation — owner sets "Mom" alias, owner GET
sees "Mom", editor GET sees null alias and the original 'PerUserAlice'
name. Pins both halves of the per-user invariant.
3. Alias does NOT modify global `person.name` — direct DB query
confirms the underlying `person.name` row stays at the original
value after alias is set.
4. DELETE removes the alias AND is idempotent on missing personId
(single test asserts both) — covers the asymmetry vs PUT.
5. PUT alias on missing personId returns 400 — pinned for symmetry
with T11/T12's missing-personId convention.
Backlog updates:
- T13 ticked with the corrected row description.
- New "Observed invariant" entry documenting the per-user + viewer-
allowed + idempotent-DELETE quirks.
Verified locally: 136 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13, 6.32s test
runtime), tsc --noEmit clean.
T13 ticked. Space-people sub-tree: 5/6 done. T14 (deduplicate) is
the last one before T15-T17 finish the space-libraries sub-tree.
* test(e2e): apply post-T13 review nit — tighten alias null assertions
Tests 2 and 4 used a defensive `alias === null || alias === undefined`
check, but `mapSpacePerson` always sets `alias: string | null` and never
omits the field. Tightened both assertions to `toBeNull()` so a future
regression that drops the field would be caught (the disjunction would
silently still pass on a missing field).
Other NITs from the post-T13 review (split the two-in-one DELETE test,
trim the verbose backlog row) were judgment calls — leaving as-is.
Verified locally: 136 tests still green, tsc --noEmit clean.
* test(e2e): T14 deduplicate space people — Owner-only + jobId dedup pin
Adds 4 tests in a new nested describe inside the T09/T10/T11/T12/T13
block, covering POST /shared-spaces/:id/people/deduplicate.
Service shape (shared-space.service.ts:721-728): the manual dedup
trigger requires `Owner` role (NOT Editor — distinct from PUT/DELETE/
merge), then queues a SharedSpacePersonDedup job on the
FacialRecognition queue with jobId `space-dedup-${spaceId}`
(job.repository.ts:239-241).
Tests:
1. **Owner-only access matrix** — owner 204, editor 403 (this is the
distinguishing test from T11/T12 which all only required Editor),
viewer 403, non-member 403, anon 401. Pins the role distinction.
2. Owner happy path returns 204 — sanity check on the success shape.
3. Two consecutive owner calls both return 204 — pins HTTP-level
idempotency, independent of whether BullMQ deduplicates underneath.
4. **PR #292 jobId dedup verification.** The load-bearing test for the
whole T14 task. Strategy: pause the FacialRecognition queue, empty
it, trigger dedup twice, count jobs whose data matches the test
space, restore in try/finally. The two triggers should produce
exactly ONE queued job — BullMQ's queue() with a duplicate jobId
is a no-op, so PR #292's behaviour is preserved.
Queue manipulation requires admin token (queue.controller.ts:23 —
admin: true). The `admin` token is already set up in the outer
beforeAll. The pause/restore is bracketed in try/finally so a test
failure doesn't leave the queue in a paused state and break the
rest of the suite.
Backlog updates:
- T14 ticked.
- New "Observed invariant" pinning the Owner-only role and the
jobId-based queue dedup, with the verification mechanism documented.
Verified locally: 140 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14, 5.29s
test runtime), tsc --noEmit clean.
T14 ticked. **Space-people sub-tree COMPLETE** (T09-T14, all 6 tasks).
Phase 1 progress: T01-T14 done, only T15-T17 (space-libraries
sub-tree) remaining before Phase 1 closes.
* test(e2e): apply post-T14 review fixes — DTO body + tightened assertions
Four post-T14 review findings, all addressed in this commit. The test
behaviour is unchanged (still 140/140 green) but the test logic now
matches the actual server contract instead of relying on dead body
fields and over-permissive predicates.
1. **BLOCKING — DTO body shape was wrong.**
The test's `DELETE /queues/:name/jobs` calls sent
`{statuses: [...]}`, but `QueueDeleteDto` only has `failed?: boolean`
(queue.dto.ts:24-31). The `statuses` field was silently dropped by
NestJS's whitelist validator and the underlying
`jobRepository.empty(name)` drained the queue unconditionally
anyway, so the test passed — but the body was misleading: it
implied a statuses-based filter that doesn't exist. Removed both
sends in try and finally; the empty call is now bodyless and the
intent is clear.
2. **Comment misleading.** The GET /queues/:name/jobs call applied no
filter, so it returned ALL jobs in any state, not just waiting/paused.
Updated the comment to spell that out and removed the misleading
"waiting/paused" language.
3. **NIT — try/finally hole.** The test now asserts `pauseRes.status
=== 200` immediately after the PUT pause, so a pause failure fails
loudly instead of silently letting the worker race the assertion.
4. **NIT — over-permissive filter.** The original predicate was
`j.data?.spaceId === spaceId || j.id === space-dedup-${spaceId}` —
the `||` would only widen if the jobId encoded something OTHER than
spaceId, which is exactly the kind of refactor we want to catch.
Tightened to a strict `j.data?.spaceId === spaceId` check on the
job's data payload, which is the load-bearing field we care about.
Verified locally: 140 tests still green in shared-space.e2e-spec.ts,
tsc --noEmit clean.
* test(e2e): T15 PUT space libraries — link library to space
Adds 7 tests in a new nested describe inside the T09-T14 block,
covering PUT /shared-spaces/:id/libraries.
Service shape (shared-space.service.ts:449-477) has a TWO-step gate:
1. `if (!auth.user.isAdmin)` → ForbiddenException 'Only admins can
link libraries to spaces' — admin gate, fires FIRST.
2. `requireRole(Editor)` — must be a space member with Editor or
Owner role.
Then library existence is checked (400 if missing). On success,
addLibrary returns null for duplicate (spaceId, libraryId) and skips
the face-sync queue — making the operation idempotent at HTTP level.
Tests:
1. **Non-admin owner of the space cannot link** — pins the admin
gate firing BEFORE role check. Even the space owner gets 403 if
they're not also a global admin. Asserts the message matches /admins/.
2. Non-admin editor cannot link — same admin gate.
3. Non-admin viewer cannot link — same.
4. Anon → 401.
5. **Admin who is an Editor in the space CAN link** — happy path. The
block's beforeAll adds the global `admin` user as an Editor in the
T09 test space.
6. **Idempotent on duplicate link** — calling link twice with the
same library returns 204 both times, and the shared_space_library
table has exactly 1 row for the (spaceId, libraryId) pair. Pins
the "204, not 409" behaviour explicitly.
7. **Library not found returns 400** — pins the existence check
message.
Setup creates two libraries via utils.createLibrary (admin token,
admin.userId as owner). The second library is used for the success
test so the idempotency test has a clean slate on the first library.
Backlog updates:
- T15 ticked with the corrected description (was "409 on duplicate" —
the actual behaviour is 204 idempotent).
- New "Observed invariant" pinning the two-step gate ordering and the
idempotent duplicate behaviour, with line references.
Verified locally: 147 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15,
5.35s test runtime), tsc --noEmit clean.
T15 ticked. Space-libraries sub-tree: 1/3 done. T16 (unlink) and T17
(link side effects) remaining before Phase 1 closes.
* test(e2e): T16 DELETE space libraries — unlink, idempotent
Adds 5 tests in a new nested describe inside the T09-T15 block,
covering DELETE /shared-spaces/:id/libraries/:libraryId.
Service shape (shared-space.service.ts:479-487) — same two-step gate
as T15:
1. `if (!auth.user.isAdmin)` → ForbiddenException 'Only admins...'
2. `requireRole(Editor)`
Then repository.removeLibrary(spaceId, libraryId) is a plain DELETE
on the (spaceId, libraryId) pair (shared-space.repository.ts:220-226)
with NO row-existence check. So unlink is idempotent at the HTTP
level: deleting an already-unlinked link, or deleting with a bogus
libraryId, both return 204.
Tests:
1. Non-admin owner cannot unlink (admin gate fires first, message
matches /admins/i).
2. Anon → 401.
3. **Admin Editor CAN unlink + DB row count goes 1 → 0.** Pre-checks
the row exists, calls unlink, then verifies the row is gone via
direct DB query. Pins the actual mutation.
4. **Unlinking an already-unlinked library is idempotent.** Reuses
scratchLibrary which test 3 just unlinked, calls again, expects
204.
5. **Unlinking with a non-existent libraryId returns 204.** Pins the
"no existence check" behaviour — even a bogus UUID is a no-op
DELETE → 204, NOT 404 (the original design row was wrong about
this).
Setup creates `scratchLibrary` per beforeAll and pre-links it via
the API so test 3 has a real link row to remove.
Backlog updates:
- T16 ticked with the corrected description (was "non-existent link
→ 404" — actual is 204 idempotent, no existence check).
Verified locally: 152 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15
+ 5 T16, 5.31s test runtime), tsc --noEmit clean.
T16 ticked. Space-libraries sub-tree: 2/3 done. T17 (link side
effects) is the last Phase 1 task before the entire phase closes.
* test(e2e): T17 library link side effects — closes Phase 1
Adds 6 tests in a new nested describe inside the T09-T16 block, the
final task of the space-libraries sub-tree and the last task of the
entire Phase 1 of the e2e coverage backlog.
T17 exercises the cross-table query path that link/unlink enable:
library assets becoming visible to space members via /timeline/bucket
?spaceId=. PR #163 was specifically about this code path. Setup
uploads an admin asset normally and UPDATEs its libraryId via direct
DB to associate it with a fresh library — bypassing the fragile
library scan path while exercising the same JOIN.
Tests:
1. **After link, a non-owner space member sees the library asset via
/timeline/bucket?spaceId=** — the load-bearing PR #163 invariant.
Editor (who owns no assets) calls the bucket query for the space
and the library asset appears via the shared_space_library JOIN.
2. **Viewer sees it too** — symmetric assertion for the read-only role.
3. **After unlink, library assets are no longer visible** — round-trip
pin: link → see → unlink → don't see.
4. **Soft-deleted library asset is hidden** via the `deletedAt IS NULL`
filter on the timeline query. Mutate `asset.deletedAt` to NOW(),
verify hidden, restore.
5. **Offline library asset IS still visible (NOT hidden)** — SURPRISING
FINDING. asset.repository.ts:835-849 joins shared_space_library on
(libraryId, spaceId) but does NOT filter on asset.isOffline. So a
library asset whose underlying file went offline is still listed in
the space's timeline bucket. The asymmetry vs `deletedAt` filtering
is real. Test pins the actual behavior — if a future change adds
the missing isOffline filter, the test fails and forces a deliberate
update. The access.repository's checkSpaceAccess in a different
code path DOES filter on isOffline=false, so the timeline path is
the inconsistent one.
6. **Library delete eventually cascades to shared_space_library** —
library.service.ts:370-379 is a SOFT delete: it sets `deletedAt`,
queues a LibraryDelete job, returns 204 immediately. The cascade
happens async when the job processes. Test calls
waitForQueueFinish('library') after the DELETE before asserting
the FK row is gone.
Setup uses a DB-direct approach (createAsset + UPDATE libraryId)
instead of the library scan helper because the scan path was hitting
unexplained timing/timeout issues — bypassing the scan keeps the
test focused on the JOIN behavior, which is what T17 actually probes.
Backlog updates:
- T17 ticked.
- New "Observed invariant": timeline spaceId query lacks the
isOffline filter on library assets — pinned with file:line and
the asymmetry called out.
- New "Observed invariant": library delete is soft + async cascade —
tests must wait for the LibraryDelete job to drain before asserting
the FK cascade.
Verified locally: 158 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15
+ 5 T16 + 6 T17, 5.50s test runtime), tsc --noEmit clean.
T17 ticked. **Phase 1 COMPLETE** — all 17 tasks (T01-T17) done.
* test(e2e): revert T01 duplicate spec move — upstream-broken, leave for upstream
Revert the file move from T01 (f19eb16bc). The original task moved
e2e/src/api/specs/duplicate.e2e-spec.ts → e2e/src/specs/server/api/
so the vitest glob would pick it up. T01's commit message said
"audit found zero violations" — but the audit was for waitForQueueFinish,
NOT spec correctness. I never actually ran the spec to verify it works
after the move.
The full Phase 1 review surfaced that the spec is upstream-broken:
- Added in upstream PR #25316 (2026-03-26) by @Phlogi as part of the
"feat(server): resolve duplicates" feature.
- Upstream PR #25856 (Feb 10, 2026, by @minidzelis) had restructured
e2e/src/api/specs/ → e2e/src/specs/server/api/ six WEEKS earlier.
- PR #25316 didn't notice and added the new spec in the OLD path.
The file went into the wrong directory and was never picked up by
the vitest glob in upstream's CI either.
- A subsequent upstream PR changed the response shape of
POST /duplicates/resolve from `{status, results: [{duplicateId, status}]}`
to a bare `BulkIdResponseDto[]`. The never-running spec didn't get
updated. 18 of 21 tests now fail against the current API.
- The 3 tests that DO pass are access-matrix style assertions that
are robust to body shape changes.
This is upstream's bug. Maintaining a fork-local fix would generate
merge conflicts every rebase. The right fix lives upstream — file an
issue / PR there.
Reverting the move puts the file back where upstream put it. Our
vitest glob doesn't pick it up, so our CI is unaffected by upstream's
broken tests. When upstream eventually fixes the spec (either fixes
the assertions or re-locates the file or both), we inherit the fix
cleanly via the next rebase.
Backlog updates:
- T01 row rewritten to reflect that only the audit landed (not the
file move) and to point at the new T-cleanup-01 follow-up.
- New "Upstream cleanup tasks" section before the Decision log,
with T-cleanup-01: bring upstream duplicate.e2e-spec.ts up to date
with the current /duplicates/resolve API and PR upstream.
Estimated 2-4 hours of mechanical assertion rewriting.
Verified locally: the 4 working specs (helpers + timeline + face +
shared-space) still pass 205/205 across 3 consecutive runs after the
revert. tsc --noEmit clean. No regressions to our coverage.
* test(e2e): T18 gallery-map filter access matrix + filters
Adds e2e/src/specs/server/api/gallery-map.e2e-spec.ts with 12 tests
covering the fork-only `/gallery/map/markers` controller. This is the
filtered map endpoint distinct from `/map/markers` — accepts a rich
query (people, tags, EXIF, dates, favorite, country/city) used by the
web map view's filter panel.
Service shape (shared-space.service.ts:561-585):
- Without spaceId: scoped to auth.user.id
- With spaceId: requireAccess(SharedSpaceRead) → 400 for non-member
- personIds re-route to spacePersonIds when spaceId is set (same DTO
field, different semantics)
- Always filters visibility=Timeline regardless of client input
Tests:
1. Auth required (anon → 401)
2. Authenticated user with no filters returns their geotagged assets
3. Empty array for a fresh user with no uploads
4. country filter narrows correctly (matching + non-matching cases)
5. city filter narrows correctly (matching + non-matching cases)
6. isFavorite=true excludes non-favorite assets
7. takenAfter cutoff in the future excludes the asset
8. takenBefore cutoff in the past excludes the asset
9. rating outside 1-5 returns 400 (DTO @Min/@Max validation)
10. type with invalid enum value returns 400
11. Archived assets are excluded — service hardcodes visibility=Timeline.
Test toggles the asset to archive and verifies the marker disappears.
12. Cross-user isolation — another user does not see this user's markers
(without spaceId, the service scopes to auth.user.id).
Setup uses the existing thompson-springs.jpg fixture (real GPS in
Colorado, USA, with camera EXIF) so the metadata-based filters have
real data to match against. Pattern matches the existing /map e2e
spec for fixture upload + websocket wait.
T19 will cover spaceId scoping and the personIds → spacePersonIds
re-routing as a separate task per the backlog.
Verified locally: 12 tests green (986ms test runtime), tsc --noEmit
clean.
T18 ticked. Phase 2: 1/5 done.
* test(e2e): T19 gallery-map spaceId scoping
Adds 6 tests in a new nested describe inside the T18 block, covering
the spaceId code path in /gallery/map/markers.
Setup creates a fresh space owned by `user`, adds a second member
(spaceMember), and adds the geotagged fixture asset to the space so
it should appear in space-scoped queries. spaceNonMember is created
but never added to the space.
Tests:
1. Non-member gets 400 (requireAccess BadRequestException at
shared-space.service.ts:563). Same taxonomy as T03 timeline —
the bulk-access pattern returns 400 not 403.
2. Anon → 401.
3. Space member sees the space asset via spaceId — the load-bearing
read invariant for space-scoped map queries.
4. Space owner with spaceId sees space-scoped content (the assertion
would catch a regression that returned the full owner library
instead of the space subset).
5. Non-existent spaceId returns 400 — bulk-access pattern (no 404).
6. country filter composes with spaceId — `country=Antarctica`
returns empty even though the asset would otherwise be visible.
PR #202's "hidden persons exclusion on gallery-map" angle is deferred
to a follow-up — it would need a shared_space_person fixture with
isHidden=true similar to T09's setup, which is more involved than the
T19 scope.
Verified locally: 18 tests green in gallery-map.e2e-spec.ts (12 T18
+ 6 T19, 898ms test runtime), tsc --noEmit clean.
T19 ticked. Phase 2: 2/5 done.
* docs(plans): mark T20 N/A — /map/markers has no spaceId support
T20 was originally planned as "space scoping extension to map.e2e-spec.ts"
but the upstream /map/markers endpoint has no spaceId support at all:
- server/src/dtos/map.dto.ts:29-47 — MapMarkerDto has no spaceId field
- server/src/services/map.service.ts:9-27 — getMapMarkers has no spaceId
branch (only userIds + albumIds)
- server/src/repositories/map.repository.ts — no spaceId references
Space-scoped map queries are entirely on the fork-only /gallery/map/markers
endpoint that we just covered in T18+T19. T20 is moot.
Marked the row as N/A in the backlog with the verification line refs so
a future maintainer doesn't reattempt the task.
Phase 2 is now effectively a 4-task group (T18/T19/T21/T22), with T20
crossed out.
* test(e2e): T21 view folder browsing tests
Adds e2e/src/specs/server/api/view.e2e-spec.ts with 9 tests covering
the /view controller's two folder-browsing endpoints.
Service shape (view.service.ts:7-16): both endpoints strictly scope
to auth.user.id. No partner sharing, no space scoping, no library
joins. The folder browse is owner-only.
Tests:
GET /view/folder/unique-paths (3):
1. Auth required (anon → 401)
2. Authenticated user gets their unique folder paths (non-empty array)
3. Cross-user isolation — userB's response does not contain any of
userA's paths that include userA's userId. The actual upload paths
include the user UUID, so this is a structural assertion that no
leak path exists between users.
GET /view/folder (5):
4. Auth required
5. Returns assets when given a known path from the user's folder list
(resolves a path via /unique-paths first, then queries /folder).
6. Empty array for a non-existent path
7. Cross-user isolation — userB calling /folder with userA's path
does NOT see userA's asset in the result. Pins that the service
really does scope by auth.user.id even when the caller-supplied
path matches another user's folder structure.
8. **Missing `path` query param returns 500** — REAL FINDING. The
controller at view.controller.ts:33 declares `@Query('path') path:
string` with no validation pipe and no default. The service passes
undefined to the repository which trips with a 500. Pinning the
actual behavior so a future server-side fix forces a deliberate
update. Worth filing upstream as a small server bug — should be
400 with a clear validation message.
Plus a small sanity assertion (9) that touches the userBAssetId
fixture so the linter doesn't flag it as unused.
T20 was N/A so this is the next Phase 2 task. T22 (workflow) is the
last one before Phase 3.
Verified locally: 9 tests green (404ms test runtime), tsc --noEmit
clean.
T21 ticked. Phase 2: 4/5 done (T20 was N/A).
* test(e2e): T22 workflow CRUD access matrix — closes Phase 2
Adds e2e/src/specs/server/api/workflow.e2e-spec.ts with 15 tests
covering the fork-only /workflows controller. Last task in Phase 2.
Service shape (workflow.service.ts):
- create: validates triggerType + per-plugin filter/action IDs (400 on bad ID)
- getAll: scoped to auth.user.id (owner-only)
- get/update/delete: requireAccess(WorkflowRead/Update/Delete) → 400 for non-owner
- update with no fields → BadRequestException('No fields to update')
- getAll cross-user returns empty array (not 403) — owner-scoped query
Workflows are per-user with no sharing concept. Cross-user access is
uniformly rejected at the access layer.
Tests:
POST /workflows (5):
1. Auth required (anon → 401)
2. Create with empty filters and actions succeeds (201, returns
workflow with ownerId, default enabled=true, AssetCreate trigger)
3. Invalid trigger type returns 400
4. Invalid pluginFilterId returns 400 with /filter/i message
5. Empty name returns 400 (DTO @IsNotEmpty)
GET /workflows (2):
6. Auth required
7. Owner-scoped — userA's workflows visible to userA, userB sees
empty array
GET /workflows/:id (3):
8. Owner can fetch their own workflow
9. Cross-user GET returns 400 (requireAccess bulk-access)
10. Non-existent workflow ID returns 400 (not 404)
PUT /workflows/:id (3):
11. Owner can rename
12. Empty PUT body returns 400 with /no fields/i message — pins the
explicit "No fields to update" service guard
13. Cross-user PUT returns 400
DELETE /workflows/:id (2):
14. Owner can delete + subsequent GET returns 400 (workflow gone)
15. Cross-user DELETE returns 400
Verified locally: 15 tests green (489ms test runtime), tsc --noEmit
clean.
T22 ticked. **Phase 2 COMPLETE** — T18/T19/T21/T22 done; T20 was N/A.
Phase 2 added 42 tests across 3 new spec files (gallery-map, view,
workflow). Total branch coverage: 247 e2e tests (Phase 1 = 205 + Phase
2 = 42).
* test(e2e): apply post-Phase-2 review nits
Three NITs from the Phase 2 review, all addressed:
1. **gallery-map T19 — missing personIds re-routing test.** The
service at shared-space.service.ts:569-570 re-routes
`dto.personIds → spacePersonIds` when `spaceId` is set, making
the same DTO field mean different things in different contexts.
Added a 7th test in the T19 nested describe that probes the
re-routing: passing a bogus person UUID with spaceId returns
empty (the lookup goes through shared_space_person, not asset_face).
Not the strongest possible assertion, but pins the behavioural
shape so a future refactor that changes the re-routing direction
would be caught.
2. **view T21 — dead sanity-check test.** The original spec had
`expect(typeof userBAssetId).toBe('string')` only to satisfy
the linter about the unused `userBAssetId` fixture. Removed both
the variable assignment and the dead test. The cross-user-isolation
tests already exercise userB by querying with their token; we
don't need their asset id specifically. Drop both, keep the
second createAsset call so userB has folder content.
3. **gallery-map backlog row count.** T19 said "6 tests" but the
actual count is 7 with the new re-routing test. Updated the row.
Verified locally: 27 tests green across gallery-map (19) + view (8),
tsc --noEmit clean.
* test(e2e): T23 asset metadata K/V CRUD
Adds e2e/src/specs/server/api/asset-metadata.e2e-spec.ts with 13
tests covering the asset metadata K/V endpoints (PUT /:id/metadata,
GET /:id/metadata, GET/DELETE /:id/metadata/:key, bulk PUT/DELETE
/assets/metadata).
All routes use Permission.AssetRead/AssetUpdate which routes through
requireAccess (bulk-access pattern → 400 for non-owner).
Tests:
GET /assets/:id/metadata (3):
1. Auth required (anon → 401)
2. Owner can list (empty initially)
3. Non-owner returns 400 (bulk-access)
PUT /assets/:id/metadata (3):
4. Owner upsert + value is queryable via the listing
5. Upsert overwrites an existing key value (set v=1, set v=2,
verify single-key fetch returns v=2)
6. Non-owner upsert returns 400
GET /assets/:id/metadata/:key (2):
7. Owner can fetch a single key set in test 4
8. Missing key returns 400 or 404 (test pins both — the actual
path uses requireAccess + service throw, behavior pinned)
DELETE /assets/:id/metadata/:key (2):
9. Owner can delete a key and it's removed from the listing
10. Non-owner delete returns 400
PUT /assets/metadata (bulk) (2):
11. Owner can upsert across multiple of their own assets in one call
12. Bulk upsert with a non-owner asset id mixed in is rejected (400)
DELETE /assets/metadata (bulk) (1):
13. Owner can bulk-delete keys across multiple of their own assets
Verified locally: 13 tests green (554ms test runtime), tsc --noEmit
clean.
T23 ticked. Phase 3: 1/6 done.
* test(e2e): T24 asset OCR endpoint access mat…
Deeds67
added a commit
to open-noodle/gallery
that referenced
this pull request
Apr 13, 2026
* docs(plans): research e2e API test coverage gaps and backlog
Catalogues the 282-endpoint server surface against the 29 existing
e2e API specs, identifies the 8 zero-coverage controllers and the
endpoint holes inside heavily-tested specs (shared-space /people and
/libraries, asset metadata/edits/copy, library->space link side
effects). Defines a reusable Permission/Actor matrix and proposes
~14 follow-up PRs prioritised by bug-catching ROI.
* docs(plans): break e2e API coverage research into per-task backlog
Adds a working backlog of 39 PR-sized tasks (T01–T39) derived from the
e2e API coverage research doc, plus full design docs for the three
upfront tasks: helpers (T02), timeline access matrix (T03), and the
shared-space people listing (T09).
The backlog records architectural decisions (helper API shape, fixture
lifetime, extending utils.createSpacePerson over duplication) separately
from observed server invariants (400 vs 403 split between requireAccess
and requireMembership, the listing's thumbnailPath gate, the metadata vs
thumbnail role split on PATCH /shared-spaces). Each task row hard-pins
its dependencies so most of Phase 1+ can run in parallel after T02.
Four code-reviewer passes caught and fixed ~15 substantive issues across
the drafts; the docs as committed here are the result.
* test(e2e): T01 move stray duplicate spec into the discovered glob
The vitest config at e2e/vitest.config.ts uses
`src/specs/server/**/*.e2e-spec.ts` for spec discovery. The file at
e2e/src/api/specs/duplicate.e2e-spec.ts was outside that glob and has
never been running in CI — it was added during the duplicate-detection
work but landed in the wrong directory.
Move it to e2e/src/specs/server/api/duplicate.e2e-spec.ts (where every
other server API spec lives), remove the now-empty src/api/specs and
src/api directories, and tick T01 in the coverage backlog.
The companion audit of waitForQueueFinish vs expect.poll for non-admin
specs (per the e2e-coverage research doc and the
feedback_e2e_admin_only_queues memory) found zero violations: all 38
existing callers correctly pass admin.accessToken. Result documented
inline in the backlog row.
Imports in the moved file use the `src/*` path alias, so file depth is
irrelevant — no code changes were needed beyond `git mv`.
* test(e2e): T02 add Actor / SpaceContext / forEachActor helpers
Adds e2e/src/actors.ts with Actor + ActorId + SpaceContext types and
the buildSpaceContext / forEachActor helpers, and extends the existing
utils.createSpacePerson to insert the shared_space_person_face junction
row, accept a type parameter, and return {globalPersonId, spacePersonId,
faceId} instead of just the space person ID.
These exist to turn the Permission/Actor matrix from §3 of the e2e
coverage research doc into a one-liner per endpoint, so downstream
specs (T03+) can write `forEachActor(...)` instead of hand-rolling six
describe blocks per endpoint. See docs/plans/2026-04-06-e2e-T02-helpers-design.md
for the rationale and decision log.
Key design points (all captured in the design doc):
- buildSpaceContext composes utils.adminSetup / userSetup / createSpace /
addSpaceMember / addSpaceAssets / createAsset — no parallel
implementations of any existing helper.
- forEachActor throws Error (not expect.toBe) so the failure message
names the actor that failed; without that, debugging an actor matrix
is needlessly painful.
- Sequential, not parallel — tests share a database and parallel actor
runs would race on the same fixtures.
- Fixture lifetime contract: ctx is read-only in beforeAll; mutating
tests own their cleanup via try/finally or nested describes.
- ActorId starts minimal (8 actors). partner / libraryOwner / apiKey* /
sharedLink land with their first consumer task.
Smoke tests in e2e/src/specs/server/api/_helpers.e2e-spec.ts validate
all three behaviours that downstream PRs depend on:
1. Auth threading — bearer token reaches the server for every actor.
2. Anon split — /users/me requires auth (anon: 401, members: 200).
3. createSpacePerson extension — returns three IDs and inserts the
shared_space_person_face junction row (verified via direct DB query).
4. Role assignment — PATCH /shared-spaces/:id with {thumbnailCropY: 0}
distinguishes Owner/Editor from Viewer. Uses thumbnailCropY (Editor-
level) rather than name (Owner-level per shared-space.service.ts:197-203)
so Editor and Viewer get distinct status codes.
Implemented test-first: each smoke test was written and run failing
before the corresponding helper code was added.
Verified locally against the e2e stack: all 4 smoke tests green
(537ms total), server.e2e-spec.ts unchanged (24/24 still pass), tsc
--noEmit clean. createSpacePerson currently has zero callers in e2e/
(verified via grep) so the signature change is risk-free.
T01 ticked in the backlog along with T02. The cleanup audit found zero
non-admin waitForQueueFinish callers — the rule from the
feedback_e2e_admin_only_queues memory is currently held everywhere.
* test(e2e): T03 add timeline /buckets and /bucket access matrix
Adds e2e/src/specs/server/api/timeline.e2e-spec.ts with 9 tests
covering the access matrix for both timeline endpoints. This is the
first real consumer of the actor / forEachActor helpers from T02.
Tests on GET /timeline/buckets:
1. Auth required (anon: 401, owner: 200)
2. Owner sees own assets without filter (count == 2)
3. spaceId access matrix — status codes for owner/editor/viewer/non-member/anon.
Non-member returns **400** (not 403), pinned because timeline uses requireAccess
which throws BadRequestException (src/utils/access.ts:37-42), distinct from the
shared-space-family endpoints which use requireMembership and return 403.
4. spaceId scopes assets to the space, not the requesting user. spaceOwner with
spaceId sees the 1 space asset, NOT the 2 they own. Catches the bug shape where
the implementation would `WHERE asset.ownerId = auth.user.id` instead of joining
through shared_space_asset.
5. Non-owner space members (editor, viewer) actually see space content via spaceId.
This is the PR #163 / #202 bug shape — it returned 200 with empty body, so pure
status-code testing would have missed it.
Tests on GET /timeline/bucket (singular):
6. Auth required.
7. spaceId access matrix mirroring test 3 — same shape, distinct endpoint. The
risk being probed: forgetting to apply the same scoping check on the singular
endpoint. PR #260 is in this family.
8. Non-owner space members see the space asset via /bucket. Same bug class as
test 5, applied to the asset-list endpoint. Asserts the actual asset ID is in
the response, not just the status code.
9. /bucket returns the parallel-array TimeBucketAssetResponseDto shape, not
bucket counts. Sanity check that the two endpoints aren't confused.
Coverage matches the 10 tests enumerated in the T03 design doc; design tests 3
and 6 are merged into one access-matrix test (the matrix already pins
spaceNonMember: 400, so a separate test would assert the same thing).
Verified locally against the e2e stack: 9 tests green (580ms total). Combined
with the 4 helper smoke tests, total: 13 tests, 1.06s. tsc --noEmit clean.
T03 ticked in the backlog.
* test(e2e): apply post-T03 review nits
Three nits from the post-T01-T03 implementation review:
1. Add `authHeaders(actor)` helper to actors.ts and use it in
_helpers.e2e-spec.ts and timeline.e2e-spec.ts. The previous
`actor.token ? asBearerAuth(actor.token) : {}` pattern at every
forEachActor call site was unprecedented in the e2e suite —
localizing the conditional inside actors.ts keeps the call sites
consistent with idiomatic supertest usage.
2. Drop the redundant `utils.initSdk()` calls in the new spec files'
beforeAll. utils.ts:809 already invokes it at module load, and no
other spec under specs/server/api calls it explicitly.
3. Drop the try/finally + utils.disconnectDatabase() in
_helpers.e2e-spec.ts smoke test 3. Existing specs that use the
raw pg client (shared-space.e2e-spec.ts, etc.) don't disconnect —
they rely on worker-process exit. Disconnecting mid-spec would
break any later test in the file that uses utils.createSpacePerson
or any other client-using helper. Add an inline comment explaining
why.
`asBearerAuth(actor.token!)` is left in place at non-forEachActor
call sites where the actor is always authenticated — that's the
existing convention across the e2e suite.
Verified locally: 13 tests still green (4 helpers + 9 timeline,
1.03s), tsc --noEmit clean.
* test(e2e): T04 add timeline withSharedSpaces / withPartners semantics
Extends actors.ts and timeline.e2e-spec.ts to cover the two flags
that gate cross-user content on the timeline endpoints.
Helper changes (actors.ts):
- Add `partner` to ActorId.
- Add optional `partner` and `partnerAssetId` fields to SpaceContext.
- Add `BuildSpaceContextOptions` with `withPartner?: boolean`. When
set, buildSpaceContext creates an extra user, has them share their
library with spaceOwner, and uploads one asset for them.
- Add `addPartner({token, userId}, {token, userId})` helper. The
default `partner.inTimeline` column is **false** (verified at
server/src/schema/tables/partner.table.ts:46), so a fresh
`createPartner` call is invisible to `withPartners=true` until the
recipient enables it. The helper auto-enables inTimeline by having
the recipient call PUT /partners/:id, so test call sites don't have
to remember the two-step dance. This default-false behaviour is
exactly the kind of footgun an integration test would have caught
if anyone had ever written one before today.
New tests in timeline.e2e-spec.ts (`describe('GET /timeline/buckets — withSharedSpaces and withPartners')`):
1. withSharedSpaces=true makes a non-owner member see space content
on their own timeline. spaceEditor owns 1 asset (editorAssetId);
with withSharedSpaces, the union picks up spaceAssetId via the
membership default of showInTimeline=true, so total goes 1 -> 2.
2. Toggling showInTimeline=false drops the space out of
getSpaceIdsForTimeline. PATCH /shared-spaces/:id/members/me/timeline
with {showInTimeline: false}; verify total goes back to 1; restore
in try/finally per the fixture lifetime contract.
3. withPartners=true makes spaceOwner see partner-shared assets.
Total = 2 own + 1 partner = 3.
4. Default (no withPartners) excludes partner assets. Total = 2.
5. Combining withSharedSpaces and withPartners doesn't double-count —
spaceAssetId is already counted in spaceOwner's own 2, so the union
stays at 3.
All calls pass `visibility=timeline` explicitly because timeline.service.ts:91-113
treats `visibility === undefined` as `requestedArchived = true` and throws 400 when
either flag is set. This invariant is now pinned in the backlog.
Backlog updates:
- T04 row ticked (5 tests).
- New "Observed invariants" entries: partner.inTimeline default-false,
withSharedSpaces/withPartners visibility requirement.
Library-linked space asset visibility (mentioned in the original T04
design) is deferred to T17, which is the right home — it requires the
linkLibrary helper that doesn't exist yet.
Verified locally: 18 tests green (4 helpers + 14 timeline, 1.11s),
tsc --noEmit clean.
* test(e2e): T05 add timeline visibility filter tests
Adds 7 new tests to timeline.e2e-spec.ts: 5 for the visibility filter
behaviour itself, plus 2 invariant pins folded in from the T04 review
NIT (visibility-undefined-throws-400).
The visibility tests use a dedicated user (visibilityUser) with 4
assets in different states: timeline, archive, hidden, and trashed.
A separate user keeps the assertions deterministic — using spaceOwner
would have to subtract the existing space-related assets from every
expected count.
Tests in `describe('GET /timeline/buckets — visibility filters')`:
1. **default visibility (no param) returns Timeline AND Archive (2)** —
pins the non-obvious server behaviour at server/src/utils/database.ts:79-81.
`withDefaultVisibility` is `where('asset.visibility', 'in', [Archive, Timeline])`,
NOT just Timeline. The web UI's main timeline view must pass
`visibility=timeline` explicitly to exclude archived assets. This was a
real surprise during implementation — the first version of the test
expected count=1 and failed with count=2 against the live server.
2. visibility=timeline returns only the strict timeline assets (1).
3. visibility=archive returns only archived assets (1).
4. visibility=hidden returns only hidden assets (1). Hidden visibility is
normally used for the video part of live photos / motion photos
(per the AssetVisibility enum docstring); pinned here to protect that
path from a future refactor.
5. trashed assets are excluded regardless of visibility filter. Asserts
the count under both default and explicit visibility=timeline so a
trash regression would inflate either independently.
Tests appended to the withSharedSpaces/withPartners describe (folded
from T04 review NIT):
6. withSharedSpaces=true without explicit visibility returns 400.
7. withPartners=true without explicit visibility returns 400.
Backlog updates:
- T05 ticked.
- New "Observed invariant" entry: default visibility filter is
permissive (Timeline + Archive) — this is exactly the kind of fact
that bites every author who hasn't read database.ts.
Verified locally: 21 tests green (4 helpers + 17 timeline, 812ms),
tsc --noEmit clean.
* test(e2e): apply post-T05 review nits
Three nits from the post-T05 implementation review:
1. Visibility-400 invariant tests now also assert on the error message
(`/withSharedSpaces/` and `/withPartners/`). Without that, a future
unrelated 400 (e.g. a DTO validation change) would silently still
satisfy the test.
2. Renamed test 5 to "soft-deleted (trashed) assets are excluded
regardless of visibility filter". Both soft-delete and force-delete
set `deletedAt`, so the test characterises the deletedAt-based
exclusion which is what the timeline query actually depends on.
Comment now spells this out.
3. Refactored the `Promise.all` 4-element destructure into a parallel
block of 3 + a sequential trash creation. The previous
`const [, , , trashedAsset] = await Promise.all([...])` was uncommon
in the e2e suite. Pulling trashed out of the parallel block is
slightly slower (4 sequential RTTs vs 3 parallel + 1) but more
readable, and the cost is negligible at fixture-setup time.
Verified locally: 21 tests still green, tsc --noEmit clean.
* test(e2e): T06 add timeline filter passthrough tests with spaceId
Adds 5 tests to timeline.e2e-spec.ts in a new describe block, probing
how the various filter parameters (`spacePersonId`, `personId`, `tagIds`)
interact with `spaceId` scoping. This is the PR #260 bug shape pin —
the fork has a *dedicated* `spacePersonId` DTO field separate from
`personId`, and the original PR #260 bug was matching a global `personId`
against a `shared_space_person.id`.
Setup creates two new fixtures attached to ctx (without mutating any
existing state):
- `spacePerson` via utils.createSpacePerson — adds Alice as a face on
spaceAssetId, with the shared_space_person_face junction row that
the timeline filter joins through.
- `spaceTagId` — a tag owned by spaceOwner, applied to spaceAssetId.
Tests:
1. **spacePersonId + spaceId** restricts to assets containing that
space person. Joins through shared_space_person_face → asset_face
→ asset; only spaceAssetId qualifies, total=1.
2. **GLOBAL personId + spaceId** does not cross-pollute. Pinned at
total=1 (which here is *legitimate* because the global person row
is the same one createSpacePerson set up under the hood — not a
bug). The test exists so a future change that decouples the two
joins or breaks the spaceId restriction is caught.
3. **spacePersonId without spaceId** falls back to owner-scoped and
still filters correctly (total=1, just spaceAssetId).
4. **tagIds + spaceId** returns the tagged subset of space content.
5. **non-owner space member sees tagged space content via
spaceId+tagIds** — spaceEditor querying with spaceOwner's tag
returns the tagged space asset. Pins that the timeline tag filter
doesn't enforce per-user tag ownership for space-scoped queries,
which is the expected UX for shared spaces (one member labels a
photo, others see the label).
Coverage matches 5 of the ~8 tests in the original T06 design. EXIF
filter passthrough (country/make/rating) is deferred because it requires
fixture images with extracted metadata (the e2e suite uses generated
PNGs for most tests). When a follow-up wants EXIF coverage it can be
appended to this describe block.
Verified locally: 26 tests green (4 helpers + 22 timeline, 868ms),
tsc --noEmit clean.
T06 ticked. All four Phase 1 timeline subtasks (T03–T06) are now done.
* test(e2e): apply post-T06 review nits
The post-T06 review caught a real coverage gap: the original test 2
("global personId does NOT cross-pollute") asserted total=1, which
was a coincidence — the global person from createSpacePerson is
legitimately attached to spaceAssetId via asset_face, so the join
hits it whether or not the spaceId restriction is working. A
regression that removed the spaceId scoping entirely would still
return 1 and the test would pass.
To actually pin the boundary:
1. beforeAll now also creates a decoy global person ("Decoy Bob")
attached only to ownerAssetId (NOT in the space). No
shared_space_person row, no junction.
2. Test 2 is now the load-bearing boundary test: query with
personId=<decoy>&spaceId=<spaceId> and assert total=0. The
decoy's asset is NOT in the space; if the spaceId scoping is
working, the join must not return it. If a future regression
breaks the scoping, this test goes from 0 to 1.
3. Added a new test 3 for the other half of the boundary: querying
with personId=<spacePerson.globalPersonId>&spaceId=<spaceId> still
returns 1 because that global person IS attached to a space asset
via asset_face. Together, tests 2 and 3 pin both sides of the
spaceId boundary on the global personId join path.
Plus the IMPORTANT and NIT comment improvements:
- Test "spacePersonId without spaceId" comment now spells out that
the join is NOT spaceId-restricted — a future test that puts the
same spacePersonId on a second space's asset would observe
count > 1.
- Test "non-owner space member sees tagged content" comment now
spells out the actual invariant being pinned: hasTags has zero
per-user check, tag IDs are universally addressable on the
timeline filter. The shared-spaces UX consequence (one member
labels, all members can filter) is documented as the load-bearing
property — a future refactor that adds an owner check to hasTags
would silently break the UX unless this test catches it.
Verified locally: 27 tests green (4 helpers + 23 timeline, 914ms),
tsc --noEmit clean.
* test(e2e): T07 add face CRUD access matrix
Adds e2e/src/specs/server/api/face.e2e-spec.ts with 10 tests covering
the four /faces endpoints. Also extends utils.createFace to return the
inserted face id (was Promise<void>) so PUT/DELETE tests can address
specific faces — backwards-compatible because all existing callers
ignore the return value.
Tests:
POST /faces (3):
1. Access matrix on the asset side. Owner can; spaceNonMember 400;
anon 401. Same Immich-wide bulk-access pattern as timeline (400
not 403 for non-owner).
2. Cross-owner asset rejected even when the person belongs to caller.
3. Cross-owner person rejected even when the asset belongs to caller.
GET /faces (2):
4. Access matrix on the asset side.
5. Owner gets the face row back with the linked person populated.
Asserts presence of the specific face we inserted (not a count) to
stay robust against unrelated faces accumulating on the asset.
PUT /faces/:personId — reassign (2):
6. Access matrix when reassigning a face to a new person owned by
the same user (Alice → Anne).
7. Reassigning to a cross-owner target person is rejected — the
person-access check on the target fires before any state mutation.
DELETE /faces/:faceId (3):
8. Owner can soft-delete (force=false).
9. Owner can force-delete (force=true).
10. Access matrix for non-owner / anon. Each test creates its own
scratch face so the access matrix doesn't permanently mutate
state.
The non-obvious API shapes are documented in the spec file header
and pinned in the backlog "Observed invariants" section:
- POST /faces returns void (no face id)
- PUT /faces/:id has path=target-person, body=face — the FaceDto is
reused with different meanings on different endpoints
- GET /faces takes ?id=<assetId> (the FaceDto field is named `id`
but represents the asset)
The first attempt at the spec hit four cascading failures from my
incorrect API understanding (POST returning void, PUT semantics
backwards, count assertion fragile to other tests). The corrected
version pins the actual server behaviour and is robust to test
isolation.
Verified locally: 41 tests green across 3 spec files
(4 helpers + 27 timeline + 10 faces, 1.82s test runtime),
tsc --noEmit clean.
T07 ticked.
* test(e2e): T08 add face deletion side effect tests + post-T07 nit fix
Adds 6 tests to face.e2e-spec.ts in a new "face deletion side effects (T08)"
describe block, plus a one-line comment fix from the post-T07 review.
T08 tests:
1. Soft-deleted face is excluded from GET /faces?id=<assetId> via the
`asset_face.deletedAt IS NULL` filter (person.repository.ts:229).
2. Hard-deleted face is excluded from GET (deleted row, same observable
result).
3. Soft-deleting the only face on a person preserves the person row.
Global persons are NOT cascade-deleted when their last face goes away;
GET /people/:id still returns the person. This matters for the
shared-spaces UX where the person row outlives any individual face
attachment.
4. Soft-deleting a face decreases the person's getStatistics asset count.
getStatistics joins through asset_face filtering on `deletedAt IS NULL`
AND `isVisible IS true` (person.repository.ts:335-352), so soft-delete
drops the row out of the count.
5. Hard-delete decreases the count via the same mechanism (row removal).
6. Re-attaching the same (assetId, personId) after a soft-delete inserts
a NEW asset_face row. There's no UNIQUE constraint blocking this;
pinned so a future schema change is caught.
Two scope adjustments documented in the backlog row:
- "Below-minFaces faces unaddressable" was a *space-person* concern from
PR #139 (space person thumbnail 404s), not a global face concern.
Moved to T10/T11.
- "Space-person dedup queue jobId dedup" from PR #292 requires probing
queue state that isn't exposed via the face controller. Moved to T14
which owns the deduplicate endpoint.
Post-T07 review NIT fix:
- POST /faces access matrix comment said "write access to BOTH the asset
and the person", but person.service.ts:641-642 actually uses
`Permission.AssetRead` + `Permission.PersonRead` (not write). Comment
reworded.
Verified locally: 47 tests green across 3 spec files (4 helpers + 27
timeline + 16 faces, 1.88s test runtime), tsc --noEmit clean.
T08 ticked. Faces group complete (T07 + T08).
* test(e2e): apply post-T08 review nit — strengthen the re-attach test
Test 6 ("re-attaching a face after a soft-delete") originally asserted
both faces on the same asset, which made the stats `count(distinct
asset.id)` = 1 with or without the deletedAt filter — the assertion
passed by accident, not because of the filter.
Strengthen by:
1. Putting the soft-deleted face on assetA and the new face on assetB.
The stats count now actually distinguishes the two semantics:
- With deletedAt filter (correct): 1 (only assetB)
- Without it (broken): 2 (assetA + assetB)
2. Adding a third face on assetA (re-attach with the existing
soft-deleted row in place). Asserts no UNIQUE constraint blocks the
second insert AND that count goes 1 → 2 to confirm the new face is
counted.
The "no UNIQUE constraint" pin is preserved; the deletedAt filter is
now genuinely exercised by the count assertion.
Verified locally: 16 tests still green, tsc --noEmit clean.
* test(e2e): T09 add shared-space people listing tests
Adds an 11-test describe block to shared-space.e2e-spec.ts covering
the GET /shared-spaces/:id/people endpoint per the T09 design doc
(docs/plans/2026-04-06-e2e-T09-space-people-listing-design.md).
Setup: dedicated owner/editor/viewer/non-member users with their
own space and 5 space-people via the extended utils.createSpacePerson
helper from T02 (which inserts the four-table chain including the
shared_space_person_face junction).
Tests:
1. Access matrix — owner/editor/viewer 200, non-member 403, anon 401.
shared-space endpoints use requireMembership → ForbiddenException,
distinct from timeline's requireAccess → BadRequestException.
2. Listing returns space person IDs (NOT global person IDs). The
canonical assertion for the whole T09–T14 sub-tree.
3. Hidden persons excluded by default.
4. ?withHidden=true includes hidden persons.
5. Unnamed persons included by default.
6. ?named=true returns only persons with non-empty names (on either
shared_space_person.name OR person.name per the OR clause in
shared-space.repository.ts:514-521).
7. Toggling shared_space.petsEnabled=false hides pets, restored in
try/finally per the fixture lifetime contract.
Pagination tests in a nested describe (own beforeAll/afterAll for the
15 extra rows so they don't leak into sibling tests):
8. ?limit=10 caps the response.
9. ?offset paginates without overlap.
10. Sort order is stable across calls.
Final test in the parent describe:
11. Empty thumbnailPath on the underlying global person excludes the
space person from the listing. Pins the fork's "minFaces gate"
mechanism (shared-space.repository.ts:512-513) — pinned now so a
future query refactor that drops the filter would be caught.
Mutates via direct DB and restores in try/finally.
All assertions match the design doc decisions in the backlog
"Decision log" section: space person ID is canonical, stable sort,
extended createSpacePerson is the helper, listing query params are
limit/offset/withHidden/named/takenAfter/takenBefore (no `top`, no
text-based name search).
Backlog updates:
- T09 ticked.
- New "Known flaky-spec footgun" section: utils.createPerson +
Promise.all is unsafe with the shared pg.Client — observed once
as an FK violation in T09 setup, didn't reproduce in 3 follow-up
runs. Latent since T07. Not blocking; documented for follow-up.
Verified locally: 110 tests green in shared-space.e2e-spec.ts
(99 existing + 11 new T09), 157 tests across 4 spec files including
helpers/timeline/face/shared-space (4 + 27 + 16 + 110), 6.78s test
runtime, tsc --noEmit clean.
* test(e2e): apply post-T09 review nits
The post-T09 review caught one IMPORTANT and two NIT issues — all
fixed in this commit.
1. **IMPORTANT — missing afterAll in pagination describe.** The T09
commit message claimed "own beforeAll/afterAll" but the
implementation only had beforeAll. The 15 extra space-people rows
would leak into test 11 (the thumbnailPath gate) and any future
sibling test added to the parent describe. T11 still passes (it
asserts NOT contain), but the leak violates the T02 fixture
lifetime contract and would bite T10+. Added afterAll with
`DELETE FROM shared_space_person WHERE id = ANY($1::uuid[])` over
the captured ids.
2. **NIT — test 11 used a JOIN query for globalPersonId.** The
extended createSpacePerson helper from T02 already returns
`{globalPersonId, spacePersonId, faceId}` — no need to re-query
the database. Stored zeroThumbGlobalId in the describe scope and
dropped the 4-line JOIN.
3. **NIT — test 1 used a manual for-loop instead of forEachActor.**
The design doc explicitly called for forEachActor. The manual loop
worked but diverged from the T03+ pattern. Switched to forEachActor
with proper Actor objects, importing from src/actors. This also
sets the precedent for T10-T14 to use the helper consistently.
Verified locally: 110 tests still green in shared-space.e2e-spec.ts,
tsc --noEmit clean.
* test(e2e): T10 single space-person + thumbnail + assets
Adds 9 tests in a new nested describe inside T09's parent block,
sharing the T09 fixture setup. Covers the three read-only sub-endpoints:
- GET /shared-spaces/:id/people/:personId
- GET /shared-spaces/:id/people/:personId/thumbnail
- GET /shared-spaces/:id/people/:personId/assets
GET /people/:personId (5 tests):
1. Access matrix (owner/editor/viewer 200, non-member 403, anon 401).
2. Returns the canonical space person ID and name.
3. Hidden person IS fetchable directly — confirms half of the T09
open hypothesis: hidden filter is listing-only.
4. **Pet person is NOT fetchable directly when petsEnabled=false** —
DISPROVES the other half of the T09 hypothesis. The pet filter
applies BOTH to the listing and to the single-fetch endpoint.
This asymmetry vs hidden is real and worth pinning. UX
consequence: turning pets off in a space hides the entire pet
sub-graph, even from members who know the pet's ID.
5. **Non-existent personId returns 400 (not 404)** — bulk-access
pattern via requireAccess uniformly returns BadRequestException
for "not found OR no access" to avoid leaking existence. Same
taxonomic split as timeline. T11+ inherit this convention.
GET /people/:personId/thumbnail (2 tests):
6. Access matrix. **Member success-case is 500**, not 200, because
the fixture thumbnailPath ('/my/awesome/thumbnail.jpg' set by
utils.createSpacePerson) doesn't exist on disk. The access check
passes; the file resolution then fails and returns 500. Pinned
as a known footgun in the backlog — fixable later but out of
scope for T10. The 500 vs 404 distinction is a small server-side
bug independent of the access path.
7. Non-member 403 fires before file resolution (sanity check that
the 500 path isn't somehow leaking access).
GET /people/:personId/assets (2 tests):
8. Access matrix.
9. Returns the asset IDs containing the person — Alice is on
spaceAssetId so the response contains it.
Backlog updates:
- T10 ticked.
- The "open hypothesis" about hidden/pets at listing only is moved
to a new "Resolved hypotheses" section with the asymmetric finding
documented.
- New "Observed invariant": pet filter asymmetry (listing AND single
fetch) vs hidden filter (listing only).
- New "Observed invariant": single-person endpoints return 400 for
unknown IDs (bulk-access pattern), not 404.
- New "Known footguns" section: the thumbnail-500 issue.
Verified locally: 119 tests green in shared-space.e2e-spec.ts
(110 from T09 + 9 new T10), tsc --noEmit clean.
* test(e2e): apply post-T10 review fixes — service mechanism + thumbnail strategy
Two IMPORTANT findings from the post-T10 review, both about explanation
correctness rather than test logic.
1. **Backlog "bulk-access pattern via requireAccess" claim was wrong.**
`getSpacePerson` at shared-space.service.ts:625-636 calls
`requireMembership` (ForbiddenException for non-member, that part was
right), then runs `getPersonById` and manually
`throw new BadRequestException('Person not found')` for both the
missing-person case AND the pet-when-disabled case. The 400s the
T10 tests observe are real and load-bearing, but they come from
manual throws inside the service handler, NOT from the
`requireAccess` bulk pattern that timeline uses. Backlog
"Observed invariants" rewritten to cite the correct mechanism with
line references.
2. **Thumbnail 500 was a fixture wart, not a server bug.**
`getSpacePersonThumbnail` at shared-space.service.ts:643-657 has
THREE return paths once the access check passes:
- person not found / wrong space → NotFoundException → 404
- thumbnailPath null/empty → NotFoundException → 404
- thumbnailPath set → serveFromBackend → 200 (or 500 if missing)
The 500 in the previous T10 commit was triggered because
utils.createSpacePerson uses '/my/awesome/thumbnail.jpg' (a
non-empty path that doesn't exist on disk), which trips
serveFromBackend. The service actually has graceful 404 handling.
Restructured the thumbnail tests to exercise the **graceful 404
path**: transiently blank `person.thumbnailPath` via DB, assert
`{member: 404, non-member: 403, anon: 401}`. This pins the layered
ordering 401 < 403 < 404 — the correct member-success path for a
person with no thumbnail. Restore in try/finally.
The 200 path is not exercised because it would require pointing
the fixture at a real file in the upload location. That's a
reasonable follow-up but out of scope here.
Merged the two thumbnail tests into one (the matrix already
covers the access ordering, the second test was redundant). T10
is now 8 tests in the file (was 9), all assertions correct.
Backlog updates:
- Two "Observed invariants" rewritten to cite the manual
BadRequestException mechanism with the correct line numbers.
- New "Observed invariant" describing the three return paths of
getSpacePersonThumbnail.
- "Known footguns" entry rewritten: it's a fixture issue, not a
server bug. Mentions the follow-up to make createSpacePerson
accept a real fixture file.
Verified locally: 118 tests green in shared-space.e2e-spec.ts
(110 from T09 + 8 from T10), tsc --noEmit clean.
* test(e2e): T11 PUT/DELETE space person — rename, hide, delete
Adds 7 tests in a new nested describe inside the T09/T10 block,
covering mutation of a single space person via PUT and DELETE.
Both endpoints route through `requireRole(SharedSpaceRole.Editor)`
(verified at shared-space.service.ts:665, 704), so:
- Owner + Editor can mutate (200/204)
- Viewer is rejected (403)
- non-member is rejected (403, via requireMembership inside requireRole)
- anon (401, auth middleware)
PUT tests (4):
1. Access matrix for rename — owner+editor 200, viewer/non-member 403,
anon 401.
2. Actually renames the person — sends `{name: 'AfterRename'}` and
asserts the response body reflects the new name.
3. Marking isHidden=true hides the person from the default listing
but the direct fetch still returns 200 — pairs with T10's listing-only
hidden invariant. The PUT path is the supported way to set isHidden,
complementing T09's direct-DB-mutation pattern.
4. Non-existent personId returns 400 (manual BadRequestException at
shared-space.service.ts:668-669, same shape as T10).
DELETE tests (3):
5. Access matrix — owner+editor 204, viewer/non-member 403, anon 401.
Uses 5 different scratch persons (one per actor) to avoid
"delete-then-delete" race conditions in the matrix.
6. Preserves the underlying global person row — verifies via direct
DB query that `person` table row stays after `shared_space_person`
delete. The shared_space_person delete is correctly scoped and
does NOT cascade to the global person table.
7. Non-existent personId returns 400.
All scratch persons are created fresh per `it()` block via
utils.createSpacePerson, so the mutations are fully isolated and
don't affect T09's listing assertions or T10's read-only fixtures.
Verified locally: 125 tests green in shared-space.e2e-spec.ts
(110 from T09 + 8 from T10 + 7 from T11, 5.15s test runtime),
tsc --noEmit clean.
T11 ticked. Space-people sub-tree progress: 3/6 (T09/T10/T11 done,
T12 merge / T13 alias / T14 deduplicate remaining).
* test(e2e): T12 POST merge space persons
Adds 6 tests in a new nested describe inside the T09/T10/T11 block,
covering POST /shared-spaces/:id/people/:personId/merge.
Service shape (shared-space.service.ts:730-778): path :personId is the
*target*, body `{ids: string[]}` lists the *sources*. Requires Editor.
Validates both sides in the same space and the same type, reassigns
the source's junction rows to the target, deletes the source rows,
recounts the target's denormalised faceCount/assetCount, and queues
a dedup pass.
Tests:
1. Access matrix — owner+editor 204, viewer 403, non-member 403, anon 401.
Uses 5 separate scratch source persons (one per actor) so the
matrix doesn't try to merge the same source twice.
2. Merge reassigns the source's junction rows and deletes the source.
Verified via direct DB queries: source row gone, target now has 2
junction rows (its own face + the inherited one).
3. After merge, target's denormalised faceCount=2 and assetCount=1
(both faces are on the same asset, so distinct asset count is 1).
Pins recountPersons (shared-space.repository.ts:686+).
4. Cannot merge a person into themselves — 400 with message
matching /themselves/ from shared-space.service.ts:743-745.
5. Cannot merge across types (person target + pet source) — 400
with message matching /different types/ from line 754-756.
Pins the type-segregation invariant: pets and persons stay
separate even within the same space.
6. Missing target OR missing source returns 400. Two requests in
one test, both pinned.
Each test creates fresh scratch persons via utils.createSpacePerson
inside the `it()` block — fully isolated from T09/T10/T11 fixtures
and from sibling T12 tests.
Verified locally: 131 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12, 6.20s test runtime),
tsc --noEmit clean.
T12 ticked. Space-people sub-tree: 4/6 done (T09/T10/T11/T12). T13
alias and T14 deduplicate remaining.
* test(e2e): T13 space person alias — per-user, viewer-allowed
Adds 5 tests in a new nested describe inside the T09/T10/T11/T12 block,
covering PUT and DELETE alias.
Three critical invariants pinned by this block — the original T13
backlog row was wrong about both:
1. **Aliases are PER-USER, not visible to all members.** The service
stores `(personId, userId, alias)` and `getAlias(personId, auth.user.id)`
returns only the caller's row (shared-space.service.ts:780-798).
Owner setting "Mom" as Alice's alias is invisible to editor and
viewer. The original backlog row claimed "visible to all members"
which is the opposite of reality.
2. **Aliases require `requireMembership`, NOT `requireRole(Editor)`.**
Viewers CAN set their own aliases. Logical: aliases are personal
metadata, not space state — a read-only viewer should still be able
to label people for themselves.
3. **DELETE has no person existence check; it's idempotent on missing
personId.** Asymmetric vs PUT (which validates and returns 400).
Service code at lines 800-803.
Tests:
1. Access matrix — owner+editor+viewer 204 (all members can set their
own alias), non-member 403, anon 401.
2. Per-user persistence + isolation — owner sets "Mom" alias, owner GET
sees "Mom", editor GET sees null alias and the original 'PerUserAlice'
name. Pins both halves of the per-user invariant.
3. Alias does NOT modify global `person.name` — direct DB query
confirms the underlying `person.name` row stays at the original
value after alias is set.
4. DELETE removes the alias AND is idempotent on missing personId
(single test asserts both) — covers the asymmetry vs PUT.
5. PUT alias on missing personId returns 400 — pinned for symmetry
with T11/T12's missing-personId convention.
Backlog updates:
- T13 ticked with the corrected row description.
- New "Observed invariant" entry documenting the per-user + viewer-
allowed + idempotent-DELETE quirks.
Verified locally: 136 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13, 6.32s test
runtime), tsc --noEmit clean.
T13 ticked. Space-people sub-tree: 5/6 done. T14 (deduplicate) is
the last one before T15-T17 finish the space-libraries sub-tree.
* test(e2e): apply post-T13 review nit — tighten alias null assertions
Tests 2 and 4 used a defensive `alias === null || alias === undefined`
check, but `mapSpacePerson` always sets `alias: string | null` and never
omits the field. Tightened both assertions to `toBeNull()` so a future
regression that drops the field would be caught (the disjunction would
silently still pass on a missing field).
Other NITs from the post-T13 review (split the two-in-one DELETE test,
trim the verbose backlog row) were judgment calls — leaving as-is.
Verified locally: 136 tests still green, tsc --noEmit clean.
* test(e2e): T14 deduplicate space people — Owner-only + jobId dedup pin
Adds 4 tests in a new nested describe inside the T09/T10/T11/T12/T13
block, covering POST /shared-spaces/:id/people/deduplicate.
Service shape (shared-space.service.ts:721-728): the manual dedup
trigger requires `Owner` role (NOT Editor — distinct from PUT/DELETE/
merge), then queues a SharedSpacePersonDedup job on the
FacialRecognition queue with jobId `space-dedup-${spaceId}`
(job.repository.ts:239-241).
Tests:
1. **Owner-only access matrix** — owner 204, editor 403 (this is the
distinguishing test from T11/T12 which all only required Editor),
viewer 403, non-member 403, anon 401. Pins the role distinction.
2. Owner happy path returns 204 — sanity check on the success shape.
3. Two consecutive owner calls both return 204 — pins HTTP-level
idempotency, independent of whether BullMQ deduplicates underneath.
4. **PR #292 jobId dedup verification.** The load-bearing test for the
whole T14 task. Strategy: pause the FacialRecognition queue, empty
it, trigger dedup twice, count jobs whose data matches the test
space, restore in try/finally. The two triggers should produce
exactly ONE queued job — BullMQ's queue() with a duplicate jobId
is a no-op, so PR #292's behaviour is preserved.
Queue manipulation requires admin token (queue.controller.ts:23 —
admin: true). The `admin` token is already set up in the outer
beforeAll. The pause/restore is bracketed in try/finally so a test
failure doesn't leave the queue in a paused state and break the
rest of the suite.
Backlog updates:
- T14 ticked.
- New "Observed invariant" pinning the Owner-only role and the
jobId-based queue dedup, with the verification mechanism documented.
Verified locally: 140 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14, 5.29s
test runtime), tsc --noEmit clean.
T14 ticked. **Space-people sub-tree COMPLETE** (T09-T14, all 6 tasks).
Phase 1 progress: T01-T14 done, only T15-T17 (space-libraries
sub-tree) remaining before Phase 1 closes.
* test(e2e): apply post-T14 review fixes — DTO body + tightened assertions
Four post-T14 review findings, all addressed in this commit. The test
behaviour is unchanged (still 140/140 green) but the test logic now
matches the actual server contract instead of relying on dead body
fields and over-permissive predicates.
1. **BLOCKING — DTO body shape was wrong.**
The test's `DELETE /queues/:name/jobs` calls sent
`{statuses: [...]}`, but `QueueDeleteDto` only has `failed?: boolean`
(queue.dto.ts:24-31). The `statuses` field was silently dropped by
NestJS's whitelist validator and the underlying
`jobRepository.empty(name)` drained the queue unconditionally
anyway, so the test passed — but the body was misleading: it
implied a statuses-based filter that doesn't exist. Removed both
sends in try and finally; the empty call is now bodyless and the
intent is clear.
2. **Comment misleading.** The GET /queues/:name/jobs call applied no
filter, so it returned ALL jobs in any state, not just waiting/paused.
Updated the comment to spell that out and removed the misleading
"waiting/paused" language.
3. **NIT — try/finally hole.** The test now asserts `pauseRes.status
=== 200` immediately after the PUT pause, so a pause failure fails
loudly instead of silently letting the worker race the assertion.
4. **NIT — over-permissive filter.** The original predicate was
`j.data?.spaceId === spaceId || j.id === space-dedup-${spaceId}` —
the `||` would only widen if the jobId encoded something OTHER than
spaceId, which is exactly the kind of refactor we want to catch.
Tightened to a strict `j.data?.spaceId === spaceId` check on the
job's data payload, which is the load-bearing field we care about.
Verified locally: 140 tests still green in shared-space.e2e-spec.ts,
tsc --noEmit clean.
* test(e2e): T15 PUT space libraries — link library to space
Adds 7 tests in a new nested describe inside the T09-T14 block,
covering PUT /shared-spaces/:id/libraries.
Service shape (shared-space.service.ts:449-477) has a TWO-step gate:
1. `if (!auth.user.isAdmin)` → ForbiddenException 'Only admins can
link libraries to spaces' — admin gate, fires FIRST.
2. `requireRole(Editor)` — must be a space member with Editor or
Owner role.
Then library existence is checked (400 if missing). On success,
addLibrary returns null for duplicate (spaceId, libraryId) and skips
the face-sync queue — making the operation idempotent at HTTP level.
Tests:
1. **Non-admin owner of the space cannot link** — pins the admin
gate firing BEFORE role check. Even the space owner gets 403 if
they're not also a global admin. Asserts the message matches /admins/.
2. Non-admin editor cannot link — same admin gate.
3. Non-admin viewer cannot link — same.
4. Anon → 401.
5. **Admin who is an Editor in the space CAN link** — happy path. The
block's beforeAll adds the global `admin` user as an Editor in the
T09 test space.
6. **Idempotent on duplicate link** — calling link twice with the
same library returns 204 both times, and the shared_space_library
table has exactly 1 row for the (spaceId, libraryId) pair. Pins
the "204, not 409" behaviour explicitly.
7. **Library not found returns 400** — pins the existence check
message.
Setup creates two libraries via utils.createLibrary (admin token,
admin.userId as owner). The second library is used for the success
test so the idempotency test has a clean slate on the first library.
Backlog updates:
- T15 ticked with the corrected description (was "409 on duplicate" —
the actual behaviour is 204 idempotent).
- New "Observed invariant" pinning the two-step gate ordering and the
idempotent duplicate behaviour, with line references.
Verified locally: 147 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15,
5.35s test runtime), tsc --noEmit clean.
T15 ticked. Space-libraries sub-tree: 1/3 done. T16 (unlink) and T17
(link side effects) remaining before Phase 1 closes.
* test(e2e): T16 DELETE space libraries — unlink, idempotent
Adds 5 tests in a new nested describe inside the T09-T15 block,
covering DELETE /shared-spaces/:id/libraries/:libraryId.
Service shape (shared-space.service.ts:479-487) — same two-step gate
as T15:
1. `if (!auth.user.isAdmin)` → ForbiddenException 'Only admins...'
2. `requireRole(Editor)`
Then repository.removeLibrary(spaceId, libraryId) is a plain DELETE
on the (spaceId, libraryId) pair (shared-space.repository.ts:220-226)
with NO row-existence check. So unlink is idempotent at the HTTP
level: deleting an already-unlinked link, or deleting with a bogus
libraryId, both return 204.
Tests:
1. Non-admin owner cannot unlink (admin gate fires first, message
matches /admins/i).
2. Anon → 401.
3. **Admin Editor CAN unlink + DB row count goes 1 → 0.** Pre-checks
the row exists, calls unlink, then verifies the row is gone via
direct DB query. Pins the actual mutation.
4. **Unlinking an already-unlinked library is idempotent.** Reuses
scratchLibrary which test 3 just unlinked, calls again, expects
204.
5. **Unlinking with a non-existent libraryId returns 204.** Pins the
"no existence check" behaviour — even a bogus UUID is a no-op
DELETE → 204, NOT 404 (the original design row was wrong about
this).
Setup creates `scratchLibrary` per beforeAll and pre-links it via
the API so test 3 has a real link row to remove.
Backlog updates:
- T16 ticked with the corrected description (was "non-existent link
→ 404" — actual is 204 idempotent, no existence check).
Verified locally: 152 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15
+ 5 T16, 5.31s test runtime), tsc --noEmit clean.
T16 ticked. Space-libraries sub-tree: 2/3 done. T17 (link side
effects) is the last Phase 1 task before the entire phase closes.
* test(e2e): T17 library link side effects — closes Phase 1
Adds 6 tests in a new nested describe inside the T09-T16 block, the
final task of the space-libraries sub-tree and the last task of the
entire Phase 1 of the e2e coverage backlog.
T17 exercises the cross-table query path that link/unlink enable:
library assets becoming visible to space members via /timeline/bucket
?spaceId=. PR #163 was specifically about this code path. Setup
uploads an admin asset normally and UPDATEs its libraryId via direct
DB to associate it with a fresh library — bypassing the fragile
library scan path while exercising the same JOIN.
Tests:
1. **After link, a non-owner space member sees the library asset via
/timeline/bucket?spaceId=** — the load-bearing PR #163 invariant.
Editor (who owns no assets) calls the bucket query for the space
and the library asset appears via the shared_space_library JOIN.
2. **Viewer sees it too** — symmetric assertion for the read-only role.
3. **After unlink, library assets are no longer visible** — round-trip
pin: link → see → unlink → don't see.
4. **Soft-deleted library asset is hidden** via the `deletedAt IS NULL`
filter on the timeline query. Mutate `asset.deletedAt` to NOW(),
verify hidden, restore.
5. **Offline library asset IS still visible (NOT hidden)** — SURPRISING
FINDING. asset.repository.ts:835-849 joins shared_space_library on
(libraryId, spaceId) but does NOT filter on asset.isOffline. So a
library asset whose underlying file went offline is still listed in
the space's timeline bucket. The asymmetry vs `deletedAt` filtering
is real. Test pins the actual behavior — if a future change adds
the missing isOffline filter, the test fails and forces a deliberate
update. The access.repository's checkSpaceAccess in a different
code path DOES filter on isOffline=false, so the timeline path is
the inconsistent one.
6. **Library delete eventually cascades to shared_space_library** —
library.service.ts:370-379 is a SOFT delete: it sets `deletedAt`,
queues a LibraryDelete job, returns 204 immediately. The cascade
happens async when the job processes. Test calls
waitForQueueFinish('library') after the DELETE before asserting
the FK row is gone.
Setup uses a DB-direct approach (createAsset + UPDATE libraryId)
instead of the library scan helper because the scan path was hitting
unexplained timing/timeout issues — bypassing the scan keeps the
test focused on the JOIN behavior, which is what T17 actually probes.
Backlog updates:
- T17 ticked.
- New "Observed invariant": timeline spaceId query lacks the
isOffline filter on library assets — pinned with file:line and
the asymmetry called out.
- New "Observed invariant": library delete is soft + async cascade —
tests must wait for the LibraryDelete job to drain before asserting
the FK cascade.
Verified locally: 158 tests green in shared-space.e2e-spec.ts
(99 existing + 11 T09 + 8 T10 + 7 T11 + 6 T12 + 5 T13 + 4 T14 + 7 T15
+ 5 T16 + 6 T17, 5.50s test runtime), tsc --noEmit clean.
T17 ticked. **Phase 1 COMPLETE** — all 17 tasks (T01-T17) done.
* test(e2e): revert T01 duplicate spec move — upstream-broken, leave for upstream
Revert the file move from T01 (f19eb16bc). The original task moved
e2e/src/api/specs/duplicate.e2e-spec.ts → e2e/src/specs/server/api/
so the vitest glob would pick it up. T01's commit message said
"audit found zero violations" — but the audit was for waitForQueueFinish,
NOT spec correctness. I never actually ran the spec to verify it works
after the move.
The full Phase 1 review surfaced that the spec is upstream-broken:
- Added in upstream PR #25316 (2026-03-26) by @Phlogi as part of the
"feat(server): resolve duplicates" feature.
- Upstream PR #25856 (Feb 10, 2026, by @minidzelis) had restructured
e2e/src/api/specs/ → e2e/src/specs/server/api/ six WEEKS earlier.
- PR #25316 didn't notice and added the new spec in the OLD path.
The file went into the wrong directory and was never picked up by
the vitest glob in upstream's CI either.
- A subsequent upstream PR changed the response shape of
POST /duplicates/resolve from `{status, results: [{duplicateId, status}]}`
to a bare `BulkIdResponseDto[]`. The never-running spec didn't get
updated. 18 of 21 tests now fail against the current API.
- The 3 tests that DO pass are access-matrix style assertions that
are robust to body shape changes.
This is upstream's bug. Maintaining a fork-local fix would generate
merge conflicts every rebase. The right fix lives upstream — file an
issue / PR there.
Reverting the move puts the file back where upstream put it. Our
vitest glob doesn't pick it up, so our CI is unaffected by upstream's
broken tests. When upstream eventually fixes the spec (either fixes
the assertions or re-locates the file or both), we inherit the fix
cleanly via the next rebase.
Backlog updates:
- T01 row rewritten to reflect that only the audit landed (not the
file move) and to point at the new T-cleanup-01 follow-up.
- New "Upstream cleanup tasks" section before the Decision log,
with T-cleanup-01: bring upstream duplicate.e2e-spec.ts up to date
with the current /duplicates/resolve API and PR upstream.
Estimated 2-4 hours of mechanical assertion rewriting.
Verified locally: the 4 working specs (helpers + timeline + face +
shared-space) still pass 205/205 across 3 consecutive runs after the
revert. tsc --noEmit clean. No regressions to our coverage.
* test(e2e): T18 gallery-map filter access matrix + filters
Adds e2e/src/specs/server/api/gallery-map.e2e-spec.ts with 12 tests
covering the fork-only `/gallery/map/markers` controller. This is the
filtered map endpoint distinct from `/map/markers` — accepts a rich
query (people, tags, EXIF, dates, favorite, country/city) used by the
web map view's filter panel.
Service shape (shared-space.service.ts:561-585):
- Without spaceId: scoped to auth.user.id
- With spaceId: requireAccess(SharedSpaceRead) → 400 for non-member
- personIds re-route to spacePersonIds when spaceId is set (same DTO
field, different semantics)
- Always filters visibility=Timeline regardless of client input
Tests:
1. Auth required (anon → 401)
2. Authenticated user with no filters returns their geotagged assets
3. Empty array for a fresh user with no uploads
4. country filter narrows correctly (matching + non-matching cases)
5. city filter narrows correctly (matching + non-matching cases)
6. isFavorite=true excludes non-favorite assets
7. takenAfter cutoff in the future excludes the asset
8. takenBefore cutoff in the past excludes the asset
9. rating outside 1-5 returns 400 (DTO @Min/@Max validation)
10. type with invalid enum value returns 400
11. Archived assets are excluded — service hardcodes visibility=Timeline.
Test toggles the asset to archive and verifies the marker disappears.
12. Cross-user isolation — another user does not see this user's markers
(without spaceId, the service scopes to auth.user.id).
Setup uses the existing thompson-springs.jpg fixture (real GPS in
Colorado, USA, with camera EXIF) so the metadata-based filters have
real data to match against. Pattern matches the existing /map e2e
spec for fixture upload + websocket wait.
T19 will cover spaceId scoping and the personIds → spacePersonIds
re-routing as a separate task per the backlog.
Verified locally: 12 tests green (986ms test runtime), tsc --noEmit
clean.
T18 ticked. Phase 2: 1/5 done.
* test(e2e): T19 gallery-map spaceId scoping
Adds 6 tests in a new nested describe inside the T18 block, covering
the spaceId code path in /gallery/map/markers.
Setup creates a fresh space owned by `user`, adds a second member
(spaceMember), and adds the geotagged fixture asset to the space so
it should appear in space-scoped queries. spaceNonMember is created
but never added to the space.
Tests:
1. Non-member gets 400 (requireAccess BadRequestException at
shared-space.service.ts:563). Same taxonomy as T03 timeline —
the bulk-access pattern returns 400 not 403.
2. Anon → 401.
3. Space member sees the space asset via spaceId — the load-bearing
read invariant for space-scoped map queries.
4. Space owner with spaceId sees space-scoped content (the assertion
would catch a regression that returned the full owner library
instead of the space subset).
5. Non-existent spaceId returns 400 — bulk-access pattern (no 404).
6. country filter composes with spaceId — `country=Antarctica`
returns empty even though the asset would otherwise be visible.
PR #202's "hidden persons exclusion on gallery-map" angle is deferred
to a follow-up — it would need a shared_space_person fixture with
isHidden=true similar to T09's setup, which is more involved than the
T19 scope.
Verified locally: 18 tests green in gallery-map.e2e-spec.ts (12 T18
+ 6 T19, 898ms test runtime), tsc --noEmit clean.
T19 ticked. Phase 2: 2/5 done.
* docs(plans): mark T20 N/A — /map/markers has no spaceId support
T20 was originally planned as "space scoping extension to map.e2e-spec.ts"
but the upstream /map/markers endpoint has no spaceId support at all:
- server/src/dtos/map.dto.ts:29-47 — MapMarkerDto has no spaceId field
- server/src/services/map.service.ts:9-27 — getMapMarkers has no spaceId
branch (only userIds + albumIds)
- server/src/repositories/map.repository.ts — no spaceId references
Space-scoped map queries are entirely on the fork-only /gallery/map/markers
endpoint that we just covered in T18+T19. T20 is moot.
Marked the row as N/A in the backlog with the verification line refs so
a future maintainer doesn't reattempt the task.
Phase 2 is now effectively a 4-task group (T18/T19/T21/T22), with T20
crossed out.
* test(e2e): T21 view folder browsing tests
Adds e2e/src/specs/server/api/view.e2e-spec.ts with 9 tests covering
the /view controller's two folder-browsing endpoints.
Service shape (view.service.ts:7-16): both endpoints strictly scope
to auth.user.id. No partner sharing, no space scoping, no library
joins. The folder browse is owner-only.
Tests:
GET /view/folder/unique-paths (3):
1. Auth required (anon → 401)
2. Authenticated user gets their unique folder paths (non-empty array)
3. Cross-user isolation — userB's response does not contain any of
userA's paths that include userA's userId. The actual upload paths
include the user UUID, so this is a structural assertion that no
leak path exists between users.
GET /view/folder (5):
4. Auth required
5. Returns assets when given a known path from the user's folder list
(resolves a path via /unique-paths first, then queries /folder).
6. Empty array for a non-existent path
7. Cross-user isolation — userB calling /folder with userA's path
does NOT see userA's asset in the result. Pins that the service
really does scope by auth.user.id even when the caller-supplied
path matches another user's folder structure.
8. **Missing `path` query param returns 500** — REAL FINDING. The
controller at view.controller.ts:33 declares `@Query('path') path:
string` with no validation pipe and no default. The service passes
undefined to the repository which trips with a 500. Pinning the
actual behavior so a future server-side fix forces a deliberate
update. Worth filing upstream as a small server bug — should be
400 with a clear validation message.
Plus a small sanity assertion (9) that touches the userBAssetId
fixture so the linter doesn't flag it as unused.
T20 was N/A so this is the next Phase 2 task. T22 (workflow) is the
last one before Phase 3.
Verified locally: 9 tests green (404ms test runtime), tsc --noEmit
clean.
T21 ticked. Phase 2: 4/5 done (T20 was N/A).
* test(e2e): T22 workflow CRUD access matrix — closes Phase 2
Adds e2e/src/specs/server/api/workflow.e2e-spec.ts with 15 tests
covering the fork-only /workflows controller. Last task in Phase 2.
Service shape (workflow.service.ts):
- create: validates triggerType + per-plugin filter/action IDs (400 on bad ID)
- getAll: scoped to auth.user.id (owner-only)
- get/update/delete: requireAccess(WorkflowRead/Update/Delete) → 400 for non-owner
- update with no fields → BadRequestException('No fields to update')
- getAll cross-user returns empty array (not 403) — owner-scoped query
Workflows are per-user with no sharing concept. Cross-user access is
uniformly rejected at the access layer.
Tests:
POST /workflows (5):
1. Auth required (anon → 401)
2. Create with empty filters and actions succeeds (201, returns
workflow with ownerId, default enabled=true, AssetCreate trigger)
3. Invalid trigger type returns 400
4. Invalid pluginFilterId returns 400 with /filter/i message
5. Empty name returns 400 (DTO @IsNotEmpty)
GET /workflows (2):
6. Auth required
7. Owner-scoped — userA's workflows visible to userA, userB sees
empty array
GET /workflows/:id (3):
8. Owner can fetch their own workflow
9. Cross-user GET returns 400 (requireAccess bulk-access)
10. Non-existent workflow ID returns 400 (not 404)
PUT /workflows/:id (3):
11. Owner can rename
12. Empty PUT body returns 400 with /no fields/i message — pins the
explicit "No fields to update" service guard
13. Cross-user PUT returns 400
DELETE /workflows/:id (2):
14. Owner can delete + subsequent GET returns 400 (workflow gone)
15. Cross-user DELETE returns 400
Verified locally: 15 tests green (489ms test runtime), tsc --noEmit
clean.
T22 ticked. **Phase 2 COMPLETE** — T18/T19/T21/T22 done; T20 was N/A.
Phase 2 added 42 tests across 3 new spec files (gallery-map, view,
workflow). Total branch coverage: 247 e2e tests (Phase 1 = 205 + Phase
2 = 42).
* test(e2e): apply post-Phase-2 review nits
Three NITs from the Phase 2 review, all addressed:
1. **gallery-map T19 — missing personIds re-routing test.** The
service at shared-space.service.ts:569-570 re-routes
`dto.personIds → spacePersonIds` when `spaceId` is set, making
the same DTO field mean different things in different contexts.
Added a 7th test in the T19 nested describe that probes the
re-routing: passing a bogus person UUID with spaceId returns
empty (the lookup goes through shared_space_person, not asset_face).
Not the strongest possible assertion, but pins the behavioural
shape so a future refactor that changes the re-routing direction
would be caught.
2. **view T21 — dead sanity-check test.** The original spec had
`expect(typeof userBAssetId).toBe('string')` only to satisfy
the linter about the unused `userBAssetId` fixture. Removed both
the variable assignment and the dead test. The cross-user-isolation
tests already exercise userB by querying with their token; we
don't need their asset id specifically. Drop both, keep the
second createAsset call so userB has folder content.
3. **gallery-map backlog row count.** T19 said "6 tests" but the
actual count is 7 with the new re-routing test. Updated the row.
Verified locally: 27 tests green across gallery-map (19) + view (8),
tsc --noEmit clean.
* test(e2e): T23 asset metadata K/V CRUD
Adds e2e/src/specs/server/api/asset-metadata.e2e-spec.ts with 13
tests covering the asset metadata K/V endpoints (PUT /:id/metadata,
GET /:id/metadata, GET/DELETE /:id/metadata/:key, bulk PUT/DELETE
/assets/metadata).
All routes use Permission.AssetRead/AssetUpdate which routes through
requireAccess (bulk-access pattern → 400 for non-owner).
Tests:
GET /assets/:id/metadata (3):
1. Auth required (anon → 401)
2. Owner can list (empty initially)
3. Non-owner returns 400 (bulk-access)
PUT /assets/:id/metadata (3):
4. Owner upsert + value is queryable via the listing
5. Upsert overwrites an existing key value (set v=1, set v=2,
verify single-key fetch returns v=2)
6. Non-owner upsert returns 400
GET /assets/:id/metadata/:key (2):
7. Owner can fetch a single key set in test 4
8. Missing key returns 400 or 404 (test pins both — the actual
path uses requireAccess + service throw, behavior pinned)
DELETE /assets/:id/metadata/:key (2):
9. Owner can delete a key and it's removed from the listing
10. Non-owner delete returns 400
PUT /assets/metadata (bulk) (2):
11. Owner can upsert across multiple of their own assets in one call
12. Bulk upsert with a non-owner asset id mixed in is rejected (400)
DELETE /assets/metadata (bulk) (1):
13. Owner can bulk-delete keys across multiple of their own assets
Verified locally: 13 tests green (554ms test runtime), tsc --noEmit
clean.
T23 ticked. Phase 3: 1/6 done.
* test(e2e): T24 asset OCR endpoint access mat…
4 tasks
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
5 participants
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.
Description
This PR moves duplicate metadata synchronization logic from the frontend to the backend, making it more robust and enabling proper batch operations. This is an improved and refactored version of #13851.
What's new:
/duplicates/resolveendpoint handles multiple duplicate groups in a single request/duplicates/stackendpoint to organize duplicates as stacks instead of deleting themasset_exif.tagsand the property is locked, so subsequent metadata re-extraction (sidecar write → extract metadata) does not overwrite the merged tag setdistinctLockedmechanism without redundant wrappersNot included (planned for follow-up):
POST /duplicates/resolve/bulkwith job queue support for processing all duplicate groups in the background.How Has This Been Tested?
Screenshots (if appropriate)
API Changes
New endpoints:
POST /duplicates/resolve- Batch resolve with metadata sync and optional trash/deletePOST /duplicates/stack- Create stack from duplicate group assetsModified endpoints:
GET /duplicates- Now includessuggestedKeepAssetIdsfield in responseChecklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc. - DuplicateService uses repositoriessrc/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/) - Only data accessAdditional external automated python test cases
Click to see the cases and reasoning
Duplicate Test Cases
This document enumerates every duplicate merge test set. Each set groups multiple assets for user Eve where one
or more assets are marked keep and the rest are marked trash to exercise merge
behavior. Each table lists the candidate assets and the expected result.
Total expected duplicate sets: 33.
Favorites (synchronizeFavorites)
Fields tested: favorite
Rating (synchronizeRating)
Fields tested: rating
Tags (synchronizeTags)
Fields tested: tags
Description (synchronizeDescription)
Fields tested: description (unique non-empty, trimmed)
Location (synchronizeLocation)
Fields tested: location
Albums (synchronizeAlbums)
Fields tested: album membership
Combined (multiple sync options)
Fields tested: favorite, rating, description, location, album
Multi-Keep (multiple keepers)
Fields tested: merged metadata applies to every kept asset
Note: Hidden assets are excluded because they do not show up in the duplicate review UI.
Screenshots
Please describe to which degree, if any, an LLM was used in creating this pull request.
Claude Code assisted with refactoring the sync logic to the backend, writing test cases, and cleaning up the frontend code. All logic was manually reviewed and tested.