fix(annotations): note LWW re-save 500'd — UserID missing from the sync update params

Pin-first batch for H3: freeze web annotation CRUD response shapes
(create 201 / update 200 / delete 204; full sqlc row models) before
the stack consolidation touches anything.

The new pins immediately caught a live server bug the 28 unit tests
could never see (annotations_test.go is pure-unit: keys and compare
logic, no DB): applyNoteLWW built UpdateMediaNoteForSyncParams without
UserID, so the query's WHERE id=$1 AND user_id=$13 compared against
zero — every dedup-hit note re-save (same content+position) returned
500 "update note: no rows in result set" and the update never landed.
The bookmark twin passes UserID and works; the note stack is the clone
that dropped the field.

Fix: pass UserID in the params (one line). Pins go green and become
the regression net; changelog records the new surfaces.
This commit is contained in:
John O'Keefe committed 2026-10-17 15:38:47 -04:00
1 parent b2b89ba789
commit ff029f1ad5
3 files changed
+160 -1

No files matched your search

+146 -1
View File
@@ -982,7 +982,6 @@ func TestH1_ConflictResolutionPreservesCFIPair(t *testing.T) {
// 7. H1 verification — resolving a conflict must preserve unknown fields
// ---------------------------------------------------------------------------
// TestH1_ConflictResolutionPreservesAudio is the executable check for the
// hardening plan's H1 done-when: a resolution applied via
// POST /api/conflicts/:id/resolve (the endpoint the web UI and the
@@ -1073,3 +1072,149 @@ func TestH1_ConflictResolutionPreservesAudio(t *testing.T) {
}
}
}
// ---------------------------------------------------------------------------
// 3b. web annotation CRUD — the shape freeze H3 needs before it consolidates
// ---------------------------------------------------------------------------
// The three response shapes are the full sqlc row models (annotations are
// richer than their create/PUT request DTOs — the sync columns ride along).
// H3's consolidation + PUT fixes must keep these key sets; response keys
// gained by routing the PUTs through the sync service (e.g. outcome
// markers) are additive and go through the whitelist.
var (
_WEBBookmarkRowKeys = []string{
"id", "media_item_id", "user_id", "page_number", "chapter_number",
"cfi_position", "title", "position", "notes", "created_at",
"dedup_key", "last_modified_at", "last_modified_source",
"device_sync_data", "percentage_location", "epubcfi_location",
"chapter_reference", "deleted", "deleted_at", "origin_source",
}
_WebNoteRowKeys = []string{
"id", "media_item_id", "user_id", "content", "position", "created_at",
"updated_at", "percentage_location", "character_start",
"character_end", "epubcfi_location", "chapter_reference",
"paragraph_reference", "device_sync_data", "dedup_key",
"last_modified_at", "last_modified_source", "deleted", "deleted_at",
}
_WebHighlightRowKeys = []string{
"id", "media_item_id", "user_id", "selection_text",
"start_position", "end_position", "color", "note_id", "created_at",
"updated_at", "percentage_start", "percentage_end",
"character_start", "character_end", "epubcfi_start", "epubcfi_end",
"chapter_reference", "paragraph_start", "paragraph_end",
"panel_number", "device_sync_data", "dedup_key",
"last_modified_at", "last_modified_source", "note_text",
"deleted", "deleted_at",
}
)
func annotationURL(serverURL, mediaItemID, kind, id string) string {
if id == "" {
return fmt.Sprintf("%s/api/media-items/%s/%s", serverURL, mediaItemID, kind)
}
return fmt.Sprintf("%s/api/media-items/%s/%s/%s", serverURL, mediaItemID, kind, id)
}
// pinAnnotationLifecycle walks create -> update -> delete on ONE annotation
// type and freezes every response shape (that's the whole contract the web
// reader and the app rely on for annotations). Every failed status includes
// the response body — pins that fail on status alone are otherwise
// undiagnosable.
func pinAnnotationLifecycle(t *testing.T, setup *TestServerSetup, fx contractFixture, kind string, createBody, updateBody interface{}, rowKeys []string) {
if testing.Short() {
t.Skip("Skipping integration test in short mode")
}
t.Run(kind+" POST response", func(t *testing.T) {
resp := doReq(t, "POST", annotationURL(setup.Server.URL, fx.itemID, kind, ""), createBody, setup.RegularToken)
defer resp.Body.Close()
raw, readErr := io.ReadAll(resp.Body)
require.NoError(t, readErr)
if !assert.Equal(t, http.StatusCreated, resp.StatusCode, "create must stay 201; body: %s", raw) {
t.FailNow()
}
var body map[string]interface{}
require.NoError(t, json.Unmarshal(raw, &body))
assertResponseShape(t, "annotation "+kind+" create", body, rowKeys, rowKeys)
})
t.Run(kind+" PUT response", func(t *testing.T) {
resp := doReq(t, "POST", annotationURL(setup.Server.URL, fx.itemID, kind, ""), createBody, setup.RegularToken)
defer resp.Body.Close()
raw0, readErr := io.ReadAll(resp.Body)
require.NoError(t, readErr)
if !assert.Equal(t, http.StatusCreated, resp.StatusCode, "inner create must stay 201; body: %s", raw0) {
t.FailNow()
}
var created map[string]interface{}
require.NoError(t, json.Unmarshal(raw0, &created))
id, _ := created["id"].(string)
require.NotEmpty(t, id)
resp2 := doReq(t, "PUT", annotationURL(setup.Server.URL, fx.itemID, kind, id), updateBody, setup.RegularToken)
defer resp2.Body.Close()
raw, readErr := io.ReadAll(resp2.Body)
require.NoError(t, readErr)
if !assert.Equal(t, http.StatusOK, resp2.StatusCode, "update must stay 200; body: %s", raw) {
t.FailNow()
}
var body2 map[string]interface{}
require.NoError(t, json.Unmarshal(raw, &body2))
assertResponseShape(t, "annotation "+kind+" update", body2, rowKeys, rowKeys)
t.Run(kind+" DELETE response", func(t *testing.T) {
resp3 := doReq(t, "DELETE", annotationURL(setup.Server.URL, fx.itemID, kind, id), nil, setup.RegularToken)
defer resp3.Body.Close()
require.Equal(t, http.StatusNoContent, resp3.StatusCode, "delete must stay 204")
})
})
}
func TestContractPin_AnnotationCRUDNote(t *testing.T) {
setup := setupTestServer(t)
fx := createContractFixture(t, setup)
pinAnnotationLifecycle(t, setup, fx, "notes",
map[string]interface{}{"content": "pin note", "position": "p1"},
map[string]interface{}{"content": "pin note 2", "position": "p1b"},
_WebNoteRowKeys)
}
func TestContractPin_AnnotationCRUDHighlight(t *testing.T) {
setup := setupTestServer(t)
fx := createContractFixture(t, setup)
pinAnnotationLifecycle(t, setup, fx, "highlights",
map[string]interface{}{
"selection_text": "pin highlight text",
"epubcfi_start": "epubcfi(/6/4/2:10)",
"epubcfi_end": "epubcfi(/6/4/2:20)",
"color": "#ffff00",
},
map[string]interface{}{
"selection_text": "pin highlight text",
"epubcfi_start": "epubcfi(/6/4/2:10)",
"epubcfi_end": "epubcfi(/6/4/2:20)",
"color": "#ff0000",
},
_WebHighlightRowKeys)
}
func TestContractPin_AnnotationCRUDBookmark(t *testing.T) {
setup := setupTestServer(t)
fx := createContractFixture(t, setup)
pinAnnotationLifecycle(t, setup, fx, "bookmarks",
map[string]interface{}{
"title": "pin bookmark",
"position": "p1",
"cfi_position": "epubcfi(/6/4/2:30)",
"page_number": 12,
"chapter_number": 3,
"percentage": 0.25,
"chapter_reference": 4,
},
map[string]interface{}{
"title": "pin bookmark 2",
"position": "p1b",
"notes": "updated by pin",
},
_WEBBookmarkRowKeys)
}
+13
View File
@@ -61,3 +61,16 @@ cover_image_path, has_conflict, format_group, badge`.
reader.ts:3113-3143 and audio.ts:518-520); keys the web never sent
(viewport_x/y, panel_number) merely become honor-able. All other pins
unchanged.
- **H3-prep (web annotation CRUD freeze)** — NEW pinned surfaces, captured
pre-refactor per the pin-first doctrine: `POST/PUT/DELETE
/api/media-items/:id/{notes,highlights,bookmarks}` — create 201 / update
200 / delete 204, response bodies are the full sqlc row models
(MediaNotes 19 keys, MediaBookmarks 20 keys, MediaHighlights 27 keys —
listed in contract_pins_test.go). No keys removed; nothing was pinned
here before. Capturing these immediately caught a LIVE server bug:
`applyNoteLWW` omitted `UserID` from its
`UpdateMediaNoteForSync` params, so every dedup-hit note re-save
(same content+position ⇒ same key) returned 500
("update note: no rows in result set") — the LWW update could never
land. Fixed in the same commit (server-side fix, freely allowed);
the pins are its regression net.
+1
View File
@@ -613,6 +613,7 @@ func (s *AnnotationService) applyNoteLWW(ctx context.Context, req SaveNoteReques
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),