From 4150b06099f0d11e3d4e4fcd4c94c0d2c6f5d0bf Mon Sep 17 00:00:00 2001 From: KodeStar Date: Fri, 16 Oct 2026 10:51:03 +0100 Subject: [PATCH 1/3] Match review: the best match first, and Match by hand over the review The review's lists page by score (descending, a tie in recorded order), the cursor still an item id, so 2,000 middling matches read best first rather than in the order the workers finished (#141). Match by hand opens the book page's match dialog over the review, which keeps its tab, picks and pages; a book matched there leaves the apply. The book page is a new tab away (#142). --- admin-ui/src/features/book/match-dialog.tsx | 30 ++++- .../features/health/bulk-match-model.test.ts | 20 +++- .../src/features/health/bulk-match-model.ts | 17 ++- .../src/features/health/bulk-match.test.tsx | 67 +++++++++++ admin-ui/src/features/health/bulk-match.tsx | 109 ++++++++++++++++-- admin-ui/src/i18n/locales/de.json | 2 + admin-ui/src/i18n/locales/en.json | 2 + admin-ui/src/i18n/locales/es.json | 2 + admin-ui/src/i18n/locales/fr.json | 2 + admin-ui/src/i18n/locales/it.json | 2 + admin-ui/src/i18n/locales/pt.json | 2 + internal/api/handlers_match_runs.go | 3 +- internal/catalog/matchruns.go | 21 +++- internal/catalog/matchruns_test.go | 57 +++++++++ 14 files changed, 308 insertions(+), 28 deletions(-) diff --git a/admin-ui/src/features/book/match-dialog.tsx b/admin-ui/src/features/book/match-dialog.tsx index 6a189b30..0defc233 100644 --- a/admin-ui/src/features/book/match-dialog.tsx +++ b/admin-ui/src/features/book/match-dialog.tsx @@ -64,15 +64,21 @@ const clip = (s: string, n: number) => (s.length > n ? `${s.slice(0, n - 3)}...` * unticked, and so does the community cover for a book with art of its own. * Covers are fetched by the server (the console's CSP loads no other host's * images). Each opening starts afresh, at the search. + * + * Opened from elsewhere (the bulk match's review), the book may still be loading: + * without a `detail` the dialog holds a placeholder until it arrives. `onMatched` + * hears that something was written to the book. */ export function MatchDialog({ open, onOpenChange, detail, + onMatched, }: { open: boolean; onOpenChange: (open: boolean) => void; - detail: AdminBookDetail; + detail: AdminBookDetail | undefined; + onMatched?: () => void; }) { const { t } = useTranslation(); const [step, setStep] = useState('search'); @@ -93,13 +99,24 @@ export function MatchDialog({ step === 'search' ? t('book.match.description') : t('book.match.compareDescription') } > - {open ? ( + {open && detail ? ( onOpenChange(false)} /> + ) : open ? ( + + {[0, 1, 2].map((i) => ( +
+ ))} + ) : null} @@ -112,11 +129,13 @@ function MatchBody({ detail, step, onStep: setStep, + onMatched, onDone, }: { detail: AdminBookDetail; step: Step; onStep: (step: Step) => void; + onMatched?: () => void; onDone: () => void; }) { const { t } = useTranslation(); @@ -180,8 +199,10 @@ function MatchBody({ coverErr, ); // With nothing taken, stay, so the admin can try again or untick it. - if (ticks.size) onDone(); - else setBusy(false); + if (ticks.size) { + onMatched?.(); + onDone(); + } else setBusy(false); return; } const tookEdits = rows.some((r) => ticks.has(r.field) && r.offered && r.source === 'edited'); @@ -192,6 +213,7 @@ function MatchBody({ : t('book.match.acceptedBody'), type: 'success', }); + onMatched?.(); onDone(); }; diff --git a/admin-ui/src/features/health/bulk-match-model.test.ts b/admin-ui/src/features/health/bulk-match-model.test.ts index 50ac3401..519a01cd 100644 --- a/admin-ui/src/features/health/bulk-match-model.test.ts +++ b/admin-ui/src/features/health/bulk-match-model.test.ts @@ -7,6 +7,7 @@ import { bulkPhase, isChosen, isPickable, + matchedByHand, runFraction, togglePick, } from './bulk-match-model'; @@ -112,8 +113,23 @@ describe('picks', () => { it('counts what an apply takes: pending confident, less left out, plus put in', () => { const r = run({}, { auto: 4, pending: 3 }); expect(applyCount(r, NO_PICKS)).toBe(3); - expect(applyCount(r, { exclude: new Set([1, 2]), include: new Set([9]) })).toBe(2); - expect(applyCount(r, { exclude: new Set([1, 2, 3, 4]), include: new Set() })).toBe(0); + const picks = (exclude: number[], include: number[]) => ({ + exclude: new Set(exclude), + include: new Set(include), + hand: new Set(), + }); + expect(applyCount(r, picks([1, 2], [9]))).toBe(2); + expect(applyCount(r, picks([1, 2, 3, 4], []))).toBe(0); + }); + + it('leaves a book matched by hand out of the apply', () => { + let picks = togglePick(NO_PICKS, doubtful); + picks = matchedByHand(picks, doubtful); + picks = matchedByHand(picks, confident); + expect(isChosen(confident, picks)).toBe(false); + expect(isChosen(doubtful, picks)).toBe(false); + expect([...picks.hand]).toEqual([2, 1]); + expect(applyRequest('fill', picks)).toEqual({ scope: 'fill', exclude: [1], include: [] }); }); }); diff --git a/admin-ui/src/features/health/bulk-match-model.ts b/admin-ui/src/features/health/bulk-match-model.ts index 674457cb..f9b848a6 100644 --- a/admin-ui/src/features/health/bulk-match-model.ts +++ b/admin-ui/src/features/health/bulk-match-model.ts @@ -34,15 +34,17 @@ export function runFraction(run: MatchRun): number | undefined { } /** - * The admin's picks in the review: confident books left out, and books that - * needed review put in. + * The admin's picks in the review: confident books left out, books that needed + * review put in, and books matched by hand from the review (which an apply leaves + * alone, so it doesn't write the run's match over the admin's). */ export interface Picks { exclude: ReadonlySet; include: ReadonlySet; + hand: ReadonlySet; } -export const NO_PICKS: Picks = { exclude: new Set(), include: new Set() }; +export const NO_PICKS: Picks = { exclude: new Set(), include: new Set(), hand: new Set() }; /** Whether an apply takes the item: confident unless left out, a review one only if put in. */ export function isChosen(item: MatchRunItem, picks: Picks): boolean { @@ -68,6 +70,15 @@ export function togglePick(picks: Picks, item: MatchRunItem): Picks { : { ...picks, include: flip(picks.include) }; } +/** Marks an item matched by hand: left out of the apply, and no longer pickable. */ +export function matchedByHand(picks: Picks, item: MatchRunItem): Picks { + const exclude = new Set(picks.exclude); + const include = new Set(picks.include); + if (item.outcome === 'auto') exclude.add(item.id); + include.delete(item.id); + return { exclude, include, hand: new Set(picks.hand).add(item.id) }; +} + /** How many books an apply writes to: the confident ones still pending, less left out, plus put in. */ export function applyCount(run: MatchRun, picks: Picks): number { return Math.max(0, run.counts.pending - picks.exclude.size) + picks.include.size; diff --git a/admin-ui/src/features/health/bulk-match.test.tsx b/admin-ui/src/features/health/bulk-match.test.tsx index fe8ed586..0725ae08 100644 --- a/admin-ui/src/features/health/bulk-match.test.tsx +++ b/admin-ui/src/features/health/bulk-match.test.tsx @@ -4,6 +4,7 @@ import { setToken } from '@/api/token'; import type { MatchRun, MatchRunItem } from '@/api/types'; import { mockFetch, type MockRoute } from '@/test/fetch-mock'; import { issuesSummary } from '@/test/fixtures'; +import { bookDetail } from '@/test/library-fixtures'; import { renderApp } from '@/test/render-app'; import { signedInRoutes } from '@/test/routes'; @@ -301,6 +302,72 @@ describe('bulk matching', () => { await waitFor(() => expect(issuesAsked()).toBeGreaterThan(before)); }); + it('matches a book by hand over the review, which keeps its place and leaves it out', async () => { + const detail = bookDetail(); + const calls = mockFetch( + routes([run()], { + 'GET /admin/libraries/1/book': { body: detail }, + 'PATCH /admin/libraries/1/book': { body: detail }, + 'GET /admin/libraries/1/book/match': { + body: { + candidates: [ + { + work_id: 'the-martian', + title: 'The Martian', + authors: [{ id: 'a1', name: 'Andy Weir' }], + first_published: '2011', + series: [], + web_url: 'https://meta.audiosilo.app/works/the-martian', + recordings: [], + score: 100, + }, + ], + }, + }, + 'POST /admin/meta/covers': { body: { covers: [''] } }, + }), + ); + const user = userEvent.setup(); + renderApp('/health?issue=unmatched'); + await user.click(await screen.findByRole('button', { name: 'Review and apply' })); + const review = await screen.findByRole('dialog', { name: 'Review matches' }); + const row = (await within(review).findByText('B0UK000001 · UK')).closest('li')!; + // The book page is a new tab away, for a closer look without leaving the review. + expect( + within(row).getByRole('link', { name: 'Open the book page for The Martian in a new tab' }), + ).toHaveAttribute('target', '_blank'); + + // Escape closes the match, not the review under it. + await user.click(within(row).getByRole('button', { name: 'Match The Martian by hand' })); + expect( + await screen.findByRole('dialog', { name: 'Match with community metadata' }), + ).toBeInTheDocument(); + await user.keyboard('{Escape}'); + await waitFor(() => + expect(screen.queryByRole('dialog', { name: 'Match with community metadata' })).toBeNull(), + ); + expect(screen.getByRole('dialog', { name: 'Review matches' })).toBeInTheDocument(); + + await user.click(within(row).getByRole('button', { name: 'Match The Martian by hand' })); + const match = await screen.findByRole('dialog', { name: 'Match with community metadata' }); + await within(match).findByRole('radio', { name: /The Martian/ }); + await user.click(within(match).getByRole('button', { name: 'Compare fields' })); + const compare = await screen.findByRole('dialog', { name: 'Compare with community metadata' }); + await user.click(within(compare).getByRole('button', { name: /^Accept \d+ field/ })); + await waitFor(() => + expect(screen.queryByRole('dialog', { name: 'Compare with community metadata' })).toBeNull(), + ); + expect(calls.some((c) => c.method === 'PATCH' && c.path === '/admin/libraries/1/book')).toBe( + true, + ); + + // Back on the review: the book reads as matched by hand, and the apply leaves it out. + const after = (await within(review).findByText('B0UK000001 · UK')).closest('li')!; + expect(within(after).getAllByText('Matched by hand').length).toBeGreaterThan(0); + expect(within(after).queryByRole('checkbox')).toBeNull(); + expect(within(review).getByRole('button', { name: 'Apply to 0 books' })).toBeDisabled(); + }); + it("shows a gone book's path once, as its title", async () => { const gone: MatchRunItem = { ...confident, diff --git a/admin-ui/src/features/health/bulk-match.tsx b/admin-ui/src/features/health/bulk-match.tsx index cb481ee3..6fe4ac03 100644 --- a/admin-ui/src/features/health/bulk-match.tsx +++ b/admin-ui/src/features/health/bulk-match.tsx @@ -2,12 +2,13 @@ import { useEffect, useId, useRef, useState } from 'react'; import { Link } from '@tanstack/react-router'; import { useQueryClient } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; -import { ArrowRight, Check, LoaderCircle, Sparkles } from 'lucide-react'; +import { ArrowRight, Check, ExternalLink, LoaderCircle, Sparkles } from 'lucide-react'; import { api } from '@/api/client'; import { invalidateMatches, keys, matchRunActive, + useAdminBook, useLibraryRoots, useMatchRunItems, useMatchRuns, @@ -37,6 +38,7 @@ import { } from '@/components/ui/dialog'; import { RadioCards } from '@/components/ui/radio-cards'; import { SegmentedControl } from '@/components/ui/segmented-control'; +import { MatchDialog } from '@/features/book/match-dialog'; import { scoreTone } from '@/features/book/match-model'; import { moreSeriesRefs, seriesRef } from '@/features/book/more-series'; import { bookRoute } from '@/lib/book-route'; @@ -60,6 +62,7 @@ import { bulkPhase, isChosen, isPickable, + matchedByHand, runFraction, runProgress, togglePick, @@ -343,6 +346,9 @@ function ReviewDialog({ const [picks, setPicks] = useState(NO_PICKS); const [scope, setScope] = useState('fill'); const [busy, setBusy] = useState(false); + // The book being matched by hand, in a dialog over the review (which keeps its + // tab, picks and loaded pages underneath). + const [byHand, setByHand] = useState(); // Each opening starts afresh (the run may have changed since). const [wasOpen, setWasOpen] = useState(open); if (open !== wasOpen) { @@ -351,8 +357,15 @@ function ReviewDialog({ setTab(firstTab); setPicks(NO_PICKS); setScope('fill'); + setByHand(undefined); } } + // A book matched by hand leaves the apply, and its row reads what the match + // wrote (each item's changes are worked out against the book as it is now). + const onMatchedByHand = (item: MatchRunItem) => { + setPicks((p) => matchedByHand(p, item)); + void qc.invalidateQueries({ queryKey: keys.matchRunItems(run.id, item.outcome) }); + }; const count = applyCount(run, picks); const country = regionName(run.region, lang); const headings: Record = { @@ -446,6 +459,7 @@ function ReviewDialog({ scope={repick ? 'ids' : scope} picks={picks} onToggle={(item) => setPicks((p) => togglePick(p, item))} + onMatchByHand={setByHand} /> @@ -457,6 +471,14 @@ function ReviewDialog({ {t('health.bulkMatch.apply', counted(count, lang))} + {byHand ? ( + setByHand(undefined)} + onMatched={() => onMatchedByHand(byHand)} + /> + ) : null} ) : null} @@ -464,6 +486,40 @@ function ReviewDialog({ ); } +/** + * The book page's match dialog for one of the review's books, over the review: + * it loads the book (fields with provenance) the dialog compares against. + */ +function HandMatch({ + item, + onClose, + onMatched, +}: { + item: MatchRunItem; + onClose: () => void; + onMatched: () => void; +}) { + const { t } = useTranslation(); + const book = useAdminBook(item.library_id, item.path); + const failed = book.isError; + useEffect(() => { + if (failed) { + toastError(t('book.error'), book.error); + onClose(); + } + }, [failed, book.error, onClose, t]); + return ( + { + if (!open) onClose(); + }} + detail={book.data} + onMatched={onMatched} + /> + ); +} + /** One outcome's books, a page at a time. */ function ItemList({ run, @@ -471,12 +527,14 @@ function ItemList({ scope, picks, onToggle, + onMatchByHand, }: { run: MatchRun; outcome: MatchOutcome; scope: MatchScope; picks: Picks; onToggle: (item: MatchRunItem) => void; + onMatchByHand: (item: MatchRunItem) => void; }) { const { t } = useTranslation(); const list = useMatchRunItems(run.id, outcome); @@ -520,7 +578,9 @@ function ItemList({ root={roots[item.library_id]} scope={scope} chosen={isChosen(item, picks)} + byHand={picks.hand.has(item.id)} onToggle={() => onToggle(item)} + onMatchByHand={() => onMatchByHand(item)} /> ))} @@ -544,14 +604,19 @@ function ItemRow({ root, scope, chosen, + byHand, onToggle, + onMatchByHand, }: { item: MatchRunItem; /** The library's folder: the book's whole path is the path's tooltip. */ root: string | undefined; scope: MatchScope; chosen: boolean; + /** Matched by hand from this review: out of the apply. */ + byHand: boolean; onToggle: () => void; + onMatchByHand: () => void; }) { const { t, i18n } = useTranslation(); const lang = i18n.resolvedLanguage ?? 'en'; @@ -598,7 +663,9 @@ function ItemRow({ )} > - {isPickable(item) ? ( + {byHand ? ( + + ) : isPickable(item) ? ( {t('health.bulkMatch.item.gone')} + ) : byHand ? ( + + {t('health.bulkMatch.item.byHand')} + ) : item.applied ? ( {t(`health.bulkMatch.item.${item.applied}`)} @@ -694,16 +765,30 @@ function ItemRow({ ) : null} {item.gone ? null : ( - - {t('health.bulkMatch.matchByHandShort')} -
diff --git a/admin-ui/src/i18n/locales/de.json b/admin-ui/src/i18n/locales/de.json index 23f66d0e..46fd31f7 100644 --- a/admin-ui/src/i18n/locales/de.json +++ b/admin-ui/src/i18n/locales/de.json @@ -1183,8 +1183,10 @@ "health.bulkMatch.item.identifierUnknown": "Die Community kennt die ASIN oder ISBN dieses Buchs nicht", "health.bulkMatch.item.error": "Der Community-Dienst hat für dieses Buch nicht geantwortet", "health.bulkMatch.item.gone": "Nicht mehr in der Bibliothek", + "health.bulkMatch.item.byHand": "Von Hand zugeordnet", "health.bulkMatch.matchByHand": "{{title}} von Hand zuordnen", "health.bulkMatch.matchByHandShort": "Von Hand", + "health.bulkMatch.openBook": "Buchseite von {{title}} in einem neuen Tab öffnen", "health.reason.no_chapters": "Über {{hours}} Stunden ohne Kapitelmarken", "health.reason.detailed_chapters_one": "Hat {{count}} Kapitel; die feineren der Community passen zu diesem Exemplar", "health.reason.detailed_chapters_other": "Hat {{count}} Kapitel; die feineren der Community passen zu diesem Exemplar", diff --git a/admin-ui/src/i18n/locales/en.json b/admin-ui/src/i18n/locales/en.json index 8c8719fc..ae25af09 100644 --- a/admin-ui/src/i18n/locales/en.json +++ b/admin-ui/src/i18n/locales/en.json @@ -1183,8 +1183,10 @@ "health.bulkMatch.item.identifierUnknown": "The community doesn't know this book's ASIN or ISBN", "health.bulkMatch.item.error": "The community service didn't answer for this book", "health.bulkMatch.item.gone": "No longer in the library", + "health.bulkMatch.item.byHand": "Matched by hand", "health.bulkMatch.matchByHand": "Match {{title}} by hand", "health.bulkMatch.matchByHandShort": "Match by hand", + "health.bulkMatch.openBook": "Open the book page for {{title}} in a new tab", "health.reason.no_chapters": "Over {{hours}} hours with no chapter marks", "health.reason.detailed_chapters_one": "Has {{count}} chapter; the community’s finer ones fit this copy", "health.reason.detailed_chapters_other": "Has {{count}} chapters; the community’s finer ones fit this copy", diff --git a/admin-ui/src/i18n/locales/es.json b/admin-ui/src/i18n/locales/es.json index c118b7a7..8525c692 100644 --- a/admin-ui/src/i18n/locales/es.json +++ b/admin-ui/src/i18n/locales/es.json @@ -1266,8 +1266,10 @@ "health.bulkMatch.item.identifierUnknown": "La comunidad no conoce el ASIN o ISBN de este libro", "health.bulkMatch.item.error": "El servicio de la comunidad no respondió para este libro", "health.bulkMatch.item.gone": "Ya no está en la biblioteca", + "health.bulkMatch.item.byHand": "Asociado a mano", "health.bulkMatch.matchByHand": "Asociar {{title}} a mano", "health.bulkMatch.matchByHandShort": "A mano", + "health.bulkMatch.openBook": "Abrir la página de {{title}} en una pestaña nueva", "health.reason.no_chapters": "Más de {{hours}} horas sin marcas de capítulo", "health.reason.detailed_chapters_one": "Tiene {{count}} capítulo; los más finos de la comunidad encajan con esta copia", "health.reason.detailed_chapters_other": "Tiene {{count}} capítulos; los más finos de la comunidad encajan con esta copia", diff --git a/admin-ui/src/i18n/locales/fr.json b/admin-ui/src/i18n/locales/fr.json index 7b08b62a..06f0bd02 100644 --- a/admin-ui/src/i18n/locales/fr.json +++ b/admin-ui/src/i18n/locales/fr.json @@ -1266,8 +1266,10 @@ "health.bulkMatch.item.identifierUnknown": "La communauté ne connaît pas l'ASIN ou l'ISBN de ce livre", "health.bulkMatch.item.error": "Le service communautaire n'a pas répondu pour ce livre", "health.bulkMatch.item.gone": "N'est plus dans la bibliothèque", + "health.bulkMatch.item.byHand": "Associé à la main", "health.bulkMatch.matchByHand": "Associer {{title}} à la main", "health.bulkMatch.matchByHandShort": "À la main", + "health.bulkMatch.openBook": "Ouvrir la page de {{title}} dans un nouvel onglet", "health.reason.no_chapters": "Plus de {{hours}} heures sans marques de chapitre", "health.reason.detailed_chapters_one": "{{count}} chapitre ; ceux, plus fins, de la communauté conviennent à cet exemplaire", "health.reason.detailed_chapters_other": "{{count}} chapitres ; ceux, plus fins, de la communauté conviennent à cet exemplaire", diff --git a/admin-ui/src/i18n/locales/it.json b/admin-ui/src/i18n/locales/it.json index 4eed4e29..1a058e38 100644 --- a/admin-ui/src/i18n/locales/it.json +++ b/admin-ui/src/i18n/locales/it.json @@ -1266,8 +1266,10 @@ "health.bulkMatch.item.identifierUnknown": "La community non conosce l'ASIN o l'ISBN di questo libro", "health.bulkMatch.item.error": "Il servizio della community non ha risposto per questo libro", "health.bulkMatch.item.gone": "Non è più nella libreria", + "health.bulkMatch.item.byHand": "Associato a mano", "health.bulkMatch.matchByHand": "Associa {{title}} a mano", "health.bulkMatch.matchByHandShort": "A mano", + "health.bulkMatch.openBook": "Apri la pagina di {{title}} in una nuova scheda", "health.reason.no_chapters": "Oltre {{hours}} ore senza segni di capitolo", "health.reason.detailed_chapters_one": "Ha {{count}} capitolo; quelli più fini della community sono adatti a questa copia", "health.reason.detailed_chapters_other": "Ha {{count}} capitoli; quelli più fini della community sono adatti a questa copia", diff --git a/admin-ui/src/i18n/locales/pt.json b/admin-ui/src/i18n/locales/pt.json index c735e9ab..c615d72f 100644 --- a/admin-ui/src/i18n/locales/pt.json +++ b/admin-ui/src/i18n/locales/pt.json @@ -1266,8 +1266,10 @@ "health.bulkMatch.item.identifierUnknown": "A comunidade não conhece o ASIN ou ISBN deste livro", "health.bulkMatch.item.error": "O serviço da comunidade não respondeu para este livro", "health.bulkMatch.item.gone": "Já não está na biblioteca", + "health.bulkMatch.item.byHand": "Associado manualmente", "health.bulkMatch.matchByHand": "Associar {{title}} manualmente", "health.bulkMatch.matchByHandShort": "Manualmente", + "health.bulkMatch.openBook": "Abrir a página de {{title}} num novo separador", "health.reason.no_chapters": "Mais de {{hours}} horas sem marcas de capítulo", "health.reason.detailed_chapters_one": "Tem {{count}} capítulo; os mais finos da comunidade ajustam-se a esta cópia", "health.reason.detailed_chapters_other": "Tem {{count}} capítulos; os mais finos da comunidade ajustam-se a esta cópia", diff --git a/internal/api/handlers_match_runs.go b/internal/api/handlers_match_runs.go index aca468d0..677ddeb9 100644 --- a/internal/api/handlers_match_runs.go +++ b/internal/api/handlers_match_runs.go @@ -145,7 +145,8 @@ func (a *API) handleGetMatchRun(w http.ResponseWriter, r *http.Request) { } // handleMatchRunItems serves GET /admin/match-runs/{id}/items: a page of the -// run's books (?outcome=auto|review|none|error, ?after=, ?limit=), each +// run's books, the best match first (?outcome=auto|review|none|error, +// ?after=, ?limit=), each // with the best candidate, the book as it is now and what each scope would change; // next_after reads the next page (0 = the last). func (a *API) handleMatchRunItems(w http.ResponseWriter, r *http.Request) { diff --git a/internal/catalog/matchruns.go b/internal/catalog/matchruns.go index d3cc2410..632aab9b 100644 --- a/internal/catalog/matchruns.go +++ b/internal/catalog/matchruns.go @@ -314,13 +314,22 @@ func scanMatchItem(rows *sql.Rows, it *MatchRunItem) error { const matchItemCols = `id, run_id, library_id, path, outcome, score, runner_up, proposal, applied, detail` -// ListMatchRunItems returns a page of a run's items in id order: of one outcome -// ("" = all), after the item `after`, at most limit, and the id the next page -// reads after (0 when this is the last). +// ListMatchRunItems returns a page of a run's items, the best match first (score +// descending, then id): of one outcome ("" = all), after the item `after`, at most +// limit, and the id the next page reads after (0 when this is the last). The +// cursor stays an item id: the page reads on from that item's score, which never +// changes once recorded, so a review paged 50 at a time sees the whole outcome in +// one order rather than each page sorted on its own. An `after` that isn't one of +// the run's items reads nothing. func (c *Catalog) ListMatchRunItems(ctx context.Context, runID int64, outcome string, after int64, limit int) ([]MatchRunItem, int64, error) { - items, err := queryRows(ctx, c.db, scanMatchItem, `SELECT `+matchItemCols+` FROM match_run_items - WHERE run_id = ? AND (? = '' OR outcome = ?) AND id > ? ORDER BY id LIMIT ?`, - runID, outcome, outcome, after, limit+1) + items, err := queryRows(ctx, c.db, scanMatchItem, `WITH cur AS ( + SELECT score FROM match_run_items WHERE id = ? AND run_id = ?) + SELECT `+matchItemCols+` FROM match_run_items + WHERE run_id = ? AND (? = '' OR outcome = ?) + AND (? = 0 OR score < (SELECT score FROM cur) + OR (score = (SELECT score FROM cur) AND id > ?)) + ORDER BY score DESC, id LIMIT ?`, + after, runID, runID, outcome, outcome, after, after, limit+1) if err != nil { return nil, 0, err } diff --git a/internal/catalog/matchruns_test.go b/internal/catalog/matchruns_test.go index ea1f2c1e..9450f28d 100644 --- a/internal/catalog/matchruns_test.go +++ b/internal/catalog/matchruns_test.go @@ -2,6 +2,7 @@ package catalog import ( "errors" + "fmt" "strings" "testing" ) @@ -43,6 +44,62 @@ func TestRecordMatchItemDeletedLibrary(t *testing.T) { } } +// TestListMatchRunItemsBestFirst: a run's items page best match first (score +// descending, a tie in the order they were recorded) across pages, so a review read +// a page at a time sees one order; an outcome reads only its own items, and an +// `after` from another run reads nothing. +func TestListMatchRunItemsBestFirst(t *testing.T) { + t.Parallel() + c, ctx := newTestCatalog(t) + lib, _ := c.CreateLibrary(ctx, Library{Name: "One", Root: "/tmp/one"}) + run, err := c.StartMatchRun(ctx, MatchRun{Mode: MatchModeMatch, Total: 7}) + if err != nil { + t.Fatal(err) + } + other, err := c.StartMatchRun(ctx, MatchRun{Mode: MatchModeMatch, Total: 1}) + if err != nil { + t.Fatal(err) + } + for i, score := range []int{70, 95, 70, 60, 95, 80} { + item := &MatchRunItem{LibraryID: lib.ID, Path: fmt.Sprintf("A/%d-%d", i, score), Outcome: OutcomeReview, Score: score} + if err := c.RecordMatchItem(ctx, run, item); err != nil { + t.Fatal(err) + } + } + if err := c.RecordMatchItem(ctx, run, &MatchRunItem{LibraryID: lib.ID, Path: "A/none", Outcome: OutcomeNone}); err != nil { + t.Fatal(err) + } + if err := c.RecordMatchItem(ctx, other, &MatchRunItem{LibraryID: lib.ID, Path: "B/other", Outcome: OutcomeReview, Score: 99}); err != nil { + t.Fatal(err) + } + + var got []string + var after int64 + for range 10 { + items, next, err := c.ListMatchRunItems(ctx, run, OutcomeReview, after, 2) + if err != nil { + t.Fatal(err) + } + for _, it := range items { + got = append(got, it.Path) + } + if after = next; after == 0 { + break + } + } + if want := "A/1-95,A/4-95,A/5-80,A/0-70,A/2-70,A/3-60"; strings.Join(got, ",") != want { + t.Errorf("pages = %s, want %s", strings.Join(got, ","), want) + } + + foreign, _, err := c.ListMatchRunItems(ctx, other, OutcomeReview, 0, 10) + if err != nil || len(foreign) != 1 { + t.Fatalf("other run = %d items %v, want its one", len(foreign), err) + } + if items, next, err := c.ListMatchRunItems(ctx, run, OutcomeReview, foreign[0].ID, 10); err != nil || len(items) != 0 || next != 0 { + t.Errorf("after another run's item = %d items next %d %v, want none", len(items), next, err) + } +} + // TestClearCommunityMatches: clearing one library removes its community overrides // and covers and rebuilds its books (the scanned values back, an admin's own edit, // upload and the manager's enrichment kept), drops its match runs and its books' From ccdd1847f569fef90b60f4599f5a5fd7f7078032 Mon Sep 17 00:00:00 2001 From: KodeStar Date: Fri, 16 Oct 2026 11:04:49 +0100 Subject: [PATCH 2/3] Match review: a book matched since the run is the server's to keep The apply never writes over a match run's book that has an ASIN or ISBN now (matched by hand from the review, on its book page, by another admin, or a rescan found one): planFor gives it no changes, the review marks it matched_since and an apply skips it with that detail. The review's own hand-match state goes; the match dialog loads its book while open and says in place when it can't; one row is patched in place instead of every page asked again; the cursor is a row-value comparison. --- CLAUDE.md | 5 + admin-ui/src/api/types.ts | 7 +- admin-ui/src/features/book/book-page.tsx | 7 +- admin-ui/src/features/book/match-dialog.tsx | 83 +++++++++++----- .../features/health/bulk-match-model.test.ts | 19 ++-- .../src/features/health/bulk-match-model.ts | 50 ++++++---- .../src/features/health/bulk-match.test.tsx | 6 +- admin-ui/src/features/health/bulk-match.tsx | 98 +++++++------------ admin-ui/src/i18n/locales/de.json | 2 +- admin-ui/src/i18n/locales/en.json | 2 +- admin-ui/src/i18n/locales/es.json | 2 +- admin-ui/src/i18n/locales/fr.json | 2 +- admin-ui/src/i18n/locales/it.json | 2 +- admin-ui/src/i18n/locales/pt.json | 2 +- internal/api/handlers_match_runs.go | 6 +- internal/catalog/matchruns.go | 11 +-- internal/matchrun/plan.go | 18 +++- internal/matchrun/runner.go | 13 ++- internal/matchrun/runner_test.go | 48 +++++++++ 19 files changed, 238 insertions(+), 145 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 62f1c495..117ca581 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1076,6 +1076,11 @@ future metadata site can attach enrichment without reshaping the schema. The repick records only books with a new ASIN), so a clean library leaves no items. A refresh proposal names the identifier it was found by (`found_by`, "asin:..."/"isbn:..."), and applying it is a no-op for a book that no longer has it (matched again since). + Likewise a match run's book that has an ASIN or ISBN now (`matchedSince`: matched by hand + from the review or anywhere, or a rescan found one) has no changes, reads `matched_since` + in the review and is skipped with detail `matched_since`, under any scope. The review's + items page best match first (`score DESC, id`), the cursor still an item id whose score + the next page reads on from. Each scope's change carries `series` {from, to} (main first, position 0 = none) when it changes the book's series, which the review shows as "old -> new". A retired work slug is a redirect the client follows, so the names written are the merged record's. diff --git a/admin-ui/src/api/types.ts b/admin-ui/src/api/types.ts index a837ea52..d07209f8 100644 --- a/admin-ui/src/api/types.ts +++ b/admin-ui/src/api/types.ts @@ -1994,12 +1994,17 @@ export interface MatchRunItem { proposal: MatchProposal; /** What applying did: '' (not yet) | applied | skipped | failed. */ applied: '' | 'applied' | 'skipped' | 'failed'; - /** Why: book_gone | nothing_to_change | edit_failed | cover_failed | metadata_unavailable | identifier_unknown. */ + /** Why: book_gone | nothing_to_change | matched_since | edit_failed | cover_failed | metadata_unavailable | identifier_unknown. */ detail?: string; /** The book as it is now. */ book: { title: string; author: string }; /** Nothing is indexed at the path any more. */ gone?: boolean; + /** + * A match run's book matched since the run (it has an ASIN or ISBN now: by hand, + * or elsewhere): an apply leaves it alone, and it has no changes. + */ + matched_since?: boolean; /** What each scope would write: fields (in display order) and the cover. */ changes: Partial>; } diff --git a/admin-ui/src/features/book/book-page.tsx b/admin-ui/src/features/book/book-page.tsx index 4a3ca448..87b7f49e 100644 --- a/admin-ui/src/features/book/book-page.tsx +++ b/admin-ui/src/features/book/book-page.tsx @@ -196,7 +196,12 @@ function BookView({ detail, matchOnOpen }: { detail: AdminBookDetail; matchOnOpe onSaved={reset} onRefused={(field, message) => setRefused((r) => ({ ...r, [field]: message }))} /> - + (s.length > n ? `${s.slice(0, n - 3)}...` * Covers are fetched by the server (the console's CSP loads no other host's * images). Each opening starts afresh, at the search. * - * Opened from elsewhere (the bulk match's review), the book may still be loading: - * without a `detail` the dialog holds a placeholder until it arrives. `onMatched` - * hears that something was written to the book. + * It reads the book itself, only while open (on the book page that's the page's + * own query, already loaded); opened elsewhere (the bulk match's review) it waits + * for it, or says why it didn't load. `onMatched` hears that something was + * written to the book. */ export function MatchDialog({ open, onOpenChange, - detail, + libraryId, + path, onMatched, }: { open: boolean; onOpenChange: (open: boolean) => void; - detail: AdminBookDetail | undefined; + libraryId: number; + path: string; onMatched?: () => void; }) { const { t } = useTranslation(); @@ -99,43 +104,70 @@ export function MatchDialog({ step === 'search' ? t('book.match.description') : t('book.match.compareDescription') } > - {open && detail ? ( - onOpenChange(false)} + onDone={() => { + onMatched?.(); + onOpenChange(false); + }} /> - ) : open ? ( - - {[0, 1, 2].map((i) => ( -
- ))} - ) : null} ); } +/** The book the dialog matches, loaded (or why it didn't), then the dialog's body. */ +function MatchLoader({ + libraryId, + path, + ...body +}: { + libraryId: number; + path: string; + step: Step; + onStep: (step: Step) => void; + onDone: () => void; +}) { + const { t } = useTranslation(); + const book = useAdminBook(libraryId, path); + if (book.data) return ; + if (book.isError) { + return ( + + void book.refetch()} + /> + + ); + } + return ( + + {[0, 1, 2].map((i) => ( +
+ ))} + + ); +} + type Step = 'search' | 'compare'; function MatchBody({ detail, step, onStep: setStep, - onMatched, onDone, }: { detail: AdminBookDetail; step: Step; onStep: (step: Step) => void; - onMatched?: () => void; + /** Closes the dialog once something was written to the book. */ onDone: () => void; }) { const { t } = useTranslation(); @@ -199,10 +231,8 @@ function MatchBody({ coverErr, ); // With nothing taken, stay, so the admin can try again or untick it. - if (ticks.size) { - onMatched?.(); - onDone(); - } else setBusy(false); + if (ticks.size) onDone(); + else setBusy(false); return; } const tookEdits = rows.some((r) => ticks.has(r.field) && r.offered && r.source === 'edited'); @@ -213,7 +243,6 @@ function MatchBody({ : t('book.match.acceptedBody'), type: 'success', }); - onMatched?.(); onDone(); }; diff --git a/admin-ui/src/features/health/bulk-match-model.test.ts b/admin-ui/src/features/health/bulk-match-model.test.ts index 519a01cd..53bbfc90 100644 --- a/admin-ui/src/features/health/bulk-match-model.test.ts +++ b/admin-ui/src/features/health/bulk-match-model.test.ts @@ -7,7 +7,7 @@ import { bulkPhase, isChosen, isPickable, - matchedByHand, + leaveOut, runFraction, togglePick, } from './bulk-match-model'; @@ -113,23 +113,16 @@ describe('picks', () => { it('counts what an apply takes: pending confident, less left out, plus put in', () => { const r = run({}, { auto: 4, pending: 3 }); expect(applyCount(r, NO_PICKS)).toBe(3); - const picks = (exclude: number[], include: number[]) => ({ - exclude: new Set(exclude), - include: new Set(include), - hand: new Set(), - }); - expect(applyCount(r, picks([1, 2], [9]))).toBe(2); - expect(applyCount(r, picks([1, 2, 3, 4], []))).toBe(0); + expect(applyCount(r, { exclude: new Set([1, 2]), include: new Set([9]) })).toBe(2); + expect(applyCount(r, { exclude: new Set([1, 2, 3, 4]), include: new Set() })).toBe(0); }); - it('leaves a book matched by hand out of the apply', () => { - let picks = togglePick(NO_PICKS, doubtful); - picks = matchedByHand(picks, doubtful); - picks = matchedByHand(picks, confident); + it('leaves a book matched by hand out of the apply, and never offers one matched since', () => { + const picks = leaveOut(leaveOut(togglePick(NO_PICKS, doubtful), doubtful), confident); expect(isChosen(confident, picks)).toBe(false); expect(isChosen(doubtful, picks)).toBe(false); - expect([...picks.hand]).toEqual([2, 1]); expect(applyRequest('fill', picks)).toEqual({ scope: 'fill', exclude: [1], include: [] }); + expect(isPickable(item({ matched_since: true }))).toBe(false); }); }); diff --git a/admin-ui/src/features/health/bulk-match-model.ts b/admin-ui/src/features/health/bulk-match-model.ts index f9b848a6..d7ceab7b 100644 --- a/admin-ui/src/features/health/bulk-match-model.ts +++ b/admin-ui/src/features/health/bulk-match-model.ts @@ -34,17 +34,22 @@ export function runFraction(run: MatchRun): number | undefined { } /** - * The admin's picks in the review: confident books left out, books that needed - * review put in, and books matched by hand from the review (which an apply leaves - * alone, so it doesn't write the run's match over the admin's). + * The admin's picks in the review: confident books left out, and books that + * needed review put in. */ export interface Picks { exclude: ReadonlySet; include: ReadonlySet; - hand: ReadonlySet; } -export const NO_PICKS: Picks = { exclude: new Set(), include: new Set(), hand: new Set() }; +export const NO_PICKS: Picks = { exclude: new Set(), include: new Set() }; + +const withId = (set: ReadonlySet, id: number) => new Set(set).add(id); +const withoutId = (set: ReadonlySet, id: number) => { + const next = new Set(set); + next.delete(id); + return next; +}; /** Whether an apply takes the item: confident unless left out, a review one only if put in. */ export function isChosen(item: MatchRunItem, picks: Picks): boolean { @@ -52,31 +57,36 @@ export function isChosen(item: MatchRunItem, picks: Picks): boolean { return item.outcome === 'auto' ? !picks.exclude.has(item.id) : picks.include.has(item.id); } -/** Whether the item has a checkbox at all: a candidate, not applied yet, its book still there. */ +/** + * Whether the item has a checkbox at all: a candidate, not applied yet, its book + * still there and not matched since the run (an apply leaves that one alone). + */ export function isPickable(item: MatchRunItem): boolean { - return (item.outcome === 'auto' || item.outcome === 'review') && !item.applied && !item.gone; + return ( + (item.outcome === 'auto' || item.outcome === 'review') && + !item.applied && + !item.gone && + !item.matched_since + ); } /** Flips one item's pick. */ export function togglePick(picks: Picks, item: MatchRunItem): Picks { - const flip = (set: ReadonlySet) => { - const next = new Set(set); - if (next.has(item.id)) next.delete(item.id); - else next.add(item.id); - return next; - }; + const flip = (set: ReadonlySet) => + set.has(item.id) ? withoutId(set, item.id) : withId(set, item.id); return item.outcome === 'auto' ? { ...picks, exclude: flip(picks.exclude) } : { ...picks, include: flip(picks.include) }; } -/** Marks an item matched by hand: left out of the apply, and no longer pickable. */ -export function matchedByHand(picks: Picks, item: MatchRunItem): Picks { - const exclude = new Set(picks.exclude); - const include = new Set(picks.include); - if (item.outcome === 'auto') exclude.add(item.id); - include.delete(item.id); - return { exclude, include, hand: new Set(picks.hand).add(item.id) }; +/** + * Leaves an item out of the apply, as a book matched by hand from the review is + * (the server leaves it alone anyway: this keeps the apply's count true). + */ +export function leaveOut(picks: Picks, item: MatchRunItem): Picks { + return item.outcome === 'auto' + ? { ...picks, exclude: withId(picks.exclude, item.id) } + : { ...picks, include: withoutId(picks.include, item.id) }; } /** How many books an apply writes to: the confident ones still pending, less left out, plus put in. */ diff --git a/admin-ui/src/features/health/bulk-match.test.tsx b/admin-ui/src/features/health/bulk-match.test.tsx index 0725ae08..a8431b93 100644 --- a/admin-ui/src/features/health/bulk-match.test.tsx +++ b/admin-ui/src/features/health/bulk-match.test.tsx @@ -361,9 +361,11 @@ describe('bulk matching', () => { true, ); - // Back on the review: the book reads as matched by hand, and the apply leaves it out. + // Back on the review: the book reads as matched since the run, and the apply leaves it out. const after = (await within(review).findByText('B0UK000001 · UK')).closest('li')!; - expect(within(after).getAllByText('Matched by hand').length).toBeGreaterThan(0); + expect( + within(after).getByText('Matched since this run: an apply leaves it as it is'), + ).toBeInTheDocument(); expect(within(after).queryByRole('checkbox')).toBeNull(); expect(within(review).getByRole('button', { name: 'Apply to 0 books' })).toBeDisabled(); }); diff --git a/admin-ui/src/features/health/bulk-match.tsx b/admin-ui/src/features/health/bulk-match.tsx index 6fe4ac03..73f5e4f2 100644 --- a/admin-ui/src/features/health/bulk-match.tsx +++ b/admin-ui/src/features/health/bulk-match.tsx @@ -1,6 +1,6 @@ import { useEffect, useId, useRef, useState } from 'react'; import { Link } from '@tanstack/react-router'; -import { useQueryClient } from '@tanstack/react-query'; +import { useQueryClient, type InfiniteData } from '@tanstack/react-query'; import { useTranslation } from 'react-i18next'; import { ArrowRight, Check, ExternalLink, LoaderCircle, Sparkles } from 'lucide-react'; import { api } from '@/api/client'; @@ -8,7 +8,6 @@ import { invalidateMatches, keys, matchRunActive, - useAdminBook, useLibraryRoots, useMatchRunItems, useMatchRuns, @@ -18,6 +17,7 @@ import { type MatchOutcome, type MatchRun, type MatchRunItem, + type MatchRunItemPage, type MatchRunMode, type MatchScope, type SeriesRef, @@ -62,7 +62,7 @@ import { bulkPhase, isChosen, isPickable, - matchedByHand, + leaveOut, runFraction, runProgress, togglePick, @@ -360,11 +360,24 @@ function ReviewDialog({ setByHand(undefined); } } - // A book matched by hand leaves the apply, and its row reads what the match - // wrote (each item's changes are worked out against the book as it is now). + // A book matched by hand leaves the apply, and its row reads as the server + // would now show it (matched since the run, nothing to change), without asking + // again for every page the review has loaded. const onMatchedByHand = (item: MatchRunItem) => { - setPicks((p) => matchedByHand(p, item)); - void qc.invalidateQueries({ queryKey: keys.matchRunItems(run.id, item.outcome) }); + setPicks((p) => leaveOut(p, item)); + qc.setQueryData>( + keys.matchRunItems(run.id, item.outcome), + (data) => + data && { + ...data, + pages: data.pages.map((page) => ({ + ...page, + items: (page.items ?? []).map((it) => + it.id === item.id ? { ...it, matched_since: true, changes: {} } : it, + ), + })), + }, + ); }; const count = applyCount(run, picks); const country = regionName(run.region, lang); @@ -472,10 +485,12 @@ function ReviewDialog({ {byHand ? ( - setByHand(undefined)} + open + onOpenChange={() => setByHand(undefined)} + libraryId={byHand.library_id} + path={byHand.path} onMatched={() => onMatchedByHand(byHand)} /> ) : null} @@ -486,40 +501,6 @@ function ReviewDialog({ ); } -/** - * The book page's match dialog for one of the review's books, over the review: - * it loads the book (fields with provenance) the dialog compares against. - */ -function HandMatch({ - item, - onClose, - onMatched, -}: { - item: MatchRunItem; - onClose: () => void; - onMatched: () => void; -}) { - const { t } = useTranslation(); - const book = useAdminBook(item.library_id, item.path); - const failed = book.isError; - useEffect(() => { - if (failed) { - toastError(t('book.error'), book.error); - onClose(); - } - }, [failed, book.error, onClose, t]); - return ( - { - if (!open) onClose(); - }} - detail={book.data} - onMatched={onMatched} - /> - ); -} - /** One outcome's books, a page at a time. */ function ItemList({ run, @@ -578,7 +559,6 @@ function ItemList({ root={roots[item.library_id]} scope={scope} chosen={isChosen(item, picks)} - byHand={picks.hand.has(item.id)} onToggle={() => onToggle(item)} onMatchByHand={() => onMatchByHand(item)} /> @@ -604,7 +584,6 @@ function ItemRow({ root, scope, chosen, - byHand, onToggle, onMatchByHand, }: { @@ -613,8 +592,6 @@ function ItemRow({ root: string | undefined; scope: MatchScope; chosen: boolean; - /** Matched by hand from this review: out of the apply. */ - byHand: boolean; onToggle: () => void; onMatchByHand: () => void; }) { @@ -623,6 +600,10 @@ function ItemRow({ const p = item.proposal; const title = item.book.title || item.path; const change = item.changes[scope]; + // What became of the book: applied, skipped or failed, or matched since the run + // (an apply leaves it alone, and says so). + const status = + item.matched_since || item.detail === 'matched_since' ? 'matchedSince' : item.applied; const changed = change ? [ ...(change.cover ? [t('book.match.cover')] : []), @@ -663,17 +644,18 @@ function ItemRow({ )} > - {byHand ? ( - - ) : isPickable(item) ? ( + {isPickable(item) ? ( - ) : item.applied === 'applied' ? ( - + ) : status === 'applied' || status === 'matchedSince' ? ( + ) : null} @@ -737,13 +719,9 @@ function ItemRow({ {t('health.bulkMatch.item.gone')} - ) : byHand ? ( - - {t('health.bulkMatch.item.byHand')} - - ) : item.applied ? ( + ) : status ? ( - {t(`health.bulkMatch.item.${item.applied}`)} + {t(`health.bulkMatch.item.${status}`)} ) : candidate ? ( @@ -781,7 +759,7 @@ function ItemRow({ {...bookRoute(item.library_id, item.path)} search={{ library: item.library_id, path: item.path }} target="_blank" - rel="noopener" + rel="noopener noreferrer" className={buttonVariants({ variant: 'ghost', size: 'icon-sm' })} aria-label={t('health.bulkMatch.openBook', { title })} aria-describedby={pathLine} diff --git a/admin-ui/src/i18n/locales/de.json b/admin-ui/src/i18n/locales/de.json index 46fd31f7..32d147ed 100644 --- a/admin-ui/src/i18n/locales/de.json +++ b/admin-ui/src/i18n/locales/de.json @@ -1183,7 +1183,7 @@ "health.bulkMatch.item.identifierUnknown": "Die Community kennt die ASIN oder ISBN dieses Buchs nicht", "health.bulkMatch.item.error": "Der Community-Dienst hat für dieses Buch nicht geantwortet", "health.bulkMatch.item.gone": "Nicht mehr in der Bibliothek", - "health.bulkMatch.item.byHand": "Von Hand zugeordnet", + "health.bulkMatch.item.matchedSince": "Seit diesem Lauf zugeordnet: Das Übernehmen lässt es, wie es ist", "health.bulkMatch.matchByHand": "{{title}} von Hand zuordnen", "health.bulkMatch.matchByHandShort": "Von Hand", "health.bulkMatch.openBook": "Buchseite von {{title}} in einem neuen Tab öffnen", diff --git a/admin-ui/src/i18n/locales/en.json b/admin-ui/src/i18n/locales/en.json index ae25af09..dc1d625b 100644 --- a/admin-ui/src/i18n/locales/en.json +++ b/admin-ui/src/i18n/locales/en.json @@ -1183,7 +1183,7 @@ "health.bulkMatch.item.identifierUnknown": "The community doesn't know this book's ASIN or ISBN", "health.bulkMatch.item.error": "The community service didn't answer for this book", "health.bulkMatch.item.gone": "No longer in the library", - "health.bulkMatch.item.byHand": "Matched by hand", + "health.bulkMatch.item.matchedSince": "Matched since this run: an apply leaves it as it is", "health.bulkMatch.matchByHand": "Match {{title}} by hand", "health.bulkMatch.matchByHandShort": "Match by hand", "health.bulkMatch.openBook": "Open the book page for {{title}} in a new tab", diff --git a/admin-ui/src/i18n/locales/es.json b/admin-ui/src/i18n/locales/es.json index 8525c692..d45d5641 100644 --- a/admin-ui/src/i18n/locales/es.json +++ b/admin-ui/src/i18n/locales/es.json @@ -1266,7 +1266,7 @@ "health.bulkMatch.item.identifierUnknown": "La comunidad no conoce el ASIN o ISBN de este libro", "health.bulkMatch.item.error": "El servicio de la comunidad no respondió para este libro", "health.bulkMatch.item.gone": "Ya no está en la biblioteca", - "health.bulkMatch.item.byHand": "Asociado a mano", + "health.bulkMatch.item.matchedSince": "Asociado después de esta búsqueda: aplicar lo deja como está", "health.bulkMatch.matchByHand": "Asociar {{title}} a mano", "health.bulkMatch.matchByHandShort": "A mano", "health.bulkMatch.openBook": "Abrir la página de {{title}} en una pestaña nueva", diff --git a/admin-ui/src/i18n/locales/fr.json b/admin-ui/src/i18n/locales/fr.json index 06f0bd02..8275774f 100644 --- a/admin-ui/src/i18n/locales/fr.json +++ b/admin-ui/src/i18n/locales/fr.json @@ -1266,7 +1266,7 @@ "health.bulkMatch.item.identifierUnknown": "La communauté ne connaît pas l'ASIN ou l'ISBN de ce livre", "health.bulkMatch.item.error": "Le service communautaire n'a pas répondu pour ce livre", "health.bulkMatch.item.gone": "N'est plus dans la bibliothèque", - "health.bulkMatch.item.byHand": "Associé à la main", + "health.bulkMatch.item.matchedSince": "Associé depuis cette recherche : l'application le laisse tel quel", "health.bulkMatch.matchByHand": "Associer {{title}} à la main", "health.bulkMatch.matchByHandShort": "À la main", "health.bulkMatch.openBook": "Ouvrir la page de {{title}} dans un nouvel onglet", diff --git a/admin-ui/src/i18n/locales/it.json b/admin-ui/src/i18n/locales/it.json index 1a058e38..e9b7e530 100644 --- a/admin-ui/src/i18n/locales/it.json +++ b/admin-ui/src/i18n/locales/it.json @@ -1266,7 +1266,7 @@ "health.bulkMatch.item.identifierUnknown": "La community non conosce l'ASIN o l'ISBN di questo libro", "health.bulkMatch.item.error": "Il servizio della community non ha risposto per questo libro", "health.bulkMatch.item.gone": "Non è più nella libreria", - "health.bulkMatch.item.byHand": "Associato a mano", + "health.bulkMatch.item.matchedSince": "Associato dopo questa ricerca: l'applicazione lo lascia com'è", "health.bulkMatch.matchByHand": "Associa {{title}} a mano", "health.bulkMatch.matchByHandShort": "A mano", "health.bulkMatch.openBook": "Apri la pagina di {{title}} in una nuova scheda", diff --git a/admin-ui/src/i18n/locales/pt.json b/admin-ui/src/i18n/locales/pt.json index c615d72f..d0bc8a64 100644 --- a/admin-ui/src/i18n/locales/pt.json +++ b/admin-ui/src/i18n/locales/pt.json @@ -1266,7 +1266,7 @@ "health.bulkMatch.item.identifierUnknown": "A comunidade não conhece o ASIN ou ISBN deste livro", "health.bulkMatch.item.error": "O serviço da comunidade não respondeu para este livro", "health.bulkMatch.item.gone": "Já não está na biblioteca", - "health.bulkMatch.item.byHand": "Associado manualmente", + "health.bulkMatch.item.matchedSince": "Associado depois desta pesquisa: aplicar deixa-o como está", "health.bulkMatch.matchByHand": "Associar {{title}} manualmente", "health.bulkMatch.matchByHandShort": "Manualmente", "health.bulkMatch.openBook": "Abrir a página de {{title}} num novo separador", diff --git a/internal/api/handlers_match_runs.go b/internal/api/handlers_match_runs.go index 677ddeb9..b3738206 100644 --- a/internal/api/handlers_match_runs.go +++ b/internal/api/handlers_match_runs.go @@ -146,9 +146,9 @@ func (a *API) handleGetMatchRun(w http.ResponseWriter, r *http.Request) { // handleMatchRunItems serves GET /admin/match-runs/{id}/items: a page of the // run's books, the best match first (?outcome=auto|review|none|error, -// ?after=, ?limit=), each -// with the best candidate, the book as it is now and what each scope would change; -// next_after reads the next page (0 = the last). +// ?after=, ?limit=), each with the best candidate, the book as it is now +// and what each scope would change; next_after reads the next page (0 = the +// last). func (a *API) handleMatchRunItems(w http.ResponseWriter, r *http.Request) { if a.metadataOff(w) { return diff --git a/internal/catalog/matchruns.go b/internal/catalog/matchruns.go index 632aab9b..9fd85e76 100644 --- a/internal/catalog/matchruns.go +++ b/internal/catalog/matchruns.go @@ -320,16 +320,13 @@ const matchItemCols = `id, run_id, library_id, path, outcome, score, runner_up, // cursor stays an item id: the page reads on from that item's score, which never // changes once recorded, so a review paged 50 at a time sees the whole outcome in // one order rather than each page sorted on its own. An `after` that isn't one of -// the run's items reads nothing. +// the run's items reads nothing (the row it compares with is all NULL). func (c *Catalog) ListMatchRunItems(ctx context.Context, runID int64, outcome string, after int64, limit int) ([]MatchRunItem, int64, error) { - items, err := queryRows(ctx, c.db, scanMatchItem, `WITH cur AS ( - SELECT score FROM match_run_items WHERE id = ? AND run_id = ?) - SELECT `+matchItemCols+` FROM match_run_items + items, err := queryRows(ctx, c.db, scanMatchItem, `SELECT `+matchItemCols+` FROM match_run_items WHERE run_id = ? AND (? = '' OR outcome = ?) - AND (? = 0 OR score < (SELECT score FROM cur) - OR (score = (SELECT score FROM cur) AND id > ?)) + AND (? = 0 OR (score, -id) < (SELECT score, -id FROM match_run_items WHERE id = ? AND run_id = ?)) ORDER BY score DESC, id LIMIT ?`, - after, runID, runID, outcome, outcome, after, after, limit+1) + runID, outcome, outcome, after, after, runID, limit+1) if err != nil { return nil, 0, err } diff --git a/internal/matchrun/plan.go b/internal/matchrun/plan.go index a5f9147c..7ac9d514 100644 --- a/internal/matchrun/plan.go +++ b/internal/matchrun/plan.go @@ -283,8 +283,9 @@ func planSeries(scope string, st *catalog.MatchState, p catalog.MatchProposal, w // planFor is what applying p to a book in state st writes for a run in mode: a // repick's ASIN (whatever the scope), else Plan's (a refresh's proposal carries -// no identifier, so its Plan writes none: refreshBook), and for a refresh only -// while the book still has the identifier it was found by. +// no identifier, so its Plan writes none: refreshBook), for a refresh only while +// the book still has the identifier it was found by, and for a match only while +// the book is still unmatched (matchedSince). func planFor(mode, scope string, st *catalog.MatchState, p catalog.MatchProposal) Writes { switch mode { case catalog.MatchModeRepick: @@ -293,10 +294,23 @@ func planFor(mode, scope string, st *catalog.MatchState, p catalog.MatchProposal if !stillFoundBy(st, p.FoundBy) { return Writes{} } + case catalog.MatchModeMatch: + if matchedSince(st) { + return Writes{} + } } return Plan(scope, st, p) } +// matchedSince reports whether a match run's book has an ASIN or ISBN now. The +// run took only books with neither (UnmatchedBooks), so one has been matched +// since: by hand (from the review or the book page), by another admin or the +// manager, or by a rescan reading one from the tags. The run's match never +// writes over it, whatever the scope. +func matchedSince(st *catalog.MatchState) bool { + return st.Fields[catalog.FieldASIN].Value != "" || st.Fields[catalog.FieldISBN].Value != "" +} + // stillFoundBy reports whether the book still has the identifier a refresh // found its record by (Proposal.FoundBy): a book matched again since (by hand, // to another record) is not overwritten with the old one's details. diff --git a/internal/matchrun/runner.go b/internal/matchrun/runner.go index e7a53c8c..bbefc9c9 100644 --- a/internal/matchrun/runner.go +++ b/internal/matchrun/runner.go @@ -448,6 +448,9 @@ func (r *Runner) applyItem(ctx context.Context, mode string, it *catalog.MatchRu } w := planFor(mode, o.Scope, st, it.Proposal) if w.Empty() { + if mode == catalog.MatchModeMatch && matchedSince(st) { + return catalog.ItemSkipped, "matched_since" + } return catalog.ItemSkipped, "nothing_to_change" } edits := len(w.Set) > 0 || len(w.Revert) > 0 @@ -516,9 +519,12 @@ func changeOf(st *catalog.MatchState, w Writes) Change { // nothing is indexed at its path any more) and what each scope would change. type ItemView struct { catalog.MatchRunItem - Book ItemBook `json:"book"` - Gone bool `json:"gone,omitempty"` - Changes map[string]Change `json:"changes"` + Book ItemBook `json:"book"` + Gone bool `json:"gone,omitempty"` + // MatchedSince: a match run's book matched since the run (matchedSince), + // which an apply leaves alone; its changes are none. + MatchedSince bool `json:"matched_since,omitempty"` + Changes map[string]Change `json:"changes"` } // ItemBook is the book an item is for, as it is now. @@ -555,6 +561,7 @@ func (r *Runner) Items(ctx context.Context, run *catalog.MatchRun, outcome strin return nil, 0, err default: v.Book = ItemBook{Title: st.Title, Author: st.Author} + v.MatchedSince = run.Mode == catalog.MatchModeMatch && it.Applied == "" && matchedSince(st) for _, scope := range scopes { v.Changes[scope] = changeOf(st, planFor(run.Mode, scope, st, it.Proposal)) } diff --git a/internal/matchrun/runner_test.go b/internal/matchrun/runner_test.go index 7134e8bb..ecc94aae 100644 --- a/internal/matchrun/runner_test.go +++ b/internal/matchrun/runner_test.go @@ -255,6 +255,54 @@ func TestApplyExcludesAndSkips(t *testing.T) { } } +// TestApplyLeavesABookMatchedSince: a match run's book matched since the run (by +// hand, here to another work) reads as matched since in the review, with nothing +// to change under any scope, and an apply, even one that overwrites, leaves it +// alone while it matches the rest. +func TestApplyLeavesABookMatchedSince(t *testing.T) { + e := newEnv(t) + ctx := context.Background() + e.book(t, "A/One", "One", "", nil) + e.book(t, "A/Two", "Two", "", nil) + for _, title := range []string{"One", "Two"} { + e.matcher.byTitle[title] = []meta.MatchCandidate{cand("the-martian", 97, "")} + } + run := e.start(t, catalog.MatchModeMatch, "") + if err := e.cat.EditBook(ctx, e.lib, "A/Two", catalog.BookEdit{ + Set: map[string]string{catalog.FieldASIN: "B0HAND0001", catalog.FieldTitle: "By Hand"}, + Source: catalog.SourceCommunity, + }); err != nil { + t.Fatal(err) + } + for _, it := range e.items(t, run, catalog.OutcomeAuto) { + hand := it.Path == "A/Two" + if it.MatchedSince != hand { + t.Errorf("%s matched since = %v, want %v", it.Path, it.MatchedSince, hand) + } + if hand && len(it.Changes[ScopeOverwrite].Fields) != 0 { + t.Errorf("matched since changes = %+v, want none", it.Changes) + } + } + + if _, err := e.runner.Apply(ctx, ctx, run.ID, ApplyOptions{Scope: ScopeOverwrite}); err != nil { + t.Fatal(err) + } + e.runner.Wait() + for _, it := range e.items(t, run, catalog.OutcomeAuto) { + want := map[string]string{"A/One": catalog.ItemApplied + "/", "A/Two": catalog.ItemSkipped + "/matched_since"}[it.Path] + if got := it.Applied + "/" + it.Detail; got != want { + t.Errorf("%s = %s, want %s", it.Path, got, want) + } + if it.MatchedSince { + t.Errorf("%s still reads as matched since once applied", it.Path) + } + } + st, _ := e.cat.BookMatchState(ctx, e.lib, "A/Two") + if st.Fields[catalog.FieldASIN].Value != "B0HAND0001" || st.Fields[catalog.FieldTitle].Value != "By Hand" { + t.Fatalf("the hand match was overwritten: %+v", st.Fields) + } +} + func TestRunStopsWhenTheServiceIsDown(t *testing.T) { e := newEnv(t) for i := range 12 { From a58496be4073977080804e343215ce5a7f80903e Mon Sep 17 00:00:00 2001 From: KodeStar Date: Fri, 16 Oct 2026 11:26:39 +0100 Subject: [PATCH 3/3] Match review: the code review's fixes (xhigh) A match whose cover a stop cut short stays to apply (cover_pending), so its own ASIN doesn't read as matched since and the next apply takes the cover; the review marks a row matched since only when the server would (a match run's pending book that has an identifier now), else asks for that tab again; an applied row is never left out twice; a hand match refreshes the Health issues. --- CLAUDE.md | 4 +- admin-ui/src/api/types.ts | 2 +- .../features/health/bulk-match-model.test.ts | 2 + .../src/features/health/bulk-match-model.ts | 4 +- .../src/features/health/bulk-match.test.tsx | 96 +++++++++++++------ admin-ui/src/features/health/bulk-match.tsx | 24 +++-- internal/matchrun/plan.go | 24 +++-- internal/matchrun/runner.go | 30 ++++-- internal/matchrun/runner_test.go | 34 +++++++ 9 files changed, 166 insertions(+), 54 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 117ca581..3c8c2b6d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1078,7 +1078,9 @@ future metadata site can attach enrichment without reshaping the schema. The applying it is a no-op for a book that no longer has it (matched again since). Likewise a match run's book that has an ASIN or ISBN now (`matchedSince`: matched by hand from the review or anywhere, or a rescan found one) has no changes, reads `matched_since` - in the review and is skipped with detail `matched_since`, under any scope. The review's + in the review and is skipped with detail `matched_since`, under any scope; a match whose + fields went in before a stop cut its cover short stays to apply, detail `cover_pending`, its + ASIN the run's own rather than one matched since, so the next apply takes the cover. The review's items page best match first (`score DESC, id`), the cursor still an item id whose score the next page reads on from. Each scope's change carries `series` {from, to} (main first, position 0 = none) when it diff --git a/admin-ui/src/api/types.ts b/admin-ui/src/api/types.ts index d07209f8..ba443032 100644 --- a/admin-ui/src/api/types.ts +++ b/admin-ui/src/api/types.ts @@ -1994,7 +1994,7 @@ export interface MatchRunItem { proposal: MatchProposal; /** What applying did: '' (not yet) | applied | skipped | failed. */ applied: '' | 'applied' | 'skipped' | 'failed'; - /** Why: book_gone | nothing_to_change | matched_since | edit_failed | cover_failed | metadata_unavailable | identifier_unknown. */ + /** Why: book_gone | nothing_to_change | matched_since | edit_failed | cover_failed | metadata_unavailable | identifier_unknown; cover_pending while still to apply (a stop cut its cover short). */ detail?: string; /** The book as it is now. */ book: { title: string; author: string }; diff --git a/admin-ui/src/features/health/bulk-match-model.test.ts b/admin-ui/src/features/health/bulk-match-model.test.ts index 53bbfc90..6ace03a5 100644 --- a/admin-ui/src/features/health/bulk-match-model.test.ts +++ b/admin-ui/src/features/health/bulk-match-model.test.ts @@ -123,6 +123,8 @@ describe('picks', () => { expect(isChosen(doubtful, picks)).toBe(false); expect(applyRequest('fill', picks)).toEqual({ scope: 'fill', exclude: [1], include: [] }); expect(isPickable(item({ matched_since: true }))).toBe(false); + // One already applied isn't pending, so leaving it out doesn't lower the count. + expect(leaveOut(NO_PICKS, item({ applied: 'applied' }))).toBe(NO_PICKS); }); }); diff --git a/admin-ui/src/features/health/bulk-match-model.ts b/admin-ui/src/features/health/bulk-match-model.ts index d7ceab7b..b787ec0c 100644 --- a/admin-ui/src/features/health/bulk-match-model.ts +++ b/admin-ui/src/features/health/bulk-match-model.ts @@ -81,9 +81,11 @@ export function togglePick(picks: Picks, item: MatchRunItem): Picks { /** * Leaves an item out of the apply, as a book matched by hand from the review is - * (the server leaves it alone anyway: this keeps the apply's count true). + * (the server leaves it alone anyway: this keeps the apply's count true). One + * already applied is no pick at all (applyCount counts only pending ones). */ export function leaveOut(picks: Picks, item: MatchRunItem): Picks { + if (item.applied) return picks; return item.outcome === 'auto' ? { ...picks, exclude: withId(picks.exclude, item.id) } : { ...picks, include: withoutId(picks.include, item.id) }; diff --git a/admin-ui/src/features/health/bulk-match.test.tsx b/admin-ui/src/features/health/bulk-match.test.tsx index a8431b93..e0181648 100644 --- a/admin-ui/src/features/health/bulk-match.test.tsx +++ b/admin-ui/src/features/health/bulk-match.test.tsx @@ -4,7 +4,7 @@ import { setToken } from '@/api/token'; import type { MatchRun, MatchRunItem } from '@/api/types'; import { mockFetch, type MockRoute } from '@/test/fetch-mock'; import { issuesSummary } from '@/test/fixtures'; -import { bookDetail } from '@/test/library-fixtures'; +import { bookDetail, books } from '@/test/library-fixtures'; import { renderApp } from '@/test/render-app'; import { signedInRoutes } from '@/test/routes'; @@ -302,29 +302,47 @@ describe('bulk matching', () => { await waitFor(() => expect(issuesAsked()).toBeGreaterThan(before)); }); + // The match dialog over the review: one candidate, no recordings (so no cover). + const byHandRoutes: Record = { + 'GET /admin/libraries/1/book/match': { + body: { + candidates: [ + { + work_id: 'the-martian', + title: 'The Martian', + authors: [{ id: 'a1', name: 'Andy Weir' }], + first_published: '2011', + series: [], + web_url: 'https://meta.audiosilo.app/works/the-martian', + recordings: [], + score: 100, + }, + ], + }, + }, + 'POST /admin/meta/covers': { body: { covers: [''] } }, + }; + + /** Matches the review's confident book by hand, accepting the default ticks. */ + async function matchByHand(user: ReturnType, row: HTMLElement) { + await user.click(within(row).getByRole('button', { name: 'Match The Martian by hand' })); + const match = await screen.findByRole('dialog', { name: 'Match with community metadata' }); + await within(match).findByRole('radio', { name: /The Martian/ }); + await user.click(within(match).getByRole('button', { name: 'Compare fields' })); + const compare = await screen.findByRole('dialog', { name: 'Compare with community metadata' }); + await user.click(within(compare).getByRole('button', { name: /^Accept \d+ field/ })); + await waitFor(() => + expect(screen.queryByRole('dialog', { name: 'Compare with community metadata' })).toBeNull(), + ); + } + it('matches a book by hand over the review, which keeps its place and leaves it out', async () => { const detail = bookDetail(); const calls = mockFetch( routes([run()], { 'GET /admin/libraries/1/book': { body: detail }, 'PATCH /admin/libraries/1/book': { body: detail }, - 'GET /admin/libraries/1/book/match': { - body: { - candidates: [ - { - work_id: 'the-martian', - title: 'The Martian', - authors: [{ id: 'a1', name: 'Andy Weir' }], - first_published: '2011', - series: [], - web_url: 'https://meta.audiosilo.app/works/the-martian', - recordings: [], - score: 100, - }, - ], - }, - }, - 'POST /admin/meta/covers': { body: { covers: [''] } }, + ...byHandRoutes, }), ); const user = userEvent.setup(); @@ -348,15 +366,7 @@ describe('bulk matching', () => { ); expect(screen.getByRole('dialog', { name: 'Review matches' })).toBeInTheDocument(); - await user.click(within(row).getByRole('button', { name: 'Match The Martian by hand' })); - const match = await screen.findByRole('dialog', { name: 'Match with community metadata' }); - await within(match).findByRole('radio', { name: /The Martian/ }); - await user.click(within(match).getByRole('button', { name: 'Compare fields' })); - const compare = await screen.findByRole('dialog', { name: 'Compare with community metadata' }); - await user.click(within(compare).getByRole('button', { name: /^Accept \d+ field/ })); - await waitFor(() => - expect(screen.queryByRole('dialog', { name: 'Compare with community metadata' })).toBeNull(), - ); + await matchByHand(user, row); expect(calls.some((c) => c.method === 'PATCH' && c.path === '/admin/libraries/1/book')).toBe( true, ); @@ -370,6 +380,38 @@ describe('bulk matching', () => { expect(within(review).getByRole('button', { name: 'Apply to 0 books' })).toBeDisabled(); }); + it('asks the server again for a book matched by hand that took no ASIN or ISBN', async () => { + const unmatched = bookDetail({ book: { ...books[0], asin: '' } }); + const calls = mockFetch( + routes([run()], { + 'GET /admin/libraries/1/book': { body: unmatched }, + 'PATCH /admin/libraries/1/book': { body: unmatched }, + ...byHandRoutes, + }), + ); + const autoAsked = () => + calls.filter( + (c) => c.path === '/admin/match-runs/5/items' && c.query.get('outcome') === 'auto', + ).length; + const issuesAsked = () => calls.filter((c) => c.path === '/admin/issues').length; + const user = userEvent.setup(); + renderApp('/health?issue=unmatched'); + await user.click(await screen.findByRole('button', { name: 'Review and apply' })); + const review = await screen.findByRole('dialog', { name: 'Review matches' }); + const row = (await within(review).findByText('B0UK000001 · UK')).closest('li')!; + const [items, issues] = [autoAsked(), issuesAsked()]; + await matchByHand(user, row); + + // Still unmatched to the server, so not "matched since": the review asks again, + // the Health page under it too, and the book stays left out of the apply. + await waitFor(() => expect(autoAsked()).toBeGreaterThan(items)); + await waitFor(() => expect(issuesAsked()).toBeGreaterThan(issues)); + const after = (await within(review).findByText('B0UK000001 · UK')).closest('li')!; + expect(within(after).queryByText(/Matched since this run/)).toBeNull(); + expect(within(after).getByRole('checkbox')).not.toBeChecked(); + expect(within(review).getByRole('button', { name: 'Apply to 0 books' })).toBeDisabled(); + }); + it("shows a gone book's path once, as its title", async () => { const gone: MatchRunItem = { ...confident, diff --git a/admin-ui/src/features/health/bulk-match.tsx b/admin-ui/src/features/health/bulk-match.tsx index 73f5e4f2..4eaaf982 100644 --- a/admin-ui/src/features/health/bulk-match.tsx +++ b/admin-ui/src/features/health/bulk-match.tsx @@ -5,6 +5,7 @@ import { useTranslation } from 'react-i18next'; import { ArrowRight, Check, ExternalLink, LoaderCircle, Sparkles } from 'lucide-react'; import { api } from '@/api/client'; import { + invalidateIssues, invalidateMatches, keys, matchRunActive, @@ -14,6 +15,7 @@ import { } from '@/api/hooks'; import { MATCH_SCOPES, + type AdminBookDetail, type MatchOutcome, type MatchRun, type MatchRunItem, @@ -360,20 +362,31 @@ function ReviewDialog({ setByHand(undefined); } } - // A book matched by hand leaves the apply, and its row reads as the server - // would now show it (matched since the run, nothing to change), without asking - // again for every page the review has loaded. + // A book matched by hand leaves the apply, and the Health page under the review + // reads its issues again. A match run's book that took an ASIN or ISBN is one + // the server now calls matched since (matchrun.matchedSince): its row reads so + // at once, without asking again for every page the review has loaded. Any + // other (another mode, an applied item, a hand match that took no identifier) + // the server says what applying would do now. const onMatchedByHand = (item: MatchRunItem) => { setPicks((p) => leaveOut(p, item)); + invalidateIssues(qc); + const book = qc.getQueryData(keys.book(item.library_id, item.path))?.book; + const itemsKey = keys.matchRunItems(run.id, item.outcome); + if (run.mode !== 'match' || item.applied || !(book?.asin || book?.isbn)) { + void qc.invalidateQueries({ queryKey: itemsKey }); + return; + } + const now = { title: book.title, author: book.author }; qc.setQueryData>( - keys.matchRunItems(run.id, item.outcome), + itemsKey, (data) => data && { ...data, pages: data.pages.map((page) => ({ ...page, items: (page.items ?? []).map((it) => - it.id === item.id ? { ...it, matched_since: true, changes: {} } : it, + it.id === item.id ? { ...it, matched_since: true, changes: {}, book: now } : it, ), })), }, @@ -757,7 +770,6 @@ function ItemRow({ {/* The book page in a new tab, to listen or look closer without leaving the review. */} 0 || len(w.Revert) > 0 @@ -561,9 +573,9 @@ func (r *Runner) Items(ctx context.Context, run *catalog.MatchRun, outcome strin return nil, 0, err default: v.Book = ItemBook{Title: st.Title, Author: st.Author} - v.MatchedSince = run.Mode == catalog.MatchModeMatch && it.Applied == "" && matchedSince(st) + v.MatchedSince = run.Mode == catalog.MatchModeMatch && it.Applied == "" && matchedSince(st, &it) for _, scope := range scopes { - v.Changes[scope] = changeOf(st, planFor(run.Mode, scope, st, it.Proposal)) + v.Changes[scope] = changeOf(st, planFor(run.Mode, scope, st, &it)) } } out[i] = v diff --git a/internal/matchrun/runner_test.go b/internal/matchrun/runner_test.go index ecc94aae..4d063052 100644 --- a/internal/matchrun/runner_test.go +++ b/internal/matchrun/runner_test.go @@ -569,6 +569,40 @@ func TestCancelledApplyMarksWhatWentIn(t *testing.T) { } } +// TestStoppedCoverIsTakenAgain: a match whose fields went in before a stop cut +// its cover short stays to apply, not matched since (the ASIN the book has now +// is the run's own), and the next apply takes the cover. +func TestStoppedCoverIsTakenAgain(t *testing.T) { + e := newEnv(t) + ctx := context.Background() + e.book(t, "A/a", "Book", "", no) + e.matcher.byTitle["Book"] = []meta.MatchCandidate{cand("the-martian", 97, "https://c/rec.jpg")} + run := e.start(t, catalog.MatchModeMatch, "") + save := e.runner.saveCover + e.runner.saveCover = func(ctx context.Context, _ int64, _, _ string, _ int64) error { + e.runner.Cancel(run.ID) + return ctx.Err() + } + if _, err := e.runner.Apply(ctx, ctx, run.ID, ApplyOptions{Scope: ScopeFill}); err != nil { + t.Fatal(err) + } + e.runner.Wait() + it := e.items(t, run, catalog.OutcomeAuto)[0] + if it.Applied != "" || it.MatchedSince || !it.Changes[ScopeFill].Cover { + t.Fatalf("item after the stop = %+v, want still to apply, its cover to take", it) + } + + e.runner.saveCover = save + if _, err := e.runner.Apply(ctx, ctx, run.ID, ApplyOptions{Scope: ScopeFill}); err != nil { + t.Fatal(err) + } + e.runner.Wait() + it = e.items(t, run, catalog.OutcomeAuto)[0] + if it.Applied != catalog.ItemApplied || it.Detail != "" || e.covers.Load() != 1 { + t.Fatalf("item = %+v, covers %d; want applied with the cover taken", it.MatchRunItem, e.covers.Load()) + } +} + func TestApplyCoverFailureAndInterruptedApply(t *testing.T) { e := newEnv(t) ctx := context.Background()