From b124a65d6e32d9501b9ccb81d77adf6716629d09 Mon Sep 17 00:00:00 2001 From: patrick Date: Thu, 13 Aug 2026 17:23:57 -0400 Subject: [PATCH] feat: report skipped duplicates in the upload tray A book whose files were all already stored settles as skipped rather than done, naming the book that holds them and offering to add it anyway. --- frontend/AGENTS.md | 10 +- frontend/src/lib/api/book.remote.ts | 56 +++++++---- .../forms/edit-book/edit-files.svelte | 19 +++- .../lib/components/layout/upload-tray.svelte | 84 +++++++++++++++-- frontend/src/lib/schema/book.ts | 9 ++ frontend/src/lib/schema/openapi/schema.d.ts | 92 +++++++++++++++++-- frontend/src/lib/state/upload-queue.svelte.ts | 90 ++++++++++++++---- 7 files changed, 302 insertions(+), 58 deletions(-) diff --git a/frontend/AGENTS.md b/frontend/AGENTS.md index caeb146..14a2897 100644 --- a/frontend/AGENTS.md +++ b/frontend/AGENTS.md @@ -157,10 +157,12 @@ but take a baseline first, because neither is clean (see below). Observed in the current tree — don't mistake these for intentional patterns to copy: -- `src/lib/schema/openapi/schema.d.ts` is **stale** — it predates the `BookProgress` rework and has - no `BookProgressRead`, so `book.progress` types as `{}`. It is the source of most of the errors - `pnpm check` reports; regenerating it should clear them. **Baseline as of 2026-08-11: 37 errors, - 1 warning, 11 files.** Get your own baseline before assuming an error is yours. +- `pnpm check` is not clean. **Baseline as of 2026-08-13: 30 errors, 1 warning, 8 files**, most of + them in `src/routes/api/[...path]/+server.ts` (see below). Get your own baseline before assuming + an error is yours. `src/lib/schema/openapi/schema.d.ts` was regenerated on that date and is + current; regenerate it again after any backend API change, with + `pnpm exec openapi-typescript http://localhost:8000/schema/openapi.json -o src/lib/schema/openapi/schema.d.ts` + against a backend running **your** branch — a stale server silently writes a stale file. - `pnpm lint` does not pass either — 131 pre-existing ESLint errors, 35 of them `svelte/no-navigation-without-resolve` on plain `href`s, plus ~59 files Prettier would rewrite (mostly vendored shadcn components). Check the files you touched, not the whole tree. diff --git a/frontend/src/lib/api/book.remote.ts b/frontend/src/lib/api/book.remote.ts index 5095162..fed4dcc 100644 --- a/frontend/src/lib/api/book.remote.ts +++ b/frontend/src/lib/api/book.remote.ts @@ -9,11 +9,27 @@ import { editBookMetadataSchema, updateBookProgressSchema, type Book, + type BooksUploadResult, bookFilesUpload } from '$lib/schema/index'; import { stringCoerce, type PaginatedResponse } from '$lib/schema/common'; import { createQueryParams } from '$lib/utils'; +/** + * The backend's own message for a failed response, rather than its JSON envelope. + * + * A refused duplicate answers 409 with a `detail` worth reading and the offending + * files in `extra`; passing the body through whole puts JSON in front of the reader. + */ +function detailOf(body: string): string { + try { + const parsed = JSON.parse(body); + return typeof parsed?.detail === 'string' ? parsed.detail : body; + } catch { + return body; + } +} + export const getBook = query(stringCoerce, async (id): Promise => { const { locals } = getRequestEvent(); @@ -68,26 +84,29 @@ export const updateBookCover = form(bookCoverUpload, async ({ book_id, file }) = return await response.json(); }); -export const uploadBooks = form(booksUpload, async ({ library_id, files }) => { - const { locals } = getRequestEvent(); +export const uploadBooks = form( + booksUpload, + async ({ library_id, files }): Promise => { + const { locals } = getRequestEvent(); - const formData = new FormData(); - files.forEach((file) => { - formData.append('files', file); - }); + const formData = new FormData(); + files.forEach((file) => { + formData.append('files', file); + }); - const response = await locals.api.postMultipart( - `/books/fromFiles?library_id=${library_id}`, - formData - ); + const response = await locals.api.postMultipart( + `/books/fromFiles?library_id=${library_id}`, + formData + ); - if (!response.ok) { - const message = await response.text(); - error(response.status, message); + if (!response.ok) { + const message = await response.text(); + error(response.status, message); + } + + return await response.json(); } - - return await response.json(); -}); +); export const uploadBookFiles = form(bookFilesUpload, async ({ book_id, files }) => { const { locals } = getRequestEvent(); @@ -100,8 +119,9 @@ export const uploadBookFiles = form(bookFilesUpload, async ({ book_id, files }) const response = await locals.api.postMultipart(`/books/${book_id}/files`, formData); if (!response.ok) { - const message = await response.text(); - error(response.status, message); + // 409 here means the file is already stored under a different book, which is + // something the reader can act on — so the message has to survive the trip. + error(response.status, detailOf(await response.text())); } return await response.json(); diff --git a/frontend/src/lib/components/forms/edit-book/edit-files.svelte b/frontend/src/lib/components/forms/edit-book/edit-files.svelte index 31dbf95..c9a4c1a 100644 --- a/frontend/src/lib/components/forms/edit-book/edit-files.svelte +++ b/frontend/src/lib/components/forms/edit-book/edit-files.svelte @@ -50,6 +50,22 @@ toast.error(`${file.name} was not added`, { description: reason }); }; + /** + * The API's own words, when it has any. + * + * Adding a file the library already holds under another book is refused with a + * 409 naming it — far more use than "failed to add files". SvelteKit hands an + * `error()` back as an HttpError on the client, so the message sits on `body`. + */ + function apiMessage(error: unknown): string | undefined { + if (typeof error !== 'object' || error === null) return undefined; + + const body = (error as { body?: { message?: string } }).body; + if (typeof body?.message === 'string') return body.message; + + return error instanceof Error ? error.message : undefined; + } + function confirmDelete(file: BookFile) { fileToDelete = file; confirmOpen = true; @@ -148,7 +164,8 @@ toast.success('Files added'); } catch (error) { console.error('Failed to add files: ', error); - toast.error('Failed to add files'); + toast.error(apiMessage(error) ?? 'Failed to add files'); + uploadBookFiles.fields.files.set([]); } })} enctype="multipart/form-data" diff --git a/frontend/src/lib/components/layout/upload-tray.svelte b/frontend/src/lib/components/layout/upload-tray.svelte index 8c66404..9a27cc0 100644 --- a/frontend/src/lib/components/layout/upload-tray.svelte +++ b/frontend/src/lib/components/layout/upload-tray.svelte @@ -1,11 +1,13 @@ {#if queue.total > 0} @@ -44,6 +70,9 @@ {:else if queue.failed > 0} + {:else if queue.skipped > 0} + + {/if} {heading} @@ -78,12 +107,51 @@ {#if !queue.collapsed}
    {#each queue.jobs as job (job.id)} + {@const duplicates = job.duplicates ?? []} + {@const target = noteTarget(duplicates)}
  • {job.label} - - {job.error ?? formatFileSize(job.size)} - + + {#if job.error} + + {job.error} + + {:else if duplicates.length > 0} + + {#if target} + + {duplicateNote(duplicates)} + + {:else} + {duplicateNote(duplicates)} + {/if} + + + {#if job.status === 'skipped' && !queue.active} + + {/if} + + {:else} + + {formatFileSize(job.size)} + + {/if} job.status === 'done').length); + readonly skipped = $derived(this.jobs.filter((job) => job.status === 'skipped').length); readonly failed = $derived(this.jobs.filter((job) => job.status === 'failed').length); readonly active = $derived( this.jobs.some((job) => job.status === 'queued' || job.status === 'uploading') ); readonly current = $derived(this.jobs.find((job) => job.status === 'uploading')); - readonly settled = $derived(this.done + this.failed); + readonly settled = $derived(this.done + this.skipped + this.failed); + + /** + * Whether the run left something the reader still has to see. + * + * A skipped book is not a failure, but it is the only place that says the file + * was already here — and the only place to override it from. + */ + readonly needsAttention = $derived( + this.jobs.some((job) => job.status === 'failed' || (job.duplicates?.length ?? 0) > 0) + ); #running = false; #dismissTimer: ReturnType | undefined; @@ -129,20 +149,20 @@ export class UploadQueueState { release() { this.#held = false; - if (!this.active && this.failed === 0 && this.total > 0) this.#scheduleDismiss(); + if (!this.active && !this.needsAttention && this.total > 0) this.#scheduleDismiss(); } /** - * Clears itself only when everything worked. A run with failures stays until - * dismissed — it is the only record of what did not make it in, and the only - * place to retry from. + * Clears itself only when everything went in cleanly. A run with failures or + * skipped files stays until dismissed — it is the only record of what did not + * make it in, and the only place to retry or override from. */ #scheduleDismiss() { clearTimeout(this.#dismissTimer); if (this.#held) return; this.#dismissTimer = setTimeout(() => { - if (!this.active && this.failed === 0) this.jobs = []; + if (!this.active && !this.needsAttention) this.jobs = []; }, DISMISS_AFTER_MS); } @@ -154,6 +174,27 @@ export class UploadQueueState { void this.#run(); } + /** + * Sends a job again, telling the server to store it even though it matched. + * + * The hash samples a small part of the file, so a match is strong evidence + * rather than proof; this is how someone who knows better overrules it. + */ + addAnyway(id: string) { + this.jobs = this.jobs.map((job) => + job.id === id + ? { + ...job, + status: 'queued' as UploadStatus, + force: true, + duplicates: undefined, + error: undefined + } + : job + ); + void this.#run(); + } + async #run(onFinished?: (summary: UploadSummary) => void) { if (this.#running) return; this.#running = true; @@ -169,26 +210,37 @@ export class UploadQueueState { const index = this.jobs.findIndex((job) => job.status === 'queued'); if (index === -1) break; + const job = this.jobs[index]; this.#patch(index, { status: 'uploading' }); try { const body = new FormData(); - for (const file of this.jobs[index].files) body.append('files', file); + for (const file of job.files) body.append('files', file); - const response = await fetch( - `/api/books/fromFiles?library_id=${encodeURIComponent(String(this.jobs[index].libraryId))}`, - { method: 'POST', body } - ); + const query = + `library_id=${encodeURIComponent(String(job.libraryId))}` + + (job.force ? '&allow_duplicates=true' : ''); + + const response = await fetch(`/api/books/fromFiles?${query}`, { + method: 'POST', + body + }); if (!response.ok) throw new Error(`The server returned ${response.status}`); - const result: PaginatedResponse = await response.json(); - const book = result.items[0]; + const result: BooksUploadResult = await response.json(); + const book = result.created[0]; - created += result.total ?? result.items.length; + created += result.created.length; firstBook ??= book; - this.#patch(index, { status: 'done', book }); + // Nothing created means every file in this folder was already here. + // That is not a failure, and it is not something to hide either. + this.#patch(index, { + status: result.created.length === 0 ? 'skipped' : 'done', + book, + duplicates: result.skipped + }); } catch (error) { // One bad book must not take the rest of the queue with it. console.error(`Failed to upload ${this.jobs[index].label}`, error); @@ -205,9 +257,9 @@ export class UploadQueueState { if (created > 0) await invalidate('app:books'); - if (this.failed === 0) this.#scheduleDismiss(); + if (!this.needsAttention) this.#scheduleDismiss(); - onFinished?.({ created, failed: this.failed, firstBook }); + onFinished?.({ created, skipped: this.skipped, failed: this.failed, firstBook }); } #patch(index: number, changes: Partial) {