From 940b616804b6a537e22ef4b716d545078b3a5c65 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Wed, 17 Jun 2026 13:38:47 +0200 Subject: [PATCH] fix(player): persist watched at end-of-playback (atomic upsert) The in-app player marks a video watched when it reaches the end, firing POST /videos/{id}/state at the same instant as a progress checkpoint. The progress endpoint deletes the 'new' video_states row near the end, so the concurrent state UPDATE matched 0 rows and raised StaleDataError -> 500. The watched status was never persisted (and Feed swallowed the error), so the video stayed in the unwatched feed even after a refresh. - set_video_state: atomic INSERT ... ON CONFLICT (uq_user_video) DO UPDATE for watched/hidden, immune to a concurrent delete; tolerate StaleDataError on the 'new' branch and in the progress endpoint. - PlayerModal: skip the redundant near-end progress checkpoint when we just auto-marked watched, removing the self-inflicted race. - Feed: drop the optimistic override if the server rejects the change, so a failed request can't leave a card phantom-hidden. --- backend/app/routes/feed.py | 54 ++++++++++++++++++------- frontend/src/components/Feed.tsx | 10 ++++- frontend/src/components/PlayerModal.tsx | 11 +++-- 3 files changed, 57 insertions(+), 18 deletions(-) diff --git a/backend/app/routes/feed.py b/backend/app/routes/feed.py index 03863a1..8db08b6 100644 --- a/backend/app/routes/feed.py +++ b/backend/app/routes/feed.py @@ -2,7 +2,9 @@ from datetime import date, datetime, timedelta, timezone from fastapi import APIRouter, Depends, HTTPException, Query from sqlalchemy import Select, and_, false, func, or_, select +from sqlalchemy.dialects.postgresql import insert as pg_insert from sqlalchemy.orm import Session, aliased +from sqlalchemy.orm.exc import StaleDataError from app import quota from app.auth import current_user @@ -387,29 +389,48 @@ def set_video_state( if db.get(Video, video_id) is None: raise HTTPException(status_code=404, detail="Unknown video") - row = db.execute( - select(VideoState).where( - VideoState.user_id == user.id, VideoState.video_id == video_id - ) - ).scalar_one_or_none() + now = datetime.now(timezone.utc) if status == "new": + # Un-marking: keep the row if it still holds a resume position (un-marking + # "watched" should restore the in-progress state, not wipe where the user left + # off); otherwise drop it. The in-app player fires this alongside a progress + # checkpoint that may DELETE the same row, so tolerate it vanishing under us. + row = db.execute( + select(VideoState).where( + VideoState.user_id == user.id, VideoState.video_id == video_id + ) + ).scalar_one_or_none() if row is not None: - # Keep the row if it still holds a resume position (un-marking "watched" - # should restore the in-progress state, not wipe where the user left off). if row.position_seconds: row.status = "new" row.watched_at = None else: db.delete(row) - db.commit() + try: + db.commit() + except StaleDataError: + db.rollback() return {"video_id": video_id, "status": "new"} - if row is None: - row = VideoState(user_id=user.id, video_id=video_id) - db.add(row) - row.status = status - row.watched_at = datetime.now(timezone.utc) if status == "watched" else row.watched_at + # watched / hidden: atomic upsert. The player marks "watched" at end-of-playback at + # the same instant a progress checkpoint may DELETE the in-progress row; a plain + # SELECT-then-UPDATE then raises StaleDataError ("0 rows matched") on that race. An + # INSERT … ON CONFLICT DO UPDATE keyed on uq_user_video is immune to it. + set_: dict = {"status": status, "updated_at": now} + if status == "watched": + set_["watched_at"] = now # hidden keeps any existing watched_at + stmt = ( + pg_insert(VideoState) + .values( + user_id=user.id, + video_id=video_id, + status=status, + watched_at=now if status == "watched" else None, + ) + .on_conflict_do_update(constraint="uq_user_video", set_=set_) + ) + db.execute(stmt) db.commit() return {"video_id": video_id, "status": status} @@ -455,7 +476,12 @@ def set_video_progress( else: row.position_seconds = 0 row.progress_updated_at = datetime.now(timezone.utc) - db.commit() + # A concurrent state change (e.g. the end-of-playback "watched" mark) may + # have removed/updated this row already — tolerate the lost race. + try: + db.commit() + except StaleDataError: + db.rollback() return {"video_id": video_id, "position_seconds": 0} if row is None: diff --git a/frontend/src/components/Feed.tsx b/frontend/src/components/Feed.tsx index ff4c09d..6b0e0f5 100644 --- a/frontend/src/components/Feed.tsx +++ b/frontend/src/components/Feed.tsx @@ -147,7 +147,15 @@ export default function Feed({ api .setState(id, status) .then(() => qc.invalidateQueries({ queryKey: ["feed"] })) - .catch(() => {}); + .catch(() => + // Server rejected the change — drop the optimistic override so the card + // reverts to the real (unchanged) state instead of staying phantom-hidden. + setOverrides((o) => { + const next = { ...o }; + delete next[id]; + return next; + }) + ); if (status === "hidden") { const v = loadedRef.current.find((x) => x.id === id); notify({ diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 1d38fed..a97ebbd 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -339,12 +339,14 @@ export default function PlayerModal({ // Auto-watch only applies to the active item — not to other videos the player // navigated to via description links. - const maybeAutoWatch = (current: number, duration: number) => { - if (autoMarkedRef.current || duration <= 0 || currentIdRef.current !== id) return; + const maybeAutoWatch = (current: number, duration: number): boolean => { + if (autoMarkedRef.current || duration <= 0 || currentIdRef.current !== id) return false; if (current > duration - FINISH_MARGIN) { autoMarkedRef.current = true; setWatched(true); + return true; } + return false; }; const persist = (): Promise | void => { const p = playerRef.current; @@ -352,7 +354,10 @@ export default function PlayerModal({ try { const cur = p.getCurrentTime(); const dur = typeof p.getDuration === "function" ? p.getDuration() : 0; - maybeAutoWatch(cur, dur); + // If we just auto-marked watched, skip the near-end progress checkpoint: it would + // only clear the resume position, and firing both at once needlessly races the + // watched write against the progress row it deletes. + if (maybeAutoWatch(cur, dur)) return; // Only checkpoint the feed video we opened with — a navigated-to video may not // be in this user's library, so the server would 404 on it. if (currentIdRef.current === id) {