diff --git a/scripts/e2e/suite-registry.mjs b/scripts/e2e/suite-registry.mjs index 74b4ec20a..3fef12e3d 100644 --- a/scripts/e2e/suite-registry.mjs +++ b/scripts/e2e/suite-registry.mjs @@ -513,6 +513,25 @@ export const E2E_SUITES = Object.freeze({ ...SHARED_DB_AND_HELPER_PATHS, ], }, + "koreader-progress-position": { + id: "koreader-progress-position", + name: "KOReader Progress Position Routing", + timeout: 45, + lane: "full", + description: "KOReader progress position routing into cfi or pageNumber per book format", + vitestTarget: "test/koreader-progress-position.e2e-spec.ts", + junitOutput: `${TEST_RESULTS_DIR}/koreader-progress-position-e2e-junit.xml`, + prepareDedicatedDatabase: true, + useDedicatedDatabase: true, + changedPaths: [ + "server/src/modules/koreader/**", + "server/src/modules/book/book.service.ts", + "server/src/db/schema/reader.ts", + "server/test/koreader-progress-position.e2e-spec.ts", + "server/test/e2e/reader-state-isolation/**", + ...SHARED_DB_AND_HELPER_PATHS, + ], + }, "koreader-file-naming": { id: "koreader-file-naming", name: "KOReader File Naming", diff --git a/server/src/db/schema/reader.ts b/server/src/db/schema/reader.ts index 500609802..d1ff4e393 100644 --- a/server/src/db/schema/reader.ts +++ b/server/src/db/schema/reader.ts @@ -152,7 +152,7 @@ export const readingProgress = pgTable( percentage: real('percentage').notNull().default(0), // EPUB: CFI string pinpoints exact location cfi: varchar('cfi', { length: 2000 }), - // PDF / CBX / CBR: zero-based page index + // PDF / CBX / CBR: 1-based page number pageNumber: integer('page_number'), // Audio: playback position in seconds positionSeconds: real('position_seconds'), diff --git a/server/src/modules/koreader/koreader-plugin.service.ts b/server/src/modules/koreader/koreader-plugin.service.ts index 29bf5b270..0518e0b26 100644 --- a/server/src/modules/koreader/koreader-plugin.service.ts +++ b/server/src/modules/koreader/koreader-plugin.service.ts @@ -224,7 +224,7 @@ export class KoreaderPluginService { } entries.push({ - bookFile: { id: match.bookFileId, bookId: match.bookId, libraryId: match.libraryId }, + bookFile: { id: match.bookFileId, bookId: match.bookId, libraryId: match.libraryId, format: match.format }, percentage: item.percentage, progress: item.progress, timestamp: item.timestamp, diff --git a/server/src/modules/koreader/koreader-progress-position.util.test.ts b/server/src/modules/koreader/koreader-progress-position.util.test.ts new file mode 100644 index 000000000..c0eccf852 --- /dev/null +++ b/server/src/modules/koreader/koreader-progress-position.util.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from 'vitest'; + +import { isPagedReadingFormat, parseKoreaderPageNumber } from './koreader-progress-position.util'; + +describe('isPagedReadingFormat', () => { + it.each(['pdf', 'PDF', 'cbz', 'cbr', 'cb7', 'cbx', 'CBZ'])('treats %s as paged', (format) => { + expect(isPagedReadingFormat(format)).toBe(true); + }); + + it.each(['epub', 'mobi', 'azw3', 'azw', 'fb2', 'txt', 'EPUB'])('treats %s as reflowable', (format) => { + expect(isPagedReadingFormat(format)).toBe(false); + }); + + it.each(['m4b', 'mp3', 'm4a', 'opus', 'ogg', 'flac'])('treats %s as non-paged', (format) => { + expect(isPagedReadingFormat(format)).toBe(false); + }); + + it.each([null, undefined, '', ' ', 'djvu'])('falls back to non-paged for %s', (format) => { + expect(isPagedReadingFormat(format)).toBe(false); + }); +}); + +describe('parseKoreaderPageNumber', () => { + it('parses the page number KOReader sends for a paged document', () => { + expect(parseKoreaderPageNumber('117')).toBe(117); + }); + + it('accepts the first page', () => { + expect(parseKoreaderPageNumber('1')).toBe(1); + }); + + it('tolerates surrounding whitespace and leading zeros', () => { + expect(parseKoreaderPageNumber(' 42 ')).toBe(42); + expect(parseKoreaderPageNumber('0042')).toBe(42); + }); + + it('rejects an xpointer that reached a paged file by mistake', () => { + expect(parseKoreaderPageNumber('/body/DocFragment[8]/body/p[12]/text().0')).toBeNull(); + }); + + it.each([null, undefined, '', ' '])('returns null for %s so the stored page is cleared', (progress) => { + expect(parseKoreaderPageNumber(progress)).toBeNull(); + }); + + it.each(['0', '-1', '-12'])('rejects %s because KOReader pages start at 1', (progress) => { + expect(parseKoreaderPageNumber(progress)).toBeNull(); + }); + + it.each(['12.5', '12.0', '1e3', '0x2a', '4 2', '12abc', 'NaN', 'Infinity', '+7'])('rejects the non-integer %s', (progress) => { + expect(parseKoreaderPageNumber(progress)).toBeNull(); + }); + + it('accepts the largest page a PostgreSQL integer column can hold', () => { + expect(parseKoreaderPageNumber('2147483647')).toBe(2_147_483_647); + }); + + it('rejects a page that would overflow the integer column', () => { + expect(parseKoreaderPageNumber('2147483648')).toBeNull(); + }); + + it('rejects a digit string too long to survive as a safe integer', () => { + expect(parseKoreaderPageNumber('9'.repeat(400))).toBeNull(); + }); + + it('rejects non-string input defensively', () => { + expect(parseKoreaderPageNumber(42 as unknown as string)).toBeNull(); + }); +}); diff --git a/server/src/modules/koreader/koreader-progress-position.util.ts b/server/src/modules/koreader/koreader-progress-position.util.ts new file mode 100644 index 000000000..6df21d2cb --- /dev/null +++ b/server/src/modules/koreader/koreader-progress-position.util.ts @@ -0,0 +1,35 @@ +import { getFormatGroup } from '@bookorbit/types'; + +/** reading_progress.page_number is a PostgreSQL integer, so anything above int4 would fail the insert. */ +const MAX_PAGE_NUMBER = 2_147_483_647; + +/** + * KOReader reports a reading position two different ways: reflowable documents get a + * CREngine xpointer, paged ones (PDF and the CBX comic containers) get a plain page number. + * The two land in different reading_progress columns, so every incoming position has + * to be routed by the book file's format. + * + * An unknown or missing format resolves to the reflowable group, matching getFormatGroup. + */ +export function isPagedReadingFormat(format: string | null | undefined): boolean { + const group = getFormatGroup(format ?? ''); + return group === 'pdf' || group === 'cbx'; +} + +/** + * Parses the 1-based page number KOReader sends for paged documents + * (ReaderPaging:getLastProgress). Returns null for anything else, including an xpointer + * that reached a paged file by mistake, so the caller clears the stored page instead of + * replacing it with a position the reader would misinterpret. + */ +export function parseKoreaderPageNumber(progress: string | null | undefined): number | null { + if (typeof progress !== 'string') return null; + + const trimmed = progress.trim(); + if (!/^\d+$/.test(trimmed)) return null; + + const page = Number(trimmed); + if (!Number.isSafeInteger(page) || page < 1 || page > MAX_PAGE_NUMBER) return null; + + return page; +} diff --git a/server/src/modules/koreader/koreader.repository.test.ts b/server/src/modules/koreader/koreader.repository.test.ts index 67674aba0..0db7bca82 100644 --- a/server/src/modules/koreader/koreader.repository.test.ts +++ b/server/src/modules/koreader/koreader.repository.test.ts @@ -61,7 +61,7 @@ describe('KoreaderRepository', () => { }); it('returns the book file when found by current hash', async () => { - const file = { id: 10, bookId: 20, libraryId: 1 }; + const file = { id: 10, bookId: 20, libraryId: 1, format: 'epub' }; db.select.mockReturnValue(makeQueryChain([file])); const result = await repo.resolveBookFileByHash('abc123', null); @@ -71,7 +71,7 @@ describe('KoreaderRepository', () => { }); it('falls back to hash history when current hash lookup returns nothing', async () => { - const file = { id: 10, bookId: 20, libraryId: 1 }; + const file = { id: 10, bookId: 20, libraryId: 1, format: 'pdf' }; db.select.mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce(makeQueryChain([file])); const result = await repo.resolveBookFileByHash('oldhash', null); @@ -81,7 +81,7 @@ describe('KoreaderRepository', () => { }); it('falls back to a user-scoped manual link after direct and history lookups miss', async () => { - const file = { id: 10, bookId: 20, libraryId: 1 }; + const file = { id: 10, bookId: 20, libraryId: 1, format: 'cbz' }; db.select .mockReturnValueOnce(makeQueryChain([])) .mockReturnValueOnce(makeQueryChain([])) @@ -93,6 +93,17 @@ describe('KoreaderRepository', () => { expect(db.select).toHaveBeenCalledTimes(3); }); + it('selects the file format on every lookup so callers can route the position', async () => { + db.select.mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce(makeQueryChain([])); + + await repo.resolveBookFileByHash('manualhash', [1], 7); + + expect(db.select).toHaveBeenCalledTimes(3); + for (const call of db.select.mock.calls) { + expect(call[0]).toHaveProperty('format'); + } + }); + it('returns null when a user-scoped manual link lookup also misses', async () => { db.select.mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce(makeQueryChain([])); @@ -122,41 +133,52 @@ describe('KoreaderRepository', () => { db.select .mockReturnValueOnce( makeQueryChain([ - { hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31 }, - { hash: null, bookFileId: 12, bookId: 22, libraryId: 32 }, + { hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }, + { hash: null, bookFileId: 12, bookId: 22, libraryId: 32, format: 'epub' }, ]), ) - .mockReturnValueOnce(makeQueryChain([{ hash: 'old', bookFileId: 13, bookId: 23, libraryId: 33 }])); + .mockReturnValueOnce(makeQueryChain([{ hash: 'old', bookFileId: 13, bookId: 23, libraryId: 33, format: 'pdf' }])); const result = await repo.resolveBookFilesByHashes(['current', 'old'], [31, 33]); - expect(result.get('current')).toEqual({ bookFileId: 11, bookId: 21, libraryId: 31 }); - expect(result.get('old')).toEqual({ bookFileId: 13, bookId: 23, libraryId: 33 }); + expect(result.get('current')).toEqual({ bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }); + expect(result.get('old')).toEqual({ bookFileId: 13, bookId: 23, libraryId: 33, format: 'pdf' }); expect(result.size).toBe(2); expect(db.select).toHaveBeenCalledTimes(2); }); it('skips hash history lookup when all hashes resolve directly', async () => { - db.select.mockReturnValueOnce(makeQueryChain([{ hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31 }])); + db.select.mockReturnValueOnce(makeQueryChain([{ hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31, format: 'pdf' }])); const result = await repo.resolveBookFilesByHashes(['current'], null); - expect(result.get('current')).toEqual({ bookFileId: 11, bookId: 21, libraryId: 31 }); + expect(result.get('current')).toEqual({ bookFileId: 11, bookId: 21, libraryId: 31, format: 'pdf' }); expect(db.select).toHaveBeenCalledTimes(1); }); it('resolves remaining hashes from user-scoped manual links', async () => { db.select - .mockReturnValueOnce(makeQueryChain([{ hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31 }])) + .mockReturnValueOnce(makeQueryChain([{ hash: 'current', bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }])) .mockReturnValueOnce(makeQueryChain([])) - .mockReturnValueOnce(makeQueryChain([{ hash: 'manual', bookFileId: 12, bookId: 22, libraryId: 32 }])); + .mockReturnValueOnce(makeQueryChain([{ hash: 'manual', bookFileId: 12, bookId: 22, libraryId: 32, format: 'cbr' }])); const result = await repo.resolveBookFilesByHashes(['current', 'manual'], [31, 32], 7); - expect(result.get('current')).toEqual({ bookFileId: 11, bookId: 21, libraryId: 31 }); - expect(result.get('manual')).toEqual({ bookFileId: 12, bookId: 22, libraryId: 32 }); + expect(result.get('current')).toEqual({ bookFileId: 11, bookId: 21, libraryId: 31, format: 'epub' }); + expect(result.get('manual')).toEqual({ bookFileId: 12, bookId: 22, libraryId: 32, format: 'cbr' }); expect(db.select).toHaveBeenCalledTimes(3); }); + + it('selects the file format on every lookup so callers can route the position', async () => { + db.select.mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce(makeQueryChain([])).mockReturnValueOnce(makeQueryChain([])); + + await repo.resolveBookFilesByHashes(['current', 'manual'], [31, 32], 7); + + expect(db.select).toHaveBeenCalledTimes(3); + for (const call of db.select.mock.calls) { + expect(call[0]).toHaveProperty('format'); + } + }); }); describe('unmatched books and manual hash links', () => { @@ -743,12 +765,22 @@ describe('KoreaderRepository', () => { }); describe('upsertReadingProgress', () => { - it('upserts percentage and clears stale web locator fields on conflict', async () => { + function mockUpsertChain() { const onConflictDoUpdate = vi.fn().mockResolvedValue(undefined); const values = vi.fn().mockReturnValue({ onConflictDoUpdate }); db.insert.mockReturnValue({ values }); + return { onConflictDoUpdate, values }; + } - await repo.upsertReadingProgress(44, 12, 41.25); + function conflictSet(onConflictDoUpdate: ReturnType) { + const arg = onConflictDoUpdate.mock.calls[0]?.[0] as { set?: Record } | undefined; + return arg?.set; + } + + it('upserts percentage and clears stale web locator fields on conflict', async () => { + const { onConflictDoUpdate, values } = mockUpsertChain(); + + await repo.upsertReadingProgress({ bookFileId: 44, userId: 12, percentage: 41.25 }); expect(db.insert).toHaveBeenCalledTimes(1); expect(values).toHaveBeenCalledWith( @@ -756,6 +788,9 @@ describe('KoreaderRepository', () => { bookFileId: 44, userId: 12, percentage: 41.25, + cfi: null, + pageNumber: null, + koreaderProgress: null, }), ); @@ -767,12 +802,47 @@ describe('KoreaderRepository', () => { cfi: null, pageNumber: null, koreaderProgress: null, + koboLocationSource: null, + koboLocationType: null, + koboLocationValue: null, + koboContentSourceProgressPercent: null, }), }), ); - const conflictArg = onConflictDoUpdate.mock.calls[0]?.[0] as { set?: Record } | undefined; - expect(conflictArg?.set?.['updatedAt']).toBeDefined(); + expect(conflictSet(onConflictDoUpdate)?.['updatedAt']).toBeDefined(); + }); + + it('persists a paged position on both the insert and the conflict update', async () => { + const { onConflictDoUpdate, values } = mockUpsertChain(); + + await repo.upsertReadingProgress({ bookFileId: 44, userId: 12, percentage: 30, xpointer: '117', pageNumber: 117 }); + + expect(values).toHaveBeenCalledWith(expect.objectContaining({ pageNumber: 117, cfi: null, koreaderProgress: '117' })); + expect(conflictSet(onConflictDoUpdate)).toEqual(expect.objectContaining({ pageNumber: 117, cfi: null, koreaderProgress: '117' })); + }); + + it('persists a reflowable position without touching the page column', async () => { + const { onConflictDoUpdate, values } = mockUpsertChain(); + + await repo.upsertReadingProgress({ + bookFileId: 44, + userId: 12, + percentage: 30, + cfi: 'epubcfi(/6/14!/4/2/6)', + xpointer: '/body/DocFragment[7]', + }); + + expect(values).toHaveBeenCalledWith(expect.objectContaining({ cfi: 'epubcfi(/6/14!/4/2/6)', pageNumber: null })); + expect(conflictSet(onConflictDoUpdate)).toEqual(expect.objectContaining({ cfi: 'epubcfi(/6/14!/4/2/6)', pageNumber: null })); + }); + + it('clears a stale stored page when the sync carries no page', async () => { + const { onConflictDoUpdate } = mockUpsertChain(); + + await repo.upsertReadingProgress({ bookFileId: 44, userId: 12, percentage: 30, pageNumber: null }); + + expect(conflictSet(onConflictDoUpdate)).toEqual(expect.objectContaining({ pageNumber: null })); }); }); diff --git a/server/src/modules/koreader/koreader.repository.ts b/server/src/modules/koreader/koreader.repository.ts index 4e185909b..b53aaf49c 100644 --- a/server/src/modules/koreader/koreader.repository.ts +++ b/server/src/modules/koreader/koreader.repository.ts @@ -7,7 +7,8 @@ import { DB } from '../../db'; import * as schema from '../../db/schema'; type Db = NodePgDatabase; -type ResolvedBookFileByHash = { id: number; bookId: number; libraryId: number }; +type ResolvedBookFileByHash = { id: number; bookId: number; libraryId: number; format: string | null }; +type ResolvedBookFileByHashes = { bookFileId: number; bookId: number; libraryId: number; format: string | null }; type KoreaderUnmatchedSource = 'current_file' | 'file' | 'statistics'; type KoreaderUnmatchedCandidate = { hash: string; @@ -23,6 +24,18 @@ type KoreaderHashLinkMetadata = { lastOpen?: number | null; }; +export interface ReadingProgressUpsert { + bookFileId: number; + userId: number; + percentage: number; + /** Reflowable position converted from the device xpointer; null for paged formats or a failed conversion. */ + cfi?: string | null; + /** The device position string verbatim, so the next pull hands KOReader back what it sent. */ + xpointer?: string | null; + /** 1-based page for paged formats; null for reflowable and audio. */ + pageNumber?: number | null; +} + export interface DeviceProgressUpsert { bookFileId: number; userId: number; @@ -71,7 +84,7 @@ export class KoreaderRepository { const libraryFilter = accessibleLibraryIds ? inArray(schema.books.libraryId, accessibleLibraryIds) : undefined; const [byFileHash] = await this.db - .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId }) + .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, format: schema.bookFiles.format }) .from(schema.bookFiles) .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) .where(and(eq(schema.bookFiles.fileHash, hash), libraryFilter)) @@ -80,7 +93,7 @@ export class KoreaderRepository { if (byFileHash) return byFileHash; const [byFileHashHistory] = await this.db - .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId }) + .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, format: schema.bookFiles.format }) .from(schema.bookFileHashHistory) .innerJoin(schema.bookFiles, eq(schema.bookFiles.id, schema.bookFileHashHistory.bookFileId)) .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) @@ -91,7 +104,7 @@ export class KoreaderRepository { if (userId !== undefined) { const [byManualLink] = await this.db - .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId }) + .select({ id: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, format: schema.bookFiles.format }) .from(schema.koreaderBookHashLinks) .innerJoin(schema.bookFiles, eq(schema.bookFiles.id, schema.koreaderBookHashLinks.bookFileId)) .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) @@ -108,8 +121,8 @@ export class KoreaderRepository { hashes: string[], accessibleLibraryIds: number[] | null, userId?: number, - ): Promise> { - const result = new Map(); + ): Promise> { + const result = new Map(); if (hashes.length === 0) return result; if (accessibleLibraryIds !== null && accessibleLibraryIds.length === 0) return result; @@ -121,6 +134,7 @@ export class KoreaderRepository { bookFileId: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, + format: schema.bookFiles.format, }) .from(schema.bookFiles) .innerJoin(schema.books, eq(schema.books.id, schema.bookFiles.bookId)) @@ -128,7 +142,7 @@ export class KoreaderRepository { for (const row of direct) { if (row.hash && !result.has(row.hash)) { - result.set(row.hash, { bookFileId: row.bookFileId, bookId: row.bookId, libraryId: row.libraryId }); + result.set(row.hash, { bookFileId: row.bookFileId, bookId: row.bookId, libraryId: row.libraryId, format: row.format }); } } @@ -141,6 +155,7 @@ export class KoreaderRepository { bookFileId: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, + format: schema.bookFiles.format, }) .from(schema.bookFileHashHistory) .innerJoin(schema.bookFiles, eq(schema.bookFiles.id, schema.bookFileHashHistory.bookFileId)) @@ -149,7 +164,7 @@ export class KoreaderRepository { for (const row of history) { if (!result.has(row.hash)) { - result.set(row.hash, { bookFileId: row.bookFileId, bookId: row.bookId, libraryId: row.libraryId }); + result.set(row.hash, { bookFileId: row.bookFileId, bookId: row.bookId, libraryId: row.libraryId, format: row.format }); } } @@ -162,6 +177,7 @@ export class KoreaderRepository { bookFileId: schema.bookFiles.id, bookId: schema.bookFiles.bookId, libraryId: schema.books.libraryId, + format: schema.bookFiles.format, }) .from(schema.koreaderBookHashLinks) .innerJoin(schema.bookFiles, eq(schema.bookFiles.id, schema.koreaderBookHashLinks.bookFileId)) @@ -170,7 +186,7 @@ export class KoreaderRepository { for (const row of manualLinks) { if (!result.has(row.hash)) { - result.set(row.hash, { bookFileId: row.bookFileId, bookId: row.bookId, libraryId: row.libraryId }); + result.set(row.hash, { bookFileId: row.bookFileId, bookId: row.bookId, libraryId: row.libraryId, format: row.format }); } } @@ -533,24 +549,27 @@ export class KoreaderRepository { return row ?? null; } - async upsertReadingProgress(bookFileId: number, userId: number, percentage: number, cfi: string | null = null, xpointer: string | null = null) { + async upsertReadingProgress(entry: ReadingProgressUpsert) { + const { bookFileId, userId, percentage, cfi = null, xpointer = null, pageNumber = null } = entry; + await this.db .insert(schema.readingProgress) - .values({ bookFileId, userId, percentage, cfi, koreaderProgress: xpointer }) + .values({ bookFileId, userId, percentage, cfi, pageNumber, koreaderProgress: xpointer }) .onConflictDoUpdate({ target: [schema.readingProgress.bookFileId, schema.readingProgress.userId], // Deliberately do NOT update updatedAt here. reading_progress.updatedAt must only // change when the web reader writes it, so getProgress can use it as an accurate // "last web-reader sync time" for comparison against koreader_device_progress.updatedAt. - // The cfi stored here is the server-side conversion of KOReader's XPointer (null when - // conversion fails) so the web reader resumes at the same paragraph; stale web locator - // fields are never kept, or clients may resume at an older location. Kobo location - // fields clear because the position no longer matches the device's bookmark; the Kobo - // pull path recomputes a precise Location from the cfi. + // Exactly one of cfi and pageNumber carries the position, chosen by the book file's + // format: cfi is the server-side conversion of KOReader's XPointer for reflowable + // documents, pageNumber the page KOReader reports for paged ones. Both are written on + // every sync so stale web locator fields are never kept, or clients may resume at an + // older location. Kobo location fields clear because the position no longer matches the + // device's bookmark; the Kobo pull path recomputes a precise Location from the cfi. set: { percentage, cfi, - pageNumber: null, + pageNumber, koreaderProgress: xpointer, koboLocationSource: null, koboLocationType: null, diff --git a/server/src/modules/koreader/koreader.service.test.ts b/server/src/modules/koreader/koreader.service.test.ts index a486ce749..642fdb354 100644 --- a/server/src/modules/koreader/koreader.service.test.ts +++ b/server/src/modules/koreader/koreader.service.test.ts @@ -357,7 +357,7 @@ describe('KoreaderService', () => { describe('saveProgress', () => { it('resolves the book file, parses progress, extracts chapters, and updates synced progress', async () => { - mockRepo.resolveBookFileByHash.mockResolvedValue({ id: 44, bookId: 55, libraryId: 3 }); + mockRepo.resolveBookFileByHash.mockResolvedValue({ id: 44, bookId: 55, libraryId: 3, format: 'epub' }); mockChapterService.parseChapterIndexFromProgress.mockReturnValue(6); mockChapterExtractor.extractAndStoreChapters.mockRejectedValueOnce(new Error('extract failed')); @@ -383,11 +383,18 @@ describe('KoreaderService', () => { chapterIndex: 6, syncTimestamp: 1700000000, }); - expect(mockRepo.upsertReadingProgress).toHaveBeenCalledWith(44, 12, 50, null, '/body/DocFragment[7]'); + expect(mockRepo.upsertReadingProgress).toHaveBeenCalledWith({ + bookFileId: 44, + userId: 12, + percentage: 50, + cfi: null, + xpointer: '/body/DocFragment[7]', + pageNumber: null, + }); expect(mockBookService.syncKoboReadingStateForExternalProgress).toHaveBeenCalledWith(12, 44, 50); expect(mockBookService.autoUpdateReadStatusForProgress).toHaveBeenCalledWith( 12, - { id: 44, bookId: 55, libraryId: 3 }, + { id: 44, bookId: 55, libraryId: 3, format: 'epub' }, 50, expect.objectContaining({ origin: 'koreader', strongRereadEvidence: false }), ); @@ -438,7 +445,7 @@ describe('KoreaderService', () => { }); it('uses the default device and generated device id when the payload leaves them empty', async () => { - mockRepo.resolveBookFileByHash.mockResolvedValue({ id: 88, bookId: 99, libraryId: 4 }); + mockRepo.resolveBookFileByHash.mockResolvedValue({ id: 88, bookId: 99, libraryId: 4, format: 'epub' }); await service.saveProgress(12, { document: 'default-device-document', @@ -458,11 +465,71 @@ describe('KoreaderService', () => { }); }); + describe('shared progress position routing', () => { + async function syncFormat(format: string | null, progress?: string) { + mockRepo.resolveBookFileByHash.mockResolvedValue({ id: 44, bookId: 55, libraryId: 3, format }); + await service.saveProgress(12, { document: 'abcdef1234567890fedcba', percentage: 0.5, progress }); + return mockRepo.upsertReadingProgress.mock.calls[0]![0] as { cfi: string | null; pageNumber: number | null; xpointer: string | null }; + } + + it.each(['pdf', 'cbz', 'cbr', 'cb7', 'PDF'])('stores the KOReader page as pageNumber for %s', async (format) => { + const written = await syncFormat(format, '117'); + + expect(written.pageNumber).toBe(117); + expect(written.cfi).toBeNull(); + expect(mockPositionConverter.xpointerPointToCfi).not.toHaveBeenCalled(); + }); + + it('keeps the raw device position alongside the parsed page', async () => { + const written = await syncFormat('pdf', '117'); + + expect(written.xpointer).toBe('117'); + }); + + it('converts the xpointer to a cfi and writes no page for a reflowable format', async () => { + mockPositionConverter.xpointerPointToCfi.mockResolvedValue({ status: 'exact', cfi: 'epubcfi(/6/14!/4/2/6)' }); + + const written = await syncFormat('epub', '/body/DocFragment[7]/body/p[3]/text().0'); + + expect(written.cfi).toBe('epubcfi(/6/14!/4/2/6)'); + expect(written.pageNumber).toBeNull(); + expect(mockPositionConverter.xpointerPointToCfi).toHaveBeenCalledWith({ bookFileId: 44, pos: '/body/DocFragment[7]/body/p[3]/text().0' }); + }); + + it('treats an unknown format as reflowable', async () => { + const written = await syncFormat(null, '/body/DocFragment[7]'); + + expect(written.pageNumber).toBeNull(); + expect(mockPositionConverter.xpointerPointToCfi).toHaveBeenCalled(); + }); + + it('clears the stored page when a paged sync carries no position', async () => { + const written = await syncFormat('pdf', undefined); + + expect(written.pageNumber).toBeNull(); + expect(written.cfi).toBeNull(); + expect(written.xpointer).toBeNull(); + }); + + it('clears the stored page rather than storing an xpointer that reached a paged file', async () => { + const written = await syncFormat('pdf', '/body/DocFragment[7]/body/p[3]/text().0'); + + expect(written.pageNumber).toBeNull(); + expect(written.cfi).toBeNull(); + }); + + it('still records the percentage when the page cannot be parsed', async () => { + const written = (await syncFormat('pdf', 'not-a-page')) as unknown as { percentage: number }; + + expect(written.percentage).toBe(50); + }); + }); + describe('applyBulkProgress', () => { const device = { device: 'Kobo Libra 2', deviceId: 'device-a' }; - function bookFile(id: number, bookId = id * 10) { - return { id, bookId, libraryId: 1 }; + function bookFile(id: number, bookId = id * 10, format: string | null = 'epub') { + return { id, bookId, libraryId: 1, format }; } it('reads state once, writes one device statement, and applies shared progress per entry', async () => { @@ -488,6 +555,31 @@ describe('KoreaderService', () => { expect(result).toEqual({ shared: 2, stale: 0 }); }); + it('routes each entry by its own format within a single sweep', async () => { + mockPositionConverter.xpointerPointToCfi.mockResolvedValue({ status: 'exact', cfi: 'epubcfi(/6/14!/4/2/6)' }); + + await service.applyBulkProgress( + 7, + [ + { bookFile: bookFile(10, 100, 'pdf'), percentage: 0.5, progress: '84' }, + { bookFile: bookFile(11, 110, 'epub'), percentage: 0.2, progress: '/body/DocFragment[3]' }, + { bookFile: bookFile(12, 120, 'cbz'), percentage: 0.9, progress: '31' }, + ], + device, + ); + + const written = new Map( + mockRepo.upsertReadingProgress.mock.calls.map((call) => { + const entry = call[0] as { bookFileId: number }; + return [entry.bookFileId, entry]; + }), + ); + + expect(written.get(10)).toEqual(expect.objectContaining({ pageNumber: 84, cfi: null })); + expect(written.get(11)).toEqual(expect.objectContaining({ pageNumber: null, cfi: 'epubcfi(/6/14!/4/2/6)' })); + expect(written.get(12)).toEqual(expect.objectContaining({ pageNumber: 31, cfi: null })); + }); + it('issues no query for an empty entry list', async () => { await expect(service.applyBulkProgress(7, [], device)).resolves.toEqual({ shared: 0, stale: 0 }); @@ -550,7 +642,7 @@ describe('KoreaderService', () => { expect(mockBookService.autoUpdateReadStatusForProgress).toHaveBeenCalledWith( 7, - { id: 10, bookId: 100, libraryId: 1 }, + { id: 10, bookId: 100, libraryId: 1, format: 'epub' }, 5, expect.objectContaining({ origin: 'koreader', strongRereadEvidence: true }), ); diff --git a/server/src/modules/koreader/koreader.service.ts b/server/src/modules/koreader/koreader.service.ts index 0a74cd063..81e926250 100644 --- a/server/src/modules/koreader/koreader.service.ts +++ b/server/src/modules/koreader/koreader.service.ts @@ -13,6 +13,7 @@ import { KoreaderChapterExtractorService } from './koreader-chapter-extractor.se import { KoreaderPackageService } from './koreader-package.service'; import { pluginRequiresManualUpdate } from './koreader-plugin-update.util'; import { KoreaderPluginRepository } from './koreader-plugin.repository'; +import { isPagedReadingFormat, parseKoreaderPageNumber } from './koreader-progress-position.util'; import { BookService } from '../book/book.service'; import { PositionConverterService } from '../position-converter/position-converter.service'; import { AchievementEventsService, ACHIEVEMENT_EVENT_BOOK_PROGRESS_CHANGED } from '../achievement/achievement-events.service'; @@ -28,8 +29,16 @@ const FILE_NAMING_CACHE_MAX_ENTRIES = 5_000; const SHARED_PROGRESS_CONCURRENCY = 4; const CHAPTER_EXTRACTION_CONCURRENCY = 2; +/** `format` routes the incoming position to the cfi or the pageNumber column. */ +export interface KoreaderProgressBookFile { + id: number; + bookId: number; + libraryId: number; + format: string | null; +} + export interface BulkProgressEntry { - bookFile: { id: number; bookId: number; libraryId: number }; + bookFile: KoreaderProgressBookFile; percentage: number; progress?: string; timestamp?: number; @@ -160,7 +169,7 @@ export class KoreaderService { async applyProgressForResolvedFile( userId: number, - bookFile: { id: number; bookId: number; libraryId: number }, + bookFile: KoreaderProgressBookFile, data: { percentage: number; progress?: string; device: string; deviceId: string; timestamp?: number }, options?: { skipSharedProgress?: boolean }, ) { @@ -287,13 +296,25 @@ export class KoreaderService { private async applySharedProgress( userId: number, - bookFile: { id: number; bookId: number; libraryId: number }, + bookFile: KoreaderProgressBookFile, data: { percentage: number; progress?: string; timestamp?: number }, previousPercentage: number | null, ) { const bookorbitPercentage = toBookorbitPercentage(data.percentage); - const cfi = data.progress ? await this.convertProgressToCfi(bookFile.id, data.progress) : null; - await this.repo.upsertReadingProgress(bookFile.id, userId, bookorbitPercentage, cfi, data.progress ?? null); + // KOReader reports a paged document's position as a page number and a reflowable one's as + // an xpointer, and the web reader resumes from a different column for each, so the format + // decides which one this position becomes. + const paged = isPagedReadingFormat(bookFile.format); + const cfi = !paged && data.progress ? await this.convertProgressToCfi(bookFile.id, data.progress) : null; + const pageNumber = paged ? parseKoreaderPageNumber(data.progress) : null; + await this.repo.upsertReadingProgress({ + bookFileId: bookFile.id, + userId, + percentage: bookorbitPercentage, + cfi, + xpointer: data.progress ?? null, + pageNumber, + }); await this.bookService.syncKoboReadingStateForExternalProgress(userId, bookFile.id, bookorbitPercentage).catch(() => undefined); const strongRereadEvidence = previousPercentage !== null && previousPercentage - bookorbitPercentage >= 10; await this.bookService.autoUpdateReadStatusForProgress(userId, bookFile, bookorbitPercentage, { diff --git a/server/test/koreader-progress-position.e2e-spec.ts b/server/test/koreader-progress-position.e2e-spec.ts new file mode 100644 index 000000000..117d0c0a9 --- /dev/null +++ b/server/test/koreader-progress-position.e2e-spec.ts @@ -0,0 +1,180 @@ +import { randomUUID } from 'crypto'; +import { eq } from 'drizzle-orm'; + +import * as schema from '../src/db/schema'; +import { createCbzFixture, createEpubFixture } from './e2e/reader-state-isolation/reader-state-isolation-fixture-builder'; +import { + authHeader, + closeReaderStateIsolationE2EContext, + createLibraryWithFolder, + createReaderStateIsolationE2EContext, + locateBookByAbsolutePath, + triggerAndWaitForLibraryScan, + type CreatedLibrary, + type LocatedBookFile, + type ReaderStateIsolationE2EContext, +} from './e2e/reader-state-isolation/reader-state-isolation-harness'; + +const KOREADER_USERNAME = `progress-position-device-${randomUUID().slice(0, 8)}`; +const KOREADER_PASSWORD = 'ProgressPositionPass123'; +const XPOINTER = '/body/DocFragment[8]/body/p[12]/text().0'; + +/** + * KOReader reports a paged document's position as a page number and a reflowable one's as an + * xpointer. Both arrive on the same endpoint, so these cases assert the position lands in the + * column the web reader actually resumes from for that format, against a real database. + */ +describe('KOReader progress position routing (e2e)', { timeout: 180_000 }, () => { + let ctx!: ReaderStateIsolationE2EContext; + let library!: CreatedLibrary; + let comic!: LocatedBookFile; + let epub!: LocatedBookFile; + let comicHash!: string; + let epubHash!: string; + + function deviceHeaders(): Record { + return { 'x-auth-user': KOREADER_USERNAME, 'x-auth-key': KOREADER_PASSWORD }; + } + + async function fileHashFor(bookFileId: number): Promise { + const [row] = await ctx.db.select({ fileHash: schema.bookFiles.fileHash }).from(schema.bookFiles).where(eq(schema.bookFiles.id, bookFileId)); + expect(row?.fileHash).toBeTruthy(); + return row!.fileHash!; + } + + async function syncFromDevice(hash: string, percentage: number, progress?: string | number) { + const response = await ctx.app.inject({ + method: 'PUT', + url: '/api/v1/koreader/syncs/progress', + headers: deviceHeaders(), + payload: { document: hash, percentage, ...(progress === undefined ? {} : { progress }) }, + }); + expect(response.statusCode).toBe(200); + } + + async function storedProgress(bookFileId: number) { + const [row] = await ctx.db + .select({ + percentage: schema.readingProgress.percentage, + cfi: schema.readingProgress.cfi, + pageNumber: schema.readingProgress.pageNumber, + koreaderProgress: schema.readingProgress.koreaderProgress, + }) + .from(schema.readingProgress) + .where(eq(schema.readingProgress.bookFileId, bookFileId)); + return row; + } + + /** What the web reader loads when it opens the book. */ + async function readerProgress(bookFileId: number) { + const response = await ctx.app.inject({ + method: 'GET', + url: `/api/v1/books/files/${bookFileId}/progress`, + headers: authHeader(ctx.adminToken), + }); + expect(response.statusCode).toBe(200); + return response.json() as { percentage: number; cfi: string | null; pageNumber: number | null }; + } + + async function saveFromWebReader(bookFileId: number, payload: Record) { + const response = await ctx.app.inject({ + method: 'POST', + url: `/api/v1/books/files/${bookFileId}/progress`, + headers: authHeader(ctx.adminToken), + payload, + }); + expect([200, 201, 204]).toContain(response.statusCode); + } + + beforeAll(async () => { + ctx = await createReaderStateIsolationE2EContext(); + library = await createLibraryWithFolder(ctx, { name: `koreader-progress-position-${randomUUID()}` }); + + const comicPath = await createCbzFixture(library.folderPath, 'progress-position-comic.cbz', { + title: `Progress Position Comic ${randomUUID()}`, + }); + const epubPath = await createEpubFixture(library.folderPath, 'progress-position-book.epub', { + title: `Progress Position Book ${randomUUID()}`, + uid: `urn:uuid:${randomUUID()}`, + }); + + await triggerAndWaitForLibraryScan(ctx, library.libraryId); + comic = await locateBookByAbsolutePath(ctx, comicPath); + epub = await locateBookByAbsolutePath(ctx, epubPath); + comicHash = await fileHashFor(comic.bookFileId); + epubHash = await fileHashFor(epub.bookFileId); + + const credentials = await ctx.app.inject({ + method: 'POST', + url: '/api/v1/koreader/credentials', + headers: authHeader(ctx.adminToken), + payload: { username: KOREADER_USERNAME, password: KOREADER_PASSWORD }, + }); + expect([200, 201]).toContain(credentials.statusCode); + }, 180_000); + + afterAll(async () => { + if (ctx) await closeReaderStateIsolationE2EContext(ctx); + }); + + it('stores a paged position as a page number the web reader can resume from', async () => { + await syncFromDevice(comicHash, 0.4, '117'); + + const stored = await storedProgress(comic.bookFileId); + expect(stored?.pageNumber).toBe(117); + expect(stored?.cfi).toBeNull(); + expect(stored?.koreaderProgress).toBe('117'); + expect(stored?.percentage).toBeCloseTo(40, 5); + + await expect(readerProgress(comic.bookFileId)).resolves.toEqual(expect.objectContaining({ pageNumber: 117, cfi: null })); + }); + + it('accepts the numeric page KOReader serializes for a paged document', async () => { + await syncFromDevice(comicHash, 0.5, 208); + + expect((await storedProgress(comic.bookFileId))?.pageNumber).toBe(208); + }); + + it('advances the stored page on the next sync of the same book', async () => { + await syncFromDevice(comicHash, 0.6, '242'); + + expect((await storedProgress(comic.bookFileId))?.pageNumber).toBe(242); + }); + + it('leaves the page column empty for a reflowable document', async () => { + await syncFromDevice(epubHash, 0.3, XPOINTER); + + const stored = await storedProgress(epub.bookFileId); + expect(stored?.pageNumber).toBeNull(); + expect(stored?.koreaderProgress).toBe(XPOINTER); + expect((await readerProgress(epub.bookFileId)).pageNumber).toBeNull(); + }); + + it('replaces a page saved by the web reader instead of discarding it', async () => { + await saveFromWebReader(comic.bookFileId, { percentage: 12, pageNumber: 34 }); + expect((await storedProgress(comic.bookFileId))?.pageNumber).toBe(34); + + await syncFromDevice(comicHash, 0.55, '221'); + + expect((await storedProgress(comic.bookFileId))?.pageNumber).toBe(221); + }); + + it('clears a stale page when the device sends a position it cannot supply', async () => { + await syncFromDevice(comicHash, 0.7, '300'); + expect((await storedProgress(comic.bookFileId))?.pageNumber).toBe(300); + + await syncFromDevice(comicHash, 0.75); + + const stored = await storedProgress(comic.bookFileId); + expect(stored?.pageNumber).toBeNull(); + expect(stored?.percentage).toBeCloseTo(75, 5); + }); + + it('rejects a page that would overflow the column rather than failing the sync', async () => { + await syncFromDevice(comicHash, 0.8, '99999999999999'); + + const stored = await storedProgress(comic.bookFileId); + expect(stored?.pageNumber).toBeNull(); + expect(stored?.percentage).toBeCloseTo(80, 5); + }); +});