diff --git a/docs/architecture/02-svelte-frontend.md b/docs/architecture/02-svelte-frontend.md index dd4e9e83b..d3725dcd7 100644 --- a/docs/architecture/02-svelte-frontend.md +++ b/docs/architecture/02-svelte-frontend.md @@ -666,6 +666,14 @@ render behind it. There is no measurement and no reserved padding. If you restructure the shell, preserve the scroll containment — reintroducing padding math reintroduces the bug. +**The route renders in exactly one element.** `+layout.svelte` switches the +wrapper's *classes* between the shell scroller and the plain clipped box that +layout-owning routes (library, settings, player) get — it must not switch +between two branches that each render `children`. The page store that decides +the mode can update a flush after the new route renders, so two branches +mounted a page under one and then remounted it under the other: every +navigation between the two kinds of route loaded the page twice (DR-295). + ### AccountMenu One component for both breakpoints, anchored to the username/avatar (a real @@ -719,6 +727,22 @@ hero button labelled `Resume S2E4` / `Play S1E1`. A season is not a destination: `/library/` redirects to its series (DR-103). Video library routes collapse to one per library (DR-105). +**The episode list never waits for Next Up or resume.** `resolve_series_view` +(`series_progress.rs`, `with_hints`) returns as soon as the episodes are in; +Next Up and resume are used if they have answered by then and dropped if not, +and `pick_current_episode` falls back to the episodes' own watch state. They +only refine which episode is current, and waiting for them held the list for +the server's 2–3 s although every episode was cached. Next Up is cache-first +like every other query (03-data-flow). + +**One load per item, however many triggers.** The detail page loads through +`createCoalescedLoader` (`utils/coalescedLoader.ts`): calls for the item +already loading share that load, and callers that know the data changed +(`fresh`: reconnect, filter change, mark watched, clear history) get exactly +one re-run after it. `onMount`, a mount-time `$effect`, the reachability +effect's first run and the double mount above used to each start a full load — +six per open, about seventy requests in flight. + `episodeStrip.ts` holds the pure logic for the "More Episodes" strip, extracted from the component because it had three distinct bugs that markup made untestable: the strip collapsing to just the current episode while real siblings diff --git a/src-tauri/src/repository/hybrid.rs b/src-tauri/src/repository/hybrid.rs index a4c71b04f..e9a2bfbac 100644 --- a/src-tauri/src/repository/hybrid.rs +++ b/src-tauri/src/repository/hybrid.rs @@ -1000,22 +1000,32 @@ impl MediaRepository for HybridRepository { series_id: Option<&str>, limit: Option, ) -> Result, RepoError> { - // Next Up is dynamic, so the server's answer is preferred — but when the - // server cannot answer, the cache's stands in. It used to be server-only, - // and the TV landing page loads Next Up in one `Promise.all` with its - // other rows, so offline that single failure blanked the whole page with - // Continue Watching and Latest sitting in the cache (DR-294). - // TRACES: UR-002 | DR-294 | UT-261 - match self.online.get_next_up_episodes(series_id, limit).await { - Ok(items) => Ok(items.without_excluded()), - Err(e) => { - debug!("[HybridRepo] Next Up from server failed ({e}); using the cache"); - self.offline - .get_next_up_episodes(series_id, limit) - .await - .map(ExcludeHidden::without_excluded) - } - } + // Cache-first like every other query: the local answer is computed + // from the same watch state the cache refreshes from the server in the + // background (`user_data_mirror_query`), and whichever answers first + // with content wins. It used to wait for the server outright, which + // held the series page's episode list for 2-3 s on a phone. An empty + // local answer still defers to the server, and a failed server falls + // back to the cache — offline, the TV page's Next Up row must not blank + // the page (DR-294). + // TRACES: UR-002 | DR-013, DR-294 | UT-261 + let offline = Arc::clone(&self.offline); + let online = Arc::clone(&self.online); + let series = series_id.map(str::to_string); + let series_for_server = series.clone(); + + let cache_future = + Self::cache_leg( + async move { offline.get_next_up_episodes(series.as_deref(), limit).await }, + ) + .await; + let server_future = async move { + online + .get_next_up_episodes(series_for_server.as_deref(), limit) + .await + }; + + Self::parallel_race(cache_future, server_future).await } async fn get_recently_played_audio( diff --git a/src-tauri/src/repository/series_progress.rs b/src-tauri/src/repository/series_progress.rs index b1173bb34..07badd5e6 100644 --- a/src-tauri/src/repository/series_progress.rs +++ b/src-tauri/src/repository/series_progress.rs @@ -287,24 +287,56 @@ pub async fn resolve_series_view( repo: &dyn MediaRepository, series_id: &str, ) -> Result { - let (episodes, next_up, resume) = futures_util::join!( - fetch_series_episodes(repo, series_id), - async { - repo.get_next_up_episodes(Some(series_id), Some(1)) - .await - .unwrap_or_default() - }, - async { - repo.get_resume_items(Some(series_id), Some(10)) - .await - .unwrap_or_default() - }, - ); + let (episodes, (next_up, resume)) = with_hints(fetch_series_episodes(repo, series_id), async { + futures_util::join!( + async { + repo.get_next_up_episodes(Some(series_id), Some(1)) + .await + .unwrap_or_default() + }, + async { + repo.get_resume_items(Some(series_id), Some(10)) + .await + .unwrap_or_default() + }, + ) + }) + .await; let episodes = episodes?; let current = pick_current_episode(series_id, &episodes, &next_up, &resume); Ok(SeriesView { episodes, current }) } +/// Run `primary` and `hints` together, but never hold `primary` back for +/// `hints`: once `primary` is ready, the hints are taken if they have already +/// answered and dropped (`H::default()`) if not. +/// +/// For the series view the primary is the episode list and the hints are Next +/// Up and resume, which only refine which episode is "current" — and the +/// picker falls back to the episodes' own watch state without them. Waiting +/// for them made the episode list wait for the server (2-3 s on a phone) +/// although every episode was in the cache in 50 ms. The cache legs of the +/// hints usually answer before the episodes do, so they are normally kept. +/// +/// TRACES: UR-062 | DR-101, DR-295 +async fn with_hints( + primary: impl std::future::Future, + hints: impl std::future::Future, +) -> (P, H) +where + H: Default, +{ + use futures_util::future::{select, Either}; + use futures_util::FutureExt; + + let primary = std::pin::pin!(primary); + let hints = std::pin::pin!(hints); + match select(primary, hints).await { + Either::Left((primary, hints)) => (primary, hints.now_or_never().unwrap_or_default()), + Either::Right((hints, primary)) => (primary.await, hints), + } +} + /// Resolve the current episode, fetching everything the policy needs. /// /// Next Up and resume are best-effort: offline they fail or come back empty, and @@ -404,6 +436,55 @@ mod tests { assert_eq!(episodes.len(), 9, "every season but the failing one"); } + /// The episode list must not wait for Next Up or resume. + /// + /// The series page rendered its episodes only once Next Up had come back + /// from the server — 2-3 s on a phone while the page's other requests were + /// in flight — although every episode was in the cache after 50 ms. Those + /// two only refine which episode is "current", and the picker falls back + /// to the episodes' own watch state without them. + /// + /// TRACES: UR-062 | DR-101, DR-295 + #[tokio::test] + async fn the_episode_list_does_not_wait_for_slow_hints() { + let started = std::time::Instant::now(); + let (episodes, hints) = with_hints( + async { + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + vec![episode("e1", 1, 1)] + }, + async { + tokio::time::sleep(std::time::Duration::from_millis(2000)).await; + vec![episode("from-server", 1, 2)] + }, + ) + .await; + let elapsed = started.elapsed(); + + assert_eq!(episodes.len(), 1); + assert!(hints.is_empty(), "late hints are dropped, not waited for"); + assert!( + elapsed < std::time::Duration::from_millis(500), + "the episode list waited {elapsed:?} for Next Up / resume" + ); + } + + /// Hints that are already in (a cache answer) are used. + /// + /// TRACES: UR-062 | DR-101, DR-295 + #[tokio::test] + async fn hints_that_answer_first_are_kept() { + let (_, hints) = with_hints( + async { + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + vec![episode("e1", 1, 1)] + }, + async { vec![episode("cached", 1, 2)] }, + ) + .await; + assert_eq!(hints.len(), 1); + } + fn watched(mut item: MediaItem) -> MediaItem { item.user_data = Some(UserData { is_played: Some(true), diff --git a/src/lib/utils/coalescedLoader.test.ts b/src/lib/utils/coalescedLoader.test.ts new file mode 100644 index 000000000..58445dfdd --- /dev/null +++ b/src/lib/utils/coalescedLoader.test.ts @@ -0,0 +1,86 @@ +import { describe, it, expect, vi } from "vitest"; +import { createCoalescedLoader } from "./coalescedLoader"; + +/** + * TRACES: UR-062 | DR-295 + * + * The series page loaded itself six times on every open: `onMount` and a + * `$effect` both ran on mount, the "server became reachable" effect fired on + * its first run, and navigation updates re-ran the effect. Each load repeated + * the item, the season list and the whole series view — about six times a + * dozen requests in flight at once, which alone slowed every server call on a + * phone to 2-3 s. + */ +function deferred() { + let resolve!: () => void; + const promise = new Promise((r) => (resolve = r)); + return { promise, resolve }; +} + +describe("createCoalescedLoader", () => { + it("shares one run between calls for the same key while it is in flight", async () => { + const gate = deferred(); + const run = vi.fn(() => gate.promise); + const loader = createCoalescedLoader(run); + + const calls = [1, 2, 3, 4, 5, 6].map(() => loader.load("frasier")); + gate.resolve(); + await Promise.all(calls); + + expect(run).toHaveBeenCalledTimes(1); + }); + + it("re-runs once after the in-flight load when a caller needs fresh data", async () => { + const gates = [deferred(), deferred()]; + let n = 0; + const run = vi.fn(() => gates[n++].promise); + const loader = createCoalescedLoader(run); + + const first = loader.load("frasier"); + // e.g. "mark watched" finished while the page was still loading: the + // in-flight load may predate the change, so it must not be the answer. + const fresh = loader.load("frasier", { fresh: true }); + const fresh2 = loader.load("frasier", { fresh: true }); + gates[0].resolve(); + await vi.waitFor(() => expect(run).toHaveBeenCalledTimes(2)); + gates[1].resolve(); + // Every caller is answered by the load that includes the re-run. + await Promise.all([first, fresh, fresh2]); + + expect(run).toHaveBeenCalledTimes(2); + }); + + it("does not share a run between different keys", async () => { + const run = vi.fn(() => Promise.resolve()); + const loader = createCoalescedLoader(run); + + await Promise.all([loader.load("frasier"), loader.load("cheers")]); + + expect(run).toHaveBeenCalledTimes(2); + expect(run).toHaveBeenNthCalledWith(1, "frasier"); + expect(run).toHaveBeenNthCalledWith(2, "cheers"); + }); + + it("runs again once the previous load has finished", async () => { + const run = vi.fn(() => Promise.resolve()); + const loader = createCoalescedLoader(run); + + await loader.load("frasier"); + await loader.load("frasier"); + + expect(run).toHaveBeenCalledTimes(2); + }); + + it("releases the key when a load fails", async () => { + const run = vi + .fn<(key: string) => Promise>() + .mockRejectedValueOnce(new Error("offline")) + .mockResolvedValueOnce(undefined); + const loader = createCoalescedLoader(run); + + await expect(loader.load("frasier")).rejects.toThrow("offline"); + await loader.load("frasier"); + + expect(run).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/lib/utils/coalescedLoader.ts b/src/lib/utils/coalescedLoader.ts new file mode 100644 index 000000000..7fa0eeb12 --- /dev/null +++ b/src/lib/utils/coalescedLoader.ts @@ -0,0 +1,55 @@ +/** + * Load one keyed thing at a time, however many triggers ask for it. + * + * Calls for the key already loading share that load instead of starting their + * own. A caller that knows the data changed (`fresh` — after "mark watched", + * on reconnect, when a filter flips) must not be answered by a load that may + * predate the change, so it gets exactly one re-run once the current load + * ends, however many such callers there were. + * + * Exists because the series page loaded itself six times on every open — + * `onMount`, a mount-time `$effect`, the reachability effect's first run and + * navigation updates each started a full load — putting about six times a + * dozen requests in flight at once. + * + * TRACES: UR-062 | DR-295 + */ +export interface CoalescedLoader { + /** Load `key`. `fresh`: the caller knows the data changed. */ + load(key: string, options?: { fresh?: boolean }): Promise; +} + +interface InFlight { + key: string; + /** Settles when this load and any re-run it owes have finished. */ + done: Promise; + rerun: boolean; +} + +export function createCoalescedLoader(run: (key: string) => Promise): CoalescedLoader { + let inFlight: InFlight | null = null; + + return { + load(key, options = {}) { + if (inFlight && inFlight.key === key) { + if (options.fresh) inFlight.rerun = true; + return inFlight.done; + } + + const entry: InFlight = { key, rerun: false, done: Promise.resolve() }; + entry.done = (async () => { + try { + await run(key); + while (entry.rerun) { + entry.rerun = false; + await run(key); + } + } finally { + if (inFlight === entry) inFlight = null; + } + })(); + inFlight = entry; + return entry.done; + }, + }; +} diff --git a/src/routes/+layout.svelte b/src/routes/+layout.svelte index eeb8b7a09..51dbb4883 100644 --- a/src/routes/+layout.svelte +++ b/src/routes/+layout.svelte @@ -73,7 +73,8 @@ // a new page inherits the previous page's offset. Must be registered here at // init, alongside the tracker above, for the same reason. (DR-156) let shellScroller = $state(); - useScrollRestore(() => shellScroller, "shell"); + // Owned routes scroll inside their own column; the shell box does not. + useScrollRestore(() => (routeOwnsLayout ? undefined : shellScroller), "shell"); // Layout-shell visibility rules live in one pure, unit-tested module // ($lib/utils/layoutShell) so they can't drift per route/platform. @@ -357,29 +358,29 @@ scrolling internally. All other top-level pages render directly here, so this wrapper must scroll and reserve the fixed bottom UI's measured height so the mini player / bottom nav never overlap the last rows. --> - {#if routeOwnsLayout} - -
- {@render children()} -
- {:else} - - {#if showGlobalHeader} - - {/if} - -
- {@render children()} -
+ + {#if !routeOwnsLayout && showGlobalHeader} + {/if} + +
+ {@render children()} +
diff --git a/src/routes/library/[id]/+page.svelte b/src/routes/library/[id]/+page.svelte index c3df440e5..c4c6dcd33 100644 --- a/src/routes/library/[id]/+page.svelte +++ b/src/routes/library/[id]/+page.svelte @@ -1,6 +1,6 @@