fix(sync): let content-equal echoes refresh drifted locators
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.
This commit is contained in:
@@ -279,7 +279,9 @@ func (s *AnnotationService) compareIncoming(req SaveHighlightRequest, existing d
|
|||||||
textEq(req.Color, existing.Color) &&
|
textEq(req.Color, existing.Color) &&
|
||||||
textEq(req.NoteText, existing.NoteText) &&
|
textEq(req.NoteText, existing.NoteText) &&
|
||||||
floatEq(req.PercentageStart, existing.PercentageStart) &&
|
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
|
return !contentSame, !contentSame
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -290,6 +292,22 @@ func (s *AnnotationService) compareIncoming(req SaveHighlightRequest, existing d
|
|||||||
return req.ModifiedAt.After(existingMod.Time), true
|
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(
|
func (s *AnnotationService) TombstoneHighlight(
|
||||||
ctx context.Context,
|
ctx context.Context,
|
||||||
userID, mediaItemID pgtype.UUID,
|
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)
|
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{
|
bm, err := s.db.UpdateMediaBookmarkForSync(ctx, database.UpdateMediaBookmarkForSyncParams{
|
||||||
ID: existing.ID,
|
ID: existing.ID,
|
||||||
PageNumber: pgInt4(req.PageNumber),
|
PageNumber: pgInt4(req.PageNumber),
|
||||||
ChapterNumber: pgInt4(req.ChapterNumber),
|
ChapterNumber: pgInt4(req.ChapterNumber),
|
||||||
CfiPosition: pgText(req.CFIPosition),
|
CfiPosition: pgText(cfiPosition),
|
||||||
Title: req.Title,
|
Title: req.Title,
|
||||||
Position: pgText(req.Position),
|
Position: pgText(position),
|
||||||
Notes: pgText(req.Notes),
|
Notes: pgText(req.Notes),
|
||||||
PercentageLocation: pgFloat8(req.PercentageLoc),
|
PercentageLocation: pgFloat8(req.PercentageLoc),
|
||||||
EpubcfiLocation: pgText(req.EpubcfiLocation),
|
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) {
|
func (s *AnnotationService) compareIncomingBookmark(req SaveBookmarkRequest, existing database.MediaBookmarks) (incomingNewer bool, contentChanged bool) {
|
||||||
if req.ModifiedAt.IsZero() {
|
if req.ModifiedAt.IsZero() {
|
||||||
contentSame := strings.EqualFold(req.Title, existing.Title) &&
|
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
|
return !contentSame, !contentSame
|
||||||
}
|
}
|
||||||
existingMod := existing.LastModifiedAt
|
existingMod := existing.LastModifiedAt
|
||||||
|
|||||||
@@ -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")
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user