From edb4e606792c2aec3ee2ecdcb7a4c1bee2fc8b81 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sat, 17 Oct 2026 16:03:49 -0400 Subject: [PATCH] =?UTF-8?q?fix(annotations):=20LWW=20applies=20coalesce=20?= =?UTF-8?q?omitted=20fields=20=E2=80=94=20partial=20payloads=20no=20longer?= =?UTF-8?q?=20erase=20stored=20locators?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caught by the live round-trip sweep: a dedup-key re-save (a device echo or a client re-POSTing a bookmark it already had) carried no locator fields, and the full-column sync update wrote zeroes — which pgFloat8/pgInt4 turn into NULLs — erasing the stored percentage/page/cfi on every echo. The highlight stack had string coalescing for exactly this reason (its locatorRefresher doctrine comment); the numeric columns and the two cloned stacks never got it. One primitive now carries the whole doctrine: coalesce[T comparable] (zero value -> stored value), applied at the single write point of each LWW apply — highlight (percentages, color, note_text, chapter_reference added to its existing string coalescing), bookmark (page/chapter/percentage/epubcfi/chapter_reference added to its cfi/position pair), note (all locators). The redundant pre-coalescing in the id-targeted resolution blocks is removed — the apply is the one coalesce point, so dedup-path and id-path edits behave identically. Verified live on the rebuilt container: dedup re-save and legacy 3-field PUTs preserve every stored locator on all three stacks; an explicitly-sent field still lands. Pins, H1/H3 tests, unit suites green. --- internal/sync/annotations.go | 118 +++++++++++++---------------------- 1 file changed, 44 insertions(+), 74 deletions(-) diff --git a/internal/sync/annotations.go b/internal/sync/annotations.go index 2ad0f12b..b012fc91 100644 --- a/internal/sync/annotations.go +++ b/internal/sync/annotations.go @@ -95,6 +95,18 @@ type SaveHighlightResult struct { Conflict bool } +// coalesce returns fallback when v is the zero value — the LWW-apply +// "an omitted field preserves the stored value" primitive. Zeroes matter +// here because pgFloat8/pgInt4 turn them into NULLs: an uncoalesced +// partial payload would erase stored locators on every re-save. +func coalesce[T comparable](v, fallback T) T { + var zero T + if v == zero { + return fallback + } + return v +} + // idTargetOwnership verifies an id-targeted edit's ownership: the URL row // must belong to the requesting user and media item. func idTargetOwnership(rowUser, rowItem, reqUser, reqItem pgtype.UUID, kind string, id pgtype.UUID) error { @@ -238,35 +250,23 @@ func (s *AnnotationService) applyLWW( // xpointer) and may carry no CFI either: keep the stored ones so // device-native serve-back and round-trip identity survive a web-side // note/color edit instead of being wiped to empty. - startPosition := req.StartPosition - if startPosition == "" { - startPosition = existing.StartPosition.String - } - endPosition := req.EndPosition - if endPosition == "" { - endPosition = existing.EndPosition.String - } - epubcfiStart := req.EpubcfiStart - if epubcfiStart == "" { - epubcfiStart = existing.EpubcfiStart.String - } - epubcfiEnd := req.EpubcfiEnd - if epubcfiEnd == "" { - epubcfiEnd = existing.EpubcfiEnd.String - } + startPosition := coalesce(req.StartPosition, existing.StartPosition.String) + endPosition := coalesce(req.EndPosition, existing.EndPosition.String) + epubcfiStart := coalesce(req.EpubcfiStart, existing.EpubcfiStart.String) + epubcfiEnd := coalesce(req.EpubcfiEnd, existing.EpubcfiEnd.String) highlight, err := s.db.UpdateMediaHighlightForSync(ctx, database.UpdateMediaHighlightForSyncParams{ ID: existing.ID, SelectionText: req.SelectionText, StartPosition: pgText(startPosition), EndPosition: pgText(endPosition), - Color: pgText(req.Color), - NoteText: pgText(req.NoteText), - PercentageStart: pgFloat8(req.PercentageStart), - PercentageEnd: pgFloat8(req.PercentageEnd), + Color: pgText(coalesce(req.Color, existing.Color.String)), + NoteText: pgText(coalesce(req.NoteText, existing.NoteText.String)), + PercentageStart: pgFloat8(coalesce(req.PercentageStart, existing.PercentageStart.Float64)), + PercentageEnd: pgFloat8(coalesce(req.PercentageEnd, existing.PercentageEnd.Float64)), EpubcfiStart: pgText(epubcfiStart), EpubcfiEnd: pgText(epubcfiEnd), - ChapterReference: pgInt4(req.ChapterReference), + ChapterReference: pgInt4(coalesce(req.ChapterReference, existing.ChapterReference.Int32)), LastModifiedAt: pgtype.Timestamptz{Time: modifiedAt, Valid: true}, LastModifiedSource: pgtype.Text{String: req.Source, Valid: req.Source != ""}, DeviceSyncData: deviceData, @@ -561,24 +561,6 @@ func (s *AnnotationService) SaveNote(ctx context.Context, req SaveNoteRequest) ( if dedupKey == "" { dedupKey = byID.DedupKey.String } - if req.EpubcfiLocation == "" { - req.EpubcfiLocation = byID.EpubcfiLocation.String - } - if req.PercentageLocation == 0 { - req.PercentageLocation = byID.PercentageLocation.Float64 - } - if req.CharacterStart == 0 { - req.CharacterStart = byID.CharacterStart.Int32 - } - if req.CharacterEnd == 0 { - req.CharacterEnd = byID.CharacterEnd.Int32 - } - if req.ChapterReference == 0 { - req.ChapterReference = byID.ChapterReference.Int32 - } - if req.ParagraphReference == 0 { - req.ParagraphReference = byID.ParagraphReference.Int32 - } if tombstoneWins(byID.Deleted.Bool, byID.DeletedAt, byID.LastModifiedAt, req.ModifiedAt) { return &SaveNoteResult{Note: byID, Outcome: SaveOutcomeDeleted}, nil } @@ -663,17 +645,21 @@ func (s *AnnotationService) applyNoteLWW(ctx context.Context, req SaveNoteReques deviceData := mergeDeviceSyncData(existing.DeviceSyncData, req.Source, req.DeviceSyncData) + // Coalesce omitted fields with the stored row (same doctrine as the + // highlight apply): a partial payload — a web PUT carrying only + // content/position, a device echo without percentages — must not + // wipe the locators it didn't mention (see coalesce). note, err := s.db.UpdateMediaNoteForSync(ctx, database.UpdateMediaNoteForSyncParams{ ID: existing.ID, UserID: req.UserID, Content: req.Content, - Position: pgText(req.Position), - PercentageLocation: pgFloat8(req.PercentageLocation), - CharacterStart: pgInt4(req.CharacterStart), - CharacterEnd: pgInt4(req.CharacterEnd), - EpubcfiLocation: pgText(req.EpubcfiLocation), - ChapterReference: pgInt4(req.ChapterReference), - ParagraphReference: pgInt4(req.ParagraphReference), + Position: pgText(coalesce(req.Position, existing.Position.String)), + PercentageLocation: pgFloat8(coalesce(req.PercentageLocation, existing.PercentageLocation.Float64)), + CharacterStart: pgInt4(coalesce(req.CharacterStart, existing.CharacterStart.Int32)), + CharacterEnd: pgInt4(coalesce(req.CharacterEnd, existing.CharacterEnd.Int32)), + EpubcfiLocation: pgText(coalesce(req.EpubcfiLocation, existing.EpubcfiLocation.String)), + ChapterReference: pgInt4(coalesce(req.ChapterReference, existing.ChapterReference.Int32)), + ParagraphReference: pgInt4(coalesce(req.ParagraphReference, existing.ParagraphReference.Int32)), LastModifiedAt: pgtype.Timestamptz{Time: modifiedAt, Valid: true}, LastModifiedSource: pgtype.Text{String: req.Source, Valid: req.Source != ""}, DeviceSyncData: deviceData, @@ -778,24 +764,6 @@ func (s *AnnotationService) SaveBookmark(ctx context.Context, req SaveBookmarkRe if dedupKey == "" { dedupKey = byID.DedupKey.String } - if req.CFIPosition == "" { - req.CFIPosition = byID.CfiPosition.String - } - if req.EpubcfiLocation == "" { - req.EpubcfiLocation = byID.EpubcfiLocation.String - } - if req.PercentageLoc == 0 { - req.PercentageLoc = byID.PercentageLocation.Float64 - } - if req.PageNumber == 0 { - req.PageNumber = byID.PageNumber.Int32 - } - if req.ChapterNumber == 0 { - req.ChapterNumber = byID.ChapterNumber.Int32 - } - if req.ChapterReference == 0 { - req.ChapterReference = byID.ChapterReference.Int32 - } if tombstoneWins(byID.Deleted.Bool, byID.DeletedAt, byID.LastModifiedAt, req.ModifiedAt) { return &SaveBookmarkResult{Bookmark: byID, Outcome: SaveOutcomeDeleted}, nil } @@ -898,16 +866,18 @@ func (s *AnnotationService) applyBookmarkLWW(ctx context.Context, req SaveBookma } bm, err := s.db.UpdateMediaBookmarkForSync(ctx, database.UpdateMediaBookmarkForSyncParams{ - ID: existing.ID, - PageNumber: pgInt4(req.PageNumber), - ChapterNumber: pgInt4(req.ChapterNumber), - CfiPosition: pgText(cfiPosition), - Title: req.Title, - Position: pgText(position), - Notes: pgText(req.Notes), - PercentageLocation: pgFloat8(req.PercentageLoc), - EpubcfiLocation: pgText(req.EpubcfiLocation), - ChapterReference: pgInt4(req.ChapterReference), + ID: existing.ID, + PageNumber: pgInt4(coalesce(req.PageNumber, existing.PageNumber.Int32)), + ChapterNumber: pgInt4(coalesce(req.ChapterNumber, existing.ChapterNumber.Int32)), + CfiPosition: pgText(cfiPosition), + Title: req.Title, + Position: pgText(position), + Notes: pgText(req.Notes), + // Locator fields a partial payload omits must survive: zeroes + // become NULLs (pgFloat8/pgInt4) and would erase stored values. + PercentageLocation: pgFloat8(coalesce(req.PercentageLoc, existing.PercentageLocation.Float64)), + EpubcfiLocation: pgText(coalesce(req.EpubcfiLocation, existing.EpubcfiLocation.String)), + ChapterReference: pgInt4(coalesce(req.ChapterReference, existing.ChapterReference.Int32)), LastModifiedAt: pgtype.Timestamptz{Time: modifiedAt, Valid: true}, LastModifiedSource: pgtype.Text{String: req.Source, Valid: req.Source != ""}, DeviceSyncData: deviceData,