From 75c1d9bb9525e45c8142de2fafbb8968a728043f Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Wed, 9 Sep 2026 14:12:18 -0400 Subject: [PATCH] fix(highlights): web edits update the targeted row instead of minting duplicates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PUT /highlights/:id parsed the row id from the URL and then dropped it: the sync-aware path routed through SaveHighlight's content-derived dedup key, on the assumption that the same text + CFI always resolves to the same key. That assumption breaks in practice — the stored epubcfi_start drifts from foliate's range shape to the converter's point shape after a device echo rewrites the row (bucketPosition cuts at the last colon, so '…/6,/1:367,…' and '…/6/1:367' bucket differently), and the user can edit the selection text. The recomputed key then misses the row being edited and createHighlight mints a second one: the edited row (with note, no device pos0) beside the original — served to KOReader as two highlights, one noted and one not. Editing a selection's text would hit the same trap. SaveHighlightRequest gains an optional HighlightID. When set, the save resolves the row by id (ownership-checked), LWWs against it under its stored dedup key, and never re-derives identity from content. The PUT handler passes the already-parsed id. Device pushes, Kobo, and the sync queue send no id and keep the identity-based flow untouched. applyLWW also stops wiping stored locators on web edits: the web reader sends empty start/end positions (it never had a CRE xpointer), so a note/color edit now keeps the device-native positions and CFIs instead of blanking them — round-trip serve-back for device-created highlights survives web-side edits. --- internal/handlers/media.go | 5 ++ internal/sync/annotations.go | 95 ++++++++++++++++++++++++++++-------- 2 files changed, 81 insertions(+), 19 deletions(-) diff --git a/internal/handlers/media.go b/internal/handlers/media.go index e1924fa..c31bee2 100644 --- a/internal/handlers/media.go +++ b/internal/handlers/media.go @@ -1656,6 +1656,11 @@ func (mh *MediaHandler) UpdateMediaHighlight(c *echo.Context) error { ChapterReference: req.ChapterReference, Source: "web", ModifiedAt: time.Now(), + // The PUT targets this exact row (from the URL): identity must + // not be re-derived from content — a device echo has usually + // rewritten the stored CFI to point shape, so the computed key + // would miss and mint a duplicate beside the edited row. + HighlightID: pgtype.UUID{Bytes: highlightUUID, Valid: true}, }) if err != nil { return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) diff --git a/internal/sync/annotations.go b/internal/sync/annotations.go index abae2fb..ff8beb9 100644 --- a/internal/sync/annotations.go +++ b/internal/sync/annotations.go @@ -60,21 +60,28 @@ const ( ) type SaveHighlightRequest struct { - MediaItemID pgtype.UUID - UserID pgtype.UUID - SelectionText string - StartPosition string - EndPosition string - Color string - NoteText string - PercentageStart float64 - PercentageEnd float64 - EpubcfiStart string - EpubcfiEnd string - ChapterReference int32 - Source string - ModifiedAt time.Time - DeviceSyncData json.RawMessage + MediaItemID pgtype.UUID + UserID pgtype.UUID + SelectionText string + StartPosition string + EndPosition string + Color string + NoteText string + // HighlightID, when valid, targets that exact row (web PUTs edit by + // id): the save LWWs against it directly under its stored dedup key. + // The computed key depends on fields that legitimately change — the + // stored CFI drifts range→point shape after device echoes, and the + // user can edit the selection text — so a key-based upsert would mint + // a duplicate beside the very row being edited. + HighlightID pgtype.UUID + PercentageStart float64 + PercentageEnd float64 + EpubcfiStart string + EpubcfiEnd string + ChapterReference int32 + Source string + ModifiedAt time.Time + DeviceSyncData json.RawMessage // DedupKey overrides the computed key when the client echoes back an // annotation it received from us (device echoes carry device-native // locators, so the computed key would never match the original row and @@ -90,6 +97,35 @@ type SaveHighlightResult struct { func (s *AnnotationService) SaveHighlight(ctx context.Context, req SaveHighlightRequest) (*SaveHighlightResult, error) { dedupKey := req.DedupKey + + // Web edits arrive with the row id from the URL: resolve by id first + // and LWW against that row under its stored key. Content-derived keys + // are for lookups by identity (device pushes carry no row ids); an + // edit must never re-derive identity from (possibly edited) content. + if req.HighlightID.Valid { + byID, idErr := s.db.GetMediaHighlight(ctx, req.HighlightID) + if idErr != nil && !errors.Is(idErr, pgx.ErrNoRows) { + return nil, fmt.Errorf("query highlight by id: %w", idErr) + } + if idErr == nil { + if byID.UserID != req.UserID || byID.MediaItemID != req.MediaItemID { + return nil, fmt.Errorf("highlight %s belongs to another user or media item", req.HighlightID) + } + if dedupKey == "" { + dedupKey = byID.DedupKey.String + } + if byID.Deleted.Bool { + if !incomingNewerThanTombstone(req.ModifiedAt, byID.DeletedAt, byID.LastModifiedAt) { + return &SaveHighlightResult{Highlight: byID, Outcome: SaveOutcomeDeleted}, nil + } + // Newer than the tombstone: a deliberate re-create. Resurrect + // via the LWW update (which clears deleted/deleted_at). + } + return s.applyLWW(ctx, req, byID, dedupKey) + } + // No row with that id: fall through to identity-based resolution. + } + if dedupKey == "" { dedupKey = ComputeDedupKey(req.SelectionText, req.EpubcfiStart, req.StartPosition) } @@ -186,17 +222,38 @@ func (s *AnnotationService) applyLWW( deviceData := mergeDeviceSyncData(existing.DeviceSyncData, req.Source, req.DeviceSyncData) + // Web edits carry no device locators (the web reader never had a CRE + // 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 + } + highlight, err := s.db.UpdateMediaHighlightForSync(ctx, database.UpdateMediaHighlightForSyncParams{ ID: existing.ID, SelectionText: req.SelectionText, - StartPosition: pgText(req.StartPosition), - EndPosition: pgText(req.EndPosition), + StartPosition: pgText(startPosition), + EndPosition: pgText(endPosition), Color: pgText(req.Color), NoteText: pgText(req.NoteText), PercentageStart: pgFloat8(req.PercentageStart), PercentageEnd: pgFloat8(req.PercentageEnd), - EpubcfiStart: pgText(req.EpubcfiStart), - EpubcfiEnd: pgText(req.EpubcfiEnd), + EpubcfiStart: pgText(epubcfiStart), + EpubcfiEnd: pgText(epubcfiEnd), ChapterReference: pgInt4(req.ChapterReference), LastModifiedAt: pgtype.Timestamptz{Time: modifiedAt, Valid: true}, LastModifiedSource: pgtype.Text{String: req.Source, Valid: req.Source != ""},