feat(face-cleanup): full per-face resolution — 5 terminal states, resolve endpoint, resolutions page (supersedes #664)#770
Open
Deeds67 wants to merge 33 commits into
Open
feat(face-cleanup): full per-face resolution — 5 terminal states, resolve endpoint, resolutions page (supersedes #664)#770Deeds67 wants to merge 33 commits into
Deeds67 wants to merge 33 commits into
Conversation
github-actions
Bot
added
documentation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Deeds67
added a commit
that referenced
this pull request
Jul 15, 2026
…rvive merge, move-and-lock, dismiss drain (stacked on #770) (#773) * docs(face-cleanup): temporal-consistency hardening design (lock/decline survive merge, move-and-lock, dismiss drain, scoped load) * docs(face-cleanup): spec-review fixes — executeRepair moved-ids for move-lock, defer person-dismiss-merge, txn-handle + decline personId-null notes * docs(face-cleanup): consistency-hardening implementation plan (4 slices, TDD) * fix(face-cleanup): lock & face-decline survive person merge/delete face_repair_lock.personId and face_repair_decline.suspectedOwnerId were ON DELETE CASCADE, so a confirm/lock or soft-decline silently vanished whenever the reviewed/suspected person was merged away or hard-deleted, letting the same face resurface on the next scan. Make the lock's personId nullable with ON DELETE SET NULL (it's audit-only; getLockedFaceIds is keyed on assetFaceId alone) and have mergePersonProfile re-point both the lock's personId and any type='face' decline's suspectedOwnerId from source to target before the source person is deleted, using a conflict-safe insert-then-delete for declines so a pre-existing (face, target) decline dedupes instead of throwing a unique violation. * fix(face-cleanup): dismiss drains the latest scan snapshot * feat(face-cleanup): move-and-lock (server + SDK) A deliberate "Move -> chosen person" can now also durably, owner-agnostically lock the moved faces to their destination (moveToPerson[].lock), so a later re-scan never re-flags them. - MoveGroupSchema gains lock (default false). - executeRepair returns movedFaceIds alongside moved/skipped. - resolveFaces locks only faces that actually moved for lock:true groups, bypassing the flagged-snapshot check (locks are tied to the move, not the standalone lock bucket); idempotent via insertLocks' unique index. - Medium coverage: M5 move-and-lock + idempotent re-issue, M7 rest-of-cluster move-lock, M8 skips a face that moved off-person before the call (no orphan lock), M10 undo re-enables flagging, M12 lock survives a later person merge. - Regenerated the OpenAPI spec, TypeScript SDK, and Dart client. * feat(face-cleanup): move-and-lock picker toggle (web) Thread the server's moveToPerson[].lock flag through the review UI: the PersonPicker gets a "Lock so it won't re-flag" checkbox (checked by default), review.svelte.ts's buildResolveRequest() emits it on the chosen-person group only (owner-state groups always pass lock:false), and the resolve controller is covered directly for lock:true/false, its default, and non-boolean rejection. * fix(face-cleanup): never auto-lock a suggested-owner move when it shares a destination * perf(face-cleanup): scope the per-scan decline/lock load to the flagged set buildRepairPlan's getDeclineMaps read was unscoped, re-fetching the entire face_repair_decline/face_repair_lock tables on every scan pass even though only the faces/persons just flagged can ever be affected. Scope it from flaggedByPerson (built before applyDeclineFilters runs), matching the scoped read getPersonFlaggedFaces already uses. Adds U1 (pure-helper regression lock: applyDeclineFilters produces identical drops whether fed full or pre-scoped maps) and M11 (medium: buildRepairPlan flags the same faces via the scoped read as the current unscoped baseline, over a fixture with declines/locks both inside and outside the flagged set) from the temporal-consistency hardening test matrix (design doc §7). Also adds M6, a previously-missing matrix id: a moveToPerson group with lock:false moves the face and writes no lock row. * test(face-cleanup): consistency capstone e2e + i18n + gate Add e2e coverage for the two remaining design-matrix ids (§7.6 X1/X2, titled "Consistency X1"/"Consistency X2" to avoid colliding with this file's existing, differently-scoped X1/X2 from the full-resolution feature): - Consistency X1: a chosen-person move via the review page's picker with the lock toggle on (checked by default) persists a lock at the destination; seeding a fresh scan snapshot that would otherwise re-flag the face proves it stays dropped via getFaceRepairPersonFaces's scoped-read seam. - Consistency X2: locking a face, then merging its person into another via the real /people/{id}/merge endpoint, re-points the lock instead of losing it; the same re-scan simulation proves it still isn't re-flagged after the merge, regardless of who a later scan proposes. i18n audit: every admin.face_cleanup_* key referenced in web/src (138 distinct, incl. the 5 concrete admin.face_cleanup_review_tally_${state} variants) already exists in i18n/en.json — no changes needed. Full gate green: server check/lint/unit (4909 tests)/medium face-repair (134 tests); web check:typescript/check:svelte/lint/unit (3481 tests); e2e check/lint. * fix(e2e): make seedFaceSearch idempotent so a second flagged-scan seed doesn't collide The Consistency X1/X2 specs seed a second flagged scan over the same faces to simulate a later re-scan; seedFaceSearch's plain INSERT hit face_search's primary key. ON CONFLICT DO NOTHING (identical embedding). * docs(face-cleanup): actions help-modal design (two entry points, one modal) * feat(face-cleanup): explain the five resolution actions in a help modal The bulk bar ships five terminal actions with terse labels and no explanation of what they mean or what they do on apply. The distinctions that matter most are invisible: Keep here declines only THIS suspected owner (a later scan can re-flag the face toward someone else), which is the whole reason Confirm / lock exists; Not a face strips the identity link and regenerates the person's thumbnail if the detached crop was its avatar. An (i) on the review banner (visible on load, before anything is selected) and one on the bulk bar (in context with the buttons) open the same modal. Each action gets its name, what it means, and what it does on apply — copy grounded in resolveFaces, not in what the labels imply. The names are not re-declared: the modal reuses each button's own bulk-bar i18n key, so a translated heading can never drift from its translated button. STATE_COLOR moves to review.svelte.ts so the page and the modal read the five colors from one source. * fix(face-cleanup): never drain a person on a resolve that settles no flagged face resolveFaces drained the reviewed person from the console on ANY committed resolve. The web review page fired a second, independent resolve for its rest-of-cluster selection, so moving one unflagged face dropped the person from the scan while every flagged face was still awaiting a decision — the admin's staged review was silently discarded, and the same faces came back on the next scan. That is the bug behind "I cleaned this person up and it reappeared". Drop-on-any-resolution becomes drop-on-SETTLED-resolution: drain only when the resolve accounts for every face in the person's flagged snapshot (a kept/locked face IS settled, so the stay-only case still drains), or on entireCluster, which moves the whole cluster by definition. * fix(face-cleanup): one terminal Apply, never swallow a rejected resolve, icon per state The review page had two independent commits. "Apply · N faces" sent only the flagged review model, ignoring any rest-of-cluster faces ticked below it; "Move N faces" sent only the rest faces, ignoring every staged flagged decision. Either one closed the person out of the console having done half the job, and neither told the admin what it had done. Worse, the rest-of-cluster and whole-cluster paths swallowed every error from resolveFaces in an empty catch — including the server's 409 ("Refusing to apply while a scan is in progress"). Confirming "Move entire cluster" during a re-scan therefore moved nothing, showed nothing, and left the admin believing it had worked until the next scan handed the same faces back. Now: ticking a rest-of-cluster face STAGES it into the single Apply (the dock counts it, "+N added"), every resolve funnels through one commit path that surfaces failures in the existing error banner, and a successful apply reports the server's own counts (moved/kept/locked/detached/skipped) instead of blind navigating. "Move entire cluster" stays a separate explicit commit. Also: STATE_ICON gives every state its own glyph (arrow → owner, pin → keep, lock → confirm, person-arrow → move, image-off → not a face) on the tile badge, bulk button, tally chip and help modal. The badge used to stamp one check mark on owner/stay/other, leaving indigo-vs-violet as the only thing separating "moved away" from "locked in place" — unreadable for a colourblind admin. * feat(face-cleanup): tell the admin what to do after a scan The first screen after a scan opens with five stat cards, four filters, a search box, a pre-selected bulk bar and a grouped table — and never says what any of it is for or what to do next. The review-first/confident split, which IS the workflow, is explained nowhere. And the riskiest fact on the page is silent: the confident clusters arrive pre-selected, so the biggest button on screen re-attributes all of them in one click. A three-step checklist replaces the description paragraph (its gist becomes the subtitle, so the page gains guidance without gaining height): open the ones that need a decision, note that the confident ones are already selected, then commit. It reads real scan state rather than being a static blurb, so it doubles as progress: the model already gates a review-first cluster's selectability on having opened it, so "3 of 16 opened" is state that existed and was never shown. Step 1 ticks green when nothing is left, step 2 goes quiet when a scan produced no confident clusters, step 3 when nothing is selected. * fix(face-cleanup): drop the checklist's "Review first" shortcut button It only flipped the filter chip, but it looked like navigation — and its effect landed in the table far below the fold, so clicking it appeared to do nothing. The checklist is guidance; the filter toolbar is the control surface, and its "Review first" chip is right there. A test pins the checklist as informational: it renders no buttons of its own. * feat(face-cleanup): park unnamed strangers in their own cluster, and make "Not a face" stick An admin reviewing someone else's library constantly hits a face they know does not belong to the cluster but cannot name — a friend of the family. Every existing action was a lie for that face (moving it to the suggested owner is wrong, keeping it is wrong, and "Not a face" is wrong because it plainly IS a face), so the review could not be finished. "Unknown person" parks the selected faces in a fresh unnamed cluster of their own and locks them there. Deliberately NOT a bare unassign back to the "unknown pool": queueRecognizeFaces streams every personId IS NULL face back through recognition, which re-matches it onto its nearest neighbour-with-a-person — very often the cluster it was just pulled out of. Giving the face a person of its own is what makes the decision stick, and it still surfaces as an unnamed cluster on the People page for the owner (or face-recognition suggestions) to name later. Same bug made "Not a face" hollow: it only nulled personId, so a detached crop was re-queued and could be re-assigned, contradicting the console's promise that it "stops being proposed for anyone". Detach now soft-deletes the face, the same primitive the face editor's delete-face uses, which every recognition-candidate query filters on. Also repairs the German console: three keys were translations of the pre-redesign strings and rendered raw {moving}/{name}/{faceCount} placeholders, and 76 were missing entirely — so the one sentence that enumerates the actions never reached the (German) admin whose feedback this is. All 173 face-cleanup keys are now translated, with placeholder parity checked against en.json. Fixes a pre-existing svelte-check error in ActionsHelpModal ($t given a widened string) that would have failed CI Lint Web. * fix(face-cleanup): pin the review dock to the bottom instead of letting it float mid-page The dock was `sticky bottom-0` inside the scrolled content. Sticky only pins while its containing block still extends below it, and the containing block here is the auto-height Container inside the Scrollable — so on a short review (a handful of flagged faces) the page never overflows, sticky is inert, and the bar comes to rest wherever the content happens to end: adrift in the middle of the page, with the content's pb-32 opening a further gap beneath it. Render it through a new optional `footer` snippet on AdminPageLayout / BreadcrumbActionPage instead, as a sibling of the scroll area rather than inside it. That pins it at any content length, lets the grid scroll above it rather than underneath, and drops the pb-32 that only existed to reserve space for it. It still never overlaps the sidebar, which is why `fixed` was rejected originally. The footer renders no wrapper element, so the 16 other admin pages that pass no footer are untouched. * docs(face-cleanup): prettier-format the plan and design docs CI Docs Build runs `prettier --check .` over docs/ and was already failing on this branch: three face-cleanup markdown files landed unformatted. * fix(face-cleanup): confirm before discarding faces, and guard the bugs review found "Not a face" is now irreversible (it soft-deletes the detected face so recognition can never hand it back), it sits one button away from "Unknown person", and the two mean opposite things. A slip between them destroys real face data with no undo anywhere in the app. An Apply carrying any discarded face now has to be confirmed; everything else still applies straight through, because a prompt on every Apply trains you to click past the one that matters. Review also turned up three things nothing was guarding: - The German placeholder bug was NOT only German. fr.json carried the identical stale face-cleanup strings, and mr/ms had the ARGUMENT NAME itself translated ({संख्या}, {bulan}) — all render as literal braces to the user. Fixed all five, and added web/src/lib/i18n/placeholders.spec.ts, which parses ICU properly (a naive regex mistakes plural branches like `one {hour}` for placeholders) and fails if any locale references an argument en.json does not supply. Verified it goes red on the original bug. CI's "Test i18n" job only ran prettier, which is why this class of bug reached a user. - resolveFaces created the unknown cluster BEFORE moving faces into it, outside any try/catch: a failure in between stranded a nameless, faceless person on the owner's People page forever. Now removed on failure — but only when genuinely empty, since asset_face.personId is ON DELETE SET NULL and deleting a cluster that did receive faces would dump them back into the recognition pool this action exists to keep them out of. - The e2e named "routing every state" covered five of six. Adds the unknown state, plus a test that the dock is pinned to the viewport bottom on a short review — the float bug no component test can catch, because the AdminPageLayout stub renders children and footer into the same element. Also covers the unknown cluster's representative face and thumbnail (a cluster with no avatar is unnameable on the People page, which is the whole point of parking it) and a combined move+unknown resolve pinning the disjoint moved/unknown counts. * test(e2e): measure the dock against the app shell's inset, not the raw viewport The pinned-dock assertion demanded the dock's bottom edge land within 2px of the viewport bottom. It measured 712 of 720: the app shell insets its content region by ~8px on every admin page, so a correctly pinned dock never reaches the raw viewport edge. Loosen to 16px, which still sits an order of magnitude clear of the bug it guards (the floating dock measured ~124px short of the bottom), and say so in the test. Distance-to-bottom is the ONLY thing separating the two states — the floating dock was below the content too, just adrift — so the "below the grid" check is labelled as the sanity check it is, not as the guard.
Deeds67
force-pushed
the
feat/face-cleanup-resolution
branch
from
July 15, 2026 19:54
b4c4577 to
1c8bd02
Compare
Deeds67
added a commit
that referenced
this pull request
Jul 15, 2026
…rvive merge, move-and-lock, dismiss drain (stacked on #770) (#773) * docs(face-cleanup): temporal-consistency hardening design (lock/decline survive merge, move-and-lock, dismiss drain, scoped load) * docs(face-cleanup): spec-review fixes — executeRepair moved-ids for move-lock, defer person-dismiss-merge, txn-handle + decline personId-null notes * docs(face-cleanup): consistency-hardening implementation plan (4 slices, TDD) * fix(face-cleanup): lock & face-decline survive person merge/delete face_repair_lock.personId and face_repair_decline.suspectedOwnerId were ON DELETE CASCADE, so a confirm/lock or soft-decline silently vanished whenever the reviewed/suspected person was merged away or hard-deleted, letting the same face resurface on the next scan. Make the lock's personId nullable with ON DELETE SET NULL (it's audit-only; getLockedFaceIds is keyed on assetFaceId alone) and have mergePersonProfile re-point both the lock's personId and any type='face' decline's suspectedOwnerId from source to target before the source person is deleted, using a conflict-safe insert-then-delete for declines so a pre-existing (face, target) decline dedupes instead of throwing a unique violation. * fix(face-cleanup): dismiss drains the latest scan snapshot * feat(face-cleanup): move-and-lock (server + SDK) A deliberate "Move -> chosen person" can now also durably, owner-agnostically lock the moved faces to their destination (moveToPerson[].lock), so a later re-scan never re-flags them. - MoveGroupSchema gains lock (default false). - executeRepair returns movedFaceIds alongside moved/skipped. - resolveFaces locks only faces that actually moved for lock:true groups, bypassing the flagged-snapshot check (locks are tied to the move, not the standalone lock bucket); idempotent via insertLocks' unique index. - Medium coverage: M5 move-and-lock + idempotent re-issue, M7 rest-of-cluster move-lock, M8 skips a face that moved off-person before the call (no orphan lock), M10 undo re-enables flagging, M12 lock survives a later person merge. - Regenerated the OpenAPI spec, TypeScript SDK, and Dart client. * feat(face-cleanup): move-and-lock picker toggle (web) Thread the server's moveToPerson[].lock flag through the review UI: the PersonPicker gets a "Lock so it won't re-flag" checkbox (checked by default), review.svelte.ts's buildResolveRequest() emits it on the chosen-person group only (owner-state groups always pass lock:false), and the resolve controller is covered directly for lock:true/false, its default, and non-boolean rejection. * fix(face-cleanup): never auto-lock a suggested-owner move when it shares a destination * perf(face-cleanup): scope the per-scan decline/lock load to the flagged set buildRepairPlan's getDeclineMaps read was unscoped, re-fetching the entire face_repair_decline/face_repair_lock tables on every scan pass even though only the faces/persons just flagged can ever be affected. Scope it from flaggedByPerson (built before applyDeclineFilters runs), matching the scoped read getPersonFlaggedFaces already uses. Adds U1 (pure-helper regression lock: applyDeclineFilters produces identical drops whether fed full or pre-scoped maps) and M11 (medium: buildRepairPlan flags the same faces via the scoped read as the current unscoped baseline, over a fixture with declines/locks both inside and outside the flagged set) from the temporal-consistency hardening test matrix (design doc §7). Also adds M6, a previously-missing matrix id: a moveToPerson group with lock:false moves the face and writes no lock row. * test(face-cleanup): consistency capstone e2e + i18n + gate Add e2e coverage for the two remaining design-matrix ids (§7.6 X1/X2, titled "Consistency X1"/"Consistency X2" to avoid colliding with this file's existing, differently-scoped X1/X2 from the full-resolution feature): - Consistency X1: a chosen-person move via the review page's picker with the lock toggle on (checked by default) persists a lock at the destination; seeding a fresh scan snapshot that would otherwise re-flag the face proves it stays dropped via getFaceRepairPersonFaces's scoped-read seam. - Consistency X2: locking a face, then merging its person into another via the real /people/{id}/merge endpoint, re-points the lock instead of losing it; the same re-scan simulation proves it still isn't re-flagged after the merge, regardless of who a later scan proposes. i18n audit: every admin.face_cleanup_* key referenced in web/src (138 distinct, incl. the 5 concrete admin.face_cleanup_review_tally_${state} variants) already exists in i18n/en.json — no changes needed. Full gate green: server check/lint/unit (4909 tests)/medium face-repair (134 tests); web check:typescript/check:svelte/lint/unit (3481 tests); e2e check/lint. * fix(e2e): make seedFaceSearch idempotent so a second flagged-scan seed doesn't collide The Consistency X1/X2 specs seed a second flagged scan over the same faces to simulate a later re-scan; seedFaceSearch's plain INSERT hit face_search's primary key. ON CONFLICT DO NOTHING (identical embedding). * docs(face-cleanup): actions help-modal design (two entry points, one modal) * feat(face-cleanup): explain the five resolution actions in a help modal The bulk bar ships five terminal actions with terse labels and no explanation of what they mean or what they do on apply. The distinctions that matter most are invisible: Keep here declines only THIS suspected owner (a later scan can re-flag the face toward someone else), which is the whole reason Confirm / lock exists; Not a face strips the identity link and regenerates the person's thumbnail if the detached crop was its avatar. An (i) on the review banner (visible on load, before anything is selected) and one on the bulk bar (in context with the buttons) open the same modal. Each action gets its name, what it means, and what it does on apply — copy grounded in resolveFaces, not in what the labels imply. The names are not re-declared: the modal reuses each button's own bulk-bar i18n key, so a translated heading can never drift from its translated button. STATE_COLOR moves to review.svelte.ts so the page and the modal read the five colors from one source. * fix(face-cleanup): never drain a person on a resolve that settles no flagged face resolveFaces drained the reviewed person from the console on ANY committed resolve. The web review page fired a second, independent resolve for its rest-of-cluster selection, so moving one unflagged face dropped the person from the scan while every flagged face was still awaiting a decision — the admin's staged review was silently discarded, and the same faces came back on the next scan. That is the bug behind "I cleaned this person up and it reappeared". Drop-on-any-resolution becomes drop-on-SETTLED-resolution: drain only when the resolve accounts for every face in the person's flagged snapshot (a kept/locked face IS settled, so the stay-only case still drains), or on entireCluster, which moves the whole cluster by definition. * fix(face-cleanup): one terminal Apply, never swallow a rejected resolve, icon per state The review page had two independent commits. "Apply · N faces" sent only the flagged review model, ignoring any rest-of-cluster faces ticked below it; "Move N faces" sent only the rest faces, ignoring every staged flagged decision. Either one closed the person out of the console having done half the job, and neither told the admin what it had done. Worse, the rest-of-cluster and whole-cluster paths swallowed every error from resolveFaces in an empty catch — including the server's 409 ("Refusing to apply while a scan is in progress"). Confirming "Move entire cluster" during a re-scan therefore moved nothing, showed nothing, and left the admin believing it had worked until the next scan handed the same faces back. Now: ticking a rest-of-cluster face STAGES it into the single Apply (the dock counts it, "+N added"), every resolve funnels through one commit path that surfaces failures in the existing error banner, and a successful apply reports the server's own counts (moved/kept/locked/detached/skipped) instead of blind navigating. "Move entire cluster" stays a separate explicit commit. Also: STATE_ICON gives every state its own glyph (arrow → owner, pin → keep, lock → confirm, person-arrow → move, image-off → not a face) on the tile badge, bulk button, tally chip and help modal. The badge used to stamp one check mark on owner/stay/other, leaving indigo-vs-violet as the only thing separating "moved away" from "locked in place" — unreadable for a colourblind admin. * feat(face-cleanup): tell the admin what to do after a scan The first screen after a scan opens with five stat cards, four filters, a search box, a pre-selected bulk bar and a grouped table — and never says what any of it is for or what to do next. The review-first/confident split, which IS the workflow, is explained nowhere. And the riskiest fact on the page is silent: the confident clusters arrive pre-selected, so the biggest button on screen re-attributes all of them in one click. A three-step checklist replaces the description paragraph (its gist becomes the subtitle, so the page gains guidance without gaining height): open the ones that need a decision, note that the confident ones are already selected, then commit. It reads real scan state rather than being a static blurb, so it doubles as progress: the model already gates a review-first cluster's selectability on having opened it, so "3 of 16 opened" is state that existed and was never shown. Step 1 ticks green when nothing is left, step 2 goes quiet when a scan produced no confident clusters, step 3 when nothing is selected. * fix(face-cleanup): drop the checklist's "Review first" shortcut button It only flipped the filter chip, but it looked like navigation — and its effect landed in the table far below the fold, so clicking it appeared to do nothing. The checklist is guidance; the filter toolbar is the control surface, and its "Review first" chip is right there. A test pins the checklist as informational: it renders no buttons of its own. * feat(face-cleanup): park unnamed strangers in their own cluster, and make "Not a face" stick An admin reviewing someone else's library constantly hits a face they know does not belong to the cluster but cannot name — a friend of the family. Every existing action was a lie for that face (moving it to the suggested owner is wrong, keeping it is wrong, and "Not a face" is wrong because it plainly IS a face), so the review could not be finished. "Unknown person" parks the selected faces in a fresh unnamed cluster of their own and locks them there. Deliberately NOT a bare unassign back to the "unknown pool": queueRecognizeFaces streams every personId IS NULL face back through recognition, which re-matches it onto its nearest neighbour-with-a-person — very often the cluster it was just pulled out of. Giving the face a person of its own is what makes the decision stick, and it still surfaces as an unnamed cluster on the People page for the owner (or face-recognition suggestions) to name later. Same bug made "Not a face" hollow: it only nulled personId, so a detached crop was re-queued and could be re-assigned, contradicting the console's promise that it "stops being proposed for anyone". Detach now soft-deletes the face, the same primitive the face editor's delete-face uses, which every recognition-candidate query filters on. Also repairs the German console: three keys were translations of the pre-redesign strings and rendered raw {moving}/{name}/{faceCount} placeholders, and 76 were missing entirely — so the one sentence that enumerates the actions never reached the (German) admin whose feedback this is. All 173 face-cleanup keys are now translated, with placeholder parity checked against en.json. Fixes a pre-existing svelte-check error in ActionsHelpModal ($t given a widened string) that would have failed CI Lint Web. * fix(face-cleanup): pin the review dock to the bottom instead of letting it float mid-page The dock was `sticky bottom-0` inside the scrolled content. Sticky only pins while its containing block still extends below it, and the containing block here is the auto-height Container inside the Scrollable — so on a short review (a handful of flagged faces) the page never overflows, sticky is inert, and the bar comes to rest wherever the content happens to end: adrift in the middle of the page, with the content's pb-32 opening a further gap beneath it. Render it through a new optional `footer` snippet on AdminPageLayout / BreadcrumbActionPage instead, as a sibling of the scroll area rather than inside it. That pins it at any content length, lets the grid scroll above it rather than underneath, and drops the pb-32 that only existed to reserve space for it. It still never overlaps the sidebar, which is why `fixed` was rejected originally. The footer renders no wrapper element, so the 16 other admin pages that pass no footer are untouched. * docs(face-cleanup): prettier-format the plan and design docs CI Docs Build runs `prettier --check .` over docs/ and was already failing on this branch: three face-cleanup markdown files landed unformatted. * fix(face-cleanup): confirm before discarding faces, and guard the bugs review found "Not a face" is now irreversible (it soft-deletes the detected face so recognition can never hand it back), it sits one button away from "Unknown person", and the two mean opposite things. A slip between them destroys real face data with no undo anywhere in the app. An Apply carrying any discarded face now has to be confirmed; everything else still applies straight through, because a prompt on every Apply trains you to click past the one that matters. Review also turned up three things nothing was guarding: - The German placeholder bug was NOT only German. fr.json carried the identical stale face-cleanup strings, and mr/ms had the ARGUMENT NAME itself translated ({संख्या}, {bulan}) — all render as literal braces to the user. Fixed all five, and added web/src/lib/i18n/placeholders.spec.ts, which parses ICU properly (a naive regex mistakes plural branches like `one {hour}` for placeholders) and fails if any locale references an argument en.json does not supply. Verified it goes red on the original bug. CI's "Test i18n" job only ran prettier, which is why this class of bug reached a user. - resolveFaces created the unknown cluster BEFORE moving faces into it, outside any try/catch: a failure in between stranded a nameless, faceless person on the owner's People page forever. Now removed on failure — but only when genuinely empty, since asset_face.personId is ON DELETE SET NULL and deleting a cluster that did receive faces would dump them back into the recognition pool this action exists to keep them out of. - The e2e named "routing every state" covered five of six. Adds the unknown state, plus a test that the dock is pinned to the viewport bottom on a short review — the float bug no component test can catch, because the AdminPageLayout stub renders children and footer into the same element. Also covers the unknown cluster's representative face and thumbnail (a cluster with no avatar is unnameable on the People page, which is the whole point of parking it) and a combined move+unknown resolve pinning the disjoint moved/unknown counts. * test(e2e): measure the dock against the app shell's inset, not the raw viewport The pinned-dock assertion demanded the dock's bottom edge land within 2px of the viewport bottom. It measured 712 of 720: the app shell insets its content region by ~8px on every admin page, so a correctly pinned dock never reaches the raw viewport edge. Loosen to 16px, which still sits an order of magnitude clear of the bug it guards (the floating dock measured ~124px short of the bottom), and say so in the test. Distance-to-bottom is the ONLY thing separating the two states — the floating dock was below the content too, just adrift — so the "below the grid" check is labelled as the sanity check it is, not as the guard.
Deeds67
added a commit
that referenced
this pull request
Jul 16, 2026
… 500, cross-owner guard Addresses the multi-agent review of PR #770. Correctness / edge: - C1: the console-drain check compared the RAW flagged snapshot, so a person soft-stayed or locked across sessions could never drain (the settled face is filtered out of the review UI and can't be re-submitted). Measure the drain against the decline/lock-filtered pending set (`resolvable`) instead. - C7: "Keep here" 500'd when a stayed face's scan-suggested owner was deleted/merged after the scan — the decline insert hit the person FK (23503), after any moves already committed. Drop stay faces whose owner is gone (they are already effectively kept); still settles the face. - C2: moveToPerson silently dropped a previously soft-stayed face moved to a DIFFERENT chosen destination. Scope the pre-skip to the requested destination (still skips locked faces and a re-move toward the same declined owner). Security / RBAC: - C6 (defense-in-depth): executeRepair refuses a cross-owner route at the write layer, so no caller — including the legacy POST /admin/face-repair path — can collapse two owners. - C4: bound every resolve/decline face-array at 1000 (admin-only DoS backstop; mirrors the existing max(200) on cluster-faces). Maintainability / coverage: - C13/C15: drop removeLocks's dead natural-key branch; refresh a stale DTO doc comment. - C9: controller auth tests for GET scan/latest and scan/person/:personId (+ non-uuid 400). - C10: boundary test for a resolve with no persisted scan. Regression tests cover every fix. Regenerated the OpenAPI spec (maxItems). Larger refactors (tri-naming rename, resolveFaces god-method split, repository dedup) intentionally deferred to a separate change.
Deeds67
force-pushed
the
feat/face-cleanup-resolution
branch
from
July 21, 2026 20:30
acb4cec to
7db7012
Compare
Deeds67
added a commit
that referenced
this pull request
Jul 21, 2026
…rvive merge, move-and-lock, dismiss drain (stacked on #770) (#773) * docs(face-cleanup): temporal-consistency hardening design (lock/decline survive merge, move-and-lock, dismiss drain, scoped load) * docs(face-cleanup): spec-review fixes — executeRepair moved-ids for move-lock, defer person-dismiss-merge, txn-handle + decline personId-null notes * docs(face-cleanup): consistency-hardening implementation plan (4 slices, TDD) * fix(face-cleanup): lock & face-decline survive person merge/delete face_repair_lock.personId and face_repair_decline.suspectedOwnerId were ON DELETE CASCADE, so a confirm/lock or soft-decline silently vanished whenever the reviewed/suspected person was merged away or hard-deleted, letting the same face resurface on the next scan. Make the lock's personId nullable with ON DELETE SET NULL (it's audit-only; getLockedFaceIds is keyed on assetFaceId alone) and have mergePersonProfile re-point both the lock's personId and any type='face' decline's suspectedOwnerId from source to target before the source person is deleted, using a conflict-safe insert-then-delete for declines so a pre-existing (face, target) decline dedupes instead of throwing a unique violation. * fix(face-cleanup): dismiss drains the latest scan snapshot * feat(face-cleanup): move-and-lock (server + SDK) A deliberate "Move -> chosen person" can now also durably, owner-agnostically lock the moved faces to their destination (moveToPerson[].lock), so a later re-scan never re-flags them. - MoveGroupSchema gains lock (default false). - executeRepair returns movedFaceIds alongside moved/skipped. - resolveFaces locks only faces that actually moved for lock:true groups, bypassing the flagged-snapshot check (locks are tied to the move, not the standalone lock bucket); idempotent via insertLocks' unique index. - Medium coverage: M5 move-and-lock + idempotent re-issue, M7 rest-of-cluster move-lock, M8 skips a face that moved off-person before the call (no orphan lock), M10 undo re-enables flagging, M12 lock survives a later person merge. - Regenerated the OpenAPI spec, TypeScript SDK, and Dart client. * feat(face-cleanup): move-and-lock picker toggle (web) Thread the server's moveToPerson[].lock flag through the review UI: the PersonPicker gets a "Lock so it won't re-flag" checkbox (checked by default), review.svelte.ts's buildResolveRequest() emits it on the chosen-person group only (owner-state groups always pass lock:false), and the resolve controller is covered directly for lock:true/false, its default, and non-boolean rejection. * fix(face-cleanup): never auto-lock a suggested-owner move when it shares a destination * perf(face-cleanup): scope the per-scan decline/lock load to the flagged set buildRepairPlan's getDeclineMaps read was unscoped, re-fetching the entire face_repair_decline/face_repair_lock tables on every scan pass even though only the faces/persons just flagged can ever be affected. Scope it from flaggedByPerson (built before applyDeclineFilters runs), matching the scoped read getPersonFlaggedFaces already uses. Adds U1 (pure-helper regression lock: applyDeclineFilters produces identical drops whether fed full or pre-scoped maps) and M11 (medium: buildRepairPlan flags the same faces via the scoped read as the current unscoped baseline, over a fixture with declines/locks both inside and outside the flagged set) from the temporal-consistency hardening test matrix (design doc §7). Also adds M6, a previously-missing matrix id: a moveToPerson group with lock:false moves the face and writes no lock row. * test(face-cleanup): consistency capstone e2e + i18n + gate Add e2e coverage for the two remaining design-matrix ids (§7.6 X1/X2, titled "Consistency X1"/"Consistency X2" to avoid colliding with this file's existing, differently-scoped X1/X2 from the full-resolution feature): - Consistency X1: a chosen-person move via the review page's picker with the lock toggle on (checked by default) persists a lock at the destination; seeding a fresh scan snapshot that would otherwise re-flag the face proves it stays dropped via getFaceRepairPersonFaces's scoped-read seam. - Consistency X2: locking a face, then merging its person into another via the real /people/{id}/merge endpoint, re-points the lock instead of losing it; the same re-scan simulation proves it still isn't re-flagged after the merge, regardless of who a later scan proposes. i18n audit: every admin.face_cleanup_* key referenced in web/src (138 distinct, incl. the 5 concrete admin.face_cleanup_review_tally_${state} variants) already exists in i18n/en.json — no changes needed. Full gate green: server check/lint/unit (4909 tests)/medium face-repair (134 tests); web check:typescript/check:svelte/lint/unit (3481 tests); e2e check/lint. * fix(e2e): make seedFaceSearch idempotent so a second flagged-scan seed doesn't collide The Consistency X1/X2 specs seed a second flagged scan over the same faces to simulate a later re-scan; seedFaceSearch's plain INSERT hit face_search's primary key. ON CONFLICT DO NOTHING (identical embedding). * docs(face-cleanup): actions help-modal design (two entry points, one modal) * feat(face-cleanup): explain the five resolution actions in a help modal The bulk bar ships five terminal actions with terse labels and no explanation of what they mean or what they do on apply. The distinctions that matter most are invisible: Keep here declines only THIS suspected owner (a later scan can re-flag the face toward someone else), which is the whole reason Confirm / lock exists; Not a face strips the identity link and regenerates the person's thumbnail if the detached crop was its avatar. An (i) on the review banner (visible on load, before anything is selected) and one on the bulk bar (in context with the buttons) open the same modal. Each action gets its name, what it means, and what it does on apply — copy grounded in resolveFaces, not in what the labels imply. The names are not re-declared: the modal reuses each button's own bulk-bar i18n key, so a translated heading can never drift from its translated button. STATE_COLOR moves to review.svelte.ts so the page and the modal read the five colors from one source. * fix(face-cleanup): never drain a person on a resolve that settles no flagged face resolveFaces drained the reviewed person from the console on ANY committed resolve. The web review page fired a second, independent resolve for its rest-of-cluster selection, so moving one unflagged face dropped the person from the scan while every flagged face was still awaiting a decision — the admin's staged review was silently discarded, and the same faces came back on the next scan. That is the bug behind "I cleaned this person up and it reappeared". Drop-on-any-resolution becomes drop-on-SETTLED-resolution: drain only when the resolve accounts for every face in the person's flagged snapshot (a kept/locked face IS settled, so the stay-only case still drains), or on entireCluster, which moves the whole cluster by definition. * fix(face-cleanup): one terminal Apply, never swallow a rejected resolve, icon per state The review page had two independent commits. "Apply · N faces" sent only the flagged review model, ignoring any rest-of-cluster faces ticked below it; "Move N faces" sent only the rest faces, ignoring every staged flagged decision. Either one closed the person out of the console having done half the job, and neither told the admin what it had done. Worse, the rest-of-cluster and whole-cluster paths swallowed every error from resolveFaces in an empty catch — including the server's 409 ("Refusing to apply while a scan is in progress"). Confirming "Move entire cluster" during a re-scan therefore moved nothing, showed nothing, and left the admin believing it had worked until the next scan handed the same faces back. Now: ticking a rest-of-cluster face STAGES it into the single Apply (the dock counts it, "+N added"), every resolve funnels through one commit path that surfaces failures in the existing error banner, and a successful apply reports the server's own counts (moved/kept/locked/detached/skipped) instead of blind navigating. "Move entire cluster" stays a separate explicit commit. Also: STATE_ICON gives every state its own glyph (arrow → owner, pin → keep, lock → confirm, person-arrow → move, image-off → not a face) on the tile badge, bulk button, tally chip and help modal. The badge used to stamp one check mark on owner/stay/other, leaving indigo-vs-violet as the only thing separating "moved away" from "locked in place" — unreadable for a colourblind admin. * feat(face-cleanup): tell the admin what to do after a scan The first screen after a scan opens with five stat cards, four filters, a search box, a pre-selected bulk bar and a grouped table — and never says what any of it is for or what to do next. The review-first/confident split, which IS the workflow, is explained nowhere. And the riskiest fact on the page is silent: the confident clusters arrive pre-selected, so the biggest button on screen re-attributes all of them in one click. A three-step checklist replaces the description paragraph (its gist becomes the subtitle, so the page gains guidance without gaining height): open the ones that need a decision, note that the confident ones are already selected, then commit. It reads real scan state rather than being a static blurb, so it doubles as progress: the model already gates a review-first cluster's selectability on having opened it, so "3 of 16 opened" is state that existed and was never shown. Step 1 ticks green when nothing is left, step 2 goes quiet when a scan produced no confident clusters, step 3 when nothing is selected. * fix(face-cleanup): drop the checklist's "Review first" shortcut button It only flipped the filter chip, but it looked like navigation — and its effect landed in the table far below the fold, so clicking it appeared to do nothing. The checklist is guidance; the filter toolbar is the control surface, and its "Review first" chip is right there. A test pins the checklist as informational: it renders no buttons of its own. * feat(face-cleanup): park unnamed strangers in their own cluster, and make "Not a face" stick An admin reviewing someone else's library constantly hits a face they know does not belong to the cluster but cannot name — a friend of the family. Every existing action was a lie for that face (moving it to the suggested owner is wrong, keeping it is wrong, and "Not a face" is wrong because it plainly IS a face), so the review could not be finished. "Unknown person" parks the selected faces in a fresh unnamed cluster of their own and locks them there. Deliberately NOT a bare unassign back to the "unknown pool": queueRecognizeFaces streams every personId IS NULL face back through recognition, which re-matches it onto its nearest neighbour-with-a-person — very often the cluster it was just pulled out of. Giving the face a person of its own is what makes the decision stick, and it still surfaces as an unnamed cluster on the People page for the owner (or face-recognition suggestions) to name later. Same bug made "Not a face" hollow: it only nulled personId, so a detached crop was re-queued and could be re-assigned, contradicting the console's promise that it "stops being proposed for anyone". Detach now soft-deletes the face, the same primitive the face editor's delete-face uses, which every recognition-candidate query filters on. Also repairs the German console: three keys were translations of the pre-redesign strings and rendered raw {moving}/{name}/{faceCount} placeholders, and 76 were missing entirely — so the one sentence that enumerates the actions never reached the (German) admin whose feedback this is. All 173 face-cleanup keys are now translated, with placeholder parity checked against en.json. Fixes a pre-existing svelte-check error in ActionsHelpModal ($t given a widened string) that would have failed CI Lint Web. * fix(face-cleanup): pin the review dock to the bottom instead of letting it float mid-page The dock was `sticky bottom-0` inside the scrolled content. Sticky only pins while its containing block still extends below it, and the containing block here is the auto-height Container inside the Scrollable — so on a short review (a handful of flagged faces) the page never overflows, sticky is inert, and the bar comes to rest wherever the content happens to end: adrift in the middle of the page, with the content's pb-32 opening a further gap beneath it. Render it through a new optional `footer` snippet on AdminPageLayout / BreadcrumbActionPage instead, as a sibling of the scroll area rather than inside it. That pins it at any content length, lets the grid scroll above it rather than underneath, and drops the pb-32 that only existed to reserve space for it. It still never overlaps the sidebar, which is why `fixed` was rejected originally. The footer renders no wrapper element, so the 16 other admin pages that pass no footer are untouched. * docs(face-cleanup): prettier-format the plan and design docs CI Docs Build runs `prettier --check .` over docs/ and was already failing on this branch: three face-cleanup markdown files landed unformatted. * fix(face-cleanup): confirm before discarding faces, and guard the bugs review found "Not a face" is now irreversible (it soft-deletes the detected face so recognition can never hand it back), it sits one button away from "Unknown person", and the two mean opposite things. A slip between them destroys real face data with no undo anywhere in the app. An Apply carrying any discarded face now has to be confirmed; everything else still applies straight through, because a prompt on every Apply trains you to click past the one that matters. Review also turned up three things nothing was guarding: - The German placeholder bug was NOT only German. fr.json carried the identical stale face-cleanup strings, and mr/ms had the ARGUMENT NAME itself translated ({संख्या}, {bulan}) — all render as literal braces to the user. Fixed all five, and added web/src/lib/i18n/placeholders.spec.ts, which parses ICU properly (a naive regex mistakes plural branches like `one {hour}` for placeholders) and fails if any locale references an argument en.json does not supply. Verified it goes red on the original bug. CI's "Test i18n" job only ran prettier, which is why this class of bug reached a user. - resolveFaces created the unknown cluster BEFORE moving faces into it, outside any try/catch: a failure in between stranded a nameless, faceless person on the owner's People page forever. Now removed on failure — but only when genuinely empty, since asset_face.personId is ON DELETE SET NULL and deleting a cluster that did receive faces would dump them back into the recognition pool this action exists to keep them out of. - The e2e named "routing every state" covered five of six. Adds the unknown state, plus a test that the dock is pinned to the viewport bottom on a short review — the float bug no component test can catch, because the AdminPageLayout stub renders children and footer into the same element. Also covers the unknown cluster's representative face and thumbnail (a cluster with no avatar is unnameable on the People page, which is the whole point of parking it) and a combined move+unknown resolve pinning the disjoint moved/unknown counts. * test(e2e): measure the dock against the app shell's inset, not the raw viewport The pinned-dock assertion demanded the dock's bottom edge land within 2px of the viewport bottom. It measured 712 of 720: the app shell insets its content region by ~8px on every admin page, so a correctly pinned dock never reaches the raw viewport edge. Loosen to 16px, which still sits an order of magnitude clear of the bug it guards (the floating dock measured ~124px short of the bottom), and say so in the test. Distance-to-bottom is the ONLY thing separating the two states — the floating dock was below the content too, just adrift — so the "below the grid" check is labelled as the sanity check it is, not as the guard.
Deeds67
added a commit
that referenced
this pull request
Jul 21, 2026
… 500, cross-owner guard Addresses the multi-agent review of PR #770. Correctness / edge: - C1: the console-drain check compared the RAW flagged snapshot, so a person soft-stayed or locked across sessions could never drain (the settled face is filtered out of the review UI and can't be re-submitted). Measure the drain against the decline/lock-filtered pending set (`resolvable`) instead. - C7: "Keep here" 500'd when a stayed face's scan-suggested owner was deleted/merged after the scan — the decline insert hit the person FK (23503), after any moves already committed. Drop stay faces whose owner is gone (they are already effectively kept); still settles the face. - C2: moveToPerson silently dropped a previously soft-stayed face moved to a DIFFERENT chosen destination. Scope the pre-skip to the requested destination (still skips locked faces and a re-move toward the same declined owner). Security / RBAC: - C6 (defense-in-depth): executeRepair refuses a cross-owner route at the write layer, so no caller — including the legacy POST /admin/face-repair path — can collapse two owners. - C4: bound every resolve/decline face-array at 1000 (admin-only DoS backstop; mirrors the existing max(200) on cluster-faces). Maintainability / coverage: - C13/C15: drop removeLocks's dead natural-key branch; refresh a stale DTO doc comment. - C9: controller auth tests for GET scan/latest and scan/person/:personId (+ non-uuid 400). - C10: boundary test for a resolve with no persisted scan. Regression tests cover every fix. Regenerated the OpenAPI spec (maxItems). Larger refactors (tri-naming rename, resolveFaces god-method split, repository dedup) intentionally deferred to a separate change.
…664) Admin face-cleanup review UI + server endpoints: see & select all cluster faces, add faces to a person, and move an entire cluster. Paginated cluster-face lister, manualMove passthrough, and the review view-model.
, #664) Persist the flagged faces a scan computes into face_repair_scan_flagged_face and read them on review (bounded indexed read, no per-face KNN recompute). Plus PR #664 review fixes: keyset-paginated scan with bounded ANN concurrency, in-flight partial unique index, per-route transactional reattribute, lazy/virtualized triage list, O(1) canSelect.
… scan vs backfill Concurrent shared-space jobs — SharedSpaceFaceMatchFromBackfill on the PeopleBackfill queue plus the dedup/reconciliation jobs it enqueues on FacialRecognition, all mutating the same space — race on the partial (spaceId, identityId) unique index in findOrCreateCompatibleSpacePersonForIdentity. The non-idempotent createPerson threw shared_space_person_spaceId_identityId_key on the loser, failing the whole job and driving a BullMQ retry storm. That storm saturated the shared 10-connection postgres.js pool and starved the read-only face-repair scan's heartbeat/progress writes, wedging the scan (reported by Hagen: frozen at 16,400/428,551 on a 16k-person space). - createPersonForIdentity: INSERT ... ON CONFLICT (spaceId, identityId) WHERE identityId IS NOT NULL DO NOTHING; the caller re-reads the winner's row on a no-op, so a concurrent create is a benign fold-in instead of a job failure. - triggerScan now also refuses while PeopleBackfill is active, mirroring the existing FacialRecognition guard, so a scan can't start on top of an identity-backfill storm. Tests: unit race-recovery + PeopleBackfill guard; medium DB-level idempotency (real pg).
…r boot-time drift
1783000000000-AddFaceRepairScanInFlightIndex stored the partial index's schema override
with a bare `WHERE "status" IN (...)`, but sql-tools' schemaFromCode always emits the
predicate parenthesized (`WHERE ("status" IN (...))`, see asIndexCreate). The mismatch made
the decorator-vs-database schema check log perpetual drift for this index on every boot
("index exists but is no longer needed" + "is missing and needs to be created" + "override
needs to be updated") — the same class 1778800000000-ReconcileFaceIdentityIndexOverrides
already fixed for the identity indexes. Pre-existing since rc12; unrelated to the rc13 fix.
- New migration UPDATEs the stored override to the parenthesized form so it byte-matches the
code-side override (haveEqualOverrides compares name+sql; a match short-circuits both the
index comparer and the override comparer, clearing all three drift items).
- Medium regression test replicates DatabaseRepository.getSchemaDrift() against the migrated
template DB and asserts no drift references the index.
- Register the new migration in revert-to-immich.sql's kysely_migrations cleanup list.
Extends the Face Cleanup console (#664) with the five terminal states from Hagen's rc12 feedback: move to owner, move to a chosen person, soft stay, confirm/lock (age-gap), and detach (not-a-face). Model B (select + bulk bar) interaction; batch Apply; new face_repair_lock table; unified Resolutions manage page. Supersedes the add-faces single-destination constraint.
… per-face owner, owner-scoped picker, mockup) - Mandate TDD (red-green-refactor) with a full §8 test matrix mapping every edge case E1-E15 to a named test. - Fix correctness: resolve endpoint reconciled with the retained rest-of-cluster + entire-cluster feature (old apply removed would have orphaned it); owner-move is per-face (each flagged face -> its own suspectedOwnerId); drop person from scan on ANY resolution, not just moves. - Scope the person-picker to the cluster's owner (cross-owner reject, E11). - Commit the Model B prototype as the durable frontend source of truth.
…ution 8 linear vertical slices (each DB->server->SDK->web->tests, independently shippable) for /impl-loop: (1) resolve endpoint + Model B move-to-owner, (2) soft stay, (3) confirm/lock, (4) chosen-person picker, (5) detach, (6) rest/entire-cluster + drop old apply, (7) resolutions manage page, (8) capstone e2e + gate. TDD throughout; §8 test matrix mapped per slice.
…e picker Immich's createPerson/getAllPeople are self-scoped; an admin re-attributing another user's cluster needs dedicated owner-scoped people search/create endpoints. Reconciles the spec with the implementation plan (Slice 4).
Slice 1 of the full per-face resolution: POST /admin/face-repair/resolve replaces the 2-state apply for a single reviewed person. Groups flagged faces by each face's own suspected owner (mixed clusters can point to different owners) and moves them via the existing executeRepair engine. stay/lock/detach are validated (disjoint buckets + snapshot membership) but always empty until later slices wire them through; resolvedBy is threaded from the controller now so those slices don't touch it again. A face that moved off the person since the scan snapshot was taken is silently skipped and counted in `skipped` rather than rejected; guards (facial-recognition-active, scan-in-progress) and the empty-unnamed cleanup are reused verbatim from applyRepair, but the person always drains from the latest scan on any committed resolution, not just when something moved. Regenerates the OpenAPI spec and TypeScript SDK for the new endpoint.
Schedule the five orphaned §8 matrix tests (U3/M7/M8/M15/P5) plus the lock branch of M14 into slices; add edge cases E16–E20 (empty resolve, incomplete-resolve client invariant, missing destination, representative- face detach regen, re-stay idempotency) with tests M17–M22/C3; split the resolve-route C1/C2 auth tests into Slice 1; rename reference-fact labels C1–C6→RF1–RF6 to end the collision with the C1/C2 test ids; add a Slice 8 matrix-completeness gate. Also align the plan's resolveFaces signature with RF2 (@Auth/resolvedBy).
Web half of Slice 1 (full per-face resolution). Reworks the flagged-face review model from the old exclude/decline pair into a per-face 5-state model (owner/other/stay/lock/detach) with click/shift-range/select-all/ clear selection ops and a pure buildResolveRequest() that groups owner- routed faces by each face's OWN suspectedOwnerId (a mixed cluster can flag faces toward different owners) — only the owner path is wired into the UI this slice, but the full state model is built up front so later slices (stay/lock/other/detach) don't need another rework. +page.svelte is rebuilt to the Model B mockup: a flagged grid with select/shift/select-all/reset, a sticky dock that swaps between the outcome tally + Apply and a bulk-action bar, and Apply now posts buildResolveRequest() to the new resolveFaces SDK call instead of the retired applyFaceRepair 2-state payload. The retained rest-of-cluster section keeps using the old apply endpoint (decoupled from the new review model) until a later slice unifies it onto resolve.
Wires the `stay` bucket of resolveFaces: a kept face writes a durable face_repair_decline row against its stored suspected owner (idempotent via the existing ON CONFLICT DO NOTHING) and drains from the console on the drop-on-any-resolution path, even when the resolve is stay-only. Also re-scopes the stay/lock/detach "in flagged snapshot" check onto the raw (pre-decline-filter) snapshot, since validating against the decline-filtered set would reject re-staying an already-declined face instead of no-op succeeding. Web already modeled the stay state end-to-end from Slice 1 (buildResolveRequest, chip colors, ribbon copy); the only gap was the missing "Keep here" bulk-bar action, now wired alongside the existing "→ Owner" one.
Slice 3 of the full per-face resolution: a face confirmed via the "Confirm / lock" bucket is durably locked to the reviewed person via a new face_repair_lock table (plain unique index on assetFaceId, idempotent insert). getDeclineMaps now also loads locked face ids and applyDeclineFilters drops any locked face for every suspected owner, before the per-owner decline check, so a locked face is never re-flagged by a future scan regardless of which owner it would propose. resolveFaces wires the lock bucket into the response's `locked` count.
Wires the "Confirm / lock" bulk-bar action next to "→ Owner" and "Keep
here": selecting it sets the tiles to state='lock', rendering the violet
lock chip ("Locked") and the matching dock tally entry — the same pattern
Slice 2 used for "Keep here". buildResolveRequest already emitted `lock`
ids for lock-state faces from Slice 1's 5-state model, so no change was
needed there.
…oints + cross-owner guard)
Wires the `detach` bucket of resolveFaces (Slice 5, state 5): a detached face is unassigned from its person AND has its face_identity_face link stripped, atomically (one transaction), so it leaves every cluster and a later FaceIdentityBackfill pass can never silently re-link it back (E4). FaceRepairRepository.detachFaces mirrors reattributeFaces' ML/visible/not-deleted eligibility guard and 1000-id chunking, deleting the identity link per chunk via the same db/trx handle (never FaceIdentityRepository.unlinkFaces, whose own `this.db` would run outside the caller's transaction). Detaching the person's representative face now repoints faceAssetId and queues PersonGenerateThumbnail, mirroring the move path (E19). Web already modeled the detach state end-to-end from Slice 1 (chip color, grayscale tile, ribbon copy, tally, buildResolveRequest); the only gap was the missing "Not a face" bulk-bar action, now wired alongside the other four.
…er guard, drain-after-move)
…st flagged snapshot)
…approve off apply onto resolve
Replace the declines-only manage page with a unified Resolutions page that lists soft-declines (green) and locks (violet), each undoable via the new resolutions/remove endpoint. The old /declined route now redirects there so bookmarks keep working.
…ollision Rebasing this branch onto current main lands it on top of the #722 fork migration 1782000000000-AddAssetExifDescriptionTrigramIndex, which grabbed the 1782000000000 slot after this branch's older base was cut. Two migrations-gallery files then shared 1782000000000, which the migration-timestamps guard (LOW#2/#15) rejects as a NEW collision. renumber the not-yet-merged fork migration to 1781500000000 (between AddFaceRepairDecline@1781 and AddFaceRepairScanInFlightIndex@1783 — same apply order, still after AddFaceRepairScan@1780 for its FK). Update the revert-to-immich.sql migration-name entry to match.
CodeQL flagged js/xss-through-dom (high) in the resolution design mockup:
the people-picker search value (psearch.value) flowed into innerHTML
unescaped via the 'Create new person "${q}"' row. Route it through an
HTML-entity escaper before it reaches the sink, breaking the taint path.
Mockup-only (design artifact under docs/plans/), no production code.
…rvive merge, move-and-lock, dismiss drain (stacked on #770) (#773) * docs(face-cleanup): temporal-consistency hardening design (lock/decline survive merge, move-and-lock, dismiss drain, scoped load) * docs(face-cleanup): spec-review fixes — executeRepair moved-ids for move-lock, defer person-dismiss-merge, txn-handle + decline personId-null notes * docs(face-cleanup): consistency-hardening implementation plan (4 slices, TDD) * fix(face-cleanup): lock & face-decline survive person merge/delete face_repair_lock.personId and face_repair_decline.suspectedOwnerId were ON DELETE CASCADE, so a confirm/lock or soft-decline silently vanished whenever the reviewed/suspected person was merged away or hard-deleted, letting the same face resurface on the next scan. Make the lock's personId nullable with ON DELETE SET NULL (it's audit-only; getLockedFaceIds is keyed on assetFaceId alone) and have mergePersonProfile re-point both the lock's personId and any type='face' decline's suspectedOwnerId from source to target before the source person is deleted, using a conflict-safe insert-then-delete for declines so a pre-existing (face, target) decline dedupes instead of throwing a unique violation. * fix(face-cleanup): dismiss drains the latest scan snapshot * feat(face-cleanup): move-and-lock (server + SDK) A deliberate "Move -> chosen person" can now also durably, owner-agnostically lock the moved faces to their destination (moveToPerson[].lock), so a later re-scan never re-flags them. - MoveGroupSchema gains lock (default false). - executeRepair returns movedFaceIds alongside moved/skipped. - resolveFaces locks only faces that actually moved for lock:true groups, bypassing the flagged-snapshot check (locks are tied to the move, not the standalone lock bucket); idempotent via insertLocks' unique index. - Medium coverage: M5 move-and-lock + idempotent re-issue, M7 rest-of-cluster move-lock, M8 skips a face that moved off-person before the call (no orphan lock), M10 undo re-enables flagging, M12 lock survives a later person merge. - Regenerated the OpenAPI spec, TypeScript SDK, and Dart client. * feat(face-cleanup): move-and-lock picker toggle (web) Thread the server's moveToPerson[].lock flag through the review UI: the PersonPicker gets a "Lock so it won't re-flag" checkbox (checked by default), review.svelte.ts's buildResolveRequest() emits it on the chosen-person group only (owner-state groups always pass lock:false), and the resolve controller is covered directly for lock:true/false, its default, and non-boolean rejection. * fix(face-cleanup): never auto-lock a suggested-owner move when it shares a destination * perf(face-cleanup): scope the per-scan decline/lock load to the flagged set buildRepairPlan's getDeclineMaps read was unscoped, re-fetching the entire face_repair_decline/face_repair_lock tables on every scan pass even though only the faces/persons just flagged can ever be affected. Scope it from flaggedByPerson (built before applyDeclineFilters runs), matching the scoped read getPersonFlaggedFaces already uses. Adds U1 (pure-helper regression lock: applyDeclineFilters produces identical drops whether fed full or pre-scoped maps) and M11 (medium: buildRepairPlan flags the same faces via the scoped read as the current unscoped baseline, over a fixture with declines/locks both inside and outside the flagged set) from the temporal-consistency hardening test matrix (design doc §7). Also adds M6, a previously-missing matrix id: a moveToPerson group with lock:false moves the face and writes no lock row. * test(face-cleanup): consistency capstone e2e + i18n + gate Add e2e coverage for the two remaining design-matrix ids (§7.6 X1/X2, titled "Consistency X1"/"Consistency X2" to avoid colliding with this file's existing, differently-scoped X1/X2 from the full-resolution feature): - Consistency X1: a chosen-person move via the review page's picker with the lock toggle on (checked by default) persists a lock at the destination; seeding a fresh scan snapshot that would otherwise re-flag the face proves it stays dropped via getFaceRepairPersonFaces's scoped-read seam. - Consistency X2: locking a face, then merging its person into another via the real /people/{id}/merge endpoint, re-points the lock instead of losing it; the same re-scan simulation proves it still isn't re-flagged after the merge, regardless of who a later scan proposes. i18n audit: every admin.face_cleanup_* key referenced in web/src (138 distinct, incl. the 5 concrete admin.face_cleanup_review_tally_${state} variants) already exists in i18n/en.json — no changes needed. Full gate green: server check/lint/unit (4909 tests)/medium face-repair (134 tests); web check:typescript/check:svelte/lint/unit (3481 tests); e2e check/lint. * fix(e2e): make seedFaceSearch idempotent so a second flagged-scan seed doesn't collide The Consistency X1/X2 specs seed a second flagged scan over the same faces to simulate a later re-scan; seedFaceSearch's plain INSERT hit face_search's primary key. ON CONFLICT DO NOTHING (identical embedding). * docs(face-cleanup): actions help-modal design (two entry points, one modal) * feat(face-cleanup): explain the five resolution actions in a help modal The bulk bar ships five terminal actions with terse labels and no explanation of what they mean or what they do on apply. The distinctions that matter most are invisible: Keep here declines only THIS suspected owner (a later scan can re-flag the face toward someone else), which is the whole reason Confirm / lock exists; Not a face strips the identity link and regenerates the person's thumbnail if the detached crop was its avatar. An (i) on the review banner (visible on load, before anything is selected) and one on the bulk bar (in context with the buttons) open the same modal. Each action gets its name, what it means, and what it does on apply — copy grounded in resolveFaces, not in what the labels imply. The names are not re-declared: the modal reuses each button's own bulk-bar i18n key, so a translated heading can never drift from its translated button. STATE_COLOR moves to review.svelte.ts so the page and the modal read the five colors from one source. * fix(face-cleanup): never drain a person on a resolve that settles no flagged face resolveFaces drained the reviewed person from the console on ANY committed resolve. The web review page fired a second, independent resolve for its rest-of-cluster selection, so moving one unflagged face dropped the person from the scan while every flagged face was still awaiting a decision — the admin's staged review was silently discarded, and the same faces came back on the next scan. That is the bug behind "I cleaned this person up and it reappeared". Drop-on-any-resolution becomes drop-on-SETTLED-resolution: drain only when the resolve accounts for every face in the person's flagged snapshot (a kept/locked face IS settled, so the stay-only case still drains), or on entireCluster, which moves the whole cluster by definition. * fix(face-cleanup): one terminal Apply, never swallow a rejected resolve, icon per state The review page had two independent commits. "Apply · N faces" sent only the flagged review model, ignoring any rest-of-cluster faces ticked below it; "Move N faces" sent only the rest faces, ignoring every staged flagged decision. Either one closed the person out of the console having done half the job, and neither told the admin what it had done. Worse, the rest-of-cluster and whole-cluster paths swallowed every error from resolveFaces in an empty catch — including the server's 409 ("Refusing to apply while a scan is in progress"). Confirming "Move entire cluster" during a re-scan therefore moved nothing, showed nothing, and left the admin believing it had worked until the next scan handed the same faces back. Now: ticking a rest-of-cluster face STAGES it into the single Apply (the dock counts it, "+N added"), every resolve funnels through one commit path that surfaces failures in the existing error banner, and a successful apply reports the server's own counts (moved/kept/locked/detached/skipped) instead of blind navigating. "Move entire cluster" stays a separate explicit commit. Also: STATE_ICON gives every state its own glyph (arrow → owner, pin → keep, lock → confirm, person-arrow → move, image-off → not a face) on the tile badge, bulk button, tally chip and help modal. The badge used to stamp one check mark on owner/stay/other, leaving indigo-vs-violet as the only thing separating "moved away" from "locked in place" — unreadable for a colourblind admin. * feat(face-cleanup): tell the admin what to do after a scan The first screen after a scan opens with five stat cards, four filters, a search box, a pre-selected bulk bar and a grouped table — and never says what any of it is for or what to do next. The review-first/confident split, which IS the workflow, is explained nowhere. And the riskiest fact on the page is silent: the confident clusters arrive pre-selected, so the biggest button on screen re-attributes all of them in one click. A three-step checklist replaces the description paragraph (its gist becomes the subtitle, so the page gains guidance without gaining height): open the ones that need a decision, note that the confident ones are already selected, then commit. It reads real scan state rather than being a static blurb, so it doubles as progress: the model already gates a review-first cluster's selectability on having opened it, so "3 of 16 opened" is state that existed and was never shown. Step 1 ticks green when nothing is left, step 2 goes quiet when a scan produced no confident clusters, step 3 when nothing is selected. * fix(face-cleanup): drop the checklist's "Review first" shortcut button It only flipped the filter chip, but it looked like navigation — and its effect landed in the table far below the fold, so clicking it appeared to do nothing. The checklist is guidance; the filter toolbar is the control surface, and its "Review first" chip is right there. A test pins the checklist as informational: it renders no buttons of its own. * feat(face-cleanup): park unnamed strangers in their own cluster, and make "Not a face" stick An admin reviewing someone else's library constantly hits a face they know does not belong to the cluster but cannot name — a friend of the family. Every existing action was a lie for that face (moving it to the suggested owner is wrong, keeping it is wrong, and "Not a face" is wrong because it plainly IS a face), so the review could not be finished. "Unknown person" parks the selected faces in a fresh unnamed cluster of their own and locks them there. Deliberately NOT a bare unassign back to the "unknown pool": queueRecognizeFaces streams every personId IS NULL face back through recognition, which re-matches it onto its nearest neighbour-with-a-person — very often the cluster it was just pulled out of. Giving the face a person of its own is what makes the decision stick, and it still surfaces as an unnamed cluster on the People page for the owner (or face-recognition suggestions) to name later. Same bug made "Not a face" hollow: it only nulled personId, so a detached crop was re-queued and could be re-assigned, contradicting the console's promise that it "stops being proposed for anyone". Detach now soft-deletes the face, the same primitive the face editor's delete-face uses, which every recognition-candidate query filters on. Also repairs the German console: three keys were translations of the pre-redesign strings and rendered raw {moving}/{name}/{faceCount} placeholders, and 76 were missing entirely — so the one sentence that enumerates the actions never reached the (German) admin whose feedback this is. All 173 face-cleanup keys are now translated, with placeholder parity checked against en.json. Fixes a pre-existing svelte-check error in ActionsHelpModal ($t given a widened string) that would have failed CI Lint Web. * fix(face-cleanup): pin the review dock to the bottom instead of letting it float mid-page The dock was `sticky bottom-0` inside the scrolled content. Sticky only pins while its containing block still extends below it, and the containing block here is the auto-height Container inside the Scrollable — so on a short review (a handful of flagged faces) the page never overflows, sticky is inert, and the bar comes to rest wherever the content happens to end: adrift in the middle of the page, with the content's pb-32 opening a further gap beneath it. Render it through a new optional `footer` snippet on AdminPageLayout / BreadcrumbActionPage instead, as a sibling of the scroll area rather than inside it. That pins it at any content length, lets the grid scroll above it rather than underneath, and drops the pb-32 that only existed to reserve space for it. It still never overlaps the sidebar, which is why `fixed` was rejected originally. The footer renders no wrapper element, so the 16 other admin pages that pass no footer are untouched. * docs(face-cleanup): prettier-format the plan and design docs CI Docs Build runs `prettier --check .` over docs/ and was already failing on this branch: three face-cleanup markdown files landed unformatted. * fix(face-cleanup): confirm before discarding faces, and guard the bugs review found "Not a face" is now irreversible (it soft-deletes the detected face so recognition can never hand it back), it sits one button away from "Unknown person", and the two mean opposite things. A slip between them destroys real face data with no undo anywhere in the app. An Apply carrying any discarded face now has to be confirmed; everything else still applies straight through, because a prompt on every Apply trains you to click past the one that matters. Review also turned up three things nothing was guarding: - The German placeholder bug was NOT only German. fr.json carried the identical stale face-cleanup strings, and mr/ms had the ARGUMENT NAME itself translated ({संख्या}, {bulan}) — all render as literal braces to the user. Fixed all five, and added web/src/lib/i18n/placeholders.spec.ts, which parses ICU properly (a naive regex mistakes plural branches like `one {hour}` for placeholders) and fails if any locale references an argument en.json does not supply. Verified it goes red on the original bug. CI's "Test i18n" job only ran prettier, which is why this class of bug reached a user. - resolveFaces created the unknown cluster BEFORE moving faces into it, outside any try/catch: a failure in between stranded a nameless, faceless person on the owner's People page forever. Now removed on failure — but only when genuinely empty, since asset_face.personId is ON DELETE SET NULL and deleting a cluster that did receive faces would dump them back into the recognition pool this action exists to keep them out of. - The e2e named "routing every state" covered five of six. Adds the unknown state, plus a test that the dock is pinned to the viewport bottom on a short review — the float bug no component test can catch, because the AdminPageLayout stub renders children and footer into the same element. Also covers the unknown cluster's representative face and thumbnail (a cluster with no avatar is unnameable on the People page, which is the whole point of parking it) and a combined move+unknown resolve pinning the disjoint moved/unknown counts. * test(e2e): measure the dock against the app shell's inset, not the raw viewport The pinned-dock assertion demanded the dock's bottom edge land within 2px of the viewport bottom. It measured 712 of 720: the app shell insets its content region by ~8px on every admin page, so a correctly pinned dock never reaches the raw viewport edge. Loosen to 16px, which still sits an order of magnitude clear of the bug it guards (the floating dock measured ~124px short of the bottom), and say so in the test. Distance-to-bottom is the ONLY thing separating the two states — the floating dock was below the content too, just adrift — so the "below the grid" check is labelled as the sanity check it is, not as the guard.
… 500, cross-owner guard Addresses the multi-agent review of PR #770. Correctness / edge: - C1: the console-drain check compared the RAW flagged snapshot, so a person soft-stayed or locked across sessions could never drain (the settled face is filtered out of the review UI and can't be re-submitted). Measure the drain against the decline/lock-filtered pending set (`resolvable`) instead. - C7: "Keep here" 500'd when a stayed face's scan-suggested owner was deleted/merged after the scan — the decline insert hit the person FK (23503), after any moves already committed. Drop stay faces whose owner is gone (they are already effectively kept); still settles the face. - C2: moveToPerson silently dropped a previously soft-stayed face moved to a DIFFERENT chosen destination. Scope the pre-skip to the requested destination (still skips locked faces and a re-move toward the same declined owner). Security / RBAC: - C6 (defense-in-depth): executeRepair refuses a cross-owner route at the write layer, so no caller — including the legacy POST /admin/face-repair path — can collapse two owners. - C4: bound every resolve/decline face-array at 1000 (admin-only DoS backstop; mirrors the existing max(200) on cluster-faces). Maintainability / coverage: - C13/C15: drop removeLocks's dead natural-key branch; refresh a stale DTO doc comment. - C9: controller auth tests for GET scan/latest and scan/person/:personId (+ non-uuid 400). - C10: boundary test for a resolve with no persisted scan. Regression tests cover every fix. Regenerated the OpenAPI spec (maxItems). Larger refactors (tri-naming rename, resolveFaces god-method split, repository dedup) intentionally deferred to a separate change.
…collision Rebasing onto current main lands this branch on top of the space-albums migration 1783000000000-AddAlbumSpaceAssetTable (#752), which took the 1783000000000 slot after this branch's previous base was cut. Two migrations-gallery files then shared that timestamp. #752's migration is already shipped on main, so it keeps its slot; renumber the not-yet-merged fork migration to 1783050000000 (still after AddFaceRepairScan@1780000000000 for its index target and before FixFaceRepairScanInFlightIndexOverride@1784000000000, so apply order is unchanged). Update the revert-to-immich.sql entry and the override migration's comment reference to match.
…people main's shared-space RBAC/visibility hardening made getPersonsBySpaceId's "has a visible face on an asset reachable from the space" EXISTS unconditional — it used to apply only when a takenAfter/takenBefore filter was supplied. The createPersonForIdentity idempotency test built a space person but never linked a face, so after the rebase the read-back returned [] and the assertion failed. createPersonForIdentity only records representativeFaceId; the real face-match path links the face separately via addPersonFaces. Do the same in the fixture, matching every other test in this file. The assertion (exactly one row, and it is the winner) is unchanged.
Deeds67
force-pushed
the
feat/face-cleanup-resolution
branch
from
July 22, 2026 18:12
34117fc to
34d46d1
Compare
Deeds67
added a commit
that referenced
this pull request
Jul 23, 2026
… suite X1 (unmodified from #770) was silently SKIPPED in earlier runs because Playwright serial-skips tests after a failure, so it never actually validated. Once keep-here passed, X1 ran and failed with 0 flagged tiles: my keep-here test seeded a completed scan BEFORE X1, which #770 never did (its pre-X1 test seeded a decline, not a scan). The server handles this correctly (proven by a medium repro), but restoring #770's order — X1 as the first scan-seeder, my new test last — removes the cross-test scan-state coupling in the e2e stack.
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
2 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.
What this does
Turns the Face Cleanup review console (#664) into a full per-face resolution system. Every flagged face gets exactly one durable terminal state, and the person drains from the console on Apply:
Supporting changes:
POST /admin/face-repair/resolveendpoint; the old 2-state apply endpoint is retired.resolve./admin/face-cleanup/resolutions) listing declines + locks, each undoable; the old/admin/face-cleanup/declinedroute redirects here.AddFaceRepairScanFlaggedFace,AddFaceRepairScanInFlightIndex,FixFaceRepairScanInFlightIndexOverride,AddFaceRepairLock.This branch is a superset of #664 — it contains the entire review-console feature and layers the resolution UX/endpoints on top. Supersedes #664, which can be closed once this lands.
Base / rebase
Rebased onto current
main(Immich v3.0.2). Two base-drift fixes were needed:AddFaceRepairScanFlaggedFacecollided with main's List all assets whose description, filename or OCR matches a search term #722AddAssetExifDescriptionTrigramIndexat1782000000000(they didn't collide before, since this work predates List all assets whose description, filename or OCR matches a search term #722). Themigration-timestampsguard rejects new collisions, so the not-yet-deployed fork migration was renumbered to1781500000000— same apply order, and List all assets whose description, filename or OCR matches a search term #722 keeps its already-deployed slot.CI
All gating workflows dispatched on the branch are green: Test, Static Code Analysis, Docker, Storage Migration Tests, Revert-to-Immich Validation. (Docs Build + mobile smoke will run here on the PR; the mobile Dart regen was zero-diff.)
Manual testing (what CI can't cover)
/admin/face-cleanup/[personId](needs a completed scan with flagged faces): apply each bulk action; verify tile chips/colors (owner indigo, keep green, lock violet, other amber, detach slate + grayscale) and that the tally always sums to N. Apply → the person drains; spot-check faces landed correctly.Known follow-ups (non-blocking)
GET/DELETE /declineserver routes.face_cleanup_*keys in the de/fr locale files.AddFaceRepairScanFlaggedFaceunder the old1782000000000name — reset that clone rather than upgrading in place (fresh installs are unaffected).