fix(people): make "Fix incorrect match" work in shared spaces (#765) - #811
Open
Deeds67 wants to merge 22 commits into
Open
fix(people): make "Fix incorrect match" work in shared spaces (#765)#811Deeds67 wants to merge 22 commits into
Deeds67 wants to merge 22 commits into
Conversation
Deeds67
force-pushed
the
worktree-fix-765-space-editor-face-reassign
branch
from
July 21, 2026 20:24
2eb1fcc to
2a7231c
Compare
Deeds67
force-pushed
the
worktree-fix-765-space-editor-face-reassign
branch
from
July 30, 2026 07:22
4946b15 to
65d1506
Compare
Reassigning a face appeared to succeed but the photo reappeared under the original person. Two independent defects, both blocking a non-admin Editor. Stale space projection. reassignFaces/reassignFacesById wrote asset_face .personId and face_identity_face, but every space-scoped person view reads the materialized shared_space_person_face projection, which was never refreshed. The write landed, nothing errored, and the UI re-read the stale row — so the correction looked like a no-op. The recognition path already queues SharedSpaceFaceMatch for exactly this reason; the reassign paths queued nothing. Now the stale assignment is evicted synchronously, so the face leaves the wrong person immediately, and the match job is queued to re-project it under the corrected one. Eviction is gated on the job's own isAssetInSpace guard: getSpaceIdsForAsset is broader (it ignores deletedAt/isOffline/visibility), so evicting on it alone would drop faces the job then refuses to re-add. The vacated space person is recounted and reaped, mirroring processSpaceFaceMatch. Owner-only permissions. Both gates were owner-scoped — PersonUpdate on the target and PersonCreate on the face — so an Editor correcting a face on another member's asset was rejected outright. Reassign now mirrors updateRepresentativeFace: owner fast path, then checkSharedSpaceEditAccess for the target person, and Permission.AssetUpdate (already owner-or-space- Editor) for the face's asset. Viewers hold PersonRead only and stay denied. Resolving the face before its access check means an unknown id would have surfaced as a Kysely error; NoResultError is mapped back to a 400 so it stays indistinguishable from an unauthorized face, while real database failures still propagate. Ordering is load-bearing: the match job resolves the target space person from face_identity_face, so the refresh must run after replaceFaceIdentity or the job would re-add the old person and silently restore this bug.
…gePropagationService (#765)
…sForSpacePersonAssets (#765) Review findings on the Slice 1 projection query: - IMPORTANT: bring the query to guard parity with the sibling getSpaceRepresentativeFaceForUpdate/getSpaceRepresentativeFaces — add asset.isOffline, asset.visibility (visibleSpaceAssetVisibilities), and the direct/library/album space-scope existence OR-group (derived by joining shared_space_person for spaceId). Without this a stale shared_space_person_face row could make an offline / non-space-visible / removed-from-space asset's face resolvable and reassignable by an editor. - MINOR: add a test proving the personId equality filter itself (a face projected onto a sibling shared_space_person in the same space must not leak into the source person's results), not just that the inner join is required. - MINOR: bind the empty-assetIds early return to the same inferred return type as the query branch via an explicit method return-type annotation, removing the hand-duplicated inline type. Regenerated src/queries/shared.space.repository.sql accordingly.
…trap guard (#765) Review hardening on Slice 2's reassignSpaceFacesToTarget: pin the identity-relink call args so a regression to the face's OLD person (or wrong source) can't slip through green tests, switch the space-person resolution branch onto its type discriminant instead of an id's truthiness so a falsy resolved identity throws instead of silently falling through to the #765 duplicate-space-person trap, and give newly created persons a feature face at creation (matching the create({ faceAssetId }) convention elsewhere). Strengthens the ordering/refresh/empty-list/batch-hoist tests to actually detect phase-batching, single-refresh-per-batch, and per-face ensureSpacePersonIdentity regressions.
reassignSpaceFacesToTarget set faceAssetId on both create() calls but never queued PersonGenerateThumbnail, unlike every other faceAssetId- at-creation site in the codebase. Newly created persons rendered avatar-less on the People page since thumbnailPath stayed ''. Also: distinct owners in the batch-hoist test so it can't be satisfied by resolving the identity inside the per-owner branch; a message matcher on the falsy-identity guard's rejects.toThrow(); and cached !== undefined instead of truthiness for the per-owner cache lookup.
- Separate the misassigned asset's owner from the space creator (4th fixture user) so AC1's/AC2's ownership assertions can't pass vacuously when production misattributes to the space owner instead of the asset owner. - Add pre-condition assertions before AC1 and the two-space reassign calls so the post-call empty-projection checks can't pass vacuously. - Add a positive control on AC1's getSpacePersonFaces re-read. - Switch AC2's owner-aligned person lookup to .execute() + length check so a wrongly-minted duplicate can't be masked non- deterministically. - Assert AC2's synchronous eviction, symmetric with AC1/two-space. - Pin the reap test's rejection to the "Person not found" message. - Add Vitest assertion messages to the #765-critical expectations.
…ating #765 (#765) Slice 4 review findings: getAllPeople was requesting withSharedSpaces unconditionally, so a normal owned-person source also saw shared-space candidates whose id would be sent to the personal reassignFaces branch on selection — the same id-mismatch #765 fixed, in reverse. Scope the flag to space-scoped sources only. Also gate onConfirm() on reassigned > 0 in both handlers: a zero-result reassign was still firing the caller's optimistic asset removal, so the UI showed a danger toast and emptied the grid of photos that never moved. Throw-path behaviour (onConfirm still fires after a caught error) is unchanged. Plus the review's minor fixes: give reassignInSpace's spaceRef parameter instead of `!`-asserting the derived value inside it, fix two fixtures that couldn't distinguish primaryProfile.id from person.id, and tighten several test assertions (missing reassignFaces-not-called checks, an untested handleReassign zero-result branch, and toast-content assertions instead of toast-was-called-at-all).
Four targeted fixes on the "Fix incorrect match" flow, from the whole-branch review: - The local `target` type in UnmergeFaceSelector contradicted its own generated DTO: string literals cannot satisfy the `Type3`/`Type4` string-enum members `SharedSpacePersonReassignDto['target']` uses. CI was blind to it (`check-svelte` runs with `--no-tsconfig`, `tsc` never reads `.svelte`), and runtime was unaffected since enums erase to the same strings — but the file disagreed with the API it calls. Now typed as the DTO's own member and constructed from the generated enums, like `scoped-person-ref.ts` already does. - A thrown reassign still fired `onConfirm()`, which drives the caller's optimistic `timelineManager.removeAssets` — so a failure showed a danger toast AND vanished the photos until refresh: #765's symptom relocated. Slice 4 fixed this for the zero-result case only. Both catch blocks now clear `shouldConfirm`. - `assetIds` is capped at 100 server-side while "Select all" sits on the same toolbar unbounded, so a >100 selection 400'd outright. The space-path helper now chunks into batches of <=100, issued sequentially with the server-reported counts summed; any chunk throwing fails the whole operation. - "Fix incorrect match" was not gated by `canEditSpacePerson`, unlike the rename/birthday/merge actions, so a space viewer was offered an action that 403s. Server-side enforcement was already correct; this is the UI affordance.
…765) The space reassign path skipped both feature-photo repairs the global path performs (PersonService.reassignFaces / reassignFacesById): (a) when the moved face WAS the source person's `faceAssetId`, that person's owner — often a different space member — kept seeing a face that now belongs to someone else as their person's avatar; (b) an existing-but-empty target (resolved via `getPersonByIdentity`, or a global `person` target) gained the face with no feature photo at all. Nothing repairs a stale `person.faceAssetId` in the background, so both were permanent. `reassignSpaceFacesToTarget` now collects the affected person ids during the loop — reading the source person BEFORE `reassignFace` rewrites `asset_face.personId`, and remembering the answer per person rather than merely noting the lookup, so a feature face that is the batch's second face still counts — and applies the repairs after the loop, mirroring the global path's `createNewFeaturePhoto` (re-point + queue PersonGenerateThumbnail; a person with no eligible face left keeps its stale pointer, same as the global path). This is what the previously-unread `personId` on the projection query is for. Space-side sibling: `removePersonFaceAssignmentsForSpaceFace` did not clear a `shared_space_person.representativeFaceId` pointing at the evicted face, and neither repair helper covers it (`repairInvalidRepresentativeFaces` only inspects manual picks, `repairOrphanedRepresentativeFaces` only fills NULLs) — so the source space person's avatar could keep showing the face that just left it. It now re-picks after the delete, degrading a no-longer-valid manual pick to 'auto' exactly as `repairInvalidRepresentativeFaces` does.
Two defects made "Fix incorrect match" unusable in a shared space even after
the endpoint itself worked:
Avatars: FaceThumbnail resolved every candidate through the owner-only
`GET /people/{id}/thumbnail`. A space person has no row in the owner-only
person table, so that endpoint 400s for it and the whole picker rendered as
"Error loading". Route through getGlobalPersonThumbnailUrl, which picks the
membership-gated `GET /shared-spaces/{spaceId}/people/{id}/thumbnail` for a
space profile and falls through to the owner-only URL for personal people.
Candidates: the picker loaded getAllPeople({ withSharedSpaces: true }), which
spans every space the viewer belongs to and collapses each identity to a
single primary profile. That offered targets the endpoint can only reject with
"Target person not found in this space", and hid the in-space profile of any
identity whose primary profile lives in another space. Ask the space for its
own people instead (getSpacePeople), shaping each into the PersonResponseDto
the picker renders with a space-scoped primaryProfile.
The personal (owned-person) path is unchanged and still sees own people only.
…mes (#765) GET /api/people/{id} used the requested profile id only as a lookup key for the identity, then re-picked a profile from scratch via primary_rn. For an identity with profiles in two spaces the caller can see, that meant /people/{fv-profile} could resolve to the Photography Club profile — and since rename, birthday, merge and reassign all take their space from primaryProfile, every write on that page targeted a space the caller never asked for. It was also unstable: updatedAt DESC is one of the tiebreakers, so a rename or a projection refresh could move the addressed space under the user, and the final tiebreak is a raw UUID comparison. Thread the requested profile into hydrateAccessiblePeople and rank it directly below the caller's own person. Ordering the caller's own person first is deliberate: an owner who deep-links a space profile of an identity they also hold personally keeps their own page, which shows their whole library rather than one space's slice and is the only one they can always edit. The list paths pass no requested profile, so their ranking is unchanged — the new branch only refines a tiebreak that was previously arbitrary. This does not change that the person page still loads photos identity-wide while writes stay space-scoped; reconciling those is a separate change.
Deeds67
force-pushed
the
worktree-fix-765-space-editor-face-reassign
branch
from
July 30, 2026 08:24
65d1506 to
c73cb82
Compare
#765 replaced two owner-only upstream gates on reassignFaces / reassignFacesById with fork helpers: Permission.PersonUpdate (person.checkOwnerAccess) -> requireReassignTargetAccess: owner, else person.checkSharedSpaceEditAccess Permission.PersonCreate (person.checkFaceOwnerAccess) -> requireReassignFaceAccess: face owner, else Permission.AssetUpdate Both are supersets of what they replaced: each ORs in exactly one extra predicate, and both of those require a shared_space_member row with role owner|editor. Record that as tests rather than as a reading of the diff. Unit (person.service.spec.ts), against the mocked access repository: both entry points refuse the any-role PersonRead grant on the target person and the any-role space / partner / album grants on the face's asset, refuse an admin at each gate, and reject a denied target before the face row is read at all. Two bulk-path rows pin that the face gate runs once per face and that a refusal aborts the batch (non-atomic, as upstream). Medium (people-identity-rbac.spec.ts), against the real access repository so the role predicates in checkSharedSpaceEditAccess / checkSpaceEditAccess are actually exercised: space Owner and Editor may reassign a face on another member's asset; Viewer, non-member, admin non-member and an unrelated user may not, and nothing moves; an Editor still cannot touch a face whose asset is outside any space they edit, including one in a different space they are not a member of. Every allow row uses an actor who owns neither the target person nor the asset, so none of them can pass through an owner fast path. One row is characterisation rather than a requirement: the two gates are evaluated independently, so an Editor of two spaces can move a face from one onto a person they only reach through the other. The picker's same-space scoping is client-side; making it a server invariant would now be a visible change. Also gates the deep-link resolution the branch added: a requested space profile is only resolved for a caller with a membership in that profile's space, at both the repository and the service edge.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #765.
A non-admin Editor in a Shared Space selects misassigned photos, runs Fix incorrect match, reassigns them — the flow completes with no error, and the photos immediately reappear under the original person.
Root cause
The issue title points at permissions, but the reported symptom is a different defect. There are two, and both independently block that user.
1. Stale space projection — this is the silent no-op
reassignFaces/reassignFacesByIdwroteasset_face.personIdandface_identity_face, but every space-scoped person view reads the materializedshared_space_person_faceprojection (e.g.shared-space.repository.ts:2588), which was never refreshed.So the write landed, nothing threw, and the UI re-read a stale row — the correction looked like it had no effect. The recognition path already queues
SharedSpaceFaceMatchfor exactly this reason (person.service.ts:1035,1134); the reassign paths queued nothing.The stale assignment is now evicted synchronously, so the face leaves the wrong person immediately, and the match job is queued to re-project it under the corrected person.
Two subtleties that make this safe:
isAssetInSpaceguard.getSpaceIdsForAssetis broader — it ignoresdeletedAt/isOffline/visibility. Evicting on it alone would remove a face for a trashed/offline/hidden asset that the job then refuses to re-add, deleting it from the space entirely — worse than the original bug.replaceFaceIdentity. The match job resolves the target space person fromface_identity_face; evicting before the identity swap would re-add the old person and silently restore this bug. Pinned by a test and a comment.The vacated space person is recounted and reaped, mirroring
processSpaceFaceMatch.2. Owner-only permissions
Both gates were owner-scoped —
PersonUpdate→checkOwnerAccessandPersonCreate→checkFaceOwnerAccess(access.ts:328,315) — so an Editor correcting a face on another member's asset was rejected outright.Reassign now mirrors the existing
updateRepresentativeFacepattern: owner fast path, thencheckSharedSpaceEditAccessfor the target person, andPermission.AssetUpdate(already owner-or-space-Editor) for the face's asset. Viewers holdPersonReadonly and stay denied.Resolving the face before its access check meant an unknown id would surface as a Kysely error, so
NoResultErroris mapped back to a400— indistinguishable from an unauthorized face — while real database failures still propagate.Tests
TDD throughout: every test was watched fail first, for the right reason.
17 unit tests (
person.service.spec.ts) covering per-space evict+queue, evict-before-queue ordering, duplicate-space dedup, no-space assets, the bulk path, theisAssetInSpaceguard, recount/reap, identity-before-evict ordering, Editor allowed / Viewer denied on both target and face, unknown face id, and DB errors not being masked.4 medium tests against a real Postgres (
people-identity-rbac.spec.ts) asserting actual projection rows: the row exists under the wrong identity, is gone before any job drain, and lands under the correct identity after. Plus Editor-allowed, Viewer-denied, and no-space.The negative tests pass pre-fix by design — they exist to catch an over-permissive fix.
Red-validated: reverting the refresh calls fails the medium test with the stale row still under the wrong person.
No client change needed — eviction is synchronous, so the existing refresh shows the corrected state right away.