From 9dddccd831d9baab84c95a3668bbbadd6a71825c Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Wed, 4 Feb 2026 12:27:52 -0500 Subject: [PATCH 01/43] fix: null validation (#25891) --- .../lib/model/asset_bulk_update_dto.dart | 2 +- .../lib/model/user_admin_create_dto.dart | 14 +++- open-api/immich-openapi-specs.json | 10 ++- open-api/typescript-sdk/src/fetch-client.ts | 4 +- .../src/controllers/asset.controller.spec.ts | 28 +++++++ .../notification-admin.controller.spec.ts | 36 +++++++++ .../notification.controller.spec.ts | 32 +++++++- .../src/controllers/person.controller.spec.ts | 5 ++ .../shared-link.controller.spec.ts | 34 +++++++++ server/src/controllers/tag.controller.spec.ts | 73 +++++++++++++++++++ .../controllers/user-admin.controller.spec.ts | 55 ++++++++++++++ .../src/controllers/user.controller.spec.ts | 8 ++ server/src/dtos/asset.dto.ts | 3 +- server/src/dtos/notification.dto.ts | 21 +++--- server/src/dtos/shared-link.dto.ts | 2 +- server/src/dtos/tag.dto.ts | 4 +- server/src/dtos/user.dto.ts | 30 +++++++- server/src/validation.ts | 43 ++++++----- 18 files changed, 357 insertions(+), 47 deletions(-) create mode 100644 server/src/controllers/notification-admin.controller.spec.ts create mode 100644 server/src/controllers/shared-link.controller.spec.ts create mode 100644 server/src/controllers/tag.controller.spec.ts diff --git a/mobile/openapi/lib/model/asset_bulk_update_dto.dart b/mobile/openapi/lib/model/asset_bulk_update_dto.dart index c770265860..a373743852 100644 --- a/mobile/openapi/lib/model/asset_bulk_update_dto.dart +++ b/mobile/openapi/lib/model/asset_bulk_update_dto.dart @@ -53,7 +53,7 @@ class AssetBulkUpdateDto { /// String? description; - /// Duplicate asset ID + /// Duplicate ID String? duplicateId; /// Asset IDs to update diff --git a/mobile/openapi/lib/model/user_admin_create_dto.dart b/mobile/openapi/lib/model/user_admin_create_dto.dart index 320d318062..485b2e00e5 100644 --- a/mobile/openapi/lib/model/user_admin_create_dto.dart +++ b/mobile/openapi/lib/model/user_admin_create_dto.dart @@ -19,6 +19,7 @@ class UserAdminCreateDto { required this.name, this.notify, required this.password, + this.pinCode, this.quotaSizeInBytes, this.shouldChangePassword, this.storageLabel, @@ -54,6 +55,9 @@ class UserAdminCreateDto { /// User password String password; + /// PIN code + String? pinCode; + /// Storage quota in bytes /// /// Minimum value: 0 @@ -79,6 +83,7 @@ class UserAdminCreateDto { other.name == name && other.notify == notify && other.password == password && + other.pinCode == pinCode && other.quotaSizeInBytes == quotaSizeInBytes && other.shouldChangePassword == shouldChangePassword && other.storageLabel == storageLabel; @@ -92,12 +97,13 @@ class UserAdminCreateDto { (name.hashCode) + (notify == null ? 0 : notify!.hashCode) + (password.hashCode) + + (pinCode == null ? 0 : pinCode!.hashCode) + (quotaSizeInBytes == null ? 0 : quotaSizeInBytes!.hashCode) + (shouldChangePassword == null ? 0 : shouldChangePassword!.hashCode) + (storageLabel == null ? 0 : storageLabel!.hashCode); @override - String toString() => 'UserAdminCreateDto[avatarColor=$avatarColor, email=$email, isAdmin=$isAdmin, name=$name, notify=$notify, password=$password, quotaSizeInBytes=$quotaSizeInBytes, shouldChangePassword=$shouldChangePassword, storageLabel=$storageLabel]'; + String toString() => 'UserAdminCreateDto[avatarColor=$avatarColor, email=$email, isAdmin=$isAdmin, name=$name, notify=$notify, password=$password, pinCode=$pinCode, quotaSizeInBytes=$quotaSizeInBytes, shouldChangePassword=$shouldChangePassword, storageLabel=$storageLabel]'; Map toJson() { final json = {}; @@ -119,6 +125,11 @@ class UserAdminCreateDto { // json[r'notify'] = null; } json[r'password'] = this.password; + if (this.pinCode != null) { + json[r'pinCode'] = this.pinCode; + } else { + // json[r'pinCode'] = null; + } if (this.quotaSizeInBytes != null) { json[r'quotaSizeInBytes'] = this.quotaSizeInBytes; } else { @@ -152,6 +163,7 @@ class UserAdminCreateDto { name: mapValueOfType(json, r'name')!, notify: mapValueOfType(json, r'notify'), password: mapValueOfType(json, r'password')!, + pinCode: mapValueOfType(json, r'pinCode'), quotaSizeInBytes: mapValueOfType(json, r'quotaSizeInBytes'), shouldChangePassword: mapValueOfType(json, r'shouldChangePassword'), storageLabel: mapValueOfType(json, r'storageLabel'), diff --git a/open-api/immich-openapi-specs.json b/open-api/immich-openapi-specs.json index 498de43471..8359ebc173 100644 --- a/open-api/immich-openapi-specs.json +++ b/open-api/immich-openapi-specs.json @@ -15760,7 +15760,7 @@ "type": "string" }, "duplicateId": { - "description": "Duplicate asset ID", + "description": "Duplicate ID", "nullable": true, "type": "string" }, @@ -19038,6 +19038,7 @@ "format": "uuid", "type": "string" }, + "minItems": 1, "type": "array" } }, @@ -19128,6 +19129,7 @@ "format": "uuid", "type": "string" }, + "minItems": 1, "type": "array" }, "readAt": { @@ -25069,6 +25071,12 @@ "description": "User password", "type": "string" }, + "pinCode": { + "description": "PIN code", + "example": "123456", + "nullable": true, + "type": "string" + }, "quotaSizeInBytes": { "description": "Storage quota in bytes", "format": "int64", diff --git a/open-api/typescript-sdk/src/fetch-client.ts b/open-api/typescript-sdk/src/fetch-client.ts index a08065c0e5..75c55f7853 100644 --- a/open-api/typescript-sdk/src/fetch-client.ts +++ b/open-api/typescript-sdk/src/fetch-client.ts @@ -233,6 +233,8 @@ export type UserAdminCreateDto = { notify?: boolean; /** User password */ password: string; + /** PIN code */ + pinCode?: string | null; /** Storage quota in bytes */ quotaSizeInBytes?: number | null; /** Require password change on next login */ @@ -822,7 +824,7 @@ export type AssetBulkUpdateDto = { dateTimeRelative?: number; /** Asset description */ description?: string; - /** Duplicate asset ID */ + /** Duplicate ID */ duplicateId?: string | null; /** Asset IDs to update */ ids: string[]; diff --git a/server/src/controllers/asset.controller.spec.ts b/server/src/controllers/asset.controller.spec.ts index cf8b80be38..197e06d02d 100644 --- a/server/src/controllers/asset.controller.spec.ts +++ b/server/src/controllers/asset.controller.spec.ts @@ -24,6 +24,34 @@ describe(AssetController.name, () => { await request(ctx.getHttpServer()).put(`/assets`); expect(ctx.authenticate).toHaveBeenCalled(); }); + + it('should require a valid uuid', async () => { + const { status, body } = await request(ctx.getHttpServer()) + .put(`/assets`) + .send({ ids: ['123'] }); + + expect(status).toBe(400); + expect(body).toEqual(factory.responses.badRequest(['each value in ids must be a UUID'])); + }); + + it('should require duplicateId to be a string', async () => { + const id = factory.uuid(); + const { status, body } = await request(ctx.getHttpServer()) + .put(`/assets`) + .send({ ids: [id], duplicateId: true }); + + expect(status).toBe(400); + expect(body).toEqual(factory.responses.badRequest(['duplicateId must be a string'])); + }); + + it('should accept a null duplicateId', async () => { + const id = factory.uuid(); + await request(ctx.getHttpServer()) + .put(`/assets`) + .send({ ids: [id], duplicateId: null }); + + expect(service.updateAll).toHaveBeenCalledWith(undefined, expect.objectContaining({ duplicateId: null })); + }); }); describe('DELETE /assets', () => { diff --git a/server/src/controllers/notification-admin.controller.spec.ts b/server/src/controllers/notification-admin.controller.spec.ts new file mode 100644 index 0000000000..b93726eb32 --- /dev/null +++ b/server/src/controllers/notification-admin.controller.spec.ts @@ -0,0 +1,36 @@ +import { NotificationAdminController } from 'src/controllers/notification-admin.controller'; +import { NotificationAdminService } from 'src/services/notification-admin.service'; +import request from 'supertest'; +import { factory } from 'test/small.factory'; +import { ControllerContext, controllerSetup, mockBaseService } from 'test/utils'; + +describe(NotificationAdminController.name, () => { + let ctx: ControllerContext; + const service = mockBaseService(NotificationAdminService); + + beforeAll(async () => { + ctx = await controllerSetup(NotificationAdminController, [ + { provide: NotificationAdminService, useValue: service }, + ]); + return () => ctx.close(); + }); + + beforeEach(() => { + service.resetAllMocks(); + ctx.reset(); + }); + + describe('POST /admin/notifications', () => { + it('should be an authenticated route', async () => { + await request(ctx.getHttpServer()).post('/admin/notifications'); + expect(ctx.authenticate).toHaveBeenCalled(); + }); + + it('should accept a null readAt', async () => { + await request(ctx.getHttpServer()) + .post(`/admin/notifications`) + .send({ title: 'Test', userId: factory.uuid(), readAt: null }); + expect(service.create).toHaveBeenCalledWith(undefined, expect.objectContaining({ readAt: null })); + }); + }); +}); diff --git a/server/src/controllers/notification.controller.spec.ts b/server/src/controllers/notification.controller.spec.ts index 0dce7d73b5..a64aee2912 100644 --- a/server/src/controllers/notification.controller.spec.ts +++ b/server/src/controllers/notification.controller.spec.ts @@ -37,9 +37,33 @@ describe(NotificationController.name, () => { describe('PUT /notifications', () => { it('should be an authenticated route', async () => { - await request(ctx.getHttpServer()).get('/notifications'); + await request(ctx.getHttpServer()).put('/notifications'); expect(ctx.authenticate).toHaveBeenCalled(); }); + + describe('ids', () => { + it('should require a list', async () => { + const { status, body } = await request(ctx.getHttpServer()).put(`/notifications`).send({ ids: true }); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest(expect.arrayContaining(['ids must be an array']))); + }); + + it('should require uuids', async () => { + const { status, body } = await request(ctx.getHttpServer()) + .put(`/notifications`) + .send({ ids: [true] }); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest(['each value in ids must be a UUID'])); + }); + + it('should accept valid uuids', async () => { + const id = factory.uuid(); + await request(ctx.getHttpServer()) + .put(`/notifications`) + .send({ ids: [id] }); + expect(service.updateAll).toHaveBeenCalledWith(undefined, expect.objectContaining({ ids: [id] })); + }); + }); }); describe('GET /notifications/:id', () => { @@ -60,5 +84,11 @@ describe(NotificationController.name, () => { await request(ctx.getHttpServer()).put(`/notifications/${factory.uuid()}`).send({ readAt: factory.date() }); expect(ctx.authenticate).toHaveBeenCalled(); }); + + it('should accept a null readAt', async () => { + const id = factory.uuid(); + await request(ctx.getHttpServer()).put(`/notifications/${id}`).send({ readAt: null }); + expect(service.update).toHaveBeenCalledWith(undefined, id, expect.objectContaining({ readAt: null })); + }); }); }); diff --git a/server/src/controllers/person.controller.spec.ts b/server/src/controllers/person.controller.spec.ts index 5b63fcc6cd..a28ac9b659 100644 --- a/server/src/controllers/person.controller.spec.ts +++ b/server/src/controllers/person.controller.spec.ts @@ -58,6 +58,11 @@ describe(PersonController.name, () => { await request(ctx.getHttpServer()).post('/people').send({ birthDate: '' }); expect(service.create).toHaveBeenCalledWith(undefined, { birthDate: null }); }); + + it('should map an empty color to null', async () => { + await request(ctx.getHttpServer()).post('/people').send({ color: '' }); + expect(service.create).toHaveBeenCalledWith(undefined, { color: null }); + }); }); describe('DELETE /people', () => { diff --git a/server/src/controllers/shared-link.controller.spec.ts b/server/src/controllers/shared-link.controller.spec.ts new file mode 100644 index 0000000000..96c84040ca --- /dev/null +++ b/server/src/controllers/shared-link.controller.spec.ts @@ -0,0 +1,34 @@ +import { SharedLinkController } from 'src/controllers/shared-link.controller'; +import { SharedLinkType } from 'src/enum'; +import { SharedLinkService } from 'src/services/shared-link.service'; +import request from 'supertest'; +import { ControllerContext, controllerSetup, mockBaseService } from 'test/utils'; + +describe(SharedLinkController.name, () => { + let ctx: ControllerContext; + const service = mockBaseService(SharedLinkService); + + beforeAll(async () => { + ctx = await controllerSetup(SharedLinkController, [{ provide: SharedLinkService, useValue: service }]); + return () => ctx.close(); + }); + + beforeEach(() => { + service.resetAllMocks(); + ctx.reset(); + }); + + describe('POST /shared-links', () => { + it('should be an authenticated route', async () => { + await request(ctx.getHttpServer()).post('/shared-links'); + expect(ctx.authenticate).toHaveBeenCalled(); + }); + + it('should allow an null expiresAt', async () => { + await request(ctx.getHttpServer()) + .post('/shared-links') + .send({ expiresAt: null, type: SharedLinkType.Individual }); + expect(service.create).toHaveBeenCalledWith(undefined, expect.objectContaining({ expiresAt: null })); + }); + }); +}); diff --git a/server/src/controllers/tag.controller.spec.ts b/server/src/controllers/tag.controller.spec.ts new file mode 100644 index 0000000000..60fc3d65ae --- /dev/null +++ b/server/src/controllers/tag.controller.spec.ts @@ -0,0 +1,73 @@ +import { TagController } from 'src/controllers/tag.controller'; +import { TagService } from 'src/services/tag.service'; +import request from 'supertest'; +import { errorDto } from 'test/medium/responses'; +import { factory } from 'test/small.factory'; +import { ControllerContext, controllerSetup, mockBaseService } from 'test/utils'; + +describe(TagController.name, () => { + let ctx: ControllerContext; + const service = mockBaseService(TagService); + + beforeAll(async () => { + ctx = await controllerSetup(TagController, [{ provide: TagService, useValue: service }]); + return () => ctx.close(); + }); + + beforeEach(() => { + service.resetAllMocks(); + ctx.reset(); + }); + + describe('GET /tags', () => { + it('should be an authenticated route', async () => { + await request(ctx.getHttpServer()).get('/tags'); + expect(ctx.authenticate).toHaveBeenCalled(); + }); + }); + + describe('POST /tags', () => { + it('should be an authenticated route', async () => { + await request(ctx.getHttpServer()).post('/tags'); + expect(ctx.authenticate).toHaveBeenCalled(); + }); + + it('should a null parentId', async () => { + await request(ctx.getHttpServer()).post(`/tags`).send({ name: 'tag', parentId: null }); + expect(service.create).toHaveBeenCalledWith(undefined, expect.objectContaining({ parentId: null })); + }); + }); + + describe('PUT /tags', () => { + it('should be an authenticated route', async () => { + await request(ctx.getHttpServer()).put('/tags'); + expect(ctx.authenticate).toHaveBeenCalled(); + }); + }); + + describe('GET /tags/:id', () => { + it('should be an authenticated route', async () => { + await request(ctx.getHttpServer()).get(`/tags/${factory.uuid()}`); + expect(ctx.authenticate).toHaveBeenCalled(); + }); + + it('should require a valid uuid', async () => { + const { status, body } = await request(ctx.getHttpServer()).get(`/tags/123`); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest([expect.stringContaining('id must be a UUID')])); + }); + }); + + describe('PUT /tags/:id', () => { + it('should be an authenticated route', async () => { + await request(ctx.getHttpServer()).put(`/tags/${factory.uuid()}`); + expect(ctx.authenticate).toHaveBeenCalled(); + }); + + it('should allow setting a null color via an empty string', async () => { + const id = factory.uuid(); + await request(ctx.getHttpServer()).put(`/tags/${id}`).send({ color: '' }); + expect(service.update).toHaveBeenCalledWith(undefined, id, expect.objectContaining({ color: null })); + }); + }); +}); diff --git a/server/src/controllers/user-admin.controller.spec.ts b/server/src/controllers/user-admin.controller.spec.ts index bd9c966d42..edda974476 100644 --- a/server/src/controllers/user-admin.controller.spec.ts +++ b/server/src/controllers/user-admin.controller.spec.ts @@ -31,12 +31,55 @@ describe(UserAdminController.name, () => { }); }); + describe('PUT /admin/users/:id', () => { + it('should be an authenticated route', async () => { + await request(ctx.getHttpServer()).put(`/admin/users/${factory.uuid()}`); + expect(ctx.authenticate).toHaveBeenCalled(); + }); + }); + describe('POST /admin/users', () => { it('should be an authenticated route', async () => { await request(ctx.getHttpServer()).post('/admin/users'); expect(ctx.authenticate).toHaveBeenCalled(); }); + it('should allow a null pinCode', async () => { + await request(ctx.getHttpServer()).post(`/admin/users`).send({ + name: 'Test user', + email: 'test@immich.cloud', + password: 'password', + pinCode: null, + }); + expect(service.create).toHaveBeenCalledWith(expect.objectContaining({ pinCode: null })); + }); + + it('should allow a null avatarColor', async () => { + await request(ctx.getHttpServer()).post(`/admin/users`).send({ + name: 'Test user', + email: 'test@immich.cloud', + password: 'password', + avatarColor: null, + }); + expect(service.create).toHaveBeenCalledWith(expect.objectContaining({ avatarColor: null })); + }); + + it(`should `, async () => { + const dto: UserAdminCreateDto = { + email: 'user@immich.app', + password: 'test', + name: 'Test User', + quotaSizeInBytes: 1.2, + }; + + const { status, body } = await request(ctx.getHttpServer()) + .post(`/admin/users`) + .set('Authorization', `Bearer token`) + .send(dto); + expect(status).toBe(400); + expect(body).toEqual(errorDto.badRequest(expect.arrayContaining(['quotaSizeInBytes must be an integer number']))); + }); + it(`should not allow decimal quota`, async () => { const dto: UserAdminCreateDto = { email: 'user@immich.app', @@ -75,5 +118,17 @@ describe(UserAdminController.name, () => { expect(status).toBe(400); expect(body).toEqual(errorDto.badRequest(expect.arrayContaining(['quotaSizeInBytes must be an integer number']))); }); + + it('should allow a null pinCode', async () => { + const id = factory.uuid(); + await request(ctx.getHttpServer()).put(`/admin/users/${id}`).send({ pinCode: null }); + expect(service.update).toHaveBeenCalledWith(undefined, id, expect.objectContaining({ pinCode: null })); + }); + + it('should allow a null avatarColor', async () => { + const id = factory.uuid(); + await request(ctx.getHttpServer()).put(`/admin/users/${id}`).send({ avatarColor: null }); + expect(service.update).toHaveBeenCalledWith(undefined, id, expect.objectContaining({ avatarColor: null })); + }); }); }); diff --git a/server/src/controllers/user.controller.spec.ts b/server/src/controllers/user.controller.spec.ts index 19f9e919de..3c3e103814 100644 --- a/server/src/controllers/user.controller.spec.ts +++ b/server/src/controllers/user.controller.spec.ts @@ -54,6 +54,14 @@ describe(UserController.name, () => { expect(body).toEqual(errorDto.badRequest()); }); } + + it('should allow an empty avatarColor', async () => { + await request(ctx.getHttpServer()) + .put(`/users/me`) + .set('Authorization', `Bearer token`) + .send({ avatarColor: null }); + expect(service.updateMe).toHaveBeenCalledWith(undefined, expect.objectContaining({ avatarColor: null })); + }); }); describe('GET /users/:id', () => { diff --git a/server/src/dtos/asset.dto.ts b/server/src/dtos/asset.dto.ts index 47226e1503..00ea46f789 100644 --- a/server/src/dtos/asset.dto.ts +++ b/server/src/dtos/asset.dto.ts @@ -73,8 +73,7 @@ export class AssetBulkUpdateDto extends UpdateAssetBase { @ValidateUUID({ each: true, description: 'Asset IDs to update' }) ids!: string[]; - @ApiProperty({ description: 'Duplicate asset ID' }) - @Optional() + @ValidateString({ optional: true, nullable: true, description: 'Duplicate ID' }) duplicateId?: string | null; @ApiProperty({ description: 'Relative time offset in seconds' }) diff --git a/server/src/dtos/notification.dto.ts b/server/src/dtos/notification.dto.ts index 5331db4e85..87a15f29e3 100644 --- a/server/src/dtos/notification.dto.ts +++ b/server/src/dtos/notification.dto.ts @@ -1,7 +1,7 @@ import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger'; -import { IsString } from 'class-validator'; +import { ArrayMinSize, IsString } from 'class-validator'; import { NotificationLevel, NotificationType } from 'src/enum'; -import { Optional, ValidateBoolean, ValidateDate, ValidateEnum, ValidateUUID } from 'src/validation'; +import { Optional, ValidateBoolean, ValidateDate, ValidateEnum, ValidateString, ValidateUUID } from 'src/validation'; export class TestEmailResponseDto { @ApiProperty({ description: 'Email message ID' }) @@ -75,20 +75,17 @@ export class NotificationCreateDto { @ValidateEnum({ enum: NotificationType, name: 'NotificationType', optional: true, description: 'Notification type' }) type?: NotificationType; - @ApiProperty({ description: 'Notification title' }) - @IsString() + @ValidateString({ description: 'Notification title' }) title!: string; - @ApiPropertyOptional({ description: 'Notification description' }) - @IsString() - @Optional({ nullable: true }) + @ValidateString({ optional: true, nullable: true, description: 'Notification description' }) description?: string | null; @ApiPropertyOptional({ description: 'Additional notification data' }) @Optional({ nullable: true }) data?: any; - @ValidateDate({ optional: true, description: 'Date when notification was read' }) + @ValidateDate({ optional: true, nullable: true, description: 'Date when notification was read' }) readAt?: Date | null; @ValidateUUID({ description: 'User ID to send notification to' }) @@ -96,20 +93,22 @@ export class NotificationCreateDto { } export class NotificationUpdateDto { - @ValidateDate({ optional: true, description: 'Date when notification was read' }) + @ValidateDate({ optional: true, nullable: true, description: 'Date when notification was read' }) readAt?: Date | null; } export class NotificationUpdateAllDto { - @ValidateUUID({ each: true, optional: true, description: 'Notification IDs to update' }) + @ValidateUUID({ each: true, description: 'Notification IDs to update' }) + @ArrayMinSize(1) ids!: string[]; - @ValidateDate({ optional: true, description: 'Date when notifications were read' }) + @ValidateDate({ optional: true, nullable: true, description: 'Date when notifications were read' }) readAt?: Date | null; } export class NotificationDeleteAllDto { @ValidateUUID({ each: true, description: 'Notification IDs to delete' }) + @ArrayMinSize(1) ids!: string[]; } diff --git a/server/src/dtos/shared-link.dto.ts b/server/src/dtos/shared-link.dto.ts index 7b92f48e28..1465f68953 100644 --- a/server/src/dtos/shared-link.dto.ts +++ b/server/src/dtos/shared-link.dto.ts @@ -44,7 +44,7 @@ export class SharedLinkCreateDto { @IsString() slug?: string | null; - @ValidateDate({ optional: true, description: 'Expiration date' }) + @ValidateDate({ optional: true, nullable: true, description: 'Expiration date' }) expiresAt?: Date | null = null; @ValidateBoolean({ optional: true, description: 'Allow uploads' }) diff --git a/server/src/dtos/tag.dto.ts b/server/src/dtos/tag.dto.ts index 231e6cc501..bb33659bfe 100644 --- a/server/src/dtos/tag.dto.ts +++ b/server/src/dtos/tag.dto.ts @@ -9,7 +9,7 @@ export class TagCreateDto { @IsNotEmpty() name!: string; - @ValidateUUID({ optional: true, description: 'Parent tag ID' }) + @ValidateUUID({ nullable: true, optional: true, description: 'Parent tag ID' }) parentId?: string | null; @ApiPropertyOptional({ description: 'Tag color (hex)' }) @@ -20,7 +20,7 @@ export class TagCreateDto { export class TagUpdateDto { @ApiPropertyOptional({ description: 'Tag color (hex)' }) - @Optional({ emptyToNull: true }) + @Optional({ nullable: true, emptyToNull: true }) @ValidateHexColor() color?: string | null; } diff --git a/server/src/dtos/user.dto.ts b/server/src/dtos/user.dto.ts index 598798dc44..2d4fc3934f 100644 --- a/server/src/dtos/user.dto.ts +++ b/server/src/dtos/user.dto.ts @@ -26,7 +26,13 @@ export class UserUpdateMeDto { @IsNotEmpty() name?: string; - @ValidateEnum({ enum: UserAvatarColor, name: 'UserAvatarColor', optional: true, description: 'Avatar color' }) + @ValidateEnum({ + enum: UserAvatarColor, + name: 'UserAvatarColor', + optional: true, + nullable: true, + description: 'Avatar color', + }) avatarColor?: UserAvatarColor | null; } @@ -96,9 +102,19 @@ export class UserAdminCreateDto { @IsString() name!: string; - @ValidateEnum({ enum: UserAvatarColor, name: 'UserAvatarColor', optional: true, description: 'Avatar color' }) + @ValidateEnum({ + enum: UserAvatarColor, + name: 'UserAvatarColor', + optional: true, + nullable: true, + description: 'Avatar color', + }) avatarColor?: UserAvatarColor | null; + @ApiPropertyOptional({ description: 'PIN code' }) + @PinCode({ optional: true, nullable: true, emptyToNull: true }) + pinCode?: string | null; + @ApiPropertyOptional({ description: 'Storage label' }) @Optional({ nullable: true }) @IsString() @@ -135,7 +151,7 @@ export class UserAdminUpdateDto { password?: string; @ApiPropertyOptional({ description: 'PIN code' }) - @PinCode({ optional: true, emptyToNull: true }) + @PinCode({ optional: true, nullable: true, emptyToNull: true }) pinCode?: string | null; @ApiPropertyOptional({ description: 'User name' }) @@ -144,7 +160,13 @@ export class UserAdminUpdateDto { @IsNotEmpty() name?: string; - @ValidateEnum({ enum: UserAvatarColor, name: 'UserAvatarColor', optional: true, description: 'Avatar color' }) + @ValidateEnum({ + enum: UserAvatarColor, + name: 'UserAvatarColor', + optional: true, + nullable: true, + description: 'Avatar color', + }) avatarColor?: UserAvatarColor | null; @ApiPropertyOptional({ description: 'Storage label' }) diff --git a/server/src/validation.ts b/server/src/validation.ts index 724c01ffe9..cdca1bc0ca 100644 --- a/server/src/validation.ts +++ b/server/src/validation.ts @@ -232,19 +232,20 @@ export const ValidateHexColor = () => { return applyDecorators(...decorators); }; -type DateOptions = { optional?: boolean; nullable?: boolean; format?: 'date' | 'date-time' }; +type DateOptions = OptionalOptions & { optional?: boolean; format?: 'date' | 'date-time' }; export const ValidateDate = (options?: DateOptions & ApiPropertyOptions) => { - const { optional, nullable, format, ...apiPropertyOptions } = { - optional: false, - nullable: false, - format: 'date-time', - ...options, - }; + const { + optional, + nullable = false, + emptyToNull = false, + format = 'date-time', + ...apiPropertyOptions + } = options || {}; - const decorators = [ + return applyDecorators( ApiProperty({ format, ...apiPropertyOptions }), IsDate(), - optional ? Optional({ nullable: true }) : IsNotEmpty(), + optional ? Optional({ nullable, emptyToNull }) : IsNotEmpty(), Transform(({ key, value }) => { if (value === null || value === undefined) { return value; @@ -256,19 +257,17 @@ export const ValidateDate = (options?: DateOptions & ApiPropertyOptions) => { return new Date(value as string); }), - ]; - - if (optional) { - decorators.push(Optional({ nullable })); - } - - return applyDecorators(...decorators); + ); }; -type StringOptions = { optional?: boolean; nullable?: boolean; trim?: boolean }; +type StringOptions = OptionalOptions & { optional?: boolean; trim?: boolean }; export const ValidateString = (options?: StringOptions & ApiPropertyOptions) => { - const { optional, nullable, trim, ...apiPropertyOptions } = options || {}; - const decorators = [ApiProperty(apiPropertyOptions), IsString(), optional ? Optional({ nullable }) : IsNotEmpty()]; + const { optional, nullable, emptyToNull, trim, ...apiPropertyOptions } = options || {}; + const decorators = [ + ApiProperty(apiPropertyOptions), + IsString(), + optional ? Optional({ nullable, emptyToNull }) : IsNotEmpty(), + ]; if (trim) { decorators.push(Transform(({ value }: { value: string }) => value?.trim())); @@ -277,9 +276,9 @@ export const ValidateString = (options?: StringOptions & ApiPropertyOptions) => return applyDecorators(...decorators); }; -type BooleanOptions = { optional?: boolean; nullable?: boolean }; +type BooleanOptions = OptionalOptions & { optional?: boolean }; export const ValidateBoolean = (options?: BooleanOptions & PropertyOptions) => { - const { optional, nullable, ...apiPropertyOptions } = options || {}; + const { optional, nullable, emptyToNull, ...apiPropertyOptions } = options || {}; const decorators = [ Property(apiPropertyOptions), IsBoolean(), @@ -291,7 +290,7 @@ export const ValidateBoolean = (options?: BooleanOptions & PropertyOptions) => { } return value; }), - optional ? Optional({ nullable }) : IsNotEmpty(), + optional ? Optional({ nullable, emptyToNull }) : IsNotEmpty(), ]; return applyDecorators(...decorators); From 6cdebdd3b3fc6c01b9971a37c59167f4b1cd400c Mon Sep 17 00:00:00 2001 From: Jason Rasmussen Date: Wed, 4 Feb 2026 12:33:37 -0500 Subject: [PATCH 02/43] fix(server): deleting stacked assets (#25874) * fix(server): deleting stacked assets * fix: log a warning when removing an empty directory fails --- server/src/queries/asset.job.repository.sql | 56 +++++++---------- .../src/repositories/asset-job.repository.ts | 37 ++++++----- server/src/repositories/storage.repository.ts | 10 ++- server/src/services/asset.service.spec.ts | 35 ++++------- server/src/services/asset.service.ts | 7 ++- .../specs/services/asset.service.spec.ts | 62 ++++++++++++++++++- 6 files changed, 127 insertions(+), 80 deletions(-) diff --git a/server/src/queries/asset.job.repository.sql b/server/src/queries/asset.job.repository.sql index 50f2c193fc..26f04bd9cb 100644 --- a/server/src/queries/asset.job.repository.sql +++ b/server/src/queries/asset.job.repository.sql @@ -430,30 +430,6 @@ select "asset"."originalPath", "asset"."isOffline", to_json("asset_exif") as "exifInfo", - ( - select - coalesce(json_agg(agg), '[]') - from - ( - select - "asset_face".*, - "person" as "person" - from - "asset_face" - left join lateral ( - select - "person".* - from - "person" - where - "asset_face"."personId" = "person"."id" - ) as "person" on true - where - "asset_face"."assetId" = "asset"."id" - and "asset_face"."deletedAt" is null - and "asset_face"."isVisible" is true - ) as agg - ) as "faces", ( select coalesce(json_agg(agg), '[]') @@ -470,27 +446,37 @@ select "asset_file"."assetId" = "asset"."id" ) as agg ) as "files", - to_json("stacked_assets") as "stack" + to_json("stack_result") as "stack" from "asset" left join "asset_exif" on "asset"."id" = "asset_exif"."assetId" - left join "stack" on "stack"."id" = "asset"."stackId" left join lateral ( select "stack"."id", "stack"."primaryAssetId", - array_agg("stacked") as "assets" + ( + select + coalesce(json_agg(agg), '[]') + from + ( + select + "stack_asset"."id" + from + "asset" as "stack_asset" + where + "stack_asset"."stackId" = "stack"."id" + and "stack_asset"."id" != "stack"."primaryAssetId" + and "stack_asset"."visibility" = $1 + and "stack_asset"."status" != $2 + ) as agg + ) as "assets" from - "asset" as "stacked" + "stack" where - "stacked"."deletedAt" is not null - and "stacked"."visibility" = $1 - and "stacked"."stackId" = "stack"."id" - group by - "stack"."id" - ) as "stacked_assets" on "stack"."id" is not null + "stack"."id" = "asset"."stackId" + ) as "stack_result" on true where - "asset"."id" = $2 + "asset"."id" = $3 -- AssetJobRepository.streamForVideoConversion select diff --git a/server/src/repositories/asset-job.repository.ts b/server/src/repositories/asset-job.repository.ts index 1608f7b6f6..6fec1b38a7 100644 --- a/server/src/repositories/asset-job.repository.ts +++ b/server/src/repositories/asset-job.repository.ts @@ -1,10 +1,10 @@ import { Injectable } from '@nestjs/common'; -import { Kysely } from 'kysely'; +import { Kysely, sql } from 'kysely'; import { jsonArrayFrom } from 'kysely/helpers/postgres'; import { InjectKysely } from 'nestjs-kysely'; -import { Asset, columns } from 'src/database'; +import { columns } from 'src/database'; import { DummyValue, GenerateSql } from 'src/decorators'; -import { AssetFileType, AssetType, AssetVisibility } from 'src/enum'; +import { AssetFileType, AssetStatus, AssetType, AssetVisibility } from 'src/enum'; import { DB } from 'src/schema'; import { anyUuid, @@ -15,7 +15,6 @@ import { withExif, withExifInner, withFaces, - withFacesAndPeople, withFilePath, withFiles, } from 'src/utils/database'; @@ -269,23 +268,29 @@ export class AssetJobRepository { 'asset.isOffline', ]) .$call(withExif) - .select(withFacesAndPeople) .select(withFiles) - .leftJoin('stack', 'stack.id', 'asset.stackId') .leftJoinLateral( (eb) => eb - .selectFrom('asset as stacked') - .select(['stack.id', 'stack.primaryAssetId']) - .select((eb) => eb.fn('array_agg', [eb.table('stacked')]).as('assets')) - .where('stacked.deletedAt', 'is not', null) - .where('stacked.visibility', '=', AssetVisibility.Timeline) - .whereRef('stacked.stackId', '=', 'stack.id') - .groupBy('stack.id') - .as('stacked_assets'), - (join) => join.on('stack.id', 'is not', null), + .selectFrom('stack') + .whereRef('stack.id', '=', 'asset.stackId') + .select((eb) => [ + 'stack.id', + 'stack.primaryAssetId', + jsonArrayFrom( + eb + .selectFrom('asset as stack_asset') + .select(['stack_asset.id']) + .whereRef('stack_asset.stackId', '=', 'stack.id') + .whereRef('stack_asset.id', '!=', 'stack.primaryAssetId') + .where('stack_asset.visibility', '=', sql.val(AssetVisibility.Timeline)) + .where('stack_asset.status', '!=', sql.val(AssetStatus.Deleted)), + ).as('assets'), + ]) + .as('stack_result'), + (join) => join.onTrue(), ) - .select((eb) => toJson(eb, 'stacked_assets').as('stack')) + .select((eb) => toJson(eb, 'stack_result').as('stack')) .where('asset.id', '=', id) .executeTakeFirst(); } diff --git a/server/src/repositories/storage.repository.ts b/server/src/repositories/storage.repository.ts index 7345dfef5b..5a1a936e77 100644 --- a/server/src/repositories/storage.repository.ts +++ b/server/src/repositories/storage.repository.ts @@ -152,7 +152,7 @@ export class StorageRepository { } async unlinkDir(folder: string, options: { recursive?: boolean; force?: boolean }) { - await fs.rm(folder, options); + await fs.rm(folder, { ...options, maxRetries: 5, retryDelay: 100 }); } async removeEmptyDirs(directory: string, self: boolean = false) { @@ -168,7 +168,13 @@ export class StorageRepository { if (self) { const updated = await fs.readdir(directory); if (updated.length === 0) { - await fs.rmdir(directory); + try { + await fs.rmdir(directory); + } catch (error: Error | any) { + if (error.code !== 'ENOTEMPTY') { + this.logger.warn(`Attempted to remove directory, but failed: ${error}`); + } + } } } } diff --git a/server/src/services/asset.service.spec.ts b/server/src/services/asset.service.spec.ts index eca49bc14e..707faa326d 100755 --- a/server/src/services/asset.service.spec.ts +++ b/server/src/services/asset.service.spec.ts @@ -8,7 +8,6 @@ import { AssetStats } from 'src/repositories/asset.repository'; import { AssetService } from 'src/services/asset.service'; import { assetStub } from 'test/fixtures/asset.stub'; import { authStub } from 'test/fixtures/auth.stub'; -import { faceStub } from 'test/fixtures/face.stub'; import { userStub } from 'test/fixtures/user.stub'; import { factory } from 'test/small.factory'; import { makeStream, newTestService, ServiceMocks } from 'test/utils'; @@ -565,12 +564,11 @@ describe(AssetService.name, () => { }); describe('handleAssetDeletion', () => { - it('should remove faces', async () => { - const assetWithFace = { ...assetStub.image, faces: [faceStub.face1, faceStub.mergeFace1] }; + it('should clean up files', async () => { + const asset = assetStub.image; + mocks.assetJob.getForAssetDeletion.mockResolvedValue(asset); - mocks.assetJob.getForAssetDeletion.mockResolvedValue(assetWithFace); - - await sut.handleAssetDeletion({ id: assetWithFace.id, deleteOnDisk: true }); + await sut.handleAssetDeletion({ id: asset.id, deleteOnDisk: true }); expect(mocks.job.queue.mock.calls).toEqual([ [ @@ -581,38 +579,29 @@ describe(AssetService.name, () => { '/uploads/user-id/webp/path.ext', '/uploads/user-id/thumbs/path.jpg', '/uploads/user-id/fullsize/path.webp', - assetWithFace.originalPath, + asset.originalPath, ], }, }, ], ]); - - expect(mocks.asset.remove).toHaveBeenCalledWith(assetWithFace); - }); - - it('should update stack primary asset if deleted asset was primary asset in a stack', async () => { - mocks.stack.update.mockResolvedValue(factory.stack() as any); - mocks.assetJob.getForAssetDeletion.mockResolvedValue(assetStub.primaryImage); - - await sut.handleAssetDeletion({ id: assetStub.primaryImage.id, deleteOnDisk: true }); - - expect(mocks.stack.update).toHaveBeenCalledWith('stack-1', { - id: 'stack-1', - primaryAssetId: 'stack-child-asset-1', - }); + expect(mocks.asset.remove).toHaveBeenCalledWith(asset); }); it('should delete the entire stack if deleted asset was the primary asset and the stack would only contain one asset afterwards', async () => { mocks.stack.delete.mockResolvedValue(); mocks.assetJob.getForAssetDeletion.mockResolvedValue({ ...assetStub.primaryImage, - stack: { ...assetStub.primaryImage.stack, assets: assetStub.primaryImage.stack!.assets.slice(0, 2) }, + stack: { + id: 'stack-id', + primaryAssetId: assetStub.primaryImage.id, + assets: [{ id: 'one-asset' }], + }, }); await sut.handleAssetDeletion({ id: assetStub.primaryImage.id, deleteOnDisk: true }); - expect(mocks.stack.delete).toHaveBeenCalledWith('stack-1'); + expect(mocks.stack.delete).toHaveBeenCalledWith('stack-id'); }); it('should delete a live photo', async () => { diff --git a/server/src/services/asset.service.ts b/server/src/services/asset.service.ts index 066084ed45..23ba29e7b8 100644 --- a/server/src/services/asset.service.ts +++ b/server/src/services/asset.service.ts @@ -327,10 +327,11 @@ export class AssetService extends BaseService { return JobStatus.Failed; } - // Replace the parent of the stack children with a new asset + // replace the parent of the stack children with a new asset if (asset.stack?.primaryAssetId === id) { - const stackAssetIds = asset.stack?.assets.map((a) => a.id) ?? []; - if (stackAssetIds.length > 2) { + // this only includes timeline visible assets and excludes the primary asset + const stackAssetIds = asset.stack.assets.map((a) => a.id); + if (stackAssetIds.length >= 2) { const newPrimaryAssetId = stackAssetIds.find((a) => a !== id)!; await this.stackRepository.update(asset.stack.id, { id: asset.stack.id, diff --git a/server/test/medium/specs/services/asset.service.spec.ts b/server/test/medium/specs/services/asset.service.spec.ts index d0949c153c..1e34544e38 100644 --- a/server/test/medium/specs/services/asset.service.spec.ts +++ b/server/test/medium/specs/services/asset.service.spec.ts @@ -1,5 +1,5 @@ import { Kysely } from 'kysely'; -import { AssetFileType, AssetMetadataKey, JobName, SharedLinkType } from 'src/enum'; +import { AssetFileType, AssetMetadataKey, AssetStatus, JobName, SharedLinkType } from 'src/enum'; import { AccessRepository } from 'src/repositories/access.repository'; import { AlbumRepository } from 'src/repositories/album.repository'; import { AssetJobRepository } from 'src/repositories/asset-job.repository'; @@ -246,6 +246,66 @@ describe(AssetService.name, () => { }); }); + it('should delete a stacked primary asset (2 assets)', async () => { + const { sut, ctx } = setup(); + ctx.getMock(EventRepository).emit.mockResolvedValue(); + ctx.getMock(JobRepository).queue.mockResolvedValue(); + const { user } = await ctx.newUser(); + const { asset: asset1 } = await ctx.newAsset({ ownerId: user.id }); + const { asset: asset2 } = await ctx.newAsset({ ownerId: user.id }); + const { stack, result } = await ctx.newStack({ ownerId: user.id }, [asset1.id, asset2.id]); + + const stackRepo = ctx.get(StackRepository); + + expect(result).toMatchObject({ primaryAssetId: asset1.id }); + + await sut.handleAssetDeletion({ id: asset1.id, deleteOnDisk: true }); + + // stack is deleted as well + await expect(stackRepo.getById(stack.id)).resolves.toBe(undefined); + }); + + it('should delete a stacked primary asset (3 assets)', async () => { + const { sut, ctx } = setup(); + ctx.getMock(EventRepository).emit.mockResolvedValue(); + ctx.getMock(JobRepository).queue.mockResolvedValue(); + const { user } = await ctx.newUser(); + const { asset: asset1 } = await ctx.newAsset({ ownerId: user.id }); + const { asset: asset2 } = await ctx.newAsset({ ownerId: user.id }); + const { asset: asset3 } = await ctx.newAsset({ ownerId: user.id }); + const { stack, result } = await ctx.newStack({ ownerId: user.id }, [asset1.id, asset2.id, asset3.id]); + + expect(result).toMatchObject({ primaryAssetId: asset1.id }); + + await sut.handleAssetDeletion({ id: asset1.id, deleteOnDisk: true }); + + // new primary asset is picked + await expect(ctx.get(StackRepository).getById(stack.id)).resolves.toMatchObject({ primaryAssetId: asset2.id }); + }); + + it('should delete a stacked primary asset (3 trashed assets)', async () => { + const { sut, ctx } = setup(); + ctx.getMock(EventRepository).emit.mockResolvedValue(); + ctx.getMock(JobRepository).queue.mockResolvedValue(); + const { user } = await ctx.newUser(); + const { asset: asset1 } = await ctx.newAsset({ ownerId: user.id }); + const { asset: asset2 } = await ctx.newAsset({ ownerId: user.id }); + const { asset: asset3 } = await ctx.newAsset({ ownerId: user.id }); + const { stack, result } = await ctx.newStack({ ownerId: user.id }, [asset1.id, asset2.id, asset3.id]); + + await ctx.get(AssetRepository).updateAll([asset1.id, asset2.id, asset3.id], { + deletedAt: new Date(), + status: AssetStatus.Deleted, + }); + + expect(result).toMatchObject({ primaryAssetId: asset1.id }); + + await sut.handleAssetDeletion({ id: asset1.id, deleteOnDisk: true }); + + // stack is deleted as well + await expect(ctx.get(StackRepository).getById(stack.id)).resolves.toBe(undefined); + }); + it('should not delete offline assets', async () => { const { sut, ctx } = setup(); ctx.getMock(EventRepository).emit.mockResolvedValue(); From 5a6fd7af06b39e59a09b3aca32f14470582cc8bd Mon Sep 17 00:00:00 2001 From: Michel Heusschen <59014050+michelheusschen@users.noreply.github.com> Date: Wed, 4 Feb 2026 18:35:00 +0100 Subject: [PATCH 03/43] fix: close tag modal after tagging assets (#25884) --- web/src/lib/modals/AssetTagModal.svelte | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/web/src/lib/modals/AssetTagModal.svelte b/web/src/lib/modals/AssetTagModal.svelte index 00862ce1b1..c0c7f8b10a 100644 --- a/web/src/lib/modals/AssetTagModal.svelte +++ b/web/src/lib/modals/AssetTagModal.svelte @@ -33,6 +33,7 @@ const updatedIds = await tagAssets({ tagIds: [...selectedIds], assetIds, showNotification: false }); eventManager.emit('AssetsTag', updatedIds); + onClose(); }; const handleSelect = async (option?: ComboBoxOption) => { @@ -81,7 +82,7 @@ {#if tag}

{tag.value} From 6bd60270b43cb51604281ff416ff3eaf9a761abc Mon Sep 17 00:00:00 2001 From: Michel Heusschen <59014050+michelheusschen@users.noreply.github.com> Date: Wed, 4 Feb 2026 18:38:42 +0100 Subject: [PATCH 04/43] fix: correctly sync shared link download with metadata toggle (#25885) --- .../components/SharedLinkFormFields.spec.ts | 34 ++++++++++ .../components/SharedLinkFormFields.svelte | 65 +++++++++++++++++++ .../lib/modals/SharedLinkCreateModal.svelte | 51 ++++----------- .../(list)/[id]/edit/+page.svelte | 46 ++++--------- 4 files changed, 122 insertions(+), 74 deletions(-) create mode 100644 web/src/lib/components/SharedLinkFormFields.spec.ts create mode 100644 web/src/lib/components/SharedLinkFormFields.svelte diff --git a/web/src/lib/components/SharedLinkFormFields.spec.ts b/web/src/lib/components/SharedLinkFormFields.spec.ts new file mode 100644 index 0000000000..9c65c43833 --- /dev/null +++ b/web/src/lib/components/SharedLinkFormFields.spec.ts @@ -0,0 +1,34 @@ +import { render } from '@testing-library/svelte'; +import userEvent from '@testing-library/user-event'; +import SharedLinkFormFields from './SharedLinkFormFields.svelte'; + +describe('SharedLinkFormFields component', () => { + const isChecked = (element: Element) => + element instanceof HTMLInputElement ? element.checked : element.getAttribute('aria-checked') === 'true'; + + it('turns downloads off when metadata is disabled', async () => { + const { container } = render(SharedLinkFormFields, { + props: { + slug: '', + password: '', + description: '', + allowDownload: true, + allowUpload: false, + showMetadata: true, + expiresAt: null, + }, + }); + const user = userEvent.setup(); + + const switches = Array.from(container.querySelectorAll('[role="switch"], input[type="checkbox"]')); + expect(switches).toHaveLength(3); + + const [showMetadataSwitch, allowDownloadSwitch] = switches; + expect(isChecked(allowDownloadSwitch)).toBe(true); + + await user.click(showMetadataSwitch); + + expect(isChecked(showMetadataSwitch)).toBe(false); + expect(isChecked(allowDownloadSwitch)).toBe(false); + }); +}); diff --git a/web/src/lib/components/SharedLinkFormFields.svelte b/web/src/lib/components/SharedLinkFormFields.svelte new file mode 100644 index 0000000000..1e7b3b754b --- /dev/null +++ b/web/src/lib/components/SharedLinkFormFields.svelte @@ -0,0 +1,65 @@ + + +

+
+ + + + {#if slug} + /s/{encodeURIComponent(slug)} + {/if} +
+ + + + + + + + + + + + + + + + + + + + + +
diff --git a/web/src/lib/modals/SharedLinkCreateModal.svelte b/web/src/lib/modals/SharedLinkCreateModal.svelte index f36500c5d8..953076667e 100644 --- a/web/src/lib/modals/SharedLinkCreateModal.svelte +++ b/web/src/lib/modals/SharedLinkCreateModal.svelte @@ -1,8 +1,8 @@
-
- +
From 57e0835b4667dc03b85c855311a40a8d095b9b31 Mon Sep 17 00:00:00 2001 From: Michel Heusschen <59014050+michelheusschen@users.noreply.github.com> Date: Thu, 5 Feb 2026 14:36:20 +0100 Subject: [PATCH 09/43] fix: ensure theme stays in sync with @immich/ui (#25922) --- web/src/lib/managers/theme-manager.svelte.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/web/src/lib/managers/theme-manager.svelte.ts b/web/src/lib/managers/theme-manager.svelte.ts index 26c3fe31d5..0ce0172c1a 100644 --- a/web/src/lib/managers/theme-manager.svelte.ts +++ b/web/src/lib/managers/theme-manager.svelte.ts @@ -2,7 +2,7 @@ import { browser } from '$app/environment'; import { Theme } from '$lib/constants'; import { eventManager } from '$lib/managers/event-manager.svelte'; import { PersistedLocalStorage } from '$lib/utils/persisted'; -import { theme as uiTheme, type Theme as UiTheme } from '@immich/ui'; +import { onThemeChange as onUiThemeChange, theme as uiTheme, type Theme as UiTheme } from '@immich/ui'; export interface ThemeSetting { value: Theme; @@ -55,15 +55,14 @@ class ThemeManager { } #onAppInit() { - globalThis.matchMedia('(prefers-color-scheme: dark)').addEventListener( - 'change', - () => { - if (this.theme.system) { - this.#update('system'); - } - }, - { passive: true }, - ); + const syncSystemTheme = () => { + this.#update(this.theme.system ? 'system' : this.theme.value); + }; + + syncSystemTheme(); + globalThis.matchMedia('(prefers-color-scheme: dark)').addEventListener('change', syncSystemTheme, { + passive: true, + }); } #update(value: Theme | 'system') { @@ -75,6 +74,7 @@ class ThemeManager { this.#theme.current = theme; uiTheme.value = theme.value as unknown as UiTheme; + onUiThemeChange(); eventManager.emit('ThemeChange', theme); } From 810e9254f331e2633c1d7eb8334b7e01613e5f70 Mon Sep 17 00:00:00 2001 From: Michel Heusschen <59014050+michelheusschen@users.noreply.github.com> Date: Thu, 5 Feb 2026 14:51:30 +0100 Subject: [PATCH 10/43] fix: preserve hidden people state across pagination (#25886) * fix: preserve hidden people state across pagination * track overrides instead * use event instead of bind:people * update test --- .../manage-people-visibility.spec.ts | 88 +++++++++++++++++++ .../manage-people-visibility.svelte | 50 ++++++----- ...nage-people-visibility.test-wrapper.svelte | 20 +++++ web/src/routes/(user)/people/+page.svelte | 4 +- 4 files changed, 139 insertions(+), 23 deletions(-) create mode 100644 web/src/lib/components/faces-page/manage-people-visibility.spec.ts create mode 100644 web/src/lib/components/faces-page/manage-people-visibility.test-wrapper.svelte diff --git a/web/src/lib/components/faces-page/manage-people-visibility.spec.ts b/web/src/lib/components/faces-page/manage-people-visibility.spec.ts new file mode 100644 index 0000000000..9bead61162 --- /dev/null +++ b/web/src/lib/components/faces-page/manage-people-visibility.spec.ts @@ -0,0 +1,88 @@ +import { getIntersectionObserverMock } from '$lib/__mocks__/intersection-observer.mock'; +import { personFactory } from '@test-data/factories/person-factory'; +import { render } from '@testing-library/svelte'; +import userEvent from '@testing-library/user-event'; +import { vi } from 'vitest'; +import ManagePeopleVisibilityWrapper from './manage-people-visibility.test-wrapper.svelte'; + +describe('ManagePeopleVisibility component', () => { + beforeEach(() => { + vi.stubGlobal('IntersectionObserver', getIntersectionObserverMock()); + }); + + it('keeps toggled hidden state when loading more people', async () => { + const onClose = vi.fn(); + const onUpdate = vi.fn(); + const loadNextPage = vi.fn(); + + const [personA, personB, personC] = [ + personFactory.build({ id: 'a', isHidden: false }), + personFactory.build({ id: 'b', isHidden: false }), + personFactory.build({ id: 'c', isHidden: true }), + ]; + + const { container, rerender } = render(ManagePeopleVisibilityWrapper, { + props: { + people: [personA, personB], + totalPeopleCount: 3, + onClose, + onUpdate, + loadNextPage, + }, + }); + const user = userEvent.setup(); + + let personButtons = container.querySelectorAll('button[aria-pressed]'); + expect(personButtons).toHaveLength(2); + + await user.click(personButtons[0]); + expect(personButtons[0].getAttribute('aria-pressed')).toBe('true'); + + await rerender({ + people: [personA, personB, personC], + totalPeopleCount: 3, + onClose, + onUpdate, + loadNextPage, + }); + + personButtons = container.querySelectorAll('button[aria-pressed]'); + expect(personButtons).toHaveLength(3); + expect(personButtons[0].getAttribute('aria-pressed')).toBe('true'); + expect(personButtons[2].getAttribute('aria-pressed')).toBe('true'); + }); + + it('shows newly loaded hidden people as hidden', async () => { + const onClose = vi.fn(); + const onUpdate = vi.fn(); + const loadNextPage = vi.fn(); + + const [personA, personB, personC] = [ + personFactory.build({ id: 'a', isHidden: false }), + personFactory.build({ id: 'b', isHidden: false }), + personFactory.build({ id: 'c', isHidden: true }), + ]; + + const { container, rerender } = render(ManagePeopleVisibilityWrapper, { + props: { + people: [personA, personB], + totalPeopleCount: 3, + onClose, + onUpdate, + loadNextPage, + }, + }); + + await rerender({ + people: [personA, personB, personC], + totalPeopleCount: 3, + onClose, + onUpdate, + loadNextPage, + }); + + const personButtons = container.querySelectorAll('button[aria-pressed]'); + expect(personButtons).toHaveLength(3); + expect(personButtons[2].getAttribute('aria-pressed')).toBe('true'); + }); +}); diff --git a/web/src/lib/components/faces-page/manage-people-visibility.svelte b/web/src/lib/components/faces-page/manage-people-visibility.svelte index 5771766f64..77696b5983 100644 --- a/web/src/lib/components/faces-page/manage-people-visibility.svelte +++ b/web/src/lib/components/faces-page/manage-people-visibility.svelte @@ -10,27 +10,22 @@ import { Button, IconButton, toastManager } from '@immich/ui'; import { mdiClose, mdiEye, mdiEyeOff, mdiEyeSettings, mdiRestart } from '@mdi/js'; import { t } from 'svelte-i18n'; + import { SvelteMap } from 'svelte/reactivity'; interface Props { people: PersonResponseDto[]; totalPeopleCount: number; titleId?: string | undefined; onClose: () => void; + onUpdate: (people: PersonResponseDto[]) => void; loadNextPage: () => void; } - let { people = $bindable(), totalPeopleCount, titleId = undefined, onClose, loadNextPage }: Props = $props(); + let { people, totalPeopleCount, titleId = undefined, onClose, onUpdate, loadNextPage }: Props = $props(); let toggleVisibility = $state(ToggleVisibility.SHOW_ALL); let showLoadingSpinner = $state(false); - - const getPersonIsHidden = (people: PersonResponseDto[]) => { - const personIsHidden: Record = {}; - for (const person of people) { - personIsHidden[person.id] = person.isHidden; - } - return personIsHidden; - }; + const overrides = new SvelteMap(); const getNextVisibility = (toggleVisibility: ToggleVisibility) => { if (toggleVisibility === ToggleVisibility.SHOW_ALL) { @@ -46,23 +41,23 @@ toggleVisibility = getNextVisibility(toggleVisibility); for (const person of people) { + let isHidden = overrides.get(person.id) ?? person.isHidden; + if (toggleVisibility === ToggleVisibility.HIDE_ALL) { - personIsHidden[person.id] = true; + isHidden = true; } else if (toggleVisibility === ToggleVisibility.SHOW_ALL) { - personIsHidden[person.id] = false; + isHidden = false; } else if (toggleVisibility === ToggleVisibility.HIDE_UNNANEMD && !person.name) { - personIsHidden[person.id] = true; + isHidden = true; } + + setHiddenOverride(person, isHidden); } }; - const handleResetVisibility = () => (personIsHidden = getPersonIsHidden(people)); - const handleSaveVisibility = async () => { showLoadingSpinner = true; - const changed = people - .filter((person) => person.isHidden !== personIsHidden[person.id]) - .map((person) => ({ id: person.id, isHidden: personIsHidden[person.id] })); + const changed = Array.from(overrides, ([id, isHidden]) => ({ id, isHidden })); try { if (changed.length > 0) { @@ -76,9 +71,14 @@ } for (const person of people) { - person.isHidden = personIsHidden[person.id]; + const isHidden = overrides.get(person.id); + if (isHidden !== undefined) { + person.isHidden = isHidden; + } } + overrides.clear(); + onUpdate(people); onClose(); } catch (error) { handleError(error, $t('errors.unable_to_change_visibility', { values: { count: changed.length } })); @@ -87,7 +87,13 @@ } }; - let personIsHidden = $state(getPersonIsHidden(people)); + const setHiddenOverride = (person: PersonResponseDto, isHidden: boolean) => { + if (isHidden === person.isHidden) { + overrides.delete(person.id); + return; + } + overrides.set(person.id, isHidden); + }; let toggleButtonOptions: Record = $derived({ [ToggleVisibility.HIDE_ALL]: { icon: mdiEyeOff, label: $t('hide_all_people') }, @@ -124,7 +130,7 @@ variant="ghost" aria-label={$t('reset_people_visibility')} icon={mdiRestart} - onclick={handleResetVisibility} + onclick={() => overrides.clear()} /> {#snippet children({ person })} - {@const hidden = personIsHidden[person.id]} + {@const hidden = overrides.get(person.id) ?? person.isHidden}