From 60ad70ff5cacf5d435e06db9e999b8f5d67430f2 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Sat, 17 Oct 2026 00:45:21 +0000 Subject: [PATCH] fix(kobo): trust a confirmed replacement for positions and annotations A replacement stayed ambiguous after the user confirmed it, so every position the Kobo sent was discarded and annotation pulls failed with 409. Confirming now makes it the device's copy and moves the stored bookmark onto it. It still cannot drag saved progress backwards, and that is judged by position, since its percentages run on a different scale. Ref #1786 --- .../kobo-annotation-exchange.service.ts | 4 +- .../services/kobo-delivery.service.test.ts | 47 +++++++- .../kobo/services/kobo-delivery.service.ts | 16 +++ .../services/kobo-kepub-context.service.ts | 4 +- .../services/kobo-progress-bridge.service.ts | 5 + .../kobo-reading-state.service.test.ts | 109 ++++++++++++++---- .../services/kobo-reading-state.service.ts | 63 ++++++---- .../position-converter/cfi.utils.test.ts | 20 ++++ .../modules/position-converter/cfi.utils.ts | 17 +++ .../position-converter.service.ts | 7 +- server/test/read-aloud-delivery.e2e-spec.ts | 3 +- 11 files changed, 244 insertions(+), 51 deletions(-) create mode 100644 server/src/modules/position-converter/cfi.utils.test.ts diff --git a/server/src/modules/kobo/services/kobo-annotation-exchange.service.ts b/server/src/modules/kobo/services/kobo-annotation-exchange.service.ts index f6e38848d..175ae1537 100644 --- a/server/src/modules/kobo/services/kobo-annotation-exchange.service.ts +++ b/server/src/modules/kobo/services/kobo-annotation-exchange.service.ts @@ -8,7 +8,7 @@ import { AnnotationSyncService, formatDeviceDatetime, type IncomingDeviceAnnotat import { AnnotationConversionService } from '../../annotation/annotation-conversion.service'; import { KoboSpanConverterService, serializeKoboSpanPos } from '../../position-converter/kobo-span-converter.service'; import { parseSpanSelector } from '../../position-converter/kobo-span.core'; -import { KoboDeliveryService } from './kobo-delivery.service'; +import { KoboDeliveryService, isUnconfirmedReplacement } from './kobo-delivery.service'; import { KoboBookIdentityService } from './kobo-book-identity.service'; import { KoboAnnotationMaterializerService } from './kobo-annotation-materializer.service'; import { KoboKepubContextService } from './kobo-kepub-context.service'; @@ -236,7 +236,7 @@ export class KoboAnnotationExchangeService { deliveryId: delivery?.id ?? null, sourceRevision: delivery?.metadata.sourceRevision ?? null, artifactId: delivery?.metadata.artifactId ?? null, - uncertainDelivery: !kepub.ok || !!delivery?.previousId || !!delivery?.options.replacement, + uncertainDelivery: !kepub.ok || (!!delivery && isUnconfirmedReplacement(delivery)), }; return annotation; }); diff --git a/server/src/modules/kobo/services/kobo-delivery.service.test.ts b/server/src/modules/kobo/services/kobo-delivery.service.test.ts index d7cb3b187..afd40c2f9 100644 --- a/server/src/modules/kobo/services/kobo-delivery.service.test.ts +++ b/server/src/modules/kobo/services/kobo-delivery.service.test.ts @@ -1,7 +1,13 @@ import { ConflictException, ForbiddenException, NotFoundException } from '@nestjs/common'; import { describe, expect, it, vi } from 'vitest'; -import { KoboDeliveryService, deliveryErrorCode, type KoboDeliveryRecord } from './kobo-delivery.service'; +import { + KoboDeliveryService, + deliveryErrorCode, + isSettledDelivery, + isUnconfirmedReplacement, + type KoboDeliveryRecord, +} from './kobo-delivery.service'; function record(overrides: Partial = {}): KoboDeliveryRecord { return { @@ -239,3 +245,42 @@ describe('KoboDeliveryService.cancelReplacement', () => { expect(confirmed.repository.reoffer).not.toHaveBeenCalled(); }); }); + +describe('delivery standing', () => { + it('settles a first delivery on transfer and a replacement only on confirmation', () => { + expect(isSettledDelivery(record({ state: 'transfer_observed' }))).toBe(true); + expect(isSettledDelivery(record({ state: 'offered' }))).toBe(false); + expect(isSettledDelivery(replacement({ state: 'transfer_observed', previousId: 'previous' }))).toBe(false); + expect(isSettledDelivery(replacement({ state: 'user_confirmed', previousId: 'previous' }))).toBe(true); + expect(isUnconfirmedReplacement(replacement({ state: 'transfer_observed', previousId: 'previous' }))).toBe(true); + expect(isUnconfirmedReplacement(replacement({ state: 'user_confirmed', previousId: 'previous' }))).toBe(false); + expect(isUnconfirmedReplacement(record({ state: 'transfer_observed' }))).toBe(false); + }); +}); + +describe('KoboDeliveryService.confirm', () => { + it('moves the device bookmark onto the replacement the user confirmed', async () => { + const transferred = replacement({ state: 'transfer_observed', previousId: 'previous' }); + const repository = { + findById: vi.fn().mockResolvedValue(transferred), + assertDevice: vi.fn().mockResolvedValue(undefined), + findCurrent: vi.fn().mockResolvedValue(transferred), + transition: vi.fn().mockResolvedValue([{}]), + adoptForDeviceReadingState: vi.fn().mockResolvedValue(undefined), + }; + const service = new KoboDeliveryService( + repository as never, + {} as never, + { assertBookAccessible: vi.fn().mockResolvedValue(undefined) } as never, + {} as never, + {} as never, + {} as never, + {} as never, + ); + + await service.confirm(1, 2, 'replacement'); + + expect(repository.transition).toHaveBeenCalledWith(1, 2, 'replacement', ['transfer_observed', 'user_confirmed'], 'user_confirmed'); + expect(repository.adoptForDeviceReadingState).toHaveBeenCalledWith(1, 2, 3, 'replacement'); + }); +}); diff --git a/server/src/modules/kobo/services/kobo-delivery.service.ts b/server/src/modules/kobo/services/kobo-delivery.service.ts index 8c3e8fff6..853862ba3 100644 --- a/server/src/modules/kobo/services/kobo-delivery.service.ts +++ b/server/src/modules/kobo/services/kobo-delivery.service.ts @@ -17,6 +17,21 @@ export type KoboDeliveryRecord = NonNullable; + +/** + * A replacement leaves the device's old copy in doubt until the user confirms they removed it and + * downloaded the new one, since a completed transfer does not prove the device switched copies. + */ +export function isUnconfirmedReplacement(delivery: DeliveryStanding): boolean { + return (Boolean(delivery.previousId) || delivery.options.replacement === true) && delivery.state !== 'user_confirmed'; +} + +/** Whether the device is known to hold this delivery's bytes, so the positions it reports can be trusted. */ +export function isSettledDelivery(delivery: DeliveryStanding): boolean { + return delivery.state === 'user_confirmed' || (delivery.state === 'transfer_observed' && !isUnconfirmedReplacement(delivery)); +} + export function deliveryErrorCode(error: unknown): string | null { if (!(error instanceof HttpException)) return null; const response = error.getResponse(); @@ -265,6 +280,7 @@ export class KoboDeliveryService { if (!['transfer_observed', 'user_confirmed'].includes(record.state)) throw new ConflictException({ code: 'KOBO_REPLACEMENT_TRANSFER_NOT_OBSERVED' }); await this.repository.transition(userId, deviceId, id, ['transfer_observed', 'user_confirmed'], 'user_confirmed'); + await this.repository.adoptForDeviceReadingState(userId, deviceId, record.bookId, id); return this.repository.findById(userId, deviceId, id); } diff --git a/server/src/modules/kobo/services/kobo-kepub-context.service.ts b/server/src/modules/kobo/services/kobo-kepub-context.service.ts index 7f0bcc5e5..399bc5861 100644 --- a/server/src/modules/kobo/services/kobo-kepub-context.service.ts +++ b/server/src/modules/kobo/services/kobo-kepub-context.service.ts @@ -1,6 +1,6 @@ import { Injectable, Optional } from '@nestjs/common'; -import { KoboDeliveryService } from './kobo-delivery.service'; +import { KoboDeliveryService, isSettledDelivery } from './kobo-delivery.service'; import { DeliveryArtifactService } from '../../book/delivery-artifact.service'; import { KepubContext } from '../../position-converter/kobo-span-converter.service'; import { BookReadService } from '../../book/book-read.service'; @@ -75,7 +75,7 @@ export class KoboKepubContextService { if (record && record.state === 'prepared' && record.previousId) return this.resolveForDelivery(userId, bookId, deviceId, record.previousId); if (!record || record.state === 'offered' || record.state === 'prepared' || record.state === 'cancelled') return { ok: false, reason: 'unknown_delivery', settings }; - if (record.previousId || record.options.replacement) return { ok: false, reason: 'replacement_events_ambiguous', settings }; + if (!isSettledDelivery(record)) return { ok: false, reason: 'replacement_events_ambiguous', settings }; if (record.metadata.format !== 'kepub') return { ok: false, reason: 'kepub_required', settings }; try { const artifact = await this.deliveries.resolve(userId, deviceId, record); diff --git a/server/src/modules/kobo/services/kobo-progress-bridge.service.ts b/server/src/modules/kobo/services/kobo-progress-bridge.service.ts index 3d2ff8e1c..c7af5a291 100644 --- a/server/src/modules/kobo/services/kobo-progress-bridge.service.ts +++ b/server/src/modules/kobo/services/kobo-progress-bridge.service.ts @@ -33,6 +33,11 @@ export class KoboProgressBridgeService { private readonly positionConverter: PositionConverterService, ) {} + /** Orders two canonical positions: negative when `a` comes first, null when either cannot be read. */ + comparePositions(a: string, b: string): number | null { + return this.positionConverter.compareCfi(a, b); + } + /** * Places a narration marker on the device. A read-along save can hold the sentence being narrated * without a text position, and a bookmark sent as a bare percent opens the book at its start. diff --git a/server/src/modules/kobo/services/kobo-reading-state.service.test.ts b/server/src/modules/kobo/services/kobo-reading-state.service.test.ts index 0f5d44dcb..762db723f 100644 --- a/server/src/modules/kobo/services/kobo-reading-state.service.test.ts +++ b/server/src/modules/kobo/services/kobo-reading-state.service.test.ts @@ -64,7 +64,12 @@ describe('KoboReadingStateService', () => { const bookAccessService = { assertBookAccessible: vi.fn() }; const userBookStatusService = { autoUpdate: vi.fn() }; const bookIdentityService = { ensureForBook: vi.fn() }; - const progressBridge = { koboBookmarkToCanonical: vi.fn(), cfiToKoboBookmark: vi.fn(), narrationToKoboBookmark: vi.fn() }; + const progressBridge = { + koboBookmarkToCanonical: vi.fn(), + cfiToKoboBookmark: vi.fn(), + narrationToKoboBookmark: vi.fn(), + comparePositions: vi.fn(), + }; const settingsService = { getSettings: vi.fn() }; const achievementEvents = { emit: vi.fn() }; const analyticsResolver = { resolveBookFileId: vi.fn() }; @@ -724,26 +729,23 @@ describe('KoboReadingStateService', () => { return { db, insert }; } const resetPayload = { CurrentBookmark: { ProgressPercent: 0, LastModified: incomingAt } }; - it.each(['offered', 'transfer_observed', 'user_confirmed', 'cancelled'])( - 'retains canonical progress and raw events during %s replacement', - async (state) => { - const { db, insert } = resetDb(); - const deliveries = { current: vi.fn().mockResolvedValue({ id: 'new', previousId: 'old', state, options: { replacement: true } }) }; - await expect(makeService(db, deliveries).upsertState(1, 5, resetPayload, 1, 99, true, 77)).resolves.toEqual(ACK('entitlement-5', 'Success')); - expect(db.insert).toHaveBeenCalledTimes(1); - expect(insert.values).toHaveBeenCalledWith( - expect.objectContaining({ - deviceId: 77, - deliveryId: 'old', - currentBookmark: resetPayload.CurrentBookmark, - }), - ); - expect(userBookStatusService.autoUpdate).not.toHaveBeenCalled(); - expect(bookService.syncAudioProgressForExternalEbookProgress).not.toHaveBeenCalled(); - expect(achievementEvents.emit).not.toHaveBeenCalled(); - expect(db.execute).toHaveBeenCalledTimes(1); - }, - ); + it.each(['offered', 'transfer_observed', 'cancelled'])('retains canonical progress and raw events during %s replacement', async (state) => { + const { db, insert } = resetDb(); + const deliveries = { current: vi.fn().mockResolvedValue({ id: 'new', previousId: 'old', state, options: { replacement: true } }) }; + await expect(makeService(db, deliveries).upsertState(1, 5, resetPayload, 1, 99, true, 77)).resolves.toEqual(ACK('entitlement-5', 'Success')); + expect(db.insert).toHaveBeenCalledTimes(1); + expect(insert.values).toHaveBeenCalledWith( + expect.objectContaining({ + deviceId: 77, + deliveryId: 'old', + currentBookmark: resetPayload.CurrentBookmark, + }), + ); + expect(userBookStatusService.autoUpdate).not.toHaveBeenCalled(); + expect(bookService.syncAudioProgressForExternalEbookProgress).not.toHaveBeenCalled(); + expect(achievementEvents.emit).not.toHaveBeenCalled(); + expect(db.execute).toHaveBeenCalledTimes(1); + }); it('does not accept an initial zero echo for an unknown device copy', async () => { const { db } = resetDb(); await makeService(db, { current: vi.fn().mockResolvedValue(null) }).upsertState(1, 5, resetPayload, 1, 99, true, 77); @@ -803,7 +805,7 @@ describe('KoboReadingStateService', () => { it('accepts forward percentage progress without decoding an ambiguous replacement location', async () => { const { db, insert } = resetDb(); await makeService(db, { - current: vi.fn().mockResolvedValue({ id: 'new', previousId: 'old', state: 'user_confirmed', options: { replacement: true } }), + current: vi.fn().mockResolvedValue({ id: 'new', previousId: 'old', state: 'transfer_observed', options: { replacement: true } }), }).upsertState( 1, 5, @@ -822,6 +824,69 @@ describe('KoboReadingStateService', () => { expect(insert.values).toHaveBeenCalledWith(expect.objectContaining({ bookFileId: 55, percentage: 65, koboLocationValue: null })); expect(progressBridge.koboBookmarkToCanonical).not.toHaveBeenCalled(); }); + it('still holds saved progress against a backwards push from a confirmed replacement', async () => { + const { db, insert } = resetDb(); + await makeService(db, { + current: vi.fn().mockResolvedValue({ id: 'new', previousId: 'old', state: 'user_confirmed', options: { replacement: true } }), + }).upsertState(1, 5, { CurrentBookmark: { ProgressPercent: 3, LastModified: incomingAt } }, 1, 99, true, 77); + expect(insert.values).not.toHaveBeenCalledWith(expect.objectContaining({ bookFileId: 55 })); + expect(insert.values).toHaveBeenCalledWith(expect.objectContaining({ deviceId: 77, deliveryId: 'new' })); + expect(userBookStatusService.autoUpdate).not.toHaveBeenCalled(); + }); + it.each([ + ['accepts', 1, 58], + ['holds back', -1, 61], + ])('%s a confirmed replacement push whose position is later or earlier than its lower percentage suggests', async (_label, order, saved) => { + const { db, insert } = resetDb(); + progressBridge.koboBookmarkToCanonical.mockResolvedValue({ cfi: 'epubcfi(/6/4!/4/2/1:0)', xpointer: '/body/p[2]' }); + progressBridge.comparePositions.mockReturnValue(order); + await makeService(db, { + current: vi.fn().mockResolvedValue({ id: 'new', previousId: 'old', state: 'user_confirmed', options: { replacement: true } }), + }).upsertState( + 1, + 5, + { + CurrentBookmark: { + ProgressPercent: 58, + LastModified: incomingAt, + Location: { Type: 'KoboSpan', Source: 'chapter.xhtml', Value: 'kobo.43.4' }, + }, + }, + 1, + 99, + true, + 77, + ); + expect(progressBridge.comparePositions).toHaveBeenCalledWith('epubcfi(/6/4!/4/2/1:0)', savedCfi); + if (saved === 58) + expect(insert.values).toHaveBeenCalledWith( + expect.objectContaining({ bookFileId: 55, percentage: 58, cfi: 'epubcfi(/6/4!/4/2/1:0)', koboLocationValue: 'kobo.43.4' }), + ); + else expect(insert.values).not.toHaveBeenCalledWith(expect.objectContaining({ bookFileId: 55 })); + }); + it('trusts a confirmed replacement, even while the device bookmark still names the copy it replaced', async () => { + const { db, insert } = resetDb(); + await makeService(db, { + current: vi.fn().mockResolvedValue({ id: 'new', previousId: 'old', state: 'user_confirmed', options: { replacement: true } }), + }).upsertState( + 1, + 5, + { + CurrentBookmark: { + ProgressPercent: 65, + LastModified: incomingAt, + Location: { Type: 'KoboSpan', Source: 'chapter.xhtml', Value: 'kobo.1.1' }, + }, + }, + 1, + 99, + true, + 77, + ); + expect(progressBridge.koboBookmarkToCanonical).toHaveBeenCalledWith(1, 5, 'chapter.xhtml', 'kobo.1.1', 77); + expect(insert.values).toHaveBeenCalledWith(expect.objectContaining({ bookFileId: 55, percentage: 65, koboLocationValue: 'kobo.1.1' })); + expect(insert.values).toHaveBeenCalledWith(expect.objectContaining({ deviceId: 77, deliveryId: 'new' })); + }); it('acknowledges an equal-percent echo without reopening a synced device', async () => { const { db } = resetDb(); await makeService(db, { current: vi.fn().mockResolvedValue({ id: 'new', state: 'offered', options: {} }) }).upsertState( diff --git a/server/src/modules/kobo/services/kobo-reading-state.service.ts b/server/src/modules/kobo/services/kobo-reading-state.service.ts index 02c24719d..47c0acca6 100644 --- a/server/src/modules/kobo/services/kobo-reading-state.service.ts +++ b/server/src/modules/kobo/services/kobo-reading-state.service.ts @@ -17,10 +17,10 @@ import { } from '../kobo-statistics-session.util'; import { KoboAnalyticsResolverService } from './kobo-analytics-resolver.service'; import { KoboReadingProgressRepository } from '../kobo-reading-progress.repository'; -import { KoboDeliveryService } from './kobo-delivery.service'; +import { KoboDeliveryService, isSettledDelivery, isUnconfirmedReplacement } from './kobo-delivery.service'; import { KoboBookAccessService } from './kobo-book-access.service'; import { KoboBookIdentityService } from './kobo-book-identity.service'; -import { KoboProgressBridgeService } from './kobo-progress-bridge.service'; +import { KoboProgressBridgeService, type CanonicalProgressPoint } from './kobo-progress-bridge.service'; import { KoboSettingsService } from './kobo-settings.service'; import { advanceIsoTimestamp, maxIsoTimestamp } from '../../../common/utils/iso-timestamp.utils'; import { sanitizeLogValue } from '../../../common/utils/log-sanitize.utils'; @@ -157,19 +157,50 @@ export class KoboReadingStateService { const delivery = await this.deliveries?.current(userId, sourceDeviceId, bookId); const preparedPrevious = delivery?.state === 'prepared' ? delivery.previousId : null; const deliveryId = preparedPrevious ?? delivery?.id ?? null; + // Confirming a replacement moves the stored bookmark onto it, but one confirmed before that did + // still points at the copy it replaced; the confirmation already says the device switched. + const confirmedOverPrevious = delivery?.state === 'user_confirmed' && existing?.deliveryId === delivery.previousId; const uncertainDelivery = Boolean( this.deliveries && (!delivery || - (!preparedPrevious && (delivery.previousId || delivery.options.replacement || delivery.state !== 'transfer_observed')) || - (existing?.deliveryId && existing.deliveryId !== deliveryId)), + (!preparedPrevious && !isSettledDelivery(delivery)) || + (existing?.deliveryId && existing.deliveryId !== deliveryId && !confirmedOverPrevious)), ); let protectedRegression = false; const initialZero = Boolean(this.deliveries && mergedPercent === 0 && (previousPercent === null || previousPercent === 0)); - if ((uncertainDelivery || initialZero) && mergedPercent !== null) { + // A replacement copy opens wherever the device left it after the re-download, often its start, so + // even once confirmed it may only move saved progress forward; its precise locations still count. + const replacedCopy = !preparedPrevious && Boolean(delivery && (delivery.previousId || delivery.options.replacement)); + const locationSource = uncertainDelivery ? null : this.extractKoboLocationSource(mergedBookmark); + const locationType = uncertainDelivery ? null : this.extractKoboLocationType(mergedBookmark); + const locationValue = uncertainDelivery ? null : this.extractKoboLocationValue(mergedBookmark); + // KoboSpan bookmarks convert to precise canonical points; web and KOReader + // resume at the same paragraph instead of a percent approximation. + let precise: CanonicalProgressPoint | null | undefined; + const resolvePrecise = async () => { + if (precise === undefined) + precise = + twoWayProgressSync && locationType === 'KoboSpan' && locationSource && locationValue + ? await this.progressBridge.koboBookmarkToCanonical(userId, bookId, locationSource, locationValue, sourceDeviceId) + : null; + return precise; + }; + let guardBackwards = uncertainDelivery || initialZero || replacedCopy; + if (guardBackwards && mergedPercent !== null) { const fileId = await this.progressRepository.findPrimaryFileId(bookId); const canonical = fileId ? await this.progressRepository.findForFile(userId, fileId) : null; const savedPercent = canonical?.percentage ?? this.extractPercent(this.asJsonObj(shared?.currentBookmark)); protectedRegression = savedPercent !== null && savedPercent > mergedPercent + PROGRESS_EPSILON; + // A replacement copy's percentages run on its own scale, so where it names its position exactly, + // that position, not the percentage, says whether it moved back. + if (protectedRegression && !uncertainDelivery && !initialZero && canonical?.cfi) { + const point = await resolvePrecise(); + const order = point ? this.progressBridge.comparePositions(point.cfi, canonical.cfi) : null; + if (order !== null && order >= 0) { + protectedRegression = false; + guardBackwards = false; + } + } } const mergedTimesStarted = typeof mergedStatus?.TimesStartedReading === 'number' ? mergedStatus.TimesStartedReading : null; const strongRereadEvidence = @@ -182,17 +213,7 @@ export class KoboReadingStateService { const effectivePriority = maxIsoTimestamp(priorityTimestamp, existing?.priorityTimestamp, bookmarkModified) ?? priorityTimestamp; if (!protectedRegression && bookmarkChanged && mergedPercent !== null) { - const locationSource = uncertainDelivery ? null : this.extractKoboLocationSource(mergedBookmark); - const locationType = uncertainDelivery ? null : this.extractKoboLocationType(mergedBookmark); - const locationValue = uncertainDelivery ? null : this.extractKoboLocationValue(mergedBookmark); - - // KoboSpan bookmarks convert to precise canonical points; web and KOReader - // resume at the same paragraph instead of a percent approximation. - const precise = - twoWayProgressSync && locationType === 'KoboSpan' && locationSource && locationValue - ? await this.progressBridge.koboBookmarkToCanonical(userId, bookId, locationSource, locationValue, sourceDeviceId) - : null; - + const point = await resolvePrecise(); protectedRegression = !(await this.syncPercentToInternalProgress( userId, bookId, @@ -202,8 +223,8 @@ export class KoboReadingStateService { locationType, locationValue, uncertainDelivery ? null : this.extractContentSourceProgressPercent(mergedBookmark), - precise, - uncertainDelivery || initialZero, + point, + guardBackwards, )); } @@ -470,9 +491,7 @@ export class KoboReadingStateService { deviceId && this.deliveries && (!delivery || - delivery.previousId || - delivery.options.replacement || - delivery.state !== 'transfer_observed' || + !isSettledDelivery(delivery) || ('deliveryId' in row && row.deliveryId !== delivery.id) || this.extractPercent(this.asJsonObj(row.currentBookmark)) === 0), ); @@ -494,7 +513,7 @@ export class KoboReadingStateService { }; } let rawBookmark = row.currentBookmark; - if (deviceId && (!delivery || delivery.previousId || delivery.options.replacement || ('deliveryId' in row && row.deliveryId !== delivery.id))) { + if (deviceId && (!delivery || isUnconfirmedReplacement(delivery) || ('deliveryId' in row && row.deliveryId !== delivery.id))) { const safe = this.asJsonObj(rawBookmark); if (safe) { const { Location: ignoredLocation, ...rest } = safe; diff --git a/server/src/modules/position-converter/cfi.utils.test.ts b/server/src/modules/position-converter/cfi.utils.test.ts new file mode 100644 index 000000000..a1adfa419 --- /dev/null +++ b/server/src/modules/position-converter/cfi.utils.test.ts @@ -0,0 +1,20 @@ +import { compareCfi } from './cfi.utils'; + +describe('compareCfi', () => { + it('orders positions by chapter, then by path within the chapter, then by character offset', () => { + expect(compareCfi('epubcfi(/6/18!/4/2/168/6/1:0)', 'epubcfi(/6/20!/4/2/14/1:0)')).toBeLessThan(0); + expect(compareCfi('epubcfi(/6/20!/4/2/86/1:0)', 'epubcfi(/6/20!/4/2/82/8/1:92)')).toBeGreaterThan(0); + expect(compareCfi('epubcfi(/6/20!/4/2/82/8/1:10)', 'epubcfi(/6/20!/4/2/82/8/1:92)')).toBeLessThan(0); + expect(compareCfi('epubcfi(/6/20!/4/2/82/8/1:92)', 'epubcfi(/6/20!/4/2/82/8/1:92)')).toBe(0); + }); + + it('compares a range by its start and an ancestor before its descendants', () => { + expect(compareCfi('epubcfi(/6/20!/4/2,/82/8[chapter6-s153]/1:92,/88/1:5)', 'epubcfi(/6/20!/4/2/84/1:0)')).toBeLessThan(0); + expect(compareCfi('epubcfi(/6/20!/4/2/82)', 'epubcfi(/6/20!/4/2/82/8/1:0)')).toBeLessThan(0); + }); + + it('returns null when either position cannot be read', () => { + expect(compareCfi('not a cfi', 'epubcfi(/6/20!/4/2/82/8/1:0)')).toBeNull(); + expect(compareCfi('epubcfi(/4/2/82)', 'epubcfi(/6/20!/4/2/82/8/1:0)')).toBeNull(); + }); +}); diff --git a/server/src/modules/position-converter/cfi.utils.ts b/server/src/modules/position-converter/cfi.utils.ts index ce8ba1569..20c34c86d 100644 --- a/server/src/modules/position-converter/cfi.utils.ts +++ b/server/src/modules/position-converter/cfi.utils.ts @@ -337,6 +337,23 @@ export function parseCfi(cfi: string): ParsedCfiRange | null { }; } +/** + * Orders two CFIs by reading position: negative when `a` comes first, zero when they name the same + * point, null when either cannot be read. Ranges compare by their start. + */ +export function compareCfi(a: string, b: string): number | null { + const left = parseCfi(a); + const right = parseCfi(b); + if (!left || !right || left.spineStep === null || right.spineStep === null) return null; + if (left.spineStep !== right.spineStep) return left.spineStep - right.spineStep; + for (let i = 0; i < Math.min(left.start.length, right.start.length); i++) { + const step = left.start[i].index - right.start[i].index; + if (step !== 0) return step; + } + if (left.start.length !== right.start.length) return left.start.length - right.start.length; + return (left.start[left.start.length - 1]?.offset ?? 0) - (right.start[right.start.length - 1]?.offset ?? 0); +} + export function chapterIndexFromSpineStep(spineStep: number | null): number | null { if (spineStep == null || spineStep % 2 !== 0 || spineStep < 2) return null; return spineStep / 2 - 1; diff --git a/server/src/modules/position-converter/position-converter.service.ts b/server/src/modules/position-converter/position-converter.service.ts index 620e21083..3b2a47461 100644 --- a/server/src/modules/position-converter/position-converter.service.ts +++ b/server/src/modules/position-converter/position-converter.service.ts @@ -1,6 +1,6 @@ import { Injectable } from '@nestjs/common'; -import { chapterIndexFromSpineStep, findElementById, parseCfi } from './cfi.utils'; +import { chapterIndexFromSpineStep, compareCfi, findElementById, parseCfi } from './cfi.utils'; import { EpubDomService } from './epub-dom.service'; import { CfiToXPointerResult, @@ -128,6 +128,11 @@ export class PositionConverterService { return { status: result.status, pos0: result.pos0, pos1: result.pos1, chapterIndex }; } + /** Orders two CFIs by reading position: negative when `a` comes first, null when either cannot be read. */ + compareCfi(a: string, b: string): number | null { + return compareCfi(a, b); + } + async fragmentToPositions(params: { bookFileId: number; chapterIndex: number; diff --git a/server/test/read-aloud-delivery.e2e-spec.ts b/server/test/read-aloud-delivery.e2e-spec.ts index c9a787cc5..fc9e01a26 100644 --- a/server/test/read-aloud-delivery.e2e-spec.ts +++ b/server/test/read-aloud-delivery.e2e-spec.ts @@ -550,7 +550,8 @@ describe('Read-aloud delivery contracts (e2e)', { timeout: 180_000 }, () => { where: and(eq(schema.koboDeviceReadingStates.deviceId, device.id), eq(schema.koboDeviceReadingStates.bookId, book.bookId)), }); expect(raw?.currentBookmark).toMatchObject({ ProgressPercent: 0 }); - expect(raw?.deliveryId).not.toBe(replacement.id); + if (stage === 'user_confirmed') expect(raw?.deliveryId).toBe(replacement.id); + else expect(raw?.deliveryId).not.toBe(replacement.id); const pulled = await (await http('GET', devicePath(device, `library/${book.bookId}/state`), undefined, false)).json(); expect(pulled[0].CurrentBookmark.ProgressPercent).toBe(61); expect(pulled[0].StatusInfo.Status).toBe('Reading');