Skip to content

fix(koreader): web reader to koreader progress sync #1064

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wesamjabali wants to merge 5 commits into
base: develop
Choose a base branch
from
+141 −0
Open

fix(koreader): web reader to koreader progress sync #1064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
30a55a8
fix(koreader): sync web reader progress back to KOReader on update
wesamjabali May 3, 2026
c33b3ef
add tests.
wesamjabali May 3, 2026
c2df8cd
disable sync on global sync flag being false, and allow exception to …
wesamjabali May 3, 2026
ca7a3a6
shift func call to null check
wesamjabali May 3, 2026
d412ced
move to koreader service
wesamjabali May 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions backend/src/main/java/org/booklore/service/koreader/KoreaderService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import org.booklore.exception.ApiError;
import org.booklore.model.dto.progress.KoreaderProgress;
import org.booklore.model.entity.*;
import org.booklore.model.enums.BookFileType;
import org.booklore.model.enums.ReadStatus;
import org.booklore.repository.*;
import org.booklore.service.hardcover.HardcoverSyncService;
Expand Down Expand Up @@ -92,6 +93,40 @@ public void saveProgress(String bookHash, KoreaderProgress koProgress) {
}
}

@Transactional
public void syncProgressToKoreader(Long bookId, Float percentage, Long userId) {
Copy link
Copy Markdown
Contributor

@imnotjames imnotjames May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can unbox these types.

Suggested change
public void syncProgressToKoreader(Long bookId, Float percentage, Long userId) {
public void syncProgressToKoreader(long bookId, float percentage, long userId) {

if (percentage == null) return;

koreaderUserRepository.findByBookLoreUserId(userId)
.filter(KoreaderUserEntity::isSyncEnabled)
.filter(KoreaderUserEntity::isSyncWithBookloreReader)
.ifPresent(koreaderUser -> {
BookEntity book = bookRepository.findById(bookId).orElse(null);
BookLoreUserEntity user = userRepository.findById(userId).orElse(null);
if (book == null || user == null) return;
Copy link
Copy Markdown
Contributor

@imnotjames imnotjames May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably log here - maybe a warn? This probably shouldn't happen.


UserBookProgressEntity progress = getOrCreateUserProgress(user, book);

float koreaderPercent = percentage / 100.0f;
progress.setKoreaderProgressPercent(koreaderPercent);
progress.setKoreaderLastSyncTime(Instant.now());

BookFileEntity primaryFile = book.getPrimaryBookFile();
if (primaryFile != null && primaryFile.getBookType() == BookFileType.EPUB
&& progress.getEpubProgress() != null) {
try {
String xpointer = epubCfiService.convertCfiToProgressXPointer(
book.getFullFilePath(), progress.getEpubProgress());
Copy link
Copy Markdown
Contributor

@imnotjames imnotjames May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using the book full file path we should be explicit and use the primaryFile full file path, right? That way if we ever have to change the book file to be something else (eg, we know a different file is being used by koreader) it's less churn to update it.

progress.setKoreaderProgress(xpointer);
Copy link
Copy Markdown
Contributor

@imnotjames imnotjames May 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if this fails to convert and the koreader progress progress is mismatched with koreader progress percent?

If that's a mismatch causes failure elsewhere, should we try to do the conversion first and then bail if it doesn't work as expected?

} catch (Exception e) {
log.warn("Failed to convert CFI to XPointer for KOReader sync", e);
}
}
Comment on lines +117 to +124
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Stale koreaderProgress value survives CFI conversion failure.

When convertCfiToProgressXPointer throws, the catch block only logs — the existing koreaderProgress (set by a previous Koreader→Booklore sync) is left intact. On the next KOReader pull, it will resume at the old position rather than the new one derived from the updated percent.

This was flagged in the previous review ("Clear KOReader sync state when CFI conversion fails") and was not addressed when the logic was moved here.

🐛 Proposed fix 
                         } catch (Exception e) {
+                            progress.setKoreaderProgress(null);
                             log.warn("Failed to convert CFI to XPointer for KOReader sync", e);
                         }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@backend/src/main/java/org/booklore/service/koreader/KoreaderService.java`
around lines 117 - 124, The catch block in KoreaderService around
epubCfiService.convertCfiToProgressXPointer currently only logs the exception,
leaving any previous koreaderProgress intact; modify the catch to clear or reset
the progress.koreaderProgress (call progress.setKoreaderProgress(null) or
equivalent) so stale KOReader state is not preserved when conversion fails,
ensuring the next KOReader pull uses the updated percent; keep the log.warn call
but add the reset logic referencing convertCfiToProgressXPointer and
progress.setKoreaderProgress in the same catch.


progressRepository.save(progress);
});
}
Comment on lines +96 to +128
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Synchronous I/O on the request thread and unconstrained transaction coupling.

syncProgressToKoreader runs entirely on the HTTP request thread, including the epubCfiService.convertCfiToProgressXPointer call which reads the EPUB file from disk. Because the method uses the default REQUIRED propagation, it joins updateReadProgress's transaction — any uncaught exception here marks the caller's transaction for rollback, which would silently undo the user's reading-progress write at line 272 in ReadingProgressService.

HardcoverSyncService.syncProgressToHardcover (called immediately after) is @Async @transactional(readOnly = true), which decouples fault isolation and keeps the request thread clean. The KOReader sync should follow the same pattern.

As per coding guidelines, virtual threads / async dispatch should be preferred for I/O-bound work.

♻️ Proposed alignment with HardcoverSyncService pattern 
+import org.springframework.scheduling.annotation.Async;

-    `@Transactional`
+    `@Async`
+    `@Transactional`(propagation = Propagation.REQUIRES_NEW)
     public void syncProgressToKoreader(Long bookId, Float percentage, Long userId) {

Using REQUIRES_NEW ensures the KOReader sync has its own transaction that can be rolled back without affecting the caller. @Async moves the I/O work off the request thread and keeps updateReadProgress's latency unaffected.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@backend/src/main/java/org/booklore/service/koreader/KoreaderService.java`
around lines 96 - 128, The syncProgressToKoreader method is doing blocking I/O
on the request thread and runs in the caller's transaction; change its execution
to be isolated and asynchronous by annotating syncProgressToKoreader with `@Async`
and `@Transactional`(propagation = Propagation.REQUIRES_NEW) (import Propagation),
so its work (including koreaderUserRepository.findByBookLoreUserId,
epubCfiService.convertCfiToProgressXPointer and progressRepository.save) runs in
a new transaction off the request thread; also ensure any unchecked exceptions
inside the method are caught and logged (so failures do not bubble up and mark
the caller transaction for rollback).


private void saveToFileProgress(BookLoreUserEntity user, BookEntity book, UserBookProgressEntity progress) {
try {
BookFileEntity primaryFile = book.getPrimaryBookFile();
Expand Down
3 changes: 3 additions & 0 deletions backend/src/main/java/org/booklore/service/progress/ReadingProgressService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.booklore.repository.*;
import org.booklore.service.hardcover.HardcoverSyncService;
import org.booklore.service.kobo.KoboReadingStateService;
import org.booklore.service.koreader.KoreaderService;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.security.core.userdetails.UsernameNotFoundException;
Expand All @@ -46,6 +47,7 @@ public class ReadingProgressService {
private final UserRepository userRepository;
private final AuthenticationService authenticationService;
private final KoboReadingStateService koboReadingStateService;
private final KoreaderService koreaderService;
private final HardcoverSyncService hardcoverSyncService;

// ==================== Methods from UserProgressService ====================
Expand Down Expand Up @@ -270,6 +272,7 @@ public void updateReadProgress(ReadProgressRequest request) {
userBookProgressRepository.save(progress);

if (percentage != null) {
koreaderService.syncProgressToKoreader(book.getId(), percentage, user.getId());
hardcoverSyncService.syncProgressToHardcover(book.getId(), percentage, user.getId());
}
}
Expand Down
103 changes: 103 additions & 0 deletions backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.booklore.repository.*;
import org.booklore.service.kobo.KoboReadingStateService;
import org.booklore.service.hardcover.HardcoverSyncService;
import org.booklore.service.koreader.KoreaderService;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.InjectMocks;
Expand Down Expand Up @@ -46,6 +47,8 @@ class ReadingProgressServiceTest {
private KoboReadingStateService koboReadingStateService;
@Mock
private HardcoverSyncService hardcoverSyncService;
@Mock
private KoreaderService koreaderService;

@InjectMocks
private ReadingProgressService readingProgressService;
Expand Down Expand Up @@ -286,6 +289,106 @@ void updateReadProgress_pdf_shouldSaveProgress() {
assertEquals(50f, progress.getPdfProgressPercent());
}

@Test
void updateReadProgress_shouldDelegateKoreaderSync() {
long bookId = 1L;
BookEntity book = new BookEntity();
book.setId(bookId);
BookFileEntity primaryFile = new BookFileEntity();
primaryFile.setId(1L);
primaryFile.setBook(book);
primaryFile.setBookType(BookFileType.EPUB);
book.setBookFiles(List.of(primaryFile));

BookLoreUser user = mock(BookLoreUser.class);
when(user.getId()).thenReturn(2L);
when(authenticationService.getAuthenticatedUser()).thenReturn(user);
when(bookRepository.findByIdWithBookFiles(bookId)).thenReturn(Optional.of(book));

BookLoreUserEntity userEntity = new BookLoreUserEntity();
userEntity.setId(2L);
when(userRepository.findById(2L)).thenReturn(Optional.of(userEntity));

UserBookProgressEntity progress = new UserBookProgressEntity();
when(userBookProgressRepository.findByUserIdAndBookId(2L, bookId)).thenReturn(Optional.of(progress));

when(userBookFileProgressRepository.findByUserIdAndBookFileId(2L, 1L)).thenReturn(Optional.empty());

ReadProgressRequest req = new ReadProgressRequest();
req.setBookId(bookId);
EpubProgress epubProgress = EpubProgress.builder().cfi("cfi").percentage(100f).build();
req.setEpubProgress(epubProgress);

readingProgressService.updateReadProgress(req);

verify(koreaderService).syncProgressToKoreader(bookId, 100f, 2L);
}
Comment on lines +292 to +325
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Missing test for the CFI-conversion exception path.

When epubCfiService.convertCfiToProgressXPointer throws, the service logs a warning and continues — so koreaderProgressPercent and koreaderLastSyncTime should still be written, but koreaderProgress should remain null. This partial-sync behaviour isn't covered by any of the three new tests.

✅ Suggested test skeleton 
`@Test`
void updateReadProgress_shouldSyncPercentButNotXPointerWhenCfiConversionFails() {
    // same book/user/koreaderUser setup as shouldSyncToKoreaderWhenEnabled...
    when(epubCfiService.convertCfiToProgressXPointer(any(Path.class), eq("cfi")))
            .thenThrow(new RuntimeException("conversion failed"));

    // ... fire updateReadProgress ...

    assertEquals(1.0f, progress.getKoreaderProgressPercent());
    assertNull(progress.getKoreaderProgress());     // XPointer not set on failure
    assertNotNull(progress.getKoreaderLastSyncTime()); // sync time still recorded
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java`
around lines 296 - 348, Add a new unit test (e.g.,
updateReadProgress_shouldSyncPercentButNotXPointerWhenCfiConversionFails) that
reuses the same Book/BookFile/BookLoreUser/KoreaderUser setup from
updateReadProgress_shouldSyncToKoreaderWhenEnabled, but mock
epubCfiService.convertCfiToProgressXPointer(any(Path.class), eq("cfi")) to throw
a RuntimeException; call readingProgressService.updateReadProgress(req) and
assert that progress.getKoreaderProgressPercent() equals 1.0f,
progress.getKoreaderProgress() is null, and progress.getKoreaderLastSyncTime()
is not null, and verify convertCfiToProgressXPointer was invoked.


@Test
void updateReadProgress_shouldDelegateKoreaderSyncWithCorrectPercentage() {
long bookId = 1L;
BookEntity book = new BookEntity();
book.setId(bookId);
BookFileEntity primaryFile = new BookFileEntity();
primaryFile.setId(1L);
primaryFile.setBook(book);
primaryFile.setBookType(BookFileType.PDF);
book.setBookFiles(List.of(primaryFile));

BookLoreUser user = mock(BookLoreUser.class);
when(user.getId()).thenReturn(2L);
when(authenticationService.getAuthenticatedUser()).thenReturn(user);
when(bookRepository.findByIdWithBookFiles(bookId)).thenReturn(Optional.of(book));

BookLoreUserEntity userEntity = new BookLoreUserEntity();
userEntity.setId(2L);
when(userRepository.findById(2L)).thenReturn(Optional.of(userEntity));

UserBookProgressEntity progress = new UserBookProgressEntity();
when(userBookProgressRepository.findByUserIdAndBookId(2L, bookId)).thenReturn(Optional.of(progress));

when(userBookFileProgressRepository.findByUserIdAndBookFileId(2L, 1L)).thenReturn(Optional.empty());

ReadProgressRequest req = new ReadProgressRequest();
req.setBookId(bookId);
PdfProgress pdfProgress = PdfProgress.builder().page(10).percentage(50f).build();
req.setPdfProgress(pdfProgress);

readingProgressService.updateReadProgress(req);

verify(koreaderService).syncProgressToKoreader(bookId, 50f, 2L);
}

@Test
void updateReadProgress_shouldNotDelegateKoreaderSyncWhenPercentageNull() {
long bookId = 1L;
BookEntity book = new BookEntity();
book.setId(bookId);
BookFileEntity primaryFile = new BookFileEntity();
primaryFile.setId(1L);
primaryFile.setBook(book);
primaryFile.setBookType(BookFileType.EPUB);
book.setBookFiles(List.of(primaryFile));

BookLoreUser user = mock(BookLoreUser.class);
when(user.getId()).thenReturn(2L);
when(authenticationService.getAuthenticatedUser()).thenReturn(user);
when(bookRepository.findByIdWithBookFiles(bookId)).thenReturn(Optional.of(book));

BookLoreUserEntity userEntity = new BookLoreUserEntity();
userEntity.setId(2L);
when(userRepository.findById(2L)).thenReturn(Optional.of(userEntity));

when(userBookProgressRepository.findByUserIdAndBookId(2L, bookId)).thenReturn(Optional.empty());

ReadProgressRequest req = new ReadProgressRequest();
req.setBookId(bookId);

readingProgressService.updateReadProgress(req);

verify(koreaderService, never()).syncProgressToKoreader(anyLong(), anyFloat(), anyLong());
}
Comment on lines +292 to +390
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Three new delegation tests cover the happy path and the no-op path, but KoreaderService.syncProgressToKoreader logic is entirely untested.

The new tests correctly verify that ReadingProgressService delegates to KoreaderService for both EPUB/PDF and skips when percentage is null. However, the logic now inside KoreaderService.syncProgressToKoreader — sync toggle checks, getOrCreateUserProgress, CFI→XPointer conversion, and the exception path (stale koreaderProgress) — has no unit tests at all. A previous review comment flagged the CFI-exception path as missing test coverage; that gap still exists, now against KoreaderService directly.

Key scenarios that should be covered in a KoreaderServiceTest:

  • isSyncEnabled = false → progress repository never written
  • isSyncEnabled = true, isSyncWithBookloreReader = false → repository never written
  • isSyncEnabled = true, isSyncWithBookloreReader = true, book is EPUB with CFI → XPointer written
  • convertCfiToProgressXPointer throws → koreaderProgress is null, percent and timestamp still written (once the stale-clear fix is applied)
  • Book or user not found → no-op

Would you like me to draft a KoreaderServiceTest covering these scenarios?

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@backend/src/test/java/org/booklore/service/progress/ReadingProgressServiceTest.java`
around lines 292 - 390, Add unit tests for
KoreaderService.syncProgressToKoreader to cover the missing logic: write tests
that toggle isSyncEnabled()/isSyncWithBookloreReader() to assert the progress
repositories are never written when disabled, test the EPUB path where
convertCfiToProgressXPointer(...) returns an xpointer so the XPointer is saved
via getOrCreateUserProgress(...) and persisted, add a test where
convertCfiToProgressXPointer(...) throws to assert koreaderProgress is cleared
(null) but percentage and timestamp still get written, and add no-op tests when
book or user lookups fail; target the KoreaderService class and its methods
syncProgressToKoreader, convertCfiToProgressXPointer, getOrCreateUserProgress
and verify interactions with the koreaderProgress repository and user progress
persistence accordingly.


@Test
void resetProgress_booklore_shouldCallBulkReset() {
BookLoreUser user = mock(BookLoreUser.class);
Expand Down
Loading