From e223e0ddf9496e562f4e5038dfd1d78b91caa137 Mon Sep 17 00:00:00 2001 From: midzelis Date: Sat, 7 Mar 2026 21:14:45 +0000 Subject: [PATCH] fix(web): preserve stacked asset selection when tagging faces --- .../ui/generators/timeline/rest-response.ts | 10 +- .../ui/mock-network/face-editor-network.ts | 120 ++++++++++++++++++ .../asset-viewer/face-overlay.e2e-spec.ts | 60 +++++++++ .../asset-viewer/stack-face-tag.e2e-spec.ts | 100 +++++++++++++++ .../asset-viewer/asset-viewer.svelte | 82 ++++++++---- .../asset-viewer/detail-panel.svelte | 18 +-- .../face-editor/face-editor.svelte | 6 +- .../asset-viewer/photo-viewer.svelte | 11 +- .../faces-page/person-side-panel.svelte | 8 +- 9 files changed, 363 insertions(+), 52 deletions(-) create mode 100644 e2e/src/ui/specs/asset-viewer/face-overlay.e2e-spec.ts create mode 100644 e2e/src/ui/specs/asset-viewer/stack-face-tag.e2e-spec.ts diff --git a/e2e/src/ui/generators/timeline/rest-response.ts b/e2e/src/ui/generators/timeline/rest-response.ts index 0c4bd06dc3..d677edbcb3 100644 --- a/e2e/src/ui/generators/timeline/rest-response.ts +++ b/e2e/src/ui/generators/timeline/rest-response.ts @@ -284,7 +284,11 @@ const createDefaultOwner = (ownerId: string) => { * Convert a TimelineAssetConfig to a full AssetResponseDto * This matches the response from GET /api/assets/:id */ -export function toAssetResponseDto(asset: MockTimelineAsset, owner?: UserResponseDto): AssetResponseDto { +export function toAssetResponseDto( + asset: MockTimelineAsset, + owner?: UserResponseDto, + overrides?: Partial>, +): AssetResponseDto { const now = new Date().toISOString(); // Default owner if not provided @@ -338,8 +342,8 @@ export function toAssetResponseDto(asset: MockTimelineAsset, owner?: UserRespons exifInfo, livePhotoVideoId: asset.livePhotoVideoId, tags: [], - people: [], - unassignedFaces: [], + people: overrides?.people ?? [], + unassignedFaces: overrides?.unassignedFaces ?? [], stack: asset.stack, isOffline: false, hasMetadata: true, diff --git a/e2e/src/ui/mock-network/face-editor-network.ts b/e2e/src/ui/mock-network/face-editor-network.ts index 778f04baf9..c52c435a41 100644 --- a/e2e/src/ui/mock-network/face-editor-network.ts +++ b/e2e/src/ui/mock-network/face-editor-network.ts @@ -1,3 +1,9 @@ +import { + type AssetFaceResponseDto, + type AssetFaceWithoutPersonResponseDto, + type AssetResponseDto, + type PersonWithFacesResponseDto, +} from '@immich/sdk'; import { BrowserContext } from '@playwright/test'; import { randomThumbnail } from 'src/ui/generators/timeline'; @@ -125,3 +131,117 @@ export const setupFaceEditorMockApiRoutes = async ( }); }); }; + +export type MockFaceSpec = { + personId: string; + personName: string; + faceId: string; + boundingBoxX1: number; + boundingBoxY1: number; + boundingBoxX2: number; + boundingBoxY2: number; +}; + +export const createMockFaceData = ( + faceSpecs: MockFaceSpec[], + imageWidth: number, + imageHeight: number, +): { people: PersonWithFacesResponseDto[]; unassignedFaces: AssetFaceWithoutPersonResponseDto[] } => { + const people: PersonWithFacesResponseDto[] = faceSpecs.map((spec) => ({ + id: spec.personId, + name: spec.personName, + birthDate: null, + isHidden: false, + thumbnailPath: `/upload/thumbs/${spec.personId}.jpeg`, + updatedAt: new Date().toISOString(), + faces: [ + { + id: spec.faceId, + imageWidth, + imageHeight, + boundingBoxX1: spec.boundingBoxX1, + boundingBoxY1: spec.boundingBoxY1, + boundingBoxX2: spec.boundingBoxX2, + boundingBoxY2: spec.boundingBoxY2, + }, + ], + })); + + return { people, unassignedFaces: [] }; +}; + +export const setupFaceOverlayMockApiRoutes = async ( + context: BrowserContext, + assetDto: AssetResponseDto, + faceSpecs: MockFaceSpec[], +) => { + const faceResponseMap = new Map(); + for (const spec of faceSpecs) { + faceResponseMap.set(spec.faceId, { + id: spec.faceId, + imageWidth: assetDto.width ?? 3000, + imageHeight: assetDto.height ?? 4000, + boundingBoxX1: spec.boundingBoxX1, + boundingBoxY1: spec.boundingBoxY1, + boundingBoxX2: spec.boundingBoxX2, + boundingBoxY2: spec.boundingBoxY2, + person: { + id: spec.personId, + name: spec.personName, + birthDate: null, + isHidden: false, + thumbnailPath: `/upload/thumbs/${spec.personId}.jpeg`, + updatedAt: new Date().toISOString(), + }, + }); + } + + await context.route(`**/api/assets/${assetDto.id}`, async (route, request) => { + if (request.method() !== 'GET') { + return route.fallback(); + } + return route.fulfill({ + status: 200, + contentType: 'application/json', + json: assetDto, + }); + }); + + await context.route(`**/api/faces?id=${assetDto.id}`, async (route, request) => { + if (request.method() !== 'GET') { + return route.fallback(); + } + return route.fulfill({ + status: 200, + contentType: 'application/json', + json: [...faceResponseMap.values()], + }); + }); + + await context.route('**/api/faces/*', async (route, request) => { + if (request.method() !== 'DELETE') { + return route.fallback(); + } + const url = new URL(request.url()); + const faceId = url.pathname.split('/').at(-1); + if (faceId) { + faceResponseMap.delete(faceId); + } + return route.fulfill({ + status: 200, + contentType: 'text/plain', + body: 'OK', + }); + }); + + await context.route('**/api/people/*/thumbnail', async (route) => { + if (!route.request().serviceWorker()) { + return route.continue(); + } + return route.fulfill({ + status: 200, + headers: { 'content-type': 'image/jpeg' }, + body: await randomThumbnail('person-thumb', 1), + }); + }); +}; diff --git a/e2e/src/ui/specs/asset-viewer/face-overlay.e2e-spec.ts b/e2e/src/ui/specs/asset-viewer/face-overlay.e2e-spec.ts new file mode 100644 index 0000000000..081541a108 --- /dev/null +++ b/e2e/src/ui/specs/asset-viewer/face-overlay.e2e-spec.ts @@ -0,0 +1,60 @@ +import { expect, test } from '@playwright/test'; +import { toAssetResponseDto } from 'src/ui/generators/timeline'; +import { + createMockFaceData, + type MockFaceSpec, + setupFaceOverlayMockApiRoutes, +} from 'src/ui/mock-network/face-editor-network'; +import { assetViewerUtils } from '../timeline/utils'; +import { ensureDetailPanelVisible, setupAssetViewerFixture } from './utils'; + +test.describe.configure({ mode: 'parallel' }); + +test.describe('face removal auto-close', () => { + const fixture = setupAssetViewerFixture(903); + const singleFaceSpec: MockFaceSpec[] = [ + { + personId: 'person-solo', + personName: 'Solo Person', + faceId: 'face-solo', + boundingBoxX1: 1000, + boundingBoxY1: 500, + boundingBoxX2: 1500, + boundingBoxY2: 1200, + }, + ]; + + test.beforeEach(async ({ context }) => { + const faceData = createMockFaceData( + singleFaceSpec, + fixture.primaryAssetDto.width ?? 3000, + fixture.primaryAssetDto.height ?? 4000, + ); + const assetDtoWithFaces = toAssetResponseDto(fixture.primaryAsset, undefined, faceData); + await setupFaceOverlayMockApiRoutes(context, assetDtoWithFaces, singleFaceSpec); + }); + + test('person side panel closes when last face is removed', async ({ page }) => { + await page.goto(`/photos/${fixture.primaryAsset.id}`); + await assetViewerUtils.waitForViewerLoad(page, fixture.primaryAsset); + + await ensureDetailPanelVisible(page); + + const editPeopleButton = page.locator('#detail-panel').getByLabel('Edit people'); + await expect(editPeopleButton).toBeVisible(); + await editPeopleButton.click(); + + const personName = page.locator('text=Solo Person'); + await expect(personName.first()).toBeVisible({ timeout: 5000 }); + + const deleteButton = page.getByLabel('Delete face'); + await expect(deleteButton).toBeVisible(); + await deleteButton.click(); + + const confirmButton = page.getByRole('button', { name: /confirm/i }); + await expect(confirmButton).toBeVisible(); + await confirmButton.click(); + + await expect(page.locator('text=Edit faces')).toBeHidden({ timeout: 5000 }); + }); +}); diff --git a/e2e/src/ui/specs/asset-viewer/stack-face-tag.e2e-spec.ts b/e2e/src/ui/specs/asset-viewer/stack-face-tag.e2e-spec.ts new file mode 100644 index 0000000000..1e27d1b12f --- /dev/null +++ b/e2e/src/ui/specs/asset-viewer/stack-face-tag.e2e-spec.ts @@ -0,0 +1,100 @@ +import { type AssetResponseDto } from '@immich/sdk'; +import { expect, test } from '@playwright/test'; +import { toAssetResponseDto } from 'src/ui/generators/timeline'; +import { + createMockStack, + createMockStackAsset, + MockStack, + setupBrokenAssetMockApiRoutes, +} from 'src/ui/mock-network/broken-asset-network'; +import { + createMockPeople, + FaceCreateCapture, + MockPerson, + setupFaceEditorMockApiRoutes, +} from 'src/ui/mock-network/face-editor-network'; +import { assetViewerUtils } from '../timeline/utils'; +import { ensureDetailPanelVisible, setupAssetViewerFixture } from './utils'; + +test.describe.configure({ mode: 'parallel' }); +test.describe('stack face-tag selection preservation', () => { + const fixture = setupAssetViewerFixture(910); + let mockStack: MockStack; + let primaryAssetDto: AssetResponseDto; + let secondAssetDto: AssetResponseDto; + let mockPeople: MockPerson[]; + let faceCreateCapture: FaceCreateCapture; + + test.beforeAll(async () => { + primaryAssetDto = toAssetResponseDto(fixture.primaryAsset); + secondAssetDto = createMockStackAsset(fixture.adminUserId); + secondAssetDto.originalFileName = 'second-stacked-asset.jpg'; + mockStack = createMockStack(primaryAssetDto, [secondAssetDto], new Set()); + mockPeople = createMockPeople(3); + }); + + test.beforeEach(async ({ context }) => { + faceCreateCapture = { requests: [] }; + await setupBrokenAssetMockApiRoutes(context, mockStack); + await setupFaceEditorMockApiRoutes(context, mockPeople, faceCreateCapture); + }); + + test('selected stacked asset is preserved after tagging a face', async ({ page }) => { + await page.goto(`/photos/${fixture.primaryAsset.id}`); + await assetViewerUtils.waitForViewerLoad(page, fixture.primaryAsset); + + const stackSlideshow = page.locator('#stack-slideshow'); + await expect(stackSlideshow).toBeVisible(); + + const stackThumbnails = stackSlideshow.locator('[data-asset]'); + await expect(stackThumbnails).toHaveCount(2); + + await stackThumbnails.nth(1).click(); + + await ensureDetailPanelVisible(page); + await expect(page.locator('#detail-panel')).toContainText('second-stacked-asset.jpg'); + + await page.getByLabel('Tag people').click(); + await page.locator('#face-selector').waitFor({ state: 'visible' }); + + await page.locator('#face-selector').getByText(mockPeople[0].name).click(); + + const confirmButton = page.getByRole('button', { name: /confirm/i }); + await expect(confirmButton).toBeVisible(); + await confirmButton.click(); + + await expect(page.locator('#face-selector')).toBeHidden(); + + expect(faceCreateCapture.requests).toHaveLength(1); + expect(faceCreateCapture.requests[0].assetId).toBe(secondAssetDto.id); + + await expect(page.locator('#detail-panel')).toContainText('second-stacked-asset.jpg'); + + const selectedThumbnail = stackSlideshow.locator(`[data-asset="${secondAssetDto.id}"]`); + await expect(selectedThumbnail).toBeVisible(); + }); + + test('primary asset stays selected after tagging a face without switching', async ({ page }) => { + await page.goto(`/photos/${fixture.primaryAsset.id}`); + await assetViewerUtils.waitForViewerLoad(page, fixture.primaryAsset); + + await ensureDetailPanelVisible(page); + await expect(page.locator('#detail-panel')).toContainText(primaryAssetDto.originalFileName); + + await page.getByLabel('Tag people').click(); + await page.locator('#face-selector').waitFor({ state: 'visible' }); + + await page.locator('#face-selector').getByText(mockPeople[0].name).click(); + + const confirmButton = page.getByRole('button', { name: /confirm/i }); + await expect(confirmButton).toBeVisible(); + await confirmButton.click(); + + await expect(page.locator('#face-selector')).toBeHidden(); + + expect(faceCreateCapture.requests).toHaveLength(1); + expect(faceCreateCapture.requests[0].assetId).toBe(primaryAssetDto.id); + + await expect(page.locator('#detail-panel')).toContainText(primaryAssetDto.originalFileName); + }); +}); diff --git a/web/src/lib/components/asset-viewer/asset-viewer.svelte b/web/src/lib/components/asset-viewer/asset-viewer.svelte index 3f7b048c8f..d448738ec2 100644 --- a/web/src/lib/components/asset-viewer/asset-viewer.svelte +++ b/web/src/lib/components/asset-viewer/asset-viewer.svelte @@ -10,6 +10,7 @@ import { AssetAction, ProjectionType } from '$lib/constants'; import { activityManager } from '$lib/managers/activity-manager.svelte'; import { assetViewerManager } from '$lib/managers/asset-viewer-manager.svelte'; + import { assetCacheManager } from '$lib/managers/AssetCacheManager.svelte'; import { authManager } from '$lib/managers/auth-manager.svelte'; import { editManager, EditToolType } from '$lib/managers/edit/edit-manager.svelte'; import { eventManager } from '$lib/managers/event-manager.svelte'; @@ -100,10 +101,11 @@ const stackThumbnailSize = 60; const stackSelectedThumbnailSize = 65; + let stack: StackResponseDto | undefined = $state(); + let selectedStackAsset: AssetResponseDto | undefined = $state(); let previewStackedAsset: AssetResponseDto | undefined = $state(); - let stack: StackResponseDto | null = $state(null); - const asset = $derived(previewStackedAsset ?? cursor.current); + const asset = $derived(previewStackedAsset ?? selectedStackAsset ?? cursor.current); const nextAsset = $derived(cursor.nextAsset); const previousAsset = $derived(cursor.previousAsset); let sharedLink = getSharedLink(); @@ -116,17 +118,25 @@ playOriginalVideo = value; }; + const selectStackedAsset = async (id: string) => { + selectedStackAsset = await assetCacheManager.getAsset({ id }); + }; + const refreshStack = async () => { if (authManager.isSharedLink || !withStacked) { return; } - if (asset.stack) { - stack = await getStack({ id: asset.stack.id }); + if (!cursor.current.stack) { + stack = undefined; + selectedStackAsset = undefined; + return; } - if (!stack?.assets.some(({ id }) => id === asset.id)) { - stack = null; + stack = await getStack({ id: cursor.current.stack.id }); + const primaryAsset = stack?.assets.find(({ id }) => id === stack?.primaryAssetId); + if (primaryAsset) { + await selectStackedAsset(primaryAsset.id); } }; @@ -184,11 +194,21 @@ onClose?.(asset); }; + const refreshPreservingSelection = async () => { + const id = asset.id; + assetCacheManager.invalidateAsset(id); + if (selectedStackAsset) { + await selectStackedAsset(id); + } else { + const asset = await assetCacheManager.getAsset({ id }); + assetViewingStore.setAsset(asset); + } + onAssetChange?.(asset); + }; + const closeEditor = async () => { if (editManager.hasAppliedEdits) { - const refreshedAsset = await getAssetInfo({ id: asset.id }); - onAssetChange?.(refreshedAsset); - assetViewingStore.setAsset(refreshedAsset); + await refreshPreservingSelection(); } assetViewerManager.closeEditor(); }; @@ -288,10 +308,6 @@ } }; - const handleStackedAssetMouseEvent = (isMouseOver: boolean, stackedAsset: AssetResponseDto) => { - previewStackedAsset = isMouseOver ? stackedAsset : undefined; - }; - const handlePreAction = (action: Action) => { preAction?.(action); }; @@ -304,7 +320,7 @@ break; } case AssetAction.REMOVE_ASSET_FROM_STACK: { - stack = action.stack; + stack = action.stack ?? undefined; if (stack) { cursor.current = stack.assets[0]; } @@ -371,7 +387,7 @@ $effect(() => { // eslint-disable-next-line @typescript-eslint/no-unused-expressions - asset; + cursor.current; untrack(() => handlePromiseError(refresh())); }); @@ -535,7 +551,12 @@ {:else if viewerKind === 'CropArea'} {:else if viewerKind === 'PhotoViewer'} - + {:else if viewerKind === 'VideoViewer'} {#if showDetailPanel}
- +
{:else if assetViewerManager.isShowEditor}
@@ -598,10 +619,14 @@ {#if stack && withStacked && !assetViewerManager.isShowEditor} {@const stackedAssets = stack.assets}
-
+ diff --git a/web/src/lib/components/asset-viewer/detail-panel.svelte b/web/src/lib/components/asset-viewer/detail-panel.svelte index e80d376f57..4ef1201f2b 100644 --- a/web/src/lib/components/asset-viewer/detail-panel.svelte +++ b/web/src/lib/components/asset-viewer/detail-panel.svelte @@ -20,13 +20,7 @@ import { handleError } from '$lib/utils/handle-error'; import { fromISODateTime, fromISODateTimeUTC, toTimelineAsset } from '$lib/utils/timeline-util'; import { getParentPath } from '$lib/utils/tree-utils'; - import { - AssetMediaSize, - getAllAlbums, - getAssetInfo, - type AlbumResponseDto, - type AssetResponseDto, - } from '@immich/sdk'; + import { AssetMediaSize, getAllAlbums, type AlbumResponseDto, type AssetResponseDto } from '@immich/sdk'; import { Icon, IconButton, LoadingSpinner, modalManager, Text } from '@immich/ui'; import { mdiCalendar, @@ -52,9 +46,10 @@ interface Props { asset: AssetResponseDto; currentAlbum?: AlbumResponseDto | null; + onRefreshPeople?: () => Promise; } - let { asset, currentAlbum = null }: Props = $props(); + let { asset, currentAlbum = null, onRefreshPeople }: Props = $props(); let showAssetPath = $state(false); let showEditFaces = $state(false); @@ -120,11 +115,6 @@ return undefined; }; - const handleRefreshPeople = async () => { - asset = await getAssetInfo({ id: asset.id }); - showEditFaces = false; - }; - const getAssetFolderHref = (asset: AssetResponseDto) => { // Remove the last part of the path to get the parent path return Route.folders({ path: getParentPath(asset.originalPath) }); @@ -575,6 +565,6 @@ assetId={asset.id} assetType={asset.type} onClose={() => (showEditFaces = false)} - onRefresh={handleRefreshPeople} + onRefresh={() => void onRefreshPeople?.()} /> {/if} diff --git a/web/src/lib/components/asset-viewer/face-editor/face-editor.svelte b/web/src/lib/components/asset-viewer/face-editor/face-editor.svelte index 8b3d672bfe..3674444934 100644 --- a/web/src/lib/components/asset-viewer/face-editor/face-editor.svelte +++ b/web/src/lib/components/asset-viewer/face-editor/face-editor.svelte @@ -1,6 +1,5 @@