From ae87c0cd6acf1ee31acfe75bfd0b3627cc3bd9b6 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Thu, 10 Sep 2026 08:10:42 -0400 Subject: [PATCH] fix(sync): let content-equal echoes refresh drifted locators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The LWW skip path compared only annotation content (text, color, note, percentages) — locator columns were not part of 'changed'. A device echo with identical content therefore resolved to skip, and the freshly re-derived canonical CFIs were discarded in the same request that computed them: a highlight whose stored start anchor had been corrupted by the old percentage-exact bug could never heal, because every subsequent echo carried the same text and was skipped before the locator columns were written. Observed live: a push converted the cross-block highlight's anchors correctly (structural, heading to paragraph) yet the row kept its garbage start CFI and the highlight stayed unpaintable on the web. Echo saves now treat a non-empty incoming locator that differs from the stored one as a change (locatorRefresher): empty locators still coalesce (no drift), and once healed the echo produces identical CFIs, so the steady state remains skip — no write churn. Highlights check epubcfi_start/end; bookmarks check cfi_position/position. applyBookmarkLWW also gains the empty-locator coalescing the highlight path already had: web bookmark edits carry no device locators, and a title/note edit must not wipe the stored device-native position. --- internal/sync/annotations.go | 40 ++++++++++++++-- internal/sync/annotations_test.go | 79 +++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 4 deletions(-) diff --git a/internal/sync/annotations.go b/internal/sync/annotations.go index ff8beb9..caa789e 100644 --- a/internal/sync/annotations.go +++ b/internal/sync/annotations.go @@ -279,7 +279,9 @@ func (s *AnnotationService) compareIncoming(req SaveHighlightRequest, existing d textEq(req.Color, existing.Color) && textEq(req.NoteText, existing.NoteText) && floatEq(req.PercentageStart, existing.PercentageStart) && - floatEq(req.PercentageEnd, existing.PercentageEnd) + floatEq(req.PercentageEnd, existing.PercentageEnd) && + locatorRefresher(req.EpubcfiStart, existing.EpubcfiStart) && + locatorRefresher(req.EpubcfiEnd, existing.EpubcfiEnd) return !contentSame, !contentSame } @@ -290,6 +292,22 @@ func (s *AnnotationService) compareIncoming(req SaveHighlightRequest, existing d return req.ModifiedAt.After(existingMod.Time), true } +// locatorRefresher reports whether an incoming locator leaves the stored +// one untouched: either the push carried none (empty coalesces to the +// stored value in applyLWW) or it matches what is stored. A non-empty +// incoming locator that DIFFERS is a deliberate refresh: device echoes +// re-derive their canonical CFIs on every push, and an improvement (a +// converter fix re-landing a corrupted anchor, drift repair) must reach +// the row even when the annotation content is otherwise identical — +// without this, content-equal echoes resolve to "skip" and a bad stored +// locator can never heal. +func locatorRefresher(incoming string, existing pgtype.Text) bool { + if incoming == "" { + return true + } + return textEq(incoming, existing) +} + func (s *AnnotationService) TombstoneHighlight( ctx context.Context, userID, mediaItemID pgtype.UUID, @@ -744,13 +762,25 @@ func (s *AnnotationService) applyBookmarkLWW(ctx context.Context, req SaveBookma } deviceData := mergeDeviceSyncData(existing.DeviceSyncData, req.Source, req.DeviceSyncData) + // Web saves carry no device locators: keep the stored ones so a web + // title/note edit never wipes the device-native position (mirrors the + // highlight path's coalescing). + cfiPosition := req.CFIPosition + if cfiPosition == "" { + cfiPosition = existing.CfiPosition.String + } + position := req.Position + if position == "" { + position = existing.Position.String + } + bm, err := s.db.UpdateMediaBookmarkForSync(ctx, database.UpdateMediaBookmarkForSyncParams{ ID: existing.ID, PageNumber: pgInt4(req.PageNumber), ChapterNumber: pgInt4(req.ChapterNumber), - CfiPosition: pgText(req.CFIPosition), + CfiPosition: pgText(cfiPosition), Title: req.Title, - Position: pgText(req.Position), + Position: pgText(position), Notes: pgText(req.Notes), PercentageLocation: pgFloat8(req.PercentageLoc), EpubcfiLocation: pgText(req.EpubcfiLocation), @@ -775,7 +805,9 @@ func (s *AnnotationService) applyBookmarkLWW(ctx context.Context, req SaveBookma func (s *AnnotationService) compareIncomingBookmark(req SaveBookmarkRequest, existing database.MediaBookmarks) (incomingNewer bool, contentChanged bool) { if req.ModifiedAt.IsZero() { contentSame := strings.EqualFold(req.Title, existing.Title) && - textEq(req.Notes, existing.Notes) + textEq(req.Notes, existing.Notes) && + locatorRefresher(req.CFIPosition, existing.CfiPosition) && + locatorRefresher(req.Position, existing.Position) return !contentSame, !contentSame } existingMod := existing.LastModifiedAt diff --git a/internal/sync/annotations_test.go b/internal/sync/annotations_test.go index 2bc8e6e..9fca6a2 100644 --- a/internal/sync/annotations_test.go +++ b/internal/sync/annotations_test.go @@ -369,3 +369,82 @@ func TestIncomingNewerThanTombstone(t *testing.T) { }) } } + +// Content-equal device echoes must still count as changed when they carry +// a locator that differs from the stored one: echoes re-derive canonical +// CFIs on every push, and a better conversion (or a repair of a corrupted +// anchor) has to reach the row — otherwise the skip path discards it and +// the bad locator can never heal. +func TestCompareIncomingLocatorDriftRefreshes(t *testing.T) { + svc := &AnnotationService{} + existing := database.MediaHighlights{ + SelectionText: "CHAPTER IV. “What a pity it is, Elinor,”", + Color: pgtype.Text{String: "#90caf9", Valid: true}, + EpubcfiStart: pgtype.Text{String: "epubcfi(/6/12!/4/2[x]/32/3:576)", Valid: true}, + EpubcfiEnd: pgtype.Text{String: "epubcfi(/6/12!/4/2[x]/4/1:93)", Valid: true}, + } + base := SaveHighlightRequest{ + SelectionText: existing.SelectionText, + Color: "#90caf9", + } + + t.Run("identical content and locators skip", func(t *testing.T) { + req := base + req.EpubcfiStart = existing.EpubcfiStart.String + req.EpubcfiEnd = existing.EpubcfiEnd.String + _, changed := svc.compareIncoming(req, existing) + if changed { + t.Error("identical echo must not rewrite the row") + } + }) + + t.Run("better start locator refreshes despite identical content", func(t *testing.T) { + req := base + req.EpubcfiStart = "epubcfi(/6/12!/4/2[x]/2/3:0)" + req.EpubcfiEnd = existing.EpubcfiEnd.String + _, changed := svc.compareIncoming(req, existing) + if !changed { + t.Error("locator drift on a content-equal echo must trigger an update") + } + }) + + t.Run("empty incoming locators leave the row alone", func(t *testing.T) { + req := base // no CFIs: applyLWW coalesces empty to stored + _, changed := svc.compareIncoming(req, existing) + if changed { + t.Error("empty locators coalesce; must not count as drift") + } + }) +} + +// Bookmarks share the echo-refresh rule for their locators. +func TestCompareIncomingBookmarkLocatorDrift(t *testing.T) { + svc := &AnnotationService{} + existing := database.MediaBookmarks{ + Title: "in CHAPTER IV.", + CfiPosition: pgtype.Text{String: "epubcfi(/6/12!/4/2[x]/2/3:0)", Valid: true}, + Position: pgtype.Text{String: "/body/DocFragment[6]/body/div[1]/h2[1]/text().0", Valid: true}, + } + + t.Run("same locators skip", func(t *testing.T) { + req := SaveBookmarkRequest{ + Title: existing.Title, + CFIPosition: existing.CfiPosition.String, + Position: existing.Position.String, + } + if _, changed := svc.compareIncomingBookmark(req, existing); changed { + t.Error("identical bookmark echo must not rewrite the row") + } + }) + + t.Run("new CFI refreshes despite same title", func(t *testing.T) { + req := SaveBookmarkRequest{ + Title: existing.Title, + CFIPosition: "epubcfi(/6/12!/4/2[x]/2/3:5)", + Position: existing.Position.String, + } + if _, changed := svc.compareIncomingBookmark(req, existing); !changed { + t.Error("bookmark locator drift must trigger an update") + } + }) +}