diff --git a/client/src/api/client.ts b/client/src/api/client.ts index 6d9ca3589..c033fc057 100644 --- a/client/src/api/client.ts +++ b/client/src/api/client.ts @@ -1050,7 +1050,10 @@ const MEMORIES_TIMEOUT = 0 export const memoriesApi = { status: (provider: string): Promise<{ connected: boolean }> => apiClient.get(`/integrations/memories/${provider}/status`, { timeout: MEMORIES_TIMEOUT }).then(r => r.data), - search: (provider: string, body: { from: string; to: string; page: number; size: number }, signal?: AbortSignal) => + // utc_offset_minutes says which 24 hours from/to name. It is NOT the Synology + // `offset`, which is rows to skip: one body goes to whichever provider, so the + // two names have to stay apart (#2336). + search: (provider: string, body: { from: string; to: string; page: number; size: number; utc_offset_minutes?: number }, signal?: AbortSignal) => apiClient.post(`/integrations/memories/${provider}/search`, body, { timeout: MEMORIES_TIMEOUT, signal }).then(r => r.data), albums: (provider: string) => apiClient.get(`/integrations/memories/${provider}/albums`, { timeout: MEMORIES_TIMEOUT }).then(r => r.data), diff --git a/client/src/components/Journey/JourneyDetailPageProviderPicker.test.tsx b/client/src/components/Journey/JourneyDetailPageProviderPicker.test.tsx index b3ecb7c69..9fb26ead9 100644 --- a/client/src/components/Journey/JourneyDetailPageProviderPicker.test.tsx +++ b/client/src/components/Journey/JourneyDetailPageProviderPicker.test.tsx @@ -1,6 +1,6 @@ -// FE-JRN-PICKER-001 to FE-JRN-PICKER-021 +// FE-JRN-PICKER-001 to FE-JRN-PICKER-023 -import { describe, it, expect, beforeEach, vi } from 'vitest' +import { describe, it, expect, afterAll, beforeAll, beforeEach, vi } from 'vitest' import { http, HttpResponse, delay } from 'msw' import userEvent from '@testing-library/user-event' import { render, screen, waitFor, within, fireEvent } from '../../../tests/helpers/render' @@ -54,6 +54,16 @@ function mountPicker(props: Partial> return { ...utils, onClose, onAdd } } +// The day headings follow the reader's wall clock now, so they would follow the +// runner's zone too. Nothing in the vitest config pins one, so this file does. +const runnerTimeZone = process.env.TZ + +beforeAll(() => { process.env.TZ = 'UTC' }) +afterAll(() => { + if (runnerTimeZone === undefined) delete process.env.TZ + else process.env.TZ = runnerTimeZone +}) + beforeEach(() => { searchReturns([asset('a1')]) }) @@ -103,7 +113,9 @@ describe('ProviderPicker', () => { await user.click(screen.getByRole('button', { name: /All Photos/ })) await waitFor(() => expect(ranges).toHaveLength(2)) - expect(ranges[1]).toEqual({ from: '', to: '', page: 1, size: 50 }) + // utc_offset_minutes rides along on every search: the server cannot tell + // which 24 hours a date-only bound means without it (#2336). + expect(ranges[1]).toEqual({ from: '', to: '', page: 1, size: 50, utc_offset_minutes: 0 }) }) it('FE-JRN-PICKER-006: the album tab loads albums and reports when there are none', async () => { @@ -286,6 +298,55 @@ describe('ProviderPicker', () => { } }) + it('FE-JRN-PICKER-022: says which zone the searched day is meant in', async () => { + const ranges: Record[] = [] + server.use(http.post('/api/integrations/memories/immich/search', async ({ request }) => { + ranges.push(await request.json() as Record) + return HttpResponse.json({ assets: [asset('a1')], hasMore: false }) + })) + process.env.TZ = 'Australia/Sydney' + try { + mountPicker({ initialDate: '2026-03-15' }) + await waitFor(() => expect(ranges).toHaveLength(1)) + } finally { + process.env.TZ = 'UTC' + } + + // +11 on 15 March, read at local noon of the day being searched rather than + // at "now" — Sydney is +10 for half the year, so a day picked out of the + // other half would otherwise be an hour off. + expect(ranges[0]).toMatchObject({ from: '2026-03-15', to: '2026-03-15', utc_offset_minutes: 660 }) + }) + + it('FE-JRN-PICKER-023: keeps paging when a page holds no photos of the searched day', async () => { + // The local-day filter can empty a whole page (50 shots taken the next + // morning). The sentinel used to live inside the grid, so an empty page + // mounted none and the picker sat on "no photos" forever (#2336). + const pages: number[] = [] + server.use(http.post('/api/integrations/memories/immich/search', async ({ request }) => { + const body = await request.json() as { page: number } + pages.push(body.page) + return HttpResponse.json({ assets: body.page < 2 ? [] : [asset('a1')], hasMore: body.page < 2 }) + })) + class ImmediateObserver { + constructor(private cb: IntersectionObserverCallback) {} + observe() { this.cb([{ isIntersecting: true } as IntersectionObserverEntry], this as unknown as IntersectionObserver) } + unobserve() {} + disconnect() {} + } + const original = globalThis.IntersectionObserver + globalThis.IntersectionObserver = ImmediateObserver as unknown as typeof IntersectionObserver + try { + mountPicker() + await waitFor(() => expect(pages).toEqual([1, 2])) + expect(await screen.findAllByAltText('')).toHaveLength(1) + // And the empty first page never claimed there were none to find. + expect(screen.queryByText('No photos yet')).not.toBeInTheDocument() + } finally { + globalThis.IntersectionObserver = original + } + }) + it('FE-JRN-PICKER-018: the embedded variant drops the header, the add-to bar and the date captions', async () => { mountPicker({ embedded: true, initialDate: '2026-03-15' }) diff --git a/client/src/components/Journey/JourneyDetailPageProviderPicker.tsx b/client/src/components/Journey/JourneyDetailPageProviderPicker.tsx index 234f6e0ba..3d5bad9d3 100644 --- a/client/src/components/Journey/JourneyDetailPageProviderPicker.tsx +++ b/client/src/components/Journey/JourneyDetailPageProviderPicker.tsx @@ -5,6 +5,7 @@ import { useTranslation } from '../../i18n'; import { groupPhotosByDate, sortProviderPhotos, + utcOffsetMinutesForDay, type GeoPoint, } from '../../pages/journeyDetail/JourneyDetailPage.helpers'; import type { JourneyEntry, JourneyTrip } from '../../store/journeyStore'; @@ -94,7 +95,17 @@ export function ProviderPicker({ setSearchTo(to); setSearchPage(page); try { - const data = await memoriesApi.search(provider, { from, to, page, size: 50 }, signal); + // The bounds are calendar days as this browser reads them, so the zone they + // are meant in travels with them. Without it the server can only take them + // as UTC days and a reader outside UTC gets somebody else's 24 hours + // (#2336). A single offset leaves a one-hour sliver at the far end of a + // multi-day range that crosses a DST change, and none at all for a single + // day, which is the case this fixes. + const data = await memoriesApi.search( + provider, + { from, to, page, size: 50, utc_offset_minutes: utcOffsetMinutesForDay(from || to) }, + signal, + ); const assets = data.assets || []; setPhotos((prev) => (append ? [...prev, ...assets] : assets)); setHasMore(!!data.hasMore); @@ -162,6 +173,9 @@ export function ProviderPicker({ [photos, contextLocation?.lat, contextLocation?.lng], ); + // Albums come back whole, so they never page. + const morePagesPending = hasMore && !selectedAlbum; + const toggleAsset = (id: string) => { setSelected((prev) => { const next = new Map(prev); @@ -474,82 +488,91 @@ export function ProviderPicker({
- ) : sortedPhotos.length === 0 ? ( -
-

- {filter === 'trip' && !tripRange.from - ? t('journey.trips.noTripsLinkedSettings') - : t('journey.detail.noPhotos')} -

-
) : (
- {groupPhotosByDate(sortedPhotos).map((group) => ( -
- {(!embedded || filter !== 'day') && ( -

- {group.label} -

- )} -
- {group.assets.map((asset: any) => { - const isSelected = selected.has(asset.id); - const alreadyAdded = existingAssetIds.has(asset.id); - return ( - - ); - })} -
+ {sortedPhotos.length === 0 ? ( +
+

+ {filter === 'trip' && !tripRange.from + ? t('journey.trips.noTripsLinkedSettings') + : morePagesPending + ? t('common.loading') + : t('journey.detail.noPhotos')} +

- ))} - {/* Infinite scroll trigger */} - {hasMore && !selectedAlbum && } + ) : ( +
+ {groupPhotosByDate(sortedPhotos).map((group) => ( +
+ {(!embedded || filter !== 'day') && ( +

+ {group.label} +

+ )} +
+ {group.assets.map((asset: any) => { + const isSelected = selected.has(asset.id); + const alreadyAdded = existingAssetIds.has(asset.id); + return ( + + ); + })} +
+
+ ))} +
+ )} + {/* Outside the empty/grid choice on purpose: a page whose photos all + belong to a neighbouring day leaves the grid empty, and a sentinel + that only exists next to photos would never ask for the next page + (#2336). */} + {morePagesPending && }
)}
diff --git a/client/src/pages/journeyDetail/JourneyDetailPage.helpers.test.ts b/client/src/pages/journeyDetail/JourneyDetailPage.helpers.test.ts index f67631427..0242c2ced 100644 --- a/client/src/pages/journeyDetail/JourneyDetailPage.helpers.test.ts +++ b/client/src/pages/journeyDetail/JourneyDetailPage.helpers.test.ts @@ -1,12 +1,35 @@ -import { describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { createDraftJourneyEntry, distanceBetweenGeoPoints, groupPhotosByDate, matchJourneyEntries, sortProviderPhotos, + utcOffsetMinutesForDay, } from './JourneyDetailPage.helpers'; +// Photo days are read off the reader's wall clock now, so the assertions below +// would follow whichever zone the runner happens to sit in. Nothing in the +// vitest config or tests/setup.ts pins one, so this file pins its own. +const runnerTimeZone = process.env.TZ; + +function setTimeZone(tz: string | undefined): void { + if (tz === undefined) delete process.env.TZ; + else process.env.TZ = tz; +} + +function inTimeZone(tz: string, fn: () => T): T { + setTimeZone(tz); + try { + return fn(); + } finally { + setTimeZone('UTC'); + } +} + +beforeAll(() => setTimeZone('UTC')); +afterAll(() => setTimeZone(runnerTimeZone)); + describe('Journey provider photo ranking', () => { it('places GPS photos nearest the selected Journey location and keeps photos without GPS', () => { const photos = [ @@ -66,6 +89,77 @@ describe('Journey provider photo grouping', () => { const photos = [{ id: 'x' }, { id: 'y', takenAt: '2026-03-16T09:00:00Z' }]; expect(groupPhotosByDate(photos).map((g) => g.date)).toEqual(['2026-03-16', '__unknown__']); }); + + it('files a photo under the day the photographer saw, not the UTC one', () => { + // 20:32Z on the 14th is 07:32 on the 15th in Sydney, and Immich says so in + // localTakenAt. It used to land under the 14th (#2336). + const photos = [ + { id: 'morning', takenAt: '2026-03-14T20:32:00Z', localTakenAt: '2026-03-15T07:32:00.000Z' }, + { id: 'evening', takenAt: '2026-03-15T09:00:00Z', localTakenAt: '2026-03-15T20:00:00.000Z' }, + ]; + const groups = groupPhotosByDate(photos); + expect(groups.map((g) => g.date)).toEqual(['2026-03-15']); + expect(groups[0].assets.map((a: { id: string }) => a.id)).toEqual(['morning', 'evening']); + }); + + it('falls back to the local day of the instant when no local stamp is sent', () => { + inTimeZone('Australia/Sydney', () => { + expect(groupPhotosByDate([{ id: 'a', takenAt: '2026-03-14T20:32:00Z' }]).map((g) => g.date)).toEqual([ + '2026-03-15', + ]); + }); + // The same instant in UTC stays on the 14th. + expect(groupPhotosByDate([{ id: 'a', takenAt: '2026-03-14T20:32:00Z' }]).map((g) => g.date)).toEqual([ + '2026-03-14', + ]); + }); + + it('leaves a bare date alone rather than shifting it by a zone it never carried', () => { + inTimeZone('America/New_York', () => { + expect(groupPhotosByDate([{ id: 'a', takenAt: '2026-02-02' }]).map((g) => g.date)).toEqual(['2026-02-02']); + }); + }); + + it('groups an unreadable timestamp the way it always did instead of dropping it', () => { + // Ten characters long, so it is taken for the calendar date it looks like + // and passed through untouched — the branch above. + expect(groupPhotosByDate([{ id: 'a', takenAt: 'not-a-date' }]).map((g) => g.date)).toEqual(['not-a-date']); + // Longer than a date and nothing Date can read: the heading falls back to + // the leading ten characters. Without that fallback every such photo would + // pile up under one heading reading NaN-NaN-NaN. + expect(groupPhotosByDate([{ id: 'b', takenAt: 'unknown-time' }]).map((g) => g.date)).toEqual(['unknown-ti']); + expect(groupPhotosByDate([{ id: 'c', takenAt: '2026-13-45T10:00' }]).map((g) => g.date)).toEqual(['2026-13-45']); + }); +}); + +describe('utcOffsetMinutesForDay', () => { + it('reads the offset of the day being searched, not of today', () => { + inTimeZone('Australia/Sydney', () => { + // Sydney is +11 on 15 March 2026 and +10 in July: a summer day looked up + // in winter would otherwise be an hour off. + expect(utcOffsetMinutesForDay('2026-03-15')).toBe(660); + expect(utcOffsetMinutesForDay('2026-07-15')).toBe(600); + }); + }); + + it('reads the offset where the day starts, which is the bound the server builds from it', () => { + inTimeZone('Europe/Berlin', () => { + // 25 October 2026 opens on summer time and closes on winter time. Read at + // midday it would be +1 and the window would open an hour after the day + // did, dropping a photo taken at 00:30 (#2336). + expect(utcOffsetMinutesForDay('2026-10-25')).toBe(120); + // 29 March is the mirror — summer time arrives at 02:00, so the day still + // starts on +1 however the afternoon reads. + expect(utcOffsetMinutesForDay('2026-03-29')).toBe(60); + }); + }); + + it('falls back to the current offset when there is no usable day', () => { + expect(utcOffsetMinutesForDay('')).toBe(0); + expect(utcOffsetMinutesForDay(undefined)).toBe(0); + expect(utcOffsetMinutesForDay('not-a-day')).toBe(0); + expect(utcOffsetMinutesForDay('2026-13-45')).toBe(0); + }); }); diff --git a/client/src/pages/journeyDetail/JourneyDetailPage.helpers.ts b/client/src/pages/journeyDetail/JourneyDetailPage.helpers.ts index c0c188a1d..c1d6199ab 100644 --- a/client/src/pages/journeyDetail/JourneyDetailPage.helpers.ts +++ b/client/src/pages/journeyDetail/JourneyDetailPage.helpers.ts @@ -1,5 +1,6 @@ import { GeoOnceError } from '../../hooks/useGeolocation'; import type { JourneyEntry } from '../../store/journeyStore'; +import { localIsoDate } from '../../utils/localDate'; import { GRADIENTS } from './JourneyDetailPage.constants'; // Shared by the desktop entry editor and the mobile entry sheet so a failed @@ -72,10 +73,67 @@ export function photoUrl(p: { photo_id: number }, size: 'thumbnail' | 'original' return `/api/photos/${p.photo_id}/${size}`; } +/** + * Which calendar day a provider photo belongs to. + * + * Providers that keep the photographer's wall clock send it along + * (`localTakenAt`, Immich's `localDateTime`) and it is already a date, so it is + * read as one. Everything else only has the capture instant, and the day that + * instant fell on is the reader's local day, never the UTC one: slicing the + * instant files a 07:32 photo in Sydney under the day before (#2336). + * + * A bare date with no time is passed through — it is a calendar date already, + * and putting it through a Date would shift it by a zone it never carried. + */ +export function photoLocalDay(asset: { takenAt?: string | null; localTakenAt?: string | null }): string { + const local = asset.localTakenAt; + if (typeof local === 'string' && local.length >= 10) return local.slice(0, 10); + const taken = asset.takenAt; + if (!taken) return '__unknown__'; + if (taken.length === 10) return taken; + const parsed = new Date(taken); + return Number.isNaN(parsed.getTime()) ? taken.slice(0, 10) : localIsoDate(parsed); +} + +/** + * The reader's own UTC offset where a given calendar day starts, in minutes east + * of UTC. + * + * Read on the day being searched rather than at "now", so a summer day looked up + * in winter is not an hour off, and where that day starts rather than at its + * midday, because the start is what the server derives from it: `from` becomes + * exactly that instant. It needs the offset at all because a date-only bound is + * otherwise a UTC day, which is the wrong 24 hours for everyone outside UTC + * (#2336). + * + * One number cannot describe both ends of a window, and the server closes its + * own at the start of `to` plus twenty-four hours. So a range whose ends sit on + * either side of a daylight-saving change is an hour out at the far end, and the + * twenty-five-hour day a zone gets when its clocks go back loses its last hour + * even when `from` and `to` name that one day. Both are an hour of a listing + * rather than the wrong day, and closing them means giving each bound its own + * offset. + */ +export function utcOffsetMinutesForDay(day?: string): number { + if (!day || !/^\d{4}-\d{2}-\d{2}$/.test(day)) return -new Date().getTimezoneOffset() || 0; + // An offset can only be read at an instant, and the instant this day starts on + // is the thing being looked for. So: read the offset half a day into the UTC + // day, which for every real zone lands inside the local day; use it to step + // back to roughly local midnight; read the offset there. Parsing + // `${day}T00:00:00` as a local time instead would be guesswork on the day a + // zone moves its clocks across midnight, where that wall clock is unreal or + // happens twice. + const utcDayStart = Date.parse(`${day}T00:00:00.000Z`); + const midDay = -new Date(utcDayStart + 43200000).getTimezoneOffset(); + // `|| 0` catches both an unreal date, which gives NaN, and the negative zero + // that negating a zero offset would otherwise hand to the server. + return -new Date(utcDayStart - midDay * 60000).getTimezoneOffset() || 0; +} + export function groupPhotosByDate(photos: any[]): { date: string; label: string; assets: any[] }[] { const map = new Map(); for (const asset of photos) { - const key = asset.takenAt ? asset.takenAt.slice(0, 10) : '__unknown__'; + const key = photoLocalDay(asset); if (!map.has(key)) map.set(key, []); map.get(key)!.push(asset); } @@ -106,6 +164,8 @@ export function groupPhotosByDate(photos: any[]): { date: string; label: string; export interface ProviderPhotoAsset { id: string; takenAt?: string | null; + /** The photographer's wall clock, when the provider knows it. Immich only. */ + localTakenAt?: string | null; lat?: number | null; lng?: number | null; [key: string]: unknown; diff --git a/server/src/nest/memories/immich.service.ts b/server/src/nest/memories/immich.service.ts index 39495f3d3..5c9e54f0f 100644 --- a/server/src/nest/memories/immich.service.ts +++ b/server/src/nest/memories/immich.service.ts @@ -8,10 +8,21 @@ 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, sortAssetsByTakenAtDesc, type Selection } from './memories.helpers'; +import { fail, handleServiceResult, isWithinLocalDayRange, pipeAsset, shiftCalendarDay, sortAssetsByTakenAtDesc, type Selection } from './memories.helpers'; const ALBUM_PAGE_SIZE = 1000; const ALBUM_MAX_PAGES = 20; +/** + * How many upstream pages one search may read while filling one answered page. + * + * The day filter runs after the fetch, so the number of raw pages an answered + * page costs depends on how much of the padding days sits in front of it. This + * is the backstop: a library with thousands of photos on the neighbouring days + * would otherwise keep a single request fetching, so the scan stops here and + * answers `hasMore: false` rather than spinning or handing back the same + * partial page for every page the caller asks for. + */ +const SEARCH_MAX_RAW_PAGES = 20; /** * Immich photo provider: credentials, connection test, timeline/search browsing, @@ -195,6 +206,64 @@ export class ImmichService { } } + /** + * One raw page of /api/search/metadata. + * + * Split out so the page-filling scan below can ask for the next one without + * restating the request body, which carries version-specific compatibility + * that has to stay identical on every page of the same search. + */ + private async fetchSearchPage( + creds: { immich_url: string; immich_api_key: string }, + from: string | undefined, + to: string | undefined, + page: number, + size: number, + ): Promise<{ items?: any[]; status?: number }> { + const resp = await safeFetch(`${creds.immich_url}/api/search/metadata`, { + method: 'POST', + headers: { 'x-api-key': creds.immich_api_key, 'Content-Type': 'application/json' }, + body: JSON.stringify({ + // Padded by a day on each end and narrowed again by the caller. These + // two filter on fileCreatedAt, a UTC instant, while `from`/`to` name + // calendar days on somebody's wall clock — for a UTC+10 reader the + // unpadded window really ran from 10:00 local on the first day to + // 09:59 on the day after the last (#2336). The padding is what makes + // the assets that belong to those days available to the local-date + // filter; nothing wider than a day is needed, since no zone sits + // further than 14 hours from UTC. + takenAfter: from ? `${shiftCalendarDay(from, -1)}T00:00:00.000Z` : undefined, + takenBefore: to ? `${shiftCalendarDay(to, 1)}T23:59:59.999Z` : undefined, + // No type filter — surface videos alongside images (#823). + // Immich 1.133–1.144 defaulted metadata search to `timeline` visibility; + // v3 defaults to any visibility except `locked`, which is what started + // surfacing Live Photo motion parts as broken tiles (#1474). Ask for + // `timeline` explicitly so hidden assets never cross the wire and a full + // page stays a full page of renderable tiles. + // + // `visibility` only exists from 1.133.0. Older servers strip it — Immich + // validates with `whitelist: true` and no `forbidNonWhitelisted`, so an + // unknown property is dropped, never a 400 — and they never defaulted + // `isVisible` either, so they still return hidden assets. this.isVisibleAsset() + // 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, + }), + signal: AbortSignal.timeout(15000) as any, + }); + if (!resp.ok) return { status: resp.status }; + const data = await resp.json() as { assets?: { items?: any[] } }; + return { items: data.assets?.items || [] }; + } + async searchPhotos( userId: number, from?: string, @@ -205,60 +274,67 @@ export class ImmichService { const creds = this.getImmichCredentials(userId); if (!creds) return { error: 'Immich not configured', status: 400 }; + // Once a day filter is in play the raw pages and the answered pages stop + // being the same pages: `order: 'desc'` hands back the padding day first and + // the filter drops all of it, so a raw page can arrive thinned or empty. An + // answered page is therefore filled from as many raw pages as it takes — + // otherwise a day with a busy neighbour costs the picker several round trips + // of nothing, and the MCP tool answers `{ assets: [], hasMore: true }` for a + // day that does have photos (#2336). The scan always restarts at raw page 1 + // and skips the assets that filled the earlier answered pages, which is what + // keeps two calls from handing back the same asset twice. Without bounds + // nothing narrows a page, so raw page and answered page still line up and + // the single round trip stays a single round trip. + const narrowed = Boolean(from || to); + const skip = narrowed ? (page - 1) * size : 0; + const wanted = skip + size; + const maxRawPages = narrowed ? SEARCH_MAX_RAW_PAGES : 1; + + const kept: any[] = []; + let rawPage = narrowed ? 1 : page; + let rawPagesRead = 0; + let windowDrained = false; + try { - const resp = await safeFetch(`${creds.immich_url}/api/search/metadata`, { - method: 'POST', - headers: { 'x-api-key': creds.immich_api_key, 'Content-Type': 'application/json' }, - body: JSON.stringify({ - takenAfter: from ? `${from}T00:00:00.000Z` : undefined, - takenBefore: to ? `${to}T23:59:59.999Z` : undefined, - // No type filter — surface videos alongside images (#823). - // Immich 1.133–1.144 defaulted metadata search to `timeline` visibility; - // v3 defaults to any visibility except `locked`, which is what started - // surfacing Live Photo motion parts as broken tiles (#1474). Ask for - // `timeline` explicitly so hidden assets never cross the wire and a full - // page stays a full page of renderable tiles. - // - // `visibility` only exists from 1.133.0. Older servers strip it — Immich - // validates with `whitelist: true` and no `forbidNonWhitelisted`, so an - // unknown property is dropped, never a 400 — and they never defaulted - // `isVisible` either, so they still return hidden assets. this.isVisibleAsset() - // 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, - }), - signal: AbortSignal.timeout(15000) as any, - }); - if (!resp.ok) return { error: 'Search failed', status: resp.status }; - const data = await resp.json() as { assets?: { items?: any[] } }; - const items = data.assets?.items || []; - // Belt-and-braces: `visibility: 'timeline'` above should mean Immich never - // sends a hidden asset, but an older server that ignores the filter would - // otherwise render broken tiles. hasMore stays on the raw page length so - // pagination still advances past a filtered page. - const assets = items - .filter((a: unknown) => this.isVisibleAsset(a)) - .map((a: any) => ({ - id: a.id, - takenAt: a.fileCreatedAt || a.createdAt, - city: a.exifInfo?.city || null, - country: a.exifInfo?.country || null, - lat: typeof a.exifInfo?.latitude === 'number' ? a.exifInfo.latitude : null, - lng: typeof a.exifInfo?.longitude === 'number' ? a.exifInfo.longitude : null, - mediaType: a.type === 'VIDEO' ? 'video' : 'image', - })); - return { assets: sortAssetsByTakenAtDesc(assets), hasMore: items.length >= size }; + while (kept.length < wanted && rawPagesRead < maxRawPages && !windowDrained) { + const raw = await this.fetchSearchPage(creds, from, to, rawPage, size); + if (!raw.items) return { error: 'Search failed', status: raw.status }; + rawPage++; + rawPagesRead++; + // A short page is the end of the padded window; a full one may have more + // behind it, whatever the filter leaves of this one. + windowDrained = raw.items.length < size; + for (const a of raw.items) { + // Belt-and-braces: `visibility: 'timeline'` above should mean Immich + // never sends a hidden asset, but an older server that ignores the + // filter would otherwise render broken tiles. + if (!this.isVisibleAsset(a)) continue; + const asset = { + id: a.id, + takenAt: a.fileCreatedAt || a.createdAt, + // Immich's own timeline groups by this: the photographer's local time, + // stored without a zone. Absent on servers that predate it, and the + // readers fall back to takenAt, which is the behaviour they had. + localTakenAt: a.localDateTime || null, + city: a.exifInfo?.city || null, + country: a.exifInfo?.country || null, + lat: typeof a.exifInfo?.latitude === 'number' ? a.exifInfo.latitude : null, + lng: typeof a.exifInfo?.longitude === 'number' ? a.exifInfo.longitude : null, + mediaType: a.type === 'VIDEO' ? 'video' : 'image', + }; + if (isWithinLocalDayRange(asset, from, to)) kept.push(asset); + } + } } catch { return { error: 'Could not reach Immich', status: 502 }; } + + // hasMore answers for the padded window rather than for whichever raw page + // was read last, so a page the filter emptied still pages forward. Running + // out of the page budget is the one case that stops: the next call would + // scan the same pages and hand back the same short page for ever. + const hasMore = windowDrained ? kept.length > wanted : kept.length >= wanted || !narrowed; + return { assets: sortAssetsByTakenAtDesc(kept.slice(skip, wanted)), hasMore }; } @@ -528,6 +604,9 @@ export class ImmichService { .map((a: any) => ({ id: a.id, takenAt: a.fileCreatedAt || a.createdAt, + // Same local stamp the search path carries, so an album groups under + // the same day headings a search does. + localTakenAt: a.localDateTime || null, city: a.exifInfo?.city || null, country: a.exifInfo?.country || null, // The search path has always carried these; dropping them here meant a diff --git a/server/src/nest/memories/memories.helpers.ts b/server/src/nest/memories/memories.helpers.ts index e6b0184e0..b6a5a8010 100644 --- a/server/src/nest/memories/memories.helpers.ts +++ b/server/src/nest/memories/memories.helpers.ts @@ -74,6 +74,12 @@ export type AlbumsList = { export type Asset = { id: string; takenAt: string; + /** + * The wall clock the photographer read, timezone-agnostic, when the provider + * knows it. Absent from providers that store instants only, so read it with + * `takenAt` as the fallback. + */ + localTakenAt?: string | null; mediaType?: string; city?: string | null; country?: string | null; @@ -122,6 +128,67 @@ export function sortAssetsByTakenAtDesc(a } +/** + * A calendar day shifted by whole days, as 'YYYY-MM-DD'. + * + * Used to pad a provider window before it is narrowed again by local capture + * date: the widest zones sit 14 hours east and 12 hours west of UTC, so a day on + * anybody's wall clock lies inside the UTC days either side of it, and one day + * of slack on each end provably catches every asset that belongs to it. + * + * Anything that is not a plain calendar day comes back untouched, so a malformed + * bound still reaches upstream exactly as it did before and fails there, rather + * than turning into a different window here. + */ +export function shiftCalendarDay(day: string, deltaDays: number): string { + const parsed = Date.parse(`${day}T00:00:00.000Z`); + if (Number.isNaN(parsed)) return day; + return new Date(parsed + deltaDays * 86400000).toISOString().slice(0, 10); +} + + +/** + * Does this asset belong to the requested calendar days? + * + * Answered against the photographer's own local capture stamp where the provider + * sends one, and otherwise against the capture instant — which is the UTC-day + * reading the window had before, so a provider or a fork without a local stamp + * keeps answering exactly as it does today. Both bounds are optional and + * independent: a one-sided range narrows only the side it was given. + * + * An asset with no usable timestamp at all is kept. Dropping it would lose a + * photo the caller could see before this filter existed. + */ +export function isWithinLocalDayRange( + asset: { takenAt?: string | null; localTakenAt?: string | null }, + from?: string, + to?: string, +): boolean { + const day = (asset.localTakenAt || asset.takenAt || '').slice(0, 10); + if (day.length < 10) return true; + if (from && day < from) return false; + if (to && day > to) return false; + return true; +} + + +/** + * When a calendar day starts, in whole epoch seconds, read in the caller's zone. + * + * `tzOffsetMinutes` is minutes east of UTC (600 for UTC+10), so 0 means the UTC + * day — the only reading available before the caller could say which day it + * meant, and therefore the behaviour a caller that sends nothing keeps. + * + * A bound that is not a plain calendar day falls back to whatever Date makes of + * it, which is what the window did with it before. + */ +export function dayStartEpochSeconds(day: string, tzOffsetMinutes = 0): number { + const dayOnly = Date.parse(`${day}T00:00:00.000Z`); + if (Number.isNaN(dayOnly)) return Math.floor(Date.parse(day) / 1000); + return Math.floor(dayOnly / 1000) - tzOffsetMinutes * 60; +} + + export type AssetInfo = { id: string; takenAt: string | null; diff --git a/server/src/nest/memories/memories.mcp.ts b/server/src/nest/memories/memories.mcp.ts index 4d2725800..e416d6e05 100644 --- a/server/src/nest/memories/memories.mcp.ts +++ b/server/src/nest/memories/memories.mcp.ts @@ -101,13 +101,14 @@ export class MemoriesMcp { to: ISO_DATE.optional().describe('Latest capture date, YYYY-MM-DD, inclusive'), page: z.number().int().min(1).optional().describe('1-based page number, defaults to 1. Page forward while hasMore is true'), size: z.number().int().min(1).max(MAX_PAGE_SIZE).optional().describe(`Photos per page, at most ${MAX_PAGE_SIZE}. Defaults to ${IMMICH_DEFAULT_SIZE} for Immich and ${SYNOLOGY_DEFAULT_LIMIT} for Synology Photos, as the REST routes do`), + utc_offset_minutes: z.number().int().min(-720).max(840).optional().describe('Which zone the dates are meant in, as minutes east of UTC (600 for UTC+10, -480 for UTC-8). Omitted means they name UTC days. Only Synology Photos needs it; Immich answers by the local capture date stored on each photo'), }, annotations: TOOL_ANNOTATIONS_OPEN_WORLD_READONLY, when: anyPhotoProviderEnabled, access: { group: 'journey', mode: 'read' }, }) async searchProviderPhotos( - { provider, from, to, page, size }: { provider: ProviderId; from?: string; to?: string; page?: number; size?: number }, + { provider, from, to, page, size, utc_offset_minutes }: { provider: ProviderId; from?: string; to?: string; page?: number; size?: number; utc_offset_minutes?: number }, ctx: McpContext, ) { const refused = this.providerRefusal(provider); @@ -125,7 +126,9 @@ export class MemoriesMcp { // meaning the same thing for both providers. const limit = size && size > 0 ? size : SYNOLOGY_DEFAULT_LIMIT; const pageIndex = (page ?? 1) - 1; - const result = await this.synology.searchSynologyPhotos(ctx.userId, from, to, pageIndex > 0 ? pageIndex * limit : 0, limit); + // Synology stores instants and knows nothing about the caller's zone, so a + // date-only bound is a UTC day unless the caller says which one it meant. + const result = await this.synology.searchSynologyPhotos(ctx.userId, from, to, pageIndex > 0 ? pageIndex * limit : 0, limit, utc_offset_minutes ?? 0); if ('error' in result) return errorResult(result.error.message); return ok({ provider, assets: result.data.assets, total: result.data.total, hasMore: result.data.hasMore }); } diff --git a/server/src/nest/memories/memories.service.ts b/server/src/nest/memories/memories.service.ts index 432430c61..21b13eab4 100644 --- a/server/src/nest/memories/memories.service.ts +++ b/server/src/nest/memories/memories.service.ts @@ -144,8 +144,8 @@ export class MemoriesService { return this.unified.syncSynologyAlbum(userId, tripId, linkId, sid); } - synologySearchPhotos(userId: number, from: string | undefined, to: string | undefined, offset: number, limit: number) { - return this.synology.searchSynologyPhotos(userId, from, to, offset, limit); + synologySearchPhotos(userId: number, from: string | undefined, to: string | undefined, offset: number, limit: number, tzOffsetMinutes = 0) { + return this.synology.searchSynologyPhotos(userId, from, to, offset, limit, tzOffsetMinutes); } synologyGetAssetInfo(userId: number, photoId: string, ownerId: number, passphrase?: string) { diff --git a/server/src/nest/memories/synology.controller.ts b/server/src/nest/memories/synology.controller.ts index 1f07605c7..707d2ae78 100644 --- a/server/src/nest/memories/synology.controller.ts +++ b/server/src/nest/memories/synology.controller.ts @@ -17,6 +17,17 @@ function _parseNumberBodyField(value: unknown, fallback: number): number { return Number.isFinite(parsed) ? parsed : fallback; } +/** + * Minutes east of UTC, clamped to the range real zones live in (-12:00 to + * +14:00). Anything unreadable is 0, which is the UTC day this route searched + * before the browser could say which day it meant. + */ +function _parseUtcOffsetBodyField(value: unknown): number { + const parsed = Number(value); + if (!Number.isFinite(parsed)) return 0; + return Math.max(-720, Math.min(840, Math.trunc(parsed))); +} + /** * /api/integrations/memories/synologyphotos — Synology Photos connection, * search, albums and asset proxy. @@ -118,10 +129,13 @@ export class SynologyMemoriesController { const page = _parseNumberBodyField(body.page, 1) - 1; let limit = _parseNumberBodyField(body.limit, 100); const size = _parseNumberBodyField(body.size, 0); + // `offset` is rows to skip, `utc_offset_minutes` is which 24 hours from/to + // name. Two different things that a shared name would silently swap (#2336). + const tzOffsetMinutes = _parseUtcOffsetBodyField(body.utc_offset_minutes); if (size > 0) limit = size; if (page > 0) offset = page * limit; - this.handle(res, await this.memories.synologySearchPhotos(user.id, from || undefined, to || undefined, offset, limit)); + this.handle(res, await this.memories.synologySearchPhotos(user.id, from || undefined, to || undefined, offset, limit, tzOffsetMinutes)); } @Get('assets/:tripId/:photoId/:ownerId/info') diff --git a/server/src/nest/memories/synology.service.ts b/server/src/nest/memories/synology.service.ts index 97c40214a..84a3c918e 100644 --- a/server/src/nest/memories/synology.service.ts +++ b/server/src/nest/memories/synology.service.ts @@ -5,6 +5,7 @@ import { safeFetch, SsrfBlockedError, checkSsrf } from '../../utils/ssrfGuard'; import { DatabaseService } from '../database/database.service'; import { MemoriesAccessService } from './memories-access.service'; import { + dayStartEpochSeconds, fail, handleServiceResult, success, @@ -569,7 +570,12 @@ export class SynologyService { return success({ selection, total: allItems.length }); } - async searchSynologyPhotos(userId: number, from?: string, to?: string, offset = 0, limit = 300): Promise> { + /** + * `offset` is the NAS row offset — pagination. `tzOffsetMinutes` is minutes + * east of UTC and says which 24 hours `from`/`to` name; they sit next to each + * other and mean nothing alike. + */ + async searchSynologyPhotos(userId: number, from?: string, to?: string, offset = 0, limit = 300, tzOffsetMinutes = 0): Promise> { const params: ApiCallParams = { api: 'SYNO.Foto.Search.Search', method: 'list_item', @@ -581,11 +587,17 @@ export class SynologyService { }; if (from || to) { + // Synology has no local capture stamp to anchor on the way Immich does, + // so the day has to come from the caller: a bare `new Date('2026-03-15')` + // is UTC midnight by spec, which for a UTC+10 reader searched from + // 10:00 that morning to 09:59 the next (#2336). tzOffsetMinutes 0 is + // that same UTC day, so a caller that says nothing is unaffected. + // The two bounds stay independent — the MCP tool makes both optional. if (from) { - params.start_time = Math.floor(new Date(from).getTime() / 1000); + params.start_time = dayStartEpochSeconds(from, tzOffsetMinutes); } if (to) { - params.end_time = Math.floor(new Date(to).getTime() / 1000) + 86400; //adding it as the next day 86400 seconds in day + params.end_time = dayStartEpochSeconds(to, tzOffsetMinutes) + 86400; //adding it as the next day 86400 seconds in day } } diff --git a/server/tests/e2e/memories.e2e.test.ts b/server/tests/e2e/memories.e2e.test.ts index 6d2422482..818769461 100644 --- a/server/tests/e2e/memories.e2e.test.ts +++ b/server/tests/e2e/memories.e2e.test.ts @@ -373,7 +373,18 @@ describe('Memories e2e (real auth guard + temp SQLite)', () => { expect(res.status).toBe(200); expect(res.body).toEqual({ assets: [], total: 0, hasMore: false }); // page=3 -> (3-1)=2; size=20 -> limit=20; offset = 2 * 20 = 40 - expect(synology.searchSynologyPhotos).toHaveBeenCalledWith(1, undefined, undefined, 40, 20); + expect(synology.searchSynologyPhotos).toHaveBeenCalledWith(1, undefined, undefined, 40, 20, 0); + }); + + it('200 search carries the zone the dates are meant in, separately from the row offset', async () => { + synology.searchSynologyPhotos.mockResolvedValue({ success: true, data: { assets: [], total: 0, hasMore: false } }); + const res = await request(server) + .post(`${SYNO}/search`) + .set('Cookie', sessionCookie(1)) + .send({ from: '2026-03-15', to: '2026-03-15', offset: 5, utc_offset_minutes: 600 }); + expect(res.status).toBe(200); + // Two numbers that mean nothing alike and must not swap places (#2336). + expect(synology.searchSynologyPhotos).toHaveBeenCalledWith(1, '2026-03-15', '2026-03-15', 5, 100, 600); }); it('200 album sync (POST stays 200)', async () => { diff --git a/server/tests/unit/mcp/tools-memories.test.ts b/server/tests/unit/mcp/tools-memories.test.ts index fb77c0ce5..f5bdae4ed 100644 --- a/server/tests/unit/mcp/tools-memories.test.ts +++ b/server/tests/unit/mcp/tools-memories.test.ts @@ -138,7 +138,7 @@ describe('Tool: search_provider_photos', () => { const data = parseToolResult(result) as any; expect(data.assets).toEqual([SYNOLOGY_ASSET]); expect(data.total).toBe(1); - expect(synologySearch).toHaveBeenCalledWith(user.id, undefined, undefined, 40, 20); + expect(synologySearch).toHaveBeenCalledWith(user.id, undefined, undefined, 40, 20, 0); }); }); @@ -146,7 +146,21 @@ describe('Tool: search_provider_photos', () => { const { user } = createUser(testDb); await withHarness(user.id, async (h) => { await h.client.callTool({ name: 'search_provider_photos', arguments: { provider: 'synologyphotos' } }); - expect(synologySearch).toHaveBeenCalledWith(user.id, undefined, undefined, 0, 100); + expect(synologySearch).toHaveBeenCalledWith(user.id, undefined, undefined, 0, 100, 0); + }); + }); + + it('tells Synology which zone the dates are meant in, so a tool call is fixable too', async () => { + const { user } = createUser(testDb); + await withHarness(user.id, async (h) => { + const result = await h.client.callTool({ + name: 'search_provider_photos', + arguments: { provider: 'synologyphotos', from: '2026-03-15', to: '2026-03-15', utc_offset_minutes: 600 }, + }); + expect(result.isError).toBeFalsy(); + // The tool takes calendar days and has no browser to ask, so the caller + // says which 24 hours it means; omitted stays the UTC day (#2336). + expect(synologySearch).toHaveBeenCalledWith(user.id, '2026-03-15', '2026-03-15', 0, 100, 600); }); }); diff --git a/server/tests/unit/nest/immich.service.test.ts b/server/tests/unit/nest/immich.service.test.ts index 0b32f4614..a4b2b8107 100644 --- a/server/tests/unit/nest/immich.service.test.ts +++ b/server/tests/unit/nest/immich.service.test.ts @@ -262,6 +262,24 @@ describe('browseTimeline', () => { }); }); +/** + * Three raw pages of a Sydney library, read newest first: the whole of 16 March + * comes back before 15 March starts, which is what makes an answered page cost + * more than one raw page once the day filter runs (#2336). + */ +const NEXT_DAY = [ + { id: 'next-1', fileCreatedAt: '2026-03-16T01:00:00.000Z', localDateTime: '2026-03-16T12:00:00.000Z' }, + { id: 'next-2', fileCreatedAt: '2026-03-16T00:30:00.000Z', localDateTime: '2026-03-16T11:30:00.000Z' }, +]; +const DAY_HEAD = [ + { id: 'day-1', fileCreatedAt: '2026-03-15T09:00:00.000Z', localDateTime: '2026-03-15T20:00:00.000Z' }, + { id: 'day-2', fileCreatedAt: '2026-03-15T08:00:00.000Z', localDateTime: '2026-03-15T19:00:00.000Z' }, +]; +const DAY_TAIL = [ + { id: 'day-3', fileCreatedAt: '2026-03-15T07:00:00.000Z', localDateTime: '2026-03-15T18:00:00.000Z' }, + { id: 'day-4', fileCreatedAt: '2026-03-15T06:00:00.000Z', localDateTime: '2026-03-15T17:00:00.000Z' }, +]; + describe('searchPhotos', () => { it('IMMICH-027: 400s without credentials and 502s when the server is unreachable', async () => { seedUser(7, null, null); @@ -320,19 +338,137 @@ describe('searchPhotos', () => { expect(result.hasMore).toBe(true); }); - it('IMMICH-032: turns a from/to pair into the upstream date bounds', async () => { + it('IMMICH-032: pads the upstream date bounds by a day on each side', async () => { + // The bounds filter on fileCreatedAt, a UTC instant, while from/to name + // calendar days on a wall clock. No zone sits further than 14 hours from + // UTC, so a day of slack each way reaches every asset that belongs to them; + // the local-date filter below narrows the page again (#2336). safeFetch.mockResolvedValue(upstream({ json: { assets: { items: [] } } })); await svc.searchPhotos(USER, '2026-01-01', '2026-01-31'); const body = JSON.parse((safeFetch.mock.calls[0][1] as { body: string }).body); - expect(body.takenAfter).toBe('2026-01-01T00:00:00.000Z'); - expect(body.takenBefore).toBe('2026-01-31T23:59:59.999Z'); + expect(body.takenAfter).toBe('2025-12-31T00:00:00.000Z'); + expect(body.takenBefore).toBe('2026-02-01T23: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-032c: pads only the bound it was given', async () => { + safeFetch.mockResolvedValue(upstream({ json: { assets: { items: [] } } })); + + await svc.searchPhotos(USER, '2026-01-01'); + const fromOnly = JSON.parse((safeFetch.mock.calls[0][1] as { body: string }).body); + expect(fromOnly.takenAfter).toBe('2025-12-31T00:00:00.000Z'); + expect(fromOnly.takenBefore).toBeUndefined(); + + await svc.searchPhotos(USER, undefined, '2026-01-31'); + const toOnly = JSON.parse((safeFetch.mock.calls[1][1] as { body: string }).body); + expect(toOnly.takenAfter).toBeUndefined(); + expect(toOnly.takenBefore).toBe('2026-02-01T23:59:59.999Z'); + }); + + it('IMMICH-032d: keeps the photos whose OWN local capture date is the day asked for', async () => { + // A Sydney reader asking for the 15th: the 07:32 shot is 20:32Z on the 14th + // and the UTC window used to miss it, while the next morning leaked in. + safeFetch.mockResolvedValue(upstream({ + json: { assets: { items: [ + { id: 'morning', fileCreatedAt: '2026-03-14T20:32:00.000Z', localDateTime: '2026-03-15T07:32:00.000Z' }, + { id: 'evening', fileCreatedAt: '2026-03-15T09:00:00.000Z', localDateTime: '2026-03-15T20:00:00.000Z' }, + { id: 'next-morning', fileCreatedAt: '2026-03-15T21:00:00.000Z', localDateTime: '2026-03-16T08:00:00.000Z' }, + ] } }, + })); + + const result = await svc.searchPhotos(USER, '2026-03-15', '2026-03-15'); + + expect(result.assets!.map(a => a.id)).toEqual(['evening', 'morning']); + expect(result.assets![1]).toMatchObject({ takenAt: '2026-03-14T20:32:00.000Z', localTakenAt: '2026-03-15T07:32:00.000Z' }); + }); + + it('IMMICH-049: a server without localDateTime answers exactly as it did before', async () => { + // An old or forked Immich: the fallback is the capture instant, so the day + // is the UTC day the window has always searched, and the padding buys the + // neighbouring days nothing. + safeFetch.mockResolvedValue(upstream({ + json: { assets: { items: [ + { id: 'day-before', fileCreatedAt: '2026-03-14T23:00:00.000Z' }, + { id: 'in-day', fileCreatedAt: '2026-03-15T09:00:00.000Z' }, + { id: 'day-after', fileCreatedAt: '2026-03-16T01:00:00.000Z' }, + ] } }, + })); + + const result = await svc.searchPhotos(USER, '2026-03-15', '2026-03-15'); + + expect(result.assets!.map(a => a.id)).toEqual(['in-day']); + expect(result.assets![0].localTakenAt).toBeNull(); + }); + + it('IMMICH-050: fills the answered page from as many raw pages as the day filter costs', async () => { + // `order: 'desc'` hands the padding day back first, so the first raw page of + // a day whose neighbour was busy is entirely the wrong day. Answering that + // page empty made the picker load nothing several times over, and made the + // MCP tool report a day that has photos as having none (#2336). + safeFetch + .mockResolvedValueOnce(upstream({ json: { assets: { items: NEXT_DAY } } })) + .mockResolvedValueOnce(upstream({ json: { assets: { items: DAY_HEAD } } })); + + const result = await svc.searchPhotos(USER, '2026-03-15', '2026-03-15', 1, 2); + + expect(result.assets!.map(a => a.id)).toEqual(['day-1', 'day-2']); + expect(result.hasMore).toBe(true); + expect(safeFetch).toHaveBeenCalledTimes(2); + expect(JSON.parse((safeFetch.mock.calls[1][1] as { body: string }).body).page).toBe(2); + }); + + it('IMMICH-050b: pages on without repeating an asset the page before already answered with', async () => { + // The scan restarts at raw page 1 every call, so what it skips has to be + // counted in assets that survived the filter rather than in raw rows. + const page = (items: unknown[]) => upstream({ json: { assets: { items } } }); + safeFetch + .mockResolvedValueOnce(page(NEXT_DAY)) + .mockResolvedValueOnce(page(DAY_HEAD)) + .mockResolvedValueOnce(page(NEXT_DAY)) + .mockResolvedValueOnce(page(DAY_HEAD)) + .mockResolvedValueOnce(page(DAY_TAIL)); + + const first = await svc.searchPhotos(USER, '2026-03-15', '2026-03-15', 1, 2); + const second = await svc.searchPhotos(USER, '2026-03-15', '2026-03-15', 2, 2); + + expect(first.assets!.map(a => a.id)).toEqual(['day-1', 'day-2']); + expect(second.assets!.map(a => a.id)).toEqual(['day-3', 'day-4']); + }); + + it('IMMICH-050c: gives up after the page budget instead of scanning a library forever', async () => { + // Every raw page is the padding day, so the answered page can never fill. + // hasMore has to go false here: the next call would scan the same pages and + // hand back the same empty page for ever. + safeFetch.mockResolvedValue(upstream({ json: { assets: { items: NEXT_DAY } } })); + + const result = await svc.searchPhotos(USER, '2026-03-15', '2026-03-15', 1, 2); + + expect(result.assets).toEqual([]); + expect(result.hasMore).toBe(false); + // SEARCH_MAX_RAW_PAGES. + expect(safeFetch).toHaveBeenCalledTimes(20); + }); + + it('IMMICH-050d: an unfiltered browse still costs one round trip, at the page it was asked for', async () => { + // Nothing narrows those pages, so a raw page and an answered page are the + // same page and the scan must not start over at page 1. + safeFetch.mockResolvedValue(upstream({ + json: { assets: { items: [{ id: 'x', visibility: 'hidden' }, { id: 'y', visibility: 'hidden' }] } }, + })); + + const result = await svc.searchPhotos(USER, undefined, undefined, 3, 2); + + expect(safeFetch).toHaveBeenCalledTimes(1); + expect(JSON.parse((safeFetch.mock.calls[0][1] as { body: string }).body).page).toBe(3); + // Every row was hidden, and pagination still advances past that page. + expect(result.assets).toEqual([]); + expect(result.hasMore).toBe(true); + }); + 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. @@ -416,7 +552,7 @@ describe('getAlbumPhotos', () => { // 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: 'older', fileCreatedAt: '2026-03-01T09:00:00Z', localDateTime: '2026-03-01T18:00:00.000Z', type: 'IMAGE', exifInfo: { latitude: 35.0, longitude: 135.7 } }, { id: 'clip', fileCreatedAt: '2026-03-31T09:00:00Z', type: 'VIDEO' }, ] }, })); @@ -426,6 +562,12 @@ describe('getAlbumPhotos', () => { 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 }); + // The album path carries the same local capture stamp the search path does, + // so a photo picked out of an album lands under the day heading a search + // would have given it instead of under the UTC one (#2336). A server that + // predates the field leaves it null, and the readers fall back to takenAt. + expect(assets[1].localTakenAt).toBe('2026-03-01T18:00:00.000Z'); + expect(assets[0].localTakenAt).toBeNull(); }); }); diff --git a/server/tests/unit/nest/memories.controller.test.ts b/server/tests/unit/nest/memories.controller.test.ts index a21933b12..9b565161e 100644 --- a/server/tests/unit/nest/memories.controller.test.ts +++ b/server/tests/unit/nest/memories.controller.test.ts @@ -663,14 +663,14 @@ describe('SynologyMemoriesController (parity with /api/integrations/memories/syn const synologySearchPhotos = vi.fn().mockResolvedValue({ success: true, data: { assets: [] } }); const svc = makeService({ synologySearchPhotos }); await new SynologyMemoriesController(svc).search(user, {}, makeRes()); - expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 0, 100); + expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 0, 100, 0); }); it('forwards from/to and uses size as the limit when size > 0', async () => { const synologySearchPhotos = vi.fn().mockResolvedValue({ success: true, data: { assets: [] } }); const svc = makeService({ synologySearchPhotos }); await new SynologyMemoriesController(svc).search(user, { from: '2024-01-01', to: '2024-02-01', size: 30 }, makeRes()); - expect(synologySearchPhotos).toHaveBeenCalledWith(7, '2024-01-01', '2024-02-01', 0, 30); + expect(synologySearchPhotos).toHaveBeenCalledWith(7, '2024-01-01', '2024-02-01', 0, 30, 0); }); it('derives the offset from a 1-based page using the limit', async () => { @@ -678,21 +678,43 @@ describe('SynologyMemoriesController (parity with /api/integrations/memories/syn const svc = makeService({ synologySearchPhotos }); await new SynologyMemoriesController(svc).search(user, { page: 3, limit: 20 }, makeRes()); // page-1 = 2, offset = 2 * 20 = 40 - expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 40, 20); + expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 40, 20, 0); }); it('keeps the explicit offset when page resolves to <= 0', async () => { const synologySearchPhotos = vi.fn().mockResolvedValue({ success: true, data: { assets: [] } }); const svc = makeService({ synologySearchPhotos }); await new SynologyMemoriesController(svc).search(user, { page: 1, offset: 5, limit: 10 }, makeRes()); - expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 5, 10); + expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 5, 10, 0); }); it('falls back to defaults when numeric fields are non-finite', async () => { const synologySearchPhotos = vi.fn().mockResolvedValue({ success: true, data: { assets: [] } }); const svc = makeService({ synologySearchPhotos }); - await new SynologyMemoriesController(svc).search(user, { offset: 'x', limit: 'y', page: 'z', size: 'q' }, makeRes()); - expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 0, 100); + await new SynologyMemoriesController(svc).search(user, { offset: 'x', limit: 'y', page: 'z', size: 'q', utc_offset_minutes: 'q' }, makeRes()); + expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 0, 100, 0); + }); + + it('forwards the zone the dates are meant in, apart from the row offset', async () => { + const synologySearchPhotos = vi.fn().mockResolvedValue({ success: true, data: { assets: [] } }); + const svc = makeService({ synologySearchPhotos }); + // A UTC+10 reader paging: 600 is the zone, 40 is the row offset. Sharing + // one name would have made the NAS skip 600 photos instead (#2336). + await new SynologyMemoriesController(svc).search(user, { page: 3, limit: 20, utc_offset_minutes: 600 }, makeRes()); + expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 40, 20, 600); + }); + + it('clamps an impossible zone to the range real ones live in', async () => { + const synologySearchPhotos = vi.fn().mockResolvedValue({ success: true, data: { assets: [] } }); + const svc = makeService({ synologySearchPhotos }); + await new SynologyMemoriesController(svc).search(user, { utc_offset_minutes: 99999 }, makeRes()); + expect(synologySearchPhotos).toHaveBeenCalledWith(7, undefined, undefined, 0, 100, 840); + + await new SynologyMemoriesController(svc).search(user, { utc_offset_minutes: '-99999' }, makeRes()); + expect(synologySearchPhotos).toHaveBeenLastCalledWith(7, undefined, undefined, 0, 100, -720); + + await new SynologyMemoriesController(svc).search(user, { utc_offset_minutes: 90.7 }, makeRes()); + expect(synologySearchPhotos).toHaveBeenLastCalledWith(7, undefined, undefined, 0, 100, 90); }); }); diff --git a/server/tests/unit/nest/memories.helpers.test.ts b/server/tests/unit/nest/memories.helpers.test.ts index 8906157f5..8f6fc32c8 100644 --- a/server/tests/unit/nest/memories.helpers.test.ts +++ b/server/tests/unit/nest/memories.helpers.test.ts @@ -8,7 +8,12 @@ * inside either provider suite. */ import { describe, it, expect } from 'vitest'; -import { sortAssetsByTakenAtDesc } from '../../../src/nest/memories/memories.helpers'; +import { + dayStartEpochSeconds, + isWithinLocalDayRange, + shiftCalendarDay, + sortAssetsByTakenAtDesc, +} from '../../../src/nest/memories/memories.helpers'; const asset = (id: string, takenAt?: string | null) => ({ id, takenAt }); @@ -78,3 +83,69 @@ describe('sortAssetsByTakenAtDesc', () => { expect(ids(out)).toEqual(['utc', 'offset']); }); }); + + +describe('shiftCalendarDay', () => { + it('MEM-DAY-001: pads a day outwards, crossing month and year ends', () => { + expect(shiftCalendarDay('2026-03-15', -1)).toBe('2026-03-14'); + expect(shiftCalendarDay('2026-03-15', 1)).toBe('2026-03-16'); + expect(shiftCalendarDay('2026-03-01', -1)).toBe('2026-02-28'); + expect(shiftCalendarDay('2026-12-31', 1)).toBe('2027-01-01'); + }); + + it('MEM-DAY-002: hands a bound it cannot read straight back, so it fails upstream as before', () => { + expect(shiftCalendarDay('not-a-day', -1)).toBe('not-a-day'); + expect(shiftCalendarDay('', 1)).toBe(''); + }); +}); + + +describe('isWithinLocalDayRange', () => { + const at = (takenAt: string | null, localTakenAt?: string | null) => ({ takenAt, localTakenAt }); + + it('MEM-DAY-010: reads the day off the local capture stamp when the provider sends one', () => { + // 21:00 UTC on the 14th is already the 15th in Sydney, and Immich says so. + expect(isWithinLocalDayRange(at('2026-03-14T21:00:00Z', '2026-03-15T08:00:00.000Z'), '2026-03-15', '2026-03-15')).toBe(true); + // And the mirror: 22:00 UTC on the 15th is already the 16th there, so it + // does not belong to the 15th however the UTC day reads. + expect(isWithinLocalDayRange(at('2026-03-15T22:00:00Z', '2026-03-16T09:00:00.000Z'), '2026-03-15', '2026-03-15')).toBe(false); + }); + + it('MEM-DAY-011: falls back to the capture instant, which is the UTC day it always used', () => { + expect(isWithinLocalDayRange(at('2026-03-15T23:30:00Z'), '2026-03-15', '2026-03-15')).toBe(true); + expect(isWithinLocalDayRange(at('2026-03-16T00:30:00Z'), '2026-03-15', '2026-03-15')).toBe(false); + }); + + it('MEM-DAY-012: narrows only the bound it was given', () => { + expect(isWithinLocalDayRange(at('2026-03-10T09:00:00Z'), '2026-03-15', undefined)).toBe(false); + expect(isWithinLocalDayRange(at('2026-03-20T09:00:00Z'), '2026-03-15', undefined)).toBe(true); + expect(isWithinLocalDayRange(at('2026-03-10T09:00:00Z'), undefined, '2026-03-15')).toBe(true); + expect(isWithinLocalDayRange(at('2026-03-20T09:00:00Z'), undefined, '2026-03-15')).toBe(false); + expect(isWithinLocalDayRange(at('2026-03-20T09:00:00Z'))).toBe(true); + }); + + it('MEM-DAY-013: keeps an asset with no usable timestamp rather than losing it', () => { + expect(isWithinLocalDayRange(at(null), '2026-03-15', '2026-03-15')).toBe(true); + expect(isWithinLocalDayRange(at('nope'), '2026-03-15', '2026-03-15')).toBe(true); + }); +}); + + +describe('dayStartEpochSeconds', () => { + it('MEM-DAY-020: with no offset it is the UTC midnight the window always used', () => { + expect(dayStartEpochSeconds('2026-03-15')).toBe(Math.floor(new Date('2026-03-15').getTime() / 1000)); + }); + + it('MEM-DAY-021: an eastern offset starts the day earlier in UTC, a western one later', () => { + const utc = dayStartEpochSeconds('2026-03-15'); + // UTC+10 reaches midnight ten hours before UTC does. + expect(dayStartEpochSeconds('2026-03-15', 600)).toBe(utc - 600 * 60); + expect(dayStartEpochSeconds('2026-03-15', -480)).toBe(utc + 480 * 60); + }); + + it('MEM-DAY-022: a bound that is not a calendar day is parsed as before, offset ignored', () => { + const instant = '2026-03-15T06:00:00Z'; + expect(dayStartEpochSeconds(instant, 600)).toBe(Math.floor(Date.parse(instant) / 1000)); + expect(Number.isNaN(dayStartEpochSeconds('nope'))).toBe(true); + }); +}); diff --git a/server/tests/unit/nest/memories.service.test.ts b/server/tests/unit/nest/memories.service.test.ts index a8201c438..e02ab3c24 100644 --- a/server/tests/unit/nest/memories.service.test.ts +++ b/server/tests/unit/nest/memories.service.test.ts @@ -177,7 +177,12 @@ describe('MemoriesService (delegation wrapper over services/memories/*)', () => expect(unified.syncSynologyAlbum).toHaveBeenCalledWith(7, '5', 'l1', 'sock'); await svc.synologySearchPhotos(7, 'f', 't', 0, 100); - expect(synology.searchSynologyPhotos).toHaveBeenCalledWith(7, 'f', 't', 0, 100); + expect(synology.searchSynologyPhotos).toHaveBeenCalledWith(7, 'f', 't', 0, 100, 0); + + // The zone the days are meant in has to reach the provider, and it is a + // different thing from the pagination offset sitting next to it (#2336). + await svc.synologySearchPhotos(7, 'f', 't', 0, 100, 600); + expect(synology.searchSynologyPhotos).toHaveBeenLastCalledWith(7, 'f', 't', 0, 100, 600); }); it('synology album-photos forwards a passphrase when present and omits it when absent', async () => { diff --git a/server/tests/unit/nest/synology.service.test.ts b/server/tests/unit/nest/synology.service.test.ts index b8d3ed01f..d8733c108 100644 --- a/server/tests/unit/nest/synology.service.test.ts +++ b/server/tests/unit/nest/synology.service.test.ts @@ -387,6 +387,49 @@ describe('the search order', () => { }); +describe('the search window', () => { + const windowOf = async (from?: string, to?: string, tzOffsetMinutes?: number) => { + safeFetch.mockResolvedValue(api({ list: [] })); + await svc.searchSynologyPhotos(USER, from, to, 0, 100, tzOffsetMinutes); + return new URLSearchParams(String((safeFetch.mock.calls[0][1] as { body: URLSearchParams }).body)); + }; + + it('SYNO-U081: with no offset the day is the UTC day it has always been', async () => { + const body = await windowOf('2026-03-15', '2026-03-15'); + + expect(body.get('start_time')).toBe(String(Date.UTC(2026, 2, 15) / 1000)); + expect(body.get('end_time')).toBe(String(Date.UTC(2026, 2, 15) / 1000 + 86400)); + }); + + it("SYNO-U082: the caller's offset moves both bounds onto the day it meant", async () => { + // UTC+10: the 15th there runs from 14:00Z on the 14th. Without this the NAS + // was asked for 10:00 that morning to 09:59 the next (#2336). + const body = await windowOf('2026-03-15', '2026-03-15', 600); + + expect(body.get('start_time')).toBe(String(Date.UTC(2026, 2, 15) / 1000 - 600 * 60)); + expect(body.get('end_time')).toBe(String(Date.UTC(2026, 2, 15) / 1000 - 600 * 60 + 86400)); + }); + + it('SYNO-U083: a one-sided range still sets only the bound it was given', async () => { + // Both are optional on the MCP tool, so this is reachable. + const fromOnly = await windowOf('2026-03-15', undefined, 600); + expect(fromOnly.get('start_time')).toBe(String(Date.UTC(2026, 2, 15) / 1000 - 600 * 60)); + expect(fromOnly.get('end_time')).toBeNull(); + + vi.clearAllMocks(); + const toOnly = await windowOf(undefined, '2026-03-15', -480); + expect(toOnly.get('start_time')).toBeNull(); + expect(toOnly.get('end_time')).toBe(String(Date.UTC(2026, 2, 15) / 1000 + 480 * 60 + 86400)); + }); + + it('SYNO-U084: no dates at all still means no window, offset or not', async () => { + const body = await windowOf(undefined, undefined, 600); + + expect(body.get('start_time')).toBeNull(); + expect(body.get('end_time')).toBeNull(); + }); +}); + describe('fetchSynologyThumbnailBytes', () => { it('SYNO-U080: fails without credentials rather than fetching', async () => { seedUser(6, { synology_url: null }); diff --git a/shared/src/memories/memories.schema.spec.ts b/shared/src/memories/memories.schema.spec.ts index 75509dcc5..97ba07ba9 100644 --- a/shared/src/memories/memories.schema.spec.ts +++ b/shared/src/memories/memories.schema.spec.ts @@ -1,4 +1,3 @@ -import { describe, it, expect } from 'vitest'; import { addTripPhotosSchema, createAlbumLinkSchema, @@ -10,6 +9,8 @@ import { synologyTestSchema, } from './memories.schema'; +import { describe, it, expect } from 'vitest'; + /** * These contracts exist to describe what the endpoints already accept, not to * tighten them — the nine routes were read as `Record` and @@ -36,6 +37,13 @@ describe('immich contracts', () => { it('rejects a page that is neither', () => { expect(immichSearchSchema.safeParse({ page: { nope: true } }).success).toBe(false); }); + + it('takes the zone the dates are meant in, even though Immich answers by the photo', () => { + // One body goes to whichever provider, so the field has to parse on both. + expect(immichSearchSchema.safeParse({ from: '2026-03-15', utc_offset_minutes: 600 }).success).toBe(true); + expect(immichSearchSchema.safeParse({ utc_offset_minutes: '-480' }).success).toBe(true); + expect(immichSearchSchema.safeParse({}).success).toBe(true); + }); }); describe('synology contracts', () => { @@ -58,6 +66,20 @@ describe('synology contracts', () => { it('accepts every paging field of the search body, in either type', () => { expect(synologySearchSchema.safeParse({ offset: '10', page: 2, limit: '100', size: 0 }).success).toBe(true); }); + + it('keeps the row offset and the UTC offset as two separate fields', () => { + // `offset` is rows to skip on the NAS; `utc_offset_minutes` is which 24 + // hours from/to name. A UTC+10 reader sending the zone as `offset` would + // have asked the NAS to skip 600 photos (#2336). + const parsed = synologySearchSchema.safeParse({ offset: 40, utc_offset_minutes: 600 }); + expect(parsed.success).toBe(true); + expect(parsed.success && parsed.data.offset).toBe(40); + expect(parsed.success && parsed.data.utc_offset_minutes).toBe(600); + }); + + it('leaves the search body valid without a zone, which is the UTC day as before', () => { + expect(synologySearchSchema.safeParse({ from: '2026-03-15', to: '2026-03-15' }).success).toBe(true); + }); }); describe('unified contracts', () => { @@ -87,6 +109,8 @@ describe('unified contracts', () => { it('leaves the album-link fields untyped — the service validates them, and it answers 400 with its own wording', () => { expect(createAlbumLinkSchema.safeParse({}).success).toBe(true); - expect(createAlbumLinkSchema.safeParse({ provider: 'synology', album_id: 12, album_name: null }).success).toBe(true); + expect(createAlbumLinkSchema.safeParse({ provider: 'synology', album_id: 12, album_name: null }).success).toBe( + true, + ); }); }); diff --git a/shared/src/memories/memories.schema.ts b/shared/src/memories/memories.schema.ts index b1ae1bfe7..505a7c5d8 100644 --- a/shared/src/memories/memories.schema.ts +++ b/shared/src/memories/memories.schema.ts @@ -26,6 +26,18 @@ const looseBoolean = z.union([z.boolean(), z.literal('true'), z.literal('false') /** `Number(x) || fallback` — the client sends these as strings from query-ish forms. */ const looseNumber = z.union([z.number(), z.string()]).optional(); +/** + * The UTC offset the caller means its calendar days in, in minutes east of UTC + * (600 for UTC+10, -480 for UTC-8). A date-only `from`/`to` names a day on + * somebody's wall clock; without this the server can only read it as a UTC day, + * which is the wrong 24 hours for everyone outside UTC. + * + * Deliberately not called `offset`: that name is taken on the Synology body and + * means the NAS pagination offset. One body shape goes to whichever provider, so + * reusing the name would make a UTC+10 user ask the NAS to skip 600 photos. + */ +const utcOffsetMinutes = looseNumber; + // ── Immich ──────────────────────────────────────────────────────────────── export const immichSettingsSchema = z.looseObject({ @@ -46,6 +58,10 @@ export const immichSearchSchema = z.looseObject({ to: optionalText, size: looseNumber, page: looseNumber, + // Accepted so the client can send one body to either provider. The Immich + // route ignores it: Immich returns each photo's own local capture stamp, so + // the day is answered from the photo rather than from the reader's zone. + utc_offset_minutes: utcOffsetMinutes, }); // ── Synology ────────────────────────────────────────────────────────────── @@ -64,10 +80,12 @@ export const synologyTestSchema = synologySettingsSchema.extend({ export const synologySearchSchema = z.looseObject({ from: optionalText, to: optionalText, + // Rows to skip on the NAS — pagination, nothing to do with time zones. offset: looseNumber, page: looseNumber, limit: looseNumber, size: looseNumber, + utc_offset_minutes: utcOffsetMinutes, }); // ── Unified (provider-agnostic trip photo surface) ────────────────────────