diff --git a/docs/requirements.md b/docs/requirements.md index ceb43e6c..cc7fa49b 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -316,6 +316,7 @@ Internal architecture, components, and application logic. | DR-151 | The player's video SurfaceView actually reaches the view hierarchy. `JellyTauPlayer.setActivity()` had zero callers, so `currentActivity` was always null and `autoAttachSurface()` returned at "Cannot attach surface - no Activity reference". The surface was created and handed to ExoPlayer but never added to the content view, so native video decoded to a surface that was never on screen — independent of any webview transparency. `MainActivity.onCreate` now supplies the reference, which also revives PiP on the video path: `canEnterPip()` gates on `isVideoSurfaceAttached()`, which had been permanently false | Playback | UR-003, UR-041 | Done | | DR-152 | Platform playback facilities are reported by Rust, not sniffed from the user agent. `webviewAudio.ts` re-derived "does this platform have a native audio backend" by matching `navigator.userAgent` against `android`/`linux` — a second copy of the `cfg!` gate the backends are compiled under, free to drift from it. `player_get_capabilities` now returns `usesWebviewAudio` and `supportsNativeVideo` from the same cfg gates, and the frontend consumes them; the settings toggle for native video is hidden entirely where the platform cannot support it | Player | UR-003, UR-005 | Done | | DR-153 | The git tag is the single source of truth for a release version. The version lived in four files (`package.json`, `tauri.conf.json`, `Cargo.toml`, `Cargo.lock`) that had to be hand-edited in lockstep, and CI's release job rewrote exactly one of them — so a tagged build produced an installer named for the tag wrapped around package metadata naming the previous release, while the Linux job had no version step at all and shipped whatever was committed. `scripts/set-version.sh` writes all four from one argument and is the only thing that does; every release job calls it with the tag. The Android `versionCode` is derived in the same place as `1000 + major*10000 + minor*100 + patch`, which is monotonic in semver order and clears the 1000 floor already installed in the field — a lower code than the installed one makes Android refuse the update. A prerelease suffix is stripped before that arithmetic, which would otherwise abort the script, and a non-tag ref (CI passes `${GITHUB_REF#refs/tags/}` unconditionally) falls back to `git describe` rather than failing a branch build | Build | - | Done | +| DR-154 | A watch position that cannot reach the server is queued, not dropped. `sync_queue` and its drain (DR-131) were built, tested and running, but the stop-report path never fed them: `HybridRepository::report_playback_stopped` is a bare pass-through to the online repository ("Playback reporting goes directly to server"), and on failure the error surfaced to a frontend `catch` whose own comment read "Server error - could queue, but for now just log". Both producers that *would* have queued it — `PlaybackReporter::queue_for_sync` in Rust and `syncService.queuePlaybackProgress` on the frontend — have no callers on the playback path, so closing a video while the server was unreachable lost the resume point outright even though `user_data.pending_sync` was dutifully set to 1 and nothing ever drains that flag for positions (unlike favourites, DR-120). The command layer now enqueues a `report_playback_stopped` row whenever the push fails, which the existing drain already knows how to parse and replay. The pending row for an item is **superseded in place** rather than appended to: progress is reported every 10s, so a server that stays down would otherwise add a row per tick, all of them obsoleted by the newest — the unbounded queue DR-131 exists to prevent. Only `pending`/`failed` rows are superseded, because an `abandoned` row has been given up on and reviving it would restore that same growing counter. Queueing is best-effort and never fails the command: the local position is already saved, so a failed *queue* write must not be reported as a lost position | Backend | UR-025, UR-002 | Done | | DR-143 | Flipping the offline downloaded-only gate actually re-queries the listing. The gate (DR-078) is a process-wide flag in Rust consulted only *while a query runs*, but no library surface re-queried when its inputs changed: `useServerReachabilityReload` fires only on the offline → **online** transition, and `GenericMediaListPage`, `GenericGenreBrowser` and the favourites page never even called its `checkServerReachability`. So going offline left the full server catalog on screen under a now-closed gate, and toggling "Show all server media" only greyed cards — `MediaCard.isServerOnly` is a pure frontend derivation that updates instantly — without adding or removing a single row. The filter therefore read as "shows everything until I filter, then greys some of it" while the backend gate was correct and simply never exercised. `catalogFilterVersion` is the refetch signal: `pushCatalogVisibility` now awaits `set_show_server_catalog` and bumps the version only **after** the backend accepts the new flag, since a reload racing the push would re-query under the old gate and undo itself. A failed push clears `lastIncludeCatalog` instead of latching it, so the next identical transition is retried rather than skipped as a no-op and left permanently disagreeing with the backend. `useOfflineFilterReload` subscribes pages to that signal, skipping the value they already loaded under; it is wired into both generic list components and the movies/music/tv/favourites landing pages and the `/library/[id]` detail page | UI | UR-052 | Done | | DR-135 | A download's media type comes from the item, not a default. `download_item` — the path a media card uses to queue an item while offline — never records `media_type`, and the reconnect resolver read that NULL as `'audio'`, so a **movie** queued from a card had its URL resolved by `get_audio_stream_url`. The file that landed on disk was an audio-only transcode, which is why a "downloaded" film could never play offline no matter how the path or protocol was fixed. The resolver now falls back to the item's own `item_type` (`VIDEO_ITEM_TYPES` in Rust, so the frontend never learns which types are video) and only defaults to audio when the item is not cached locally. An explicit `media_type` on the row still wins | Downloads | UR-071, UR-052 | Done | | DR-136 | Rows already downloaded under the audio default are repaired, not just prevented. They are identifiable after the fact — no `media_type`, but a video item — so on reconnect they are reset to `pending` with their audio URL cleared and re-resolved by DR-135's corrected logic, overwriting the audio file in place. Without this the fix is invisible to anyone who had already queued a film: the row still reads "downloaded" and still fails to play. Rows carrying an explicit `media_type` and genuine audio downloads are left untouched | Downloads | UR-071 | Done | @@ -549,6 +550,7 @@ Internal architecture, components, and application logic. | UT-148 | Forcing a transcode from the client: an undecodable default track forces one, a decodable track does not, the default track decides rather than the first, the first decides when nothing is marked default, and neither an audio-less source nor an unnamed codec is second-guessed | DR-149 | Done | | UT-149 | `createAdapter` returns the native adapter only when Rust reports native AND `experimentalNativeVideo` is on; the flag off forces HTML5 even when Rust says native, and the flag on never promotes a platform Rust reported as HTML5 | DR-150 | Done | | UT-150 | `set-version.sh` stamps all four manifests without touching dependency versions, and the Android versionCode is monotonic across an upgrade sequence, clears the 1000 floor, and survives a prerelease suffix | DR-153 | Done | +| UT-151 | An unreportable stop lands in the queue and is pushed by the existing drain; re-queueing the same item supersedes the earlier position rather than adding a row, distinct items keep their own positions, and an abandoned row is not revived by a later report | DR-154 | Done | | UT-142 | The audio codecs offered for video direct play: a Dolby device's real `MediaCodecList` output drops `ac3`/`eac3`, AMR and raw PCM are dropped too, a fully-supported list is passed through untouched, a list with nothing decodable still claims `aac`, and stray spacing or casing does not decide whether the user gets sound | DR-148 | Done | | UT-143 | Subtitle URLs resolve to plain strings before they reach the markup (never a Promise), unresolvable tracks are dropped, a stale selection collapses to "Off", and a server-default track is never auto-selected | UR-020, DR-023 | Done | | UT-144 | VideoPlayer actually renders `` children carrying `data-stream-index`, with no `default` attribute and no async `getSubtitleUrl()` bound to `src` | UR-020, DR-023 | Done | diff --git a/src-tauri/src/commands/repository.rs b/src-tauri/src/commands/repository.rs index 2af5a413..65b2820d 100644 --- a/src-tauri/src/commands/repository.rs +++ b/src-tauri/src/commands/repository.rs @@ -712,9 +712,19 @@ pub async fn repository_report_playback_progress( } /// Report playback stopped +/// +/// A stop-report that cannot reach the server is queued rather than dropped: +/// this is the position the resume point is built from, and losing it is +/// exactly the "it forgot where I was" the sync queue exists to prevent. The +/// drain (DR-131) pushes it on the next reconnect. Queueing is best-effort — +/// failing the command because the *queue* write failed would tell the caller +/// the report was lost when the local position was already saved. +/// +/// TRACES: UR-025 | DR-154 | UT-151 #[tauri::command] #[specta::specta] pub async fn repository_report_playback_stopped( + db: State<'_, crate::commands::storage::DatabaseWrapper>, manager: State<'_, RepositoryManagerWrapper>, handle: String, item_id: String, @@ -723,10 +733,39 @@ pub async fn repository_report_playback_stopped( // Milliseconds across the boundary; the Jellyfin API wants ticks. let position_ticks = position_ms * 10_000; let repo = manager.0.get(&handle).ok_or("Repository not found")?; - repo.as_ref() + + let result = repo + .as_ref() .report_playback_stopped(&item_id, position_ticks) + .await; + + if let Err(e) = &result { + let db_service = { + let database = db.0.lock().map_err(|err| err.to_string())?; + Arc::new(database.service()) + }; + let user_id = repo.user_id().to_string(); + if let Err(queue_err) = crate::commands::sync_drain::enqueue_playback_stopped( + &db_service, + &user_id, + &item_id, + position_ticks, + ) .await - .map_err(|e| format!("{:?}", e)) + { + warn!( + "[Repository] Stop-report for {} failed ({:?}) and could not be queued: {}", + item_id, e, queue_err + ); + } else { + debug!( + "[Repository] Stop-report for {} failed ({:?}); queued for the next reconnect", + item_id, e + ); + } + } + + result.map_err(|e| format!("{:?}", e)) } /// Get image URL for an item diff --git a/src-tauri/src/commands/sync_drain.rs b/src-tauri/src/commands/sync_drain.rs index e37bb8f2..60644118 100644 --- a/src-tauri/src/commands/sync_drain.rs +++ b/src-tauri/src/commands/sync_drain.rs @@ -355,6 +355,65 @@ async fn mark_failed( Ok(()) } +/// Queue a watch position that could not be reported to the server. +/// +/// The stop-report path pushed straight to the server and, on failure, logged +/// and dropped the position — so closing a video while the server was +/// unreachable lost the resume point outright, even though the queue and its +/// drain (DR-131) were built and running. This is the missing producer. +/// +/// The pending row for an item is *replaced* rather than appended to. Progress +/// is reported every 10s, so a server that stays down would otherwise grow one +/// row per tick, all of them superseded by the newest — the unbounded queue +/// DR-131 exists to prevent. Only `pending`/`failed` rows are superseded: +/// an `abandoned` row has been given up on and must not be revived, and a +/// `completed` one is history. +/// +/// TRACES: UR-025 | DR-154 | UT-151 +pub async fn enqueue_playback_stopped( + db: &Arc, + user_id: &str, + item_id: &str, + position_ticks: i64, +) -> Result<(), String> { + let payload = format!(r#"{{"position_ticks": {}}}"#, position_ticks); + + // Supersede an already-queued position for this item, keeping its place in + // the queue order (created_at) so a later item cannot overtake it. + let updated = db + .execute(Query::with_params( + "UPDATE sync_queue \ + SET payload = ?, status = 'pending', error_message = NULL \ + WHERE user_id = ? AND item_id = ? AND operation = 'report_playback_stopped' \ + AND status IN ('pending', 'failed')", + vec![ + QueryParam::String(payload.clone()), + QueryParam::String(user_id.to_string()), + QueryParam::String(item_id.to_string()), + ], + )) + .await?; + + if updated == 0 { + db.execute(Query::with_params( + "INSERT INTO sync_queue (user_id, operation, item_id, payload, status, created_at) \ + VALUES (?, 'report_playback_stopped', ?, ?, 'pending', CURRENT_TIMESTAMP)", + vec![ + QueryParam::String(user_id.to_string()), + QueryParam::String(item_id.to_string()), + QueryParam::String(payload), + ], + )) + .await?; + } + + debug!( + "[SyncQueue] Queued unreported stop for {} at {} ticks", + item_id, position_ticks + ); + Ok(()) +} + /// Drain on every offline→online transition. /// /// TRACES: UR-025 | DR-131 @@ -781,6 +840,143 @@ mod tests { assert_eq!(row_state(&db, "theirs").await.0, "pending"); } + /// The bug DR-154 fixes: a stop-report that could not reach the server was + /// logged and dropped, so the watch position was lost outright. It must + /// land in the queue the drain already knows how to push. + /// + /// TRACES: UR-025 | DR-154 | UT-151 + #[tokio::test] + async fn test_failed_stop_report_is_queued_rather_than_dropped() { + let db = test_db(); + + enqueue_playback_stopped(&db, "u1", "ep1", 5_000_000_000) + .await + .unwrap(); + + // The very drain that already exists must be able to push it. + let sink = RecordingSink::new(); + let report = drain_sync_queue(&db, &sink, "u1").await.unwrap(); + + assert_eq!( + sink.calls(), + vec![QueuedOp::PlaybackStopped { + item_id: "ep1".to_string(), + position_ticks: 5_000_000_000, + }] + ); + assert_eq!(report.pushed, 1); + assert_eq!(report.remaining, 0); + } + + /// Progress is reported every 10s, and a server that stays unreachable + /// would otherwise add a row per tick — an unbounded queue of positions + /// that are all superseded by the newest one. The pending row for an item + /// is replaced in place, so the queue holds the latest position only. + /// + /// TRACES: UR-025 | DR-154 | UT-151 + #[tokio::test] + async fn test_requeueing_the_same_item_supersedes_the_earlier_position() { + let db = test_db(); + + enqueue_playback_stopped(&db, "u1", "ep1", 1_000) + .await + .unwrap(); + enqueue_playback_stopped(&db, "u1", "ep1", 2_000) + .await + .unwrap(); + enqueue_playback_stopped(&db, "u1", "ep1", 3_000) + .await + .unwrap(); + + let sink = RecordingSink::new(); + drain_sync_queue(&db, &sink, "u1").await.unwrap(); + + // One row, carrying the newest position — not three. + assert_eq!( + sink.calls(), + vec![QueuedOp::PlaybackStopped { + item_id: "ep1".to_string(), + position_ticks: 3_000, + }] + ); + } + + /// Distinct items must not collide — superseding is per item, not global. + /// + /// TRACES: UR-025 | DR-154 | UT-151 + #[tokio::test] + async fn test_requeueing_keeps_positions_for_different_items_apart() { + let db = test_db(); + + enqueue_playback_stopped(&db, "u1", "ep1", 1_000) + .await + .unwrap(); + enqueue_playback_stopped(&db, "u1", "ep2", 2_000) + .await + .unwrap(); + + let sink = RecordingSink::new(); + drain_sync_queue(&db, &sink, "u1").await.unwrap(); + + let mut calls = sink.calls(); + calls.sort_by_key(|op| match op { + QueuedOp::PlaybackStopped { item_id, .. } => item_id.clone(), + _ => String::new(), + }); + assert_eq!( + calls, + vec![ + QueuedOp::PlaybackStopped { + item_id: "ep1".to_string(), + position_ticks: 1_000, + }, + QueuedOp::PlaybackStopped { + item_id: "ep2".to_string(), + position_ticks: 2_000, + }, + ] + ); + } + + /// A row already abandoned (DR-131 gave up on it) must not be resurrected + /// by a later report — that would restore the queue-that-only-grows this + /// whole area exists to prevent. The new report is queued as its own row. + /// + /// TRACES: UR-025 | DR-154 | UT-151 + #[tokio::test] + async fn test_requeueing_does_not_revive_an_abandoned_row() { + let db = test_db(); + seed( + &db, + &[( + "u1", + "report_playback_stopped", + "ep1", + Some(r#"{"position_ticks": 111}"#), + "abandoned", + MAX_SYNC_ATTEMPTS, + "2026-08-01T10:00:00Z", + )], + ) + .await; + + enqueue_playback_stopped(&db, "u1", "ep1", 999) + .await + .unwrap(); + + let sink = RecordingSink::new(); + drain_sync_queue(&db, &sink, "u1").await.unwrap(); + + // Only the fresh row is pushed; the abandoned one stays abandoned. + assert_eq!( + sink.calls(), + vec![QueuedOp::PlaybackStopped { + item_id: "ep1".to_string(), + position_ticks: 999, + }] + ); + } + /// Nothing queued means no server calls at all — a reconnect must not /// generate traffic just because it happened. /// diff --git a/src/lib/services/playbackReporting.ts b/src/lib/services/playbackReporting.ts index e2307721..7ed72764 100644 --- a/src/lib/services/playbackReporting.ts +++ b/src/lib/services/playbackReporting.ts @@ -103,15 +103,15 @@ export async function reportPlaybackStopped(itemId: string, positionSeconds: num } } - // Queue for sync to server (the sync service will handle retry logic) + // Report to the server. Rust queues the position for the next reconnect if + // the server cannot be reached (DR-154), so a throw here means the report + // did not land *this time* — not that the position was lost. if (userId && positionSeconds > 0) { try { - // Get the repository to check if we should queue const repo = auth.getRepository(); await repo.reportPlaybackStopped(itemId, positionMs); } catch (e) { - console.error("[PlaybackReporting] Failed to report to server:", e); - // Server error - could queue, but for now just log + console.warn("[PlaybackReporting] Stop-report did not reach the server; queued for sync:", e); } } }