Bookhoard/bookhoard
0
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases 14 Wiki Activity

fix(highlights): web edits update the targeted row instead of minting duplicates

Browse Source 
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.
This commit is contained in:
john-okeefe
2026-09-09 14:12:18 -04:00
parent 905218dd4b
commit 75c1d9bb95
2 changed files with 81 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/handlers/media.go
+5
View File
@@ -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()})
internal/sync/annotations.go
+76 -19
View File
@@ -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 != ""},