mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-22 06:56:35 +00:00
fix(journey): select all takes every photo in the range and imports keep their order
The photo picker's Select all took only the pages already loaded, 50 at a time, and replaced whatever had been picked on another tab. It now loads the rest of a day, trip or date-range search before selecting (cancelled by closing the picker or switching tab), adds to earlier picks, and Deselect all only clears what is in view. Add stays disabled until that loading is done. The whole-library tab keeps selecting what is loaded and shows the count as "N+". A date-filtered Immich search stopped at 1,000 photos without a word, because its page budget was absolute; it is now relative to the page asked for, with an absolute ceiling of 270 raw pages sized so the picker's deepest page still gets a full answer. Adds go out in batches of 500 ids, under the JSON body limit, and each batch is sent oldest first, so photos land in an entry in the order they were taken. When a later batch fails, the photos already saved are counted and the journey reloads while the error still shows, through one hook for desktop and phone. After a provider add, the capture-time lookup runs at most four at a time per user, shared by all of that user's batches. When it fills a row, a journey:photos:updated event reloads open journeys once per batch, from the REST routes and the MCP add tool alike. Device uploads, which arrive one file per request, get the same lookup without the event. Addresses discussion #1587.
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
// FE-APISURF-001 to FE-APISURF-058
|
||||
// FE-APISURF-001 to FE-APISURF-059
|
||||
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
|
||||
import type { AxiosResponse } from 'axios'
|
||||
import { http, HttpResponse } from 'msw'
|
||||
@@ -645,6 +645,41 @@ describe('client > request payloads', () => {
|
||||
.toEqual({ provider: 'immich', asset_ids: ['a1'], caption: 'cap', passphrase: 'secret', media_types: ['image', 'video'] })
|
||||
})
|
||||
|
||||
it('FE-APISURF-059: a provider-photo add past the batch size goes out in batches and answers as one (#1587)', async () => {
|
||||
const bodies: Array<{ url: string; body: Record<string, unknown> }> = []
|
||||
server.use(http.post(/\/provider-photos$/, async ({ request }) => {
|
||||
const body = await request.json() as { asset_ids: string[] }
|
||||
bodies.push({ url: new URL(request.url).pathname, body })
|
||||
return HttpResponse.json({ photos: body.asset_ids.map(id => ({ asset_id: id })), added: body.asset_ids.length })
|
||||
}))
|
||||
const ids = Array.from({ length: 1001 }, (_, i) => `a${i}`)
|
||||
const types = ids.map((_, i) => (i % 3 === 0 ? 'video' : 'image'))
|
||||
|
||||
const entry = await journeyApi.addProviderPhotos(9, 'immich', ids, 'cap', 'pw', types)
|
||||
|
||||
// One request of 2,180 Immich ids already hit the 100 kB body limit.
|
||||
expect(bodies.map(b => (b.body.asset_ids as string[]).length)).toEqual([500, 500, 1])
|
||||
for (const { url, body } of bodies) {
|
||||
expect(url).toBe('/api/journeys/entries/9/provider-photos')
|
||||
expect(body).toMatchObject({ provider: 'immich', caption: 'cap', passphrase: 'pw' })
|
||||
// Each id keeps its own media type across the cut.
|
||||
const sent = body.asset_ids as string[]
|
||||
expect(body.media_types).toEqual(sent.map(id => types[Number(id.slice(1))]))
|
||||
}
|
||||
expect(bodies.flatMap(b => b.body.asset_ids as string[])).toEqual(ids)
|
||||
expect(entry.added).toBe(1001)
|
||||
expect(entry.photos).toHaveLength(1001)
|
||||
|
||||
bodies.length = 0
|
||||
const gallery = await journeyApi.addProviderPhotosToGallery(2, 'immich', ids.slice(0, 501))
|
||||
expect(bodies.map(b => b.url)).toEqual(['/api/journeys/2/gallery/provider-photos', '/api/journeys/2/gallery/provider-photos'])
|
||||
expect(bodies.map(b => b.body)).toEqual([
|
||||
{ provider: 'immich', asset_ids: ids.slice(0, 500) },
|
||||
{ provider: 'immich', asset_ids: ids.slice(500, 501) },
|
||||
])
|
||||
expect(gallery.added).toBe(501)
|
||||
})
|
||||
|
||||
it('FE-APISURF-030: adminApi.pluginActivate only sends consent when granted', async () => {
|
||||
expect((await traceOne(() => adminApi.pluginActivate('koffi'))).body).toEqual({})
|
||||
expect((await traceOne(() => adminApi.pluginActivate('koffi', true))).body).toEqual({ consent: true })
|
||||
|
||||
@@ -4,6 +4,7 @@ import type { z } from 'zod'
|
||||
import type { Day, Place, Trip } from '../types'
|
||||
import type { TransitProvider } from '@trek/shared'
|
||||
import { randomId } from '../utils/randomId'
|
||||
import { postProviderPhotosInBatches } from './providerPhotoBatches'
|
||||
import {
|
||||
weatherResultSchema, type WeatherResult,
|
||||
inAppListResultSchema, type InAppListResult,
|
||||
@@ -1018,9 +1019,13 @@ export const journeyApi = {
|
||||
/** A clip on one entry: the video plus the poster frame the browser grabbed (issue #2341). */
|
||||
uploadEntryVideo: (entryId: number, formData: FormData, opts?: UploadOptions) =>
|
||||
postMultipart(`/journeys/entries/${entryId}/video`, formData, opts),
|
||||
addProviderPhotosToGallery: (journeyId: number, provider: string, assetIds: string[], passphrase?: string, mediaTypes?: string[]) => apiClient.post(`/journeys/${journeyId}/gallery/provider-photos`, { provider, asset_ids: assetIds, ...(passphrase ? { passphrase } : {}), ...(mediaTypes ? { media_types: mediaTypes } : {}) } satisfies JourneyProviderPhotosRequest).then(r => r.data),
|
||||
// Both provider-photo adds go out in batches of PROVIDER_PHOTO_BATCH ids: one
|
||||
// request for a whole trip ran into the 100 kB body limit (#1587).
|
||||
addProviderPhotosToGallery: (journeyId: number, provider: string, assetIds: string[], passphrase?: string, mediaTypes?: string[]) =>
|
||||
postProviderPhotosInBatches(assetIds, mediaTypes, (ids, types) => apiClient.post(`/journeys/${journeyId}/gallery/provider-photos`, { provider, asset_ids: ids, ...(passphrase ? { passphrase } : {}), ...(types ? { media_types: types } : {}) } satisfies JourneyProviderPhotosRequest).then(r => r.data)),
|
||||
addProviderPhoto: (entryId: number, provider: string, assetId: string, caption?: string, passphrase?: string) => apiClient.post(`/journeys/entries/${entryId}/provider-photos`, { provider, asset_id: assetId, caption, ...(passphrase ? { passphrase } : {}) }).then(r => r.data),
|
||||
addProviderPhotos: (entryId: number, provider: string, assetIds: string[], caption?: string, passphrase?: string, mediaTypes?: string[]) => apiClient.post(`/journeys/entries/${entryId}/provider-photos`, { provider, asset_ids: assetIds, caption, ...(passphrase ? { passphrase } : {}), ...(mediaTypes ? { media_types: mediaTypes } : {}) }).then(r => r.data),
|
||||
addProviderPhotos: (entryId: number, provider: string, assetIds: string[], caption?: string, passphrase?: string, mediaTypes?: string[]) =>
|
||||
postProviderPhotosInBatches(assetIds, mediaTypes, (ids, types) => apiClient.post(`/journeys/entries/${entryId}/provider-photos`, { provider, asset_ids: ids, caption, ...(passphrase ? { passphrase } : {}), ...(types ? { media_types: types } : {}) }).then(r => r.data)),
|
||||
linkPhoto: (entryId: number, journeyPhotoId: number) => apiClient.post(`/journeys/entries/${entryId}/link-photo`, { journey_photo_id: journeyPhotoId }).then(r => r.data),
|
||||
unlinkPhoto: (entryId: number, journeyPhotoId: number) => apiClient.delete(`/journeys/entries/${entryId}/photos/${journeyPhotoId}`).then(r => r.data),
|
||||
deleteGalleryPhoto: (journeyId: number, journeyPhotoId: number) => apiClient.delete(`/journeys/${journeyId}/gallery/${journeyPhotoId}`).then(r => r.data),
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { PROVIDER_PHOTO_BATCH, ProviderPhotoBatchError, postProviderPhotosInBatches } from './providerPhotoBatches'
|
||||
|
||||
describe('postProviderPhotosInBatches (#1587)', () => {
|
||||
it('FE-PPBATCH-001: cuts ids and media types at the same positions and sums the answers', async () => {
|
||||
const post = vi.fn(async (ids: string[]) => ({ photos: ids.map(id => ({ id })), added: ids.length - 1 }))
|
||||
const ids = ['a', 'b', 'c', 'd', 'e']
|
||||
const types = ['image', 'video', 'image', 'video', 'image']
|
||||
|
||||
const result = await postProviderPhotosInBatches(ids, types, post, 2)
|
||||
|
||||
expect(post.mock.calls).toEqual([
|
||||
[['a', 'b'], ['image', 'video']],
|
||||
[['c', 'd'], ['image', 'video']],
|
||||
[['e'], ['image']],
|
||||
])
|
||||
expect(result).toEqual({ photos: ids.map(id => ({ id })), added: 2 })
|
||||
})
|
||||
|
||||
it('FE-PPBATCH-002: leaves media types out of every batch when none were given', async () => {
|
||||
const post = vi.fn(async () => ({ added: 1 }))
|
||||
|
||||
await postProviderPhotosInBatches(['a', 'b', 'c'], undefined, post, 2)
|
||||
|
||||
expect(post.mock.calls).toEqual([[['a', 'b'], undefined], [['c'], undefined]])
|
||||
})
|
||||
|
||||
it('FE-PPBATCH-003: an empty selection still makes the one request it always made', async () => {
|
||||
const post = vi.fn(async () => undefined)
|
||||
|
||||
expect(await postProviderPhotosInBatches([], undefined, post)).toEqual({ photos: [], added: 0 })
|
||||
expect(post).toHaveBeenCalledTimes(1)
|
||||
expect(post).toHaveBeenCalledWith([], undefined)
|
||||
})
|
||||
|
||||
it('FE-PPBATCH-004: a failing batch stops the run and rejects, the batches after it are never sent', async () => {
|
||||
const post = vi.fn()
|
||||
.mockResolvedValueOnce({ photos: [], added: 2 })
|
||||
.mockRejectedValueOnce(new Error('413'))
|
||||
const ids = Array.from({ length: 5 }, (_, i) => `id${i}`)
|
||||
|
||||
await expect(postProviderPhotosInBatches(ids, undefined, post, 2)).rejects.toThrow('413')
|
||||
expect(post).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('FE-PPBATCH-006: a later batch that fails still reports what the earlier batches saved', async () => {
|
||||
const cause = new Error('502')
|
||||
const post = vi.fn()
|
||||
.mockResolvedValueOnce({ photos: [{ id: 1 }, { id: 2 }], added: 2 })
|
||||
.mockResolvedValueOnce({ photos: [{ id: 3 }], added: 1 })
|
||||
.mockRejectedValueOnce(cause)
|
||||
const ids = Array.from({ length: 7 }, (_, i) => `id${i}`)
|
||||
|
||||
const err = await postProviderPhotosInBatches(ids, undefined, post, 2).catch((e: unknown) => e)
|
||||
|
||||
expect(err).toBeInstanceOf(ProviderPhotoBatchError)
|
||||
expect(err).toMatchObject({ added: 3, photos: [{ id: 1 }, { id: 2 }, { id: 3 }], message: '502', cause })
|
||||
expect(post).toHaveBeenCalledTimes(3)
|
||||
})
|
||||
|
||||
it('FE-PPBATCH-007: a first batch that fails rejects with the request error itself, nothing having been saved', async () => {
|
||||
const cause = { response: { status: 413 } }
|
||||
const post = vi.fn().mockRejectedValueOnce(cause)
|
||||
|
||||
await expect(postProviderPhotosInBatches(['a', 'b', 'c'], undefined, post, 2)).rejects.toBe(cause)
|
||||
expect(post).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('FE-PPBATCH-008: a cause that is not an Error still gets a message', () => {
|
||||
const err = new ProviderPhotoBatchError({ photos: [], added: 0 }, 'gone')
|
||||
|
||||
expect(err.message).toBe('Adding the photos failed part way')
|
||||
expect(err.name).toBe('ProviderPhotoBatchError')
|
||||
expect(err.cause).toBe('gone')
|
||||
})
|
||||
|
||||
it('FE-PPBATCH-005: batches of 500 by default, far under the 100 kB body limit', async () => {
|
||||
const post = vi.fn(async (ids: string[], _types?: string[]) => ({ added: ids.length }))
|
||||
const ids = Array.from({ length: 1200 }, (_, i) => `00000000-0000-4000-8000-${String(i).padStart(12, '0')}`)
|
||||
|
||||
const result = await postProviderPhotosInBatches(ids, ids.map(() => 'image'), post)
|
||||
|
||||
expect(PROVIDER_PHOTO_BATCH).toBe(500)
|
||||
expect(post.mock.calls.map(([batch]) => batch.length)).toEqual([500, 500, 200])
|
||||
expect(result.added).toBe(1200)
|
||||
const largest = JSON.stringify({ provider: 'immich', asset_ids: post.mock.calls[0][0], media_types: post.mock.calls[0][1] })
|
||||
expect(largest.length).toBeLessThan(100 * 1024)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,84 @@
|
||||
/**
|
||||
* Most provider asset ids one add request carries (#1587).
|
||||
*
|
||||
* The server parses JSON bodies up to 100 kB, and an Immich id with its media
|
||||
* type costs about 47 bytes, so a single request stopped at roughly 2,180
|
||||
* photos with a 413 the moment "Select all" could reach a whole trip. 500 keeps
|
||||
* every request far under that for either provider.
|
||||
*/
|
||||
export const PROVIDER_PHOTO_BATCH = 500
|
||||
|
||||
/** What the two provider-photo add routes answer, summed over every batch. */
|
||||
export interface ProviderPhotosAdded {
|
||||
photos: unknown[]
|
||||
added: number
|
||||
}
|
||||
|
||||
/**
|
||||
* A batched add that failed after earlier batches were stored.
|
||||
*
|
||||
* Those batches are on the journey whatever happens next, so the error carries
|
||||
* what they added: a caller can count them and reload, and still report the
|
||||
* failure for the rest. `cause` is what the failing batch rejected with.
|
||||
*/
|
||||
export class ProviderPhotoBatchError extends Error implements ProviderPhotosAdded {
|
||||
readonly photos: unknown[]
|
||||
readonly added: number
|
||||
readonly cause: unknown
|
||||
|
||||
constructor(saved: ProviderPhotosAdded, cause: unknown) {
|
||||
super(cause instanceof Error ? cause.message : 'Adding the photos failed part way')
|
||||
this.name = 'ProviderPhotoBatchError'
|
||||
this.photos = saved.photos
|
||||
this.added = saved.added
|
||||
this.cause = cause
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* How many photos a failed add stored all the same: the count a
|
||||
* ProviderPhotoBatchError carries, 0 for any other failure.
|
||||
*/
|
||||
export function addedBeforeFailure(err: unknown): number {
|
||||
return err instanceof ProviderPhotoBatchError ? err.added : 0
|
||||
}
|
||||
|
||||
/**
|
||||
* Send a provider-photo add in batches the body limit lets through, and answer
|
||||
* as one request would have.
|
||||
*
|
||||
* `mediaTypes` is parallel to `assetIds` and is cut at the same positions, so
|
||||
* every id keeps its own type. The batches go one after the other: the server
|
||||
* numbers photos in the order it receives them, and the picker sends them in
|
||||
* capture order, which concurrent requests could interleave. A failing batch
|
||||
* stops the run. When it is the first, the request's own error comes through
|
||||
* unchanged; after that it is a ProviderPhotoBatchError carrying what the
|
||||
* earlier batches stored. Adding those again is harmless because the server
|
||||
* skips what is already there.
|
||||
*
|
||||
* An empty list still makes the one request it always made.
|
||||
*/
|
||||
export async function postProviderPhotosInBatches(
|
||||
assetIds: string[],
|
||||
mediaTypes: string[] | undefined,
|
||||
post: (assetIds: string[], mediaTypes: string[] | undefined) => Promise<Partial<ProviderPhotosAdded> | undefined>,
|
||||
batchSize: number = PROVIDER_PHOTO_BATCH,
|
||||
): Promise<ProviderPhotosAdded> {
|
||||
const photos: unknown[] = []
|
||||
let added = 0
|
||||
let start = 0
|
||||
do {
|
||||
const end = start + batchSize
|
||||
let result: Partial<ProviderPhotosAdded> | undefined
|
||||
try {
|
||||
result = await post(assetIds.slice(start, end), mediaTypes?.slice(start, end))
|
||||
} catch (err) {
|
||||
if (start === 0) throw err
|
||||
throw new ProviderPhotoBatchError({ photos, added }, err)
|
||||
}
|
||||
photos.push(...(result?.photos ?? []))
|
||||
added += result?.added ?? 0
|
||||
start = end
|
||||
} while (start < assetIds.length)
|
||||
return { photos, added }
|
||||
}
|
||||
@@ -73,6 +73,9 @@ export const HANDLED_OUTSIDE_TRIP_STORE = [
|
||||
'journey:entry:deleted',
|
||||
'journey:entries:reordered',
|
||||
'journey:contributor:changed',
|
||||
// Capture times landed after a photo import (#1587); the same listener reloads
|
||||
// the journey so the gallery re-sorts.
|
||||
'journey:photos:updated',
|
||||
// Studio book — components/Studio/useBookStore (its own listener: a client
|
||||
// with nothing outstanding takes the new version, one with unsaved edits
|
||||
// deliberately does not and conflicts on its next save instead)
|
||||
|
||||
Reference in New Issue
Block a user