feat(core): add BibleHighlightsRepository by bmanquen · Pull Request #143 · youversion/platform-sdk-kotlin

bmanquen · 2026-06-24T22:11:24Z

Greptile Summary

This PR introduces BibleHighlightsRepository, which coordinates optimistic cache writes with a retrying, account-stamped server sync queue, and upgrades BibleHighlightCache to expose observable state via StateFlow. It also fixes a Ktor DSL bug in updateHighlight where the URL was not being passed to the .put{} builder.

BibleHighlightCache migrates from a mutable list to MutableStateFlow<List<CachedHighlight>>, replaces non-thread-safe collections with ConcurrentHashMap, makes markChapterAsLoading atomic, and adds markHighlightsAsSynced, removeSyncedHighlights, and isHighlightServerBacked to support the new concurrent-load-safe merge logic in applyServerHighlights.
BibleHighlightsRepository wires the cache and API together: writes apply optimistically and enqueue PendingHighlightOperations processed FIFO with exponential backoff, account-change guards, and a flushPendingWrites suspend hook for sign-out. Several races flagged in prior reviews have been addressed.
The test suite is comprehensive, using StandardTestDispatcher and gated CompletableDeferred fakes to exercise timing-sensitive paths.

Confidence Score: 4/5

The core repository and cache changes are well-structured and address the concurrency issues raised in prior reviews, but one write-path defect remains before merging.

When a highlight is added and then immediately recolored before the queue is processed, the ADD and UpdateColor operations land in the same batch. The ADD succeeds but its markHighlightsAsSynced call cannot promote the cache entry (the recolor updated lastModifiedAt to a later timestamp), so isHighlightServerBacked still returns false when UpdateColor is processed — causing a second createHighlight POST for the same reference.

platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt — specifically the processOperation branch for HighlightChange.UpdateColor around the isHighlightServerBacked check.

Important Files Changed

Filename	Overview
platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt	New repository coordinating optimistic cache writes with a retrying, account-stamped server queue. One remaining issue: batching ADD+UpdateColor for the same reference causes a double POST because the ADD's markHighlightsAsSynced won't promote a timestamp-newer entry, leaving isHighlightServerBacked false when UpdateColor is processed.
platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightCache.kt	Migrates observable state to MutableStateFlow, fixes thread safety with ConcurrentHashMap, makes markChapterAsLoading atomic, and adds markHighlightsAsSynced, removeSyncedHighlights, isHighlightServerBacked, and a concurrent-load-safe applyServerHighlights merge.
platform-core/src/test/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepositoryTests.kt	Comprehensive new test suite using StandardTestDispatcher and gated CompletableDeferred fakes. Missing a test for the add-then-recolor double-create path.
platform-core/src/main/java/com/youversion/platform/core/highlights/api/HighlightsEndpoints.kt	Fixes a Ktor DSL call where the URL was not being passed to the .put{} builder — now correctly uses .put(highlightsUrl()) { }.
platform-core/src/main/java/com/youversion/platform/core/utilities/koin/PlatformCoreKoinModules.kt	Registers BibleHighlightsRepository as a singleton in the Koin DI module.

Comments Outside Diff (3)

platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightCache.kt, line 62-69 (link)

markChapterAsLoading still has a check-then-act race

Switching to ConcurrentHashMap.newKeySet() fixes per-operation thread safety, but the contains → add pair is still not atomic. Two coroutines calling markChapterAsLoading concurrently with the same chapter can both see contains return false, then both call add, and both return true — triggering duplicate server loads. KeySetView.add() already returns false when the element was already present (it delegates to putIfAbsent under the hood), so removing the explicit contains check and returning the result of add makes the whole operation atomic in one step.

Prompt To Fix With AI

This is a comment left during a code review.
Path: platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightCache.kt
Line: 62-69

Comment:
**`markChapterAsLoading` still has a check-then-act race**

Switching to `ConcurrentHashMap.newKeySet()` fixes per-operation thread safety, but the `contains` → `add` pair is still not atomic. Two coroutines calling `markChapterAsLoading` concurrently with the same chapter can both see `contains` return `false`, then both call `add`, and both return `true` — triggering duplicate server loads. `KeySetView.add()` already returns `false` when the element was already present (it delegates to `putIfAbsent` under the hood), so removing the explicit `contains` check and returning the result of `add` makes the whole operation atomic in one step.



How can I resolve this? If you propose a fix, please make it concise.

platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt, line 748-753 (link)

Successful delete doesn't clean up a REMOTE_SYNCED entry added by a concurrent server load

removeHighlights physically removes the entry from the cache immediately. If loadChapterFromServer runs while the delete is still queued, applyServerHighlights sees no local pending entry for the deleted reference and re-adds it as REMOTE_SYNCED. When the delete later succeeds here, processOperation calls nothing on the cache for removes — the stale REMOTE_SYNCED row stays forever (until the next chapter reload that happens to clear it). The user sees the highlight disappear, then reappear, then permanently remain after the delete has synced.

A minimal fix is to remove any REMOTE_SYNCED entry for the succeeded references after a successful delete, similar to how markHighlightsAsSynced is called on add/update success.

Prompt To Fix With AI

This is a comment left during a code review.
Path: platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt
Line: 748-753

Comment:
**Successful delete doesn't clean up a REMOTE_SYNCED entry added by a concurrent server load**

`removeHighlights` physically removes the entry from the cache immediately. If `loadChapterFromServer` runs while the delete is still queued, `applyServerHighlights` sees no local pending entry for the deleted reference and re-adds it as `REMOTE_SYNCED`. When the delete later succeeds here, `processOperation` calls nothing on the cache for removes — the stale `REMOTE_SYNCED` row stays forever (until the next chapter reload that happens to clear it). The user sees the highlight disappear, then reappear, then permanently remain after the delete has synced.

A minimal fix is to remove any `REMOTE_SYNCED` entry for the succeeded references after a successful delete, similar to how `markHighlightsAsSynced` is called on add/update success.

How can I resolve this? If you propose a fix, please make it concise.

platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt, line 744-757 (link)

Double POST when addHighlights and updateHighlightColors are queued together for the same reference

When both an ADD and an UpdateColor operation land in the same batch (because the user adds a highlight and immediately recolors it before the queue is processed), the ADD succeeds but markHighlightsAsSynced does NOT promote the cache entry: it was modified by updateHighlightColors after the ADD's timestamp, so !cached.lastModifiedAt.after(notModifiedAfter) is false and the entry stays LOCAL_PENDING_CREATE. The very next iteration then sees isHighlightServerBacked(reference) == false and fires a second api.createHighlight call — sending two POSTs for the same reference. Depending on server behavior this produces a duplicate highlight or a 409 that retries indefinitely.

A targeted fix for processOperation: when the change is UpdateColor and the entry's current state is LOCAL_PENDING_CREATE, treat that reference as if the chapter is still loading (return false to defer) rather than calling createHighlight. Since a pending ADD for the same reference must appear earlier in the timestamp-ordered queue, it will be processed in the next batch and will promote the entry before UpdateColor is retried.

Prompt To Fix With AI

This is a comment left during a code review.
Path: platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt
Line: 744-757

Comment:
**Double POST when `addHighlights` and `updateHighlightColors` are queued together for the same reference**

When both an `ADD` and an `UpdateColor` operation land in the same batch (because the user adds a highlight and immediately recolors it before the queue is processed), the ADD succeeds but `markHighlightsAsSynced` does NOT promote the cache entry: it was modified by `updateHighlightColors` after the ADD's `timestamp`, so `!cached.lastModifiedAt.after(notModifiedAfter)` is false and the entry stays `LOCAL_PENDING_CREATE`. The very next iteration then sees `isHighlightServerBacked(reference)` == false and fires a second `api.createHighlight` call — sending two POSTs for the same reference. Depending on server behavior this produces a duplicate highlight or a 409 that retries indefinitely.

A targeted fix for `processOperation`: when the change is `UpdateColor` and the entry's current state is `LOCAL_PENDING_CREATE`, treat that reference as if the chapter is still loading (return false to defer) rather than calling `createHighlight`. Since a pending ADD for the same reference must appear earlier in the timestamp-ordered queue, it will be processed in the next batch and will promote the entry before `UpdateColor` is retried.

How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
platform-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt:744-757
**Double POST when `addHighlights` and `updateHighlightColors` are queued together for the same reference**

When both an `ADD` and an `UpdateColor` operation land in the same batch (because the user adds a highlight and immediately recolors it before the queue is processed), the ADD succeeds but `markHighlightsAsSynced` does NOT promote the cache entry: it was modified by `updateHighlightColors` after the ADD's `timestamp`, so `!cached.lastModifiedAt.after(notModifiedAfter)` is false and the entry stays `LOCAL_PENDING_CREATE`. The very next iteration then sees `isHighlightServerBacked(reference)` == false and fires a second `api.createHighlight` call — sending two POSTs for the same reference. Depending on server behavior this produces a duplicate highlight or a 409 that retries indefinitely.

A targeted fix for `processOperation`: when the change is `UpdateColor` and the entry's current state is `LOCAL_PENDING_CREATE`, treat that reference as if the chapter is still loading (return false to defer) rather than calling `createHighlight`. Since a pending ADD for the same reference must appear earlier in the timestamp-ordered queue, it will be processed in the next batch and will promote the entry before `UpdateColor` is retried.

_{Reviews (12): Last reviewed commit: "fix(core): clean up re-added highlight a..." | Re-trigger Greptile}

- updateHighlight built the URL in a discarded expression, so the PUT was issued with no URL and never reached the highlights endpoint - Pass the URL to put() as create/delete already do - Add a test asserting the PUT method, path, and request body

- Back the cache with a MutableStateFlow so UI layers can collect changes - Rewrite mutations as immutable-copy updates - Leave the synchronous overlapping-query and throttling APIs unchanged - Add a test asserting the StateFlow emits on add/remove

- Add an observable domain orchestrator coordinating highlights between the local cache and the highlights API - Apply optimistic cache writes on add/remove/update - Fetch highlights per chapter from the server (throttled) with API-to-domain conversion - Queue writes to the server with capped exponential-backoff retry - Register the repository as a Koin single - Collapse the Swift SDK's repository + view-model responsibilities into one core class, since Kotlin ViewModels are UI-layer only

- Retry only the references that actually failed in a partial-success operation instead of re-sending already-succeeded ones, avoiding duplicate ADDs and redundant REMOVE 404 retry loops. - Cancel in-flight sync work and drain pending operations in reset() so queued writes never land on the previous user's account after sign-out. - Document that forceReload only bypasses the time-based throttle and does not cancel or re-trigger an in-flight chapter load.

camrun91

looks good to me I only have one comment from claude

…-1187 Narrow the window in which a user's queued highlight writes could reach another account, align reset() with the Swift SDK, and give consumers control and visibility over the sync queue. - Bind each PendingHighlightOperation to the account that created it (via YouVersionApi.users.currentUserId, stable across token refresh) and drop it at send time if the signed-in account has changed. This makes cross-account writes very unlikely rather than impossible: auth is attached at the HTTP layer at send time, so a switch racing an in-flight batch is still a (narrow) residual window pending per-request auth. - Change reset() to clear only the cache and per-chapter load state, leaving the queue to drain — mirroring the Swift SDK and removing the prior clear-vs-enqueue race that silently dropped writes. reset() now cancels in-flight chapter loads (via a dedicated loadScope) and loadChapterFromServer bails before applying if cancelled, so a load can no longer repopulate the cache after sign-out. - Add flushPendingWrites(), an awaitable drain so callers can flush queued writes while the current account is still authenticated (e.g. before sign-out). It loops until the queue is observed empty so a write enqueued as the processor finishes is not missed. - Expose pendingOperationCount and failedOperationCount as StateFlows for sync indicators.

camrun91

looks good a few things that cluade caught.

camrun91 · 2026-06-26T20:07:03Z

+                    val failedReferences =
+                        try {
+                            processOperation(operation)
+                        } catch (e: Exception) {


Claude caught this and it looks like it is right
"summary": "Generic catch (e: Exception) in processQueue swallows CancellationException, treating cancellation as a retryable failure",
"failure_scenario": "When the parent scope is cancelled mid-operation (e.g., user signs out), the suspend call inside processOperation throws CancellationException. The catch
at line 271 catches it (CancellationException is-a Exception on JVM), logs it as an error, and returns operation.references as failedReferences. All references are queued for
retry. The actual CancellationException surfaces only at the next suspension point (queueMutex.withLock or delay), leaving the queue in a corrupted retry-filled state.
loadChapterFromServer (line 353) already does this correctly with an explicit CancellationException guard; processQueue does not."

camrun91 · 2026-06-26T20:16:27Z

+                        current
+                    }
+                if (batch.isEmpty()) {
+                    break


This looks like it is a legit concern as well.
"summary": "Race between processQueue's empty-batch break (line 263) and isProcessingQueue=false reset (line 306) can permanently strand a newly enqueued operation",
"failure_scenario": "processQueue holds the mutex, finds pendingOperations empty, clears it, releases the mutex, and is about to hit break at line 263. Concurrently on
Dispatchers.Default, a queueOperation coroutine adds a new item and calls ensureProcessing(). ensureProcessing() acquires the mutex, sees isProcessingQueue=true, and returns the
dying processingJob. processQueue then executes break and finally resets the flag. The queueOperation coroutine joins the now-completed job and exits. The new item sits in
pendingOperations with no processor running, never sent to the server, until another write happens to re-arm the queue."

Align BibleHighlightsRepository's sync queue with the Swift SDK and fix two concurrency defects surfaced in review. - Retry failed writes indefinitely with backoff (remove the 5-retry cap) to match Swift, which has no cap. - Port per-operation result tracking to mirror Swift's surface: OperationResult plus operationResult(), clearOperationResults(), and retryFailedOperations(). failedOperationCount now derives from these results instead of retryCount. - Fix a lost-wakeup race: observe the empty queue and reset isProcessingQueue atomically, so a write enqueued as the processor exits cannot be stranded with no processor running. - Rethrow CancellationException in processQueue instead of logging scope cancellation as a retryable failure, matching loadChapterFromServer. - Correct retryFailedOperations/flushPendingWrites KDoc for the no-cap behavior. Add tests for unbounded retry past the old cap and for clearOperationResults.

…E retries YPE-1187 - Replace the HighlightOperationType enum + nullable color field with a sealed HighlightChange type: Add and UpdateColor carry a non-null color, Remove carries none, so a colorless add/recolor is unconstructible - Drop hexWithoutHash's null-to-empty-string fallback so no queued write can send a blank color to the server - Make the processOperation when() exhaustive over the sealed type - Generalize FakeHighlightsApi so failure injection covers updateHighlight and deleteHighlight - Add retry tests for UPDATE and REMOVE plus basic update/remove send tests

- Recolor of a reference with no cached highlight now syncs as a create instead of a PUT against a highlight the server has never seen: updateHighlightColors partitions references by cache state and queues Add for creates, UpdateColor for updates (matches the cache's create-if-missing behavior; correct since the queue processes in timestamp order) - Add BibleHighlightCache.containsHighlight to drive that split - Replace the HighlightOperationType enum + nullable color field with a sealed HighlightChange type: Add and UpdateColor carry a non-null color, Remove carries none, so a colorless add/recolor is unconstructible - Drop hexWithoutHash's null-to-empty-string fallback so no queued write can send a blank color to the server - Make the processOperation when() exhaustive over the sealed type - Generalize FakeHighlightsApi so failure injection covers updateHighlight and deleteHighlight - Add tests for the create-vs-update split and UPDATE/REMOVE retry paths Note: platform-sdk-swift has the same update-vs-create bug; Kotlin now diverges pending a Swift follow-up.

- Promote synced highlights to REMOTE_SYNCED after a successful ADD/UPDATE so a later server merge replaces them instead of leaving a stale pending row beside the server copy (which made the verse appear twice); guarded by lastModifiedAt so an in-flight newer local edit is not clobbered - Recolor of a reference with no cached highlight now syncs as a create instead of a PUT against a highlight the server has never seen: updateHighlightColors partitions references by cache state and queues Add for creates, UpdateColor for updates (correct since the queue processes in timestamp order) - Add BibleHighlightCache.containsHighlight and markHighlightsAsSynced - Replace the HighlightOperationType enum + nullable color field with a sealed HighlightChange type: Add and UpdateColor carry a non-null color, Remove carries none, so a colorless add/recolor is unconstructible - Drop hexWithoutHash's null-to-empty-string fallback so no queued write can send a blank color to the server - Make the processOperation when() exhaustive over the sealed type - Generalize FakeHighlightsApi failure injection to update/delete and add tests for the dedup, create-vs-update split, and UPDATE/REMOVE retry paths Note: platform-sdk-swift has the same update-vs-create and duplicate-row bugs; Kotlin now diverges pending a Swift follow-up.

- Decide recolor create-vs-update (POST vs PUT) when the write reaches the server, from live cache state, and defer while the reference's chapter is still loading — avoids sending a create for a server highlight that a stale enqueue-time snapshot hadn't loaded yet - Promote synced highlights to REMOTE_SYNCED after a successful ADD/UPDATE so a later server merge replaces them instead of leaving a stale pending row beside the server copy (which made the verse appear twice); guarded by lastModifiedAt so an in-flight newer local edit is not clobbered - Replace the HighlightOperationType enum + nullable color field with a sealed HighlightChange type: Add and UpdateColor carry a non-null color, Remove carries none, so a colorless add/recolor is unconstructible - Drop hexWithoutHash's null-to-empty-string fallback so no queued write can send a blank color to the server - Add BibleHighlightCache.isHighlightServerBacked and markHighlightsAsSynced - Generalize FakeHighlightsApi failure injection to update/delete and add tests for the load-race recolor, dedup, create-vs-update, and UPDATE/REMOVE retries

… YPE-1187 - applyServerHighlights removed only REMOTE_SYNCED rows before appending server data, so a highlight returned while a local write for the same reference was in flight was added beside the pending row; once markHighlightsAsSynced promoted the pending row, two REMOTE_SYNCED rows remained and the verse rendered as a double highlight - Skip a server row when a local-pending row for the same reference already exists, keeping the optimistic local entry instead of duplicating it - Flip a pending create to a pending update in that case so the queued write syncs as a PUT (the server is now known to hold the highlight) rather than POSTing a duplicate - Leave lastModifiedAt untouched on the flip so the later promotion to REMOTE_SYNCED still fires - Extend the in-flight-load recolor test to assert the cache holds a single row after the load settles

- removeHighlights deletes the cache row immediately with no marker, so a chapter load in flight during a queued delete re-added the reference as REMOTE_SYNCED; processOperation did nothing on the cache for a successful remove, so that stale row lingered and the highlight reappeared permanently - Add BibleHighlightCache.removeSyncedHighlights to drop REMOTE_SYNCED rows for the given references, leaving LOCAL_PENDING_* rows so a re-add made after the delete is not lost - Dispatch the processOperation success cleanup by change type: Add/UpdateColor promote via markHighlightsAsSynced, Remove clears via removeSyncedHighlights - Add a deleteGate to FakeHighlightsApi and a test covering a concurrent load re-adding the row before the delete finishes

github-actions · 2026-07-02T20:11:29Z

Code Coverage Report

Overall Coverage: 83.9%

Generated from Kover XML report.

bmanquen added 3 commits June 24, 2026 17:09

greptile-apps Bot reviewed Jun 24, 2026

View reviewed changes

bmanquen force-pushed the bm/YPE-1187-kotlin-core-implement-bible-highlights-repository branch from b6e3d2f to b83386b Compare June 25, 2026 17:28

fix(core): make markChapterAsLoading atomic to prevent duplicate loads

1d8ed48

bmanquen requested a review from camrun91 June 25, 2026 17:59

bmanquen changed the base branch from main to feat/add-highlights June 25, 2026 18:06

camrun91 reviewed Jun 25, 2026

View reviewed changes

Comment thread ...re/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt

greptile-apps Bot reviewed Jun 26, 2026

View reviewed changes

Comment thread ...re/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt

bmanquen force-pushed the bm/YPE-1187-kotlin-core-implement-bible-highlights-repository branch from 9c5308e to 7207982 Compare June 26, 2026 18:33

camrun91 requested changes Jun 26, 2026

View reviewed changes

bmanquen added 4 commits July 2, 2026 09:53

greptile-apps Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread ...re/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightsRepository.kt Outdated

greptile-apps Bot reviewed Jul 2, 2026

View reviewed changes

Comment thread ...orm-core/src/main/java/com/youversion/platform/core/highlights/domain/BibleHighlightCache.kt Outdated

bmanquen added 2 commits July 2, 2026 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(core): add BibleHighlightsRepository#143

feat(core): add BibleHighlightsRepository#143
bmanquen wants to merge 13 commits into
feat/add-highlightsfrom
bm/YPE-1187-kotlin-core-implement-bible-highlights-repository

bmanquen commented Jun 24, 2026 •

edited by greptile-apps Bot

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camrun91 left a comment

Uh oh!

Uh oh!

Uh oh!

camrun91 left a comment

Uh oh!

camrun91 Jun 26, 2026

Uh oh!

camrun91 Jun 26, 2026

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bmanquen commented Jun 24, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Comments Outside Diff (3)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camrun91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

camrun91 left a comment

Choose a reason for hiding this comment

Uh oh!

camrun91 Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

camrun91 Jun 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jul 2, 2026

Code Coverage Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bmanquen commented Jun 24, 2026 •

edited by greptile-apps Bot

Loading