From 4b1ced439be597e0da8b4f0fbfb702c19ccffae8 Mon Sep 17 00:00:00 2001 From: Min Idzelis Date: Wed, 30 Apr 2025 00:19:38 -0400 Subject: [PATCH 01/16] feat: improve/refactor focus handling (#17796) * feat: improve focus * test * lint * use modulus in loop --- .../lib/actions/__test__/focus-trap.spec.ts | 4 ++ web/src/lib/actions/focus-trap.ts | 19 +++++---- .../thumbnail/__test__/thumbnail.spec.ts | 41 +++++++++---------- .../assets/thumbnail/thumbnail.svelte | 35 +++------------- .../elements/buttons/skip-link.svelte | 8 +++- .../photos-page/asset-date-group.svelte | 5 --- .../components/photos-page/asset-grid.svelte | 31 ++------------ .../gallery-viewer/gallery-viewer.svelte | 28 ++----------- .../scrubber/scrubber.svelte | 4 +- .../lib/stores/asset-interaction.svelte.ts | 5 --- web/src/lib/utils/focus-util.ts | 41 +++++++++++++++++-- 11 files changed, 92 insertions(+), 129 deletions(-) diff --git a/web/src/lib/actions/__test__/focus-trap.spec.ts b/web/src/lib/actions/__test__/focus-trap.spec.ts index d92d8e037d..b03064a91d 100644 --- a/web/src/lib/actions/__test__/focus-trap.spec.ts +++ b/web/src/lib/actions/__test__/focus-trap.spec.ts @@ -1,8 +1,11 @@ import FocusTrapTest from '$lib/actions/__test__/focus-trap-test.svelte'; +import { setDefaultTabbleOptions } from '$lib/utils/focus-util'; import { render, screen } from '@testing-library/svelte'; import userEvent from '@testing-library/user-event'; import { tick } from 'svelte'; +setDefaultTabbleOptions({ displayCheck: 'none' }); + describe('focusTrap action', () => { const user = userEvent.setup(); @@ -38,6 +41,7 @@ describe('focusTrap action', () => { const openButton = screen.getByText('Open'); await user.click(openButton); + await tick(); expect(document.activeElement).toEqual(screen.getByTestId('one')); screen.getByText('Close').click(); diff --git a/web/src/lib/actions/focus-trap.ts b/web/src/lib/actions/focus-trap.ts index 1564dd90d0..2b03282c2d 100644 --- a/web/src/lib/actions/focus-trap.ts +++ b/web/src/lib/actions/focus-trap.ts @@ -1,5 +1,5 @@ import { shortcuts } from '$lib/actions/shortcut'; -import { getFocusable } from '$lib/utils/focus-util'; +import { getTabbable } from '$lib/utils/focus-util'; import { tick } from 'svelte'; interface Options { @@ -18,18 +18,21 @@ export function focusTrap(container: HTMLElement, options?: Options) { }; }; - const setInitialFocus = () => { - const focusableElement = getFocusable(container)[0]; - // Use tick() to ensure focus trap works correctly inside - void tick().then(() => focusableElement?.focus()); + const setInitialFocus = async () => { + const focusableElement = getTabbable(container, false)[0]; + if (focusableElement) { + // Use tick() to ensure focus trap works correctly inside + await tick(); + focusableElement?.focus(); + } }; if (withDefaults(options).active) { - setInitialFocus(); + void setInitialFocus(); } const getFocusableElements = () => { - const focusableElements = getFocusable(container); + const focusableElements = getTabbable(container); return [ focusableElements.at(0), // focusableElements.at(-1), @@ -67,7 +70,7 @@ export function focusTrap(container: HTMLElement, options?: Options) { update(newOptions?: Options) { options = newOptions; if (withDefaults(options).active) { - setInitialFocus(); + void setInitialFocus(); } }, destroy() { diff --git a/web/src/lib/components/assets/thumbnail/__test__/thumbnail.spec.ts b/web/src/lib/components/assets/thumbnail/__test__/thumbnail.spec.ts index 503ea8aefd..7f6a9d588e 100644 --- a/web/src/lib/components/assets/thumbnail/__test__/thumbnail.spec.ts +++ b/web/src/lib/components/assets/thumbnail/__test__/thumbnail.spec.ts @@ -1,7 +1,8 @@ import { getIntersectionObserverMock } from '$lib/__mocks__/intersection-observer.mock'; import Thumbnail from '$lib/components/assets/thumbnail/thumbnail.svelte'; +import { getTabbable } from '$lib/utils/focus-util'; import { assetFactory } from '@test-data/factories/asset-factory'; -import { fireEvent, render, screen } from '@testing-library/svelte'; +import { fireEvent, render } from '@testing-library/svelte'; vi.hoisted(() => { Object.defineProperty(globalThis, 'matchMedia', { @@ -31,51 +32,47 @@ describe('Thumbnail component', () => { it('should only contain a single tabbable element (the container)', () => { const asset = assetFactory.build({ originalPath: 'image.jpg', originalMimeType: 'image/jpeg' }); - render(Thumbnail, { + const { baseElement } = render(Thumbnail, { asset, - focussed: false, selected: true, }); - const container = screen.getByTestId('container-with-tabindex'); - expect(container.getAttribute('tabindex')).toBe('0'); + const container = baseElement.querySelector('[data-thumbnail-focus-container]'); + expect(container).not.toBeNull(); + expect(container!.getAttribute('tabindex')).toBe('0'); - // This isn't capturing all tabbable elements, but should be the most likely ones. Mainly guarding against - // inserting extra tabbable elments in future in - let allTabbableElements = screen.queryAllByRole('link'); - allTabbableElements = allTabbableElements.concat(screen.queryAllByRole('checkbox')); - expect(allTabbableElements.length).toBeGreaterThan(0); - for (const tabbableElement of allTabbableElements) { - const testIdValue = tabbableElement.dataset.testid; - if (testIdValue === null || testIdValue !== 'container-with-tabindex') { - expect(tabbableElement.getAttribute('tabindex')).toBe('-1'); - } - } + // Guarding against inserting extra tabbable elments in future in + const tabbables = getTabbable(container!); + expect(tabbables.length).toBe(0); }); it('handleFocus should be called on focus of container', async () => { const asset = assetFactory.build({ originalPath: 'image.jpg', originalMimeType: 'image/jpeg' }); const handleFocusSpy = vi.fn(); - render(Thumbnail, { + const { baseElement } = render(Thumbnail, { asset, handleFocus: handleFocusSpy, }); - const container = screen.getByTestId('container-with-tabindex'); - await fireEvent(container, new FocusEvent('focus')); + const container = baseElement.querySelector('[data-thumbnail-focus-container]'); + expect(container).not.toBeNull(); + await fireEvent(container as HTMLElement, new FocusEvent('focus')); expect(handleFocusSpy).toBeCalled(); }); - it('element will be focussed if not already', () => { + it('element will be focussed if not already', async () => { const asset = assetFactory.build({ originalPath: 'image.jpg', originalMimeType: 'image/jpeg' }); const handleFocusSpy = vi.fn(); - render(Thumbnail, { + const { baseElement } = render(Thumbnail, { asset, - focussed: true, handleFocus: handleFocusSpy, }); + const container = baseElement.querySelector('[data-thumbnail-focus-container]'); + expect(container).not.toBeNull(); + await fireEvent(container as HTMLElement, new FocusEvent('focus')); + expect(handleFocusSpy).toBeCalled(); }); }); diff --git a/web/src/lib/components/assets/thumbnail/thumbnail.svelte b/web/src/lib/components/assets/thumbnail/thumbnail.svelte index 076b0b17cd..3a00af34ad 100644 --- a/web/src/lib/components/assets/thumbnail/thumbnail.svelte +++ b/web/src/lib/components/assets/thumbnail/thumbnail.svelte @@ -19,7 +19,7 @@ import { thumbhash } from '$lib/actions/thumbhash'; import { authManager } from '$lib/managers/auth-manager.svelte'; import { mobileDevice } from '$lib/stores/mobile-device.svelte'; - import { getFocusable } from '$lib/utils/focus-util'; + import { focusNext } from '$lib/utils/focus-util'; import { currentUrlReplaceAssetId } from '$lib/utils/navigation'; import { TUNABLES } from '$lib/utils/tunables'; import { onMount } from 'svelte'; @@ -35,7 +35,6 @@ thumbnailWidth?: number | undefined; thumbnailHeight?: number | undefined; selected?: boolean; - focussed?: boolean; selectionCandidate?: boolean; disabled?: boolean; disableLinkMouseOver?: boolean; @@ -58,7 +57,6 @@ thumbnailWidth = undefined, thumbnailHeight = undefined, selected = false, - focussed = false, selectionCandidate = false, disabled = false, disableLinkMouseOver = false, @@ -79,17 +77,11 @@ } = TUNABLES; let usingMobileDevice = $derived(mobileDevice.pointerCoarse); - let focussableElement: HTMLElement | undefined = $state(); + let element: HTMLElement | undefined = $state(); let mouseOver = $state(false); let loaded = $state(false); let thumbError = $state(false); - $effect(() => { - if (focussed && document.activeElement !== focussableElement) { - focussableElement?.focus(); - } - }); - let width = $derived(thumbnailSize || thumbnailWidth || 235); let height = $derived(thumbnailSize || thumbnailHeight || 235); @@ -236,31 +228,14 @@ if (evt.key === 'x') { onSelect?.(asset); } - if (document.activeElement === focussableElement && evt.key === 'Escape') { - const focusable = getFocusable(document); - const index = focusable.indexOf(focussableElement); - - let i = index + 1; - while (i !== index) { - const next = focusable[i]; - if (next.dataset.thumbnailFocusContainer !== undefined) { - if (i === focusable.length - 1) { - i = 0; - } else { - i++; - } - continue; - } - next.focus(); - break; - } + if (document.activeElement === element && evt.key === 'Escape') { + focusNext((element) => element.dataset.thumbnailFocusContainer === undefined, true); } }} onclick={handleClick} - bind:this={focussableElement} + bind:this={element} onfocus={handleFocus} data-thumbnail-focus-container - data-testid="container-with-tabindex" tabindex={0} role="link" > diff --git a/web/src/lib/components/elements/buttons/skip-link.svelte b/web/src/lib/components/elements/buttons/skip-link.svelte index 40b1b9c526..a21fc60caf 100644 --- a/web/src/lib/components/elements/buttons/skip-link.svelte +++ b/web/src/lib/components/elements/buttons/skip-link.svelte @@ -1,6 +1,7 @@ - - - {#snippet buttons()} - - - - - - - {/snippet} -
-
- {#if matches.length + extras.length + orphans.length === 0} -
- -
- {:else} -
- - - - - - - - {#each matches as match (match.extra.filename)} - handleSplit(match)} - > - - - - {/each} - -
-
-

- {$t('matches').toUpperCase()} - {matches.length > 0 ? `(${matches.length.toLocaleString($locale)})` : ''} -

-

{$t('admin.these_files_matched_by_checksum')}

-
-
- {match.orphan.pathValue} => - {match.extra.filename} - - ({match.orphan.entityType}/{match.orphan.pathType}) -
- - - - - - - - - {#each orphans as orphan, index (index)} - - - - - - {/each} - -
-
-

- {$t('admin.offline_paths').toUpperCase()} - {orphans.length > 0 ? `(${orphans.length.toLocaleString($locale)})` : ''} -

-

- {$t('admin.offline_paths_description')} -

-
-
copyToClipboard(orphan.pathValue)}> - {}} /> - - {orphan.pathValue} - - ({orphan.entityType}) -
- - - - - - - - - {#each extras as extra (extra.filename)} - handleCheckOne(extra.filename)} - title={extra.filename} - > - - - - {/each} - -
-
-

- {$t('admin.untracked_files').toUpperCase()} - {extras.length > 0 ? `(${extras.length.toLocaleString($locale)})` : ''} -

-

- {$t('admin.untracked_files_description')} -

-
-
copyToClipboard(extra.filename)}> - {}} /> - - {extra.filename} - - {#if extra.checksum} - [sha1:{extra.checksum}] - {/if} - -
-
- {/if} -
-
- diff --git a/web/src/routes/admin/repair/+page.ts b/web/src/routes/admin/repair/+page.ts deleted file mode 100644 index 9e52abb573..0000000000 --- a/web/src/routes/admin/repair/+page.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { authenticate } from '$lib/utils/auth'; -import { getFormatter } from '$lib/utils/i18n'; -import { getAuditFiles } from '@immich/sdk'; -import type { PageLoad } from './$types'; - -export const load = (async () => { - await authenticate({ admin: true }); - const { orphans, extras } = await getAuditFiles(); - const $t = await getFormatter(); - - return { - orphans, - extras, - meta: { - title: $t('repair'), - }, - }; -}) satisfies PageLoad; From be5cc2cdf56b75c11da386667867f9febea7bce8 Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Wed, 30 Apr 2025 11:25:30 -0400 Subject: [PATCH 08/16] refactor: stream detect faces (#17996) --- server/src/queries/asset.job.repository.sql | 14 +++++++++ .../src/repositories/asset-job.repository.ts | 9 ++++++ server/src/repositories/asset.repository.ts | 16 +--------- server/src/services/person.service.spec.ts | 29 +++++-------------- server/src/services/person.service.ts | 26 +++++++---------- 5 files changed, 42 insertions(+), 52 deletions(-) diff --git a/server/src/queries/asset.job.repository.sql b/server/src/queries/asset.job.repository.sql index 48a233ffe4..ecf8f0b475 100644 --- a/server/src/queries/asset.job.repository.sql +++ b/server/src/queries/asset.job.repository.sql @@ -469,3 +469,17 @@ from "assets" where "assets"."deletedAt" <= $1 + +-- AssetJobRepository.streamForDetectFacesJob +select + "assets"."id" +from + "assets" + inner join "asset_job_status" as "job_status" on "assetId" = "assets"."id" +where + "assets"."isVisible" = $1 + and "assets"."deletedAt" is null + and "job_status"."previewAt" is not null + and "job_status"."facesRecognizedAt" is null +order by + "assets"."createdAt" desc diff --git a/server/src/repositories/asset-job.repository.ts b/server/src/repositories/asset-job.repository.ts index 693cd193c1..78ddeae3e2 100644 --- a/server/src/repositories/asset-job.repository.ts +++ b/server/src/repositories/asset-job.repository.ts @@ -322,4 +322,13 @@ export class AssetJobRepository { .where('assets.deletedAt', '<=', trashedBefore) .stream(); } + + @GenerateSql({ params: [], stream: true }) + streamForDetectFacesJob(force?: boolean) { + return this.assetsWithPreviews() + .$if(!force, (qb) => qb.where('job_status.facesRecognizedAt', 'is', null)) + .select(['assets.id']) + .orderBy('assets.createdAt', 'desc') + .stream(); + } } diff --git a/server/src/repositories/asset.repository.ts b/server/src/repositories/asset.repository.ts index 7b6fb862d0..ade6201cf1 100644 --- a/server/src/repositories/asset.repository.ts +++ b/server/src/repositories/asset.repository.ts @@ -48,8 +48,6 @@ export interface LivePhotoSearchOptions { export enum WithoutProperty { THUMBNAIL = 'thumbnail', ENCODED_VIDEO = 'encoded-video', - EXIF = 'exif', - FACES = 'faces', SIDECAR = 'sidecar', } @@ -543,19 +541,7 @@ export class AssetRepository { .where('assets.type', '=', AssetType.VIDEO) .where((eb) => eb.or([eb('assets.encodedVideoPath', 'is', null), eb('assets.encodedVideoPath', '=', '')])), ) - .$if(property === WithoutProperty.EXIF, (qb) => - qb - .leftJoin('asset_job_status as job_status', 'assets.id', 'job_status.assetId') - .where((eb) => eb.or([eb('job_status.metadataExtractedAt', 'is', null), eb('assetId', 'is', null)])) - .where('assets.isVisible', '=', true), - ) - .$if(property === WithoutProperty.FACES, (qb) => - qb - .innerJoin('asset_job_status as job_status', 'assetId', 'assets.id') - .where('job_status.previewAt', 'is not', null) - .where('job_status.facesRecognizedAt', 'is', null) - .where('assets.isVisible', '=', true), - ) + .$if(property === WithoutProperty.SIDECAR, (qb) => qb .where((eb) => eb.or([eb('assets.sidecarPath', '=', ''), eb('assets.sidecarPath', 'is', null)])) diff --git a/server/src/services/person.service.spec.ts b/server/src/services/person.service.spec.ts index 9808522434..5b88883472 100644 --- a/server/src/services/person.service.spec.ts +++ b/server/src/services/person.service.spec.ts @@ -2,7 +2,6 @@ import { BadRequestException, NotFoundException } from '@nestjs/common'; import { BulkIdErrorReason } from 'src/dtos/asset-ids.response.dto'; import { mapFaces, mapPerson, PersonResponseDto } from 'src/dtos/person.dto'; import { CacheControl, Colorspace, ImageFormat, JobName, JobStatus, SourceType, SystemMetadataKey } from 'src/enum'; -import { WithoutProperty } from 'src/repositories/asset.repository'; import { DetectedFaces } from 'src/repositories/machine-learning.repository'; import { FaceSearchResult } from 'src/repositories/search.repository'; import { PersonService } from 'src/services/person.service'; @@ -455,14 +454,11 @@ describe(PersonService.name, () => { }); it('should queue missing assets', async () => { - mocks.asset.getWithout.mockResolvedValue({ - items: [assetStub.image], - hasNextPage: false, - }); + mocks.assetJob.streamForDetectFacesJob.mockReturnValue(makeStream([assetStub.image])); await sut.handleQueueDetectFaces({ force: false }); - expect(mocks.asset.getWithout).toHaveBeenCalledWith({ skip: 0, take: 1000 }, WithoutProperty.FACES); + expect(mocks.assetJob.streamForDetectFacesJob).toHaveBeenCalledWith(false); expect(mocks.job.queueAll).toHaveBeenCalledWith([ { name: JobName.FACE_DETECTION, @@ -472,10 +468,7 @@ describe(PersonService.name, () => { }); it('should queue all assets', async () => { - mocks.asset.getAll.mockResolvedValue({ - items: [assetStub.image], - hasNextPage: false, - }); + mocks.assetJob.streamForDetectFacesJob.mockReturnValue(makeStream([assetStub.image])); mocks.person.getAllWithoutFaces.mockResolvedValue([personStub.withName]); await sut.handleQueueDetectFaces({ force: true }); @@ -483,7 +476,7 @@ describe(PersonService.name, () => { expect(mocks.person.deleteFaces).toHaveBeenCalledWith({ sourceType: SourceType.MACHINE_LEARNING }); expect(mocks.person.delete).toHaveBeenCalledWith([personStub.withName.id]); expect(mocks.storage.unlink).toHaveBeenCalledWith(personStub.withName.thumbnailPath); - expect(mocks.asset.getAll).toHaveBeenCalled(); + expect(mocks.assetJob.streamForDetectFacesJob).toHaveBeenCalledWith(true); expect(mocks.job.queueAll).toHaveBeenCalledWith([ { name: JobName.FACE_DETECTION, @@ -493,17 +486,14 @@ describe(PersonService.name, () => { }); it('should refresh all assets', async () => { - mocks.asset.getAll.mockResolvedValue({ - items: [assetStub.image], - hasNextPage: false, - }); + mocks.assetJob.streamForDetectFacesJob.mockReturnValue(makeStream([assetStub.image])); await sut.handleQueueDetectFaces({ force: undefined }); expect(mocks.person.delete).not.toHaveBeenCalled(); expect(mocks.person.deleteFaces).not.toHaveBeenCalled(); expect(mocks.storage.unlink).not.toHaveBeenCalled(); - expect(mocks.asset.getAll).toHaveBeenCalled(); + expect(mocks.assetJob.streamForDetectFacesJob).toHaveBeenCalledWith(undefined); expect(mocks.job.queueAll).toHaveBeenCalledWith([ { name: JobName.FACE_DETECTION, @@ -516,16 +506,13 @@ describe(PersonService.name, () => { it('should delete existing people and faces if forced', async () => { mocks.person.getAll.mockReturnValue(makeStream([faceStub.face1.person, personStub.randomPerson])); mocks.person.getAllFaces.mockReturnValue(makeStream([faceStub.face1])); - mocks.asset.getAll.mockResolvedValue({ - items: [assetStub.image], - hasNextPage: false, - }); + mocks.assetJob.streamForDetectFacesJob.mockReturnValue(makeStream([assetStub.image])); mocks.person.getAllWithoutFaces.mockResolvedValue([personStub.randomPerson]); mocks.person.deleteFaces.mockResolvedValue(); await sut.handleQueueDetectFaces({ force: true }); - expect(mocks.asset.getAll).toHaveBeenCalled(); + expect(mocks.assetJob.streamForDetectFacesJob).toHaveBeenCalledWith(true); expect(mocks.job.queueAll).toHaveBeenCalledWith([ { name: JobName.FACE_DETECTION, diff --git a/server/src/services/person.service.ts b/server/src/services/person.service.ts index 66d68857a0..227ea3c1c2 100644 --- a/server/src/services/person.service.ts +++ b/server/src/services/person.service.ts @@ -36,7 +36,6 @@ import { SourceType, SystemMetadataKey, } from 'src/enum'; -import { WithoutProperty } from 'src/repositories/asset.repository'; import { BoundingBox } from 'src/repositories/machine-learning.repository'; import { UpdateFacesData } from 'src/repositories/person.repository'; import { BaseService } from 'src/services/base.service'; @@ -44,7 +43,6 @@ import { CropOptions, ImageDimensions, InputDimensions, JobItem, JobOf } from 's import { ImmichFileResponse } from 'src/utils/file'; import { mimeTypes } from 'src/utils/mime-types'; import { isFaceImportEnabled, isFacialRecognitionEnabled } from 'src/utils/misc'; -import { usePagination } from 'src/utils/pagination'; @Injectable() export class PersonService extends BaseService { @@ -265,23 +263,19 @@ export class PersonService extends BaseService { await this.handlePersonCleanup(); } - const assetPagination = usePagination(JOBS_ASSET_PAGINATION_SIZE, (pagination) => { - return force === false - ? this.assetRepository.getWithout(pagination, WithoutProperty.FACES) - : this.assetRepository.getAll(pagination, { - orderDirection: 'desc', - withFaces: true, - withArchived: true, - isVisible: true, - }); - }); + let jobs: JobItem[] = []; + const assets = this.assetJobRepository.streamForDetectFacesJob(force); + for await (const asset of assets) { + jobs.push({ name: JobName.FACE_DETECTION, data: { id: asset.id } }); - for await (const assets of assetPagination) { - await this.jobRepository.queueAll( - assets.map((asset) => ({ name: JobName.FACE_DETECTION, data: { id: asset.id } })), - ); + if (jobs.length >= JOBS_ASSET_PAGINATION_SIZE) { + await this.jobRepository.queueAll(jobs); + jobs = []; + } } + await this.jobRepository.queueAll(jobs); + if (force === undefined) { await this.jobRepository.queue({ name: JobName.PERSON_CLEANUP }); } From 436cff72b56b0155d0bc42c65d2d0e371a5c139e Mon Sep 17 00:00:00 2001 From: Daniel Dietzler <36593685+danieldietzler@users.noreply.github.com> Date: Wed, 30 Apr 2025 17:50:38 +0200 Subject: [PATCH 09/16] refactor: activity manager (#17943) --- .../asset-viewer/activity-viewer.svelte | 89 ++++---------- .../asset-viewer/asset-viewer.svelte | 79 +++--------- .../lib/managers/activity-manager.svelte.ts | 113 ++++++++++++++++++ web/src/lib/stores/activity.store.ts | 11 -- .../[[assetId=id]]/+page.svelte | 89 +++++--------- 5 files changed, 175 insertions(+), 206 deletions(-) create mode 100644 web/src/lib/managers/activity-manager.svelte.ts delete mode 100644 web/src/lib/stores/activity.store.ts diff --git a/web/src/lib/components/asset-viewer/activity-viewer.svelte b/web/src/lib/components/asset-viewer/activity-viewer.svelte index 94b66d4c22..e98769d495 100644 --- a/web/src/lib/components/asset-viewer/activity-viewer.svelte +++ b/web/src/lib/components/asset-viewer/activity-viewer.svelte @@ -1,32 +1,24 @@