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.
This commit is contained in:
parent
79249104d4
commit
a09e0dda0c
3 changed files with 57 additions and 18 deletions
|
|
@ -2,7 +2,9 @@ from datetime import date, datetime, timedelta, timezone
|
||||||
|
|
||||||
from fastapi import APIRouter, Depends, HTTPException, Query
|
from fastapi import APIRouter, Depends, HTTPException, Query
|
||||||
from sqlalchemy import Select, and_, false, func, or_, select
|
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 import Session, aliased
|
||||||
|
from sqlalchemy.orm.exc import StaleDataError
|
||||||
|
|
||||||
from app import quota
|
from app import quota
|
||||||
from app.auth import current_user
|
from app.auth import current_user
|
||||||
|
|
@ -387,29 +389,48 @@ def set_video_state(
|
||||||
if db.get(Video, video_id) is None:
|
if db.get(Video, video_id) is None:
|
||||||
raise HTTPException(status_code=404, detail="Unknown video")
|
raise HTTPException(status_code=404, detail="Unknown video")
|
||||||
|
|
||||||
|
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(
|
row = db.execute(
|
||||||
select(VideoState).where(
|
select(VideoState).where(
|
||||||
VideoState.user_id == user.id, VideoState.video_id == video_id
|
VideoState.user_id == user.id, VideoState.video_id == video_id
|
||||||
)
|
)
|
||||||
).scalar_one_or_none()
|
).scalar_one_or_none()
|
||||||
|
|
||||||
if status == "new":
|
|
||||||
if row is not 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:
|
if row.position_seconds:
|
||||||
row.status = "new"
|
row.status = "new"
|
||||||
row.watched_at = None
|
row.watched_at = None
|
||||||
else:
|
else:
|
||||||
db.delete(row)
|
db.delete(row)
|
||||||
|
try:
|
||||||
db.commit()
|
db.commit()
|
||||||
|
except StaleDataError:
|
||||||
|
db.rollback()
|
||||||
return {"video_id": video_id, "status": "new"}
|
return {"video_id": video_id, "status": "new"}
|
||||||
|
|
||||||
if row is None:
|
# watched / hidden: atomic upsert. The player marks "watched" at end-of-playback at
|
||||||
row = VideoState(user_id=user.id, video_id=video_id)
|
# the same instant a progress checkpoint may DELETE the in-progress row; a plain
|
||||||
db.add(row)
|
# SELECT-then-UPDATE then raises StaleDataError ("0 rows matched") on that race. An
|
||||||
row.status = status
|
# INSERT … ON CONFLICT DO UPDATE keyed on uq_user_video is immune to it.
|
||||||
row.watched_at = datetime.now(timezone.utc) if status == "watched" else row.watched_at
|
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()
|
db.commit()
|
||||||
return {"video_id": video_id, "status": status}
|
return {"video_id": video_id, "status": status}
|
||||||
|
|
||||||
|
|
@ -455,7 +476,12 @@ def set_video_progress(
|
||||||
else:
|
else:
|
||||||
row.position_seconds = 0
|
row.position_seconds = 0
|
||||||
row.progress_updated_at = datetime.now(timezone.utc)
|
row.progress_updated_at = datetime.now(timezone.utc)
|
||||||
|
# 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()
|
db.commit()
|
||||||
|
except StaleDataError:
|
||||||
|
db.rollback()
|
||||||
return {"video_id": video_id, "position_seconds": 0}
|
return {"video_id": video_id, "position_seconds": 0}
|
||||||
|
|
||||||
if row is None:
|
if row is None:
|
||||||
|
|
|
||||||
|
|
@ -147,7 +147,15 @@ export default function Feed({
|
||||||
api
|
api
|
||||||
.setState(id, status)
|
.setState(id, status)
|
||||||
.then(() => qc.invalidateQueries({ queryKey: ["feed"] }))
|
.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") {
|
if (status === "hidden") {
|
||||||
const v = loadedRef.current.find((x) => x.id === id);
|
const v = loadedRef.current.find((x) => x.id === id);
|
||||||
notify({
|
notify({
|
||||||
|
|
|
||||||
|
|
@ -339,12 +339,14 @@ export default function PlayerModal({
|
||||||
|
|
||||||
// Auto-watch only applies to the active item — not to other videos the player
|
// Auto-watch only applies to the active item — not to other videos the player
|
||||||
// navigated to via description links.
|
// navigated to via description links.
|
||||||
const maybeAutoWatch = (current: number, duration: number) => {
|
const maybeAutoWatch = (current: number, duration: number): boolean => {
|
||||||
if (autoMarkedRef.current || duration <= 0 || currentIdRef.current !== id) return;
|
if (autoMarkedRef.current || duration <= 0 || currentIdRef.current !== id) return false;
|
||||||
if (current > duration - FINISH_MARGIN) {
|
if (current > duration - FINISH_MARGIN) {
|
||||||
autoMarkedRef.current = true;
|
autoMarkedRef.current = true;
|
||||||
setWatched(true);
|
setWatched(true);
|
||||||
|
return true;
|
||||||
}
|
}
|
||||||
|
return false;
|
||||||
};
|
};
|
||||||
const persist = (): Promise<unknown> | void => {
|
const persist = (): Promise<unknown> | void => {
|
||||||
const p = playerRef.current;
|
const p = playerRef.current;
|
||||||
|
|
@ -352,7 +354,10 @@ export default function PlayerModal({
|
||||||
try {
|
try {
|
||||||
const cur = p.getCurrentTime();
|
const cur = p.getCurrentTime();
|
||||||
const dur = typeof p.getDuration === "function" ? p.getDuration() : 0;
|
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
|
// 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.
|
// be in this user's library, so the server would 404 on it.
|
||||||
if (currentIdRef.current === id) {
|
if (currentIdRef.current === id) {
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue