From 28bd323505f2b0b029b368467e252147513fd3bd Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Fri, 16 Oct 2026 17:22:32 -0400 Subject: [PATCH] refactor(progress): SaveProgressRequest becomes the canonical PUT payload; GET serializes from the row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Batch H1 steps 1-2 of the hardening plan. Wire contracts frozen; the pinned shapes are unchanged modulo the whitelisted additive GET keys (docs/contract_pins.md). 1. One canonical request struct. The web PUT binds SaveProgressRequest directly (identity/attribution fields are json:"-" so they stay server-owned) and copies its server fields once — the hand-rolled DTO and the 20-line field-copy block are gone. Parse set for existing clients is IDENTICAL to the old DTO (verified in web/src: the ebook reader sends percentage/current_page/total_pages/reading_mode/epubcfi/ context_text/chapter/zoom_level — reader.ts:3113-3143; the audiobook reader sends percentage/chapter/audio_offset_seconds/elapsed_seconds/ reading_mode — audio.ts:518-520). 2. GET /api/media-items/:id/progress now serializes through one row-struct-driven builder instead of a hand-map, so a row column cannot be silently forgotten again (the audio_offset_seconds omission). Existing keys are verbatim; the row already carried audio_offset_seconds/reading_mode/viewport_x-y/zoom_level/ scroll_position_x-y/panel_number and they surface as WHITELISTED additive keys. The web audiobook reader already reads audio_offset_seconds (audio.ts:219) — with the key absent, durationless books resumed at chapter start (offset always 0); they now resume correctly. The conditional context_text bolt-on and the server-derived css_selector/anchor_href block are unchanged — restore precedence stays percentage-first, never re-derived. --- docs/contract_pins.md | 11 ++++ internal/handlers/media.go | 126 ++++++++++++++++--------------------- internal/sync/progress.go | 54 ++++++++-------- 3 files changed, 93 insertions(+), 98 deletions(-) diff --git a/docs/contract_pins.md b/docs/contract_pins.md index 1f53da3b..fe5b01fd 100644 --- a/docs/contract_pins.md +++ b/docs/contract_pins.md @@ -47,3 +47,14 @@ cover_image_path, has_conflict, format_group, badge`. - **H0 (initial pin)** — the shapes above, captured on the unmodified tree at commit `b4ae423`. +- **H1 (progress canonicalization)** — `GET /api/media-items/:id/progress` + Whitelisted ADDITIVE keys (fields the `GetUniversalProgressRow` already + carries, now surfaced by the shared row serializer): + `audio_offset_seconds, reading_mode, viewport_x, viewport_y, zoom_level, + scroll_position_x, scroll_position_y, panel_number`. No key removed or + renamed; the no-rows and PUT-response shapes are untouched. The web PUT + binds `SaveProgressRequest` directly — its parse set for existing + clients is IDENTICAL to the old DTO (verified against web/src + 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. diff --git a/internal/handlers/media.go b/internal/handlers/media.go index 7342ab25..1c53149b 100644 --- a/internal/handlers/media.go +++ b/internal/handlers/media.go @@ -1154,25 +1154,7 @@ func (mh *MediaHandler) GetMediaReadingProgress(c *echo.Context) error { return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) } - resp := map[string]interface{}{ - "id": progress.ID, - "media_item_id": progress.MediaItemID, - "user_id": progress.UserID, - "current_page": progress.CurrentPage, - "total_pages": progress.TotalPages, - "last_read_at": progress.LastReadAt, - "percentage": progress.Percentage, - "character_offset": progress.CharacterOffset, - "epubcfi": progress.Epubcfi, - "chapter": progress.Chapter, - "chapter_progress": progress.ChapterProgress, - "format_group": progress.FormatGroup, - "total_characters": progress.TotalCharacters, - "chapter_count": progress.ChapterCount, - "last_sync_device": progress.LastSyncDevice, - "last_sync_source": progress.LastSyncSource, - "last_sync_timestamp": progress.LastSyncTimestamp, - } + resp := progressResponse(progress) // Position authority: re-verify the stored anchor and derive the // client-facing handles (cssSelector + anchor document href) server- @@ -1208,6 +1190,44 @@ func (mh *MediaHandler) GetMediaReadingProgress(c *echo.Context) error { return c.JSON(http.StatusOK, resp) } +// progressResponse serializes a progress row to the client wire shape — +// one place, keyed off the row struct, so new row columns surface here by +// construction. Every existing key is preserved verbatim; the row-carried +// locator fields (audio/reading_mode/viewport/zoom/scroll/panel) are +// additive keys per docs/contract_pins.md (H1). +func progressResponse(progress database.GetUniversalProgressRow) map[string]interface{} { + return map[string]interface{}{ + "id": progress.ID, + "media_item_id": progress.MediaItemID, + "user_id": progress.UserID, + "current_page": progress.CurrentPage, + "total_pages": progress.TotalPages, + "last_read_at": progress.LastReadAt, + "percentage": progress.Percentage, + "character_offset": progress.CharacterOffset, + "epubcfi": progress.Epubcfi, + "chapter": progress.Chapter, + "chapter_progress": progress.ChapterProgress, + "format_group": progress.FormatGroup, + "total_characters": progress.TotalCharacters, + "chapter_count": progress.ChapterCount, + "last_sync_device": progress.LastSyncDevice, + "last_sync_source": progress.LastSyncSource, + "last_sync_timestamp": progress.LastSyncTimestamp, + // Row-carried fields surfaced additively (H1) — the web audiobook + // reader already reads audio_offset_seconds (audio.ts) and the + // app parses the same names. + "audio_offset_seconds": progress.AudioOffsetSeconds, + "reading_mode": progress.ReadingMode, + "viewport_x": progress.ViewportX, + "viewport_y": progress.ViewportY, + "zoom_level": progress.ZoomLevel, + "scroll_position_x": progress.ScrollPositionX, + "scroll_position_y": progress.ScrollPositionY, + "panel_number": progress.PanelNumber, + } +} + // UpdateMediaReadingProgress handles PUT /api/media-items/:id/progress func (mh *MediaHandler) UpdateMediaReadingProgress(c *echo.Context) error { userID := c.Get("user_id").(string) @@ -1246,62 +1266,22 @@ func (mh *MediaHandler) UpdateMediaReadingProgress(c *echo.Context) error { } } - var req struct { - CurrentPage *int32 `json:"current_page"` - TotalPages *int32 `json:"total_pages"` - Epubcfi *string `json:"epubcfi"` - ContextText *string `json:"context_text"` - Percentage *float64 `json:"percentage"` - Chapter *int `json:"chapter"` - ChapterProgress *float64 `json:"chapter_progress"` - CharacterOffset *int64 `json:"character_offset"` - ReadingMode *string `json:"reading_mode"` - ZoomLevel *float64 `json:"zoom_level"` - ScrollX *float64 `json:"scroll_position_x"` - ScrollY *float64 `json:"scroll_position_y"` - // Audiobook locator: seconds into the current chapter - AudioOffsetSeconds *int `json:"audio_offset_seconds"` - // Honest wall-clock playing seconds since the last push (audio - // pushes fire while playing; pauses don't count) - ElapsedSeconds *int32 `json:"elapsed_seconds"` - } + // The canonical write struct binds the wire directly; identity and + // attribution fields below are server-owned (json:"-" on the struct) + // and copied here once. + var req wsync.SaveProgressRequest if err := c.Bind(&req); err != nil { return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid request"}) } + req.MediaItemID = pgtype.UUID{Bytes: mediaUUID, Valid: true} + req.UserID = pgtype.UUID{Bytes: userUUID, Valid: true} + req.Source = deviceSource + req.DeviceID = deviceID + req.DeviceType = deviceType + req.DeviceName = deviceLabel + req.Broadcast = true if mh.progressSvc != nil { - saveReq := wsync.SaveProgressRequest{ - MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true}, - UserID: pgtype.UUID{Bytes: userUUID, Valid: true}, - Source: deviceSource, - DeviceID: deviceID, - DeviceType: deviceType, - DeviceName: deviceLabel, - Percentage: req.Percentage, - Epubcfi: req.Epubcfi, - ContextText: req.ContextText, - CharacterOffset: req.CharacterOffset, - Chapter: req.Chapter, - ChapterProgress: req.ChapterProgress, - AudioOffsetSeconds: req.AudioOffsetSeconds, - AudioElapsedSeconds: req.ElapsedSeconds, - CurrentPage: nil, - TotalPages: nil, - ZoomLevel: req.ZoomLevel, - ScrollX: req.ScrollX, - ScrollY: req.ScrollY, - ReadingMode: req.ReadingMode, - Broadcast: true, - } - if req.CurrentPage != nil { - cp := int(*req.CurrentPage) - saveReq.CurrentPage = &cp - } - if req.TotalPages != nil { - tp := int(*req.TotalPages) - saveReq.TotalPages = &tp - } - if req.Percentage != nil && *req.Percentage < 0.005 { existing, err := mh.db.GetUniversalProgress(c.Request().Context(), database.GetUniversalProgressParams{ MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true}, @@ -1312,7 +1292,7 @@ func (mh *MediaHandler) UpdateMediaReadingProgress(c *echo.Context) error { } } - result, err := mh.progressSvc.SaveProgress(c.Request().Context(), saveReq) + result, err := mh.progressSvc.SaveProgress(c.Request().Context(), req) if err != nil { return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) } @@ -1329,11 +1309,11 @@ func (mh *MediaHandler) UpdateMediaReadingProgress(c *echo.Context) error { } currentPage := int32(0) if req.CurrentPage != nil { - currentPage = *req.CurrentPage + currentPage = int32(*req.CurrentPage) } totalPages := int32(0) if req.TotalPages != nil { - totalPages = *req.TotalPages + totalPages = int32(*req.TotalPages) } progress, err := mh.db.UpdateUniversalProgress(c.Request().Context(), database.UpdateUniversalProgressParams{ diff --git a/internal/sync/progress.go b/internal/sync/progress.go index 57efcc19..97213f41 100644 --- a/internal/sync/progress.go +++ b/internal/sync/progress.go @@ -294,39 +294,43 @@ func NewProgressService(db *database.Queries, connManager *ConnectionManager) *P return &ProgressService{db: db, connManager: connManager} } +// SaveProgressRequest is the CANONICAL progress write payload — the web +// PUT binds it directly (identity fields are json:"-": server-owned, never +// client-settable) and every internal writer fills it programmatically. +// Wire keys are frozen: only additive json names are allowed here. type SaveProgressRequest struct { - MediaItemID pgtype.UUID - UserID pgtype.UUID - Source string - DeviceID pgtype.UUID + MediaItemID pgtype.UUID `json:"-"` + UserID pgtype.UUID `json:"-"` + Source string `json:"-"` + DeviceID pgtype.UUID `json:"-"` - Percentage *float64 - Epubcfi *string - ContextText *string - CharacterOffset *int64 - Chapter *int - ChapterProgress *float64 - CurrentPage *int - TotalPages *int - ViewportX *float64 - ViewportY *float64 - ZoomLevel *float64 - ScrollX *float64 - ScrollY *float64 - PanelNumber *int - ReadingMode *string + Percentage *float64 `json:"percentage"` + Epubcfi *string `json:"epubcfi"` + ContextText *string `json:"context_text"` + CharacterOffset *int64 `json:"character_offset"` + Chapter *int `json:"chapter"` + ChapterProgress *float64 `json:"chapter_progress"` + CurrentPage *int `json:"current_page"` + TotalPages *int `json:"total_pages"` + ViewportX *float64 `json:"viewport_x"` + ViewportY *float64 `json:"viewport_y"` + ZoomLevel *float64 `json:"zoom_level"` + ScrollX *float64 `json:"scroll_position_x"` + ScrollY *float64 `json:"scroll_position_y"` + PanelNumber *int `json:"panel_number"` + ReadingMode *string `json:"reading_mode"` // Audiobook locator (seconds into the current chapter) - AudioOffsetSeconds *int + AudioOffsetSeconds *int `json:"audio_offset_seconds"` // AudioElapsedSeconds: the player's honest wall-clock PLAYING time // since the last push (audio pushes fire while playing; pauses don't // count). The capture prefers it over the inter-push delta, capped to // the same 2h attributed ceiling. - AudioElapsedSeconds *int32 + AudioElapsedSeconds *int32 `json:"elapsed_seconds"` - DeviceType string - DeviceName string - Broadcast bool + DeviceType string `json:"-"` + DeviceName string `json:"-"` + Broadcast bool `json:"-"` // ClearLocator declares that this write INTENDS to carry no position // locator: percentage-only writers (devices whose converter declined, @@ -336,7 +340,7 @@ type SaveProgressRequest struct { // client-side far-anchor restore floor. Locater-less rows are honest: // percentage restores are approximate, and the next anchor-bearing // write re-establishes the pair. Request-supplied fields always win. - ClearLocator bool + ClearLocator bool `json:"-"` } func (s *ProgressService) SaveProgress(ctx context.Context, req SaveProgressRequest) (database.ReadingProgress, error) {