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") + } + }) +}