Repository navigation
feat: sync watch progress made offline - #227
mia-riezebos wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
An active sync can write one user's queued progress to another account. Newly recorded updates can also remain queued without a retry trigger.
Reviewed changes
Reviewed the offline progress endpoint, timestamp conflict handling, durable browser queue, browse-shell lifecycle, and accompanying tests.
- Server sync: Adds an authenticated progress update without a playback session, rejecting stale timestamps under the existing per-user/Item lock.
- Durable queue: Persists the newest update per user and Item, retains retryable failures, and removes acknowledged or permanently refused updates.
- Browser lifecycle: Flushes on browse-shell mount and browser reconnection, with timed retries while updates remain.
- Verification: All 291 web tests and
bun run checkpass; focused harnesses reproduce both findings below. Server integration tests were not run becauseDATABASE_URLis not configured.
gpt-6.1-sol ｜ 𝕏
| async function run(userId: string): Promise<number> { | ||
| for (const entry of pending(userId)) { | ||
| const { userId: _, ...update } = entry; | ||
| try { | ||
| await deps.send(update); |
There was a problem hiding this comment.
[P1] Preserve the queue owner's identity across account changes
This loop keeps sending after the browse effect is cleaned up, but each request uses whichever session cookie is current at send time. Holding Alice's first response, signing out and into Bob, then releasing that response makes Alice's second queued Item update Bob's progress and removes it from Alice's queue.
Technical details
## Affected sites
- [offline-progress.ts:61-80](https://github.com/mia-cx/thalia/blob/7b2393d3bea5e8424d9788cb8eb4989b2a835c88/apps/web/src/lib/offline-progress.ts#L61-L80): snapshots one user's entries and strips `userId` before sending through the shared client.
- [offline-progress.ts:95-109](https://github.com/mia-cx/thalia/blob/7b2393d3bea5e8424d9788cb8eb4989b2a835c88/apps/web/src/lib/offline-progress.ts#L95-L109): shares one in-flight promise across users and authenticates sends using ambient cookies.
- [offline-progress.ts:129-133](https://github.com/mia-cx/thalia/blob/7b2393d3bea5e8424d9788cb8eb4989b2a835c88/apps/web/src/lib/offline-progress.ts#L129-L133): cleanup removes timers/listeners but does not invalidate the runner.
- [api/progress.ts:184-190](https://github.com/mia-cx/thalia/blob/7b2393d3bea5e8424d9788cb8eb4989b2a835c88/apps/server/src/api/progress.ts#L184-L190): writes under `context.caller.user.id` with no expected-owner check.
## Required outcome
- Never apply or discard one user's queue entries under another user's session, including after an awaited request and when session cookies change in another tab.
- Invalidate obsolete runners on account transitions and keep pending updates for their owner; avoid sharing a user's flush result with another user.
- Add a regression test with two Alice entries, a held first response, synchronizer cleanup, Bob sign-in, and release of the response. The second Alice entry must not be written for Bob or lost.
## Suggested approach
- Bind requests to the expected queue owner and reject an authentication-owner mismatch before writing; ensure the client retains the entry on that mismatch. This also covers shared-cookie changes outside the current tab.
- Add account-scoped cancellation/invalidation rather than relying solely on clearing retry timers.| const flush = async () => { | ||
| clearTimeout(timer); | ||
| const left = await offlineProgress.flush(userId); | ||
| if (left > 0 && !stopped) timer = setTimeout(flush, retryMs); |
There was a problem hiding this comment.
[P2] Wake syncing when an empty queue receives new progress
Once the initial flush finds no entries, no timer remains, and record() only writes storage without waking this synchronizer. Updates recorded afterwards, including from an offline player in another tab, remain unsent if the server recovers without a browser online event, until a browse-shell remount or reload.
Technical details
## Affected sites
- [offline-progress.ts:87-92](https://github.com/mia-cx/thalia/blob/7b2393d3bea5e8424d9788cb8eb4989b2a835c88/apps/web/src/lib/offline-progress.ts#L87-L92): persisting a new entry has no notification or scheduling hook.
- [offline-progress.ts:118-128](https://github.com/mia-cx/thalia/blob/7b2393d3bea5e8424d9788cb8eb4989b2a835c88/apps/web/src/lib/offline-progress.ts#L118-L128): the initial flush and `online` event are the only ways to start a retry chain.
## Required outcome
- Newly queued progress must wake the active synchronizer after a prior flush drained the queue or found it empty, without requiring browser connectivity to change or the layout to remount.
- Preserve retries when Thalia is unreachable even though the browser remains connected to the network.
- Cover the same-origin cross-tab case if recording and browsing occur in different tabs.
## Reproduction
Start `syncOfflineProgress(userId)` with empty localStorage, let the initial flush settle, then call `offlineProgress.record(...)`. A harness using the production singleton and wrapper observes one persisted entry, zero API sends, and zero retry timers.
## Suggested approach
Notify the synchronizer when recording creates pending work, with a storage-event equivalent for other tabs, or otherwise ensure newly queued work is discovered without depending on an `online` event.
612ea60 to
434f40c
Compare
7b2393d to
0ea0530
Compare
There was a problem hiding this comment.
Important
Delayed offline-player teardown can overwrite newer progress from another device.
Reviewed changes Reviewed the incremental changes from 0ea0530 through 06b7de2, with the complete PR diff as context.
- Connected offline playback: Recorded download progress on heartbeats, pause, completion and close, with positions capped to the saved Version duration.
- Loaded saved identity: Supplied the offline player's queue owner without contacting the server.
- Exercised real playback: Replaced the seeded browser queue with a downloaded movie played to completion while the server is stopped, followed by reload and sync checks.
Validation: all 304 web tests and bun run check pass. A focused harness using the production recorder and queue reproduced the timestamp regression below. Server/browser integration tests were not run because DATABASE_URL is not configured and ffmpeg is unavailable.
gpt-6.1-sol ｜ 𝕏
| /** Records the last position, stops the remux and subtitles and releases the video element. `completed` marks it watched; safe to call twice. */ | ||
| async close(close?: { completed?: boolean }) { | ||
| if (closing) return; | ||
| report(close?.completed === true || video.ended); |
There was a problem hiding this comment.
[P2] Preserve the progress timestamp when closing unchanged playback
Closing a player that has been paused or ended for a while re-records the same position with the close time, because record() always stamps now. If another device advanced in between, this stale position now wins the server's timestamp comparison and overwrites the newer progress.
Technical details
# Delayed teardown makes old progress appear new
## Affected sites
- `apps/web/src/lib/offline/session.ts:125-127,238-240`: close unconditionally reports for any session that started playing, even when its last position/completion was already recorded on pause or ended.
- `apps/web/src/lib/components/Player.svelte:263-269`: forwards each report directly to the queue.
- `apps/web/src/lib/offline-progress.ts:87-92`: replaces the queued entry and assigns the current time on every record.
- `apps/server/src/playback/progress.ts:553-567`: accepts the newly dated offline entry over another device's genuinely later playback.
## Reproduction
Using the production `playDownload` and `createProgressQueue` with deterministic queue time and mocked remux/MSE attachment:
1. Pause offline playback at position 30 at 10:00. The queue correctly contains `at: 10:00`.
2. Another device advances to position 80 at 11:00.
3. Close the still-paused offline player at 12:00. Its queue entry becomes position 30 with `at: 12:00`.
4. The server comparison would reject the original pause entry but accepts the close entry, replacing position 80 with 30.
## Required outcome
- Teardown must not give unchanged paused/ended progress a newer timestamp than the actual playback event.
- Continue saving genuinely changed final position or completion, including playback closed between heartbeats.
- Add a regression covering pause/end, newer progress on another device, and delayed offline close; the newer playback must remain authoritative.
9086388 to
d050798
Compare
Offline playback has no session to report to, so its progress needs a
way to reach the server later without losing to newer progress from
another device.
- playback.syncProgress (PUT /items/{itemId}/progress) applies one
update dated when it happened, through the same write as a live
session, so the same watched rules apply. Progress stored later wins;
a date in the future counts as now. It opens no session.
- lib/offline-progress keeps updates in localStorage, newest per user
and Item, and sends them when the browse shell loads, when the
browser comes online, and every ten seconds while any are left.
Refused updates are dropped; unreachable ones stay.
Closes #180
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…test Page.reload answers before the new document commits, so the Downloads wait could pass on the old page. Navigating answers only once the new page commits, the same fix the folder test got. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The offline player recorded nothing, so progress made without the server was lost. It now records into the offline progress queue on the online player's cadence: every 10 s while playing, on pause, at the end as completed, and on close. The queue sends it when the server answers, and the server applies its watched rules there. The offline player route reads the last signed-in caller from the session the browse shell keeps, without asking the server, so progress has an owner. With none kept, it plays and records nothing. Positions stop at the Version's probed length, saved with the download, as the server refuses any past it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e8754da to
9f9b55a
Compare
d050798 to
550c7f9
Compare
Progress made while watching a download offline had nowhere to go: the server only takes progress through a live play session, and another device may have moved on in the meantime. This is #180 of PRD #173, stacked on #226.
Server.
playback.syncProgress(PUT /api/items/{itemId}/progress) takes one update: Item, the downloaded Version, position, completed, andat, when it happened. It checksviewandplayon the Item and the Version, then writes throughupsertProgress, the same write as a live session's heartbeat. #221's 90% rule lives there, so offline updates get it too, with no copy. Under the per-user-and-Item advisory lock, the update applies only whenatis newer than the storedupdatedAt; otherwise it answersapplied: falsewith the stored progress. The stored row is datedat, so a later sync from a third device still compares against the time it happened. Anatin the future counts as now, so a fast clock can't win. It opens no session and counts no play.Web.
lib/offline-progressqueues updates inlocalStorage, keeping the newest per user and Item. The browse shell sends the signed-in user's queue on load, on the browser'sonlineevent, and every 10 s while any are left. An unreachable or failing server keeps the queue for the next try; an update the server refuses outright (Item gone, play denied, bad input) is dropped so it can't block the queue. Another user's queued updates are never sent under the current session.Offline player. Playing a download (#225, below this PR in the stack) records into the queue on the online player's cadence: every 10 s while playing, on pause, at the end with
completed: true, and on close. The server's watched rules apply when it syncs, so #221's 90% rule covers offline viewing once it lands. Positions stop at the Version's probed length, saved with the download, since the server refuses any past it. The offline player route reads the last signed-in caller from the session the browse shell keeps (savedSession), without asking the server; with none kept it plays and records nothing.Closes #180
Conflicts with #221
Both touch
upsertProgress: #221 computescompleted: watched, this PR datesplayedAtandupdatedAtwithstamp. The resolution keeps both lines.Tests
apps/server/src/api/progress.test.ts(new case): an offline update applies dated when it happened; an older one loses to a live session's newer progress; a completed sync marks the Item watched and a futureatcounts as now; out-of-range positions answer 400 and aplaydeny 403.apps/server/src/api/offline-progress-browser.test.ts(new, headless Chromium): downloads a movie through the button, stops the server, and plays it from the Downloads page to the end. The recorded progress survives a fresh page load; once the server is back on the same port, the shell sends it, the queue empties, and the database row is completed at the movie's end, dated when it was watched rather than synced. Without the player wiring the queue stays empty and it fails. 10 of 10 passed with 12 cores busy.apps/web/src/lib/offline-progress.test.ts(new): the queue survives a reload until the server takes it; only the newest update per Item is sent, and one recorded mid-send stays; refused updates are dropped and other users' updates are left alone.progress,direct-play,marks,jellyfin/progress: 19 pass. Webbun test: 304 pass.bun run check: clean.bun run lint: 0 errors, the 9 CSS warnings already on main.No UI changes, so no screenshots.
Made with claude-opus-5.5 using Claude Code in T3 Code.
🤖 Generated with Claude Code