perf(series): the episode list no longer waits on the server
Opening Frasier on a Fairphone took ~5 s to render the episode list although every episode was cached. Three causes: - resolve_series_view waited for Next Up and resume before returning the episodes, and Next Up was server-first. The episode list now returns as soon as the episodes are in (with_hints); hints that have answered are used, late ones dropped, and the picker falls back to local watch state. Next Up is cache-first like every other query. - The page loaded itself six times per open: onMount plus a mount-time $effect, the reachability effect's first run posing as a reconnect, and a double mount. All triggers now share one coalesced load per item (createCoalescedLoader); refresh triggers get one re-run after it. - The root layout rendered the route in two branches that each rendered children; the page store deciding between them updates a flush late, so navigating Search -> library page mounted the page twice. One element now renders the route and only its classes change. On the device: one load per open, seasons from cache in 14 ms, episodes and the Resume button up in under a second (was ~5 s).
This commit is contained in:
@@ -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<void>((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<void>>()
|
||||
.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);
|
||||
});
|
||||
});
|
||||
@@ -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<void>;
|
||||
}
|
||||
|
||||
interface InFlight {
|
||||
key: string;
|
||||
/** Settles when this load and any re-run it owes have finished. */
|
||||
done: Promise<void>;
|
||||
rerun: boolean;
|
||||
}
|
||||
|
||||
export function createCoalescedLoader(run: (key: string) => Promise<void>): 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;
|
||||
},
|
||||
};
|
||||
}
|
||||
+24
-23
@@ -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<HTMLElement>();
|
||||
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}
|
||||
<!-- These routes own their own full-height flex column (header + scroller
|
||||
+ their own in-flow BottomUi), so the root just clips and steps back. -->
|
||||
<div class="flex-1 overflow-hidden">
|
||||
{@render children()}
|
||||
</div>
|
||||
{:else}
|
||||
<!-- Shared header (account menu, desktop nav) as a flex-shrink-0 sibling
|
||||
above the scroller, so it never eats into the scroller's bounds. -->
|
||||
{#if showGlobalHeader}
|
||||
<AppHeader />
|
||||
{/if}
|
||||
<!-- Scroller is flex-1/min-h-0; the in-flow BottomUi below is a flex
|
||||
sibling, so the list is physically bounded above it and can never
|
||||
render behind it. No measurement, no reserved padding. -->
|
||||
<div
|
||||
bind:this={shellScroller}
|
||||
class="flex-1 overflow-y-auto min-h-0"
|
||||
style="overscroll-behavior: contain"
|
||||
>
|
||||
{@render children()}
|
||||
</div>
|
||||
<!-- Shared header (account menu, desktop nav) as a flex-shrink-0 sibling
|
||||
above the scroller, so it never eats into the scroller's bounds. -->
|
||||
{#if !routeOwnsLayout && showGlobalHeader}
|
||||
<AppHeader />
|
||||
{/if}
|
||||
<!-- ONE element renders the route, whatever the layout mode; only its
|
||||
classes change. Routes that own their full-height column (header +
|
||||
scroller + their own in-flow BottomUi) get a plain clipped box; every
|
||||
other route gets the shell scroller (flex-1/min-h-0, bounded above the
|
||||
in-flow BottomUi, so no measurement or reserved padding).
|
||||
|
||||
This used to be two branches, each rendering `children`. The page
|
||||
store that decides the mode can update a flush after the new route
|
||||
renders, so navigating between the two kinds of route (Search → a
|
||||
library page) mounted the page under one branch and then *remounted*
|
||||
it under the other — every load it started, twice (DR-295). -->
|
||||
<div
|
||||
bind:this={shellScroller}
|
||||
class={routeOwnsLayout ? "flex-1 overflow-hidden" : "flex-1 overflow-y-auto min-h-0"}
|
||||
style={routeOwnsLayout ? undefined : "overscroll-behavior: contain"}
|
||||
>
|
||||
{@render children()}
|
||||
</div>
|
||||
|
||||
<!-- Re-authentication modal -->
|
||||
<ReauthModal isOpen={$needsReauth} />
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
<!-- TRACES: UR-035, UR-038, UR-048, UR-058, UR-062 | DR-043, DR-062, DR-102, DR-103, DR-142 -->
|
||||
<script lang="ts">
|
||||
import { onMount, untrack } from "svelte";
|
||||
import { untrack } from "svelte";
|
||||
import { formatDuration } from "$lib/utils/duration";
|
||||
import { page } from "$app/stores";
|
||||
import { goto } from "$app/navigation";
|
||||
@@ -51,6 +51,7 @@
|
||||
type SeasonData,
|
||||
} from "$lib/components/library/seriesNavigation";
|
||||
import { createLogger } from "$lib/utils/logger";
|
||||
import { createCoalescedLoader } from "$lib/utils/coalescedLoader";
|
||||
|
||||
const log = createLogger("LibraryDetail");
|
||||
|
||||
@@ -65,18 +66,27 @@
|
||||
// preference, so it resets with each load (DR-107).
|
||||
let expandedSeasons = $state<Set<string>>(new Set());
|
||||
|
||||
// Track if we've done an initial load and previous server state
|
||||
// Track if we've done an initial load and previous server state. The
|
||||
// previous state starts as "unknown" (null): the effect's first run only
|
||||
// records it. Starting at `false` made that first run look like a
|
||||
// reconnect and force a second, fresh load of the page on every open.
|
||||
let hasLoadedOnce = false;
|
||||
let previousServerReachable = false;
|
||||
let previousServerReachable: boolean | null = null;
|
||||
|
||||
const itemId = $derived($page.params.id);
|
||||
const focusedEpisodeId = $derived($page.url.searchParams.get("episode"));
|
||||
|
||||
onMount(async () => {
|
||||
await loadItem();
|
||||
hasLoadedOnce = true;
|
||||
});
|
||||
// Every trigger below goes through one coalesced loader: they used to each
|
||||
// start a full load, so opening a page loaded it six times over (DR-295).
|
||||
// `fresh` marks triggers that know the data changed.
|
||||
const loader = createCoalescedLoader(() => loadItemNow());
|
||||
function loadItem(options?: { fresh?: boolean }): Promise<void> {
|
||||
if (!itemId) return Promise.resolve();
|
||||
return loader.load(itemId, options);
|
||||
}
|
||||
const reloadFresh = () => loadItem({ fresh: true });
|
||||
|
||||
// Runs on mount and whenever the item changes.
|
||||
$effect(() => {
|
||||
if (itemId) {
|
||||
loadItem();
|
||||
@@ -89,8 +99,8 @@
|
||||
const serverReachable = $isServerReachable;
|
||||
|
||||
// If server just became reachable and we've already loaded, reload to get fresh data
|
||||
if (serverReachable && !previousServerReachable && hasLoadedOnce && itemId) {
|
||||
loadItem();
|
||||
if (serverReachable && previousServerReachable === false && hasLoadedOnce && itemId) {
|
||||
reloadFresh();
|
||||
}
|
||||
|
||||
previousServerReachable = serverReachable;
|
||||
@@ -100,10 +110,10 @@
|
||||
// contents follow the filter the same way a library listing does.
|
||||
// TRACES: UR-052 | DR-143
|
||||
useOfflineFilterReload(() => {
|
||||
if (itemId) loadItem();
|
||||
if (itemId) reloadFresh();
|
||||
});
|
||||
|
||||
async function loadItem() {
|
||||
async function loadItemNow() {
|
||||
if (!itemId) return;
|
||||
// Only show spinner when navigating to a different item
|
||||
// untrack prevents $effect from tracking `item` as a dependency (avoids infinite loop)
|
||||
@@ -579,13 +589,13 @@
|
||||
watched={allEpisodes.length > 0 && allEpisodes.every((e) => e.userData?.isPlayed)}
|
||||
scope="series"
|
||||
showLabel={true}
|
||||
onChanged={loadItem}
|
||||
onChanged={reloadFresh}
|
||||
/>
|
||||
<ClearHistoryButton
|
||||
itemId={item.id}
|
||||
itemName={item.name}
|
||||
scope="series"
|
||||
onCleared={loadItem}
|
||||
onCleared={reloadFresh}
|
||||
/>
|
||||
{:else if item.kind === "movie"}
|
||||
<VideoDownloadButton
|
||||
@@ -600,7 +610,7 @@
|
||||
watched={item.userData?.isPlayed ?? false}
|
||||
scope="episode"
|
||||
showLabel={true}
|
||||
onChanged={loadItem}
|
||||
onChanged={reloadFresh}
|
||||
/>
|
||||
{/if}
|
||||
<!-- Favourite. Sits with Play/Download rather than in the header,
|
||||
@@ -732,7 +742,7 @@
|
||||
expanded={expandedSeasons.has(season.id)}
|
||||
onToggle={() => toggleSeason(season.id)}
|
||||
onEpisodeClick={handleEpisodeClick}
|
||||
onHistoryCleared={loadItem}
|
||||
onHistoryCleared={reloadFresh}
|
||||
/>
|
||||
{/each}
|
||||
{/if}
|
||||
|
||||
Reference in New Issue
Block a user