diff --git a/server/src/nest/memories/immich.service.ts b/server/src/nest/memories/immich.service.ts index fbe96529f..39495f3d3 100644 --- a/server/src/nest/memories/immich.service.ts +++ b/server/src/nest/memories/immich.service.ts @@ -8,7 +8,7 @@ import fsPromises from 'node:fs/promises'; import path from 'node:path'; import { DatabaseService } from '../database/database.service'; import { MemoriesAccessService } from './memories-access.service'; -import { fail, handleServiceResult, pipeAsset, type Selection } from './memories.helpers'; +import { fail, handleServiceResult, pipeAsset, sortAssetsByTakenAtDesc, type Selection } from './memories.helpers'; const ALBUM_PAGE_SIZE = 1000; const ALBUM_MAX_PAGES = 20; @@ -226,6 +226,12 @@ export class ImmichService { // below is the only guard on those versions. Do not remove it. visibility: 'timeline', withExif: true, + // Immich's own default order has moved between versions, and the picker + // pages lazily: an unordered page 2 lands in the wrong day heading. + // Same whitelist reasoning as `visibility` above — an older server that + // does not know the property drops it instead of failing the request, + // which is why the result is sorted again below. + order: 'desc', size, page, }), @@ -249,7 +255,7 @@ export class ImmichService { lng: typeof a.exifInfo?.longitude === 'number' ? a.exifInfo.longitude : null, mediaType: a.type === 'VIDEO' ? 'video' : 'image', })); - return { assets, hasMore: items.length >= size }; + return { assets: sortAssetsByTakenAtDesc(assets), hasMore: items.length >= size }; } catch { return { error: 'Could not reach Immich', status: 502 }; } @@ -279,6 +285,7 @@ export class ImmichService { data: { id: asset.id, takenAt: asset.fileCreatedAt || asset.createdAt, + mediaType: asset.type === 'VIDEO' ? 'video' as const : 'image' as const, width: asset.exifInfo?.exifImageWidth || null, height: asset.exifInfo?.exifImageHeight || null, camera: asset.exifInfo?.make && asset.exifInfo?.model ? `${asset.exifInfo.make} ${asset.exifInfo.model}` : null, @@ -487,6 +494,7 @@ export class ImmichService { albumIds: [albumId], withExif: true, withDeleted: false, + order: 'desc', size: ALBUM_PAGE_SIZE, page, }), @@ -529,7 +537,9 @@ export class ImmichService { lng: typeof a.exifInfo?.longitude === 'number' ? a.exifInfo.longitude : null, mediaType: a.type === 'VIDEO' ? 'video' : 'image', })); - return { assets }; + // The v2 branch reads /api/albums/{id} and never passes a search at all, so + // this is the only ordering an album ever gets there. + return { assets: sortAssetsByTakenAtDesc(assets) }; } catch { return { error: 'Could not reach Immich', status: 502 }; } diff --git a/server/src/nest/memories/memories.helpers.ts b/server/src/nest/memories/memories.helpers.ts index 8f23eb0a6..e6b0184e0 100644 --- a/server/src/nest/memories/memories.helpers.ts +++ b/server/src/nest/memories/memories.helpers.ts @@ -87,10 +87,46 @@ export type AssetsList = { hasMore: boolean }; +/** + * Newest first, by capture time, with a stable fallback. + * + * Neither provider guarantees an order: Immich sorts by whatever its version + * defaults to and Synology's search API documents none at all. The picker groups + * by day and lazily appends pages, so an unordered page puts photos in the wrong + * day heading and, worse, drops them above the fold the reader is looking at. + * Asking upstream for `desc` is the fix; this is the belt to that pair of braces, + * and it is load-bearing for the album paths, which do not run through a sorted + * search at all. + * + * Assets without a usable timestamp keep their relative order at the end, which + * matches how the client groups them under its unknown-date heading. That, and + * the order of assets sharing a timestamp, rests on Array.prototype.sort being + * stable, which it has been since ES2019. + * + * Timestamps are parsed once up front rather than inside the comparator, which + * would re-parse the same string O(n log n) times. + */ +export function sortAssetsByTakenAtDesc(assets: T[]): T[] { + return assets + .map(asset => { + const parsed = asset.takenAt ? Date.parse(asset.takenAt) : Number.NaN; + return { asset, at: Number.isNaN(parsed) ? null : parsed }; + }) + .sort((a, b) => { + if (a.at === null && b.at === null) return 0; + if (a.at === null) return 1; + if (b.at === null) return -1; + return b.at - a.at; + }) + .map(entry => entry.asset); +} + export type AssetInfo = { id: string; takenAt: string | null; + /** What the provider says this is. Absent means the provider does not tell us. */ + mediaType?: 'image' | 'video'; city: string | null; country: string | null; state?: string | null; diff --git a/server/src/nest/memories/synology.service.ts b/server/src/nest/memories/synology.service.ts index 312ce0d62..97c40214a 100644 --- a/server/src/nest/memories/synology.service.ts +++ b/server/src/nest/memories/synology.service.ts @@ -9,6 +9,7 @@ import { handleServiceResult, success, pipeAsset, + sortAssetsByTakenAtDesc, type AlbumsList, type AssetInfo, type AssetsList, @@ -526,7 +527,8 @@ export class SynologyService { lng: typeof item.additional?.gps?.longitude === 'number' ? item.additional.gps.longitude : null, })).filter(a => a.id); - return success({ assets, total: assets.length, hasMore: false }); + const ordered = sortAssetsByTakenAtDesc(assets); + return success({ assets: ordered, total: ordered.length, hasMore: false }); } /** @@ -593,7 +595,13 @@ export class SynologyService { if (!result.success) return result as ServiceResult; const allItems = result.data.list || []; - const assets = allItems.map(item => this._normalizeSynologyPhotoInfo(item)); + // Deliberately no sort parameter upstream, on this path or the album one. + // Synology documents none for Search.Search, and _fetchSynologyJson maps + // every app code except 106/107/119 onto a 400 — a parameter one DSM build + // rejects would take listing down entirely for that user. Both paths read + // every page anyway, so ordering the mapped result costs nothing and + // cannot fail. + const assets = sortAssetsByTakenAtDesc(allItems.map(item => this._normalizeSynologyPhotoInfo(item))); return success({ assets, diff --git a/server/tests/integration/memories-immich.test.ts b/server/tests/integration/memories-immich.test.ts index 162146f02..f6b8fc4eb 100644 --- a/server/tests/integration/memories-immich.test.ts +++ b/server/tests/integration/memories-immich.test.ts @@ -558,7 +558,8 @@ describe('Immich album photos', () => { expect(res.status).toBe(200); expect(res.body.assets).toHaveLength(2); - expect(res.body.assets.map((a: any) => a.id)).toEqual(['asset-sync-1', 'asset-sync-2']); + // Newest first: asset-sync-2 was taken on the 2nd, asset-sync-1 on the 1st. + expect(res.body.assets.map((a: any) => a.id)).toEqual(['asset-sync-2', 'asset-sync-1']); }); it('IMMICH-063 — GET /albums/:id/photos filters hidden assets (both visibility and legacy isVisible markers)', async () => { @@ -582,14 +583,17 @@ describe('Immich album photos', () => { .get(`${IMMICH}/albums/album-uuid-1/photos`) .set('Cookie', authCookie(user.id)); - expect(res.body.assets[0]).toMatchObject({ + // Keyed by id, not by position: the response is sorted by capture time now, + // so an index would pin the ordering here as a side effect. + const byId = Object.fromEntries(res.body.assets.map((a: any) => [a.id, a])); + expect(byId['asset-sync-1']).toMatchObject({ id: 'asset-sync-1', takenAt: '2024-06-01T10:00:00.000Z', city: 'Paris', country: 'France', mediaType: 'image', }); - expect(res.body.assets[1].mediaType).toBe('video'); + expect(byId['asset-sync-2'].mediaType).toBe('video'); }); // #1614 — the album mapping used to drop lat/lng even though the search path @@ -602,11 +606,12 @@ describe('Immich album photos', () => { .get(`${IMMICH}/albums/album-uuid-1/photos`) .set('Cookie', authCookie(user.id)); - expect(res.body.assets[0]).toMatchObject({ lat: 48.8584, lng: 2.2945 }); - // The second fixture asset has no coordinates; it must come back null, not undefined + const byId = Object.fromEntries(res.body.assets.map((a: any) => [a.id, a])); + expect(byId['asset-sync-1']).toMatchObject({ lat: 48.8584, lng: 2.2945 }); + // asset-sync-2 has no coordinates; it must come back null, not undefined // or a half pair. - expect(res.body.assets[1].lat).toBeNull(); - expect(res.body.assets[1].lng).toBeNull(); + expect(byId['asset-sync-2'].lat).toBeNull(); + expect(byId['asset-sync-2'].lng).toBeNull(); }); it('IMMICH-065 — album photos are fetched via search/metadata with albumIds and withExif', async () => { @@ -623,6 +628,9 @@ describe('Immich album photos', () => { expect(immichState.searchCalls[0]).toMatchObject({ albumIds: ['album-uuid-1'], withExif: true, + // The album path asks for the order too; without this it could fall out + // again unnoticed, because the local sort would still hide it here. + order: 'desc', page: 1, }); }); @@ -644,7 +652,10 @@ describe('Immich album photos', () => { expect(res.status).toBe(200); expect(res.body.assets).toHaveLength(1001); - expect(res.body.assets[1000].id).toBe('tail-asset'); + // tail-asset is the only one from page two and the newest of the 1001, so + // the chronological sort puts it first. Its presence is what proves page + // two was fetched at all. + expect(res.body.assets[0].id).toBe('tail-asset'); expect(immichState.searchCalls.map((c) => c.page)).toEqual([1, 2]); }); @@ -661,7 +672,7 @@ describe('Immich album photos', () => { .set('Cookie', authCookie(user.id)); expect(res.status).toBe(200); - expect(res.body.assets.map((a: any) => a.id)).toEqual(['asset-sync-1', 'asset-sync-2']); + expect(res.body.assets.map((a: any) => a.id)).toEqual(['asset-sync-2', 'asset-sync-1']); expect(immichState.searchCalls).toHaveLength(0); }); diff --git a/server/tests/unit/nest/immich.service.test.ts b/server/tests/unit/nest/immich.service.test.ts index 91d01f04d..0b32f4614 100644 --- a/server/tests/unit/nest/immich.service.test.ts +++ b/server/tests/unit/nest/immich.service.test.ts @@ -298,7 +298,10 @@ describe('searchPhotos', () => { ] } }, })); - const [a, b] = (await svc.searchPhotos(USER)).assets!; + // By id, not by position: the result is ordered newest first, so 'b' leads. + const assets = (await svc.searchPhotos(USER)).assets!; + const a = assets.find(x => x.id === 'a'); + const b = assets.find(x => x.id === 'b'); expect(a).toMatchObject({ takenAt: '2026-02-02', city: 'Kyoto', country: 'JP', lat: 35.0, lng: 135.7, mediaType: 'video' }); // A non-numeric coordinate becomes null rather than reaching the client as a string. @@ -327,6 +330,23 @@ describe('searchPhotos', () => { expect(body.takenBefore).toBe('2026-01-31T23:59:59.999Z'); // Load-bearing on Immich >= 1.133: hidden assets must not cross the wire. expect(body.visibility).toBe('timeline'); + expect(body.order).toBe('desc'); + }); + + it('IMMICH-032b: orders each page itself, so an unsorted page still lands chronological within itself', async () => { + // Older builds drop the unknown property silently (whitelist: true without + // forbidNonWhitelisted), which is exactly why the request cannot be trusted. + safeFetch.mockResolvedValue(upstream({ + json: { assets: { items: [ + { id: 'older', fileCreatedAt: '2026-03-01T09:00:00Z' }, + { id: 'newest', fileCreatedAt: '2026-03-31T09:00:00Z' }, + { id: 'middle', fileCreatedAt: '2026-03-15T09:00:00Z' }, + ] } }, + })); + + const result = await svc.searchPhotos(USER); + + expect(result.assets!.map(a => a.id)).toEqual(['newest', 'middle', 'older']); }); }); @@ -372,6 +392,41 @@ describe('getAssetInfo', () => { const data = (await svc.getAssetInfo(USER, 'bare')).data; expect(data).toMatchObject({ id: 'bare', width: null, height: null, city: null, fileName: null }); }); + + it('IMMICH-037b: reports the media type here too, not only in the listing', async () => { + // Same mapping as the listing, so the detail route and the picker agree on + // what an asset is. Nothing persists it from here yet — trek_photos.media_type + // comes from the add call's media_types. + safeFetch.mockResolvedValueOnce(upstream({ json: { id: 'clip', type: 'VIDEO' } })); + expect((await svc.getAssetInfo(USER, 'clip')).data.mediaType).toBe('video'); + + safeFetch.mockResolvedValueOnce(upstream({ json: { id: 'still', type: 'IMAGE' } })); + expect((await svc.getAssetInfo(USER, 'still')).data.mediaType).toBe('image'); + }); +}); + +describe('getAlbumPhotos', () => { + it('IMMICH-037c: 400s without credentials', async () => { + seedUser(11, null, null); + expect(await svc.getAlbumPhotos(11, 'alb-1')).toEqual({ error: 'Immich not configured', status: 400 }); + }); + + it('IMMICH-037d: orders an album newest first and keeps its coordinates and media type', async () => { + // The v2 branch reads /api/albums/{id} directly and never passes a search, + // so this ordering is the only one an album on that version ever gets. + safeFetch.mockResolvedValue(upstream({ + json: { assets: [ + { id: 'older', fileCreatedAt: '2026-03-01T09:00:00Z', type: 'IMAGE', exifInfo: { latitude: 35.0, longitude: 135.7 } }, + { id: 'clip', fileCreatedAt: '2026-03-31T09:00:00Z', type: 'VIDEO' }, + ] }, + })); + + const assets = (await svc.getAlbumPhotos(USER, 'alb-1')).assets!; + + expect(assets.map(a => a.id)).toEqual(['clip', 'older']); + expect(assets[0].mediaType).toBe('video'); + expect(assets[1]).toMatchObject({ lat: 35.0, lng: 135.7 }); + }); }); describe('fetchImmichThumbnailBytes', () => { diff --git a/server/tests/unit/nest/memories.helpers.test.ts b/server/tests/unit/nest/memories.helpers.test.ts new file mode 100644 index 000000000..8906157f5 --- /dev/null +++ b/server/tests/unit/nest/memories.helpers.test.ts @@ -0,0 +1,80 @@ +/** + * The provider-agnostic ordering the memories domain hands to the picker. + * + * Both provider services ask upstream for a descending order now, but neither + * Immich nor Synology guarantees it across versions, and the album paths do not + * run through a sorted search at all. This is the fallback that makes the day + * headings in the picker mean something, so it is pinned here rather than + * inside either provider suite. + */ +import { describe, it, expect } from 'vitest'; +import { sortAssetsByTakenAtDesc } from '../../../src/nest/memories/memories.helpers'; + +const asset = (id: string, takenAt?: string | null) => ({ id, takenAt }); + +const ids = (assets: { id: string }[]) => assets.map(a => a.id); + +describe('sortAssetsByTakenAtDesc', () => { + it('MEM-SORT-001: puts the newest capture first regardless of the order upstream sent', () => { + const out = sortAssetsByTakenAtDesc([ + asset('middle', '2026-03-15T09:00:00Z'), + asset('oldest', '2026-03-01T09:00:00Z'), + asset('newest', '2026-03-31T09:00:00Z'), + ]); + + expect(ids(out)).toEqual(['newest', 'middle', 'oldest']); + }); + + it('MEM-SORT-002: keeps the upstream order between assets sharing a timestamp', () => { + // Burst shots land on the same second often enough that an unstable sort + // would reshuffle them on every page load. + const out = sortAssetsByTakenAtDesc([ + asset('first', '2026-03-15T09:00:00Z'), + asset('second', '2026-03-15T09:00:00Z'), + asset('third', '2026-03-15T09:00:00Z'), + ]); + + expect(ids(out)).toEqual(['first', 'second', 'third']); + }); + + it('MEM-SORT-003: sends assets without a usable timestamp to the end, in order', () => { + const out = sortAssetsByTakenAtDesc([ + asset('no-date'), + asset('dated', '2026-03-15T09:00:00Z'), + asset('null-date', null), + asset('empty-date', ''), + ]); + + expect(ids(out)).toEqual(['dated', 'no-date', 'null-date', 'empty-date']); + }); + + it('MEM-SORT-004: treats an unparsable timestamp as missing rather than sorting on the string', () => { + // Synology hands back an epoch it converts itself; a malformed value must not + // outrank a real date just because it compares high as text. + const out = sortAssetsByTakenAtDesc([ + asset('garbage', 'not-a-date'), + asset('real', '2026-03-15T09:00:00Z'), + ]); + + expect(ids(out)).toEqual(['real', 'garbage']); + }); + + it('MEM-SORT-005: leaves an empty list alone and does not mutate its input', () => { + expect(sortAssetsByTakenAtDesc([])).toEqual([]); + + const input = [asset('a', '2026-01-01T00:00:00Z'), asset('b', '2026-02-01T00:00:00Z')]; + sortAssetsByTakenAtDesc(input); + + expect(ids(input)).toEqual(['a', 'b']); + }); + + it('MEM-SORT-006: compares across timezone offsets by instant, not by text', () => { + // 23:00Z is later than 09:00-06:00 (15:00Z) even though the string sorts lower. + const out = sortAssetsByTakenAtDesc([ + asset('offset', '2026-03-15T09:00:00-06:00'), + asset('utc', '2026-03-15T23:00:00Z'), + ]); + + expect(ids(out)).toEqual(['utc', 'offset']); + }); +}); diff --git a/server/tests/unit/nest/synology.service.test.ts b/server/tests/unit/nest/synology.service.test.ts index bbe958a98..b8d3ed01f 100644 --- a/server/tests/unit/nest/synology.service.test.ts +++ b/server/tests/unit/nest/synology.service.test.ts @@ -318,6 +318,30 @@ describe('getSynologyAlbumPhotos', () => { safeFetch.mockResolvedValue(httpError(503)); expect((await svc.getSynologyAlbumPhotos(USER, '7')).success).toBe(false); }); + + it('SYNO-U063: asks Browse.Item for no ordering of its own', async () => { + // The loop below drains every page before sorting, so an upstream sort buys + // nothing — while a parameter some DSM build rejects becomes a 400 and takes + // the whole album down. + safeFetch.mockResolvedValue(api({ list: [] })); + + await svc.getSynologyAlbumPhotos(USER, '7'); + + const body = String((safeFetch.mock.calls[0][1] as { body: URLSearchParams }).body); + expect(body).not.toContain('sort_by'); + expect(body).not.toContain('sort_direction'); + }); + + it('SYNO-U064: orders the album by capture time, newest first', async () => { + safeFetch.mockResolvedValue(api({ list: [ + { id: 1, time: 1700000000, additional: { thumbnail: { cache_key: 'older' } } }, + { id: 2, time: 1800000000, additional: { thumbnail: { cache_key: 'newer' } } }, + ] })); + + const assets = (await svc.getSynologyAlbumPhotos(USER, '7') as { data: { assets: { id: string }[] } }).data.assets; + + expect(assets.map(a => a.id)).toEqual(['newer', 'older']); + }); }); describe('collectSynologyAlbumSelection', () => { @@ -343,6 +367,26 @@ describe('collectSynologyAlbumSelection', () => { }); }); +describe('the search order', () => { + it('SYNO-U065: search results come back newest first without a sort parameter upstream', async () => { + // SYNO.Foto.Search.Search documents no sort_by, and _fetchSynologyJson maps + // every app code except 106/107/119 onto a 400, so a rejected parameter would + // take search down for that user entirely. Ordering happens here instead. + safeFetch.mockResolvedValue(api({ list: [ + { id: 1, time: 1700000000, additional: { thumbnail: { cache_key: 'older' } } }, + { id: 2, time: 1800000000, additional: { thumbnail: { cache_key: 'newer' } } }, + ] })); + + const result = await svc.searchSynologyPhotos(USER); + + const assets = (result as { data: { assets: { id: string }[] } }).data.assets; + expect(assets.map(a => a.id)).toEqual(['newer', 'older']); + const body = String((safeFetch.mock.calls[0][1] as { body: URLSearchParams }).body); + expect(body).not.toContain('sort_by'); + }); + +}); + describe('fetchSynologyThumbnailBytes', () => { it('SYNO-U080: fails without credentials rather than fetching', async () => { seedUser(6, { synology_url: null });