fix(memories): give the photo picker a chronological order it can rely on

The picker got its photos back in whatever order the provider happened to
return them. It groups by day and appends pages lazily, so an unordered page
lands under the wrong heading and pushes photos out of the fold the reader
is looking at.

Immich now asks for order: desc on both the search and the album path. An
older server that does not know the property drops it silently rather than
failing, so each page is sorted again here; the album paths drain every page
before sorting and are covered outright. getAssetInfo reports mediaType, the
same mapping the listing already used, so the detail route and the picker
agree on what an asset is.

Synology gets no sort parameter upstream on either path. Search.Search
documents none, and _fetchSynologyJson maps every app code except 106/107/119
onto a 400, so 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.

No media type for Synology yet, deliberately. streamSynologyAsset serves
every asset as a JPEG out of SYNO.Foto.Thumbnail, original included; there is
no video byte path. Labelling an asset a video would make the lightbox mount
a player on a still image, which is worse than the missing badge. The badge,
the byte path and the persisted media_type belong together in their own
change.

Five album cases in the Immich integration suite pinned the old arrival
order. Where the position was incidental they now look the asset up by id;
where the order is the point they state the chronological one.
This commit is contained in:
Maurice
2026-09-08 14:04:45 +02:00
committed by Maurice
parent cfae0a1f78
commit 2d7889c237
7 changed files with 259 additions and 15 deletions
+13 -3
View File
@@ -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 };
}
@@ -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<T extends { takenAt?: string | null }>(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;
+10 -2
View File
@@ -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<AssetsList>;
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,
@@ -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);
});
+56 -1
View File
@@ -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', () => {
@@ -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']);
});
});
@@ -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 });