Skip to content

fix(annotations): pull sweep populates elided view without opening each book - #1300

Merged
pkmetski merged 10 commits into
mainfrom
pkmetski/elided-view-tab-load
Oct 23, 2026
Merged

pkmetski merged 10 commits into
mainfrom
pkmetski/elided-view-tab-load

Conversation

@pkmetski

@pkmetski pkmetski commented Oct 21, 2026 •

Copy link
Copy Markdown
Owner

Root cause

The elided-view (annotations) tab was fed entirely from the local Room DB, but the only code path that pulls peer annotation files from WebDAV/ABS into Room was AnnotationSyncController.syncOnOpen — gated on opening each individual book in the reader. On a fresh device (or after reconfiguring sync), all annotations exist on the sync backend but the tab showed nothing until every annotated book was opened.

Additionally, for ABS-only users, the AnnotationSyncTargetHolder seeds with sources = emptyList() before the init coroutine fires, causing the pull sweep to race startup and see a null target → immediate no-op.

Fix

AnnotationSyncController.pullAll(sourceId, namespace) — new method that:

  • Calls AnnotationSyncTarget.enumerateDevices(namespace) (single PROPFIND, not one per book)
  • Groups peer annotation files by itemId
  • Merges each book's peer files via the existing AnnotationMergeOrchestrator.mergeFromListing LWW policy
  • Holds per-book ReconcileLocks so the pull never interleaves with a concurrent live push
  • Skips this device's own file (already in Room)

AnnotationPullSweep — new pull-only sweep class (companion to the push-only AnnotationSweep) that iterates all configured sources and calls pullAll for each.

AnnotationSyncTargetHolder.awaitFirstLoad() — CompletableDeferred that signals when the init coroutine's first source-driven build completes. The pull sweep awaits it before reading current(), eliminating the startup race.

Null→non-null watcher (Android + iOS) — re-runs the pull sweep whenever the holder transitions from null to non-null mid-session (e.g., source added or re-authed), so the tab populates without requiring a restart. Initial value prevNull=false prevents a duplicate sweep at startup.

Wired into the same app-lifecycle triggers as the push sweep:

  • Android: runs on app start + on connectivity return via kickSweepsOnReconnect
  • iOS: runs on app-active + reconnect via ForegroundSyncDriver.runAnnotationSweep

Battery impact

The null→non-null watcher is a StateFlow.collect — idle between user-driven source changes. No polling, no timers. enumerateDevices is a single PROPFIND; the ABS target caches GET /api/me for 3s to prevent N+1 calls.

Code review fixes applied

  • buildTarget wrapped in try/finally so firstLoad.complete(Unit) always fires even if a factory throws (prevents awaitFirstLoad from deadlocking callers)
  • prevNull initialised to false so startup null→non-null doesn't fire a second concurrent sweep alongside the explicit launch
  • PullRecordingTarget.read() now scopes by itemId ("$itemId/$filename" key with fallback); multi-item test uses per-item scoped keys with distinct annotation IDs so item-routing bugs are detectable

Follow-up (not in this PR)

AnnotationSweep (push) still passes { holder.current() } as its target provider, which returns null for ABS-only users during the brief startup window before the init coroutine fires — pre-existing behaviour, tracked separately.

Performance

One PROPFIND per source (not per book) via enumerateDevices, then N reads for peer files. Runs in background (Android: applicationScope = Dispatchers.IO; iOS: Dispatchers.Default). The tab live-updates as Room rows land.

Tests

  • AnnotationPullSweepTest — 8 scenarios in commonTest: unconfigured target no-op, no-peer no-op, peer annotations merged, multiple items (item-scoped fixture), own-device file skipped, multiple sources, no-namespace source skipped, enumerateDevices failure
  • AnnotationSyncControllerLifecycleTest — 3 new pullAll tests: no-target no-op, peers merged + own skipped, multiple items
  • AnnotationSyncTargetHolderTest (androidHostTest) — 5 new tests: awaitFirstLoad resolves, idempotent, observe() null initial, null→non-null, non-null→null
  • AnnotationSyncTargetHolderStartupTest (commonTest) — 5 mirrored tests running on the iOS simulator via :core:data:iosSimulatorArm64Test

Simulator screenshot evidence

n/a — non-visual (data-layer/sync changes only)

🤖 Generated with Claude Code

…ing each book

The elided-view (annotations) tab was only populated when each book was
individually opened in the reader, because the only code path that fetches
peer annotation files from WebDAV into Room is AnnotationSyncController.syncOnOpen,
which is gated on reader startup.

Add AnnotationPullSweep: a companion to the push-only AnnotationSweep that
pulls ALL peer annotation files for every configured source by calling
AnnotationSyncTarget.enumerateDevices (one PROPFIND per namespace) and merging
each book's peer files via the existing AnnotationMergeOrchestrator.mergeFromListing
LWW policy. This replaces per-book list calls with a single directory traversal.

The pull sweep is wired into the same app-lifecycle triggers as the push sweep:
- Android: runs on app start + on connectivity return (kickSweepsOnReconnect)
- iOS: runs on app-active + reconnect via ForegroundSyncDriver.runAnnotationSweep

AnnotationSyncController gains a pullAll(sourceId, namespace) method that holds
the per-book ReconcileLocks so the pull sweep and a concurrent live push never
interleave on the same device file. Own-device files are excluded from the pull
since they are already in Room (or will be flushed by the push sweep).

Tests: AnnotationPullSweepTest (7 scenarios in commonTest, runs on iOS simulator)
and 3 new pullAll cases in AnnotationSyncControllerLifecycleTest.
AnnotationSyncTargetHolder seeds its state with sources=emptyList()
synchronously (so it starts null for ABS users), then rebuilds asynchronously
in its init block after sourceRepository emits. The pull sweep was launched at
the same time, read null from the holder before the rebuild ran, and exited
immediately — leaving the elided view tab empty until each book was opened.

Fix: add CompletableDeferred<Unit> to the holder that signals after the first
real source-driven build. The pull sweep's targetProvider is now suspend and
awaits that signal via awaitFirstLoad() before reading current(), ensuring it
always runs against the fully-initialized target.

Verified on emulator-5554: 90 annotations pulled on first launch, Annotations
tab visible in library without opening any book.
…ed mid-session

Removing and re-adding an ABS source causes AnnotationSyncTargetHolder to
rebuild (null → non-null), but there was no trigger to re-run the pull sweep
mid-session — only startup and reconnect fired it.

Add observe() to AnnotationSyncTargetHolder exposing the internal StateFlow.
On both Android (RiffleApplication) and iOS (Koin ForegroundSyncDriver block),
watch for null→non-null transitions and run the pull sweep immediately so the
elided view tab populates without a restart after source re-configuration.
…onSyncTargetHolderTest

Covers the startup-race fix and null→non-null transition that drives
the mid-session pull sweep re-run.
…observe() state-machine tests

Mirrors the androidHostTest assertions so the same state-machine behavior
is exercised on the iOS simulator in CI via :core:data:iosSimulatorArm64Test.
…fast test dispatchers

When UnconfinedTestDispatcher runs the holder's init coroutine inline during
construction, firstLoad.complete(Unit) fired before firstLoad was initialized.
Also fix commonTest to use backgroundScope (shared scheduler) and flow-first
awaiting so subsequent state-change tests don't race the scheduler.
…ep at startup, item-scoped test fixture

- Wrap buildTarget in try/finally so firstLoad.complete always fires even
  if the factory throws; prevents awaitFirstLoad from deadlocking callers.
- Change prevNull initial value to false in both Android and iOS watchers
  so the startup null→non-null transition (already handled by the explicit
  pull sweep launch) does not trigger a redundant second concurrent sweep.
- Fix PullRecordingTarget.read() to scope lookup by itemId ("$itemId/$filename")
  with fallback, and update peerAnnotationsAcrossMultipleItemsAreMerged to
  use per-item scoped keys with distinct annotation IDs so item routing bugs
  are detectable.
@pkmetski
pkmetski force-pushed the pkmetski/elided-view-tab-load branch from 5b95493 to 6dfa016 Compare October 23, 2026 07:35
…th inline anonymous object in commonTest

DefaultDispatcherProvider lives in jvmMain and is unavailable on Kotlin/Native, causing
compileTestKotlinIosSimulatorArm64 to fail with "Unresolved reference".
Replace with an inline DispatcherProvider implementation backed by Dispatchers.Default,
which is available on all KMP targets.
Compose form validation propagates asynchronously — asserting isEnabled
immediately after the last keystroke races the recomposition cycle on
loaded CI runners. Add an XCTNSPredicateExpectation (5 s) to let the
enabled state settle before asserting.
The series-list step already carries a 60s budget because slow CI runners
need more than 30s to stream items; the series-detail step had the same
problem but only a 30s budget.  Raising it to 60s gives the same headroom.
@pkmetski
pkmetski merged commit ac4c440 into main Oct 23, 2026
13 checks passed
@pkmetski
pkmetski deleted the pkmetski/elided-view-tab-load branch October 23, 2026 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant