From 5fedf743c16d51f0f1cb0f9aba26b08173533dcc Mon Sep 17 00:00:00 2001 From: KodeStar Date: Fri, 23 Oct 2026 19:05:56 +0100 Subject: [PATCH 1/5] Downloads: a re-ordered book's offline copy takes the server's file order Server v2.4.0 plays a book's files in natural order, so a downloaded book can come back with the same files in another order. refreshedChapters refused that, leaving the offline copy on the old whole-book timeline while the server and an online start used the new one. The same file set in a new order now adopts the server's chapters and order (no download), held while the player has the book loaded in the old order and taken from the cached answer once the loaded book changes. --- CONVENTIONS.md | 10 +++- src/downloads/store.test.ts | 91 +++++++++++++++++++++++++++++++++- src/downloads/store.ts | 97 +++++++++++++++++++++++++++++-------- src/playback/store.test.ts | 76 ++++++++++++++++++++++++++++- src/playback/store.ts | 27 ++++++++++- 5 files changed, 277 insertions(+), 24 deletions(-) diff --git a/CONVENTIONS.md b/CONVENTIONS.md index 4c768dca..853404ec 100644 --- a/CONVENTIONS.md +++ b/CONVENTIONS.md @@ -1769,8 +1769,14 @@ are asked, at most 10,000 rows. rescan, a community chapter list, `chapters_source: "community"`). `startChapterRefresh` (root layout) watches the query cache: a server answer for a downloaded book's chapters replaces `manifest.chapters` when its audio files are the ones on the device (`refreshedChapters`: same - paths, same order, same sizes where both answers know them); otherwise the manifest stays (that - is a new download). An answer that reached the cache before the book was downloaded (while its + paths, same sizes where both answers know them); otherwise the manifest stays (that is a new + download). The same files in another order (server v2.4.0 plays them in natural order, "2-" + before "10-") are the same audio: the manifest takes the server's order (`manifest.files` + reordered, nothing downloaded again) with its chapters, so the offline whole-book timeline and + every place saved on it is the server's. A new order is held while the player has the book + loaded in another one (`refreshedFor`, probing the playback store's `setLoadedFileOrder`: the + engine plays by track index, and Android's `adoptLoaded` gets only an index), and taken from + the cached answer once the loaded book changes (`adoptCachedChapters`). An answer that reached the cache before the book was downloaded (while its files did, or before launch's `hydrate`) is taken as it becomes downloaded (`withCachedChapters`). The audio is never touched, and a queue already playing keeps its chapters until the book is next started. diff --git a/src/downloads/store.test.ts b/src/downloads/store.test.ts index 68cf6000..7878c99f 100644 --- a/src/downloads/store.test.ts +++ b/src/downloads/store.test.ts @@ -136,9 +136,11 @@ import { downloadedCountFor, downloadedEntryOf, downloadKey, + adoptCachedChapters, isDeclined, refreshedChapters, reviveEntry, + setLoadedFileOrder, startChapterRefresh, useDownloads, } from '@/downloads/store'; @@ -558,8 +560,32 @@ describe("refreshing a downloaded book's chapters", () => { it('leaves the manifest alone when the files differ (a re-download)', () => { const e = downloadedEntry(); expect(refreshedChapters(e.manifest, fresh(['A/Book/01.mp3']))).toBeNull(); - expect(refreshedChapters(e.manifest, fresh(['A/Book/02.mp3', 'A/Book/01.mp3']))).toBeNull(); expect(refreshedChapters(e.manifest, fresh(['A/Book/01.mp3', 'A/Book/03.mp3']))).toBeNull(); + expect(refreshedChapters(e.manifest, fresh(['A/Book/01.mp3', 'A/Book/01.mp3']))).toBeNull(); + expect( + refreshedChapters(e.manifest, fresh(['A/Book/01.mp3', 'A/Book/02.mp3', 'A/Book/03.mp3'])), + ).toBeNull(); + }); + + it('takes the server order with its chapters when the same files come in another order', () => { + const e = downloadedEntry(); + e.manifest.chapters = fresh(undefined, { chapters_source: undefined }); + const reordered = fresh(['A/Book/02.mp3', 'A/Book/01.mp3'], { chapters_source: undefined }); + const next = refreshedChapters(e.manifest, reordered); + expect(next?.chapters).toBe(reordered); + // The very same local files, nothing downloaded again: only their order moves. + expect(next?.files).toEqual([e.manifest.files[1], e.manifest.files[0]]); + expect(next?.files[0]).toBe(e.manifest.files[1]); + expect({ ...next, chapters: null, files: null }).toEqual({ + ...e.manifest, + chapters: null, + files: null, + }); + // A file replaced in place on the way is still a new download. + reordered.files = reordered.files.map((f) => + f.rel_path === 'A/Book/01.mp3' ? { ...f, size: 2 } : f, + ); + expect(refreshedChapters(e.manifest, reordered)).toBeNull(); }); it('leaves the manifest alone when a file at the same path changed size (replaced in place)', () => { @@ -657,6 +683,69 @@ describe("refreshing a downloaded book's chapters", () => { expect(useDownloads.getState().entries[key]).toBe(queued); }); + describe('a new file order', () => { + const key = downloadKey('c1', 2, 'A/Book'); + const reordered = () => fresh(['A/Book/02.mp3', 'A/Book/01.mp3']); + const order = () => useDownloads.getState().entries[key].manifest.files.map((f) => f.relPath); + const loaded = jest.fn((..._a: unknown[]): string[] | null => null); + beforeEach(() => { + loaded.mockReset().mockReturnValue(null); + setLoadedFileOrder((ref) => loaded(ref)); + useDownloads.setState({ entries: { [key]: downloadedEntry() } }); + }); + afterEach(() => { + setLoadedFileOrder(() => null); + mockGetQueryData.mockReset(); + }); + + it('is taken at once when the player does not have the book loaded', () => { + answer(reordered()); + expect(order()).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); + expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(reordered()); + expect(loaded).toHaveBeenCalledWith({ connectionId: 'c1', libraryId: 2, path: 'A/Book' }); + }); + + it('is taken at once when the loaded queue already plays it', () => { + loaded.mockReturnValue(['A/Book/02.mp3', 'A/Book/01.mp3']); + answer(reordered()); + expect(order()).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); + }); + + it('waits while the book is loaded in its old order, then is taken from the cache', () => { + loaded.mockReturnValue(['A/Book/01.mp3', 'A/Book/02.mp3']); + const e = useDownloads.getState().entries[key]; + answer(reordered()); + expect(useDownloads.getState().entries[key]).toBe(e); + // Chapters alone (the same order) still refresh while it plays, as before. + answer(fresh()); + expect(order()).toEqual(['A/Book/01.mp3', 'A/Book/02.mp3']); + expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(fresh()); + // Still loaded that way: the cached answer waits. + mockGetQueryData.mockImplementation((k: unknown) => + Array.isArray(k) && k[0] === 'chapters' && k[3] === 'A/Book' ? reordered() : undefined, + ); + adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: 'A/Book' }); + expect(order()).toEqual(['A/Book/01.mp3', 'A/Book/02.mp3']); + // The player let it go. + loaded.mockReturnValue(null); + adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: 'A/Book' }); + expect(order()).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); + expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(reordered()); + }); + + it('leaves a different set of files alone, loaded or not', () => { + const e = useDownloads.getState().entries[key]; + answer(fresh(['A/Book/02.mp3', 'A/Book/03.mp3'])); + loaded.mockReturnValue(['A/Book/02.mp3', 'A/Book/03.mp3']); + answer(fresh(['A/Book/02.mp3', 'A/Book/03.mp3'])); + expect(useDownloads.getState().entries[key]).toBe(e); + // Nothing cached, or a book not downloaded: nothing to take. + adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: 'A/Book' }); + adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: 'B/Book' }); + expect(useDownloads.getState().entries[key]).toBe(e); + }); + }); + it('stops listening once unsubscribed', () => { const e = downloadedEntry(); const key = downloadKey('c1', 2, 'A/Book'); diff --git a/src/downloads/store.ts b/src/downloads/store.ts index f1e11009..19f371eb 100644 --- a/src/downloads/store.ts +++ b/src/downloads/store.ts @@ -11,6 +11,7 @@ import { mapLimit } from '@/lib/map-limit'; import { getItem, setItem } from '@/lib/storage'; import { DOWNLOADS_KEY } from '@/lib/storage-keys'; import { bookFileSpecs } from '@/playback/book-queue'; +import type { BookRef } from '@/playback/types'; import { webTranscodeFromCache } from '@/playback/transcode-capability'; import { onConnectionRemoved } from '@/stores/session'; @@ -372,10 +373,13 @@ export function reviveEntry(e: DownloadEntry, allPresent: boolean): DownloadEntr * ones, or null to leave it as it is. A book's chapters can change on the server after * it was downloaded (a rescan, a community chapter list fitted onto its audio, the admin * switching it back), and offline it plays and lists what the manifest holds. Only the - * chapters move: when the fresh answer's audio files are not the ones on the device (the - * same paths in the same order, `bookFileSpecs` as the download read them, and the same - * sizes where both answers know them: a file replaced in place keeps its path), the local - * audio no longer matches and that is a new download, so nothing changes. Null too when + * chapters move, and with them the files' play order: when the fresh answer's audio files + * are not the ones on the device (the same paths, `bookFileSpecs` as the download read + * them, and the same sizes where both answers know them: a file replaced in place keeps + * its path), the local audio no longer matches and that is a new download, so nothing + * changes. The same files in another order (a server that now sorts "2-" before "10-") + * are the same audio: the manifest takes the server's order with its chapters, so its + * whole-book timeline (and every place saved on it) is the server's. Null too when * nothing differs. */ export function refreshedChapters( @@ -385,19 +389,58 @@ export function refreshedChapters( // An unchanged refetch hands back the same object (structural sharing): skip the compare. if (fresh === manifest.chapters) return null; const want = bookFileSpecs(manifest.book, fresh); - // The files as the download read them (sizes 0 when only the chapters named them). - const read = bookFileSpecs(manifest.book, manifest.chapters ?? undefined); const have = manifest.files; - const same = - want.length === have.length && - want.every((s, i) => { - if (s.path !== have[i].relPath) return false; - const was = read[i]; - return was?.path !== s.path || !(was.size > 0 && s.size > 0) || was.size === s.size; - }); - if (!same) return null; - if (JSON.stringify(fresh) === JSON.stringify(manifest.chapters)) return null; - return { ...manifest, chapters: fresh }; + if (want.length !== have.length) return null; + // The sizes as the download read them (0 when only the chapters named the files). + const readSize = new Map( + bookFileSpecs(manifest.book, manifest.chapters ?? undefined).map((s) => [s.path, s.size]), + ); + const onDevice = new Map(have.map((f) => [f.relPath, f])); + // The device's files in the fresh answer's order: each one exactly once. + const files: DownloadedFile[] = []; + for (const s of want) { + const f = onDevice.get(s.path); + if (!f) return null; + onDevice.delete(s.path); + const was = readSize.get(s.path) ?? 0; + if (was > 0 && s.size > 0 && was !== s.size) return null; + files.push(f); + } + const reordered = files.some((f, i) => f !== have[i]); + if (!reordered && JSON.stringify(fresh) === JSON.stringify(manifest.chapters)) return null; + return reordered ? { ...manifest, chapters: fresh, files } : { ...manifest, chapters: fresh }; +} + +/** The file order (book-relative paths) the player has a book loaded in, or null when it + * is not the book loaded. The playback store sets it (`setLoadedFileOrder`): it imports + * this module, so this one cannot ask it directly. */ +let loadedFileOrder: (ref: BookRef) => string[] | null = () => null; + +/** Set by the playback store at import (see `loadedFileOrder`). */ +export function setLoadedFileOrder(probe: typeof loadedFileOrder): void { + loadedFileOrder = probe; +} + +/** + * `refreshedChapters`, held back while the player has the book loaded in another order + * than the fresh one: the engine plays that queue's files by index (Android's service + * hands a JS restart only a track index, `adoptLoaded`), so the manifest must not move + * its files under it. A queue already in the fresh order (a start from the server's + * answer) takes it at once; a held one is taken once the book is no longer loaded that + * way (`adoptCachedChapters`, from the playback store). + */ +function refreshedFor( + ref: BookRef, + manifest: DownloadManifest, + fresh: ChaptersResponse, +): DownloadManifest | null { + const next = refreshedChapters(manifest, fresh); + if (!next || next.files === manifest.files) return next; + const loaded = loadedFileOrder(ref); + const inLoadedOrder = + !loaded || + (loaded.length === next.files.length && loaded.every((p, i) => p === next.files[i].relPath)); + return inLoadedOrder ? next : null; } /** @@ -416,7 +459,21 @@ function withCachedChapters( const cached = queryClient.getQueryData( qk.chapters(connectionId, libraryId, path), ); - return (cached && refreshedChapters(manifest, cached)) || manifest; + return (cached && refreshedFor({ connectionId, libraryId, path }, manifest, cached)) || manifest; +} + +/** + * A downloaded book takes the chapters answer the cache holds for it now, when + * `refreshedChapters` allows: for one `refreshedFor` held back while it was loaded in its + * old order (the playback store calls this when the loaded book changes). + */ +export function adoptCachedChapters(ref: BookRef): void { + const entry = downloadedEntryOf(ref); + if (!entry) return; + const manifest = withCachedChapters(ref.connectionId, ref.libraryId, ref.path, entry.manifest); + if (manifest === entry.manifest) return; + patchEntry(downloadKey(ref.connectionId, ref.libraryId, ref.path), { manifest }); + persistSoon(); } /** @@ -424,7 +481,8 @@ function withCachedChapters( * server gives (any screen's fetch, the play and download paths) for a downloaded book * replaces its saved copy when `refreshedChapters` allows. The cache already holds that * answer, so there is nothing to seed. A queue already playing keeps its chapters until - * the book is next started. Returns the unsubscribe (started once from the root layout). + * the book is next started, and a new file order waits until then (`refreshedFor`). + * Returns the unsubscribe (started once from the root layout). */ export function startChapterRefresh(): () => void { return queryClient.getQueryCache().subscribe((event) => { @@ -436,7 +494,8 @@ export function startChapterRefresh(): () => void { const key = downloadKey(book.cid, book.libraryId, book.path); const entry = useDownloads.getState().entries[key]; if (entry?.status !== 'downloaded') return; - const manifest = refreshedChapters(entry.manifest, fresh); + const ref = { connectionId: book.cid, libraryId: book.libraryId, path: book.path }; + const manifest = refreshedFor(ref, entry.manifest, fresh); if (!manifest) return; patchEntry(key, { manifest }); // A burst of answers (a reconnect's refetches, a search fan-out) saves the registry diff --git a/src/playback/store.test.ts b/src/playback/store.test.ts index 06ac0898..dbe5139a 100644 --- a/src/playback/store.test.ts +++ b/src/playback/store.test.ts @@ -1,4 +1,4 @@ -import type { Book, Progress } from '@/api/types'; +import type { Book, ChaptersResponse, Progress } from '@/api/types'; import type { ResumeLookup } from './progress-sync'; import type { PlaybackService, PlaybackSnapshot, PlaybackState } from './types'; @@ -1345,6 +1345,80 @@ describe('the download hot-swap', () => { }); }); +// --- a downloaded book whose server now plays its files in another order ----------- +// (server v2.4.0's natural order: "2-" before "10-"). The download takes the server's order +// and chapters, but never under a queue the player holds in the old order. +describe('a downloaded book re-ordered on its server', () => { + const folder = 'A/Folder'; + const paths = ['A/Folder/10.mp3', 'A/Folder/2.mp3']; + const chaptersIn = (order: string[]): ChaptersResponse => ({ + library_id: 2, + path: folder, + duration: 100, + is_folder: true, + files: order.map((rel_path, seq) => ({ rel_path, seq, duration: 50, format: 'mp3', size: 1 })), + chapters: order.map((file_path, index) => ({ + index, + title: `Chapter ${index + 1}`, + file_index: index, + file_path, + start: 0, + end: 50, + book_offset: index * 50, + })), + }); + const book = makeBook({ rel_path: folder, is_folder: true, format: 'mp3' }); + const key = `c1:2:${folder}`; + const order = () => useDownloads.getState().entries[key].manifest.files.map((f) => f.relPath); + + beforeEach(() => { + const manifest: DownloadManifest = { + book, + chapters: chaptersIn(paths), + files: paths.map((relPath, i) => ({ relPath, localUri: `file:///dl/${i}.mp3` })), + coverUri: null, + savedAt: '2026-01-01T00:00:00Z', + }; + useDownloads.setState({ + entries: { + [key]: { + connectionId: 'c1', + libraryId: 2, + path: folder, + title: book.title, + status: 'downloaded', + progress: 1, + bytes: 0, + totalBytes: 0, + manifest, + }, + }, + }); + // The server's answer, in its new order, sits in the query cache. + (queryClient.getQueryData as jest.Mock).mockImplementation((k: unknown) => + Array.isArray(k) && k[0] === 'chapters' && k[3] === folder + ? chaptersIn([...paths].reverse()) + : undefined, + ); + }); + afterEach(() => (queryClient.getQueryData as jest.Mock).mockImplementation(() => undefined)); + + it("takes the server's order once a start plays it", async () => { + await usePlayer.getState().playBook('c1', 2, book, chaptersIn([...paths].reverse()), 0); + const tracks = usePlayer.getState().nowPlaying?.queue.tracks.map((t) => t.url); + expect(tracks).toEqual(['file:///dl/1.mp3', 'file:///dl/0.mp3']); + expect(order()).toEqual(['A/Folder/2.mp3', 'A/Folder/10.mp3']); + }); + + it('keeps the old order while a queue plays it, and takes the new one once it is gone', async () => { + // Started from the download's own copy (offline): the old order. + await usePlayer.getState().playBook('c1', 2, book, chaptersIn(paths), 0); + expect(order()).toEqual(paths); + usePlayer.setState({ nowPlaying: null }); + expect(order()).toEqual(['A/Folder/2.mp3', 'A/Folder/10.mp3']); + }); +}); + // --- offline playback of a downloaded book whose connection is gone ---------------- // A connection whose secure-store token fails to hydrate is dropped from the session, so // resolveClient() returns null - but downloads.hydrate keeps its entry. A fully-local diff --git a/src/playback/store.ts b/src/playback/store.ts index f4bb5c68..b1bc889b 100644 --- a/src/playback/store.ts +++ b/src/playback/store.ts @@ -6,7 +6,13 @@ import { qk } from '@/api/hooks'; import { queryClient } from '@/api/provider'; import { isReachable, noteError } from '@/api/reachability'; import type { Book, Chapter, ChaptersResponse, Progress } from '@/api/types'; -import { downloadedEntryOf, downloadKey, useDownloads } from '@/downloads/store'; +import { + adoptCachedChapters, + downloadedEntryOf, + downloadKey, + setLoadedFileOrder, + useDownloads, +} from '@/downloads/store'; import { resolveConnectionId } from '@/lib/connection-id'; import { contentKey } from '@/lib/content-key'; import { canAutoDownload } from '@/lib/network'; @@ -1378,6 +1384,25 @@ useDownloads.subscribe((state, prev) => { } }); +// A downloaded book's files take a new play order from its server only while the player +// does not hold it in another order (`refreshedFor` in the downloads store): this is the +// order the loaded queue plays, by book-relative path (a track's id is `:`). +setLoadedFileOrder((ref) => { + const np = usePlayer.getState().nowPlaying; + if (!np || np.connectionId !== ref.connectionId || np.libraryId !== ref.libraryId) return null; + if (np.path !== ref.path) return null; + const prefix = `${np.libraryId}:`; + return np.queue.tracks.map((t) => t.id.slice(prefix.length)); +}); + +// And once the loaded book changes, a new order held back for the book that was loaded, +// or for the one now loaded in that order, is taken from the cached answer. +usePlayer.subscribe((state, prev) => { + if (state.nowPlaying === prev.nowPlaying) return; + if (prev.nowPlaying) adoptCachedChapters(prev.nowPlaying); + if (state.nowPlaying) adoptCachedChapters(state.nowPlaying); +}); + // --- selectors ------------------------------------------------------------- export const selectBookPosition = (s: PlayerState): number => s.nowPlaying From 115d1330fd94ff11b39ee8ac3d2f6d2f39cb4176 Mon Sep 17 00:00:00 2001 From: KodeStar Date: Fri, 23 Oct 2026 19:10:56 +0100 Subject: [PATCH 2/5] Simplify pass --- src/downloads/store.test.ts | 9 +++++---- src/downloads/store.ts | 14 ++++++++------ src/playback/store.test.ts | 9 +++++---- src/playback/store.ts | 17 ++++++++++------- 4 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/downloads/store.test.ts b/src/downloads/store.test.ts index 7878c99f..fa63f406 100644 --- a/src/downloads/store.test.ts +++ b/src/downloads/store.test.ts @@ -685,6 +685,7 @@ describe("refreshing a downloaded book's chapters", () => { describe('a new file order', () => { const key = downloadKey('c1', 2, 'A/Book'); + const ref = { connectionId: 'c1', libraryId: 2, path: 'A/Book' }; const reordered = () => fresh(['A/Book/02.mp3', 'A/Book/01.mp3']); const order = () => useDownloads.getState().entries[key].manifest.files.map((f) => f.relPath); const loaded = jest.fn((..._a: unknown[]): string[] | null => null); @@ -702,7 +703,7 @@ describe("refreshing a downloaded book's chapters", () => { answer(reordered()); expect(order()).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(reordered()); - expect(loaded).toHaveBeenCalledWith({ connectionId: 'c1', libraryId: 2, path: 'A/Book' }); + expect(loaded).toHaveBeenCalledWith(ref); }); it('is taken at once when the loaded queue already plays it', () => { @@ -724,11 +725,11 @@ describe("refreshing a downloaded book's chapters", () => { mockGetQueryData.mockImplementation((k: unknown) => Array.isArray(k) && k[0] === 'chapters' && k[3] === 'A/Book' ? reordered() : undefined, ); - adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: 'A/Book' }); + adoptCachedChapters(ref); expect(order()).toEqual(['A/Book/01.mp3', 'A/Book/02.mp3']); // The player let it go. loaded.mockReturnValue(null); - adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: 'A/Book' }); + adoptCachedChapters(ref); expect(order()).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(reordered()); }); @@ -740,7 +741,7 @@ describe("refreshing a downloaded book's chapters", () => { answer(fresh(['A/Book/02.mp3', 'A/Book/03.mp3'])); expect(useDownloads.getState().entries[key]).toBe(e); // Nothing cached, or a book not downloaded: nothing to take. - adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: 'A/Book' }); + adoptCachedChapters(ref); adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: 'B/Book' }); expect(useDownloads.getState().entries[key]).toBe(e); }); diff --git a/src/downloads/store.ts b/src/downloads/store.ts index 19f371eb..ece24567 100644 --- a/src/downloads/store.ts +++ b/src/downloads/store.ts @@ -398,17 +398,19 @@ export function refreshedChapters( const onDevice = new Map(have.map((f) => [f.relPath, f])); // The device's files in the fresh answer's order: each one exactly once. const files: DownloadedFile[] = []; + let reordered = false; for (const s of want) { const f = onDevice.get(s.path); if (!f) return null; onDevice.delete(s.path); const was = readSize.get(s.path) ?? 0; if (was > 0 && s.size > 0 && was !== s.size) return null; + if (f !== have[files.length]) reordered = true; files.push(f); } - const reordered = files.some((f, i) => f !== have[i]); if (!reordered && JSON.stringify(fresh) === JSON.stringify(manifest.chapters)) return null; - return reordered ? { ...manifest, chapters: fresh, files } : { ...manifest, chapters: fresh }; + // Unchanged order keeps the very same `files` array (`refreshedFor` tells them apart by it). + return { ...manifest, chapters: fresh, files: reordered ? files : have }; } /** The file order (book-relative paths) the player has a book loaded in, or null when it @@ -437,10 +439,10 @@ function refreshedFor( const next = refreshedChapters(manifest, fresh); if (!next || next.files === manifest.files) return next; const loaded = loadedFileOrder(ref); - const inLoadedOrder = - !loaded || - (loaded.length === next.files.length && loaded.every((p, i) => p === next.files[i].relPath)); - return inLoadedOrder ? next : null; + if (!loaded) return next; + const same = + loaded.length === next.files.length && loaded.every((p, i) => p === next.files[i].relPath); + return same ? next : null; } /** diff --git a/src/playback/store.test.ts b/src/playback/store.test.ts index dbe5139a..b090ec76 100644 --- a/src/playback/store.test.ts +++ b/src/playback/store.test.ts @@ -1351,6 +1351,7 @@ describe('the download hot-swap', () => { describe('a downloaded book re-ordered on its server', () => { const folder = 'A/Folder'; const paths = ['A/Folder/10.mp3', 'A/Folder/2.mp3']; + const serverOrder = ['A/Folder/2.mp3', 'A/Folder/10.mp3']; const chaptersIn = (order: string[]): ChaptersResponse => ({ library_id: 2, path: folder, @@ -1397,17 +1398,17 @@ describe('a downloaded book re-ordered on its server', () => { // The server's answer, in its new order, sits in the query cache. (queryClient.getQueryData as jest.Mock).mockImplementation((k: unknown) => Array.isArray(k) && k[0] === 'chapters' && k[3] === folder - ? chaptersIn([...paths].reverse()) + ? chaptersIn(serverOrder) : undefined, ); }); afterEach(() => (queryClient.getQueryData as jest.Mock).mockImplementation(() => undefined)); it("takes the server's order once a start plays it", async () => { - await usePlayer.getState().playBook('c1', 2, book, chaptersIn([...paths].reverse()), 0); + await usePlayer.getState().playBook('c1', 2, book, chaptersIn(serverOrder), 0); const tracks = usePlayer.getState().nowPlaying?.queue.tracks.map((t) => t.url); expect(tracks).toEqual(['file:///dl/1.mp3', 'file:///dl/0.mp3']); - expect(order()).toEqual(['A/Folder/2.mp3', 'A/Folder/10.mp3']); + expect(order()).toEqual(serverOrder); }); it('keeps the old order while a queue plays it, and takes the new one once it is gone', async () => { @@ -1415,7 +1416,7 @@ describe('a downloaded book re-ordered on its server', () => { await usePlayer.getState().playBook('c1', 2, book, chaptersIn(paths), 0); expect(order()).toEqual(paths); usePlayer.setState({ nowPlaying: null }); - expect(order()).toEqual(['A/Folder/2.mp3', 'A/Folder/10.mp3']); + expect(order()).toEqual(serverOrder); }); }); diff --git a/src/playback/store.ts b/src/playback/store.ts index b1bc889b..541775dd 100644 --- a/src/playback/store.ts +++ b/src/playback/store.ts @@ -6,6 +6,7 @@ import { qk } from '@/api/hooks'; import { queryClient } from '@/api/provider'; import { isReachable, noteError } from '@/api/reachability'; import type { Book, Chapter, ChaptersResponse, Progress } from '@/api/types'; +import { trackFilePath } from '@/cast/cast-media'; import { adoptCachedChapters, downloadedEntryOf, @@ -14,7 +15,7 @@ import { useDownloads, } from '@/downloads/store'; import { resolveConnectionId } from '@/lib/connection-id'; -import { contentKey } from '@/lib/content-key'; +import { contentKey, contentKeyOf } from '@/lib/content-key'; import { canAutoDownload } from '@/lib/network'; import { mayActFor } from '@/stores/profiles'; import { useSettings } from '@/stores/settings'; @@ -1388,11 +1389,9 @@ useDownloads.subscribe((state, prev) => { // does not hold it in another order (`refreshedFor` in the downloads store): this is the // order the loaded queue plays, by book-relative path (a track's id is `:`). setLoadedFileOrder((ref) => { - const np = usePlayer.getState().nowPlaying; - if (!np || np.connectionId !== ref.connectionId || np.libraryId !== ref.libraryId) return null; - if (np.path !== ref.path) return null; - const prefix = `${np.libraryId}:`; - return np.queue.tracks.map((t) => t.id.slice(prefix.length)); + const s = usePlayer.getState(); + if (!s.nowPlaying || selectBookKey(s) !== contentKeyOf(ref)) return null; + return s.nowPlaying.queue.tracks.map(trackFilePath); }); // And once the loaded book changes, a new order held back for the book that was loaded, @@ -1400,7 +1399,11 @@ setLoadedFileOrder((ref) => { usePlayer.subscribe((state, prev) => { if (state.nowPlaying === prev.nowPlaying) return; if (prev.nowPlaying) adoptCachedChapters(prev.nowPlaying); - if (state.nowPlaying) adoptCachedChapters(state.nowPlaying); + // The same book under a new object (the hot-swap, a restart): the call above covered it. + const now = state.nowPlaying; + if (now && (!prev.nowPlaying || contentKeyOf(now) !== contentKeyOf(prev.nowPlaying))) { + adoptCachedChapters(now); + } }); // --- selectors ------------------------------------------------------------- From e88df5ecf7293a3a5070579979fbfba8ddb87499 Mon Sep 17 00:00:00 2001 From: KodeStar Date: Fri, 23 Oct 2026 20:15:44 +0100 Subject: [PATCH 3/5] Code review fixes - A new file order moves only the chapters: manifest.files stays in the order the download wrote it, since a file's index there is its name on disk (relocateEntry at every launch, runOne's retry). Reordering it swapped audio between files after a relaunch, or dropped the book and deleted its folder when the extensions differed. refreshedFor now gets the new order from chaptersRefresh instead of the files array identity. - The loaded-order probe also covers a playBook in flight (its queue is built before the place lookup, nowPlaying only after), so a new order landing in that window waits; the start takes a held one as it ends. - The car sync rebuilds the snapshot when a downloaded book's chapters change, so Android's no-JS play spec follows a new order. - Cleanups: one take step for startChapterRefresh and adoptCachedChapters, withCachedChapters takes a BookRef, selectBookKey in the subscription, shared cacheHolds and the real chapters key in tests; docs and comments (types.ts, book-queue.ts, CONVENTIONS) say what the code does now. --- CONVENTIONS.md | 26 ++++-- src/car/car-controller.test.ts | 21 +++++ src/car/car-controller.ts | 21 ++++- src/downloads/store.test.ts | 70 ++++++++++----- src/downloads/store.ts | 153 +++++++++++++++++---------------- src/downloads/types.ts | 5 +- src/playback/book-queue.ts | 7 +- src/playback/store.test.ts | 39 +++++++-- src/playback/store.ts | 37 +++++--- 9 files changed, 252 insertions(+), 127 deletions(-) diff --git a/CONVENTIONS.md b/CONVENTIONS.md index 853404ec..19904136 100644 --- a/CONVENTIONS.md +++ b/CONVENTIONS.md @@ -1771,15 +1771,23 @@ are asked, at most 10,000 rows. `manifest.chapters` when its audio files are the ones on the device (`refreshedChapters`: same paths, same sizes where both answers know them); otherwise the manifest stays (that is a new download). The same files in another order (server v2.4.0 plays them in natural order, "2-" - before "10-") are the same audio: the manifest takes the server's order (`manifest.files` - reordered, nothing downloaded again) with its chapters, so the offline whole-book timeline and - every place saved on it is the server's. A new order is held while the player has the book - loaded in another one (`refreshedFor`, probing the playback store's `setLoadedFileOrder`: the - engine plays by track index, and Android's `adoptLoaded` gets only an index), and taken from - the cached answer once the loaded book changes (`adoptCachedChapters`). An answer that reached the cache before the book was downloaded (while its - files did, or before launch's `hydrate`) is taken as it becomes downloaded (`withCachedChapters`). - The audio is never touched, and a queue already playing keeps its chapters until the book is - next started. + before "10-") are the same audio: the manifest takes the server's chapters and with them its + play order (every queue over a download orders its files by the chapters and finds each by + path), nothing downloaded again, so the offline whole-book timeline is the server's from then + on. `manifest.files` itself never moves: a file's index there is its name on disk + (`fileName`, rebuilt by `relocateEntry` at every launch and by a retry). A new order is held + while the player has the book loaded, or a start is loading it, in another one (`refreshedFor` + asks the loaded order through the probe the playback store installs with the downloads store's + `setLoadedFileOrder`: the engine plays by track index, and Android's `adoptLoaded` gets only an + index), and taken from the cached answer once the loaded book changes or the start ends + (`adoptCachedChapters`). A session started in the old order keeps saving on that timeline until + the book is next started, so its place then reads as moved (as the server's re-index moves any + whole-book place: no remap). The probe knows only what JS loaded: a book the Android service or + a Cast receiver holds before `adoptLoaded` runs is not seen. An answer that reached the cache + before the book was downloaded (while its files did, or before launch's `hydrate`) is taken as + it becomes downloaded (`withCachedChapters`). The audio is never touched, and a queue already + playing keeps its chapters until the book is next started. A downloaded book whose chapters + change gets its Android car play spec rebuilt (`car-controller.ts`). - **Offline companion** (`src/downloads/offline-meta.ts`): a downloaded book keeps its community metadata (asked by `metaEnabledFor`, so a book with content generated on its server too; the `/meta` envelope, plus the previous work(s) the envelope doesn't hold) as `meta.json` beside its audio (`engine.writeText`: the book's folder on native, its Cache API diff --git a/src/car/car-controller.test.ts b/src/car/car-controller.test.ts index d7f71889..913e4205 100644 --- a/src/car/car-controller.test.ts +++ b/src/car/car-controller.test.ts @@ -515,6 +515,27 @@ describe('startCarSync', () => { expect(e.play).toMatchObject({ startIndex: 1, positionInTrack: 50, rate: 1.25 }); }); + it("rebuilds a downloaded book's spec when its chapters change (its files in a new order)", async () => { + Object.defineProperty(Platform, 'OS', { value: 'android', configurable: true }); + connect(['c1']); + mockClients.c1 = fakeClient({}); + useDownloads.setState({ entries: { 'c1:2:D': downloaded('c1', 'D') } }); + stop = startCarSync(); + await settle(); + const played = () => tab(lastSnapshot(), 'downloads')!.items[0].play!.tracks.map((t) => t.url); + expect(played()).toEqual(['file:///d/1.mp3', 'file:///d/2.mp3']); + + // The server now plays them the other way round (`refreshedChapters`): same status. + const reordered = downloaded('c1', 'D'); + reordered.manifest.chapters = { + ...twoFileChapters, + files: twoFileChapters.files.map((f) => ({ ...f, seq: 1 - f.seq })), + }; + useDownloads.setState({ entries: { 'c1:2:D': reordered } }); + await settle(SETTLE_MS + MIN_GAP_MS); + expect(played()).toEqual(['file:///d/2.mp3', 'file:///d/1.mp3']); + }); + it('leaves Up next out where the default server keeps no queue', async () => { connect(['c1']); mockClients.c1 = fakeClient({}); diff --git a/src/car/car-controller.ts b/src/car/car-controller.ts index 045dd501..ff88d11c 100644 --- a/src/car/car-controller.ts +++ b/src/car/car-controller.ts @@ -40,7 +40,7 @@ import { startBookInPlace } from '@/components/player/start-book'; import { queueConnectionId } from '@/components/upnext/up-next-model'; import { statusSignature } from '@/downloads/downloads-view'; import { downloadedEntryOf, useDownloads } from '@/downloads/store'; -import type { DownloadManifest } from '@/downloads/types'; +import type { DownloadEntry, DownloadManifest } from '@/downloads/types'; import i18n from '@/i18n'; import { bootstrapPlayback } from '@/lib/bootstrap'; import { resolveConnectionId } from '@/lib/connection-id'; @@ -979,6 +979,23 @@ export function carSyncReady(): Promise { return ready; } +/** Whether a book downloaded in both registries has other chapters in `next` (a chapters + * refresh, `refreshedChapters`, which can also give its files a new play order): its play + * spec is built from them (`playSpec`), and a refresh changes no download's status. */ +function chaptersChanged( + next: Record, + prev: Record, +): boolean { + return Object.entries(next).some(([k, e]) => { + const was = prev[k]; + return ( + e.status === 'downloaded' && + was?.status === 'downloaded' && + e.manifest.chapters !== was.manifest.chapters + ); + }); +} + /** * Start the car sync (idempotent: callers share one; each returned stop releases one, and the * last releases everything). Native only; a no-op on the web and on a binary without the car @@ -1016,7 +1033,7 @@ export function startCarSync(): () => void { useDownloads.subscribe((s, prev) => { if (s.entries === prev.entries) return; const sig = statusSignature(s.entries); - if (sig === downloadsSig) return; + if (sig === downloadsSig && !chaptersChanged(s.entries, prev.entries)) return; downloadsSig = sig; request(false); }), diff --git a/src/downloads/store.test.ts b/src/downloads/store.test.ts index fa63f406..98419b2e 100644 --- a/src/downloads/store.test.ts +++ b/src/downloads/store.test.ts @@ -132,6 +132,7 @@ jest.mock('@/api/hooks', () => ({ /* eslint-disable import/first */ import { resolveClient } from '@/api/connection-clients'; import { engine } from '@/downloads/engine'; +import { bookFileSpecs } from '@/playback/book-queue'; import { downloadedCountFor, downloadedEntryOf, @@ -546,6 +547,15 @@ describe("refreshing a downloaded book's chapters", () => { chapters_source: 'community', ...over, }); + /** The order a downloaded book's queue plays its files in: its chapters'. */ + const playOrder = (manifest: DownloadManifest) => + bookFileSpecs(manifest.book, manifest.chapters ?? undefined).map((s) => s.path); + /** The query cache holds `data` as A/Book's chapters answer. */ + const cacheHolds = (data: ChaptersResponse) => + mockGetQueryData.mockImplementation((k: unknown) => + Array.isArray(k) && k[0] === 'chapters' && k[3] === 'A/Book' ? data : undefined, + ); + afterEach(() => mockGetQueryData.mockReset()); describe('refreshedChapters', () => { it('takes the fresh chapters when the audio files are the same', () => { @@ -572,15 +582,11 @@ describe("refreshing a downloaded book's chapters", () => { e.manifest.chapters = fresh(undefined, { chapters_source: undefined }); const reordered = fresh(['A/Book/02.mp3', 'A/Book/01.mp3'], { chapters_source: undefined }); const next = refreshedChapters(e.manifest, reordered); - expect(next?.chapters).toBe(reordered); - // The very same local files, nothing downloaded again: only their order moves. - expect(next?.files).toEqual([e.manifest.files[1], e.manifest.files[0]]); - expect(next?.files[0]).toBe(e.manifest.files[1]); - expect({ ...next, chapters: null, files: null }).toEqual({ - ...e.manifest, - chapters: null, - files: null, - }); + // The chapters carry the server's order; the local files stay as the download wrote + // them (a file's index there is its name on disk), nothing downloaded again. + expect(next).toEqual({ ...e.manifest, chapters: reordered }); + expect(next?.files).toBe(e.manifest.files); + expect(playOrder(next!)).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); // A file replaced in place on the way is still a new download. reordered.files = reordered.files.map((f) => f.rel_path === 'A/Book/01.mp3' ? { ...f, size: 2 } : f, @@ -588,6 +594,31 @@ describe("refreshing a downloaded book's chapters", () => { expect(refreshedChapters(e.manifest, reordered)).toBeNull(); }); + it('keeps each file on its own audio across a launch once its book took a new order', async () => { + // Two extensions: a file named for another one's index would be missing on disk, and + // the launch would drop the book and delete its folder. + const paths = ['A/Book/01.mp3', 'A/Book/02.m4a']; + const e = downloadedEntry(); + e.manifest.chapters = fresh(paths, { chapters_source: undefined }); + e.manifest.files = [ + { relPath: paths[0], localUri: 'live:c1:2:A/Book:0.mp3' }, + { relPath: paths[1], localUri: 'live:c1:2:A/Book:1.m4a' }, + ]; + const onDisk = new Set(e.manifest.files.map((f) => f.localUri)); + mockEngine.fileExists.mockImplementation(async (uri: string) => onDisk.has(uri)); + const next = refreshedChapters(e.manifest, fresh([paths[1], paths[0]])); + const key = downloadKey('c1', 2, 'A/Book'); + await seed({ [key]: { ...e, manifest: next! } }); + + await useDownloads.getState().hydrate(); + + const after = useDownloads.getState().entries[key]; + expect(after?.status).toBe('downloaded'); + expect(mockEngine.removeBook).not.toHaveBeenCalled(); + expect(after.manifest.files).toEqual(e.manifest.files); + expect(playOrder(after.manifest)).toEqual([paths[1], paths[0]]); + }); + it('leaves the manifest alone when a file at the same path changed size (replaced in place)', () => { const e = downloadedEntry(); e.manifest.chapters = fresh(undefined, { chapters_source: undefined }); @@ -687,17 +718,17 @@ describe("refreshing a downloaded book's chapters", () => { const key = downloadKey('c1', 2, 'A/Book'); const ref = { connectionId: 'c1', libraryId: 2, path: 'A/Book' }; const reordered = () => fresh(['A/Book/02.mp3', 'A/Book/01.mp3']); - const order = () => useDownloads.getState().entries[key].manifest.files.map((f) => f.relPath); + const order = () => playOrder(useDownloads.getState().entries[key].manifest); const loaded = jest.fn((..._a: unknown[]): string[] | null => null); beforeEach(() => { loaded.mockReset().mockReturnValue(null); setLoadedFileOrder((ref) => loaded(ref)); - useDownloads.setState({ entries: { [key]: downloadedEntry() } }); - }); - afterEach(() => { - setLoadedFileOrder(() => null); - mockGetQueryData.mockReset(); + // Downloaded from the server's old answer (its own chapters, in the old order). + const e = downloadedEntry(); + e.manifest.chapters = fresh(undefined, { chapters_source: undefined }); + useDownloads.setState({ entries: { [key]: e } }); }); + afterEach(() => setLoadedFileOrder(() => null)); it('is taken at once when the player does not have the book loaded', () => { answer(reordered()); @@ -722,9 +753,7 @@ describe("refreshing a downloaded book's chapters", () => { expect(order()).toEqual(['A/Book/01.mp3', 'A/Book/02.mp3']); expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(fresh()); // Still loaded that way: the cached answer waits. - mockGetQueryData.mockImplementation((k: unknown) => - Array.isArray(k) && k[0] === 'chapters' && k[3] === 'A/Book' ? reordered() : undefined, - ); + cacheHolds(reordered()); adoptCachedChapters(ref); expect(order()).toEqual(['A/Book/01.mp3', 'A/Book/02.mp3']); // The player let it go. @@ -761,11 +790,6 @@ describe("refreshing a downloaded book's chapters", () => { // before (while the files did, or before launch read the registry) is taken from the cache. describe('an answer that landed before the book was downloaded', () => { const key = downloadKey('c1', 2, 'A/Book'); - const cacheHolds = (data: ChaptersResponse) => - mockGetQueryData.mockImplementation((k: unknown) => - Array.isArray(k) && k[0] === 'chapters' && k[3] === 'A/Book' ? data : undefined, - ); - afterEach(() => mockGetQueryData.mockReset()); it('is taken when the download finishes', async () => { mockResolveClient.mockReturnValue({ diff --git a/src/downloads/store.ts b/src/downloads/store.ts index ece24567..bf251c77 100644 --- a/src/downloads/store.ts +++ b/src/downloads/store.ts @@ -6,7 +6,7 @@ import { chaptersKeyParts, chaptersQuery, itemQuery, qk } from '@/api/hooks'; import { queryClient } from '@/api/provider'; import type { Book, ChaptersResponse } from '@/api/types'; import { identityMovePending } from '@/lib/connection-id'; -import { contentKey } from '@/lib/content-key'; +import { contentKey, contentKeyOf } from '@/lib/content-key'; import { mapLimit } from '@/lib/map-limit'; import { getItem, setItem } from '@/lib/storage'; import { DOWNLOADS_KEY } from '@/lib/storage-keys'; @@ -118,7 +118,7 @@ export const useDownloads = create()((set, get) => ({ const key = downloadKey(e.connectionId, e.libraryId, e.path); if (revived?.status === 'downloaded') { // A chapters answer can reach the cache before this read the registry. - const manifest = withCachedChapters(e.connectionId, e.libraryId, e.path, revived.manifest); + const manifest = withCachedChapters(e, revived.manifest); cleaned[key] = manifest === revived.manifest ? revived : { ...revived, manifest }; seedQueryCache(e.connectionId, e.libraryId, e.path, manifest); } else if (revived) { @@ -373,49 +373,55 @@ export function reviveEntry(e: DownloadEntry, allPresent: boolean): DownloadEntr * ones, or null to leave it as it is. A book's chapters can change on the server after * it was downloaded (a rescan, a community chapter list fitted onto its audio, the admin * switching it back), and offline it plays and lists what the manifest holds. Only the - * chapters move, and with them the files' play order: when the fresh answer's audio files - * are not the ones on the device (the same paths, `bookFileSpecs` as the download read - * them, and the same sizes where both answers know them: a file replaced in place keeps - * its path), the local audio no longer matches and that is a new download, so nothing - * changes. The same files in another order (a server that now sorts "2-" before "10-") - * are the same audio: the manifest takes the server's order with its chapters, so its - * whole-book timeline (and every place saved on it) is the server's. Null too when - * nothing differs. + * chapters move: when the fresh answer's audio files are not the ones on the device (the + * same set of paths, and the same sizes where both answers know them: a file replaced in + * place keeps its path), the local audio no longer matches and that is a new download, so + * nothing changes. The same files in another order (a server that now sorts "2-" before + * "10-") are the same audio: the chapters carry the order every queue over the download + * plays them in (`bookFileSpecs`, each file found by its path: `localFromManifest`), so + * its whole-book timeline is the server's. `files` itself never moves: a file's index + * there is its name on disk (`fileName`, which `relocateEntry` and a retry rebuild it + * from). Null too when nothing differs. */ export function refreshedChapters( manifest: DownloadManifest, fresh: ChaptersResponse, ): DownloadManifest | null { + return chaptersRefresh(manifest, fresh)?.manifest ?? null; +} + +/** `refreshedChapters`, with the order the fresh answer plays the files in when it is not + * the order the manifest plays them in now (`newOrder`, book-relative paths; else null). */ +function chaptersRefresh( + manifest: DownloadManifest, + fresh: ChaptersResponse, +): { manifest: DownloadManifest; newOrder: string[] | null } | null { // An unchanged refetch hands back the same object (structural sharing): skip the compare. if (fresh === manifest.chapters) return null; const want = bookFileSpecs(manifest.book, fresh); - const have = manifest.files; - if (want.length !== have.length) return null; - // The sizes as the download read them (0 when only the chapters named the files). - const readSize = new Map( - bookFileSpecs(manifest.book, manifest.chapters ?? undefined).map((s) => [s.path, s.size]), - ); - const onDevice = new Map(have.map((f) => [f.relPath, f])); - // The device's files in the fresh answer's order: each one exactly once. - const files: DownloadedFile[] = []; - let reordered = false; + if (want.length !== manifest.files.length) return null; + // The files in the order the manifest plays them now, with the sizes the download read + // (0 when only the chapters named the files). + const read = bookFileSpecs(manifest.book, manifest.chapters ?? undefined); + const readSize = new Map(read.map((s) => [s.path, s.size])); + // The device's files, each one named exactly once, in any order. + const onDevice = new Set(manifest.files.map((f) => f.relPath)); for (const s of want) { - const f = onDevice.get(s.path); - if (!f) return null; - onDevice.delete(s.path); + if (!onDevice.delete(s.path)) return null; const was = readSize.get(s.path) ?? 0; if (was > 0 && s.size > 0 && was !== s.size) return null; - if (f !== have[files.length]) reordered = true; - files.push(f); } + const reordered = want.some((s, i) => s.path !== read[i]?.path); if (!reordered && JSON.stringify(fresh) === JSON.stringify(manifest.chapters)) return null; - // Unchanged order keeps the very same `files` array (`refreshedFor` tells them apart by it). - return { ...manifest, chapters: fresh, files: reordered ? files : have }; + return { + manifest: { ...manifest, chapters: fresh }, + newOrder: reordered ? want.map((s) => s.path) : null, + }; } -/** The file order (book-relative paths) the player has a book loaded in, or null when it - * is not the book loaded. The playback store sets it (`setLoadedFileOrder`): it imports - * this module, so this one cannot ask it directly. */ +/** The file order (book-relative paths) the player has a book loaded in, or is loading it + * in, else null. The playback store sets it (`setLoadedFileOrder`): it imports this + * module, so this one cannot ask it directly. */ let loadedFileOrder: (ref: BookRef) => string[] | null = () => null; /** Set by the playback store at import (see `loadedFileOrder`). */ @@ -424,66 +430,74 @@ export function setLoadedFileOrder(probe: typeof loadedFileOrder): void { } /** - * `refreshedChapters`, held back while the player has the book loaded in another order - * than the fresh one: the engine plays that queue's files by index (Android's service - * hands a JS restart only a track index, `adoptLoaded`), so the manifest must not move - * its files under it. A queue already in the fresh order (a start from the server's - * answer) takes it at once; a held one is taken once the book is no longer loaded that - * way (`adoptCachedChapters`, from the playback store). + * `refreshedChapters`, held back when it plays the files in a new order while the player + * has the book loaded (or on its way in) in another one: the engine plays that queue's + * files by index (Android's service hands a JS restart only a track index, `adoptLoaded`), + * so the order the manifest gives them must not move under it. New chapters in the + * manifest's own order are taken whatever plays. A queue already in the fresh order (a + * start from the server's answer) takes it at once; a held one is taken once the book is + * no longer loaded that way (`adoptCachedChapters`, from the playback store). */ function refreshedFor( ref: BookRef, manifest: DownloadManifest, fresh: ChaptersResponse, ): DownloadManifest | null { - const next = refreshedChapters(manifest, fresh); - if (!next || next.files === manifest.files) return next; + const refresh = chaptersRefresh(manifest, fresh); + if (!refresh?.newOrder) return refresh?.manifest ?? null; + const order = refresh.newOrder; const loaded = loadedFileOrder(ref); - if (!loaded) return next; - const same = - loaded.length === next.files.length && loaded.every((p, i) => p === next.files[i].relPath); - return same ? next : null; + if (!loaded) return refresh.manifest; + const same = loaded.length === order.length && loaded.every((p, i) => p === order[i]); + return same ? refresh.manifest : null; +} + +/** The chapters answer the query cache holds for a book, if any. */ +function cachedChapters(ref: BookRef): ChaptersResponse | undefined { + return queryClient.getQueryData( + qk.chapters(ref.connectionId, ref.libraryId, ref.path), + ); } /** * `manifest` with the chapters answer the cache already holds for its book, when - * `refreshedChapters` takes it. `startChapterRefresh` only sees answers that land while the + * `refreshedFor` takes it. `startChapterRefresh` only sees answers that land while the * book is downloaded; this takes one that landed before, while it was downloading or before * the launch's `hydrate` read the registry. That answer is the one the download was given * or a newer one (the callers read it from the same cache entry). */ -function withCachedChapters( - connectionId: string, - libraryId: number, - path: string, - manifest: DownloadManifest, -): DownloadManifest { - const cached = queryClient.getQueryData( - qk.chapters(connectionId, libraryId, path), - ); - return (cached && refreshedFor({ connectionId, libraryId, path }, manifest, cached)) || manifest; +function withCachedChapters(ref: BookRef, manifest: DownloadManifest): DownloadManifest { + const cached = cachedChapters(ref); + return (cached && refreshedFor(ref, manifest, cached)) || manifest; +} + +/** A downloaded book takes `fresh` when `refreshedFor` allows. A burst of answers (a + * reconnect's refetches, a search fan-out) saves the registry once, not once per book. */ +function takeChapters(ref: BookRef, fresh: ChaptersResponse): void { + const entry = downloadedEntryOf(ref); + const manifest = entry && refreshedFor(ref, entry.manifest, fresh); + if (!manifest) return; + patchEntry(contentKeyOf(ref), { manifest }); + persistSoon(); } /** * A downloaded book takes the chapters answer the cache holds for it now, when - * `refreshedChapters` allows: for one `refreshedFor` held back while it was loaded in its - * old order (the playback store calls this when the loaded book changes). + * `refreshedFor` allows: one held back while the book was loaded in another order (the + * playback store calls this when the loaded book changes, and as a start ends). */ export function adoptCachedChapters(ref: BookRef): void { - const entry = downloadedEntryOf(ref); - if (!entry) return; - const manifest = withCachedChapters(ref.connectionId, ref.libraryId, ref.path, entry.manifest); - if (manifest === entry.manifest) return; - patchEntry(downloadKey(ref.connectionId, ref.libraryId, ref.path), { manifest }); - persistSoon(); + const cached = cachedChapters(ref); + if (cached) takeChapters(ref, cached); } /** * Keep downloaded books' chapters in step with the server: every chapters answer the * server gives (any screen's fetch, the play and download paths) for a downloaded book - * replaces its saved copy when `refreshedChapters` allows. The cache already holds that + * replaces its saved copy when `refreshedFor` allows. The cache already holds that * answer, so there is nothing to seed. A queue already playing keeps its chapters until - * the book is next started, and a new file order waits until then (`refreshedFor`). + * the book is next started, and a new file order waits while the player has the book + * loaded in another one (`adoptCachedChapters` takes it once that ends). * Returns the unsubscribe (started once from the root layout). */ export function startChapterRefresh(): () => void { @@ -493,16 +507,7 @@ export function startChapterRefresh(): () => void { const book = chaptersKeyParts(event.query.queryKey); const fresh = event.action.data as ChaptersResponse | undefined; if (!book || !fresh) return; - const key = downloadKey(book.cid, book.libraryId, book.path); - const entry = useDownloads.getState().entries[key]; - if (entry?.status !== 'downloaded') return; - const ref = { connectionId: book.cid, libraryId: book.libraryId, path: book.path }; - const manifest = refreshedFor(ref, entry.manifest, fresh); - if (!manifest) return; - patchEntry(key, { manifest }); - // A burst of answers (a reconnect's refetches, a search fan-out) saves the registry - // once, not once per book. - persistSoon(); + takeChapters({ connectionId: book.cid, libraryId: book.libraryId, path: book.path }, fresh); }); } @@ -847,7 +852,7 @@ async function runOne(key: string) { } // With the server's chapters answer when one landed while the files did. - const manifest = withCachedChapters(connectionId, libraryId, path, { + const manifest = withCachedChapters(entry, { ...entry.manifest, files, coverUri, diff --git a/src/downloads/types.ts b/src/downloads/types.ts index 7d712a44..94b000b6 100644 --- a/src/downloads/types.ts +++ b/src/downloads/types.ts @@ -43,7 +43,10 @@ export type DownloadOutcome = /** * Offline source of truth for a downloaded book - everything the playback layer * needs to build the queue and render the player with no network: the book + - * chapters metadata, the local audio files (in play order), and a local cover. + * chapters metadata, the local audio files, and a local cover. `files` is in the order + * the download wrote them, each saved under its index there (`fileName` in store.ts), so + * it never moves; the chapters give the play order (`bookFileSpecs`), which a later + * server answer can change (`refreshedChapters`). */ export type DownloadManifest = { book: Book; diff --git a/src/playback/book-queue.ts b/src/playback/book-queue.ts index e9a8ca3b..48367920 100644 --- a/src/playback/book-queue.ts +++ b/src/playback/book-queue.ts @@ -33,9 +33,10 @@ export type FileSpec = { path: string; duration: number; size: number }; /** * The distinct audio files that make up a book, in play order - the single * source of truth for both playback (`buildBookQueue`) and offline downloads, so - * download order ≡ play order. Prefers an explicit file list; falls back to the - * files referenced by the chapters; finally the book path itself. This must never - * resolve to a folder path. + * a download writes the files in play order (a later answer for the same files can + * play them in another; the chapters say which: `refreshedChapters` in the downloads + * store). Prefers an explicit file list; falls back to the files referenced by the + * chapters; finally the book path itself. This must never resolve to a folder path. */ export function bookFileSpecs(book: Book, chapterData?: ChaptersResponse): FileSpec[] { const files = chapterData?.files?.length ? chapterData.files : (book.files ?? []); diff --git a/src/playback/store.test.ts b/src/playback/store.test.ts index b090ec76..7e55a1bc 100644 --- a/src/playback/store.test.ts +++ b/src/playback/store.test.ts @@ -129,11 +129,12 @@ import { Platform } from 'react-native'; import { qk } from '@/api/hooks'; import { queryClient } from '@/api/provider'; import { engine as downloadEngine } from '@/downloads/engine'; -import { isDeclined, useDownloads } from '@/downloads/store'; +import { adoptCachedChapters, isDeclined, useDownloads } from '@/downloads/store'; import type { DownloadEntry, DownloadManifest } from '@/downloads/types'; import { resetProfilesForTests, switchProfile, useProfiles } from '@/stores/profiles'; import { useSettings } from '@/stores/settings'; +import { bookFileSpecs } from './book-queue'; import { flushConnection } from './progress-sync'; import { AutoplayBlockedError } from './types'; import { @@ -1370,7 +1371,11 @@ describe('a downloaded book re-ordered on its server', () => { }); const book = makeBook({ rel_path: folder, is_folder: true, format: 'mp3' }); const key = `c1:2:${folder}`; - const order = () => useDownloads.getState().entries[key].manifest.files.map((f) => f.relPath); + /** The order the download plays its files in (its chapters'; `files` never moves). */ + const order = () => { + const { manifest } = useDownloads.getState().entries[key]; + return bookFileSpecs(manifest.book, manifest.chapters ?? undefined).map((s) => s.path); + }; beforeEach(() => { const manifest: DownloadManifest = { @@ -1396,10 +1401,9 @@ describe('a downloaded book re-ordered on its server', () => { }, }); // The server's answer, in its new order, sits in the query cache. + const chaptersKey = JSON.stringify(qk.chapters('c1', 2, folder)); (queryClient.getQueryData as jest.Mock).mockImplementation((k: unknown) => - Array.isArray(k) && k[0] === 'chapters' && k[3] === folder - ? chaptersIn(serverOrder) - : undefined, + JSON.stringify(k) === chaptersKey ? chaptersIn(serverOrder) : undefined, ); }); afterEach(() => (queryClient.getQueryData as jest.Mock).mockImplementation(() => undefined)); @@ -1409,6 +1413,10 @@ describe('a downloaded book re-ordered on its server', () => { const tracks = usePlayer.getState().nowPlaying?.queue.tracks.map((t) => t.url); expect(tracks).toEqual(['file:///dl/1.mp3', 'file:///dl/0.mp3']); expect(order()).toEqual(serverOrder); + // The files stay where the download wrote them. + expect(useDownloads.getState().entries[key].manifest.files.map((f) => f.relPath)).toEqual( + paths, + ); }); it('keeps the old order while a queue plays it, and takes the new one once it is gone', async () => { @@ -1418,6 +1426,27 @@ describe('a downloaded book re-ordered on its server', () => { usePlayer.setState({ nowPlaying: null }); expect(order()).toEqual(serverOrder); }); + + it('keeps the old order for a start in it whose place is still being looked up', async () => { + let lookedUp: (r: ResumeLookup) => void = () => {}; + mockLoadInitialProgress.mockImplementationOnce( + () => new Promise((resolve) => (lookedUp = resolve)), + ); + // From the download's own copy (the fresh read timed out), then the place lookup. + const start = usePlayer.getState().playBook('c1', 2, book, chaptersIn(paths)); + await flushMicrotasks(); + expect(usePlayer.getState().nowPlaying).toBeNull(); + // The server's answer lands meanwhile: the start is about to load the old order. + adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: folder }); + expect(order()).toEqual(paths); + lookedUp({ kind: 'empty' }); + await start; + const tracks = usePlayer.getState().nowPlaying?.queue.tracks.map((t) => t.url); + expect(tracks).toEqual(['file:///dl/0.mp3', 'file:///dl/1.mp3']); + expect(order()).toEqual(paths); + usePlayer.setState({ nowPlaying: null }); + expect(order()).toEqual(serverOrder); + }); }); // --- offline playback of a downloaded book whose connection is gone ---------------- diff --git a/src/playback/store.ts b/src/playback/store.ts index 541775dd..74205c85 100644 --- a/src/playback/store.ts +++ b/src/playback/store.ts @@ -148,6 +148,11 @@ let localMoves = 0; * the connection, the retry request and the save loop out from under it. */ let startsInFlight = 0; +/** The file order (book-relative paths) each `playBook` in flight loads its book in, by + * book (`contentKey`), from its queue's build until it ends: what the engine is about to + * hold before `nowPlaying` says so (`setLoadedFileOrder`, below). */ +const startingOrders = new Map(); + /** Captured so `retry()` can re-run the resume path after a lookup failure. */ let lastPlayRequest: { connectionId: string; @@ -786,6 +791,10 @@ export const usePlayer = create()((set, get) => ({ // stops only a book that is already `nowPlaying`): a start still looking up its place // must not load, play or save someone else's book once they have gone. const current = () => mayActFor(connectionId); + const ref = { connectionId, libraryId, path: book.rel_path }; + const key = contentKeyOf(ref); + /** The order this start loads the book in, once its queue is built (`startingOrders`). */ + let starting: string[] | null = null; // In flight until it ends: an adoption reading meanwhile stands back (`startsInFlight`). startsInFlight++; try { @@ -794,7 +803,6 @@ export const usePlayer = create()((set, get) => ({ const api = resolveClient(connectionId); // Play from local files when the book is downloaded (works fully offline). - const ref = { connectionId, libraryId, path: book.rel_path }; const dl = downloadedEntryOf(ref); const local = dl ? localFromManifest(dl.manifest) : undefined; @@ -831,6 +839,9 @@ export const usePlayer = create()((set, get) => ({ transcode, ); const nowPlaying = nowPlayingOf(ref, book, queue, api, local); + // A download's new file order waits for this start while its place is looked up. + starting = queue.tracks.map(trackFilePath); + startingOrders.set(key, starting); const svc = await ensureService(); let startAt = startBookPosition ?? 0; @@ -890,7 +901,6 @@ export const usePlayer = create()((set, get) => ({ // the save guard won't let progress regress below it without a deliberate seek. resumeFloor = toBookPosition(queue.offsets, index, positionInTrack); // A different book's place is not known until its load lands (`loadingBook`). - const key = contentKey(connectionId, libraryId, book.rel_path); const prev = get(); const loadingBook = selectBookKey(prev) === key ? prev.loadingBook : key; set({ rate: speed, nowPlaying, loadingBook }); @@ -928,6 +938,12 @@ export const usePlayer = create()((set, get) => ({ void maybeAutoDownloadCurrent(connectionId, libraryId, book, chapterData); } finally { startsInFlight--; + if (starting) { + if (startingOrders.get(key) === starting) startingOrders.delete(key); + // A new order held back for this start is taken now, unless the book plays on in + // another one. + adoptCachedChapters(ref); + } } }, @@ -1385,13 +1401,16 @@ useDownloads.subscribe((state, prev) => { } }); -// A downloaded book's files take a new play order from its server only while the player -// does not hold it in another order (`refreshedFor` in the downloads store): this is the -// order the loaded queue plays, by book-relative path (a track's id is `:`). +// A downloaded book takes a new file order from its server only while the player does not +// hold it in another order (`refreshedFor` in the downloads store): this is the order the +// loaded queue plays (the engine holds it until a start's load replaces it), else the order +// a start in flight is about to load, by book-relative path (a track's id is +// `:`). setLoadedFileOrder((ref) => { const s = usePlayer.getState(); - if (!s.nowPlaying || selectBookKey(s) !== contentKeyOf(ref)) return null; - return s.nowPlaying.queue.tracks.map(trackFilePath); + const key = contentKeyOf(ref); + if (s.nowPlaying && selectBookKey(s) === key) return s.nowPlaying.queue.tracks.map(trackFilePath); + return startingOrders.get(key) ?? null; }); // And once the loaded book changes, a new order held back for the book that was loaded, @@ -1401,9 +1420,7 @@ usePlayer.subscribe((state, prev) => { if (prev.nowPlaying) adoptCachedChapters(prev.nowPlaying); // The same book under a new object (the hot-swap, a restart): the call above covered it. const now = state.nowPlaying; - if (now && (!prev.nowPlaying || contentKeyOf(now) !== contentKeyOf(prev.nowPlaying))) { - adoptCachedChapters(now); - } + if (now && selectBookKey(prev) !== selectBookKey(state)) adoptCachedChapters(now); }); // --- selectors ------------------------------------------------------------- From 994d9bf48b6b93f6894b8abe430d2598744ad8f2 Mon Sep 17 00:00:00 2001 From: KodeStar Date: Fri, 23 Oct 2026 20:24:55 +0100 Subject: [PATCH 4/5] Downloads: a new file order is taken only by a start that loads it The hold could see only what JS loaded, so after an Android JS restart (or a relaunch mid-Cast) a chapters answer landing before adoptLoaded could move the order the engine's track index is mapped through. A background answer now never changes a download's file order; playBook hands the chapters it loaded the engine with to adoptStartedChapters once the load landed and the start still owns the engine, saved at once. Same order chapter refreshes are unchanged. Removes the loaded-order probe, startingOrders and the take-on-unload path. --- CONVENTIONS.md | 36 ++++----- src/car/car-controller.ts | 3 +- src/downloads/store.test.ts | 89 ++++++++------------ src/downloads/store.ts | 134 ++++++++++++------------------- src/downloads/types.ts | 2 +- src/playback/book-queue.ts | 5 +- src/playback/store-adopt.test.ts | 44 +++++++++- src/playback/store.test.ts | 36 ++++----- src/playback/store.ts | 51 ++---------- 9 files changed, 175 insertions(+), 225 deletions(-) diff --git a/CONVENTIONS.md b/CONVENTIONS.md index 19904136..b3356da9 100644 --- a/CONVENTIONS.md +++ b/CONVENTIONS.md @@ -1769,25 +1769,23 @@ are asked, at most 10,000 rows. rescan, a community chapter list, `chapters_source: "community"`). `startChapterRefresh` (root layout) watches the query cache: a server answer for a downloaded book's chapters replaces `manifest.chapters` when its audio files are the ones on the device (`refreshedChapters`: same - paths, same sizes where both answers know them); otherwise the manifest stays (that is a new - download). The same files in another order (server v2.4.0 plays them in natural order, "2-" - before "10-") are the same audio: the manifest takes the server's chapters and with them its - play order (every queue over a download orders its files by the chapters and finds each by - path), nothing downloaded again, so the offline whole-book timeline is the server's from then - on. `manifest.files` itself never moves: a file's index there is its name on disk - (`fileName`, rebuilt by `relocateEntry` at every launch and by a retry). A new order is held - while the player has the book loaded, or a start is loading it, in another one (`refreshedFor` - asks the loaded order through the probe the playback store installs with the downloads store's - `setLoadedFileOrder`: the engine plays by track index, and Android's `adoptLoaded` gets only an - index), and taken from the cached answer once the loaded book changes or the start ends - (`adoptCachedChapters`). A session started in the old order keeps saving on that timeline until - the book is next started, so its place then reads as moved (as the server's re-index moves any - whole-book place: no remap). The probe knows only what JS loaded: a book the Android service or - a Cast receiver holds before `adoptLoaded` runs is not seen. An answer that reached the cache - before the book was downloaded (while its files did, or before launch's `hydrate`) is taken as - it becomes downloaded (`withCachedChapters`). The audio is never touched, and a queue already - playing keeps its chapters until the book is next started. A downloaded book whose chapters - change gets its Android car play spec rebuilt (`car-controller.ts`). + paths, same order, same sizes where both answers know them); otherwise the manifest stays (that + is a new download). An answer that reached the cache before the book was downloaded (while its + files did, or before launch's `hydrate`) is taken as it becomes downloaded (`withCachedChapters`). + The audio is never touched, and a queue already playing keeps its chapters until the book is + next started. The same files in another order (server v2.4.0 plays them in natural order, "2-" + before "10-") are taken **only by a start that loads the engine in that order** + (`adoptStartedChapters`, from `playBook` once its load landed and it still owns the engine; + saved at once), never from a background answer: JS sees only what it loaded, not a book the + Android service or a Cast receiver holds before `adoptLoaded` / the receiver adoption, which map + the engine's track index through the manifest. So an offline start, Android's no-JS car start + and `adoptLoaded` keep the manifest's order until an online start plays the new one. The + chapters carry the order (every queue over a download orders its files by them and finds each by + path); `manifest.files` itself never moves, since a file's index there is its name on disk + (`fileName`, rebuilt by `relocateEntry` at every launch and by a retry). A place saved on the old + timeline reads as moved once the new order is taken (as the server's re-index moves any + whole-book place: no remap). A downloaded book whose chapters change gets its Android car play + spec rebuilt (`car-controller.ts`). - **Offline companion** (`src/downloads/offline-meta.ts`): a downloaded book keeps its community metadata (asked by `metaEnabledFor`, so a book with content generated on its server too; the `/meta` envelope, plus the previous work(s) the envelope doesn't hold) as `meta.json` beside its audio (`engine.writeText`: the book's folder on native, its Cache API diff --git a/src/car/car-controller.ts b/src/car/car-controller.ts index ff88d11c..46efd3ea 100644 --- a/src/car/car-controller.ts +++ b/src/car/car-controller.ts @@ -980,7 +980,8 @@ export function carSyncReady(): Promise { } /** Whether a book downloaded in both registries has other chapters in `next` (a chapters - * refresh, `refreshedChapters`, which can also give its files a new play order): its play + * refresh, `refreshedChapters`, or a start's, `adoptStartedChapters`, which can also give + * its files a new play order): its play * spec is built from them (`playSpec`), and a refresh changes no download's status. */ function chaptersChanged( next: Record, diff --git a/src/downloads/store.test.ts b/src/downloads/store.test.ts index 98419b2e..8d8ebefc 100644 --- a/src/downloads/store.test.ts +++ b/src/downloads/store.test.ts @@ -137,11 +137,10 @@ import { downloadedCountFor, downloadedEntryOf, downloadKey, - adoptCachedChapters, + adoptStartedChapters, isDeclined, refreshedChapters, reviveEntry, - setLoadedFileOrder, startChapterRefresh, useDownloads, } from '@/downloads/store'; @@ -577,21 +576,10 @@ describe("refreshing a downloaded book's chapters", () => { ).toBeNull(); }); - it('takes the server order with its chapters when the same files come in another order', () => { + it('never takes the same files in another order (only a start that plays it does)', () => { const e = downloadedEntry(); e.manifest.chapters = fresh(undefined, { chapters_source: undefined }); - const reordered = fresh(['A/Book/02.mp3', 'A/Book/01.mp3'], { chapters_source: undefined }); - const next = refreshedChapters(e.manifest, reordered); - // The chapters carry the server's order; the local files stay as the download wrote - // them (a file's index there is its name on disk), nothing downloaded again. - expect(next).toEqual({ ...e.manifest, chapters: reordered }); - expect(next?.files).toBe(e.manifest.files); - expect(playOrder(next!)).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); - // A file replaced in place on the way is still a new download. - reordered.files = reordered.files.map((f) => - f.rel_path === 'A/Book/01.mp3' ? { ...f, size: 2 } : f, - ); - expect(refreshedChapters(e.manifest, reordered)).toBeNull(); + expect(refreshedChapters(e.manifest, fresh(['A/Book/02.mp3', 'A/Book/01.mp3']))).toBeNull(); }); it('keeps each file on its own audio across a launch once its book took a new order', async () => { @@ -606,9 +594,13 @@ describe("refreshing a downloaded book's chapters", () => { ]; const onDisk = new Set(e.manifest.files.map((f) => f.localUri)); mockEngine.fileExists.mockImplementation(async (uri: string) => onDisk.has(uri)); - const next = refreshedChapters(e.manifest, fresh([paths[1], paths[0]])); const key = downloadKey('c1', 2, 'A/Book'); - await seed({ [key]: { ...e, manifest: next! } }); + useDownloads.setState({ entries: { [key]: e } }); + adoptStartedChapters( + { connectionId: 'c1', libraryId: 2, path: 'A/Book' }, + fresh([paths[1], paths[0]]), + ); + await seed({ [key]: useDownloads.getState().entries[key] }); await useDownloads.getState().hydrate(); @@ -719,59 +711,46 @@ describe("refreshing a downloaded book's chapters", () => { const ref = { connectionId: 'c1', libraryId: 2, path: 'A/Book' }; const reordered = () => fresh(['A/Book/02.mp3', 'A/Book/01.mp3']); const order = () => playOrder(useDownloads.getState().entries[key].manifest); - const loaded = jest.fn((..._a: unknown[]): string[] | null => null); beforeEach(() => { - loaded.mockReset().mockReturnValue(null); - setLoadedFileOrder((ref) => loaded(ref)); // Downloaded from the server's old answer (its own chapters, in the old order). const e = downloadedEntry(); e.manifest.chapters = fresh(undefined, { chapters_source: undefined }); useDownloads.setState({ entries: { [key]: e } }); }); - afterEach(() => setLoadedFileOrder(() => null)); - - it('is taken at once when the player does not have the book loaded', () => { - answer(reordered()); - expect(order()).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); - expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(reordered()); - expect(loaded).toHaveBeenCalledWith(ref); - }); - it('is taken at once when the loaded queue already plays it', () => { - loaded.mockReturnValue(['A/Book/02.mp3', 'A/Book/01.mp3']); - answer(reordered()); - expect(order()).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); - }); - - it('waits while the book is loaded in its old order, then is taken from the cache', () => { - loaded.mockReturnValue(['A/Book/01.mp3', 'A/Book/02.mp3']); + it('never comes from a background answer: JS cannot see what a native engine holds', async () => { const e = useDownloads.getState().entries[key]; answer(reordered()); expect(useDownloads.getState().entries[key]).toBe(e); - // Chapters alone (the same order) still refresh while it plays, as before. - answer(fresh()); - expect(order()).toEqual(['A/Book/01.mp3', 'A/Book/02.mp3']); - expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(fresh()); - // Still loaded that way: the cached answer waits. + // Nor from one the cache held when launch read the registry. + await seed({ [key]: e }); cacheHolds(reordered()); - adoptCachedChapters(ref); - expect(order()).toEqual(['A/Book/01.mp3', 'A/Book/02.mp3']); - // The player let it go. - loaded.mockReturnValue(null); - adoptCachedChapters(ref); + await useDownloads.getState().hydrate(); + expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(e.manifest.chapters); + }); + + it('is taken by a start that played it, with its chapters, nothing downloaded again', async () => { + const e = useDownloads.getState().entries[key]; + adoptStartedChapters(ref, reordered()); + const now = useDownloads.getState().entries[key]; + expect(now.manifest.chapters).toEqual(reordered()); expect(order()).toEqual(['A/Book/02.mp3', 'A/Book/01.mp3']); - expect(useDownloads.getState().entries[key].manifest.chapters).toEqual(reordered()); + // The local files stay as the download wrote them (a file's index is its name on disk). + expect(now.manifest.files).toBe(e.manifest.files); + // Saved at once: a JS restart right after the start must find the order it played. + expect((await readPersisted())[key]?.manifest.chapters).toEqual(reordered()); }); - it('leaves a different set of files alone, loaded or not', () => { + it('is not taken from a start whose chapters name other files, or another size', () => { const e = useDownloads.getState().entries[key]; - answer(fresh(['A/Book/02.mp3', 'A/Book/03.mp3'])); - loaded.mockReturnValue(['A/Book/02.mp3', 'A/Book/03.mp3']); - answer(fresh(['A/Book/02.mp3', 'A/Book/03.mp3'])); - expect(useDownloads.getState().entries[key]).toBe(e); - // Nothing cached, or a book not downloaded: nothing to take. - adoptCachedChapters(ref); - adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: 'B/Book' }); + adoptStartedChapters(ref, fresh(['A/Book/02.mp3', 'A/Book/03.mp3'])); + adoptStartedChapters(ref, fresh(['A/Book/02.mp3'])); + const resized = reordered(); + resized.files = resized.files.map((f) => + f.rel_path === 'A/Book/01.mp3' ? { ...f, size: 2 } : f, + ); + adoptStartedChapters(ref, resized); + adoptStartedChapters({ ...ref, path: 'B/Book' }, reordered()); expect(useDownloads.getState().entries[key]).toBe(e); }); }); diff --git a/src/downloads/store.ts b/src/downloads/store.ts index bf251c77..2c1d96f0 100644 --- a/src/downloads/store.ts +++ b/src/downloads/store.ts @@ -374,28 +374,27 @@ export function reviveEntry(e: DownloadEntry, allPresent: boolean): DownloadEntr * it was downloaded (a rescan, a community chapter list fitted onto its audio, the admin * switching it back), and offline it plays and lists what the manifest holds. Only the * chapters move: when the fresh answer's audio files are not the ones on the device (the - * same set of paths, and the same sizes where both answers know them: a file replaced in - * place keeps its path), the local audio no longer matches and that is a new download, so - * nothing changes. The same files in another order (a server that now sorts "2-" before - * "10-") are the same audio: the chapters carry the order every queue over the download - * plays them in (`bookFileSpecs`, each file found by its path: `localFromManifest`), so - * its whole-book timeline is the server's. `files` itself never moves: a file's index - * there is its name on disk (`fileName`, which `relocateEntry` and a retry rebuild it - * from). Null too when nothing differs. + * same paths in the same order, `bookFileSpecs` as the download read them, and the same + * sizes where both answers know them: a file replaced in place keeps its path), the local + * audio no longer matches and that is a new download, so nothing changes. The same files + * in another order are taken only by a start that plays them that way + * (`adoptStartedChapters`), never from a background answer. Null too when nothing differs. */ export function refreshedChapters( manifest: DownloadManifest, fresh: ChaptersResponse, ): DownloadManifest | null { - return chaptersRefresh(manifest, fresh)?.manifest ?? null; + const refresh = chaptersRefresh(manifest, fresh); + return refresh && !refresh.reordered ? refresh.manifest : null; } -/** `refreshedChapters`, with the order the fresh answer plays the files in when it is not - * the order the manifest plays them in now (`newOrder`, book-relative paths; else null). */ +/** The manifest with `fresh` chapters when they name the device's own files (the same set + * of paths, the same sizes where both answers know them), and whether they play those + * files in another order than the manifest's chapters do (`reordered`); else null. */ function chaptersRefresh( manifest: DownloadManifest, fresh: ChaptersResponse, -): { manifest: DownloadManifest; newOrder: string[] | null } | null { +): { manifest: DownloadManifest; reordered: boolean } | null { // An unchanged refetch hands back the same object (structural sharing): skip the compare. if (fresh === manifest.chapters) return null; const want = bookFileSpecs(manifest.book, fresh); @@ -411,94 +410,59 @@ function chaptersRefresh( const was = readSize.get(s.path) ?? 0; if (was > 0 && s.size > 0 && was !== s.size) return null; } - const reordered = want.some((s, i) => s.path !== read[i]?.path); + // The order the manifest plays them in now: its chapters', else (saved chapters that name + // no file list of their own) the order the download wrote them. + const onDisk = manifest.files.map((f) => f.relPath); + const playing = + read.length === onDisk.length && read.every((s) => onDisk.includes(s.path)) + ? read.map((s) => s.path) + : onDisk; + const reordered = want.some((s, i) => s.path !== playing[i]); if (!reordered && JSON.stringify(fresh) === JSON.stringify(manifest.chapters)) return null; - return { - manifest: { ...manifest, chapters: fresh }, - newOrder: reordered ? want.map((s) => s.path) : null, - }; -} - -/** The file order (book-relative paths) the player has a book loaded in, or is loading it - * in, else null. The playback store sets it (`setLoadedFileOrder`): it imports this - * module, so this one cannot ask it directly. */ -let loadedFileOrder: (ref: BookRef) => string[] | null = () => null; - -/** Set by the playback store at import (see `loadedFileOrder`). */ -export function setLoadedFileOrder(probe: typeof loadedFileOrder): void { - loadedFileOrder = probe; + return { manifest: { ...manifest, chapters: fresh }, reordered }; } /** - * `refreshedChapters`, held back when it plays the files in a new order while the player - * has the book loaded (or on its way in) in another one: the engine plays that queue's - * files by index (Android's service hands a JS restart only a track index, `adoptLoaded`), - * so the order the manifest gives them must not move under it. New chapters in the - * manifest's own order are taken whatever plays. A queue already in the fresh order (a - * start from the server's answer) takes it at once; a held one is taken once the book is - * no longer loaded that way (`adoptCachedChapters`, from the playback store). + * A start of a downloaded book loaded the engine from `chapters` (`playBook`, once its + * load landed): the manifest takes them, so a later start, offline or from Android's + * service (`adoptLoaded`, which gets only a track index), plays the order the engine was + * given. This is the only way the same files take a new order (server v2.4.0 plays them + * in natural order, "2-" before "10-"): the chapters carry the order every queue over the + * download plays them in (`bookFileSpecs`, each file found by its path: + * `localFromManifest`), and `files` itself never moves, since a file's index there is its + * name on disk (`fileName`, which `relocateEntry` and a retry rebuild it from). A + * background answer never does it (`refreshedChapters`): JS cannot see a book the Android + * service or a Cast receiver holds before it is adopted. Chapters naming other files + * change nothing. */ -function refreshedFor( - ref: BookRef, - manifest: DownloadManifest, - fresh: ChaptersResponse, -): DownloadManifest | null { - const refresh = chaptersRefresh(manifest, fresh); - if (!refresh?.newOrder) return refresh?.manifest ?? null; - const order = refresh.newOrder; - const loaded = loadedFileOrder(ref); - if (!loaded) return refresh.manifest; - const same = loaded.length === order.length && loaded.every((p, i) => p === order[i]); - return same ? refresh.manifest : null; -} - -/** The chapters answer the query cache holds for a book, if any. */ -function cachedChapters(ref: BookRef): ChaptersResponse | undefined { - return queryClient.getQueryData( - qk.chapters(ref.connectionId, ref.libraryId, ref.path), - ); +export function adoptStartedChapters(ref: BookRef, chapters: ChaptersResponse): void { + const entry = downloadedEntryOf(ref); + const manifest = entry && chaptersRefresh(entry.manifest, chapters)?.manifest; + if (!manifest) return; + patchEntry(contentKeyOf(ref), { manifest }); + void persist(); } /** * `manifest` with the chapters answer the cache already holds for its book, when - * `refreshedFor` takes it. `startChapterRefresh` only sees answers that land while the + * `refreshedChapters` takes it. `startChapterRefresh` only sees answers that land while the * book is downloaded; this takes one that landed before, while it was downloading or before * the launch's `hydrate` read the registry. That answer is the one the download was given * or a newer one (the callers read it from the same cache entry). */ function withCachedChapters(ref: BookRef, manifest: DownloadManifest): DownloadManifest { - const cached = cachedChapters(ref); - return (cached && refreshedFor(ref, manifest, cached)) || manifest; -} - -/** A downloaded book takes `fresh` when `refreshedFor` allows. A burst of answers (a - * reconnect's refetches, a search fan-out) saves the registry once, not once per book. */ -function takeChapters(ref: BookRef, fresh: ChaptersResponse): void { - const entry = downloadedEntryOf(ref); - const manifest = entry && refreshedFor(ref, entry.manifest, fresh); - if (!manifest) return; - patchEntry(contentKeyOf(ref), { manifest }); - persistSoon(); -} - -/** - * A downloaded book takes the chapters answer the cache holds for it now, when - * `refreshedFor` allows: one held back while the book was loaded in another order (the - * playback store calls this when the loaded book changes, and as a start ends). - */ -export function adoptCachedChapters(ref: BookRef): void { - const cached = cachedChapters(ref); - if (cached) takeChapters(ref, cached); + const cached = queryClient.getQueryData( + qk.chapters(ref.connectionId, ref.libraryId, ref.path), + ); + return (cached && refreshedChapters(manifest, cached)) || manifest; } /** * Keep downloaded books' chapters in step with the server: every chapters answer the * server gives (any screen's fetch, the play and download paths) for a downloaded book - * replaces its saved copy when `refreshedFor` allows. The cache already holds that + * replaces its saved copy when `refreshedChapters` allows. The cache already holds that * answer, so there is nothing to seed. A queue already playing keeps its chapters until - * the book is next started, and a new file order waits while the player has the book - * loaded in another one (`adoptCachedChapters` takes it once that ends). - * Returns the unsubscribe (started once from the root layout). + * the book is next started. Returns the unsubscribe (started once from the root layout). */ export function startChapterRefresh(): () => void { return queryClient.getQueryCache().subscribe((event) => { @@ -507,7 +471,15 @@ export function startChapterRefresh(): () => void { const book = chaptersKeyParts(event.query.queryKey); const fresh = event.action.data as ChaptersResponse | undefined; if (!book || !fresh) return; - takeChapters({ connectionId: book.cid, libraryId: book.libraryId, path: book.path }, fresh); + const key = downloadKey(book.cid, book.libraryId, book.path); + const entry = useDownloads.getState().entries[key]; + if (entry?.status !== 'downloaded') return; + const manifest = refreshedChapters(entry.manifest, fresh); + if (!manifest) return; + patchEntry(key, { manifest }); + // A burst of answers (a reconnect's refetches, a search fan-out) saves the registry + // once, not once per book. + persistSoon(); }); } diff --git a/src/downloads/types.ts b/src/downloads/types.ts index 94b000b6..6c60d915 100644 --- a/src/downloads/types.ts +++ b/src/downloads/types.ts @@ -46,7 +46,7 @@ export type DownloadOutcome = * chapters metadata, the local audio files, and a local cover. `files` is in the order * the download wrote them, each saved under its index there (`fileName` in store.ts), so * it never moves; the chapters give the play order (`bookFileSpecs`), which a later - * server answer can change (`refreshedChapters`). + * start in the server's new order can change (`adoptStartedChapters`). */ export type DownloadManifest = { book: Book; diff --git a/src/playback/book-queue.ts b/src/playback/book-queue.ts index 48367920..2ce7a958 100644 --- a/src/playback/book-queue.ts +++ b/src/playback/book-queue.ts @@ -33,9 +33,8 @@ export type FileSpec = { path: string; duration: number; size: number }; /** * The distinct audio files that make up a book, in play order - the single * source of truth for both playback (`buildBookQueue`) and offline downloads, so - * a download writes the files in play order (a later answer for the same files can - * play them in another; the chapters say which: `refreshedChapters` in the downloads - * store). Prefers an explicit file list; falls back to the files referenced by the + * a download writes the files in play order (a later start can play the same files + * in another; the chapters say which: `adoptStartedChapters` in the downloads store). Prefers an explicit file list; falls back to the files referenced by the * chapters; finally the book path itself. This must never resolve to a folder path. */ export function bookFileSpecs(book: Book, chapterData?: ChaptersResponse): FileSpec[] { diff --git a/src/playback/store-adopt.test.ts b/src/playback/store-adopt.test.ts index 7e013ab7..9f748931 100644 --- a/src/playback/store-adopt.test.ts +++ b/src/playback/store-adopt.test.ts @@ -54,8 +54,16 @@ jest.mock('./progress-sync', () => ({ })); const mockFetchQuery = jest.fn(async (..._a: unknown[]): Promise => ({})); +// The query cache's listeners (`startChapterRefresh` subscribes one). +const mockCacheListeners = new Set<(event: unknown) => void>(); jest.mock('@/api/provider', () => ({ queryClient: { + getQueryCache: () => ({ + subscribe: (l: (event: unknown) => void) => { + mockCacheListeners.add(l); + return () => mockCacheListeners.delete(l); + }, + }), invalidateQueries: jest.fn(), setQueryData: jest.fn(), setQueryDefaults: jest.fn(), @@ -79,7 +87,8 @@ jest.mock('@/api/connection-clients', () => ({ })); /* eslint-disable import/first */ -import { useDownloads } from '@/downloads/store'; +import { qk } from '@/api/hooks'; +import { startChapterRefresh, useDownloads } from '@/downloads/store'; import type { DownloadEntry } from '@/downloads/types'; import { setConnectionAliases } from '@/lib/connection-id'; import { resetProfilesForTests, switchProfile, useProfiles } from '@/stores/profiles'; @@ -323,6 +332,39 @@ describe('adoptLoaded', () => { ); }); + // The service plays B in its download's order after a JS restart; the server has since + // re-ordered B's files (v2.4.0's natural order). Its answer landing before the adoption must + // not move the order the adoption maps the engine's track index through. + it('maps the engine onto the download order a re-ordered server answer left alone', async () => { + useDownloads.setState({ entries: { 'c1:2:B': downloadedB() } }); + const stop = startChapterRefresh(); + const reordered: ChaptersResponse = { + ...twoFileChapters, + files: [...twoFileChapters.files].reverse().map((f, seq) => ({ ...f, seq })), + }; + for (const l of mockCacheListeners) { + l({ + type: 'updated', + query: { queryKey: qk.chapters('c1', 2, 'B') }, + action: { type: 'success', data: reordered }, + }); + } + stop(); + expect(useDownloads.getState().entries['c1:2:B'].manifest.chapters).toBe(twoFileChapters); + + await usePlayer.getState().adoptLoaded(loadedB({ trackIndex: 1, position: 30 })); + const { nowPlaying } = usePlayer.getState(); + expect(nowPlaying!.queue.tracks.map((t) => t.url)).toEqual([ + 'file:///d/1.mp3', + 'file:///d/2.mp3', + ]); + pushSnapshot(snap('playing', 1, 45)); + jest.advanceTimersByTime(15_000); // one save tick + await flushMicrotasks(); + // Track 1 is B/2.mp3, the second 100 s of the download's timeline. + expect(mockSaveProgress.mock.calls[0][1]).toMatchObject({ path: 'B', position: 145 }); + }); + it("saves the next time under the adopted book's path, at the engine's place", async () => { // Another book was loaded and playing in the app. await usePlayer.getState().playBook('c1', 2, makeBook(), undefined, 10); diff --git a/src/playback/store.test.ts b/src/playback/store.test.ts index 7e55a1bc..5dd22390 100644 --- a/src/playback/store.test.ts +++ b/src/playback/store.test.ts @@ -129,7 +129,7 @@ import { Platform } from 'react-native'; import { qk } from '@/api/hooks'; import { queryClient } from '@/api/provider'; import { engine as downloadEngine } from '@/downloads/engine'; -import { adoptCachedChapters, isDeclined, useDownloads } from '@/downloads/store'; +import { isDeclined, useDownloads } from '@/downloads/store'; import type { DownloadEntry, DownloadManifest } from '@/downloads/types'; import { resetProfilesForTests, switchProfile, useProfiles } from '@/stores/profiles'; import { useSettings } from '@/stores/settings'; @@ -1348,7 +1348,7 @@ describe('the download hot-swap', () => { // --- a downloaded book whose server now plays its files in another order ----------- // (server v2.4.0's natural order: "2-" before "10-"). The download takes the server's order -// and chapters, but never under a queue the player holds in the old order. +// and chapters only from a start that loaded the engine in it. describe('a downloaded book re-ordered on its server', () => { const folder = 'A/Folder'; const paths = ['A/Folder/10.mp3', 'A/Folder/2.mp3']; @@ -1419,33 +1419,29 @@ describe('a downloaded book re-ordered on its server', () => { ); }); - it('keeps the old order while a queue plays it, and takes the new one once it is gone', async () => { - // Started from the download's own copy (offline): the old order. + it('keeps the old order for an offline start, and after it, whatever the cache holds', async () => { + // Started from the download's own copy (offline, or the fresh read timed out). await usePlayer.getState().playBook('c1', 2, book, chaptersIn(paths), 0); + const tracks = usePlayer.getState().nowPlaying?.queue.tracks.map((t) => t.url); + expect(tracks).toEqual(['file:///dl/0.mp3', 'file:///dl/1.mp3']); expect(order()).toEqual(paths); + // The book unloads: the server's answer in the cache is still not taken. usePlayer.setState({ nowPlaying: null }); - expect(order()).toEqual(serverOrder); + expect(order()).toEqual(paths); }); - it('keeps the old order for a start in it whose place is still being looked up', async () => { - let lookedUp: (r: ResumeLookup) => void = () => {}; - mockLoadInitialProgress.mockImplementationOnce( - () => new Promise((resolve) => (lookedUp = resolve)), + it('takes nothing from a start overtaken by another before its load landed', async () => { + let loaded: () => void = () => {}; + (mockSvc.load as jest.Mock).mockImplementationOnce( + () => new Promise((resolve) => (loaded = resolve)), ); - // From the download's own copy (the fresh read timed out), then the place lookup. - const start = usePlayer.getState().playBook('c1', 2, book, chaptersIn(paths)); + const start = usePlayer.getState().playBook('c1', 2, book, chaptersIn(serverOrder), 0); await flushMicrotasks(); - expect(usePlayer.getState().nowPlaying).toBeNull(); - // The server's answer lands meanwhile: the start is about to load the old order. - adoptCachedChapters({ connectionId: 'c1', libraryId: 2, path: folder }); - expect(order()).toEqual(paths); - lookedUp({ kind: 'empty' }); + // Another book starts while this load is on its way: it owns the engine. + await startBook(makeBook(), 0); + loaded(); await start; - const tracks = usePlayer.getState().nowPlaying?.queue.tracks.map((t) => t.url); - expect(tracks).toEqual(['file:///dl/0.mp3', 'file:///dl/1.mp3']); expect(order()).toEqual(paths); - usePlayer.setState({ nowPlaying: null }); - expect(order()).toEqual(serverOrder); }); }); diff --git a/src/playback/store.ts b/src/playback/store.ts index 74205c85..db98e493 100644 --- a/src/playback/store.ts +++ b/src/playback/store.ts @@ -6,16 +6,14 @@ import { qk } from '@/api/hooks'; import { queryClient } from '@/api/provider'; import { isReachable, noteError } from '@/api/reachability'; import type { Book, Chapter, ChaptersResponse, Progress } from '@/api/types'; -import { trackFilePath } from '@/cast/cast-media'; import { - adoptCachedChapters, + adoptStartedChapters, downloadedEntryOf, downloadKey, - setLoadedFileOrder, useDownloads, } from '@/downloads/store'; import { resolveConnectionId } from '@/lib/connection-id'; -import { contentKey, contentKeyOf } from '@/lib/content-key'; +import { contentKey } from '@/lib/content-key'; import { canAutoDownload } from '@/lib/network'; import { mayActFor } from '@/stores/profiles'; import { useSettings } from '@/stores/settings'; @@ -148,11 +146,6 @@ let localMoves = 0; * the connection, the retry request and the save loop out from under it. */ let startsInFlight = 0; -/** The file order (book-relative paths) each `playBook` in flight loads its book in, by - * book (`contentKey`), from its queue's build until it ends: what the engine is about to - * hold before `nowPlaying` says so (`setLoadedFileOrder`, below). */ -const startingOrders = new Map(); - /** Captured so `retry()` can re-run the resume path after a lookup failure. */ let lastPlayRequest: { connectionId: string; @@ -791,10 +784,6 @@ export const usePlayer = create()((set, get) => ({ // stops only a book that is already `nowPlaying`): a start still looking up its place // must not load, play or save someone else's book once they have gone. const current = () => mayActFor(connectionId); - const ref = { connectionId, libraryId, path: book.rel_path }; - const key = contentKeyOf(ref); - /** The order this start loads the book in, once its queue is built (`startingOrders`). */ - let starting: string[] | null = null; // In flight until it ends: an adoption reading meanwhile stands back (`startsInFlight`). startsInFlight++; try { @@ -803,6 +792,7 @@ export const usePlayer = create()((set, get) => ({ const api = resolveClient(connectionId); // Play from local files when the book is downloaded (works fully offline). + const ref = { connectionId, libraryId, path: book.rel_path }; const dl = downloadedEntryOf(ref); const local = dl ? localFromManifest(dl.manifest) : undefined; @@ -839,9 +829,6 @@ export const usePlayer = create()((set, get) => ({ transcode, ); const nowPlaying = nowPlayingOf(ref, book, queue, api, local); - // A download's new file order waits for this start while its place is looked up. - starting = queue.tracks.map(trackFilePath); - startingOrders.set(key, starting); const svc = await ensureService(); let startAt = startBookPosition ?? 0; @@ -901,6 +888,7 @@ export const usePlayer = create()((set, get) => ({ // the save guard won't let progress regress below it without a deliberate seek. resumeFloor = toBookPosition(queue.offsets, index, positionInTrack); // A different book's place is not known until its load lands (`loadingBook`). + const key = contentKey(connectionId, libraryId, book.rel_path); const prev = get(); const loadingBook = selectBookKey(prev) === key ? prev.loadingBook : key; set({ rate: speed, nowPlaying, loadingBook }); @@ -928,6 +916,9 @@ export const usePlayer = create()((set, get) => ({ // Another book was started while this one loaded (a second tap, Up next): that start // owns the engine now, so this one neither sets its speed nor plays. if (selectBookKey(get()) !== key) return; + // The engine holds the queue built from these chapters now: a download takes them, + // with a new file order when its server re-ordered the files (`adoptStartedChapters`). + if (dl && chapterData) adoptStartedChapters(ref, chapterData); await svc.setRate(speed); if (!current()) return; await startEngine(svc); @@ -938,12 +929,6 @@ export const usePlayer = create()((set, get) => ({ void maybeAutoDownloadCurrent(connectionId, libraryId, book, chapterData); } finally { startsInFlight--; - if (starting) { - if (startingOrders.get(key) === starting) startingOrders.delete(key); - // A new order held back for this start is taken now, unless the book plays on in - // another one. - adoptCachedChapters(ref); - } } }, @@ -1401,28 +1386,6 @@ useDownloads.subscribe((state, prev) => { } }); -// A downloaded book takes a new file order from its server only while the player does not -// hold it in another order (`refreshedFor` in the downloads store): this is the order the -// loaded queue plays (the engine holds it until a start's load replaces it), else the order -// a start in flight is about to load, by book-relative path (a track's id is -// `:`). -setLoadedFileOrder((ref) => { - const s = usePlayer.getState(); - const key = contentKeyOf(ref); - if (s.nowPlaying && selectBookKey(s) === key) return s.nowPlaying.queue.tracks.map(trackFilePath); - return startingOrders.get(key) ?? null; -}); - -// And once the loaded book changes, a new order held back for the book that was loaded, -// or for the one now loaded in that order, is taken from the cached answer. -usePlayer.subscribe((state, prev) => { - if (state.nowPlaying === prev.nowPlaying) return; - if (prev.nowPlaying) adoptCachedChapters(prev.nowPlaying); - // The same book under a new object (the hot-swap, a restart): the call above covered it. - const now = state.nowPlaying; - if (now && selectBookKey(prev) !== selectBookKey(state)) adoptCachedChapters(now); -}); - // --- selectors ------------------------------------------------------------- export const selectBookPosition = (s: PlayerState): number => s.nowPlaying From 19c79fbdfe4427de5e13bdf7cb7384ef020b7572 Mon Sep 17 00:00:00 2001 From: KodeStar Date: Fri, 23 Oct 2026 20:35:39 +0100 Subject: [PATCH 5/5] Code review fixes --- CONVENTIONS.md | 8 +++++--- src/car/car-controller.ts | 4 ++-- src/downloads/store.test.ts | 2 ++ src/downloads/store.ts | 26 ++++++++++++++------------ src/playback/book-queue.ts | 5 +++-- src/playback/store.test.ts | 22 ++++++++++++++++++++++ src/playback/store.ts | 11 ++++++++--- 7 files changed, 56 insertions(+), 22 deletions(-) diff --git a/CONVENTIONS.md b/CONVENTIONS.md index b3356da9..1c809573 100644 --- a/CONVENTIONS.md +++ b/CONVENTIONS.md @@ -1776,9 +1776,11 @@ are asked, at most 10,000 rows. next started. The same files in another order (server v2.4.0 plays them in natural order, "2-" before "10-") are taken **only by a start that loads the engine in that order** (`adoptStartedChapters`, from `playBook` once its load landed and it still owns the engine; - saved at once), never from a background answer: JS sees only what it loaded, not a book the - Android service or a Cast receiver holds before `adoptLoaded` / the receiver adoption, which map - the engine's track index through the manifest. So an offline start, Android's no-JS car start + saved at once; a start given no chapters plays the download's own files and chapters), never + from a background answer, and a start's answer in the manifest's own order is left to the + background refresh: JS sees only what it loaded, not a book the Android service or a Cast + receiver holds before `adoptLoaded` / the receiver adoption, which map the engine's track index + through the manifest. So an offline start, Android's no-JS car start and `adoptLoaded` keep the manifest's order until an online start plays the new one. The chapters carry the order (every queue over a download orders its files by them and finds each by path); `manifest.files` itself never moves, since a file's index there is its name on disk diff --git a/src/car/car-controller.ts b/src/car/car-controller.ts index 46efd3ea..f205324f 100644 --- a/src/car/car-controller.ts +++ b/src/car/car-controller.ts @@ -981,8 +981,8 @@ export function carSyncReady(): Promise { /** Whether a book downloaded in both registries has other chapters in `next` (a chapters * refresh, `refreshedChapters`, or a start's, `adoptStartedChapters`, which can also give - * its files a new play order): its play - * spec is built from them (`playSpec`), and a refresh changes no download's status. */ + * its files a new play order): its play spec is built from them (`playSpec`), and a + * refresh changes no download's status. */ function chaptersChanged( next: Record, prev: Record, diff --git a/src/downloads/store.test.ts b/src/downloads/store.test.ts index 8d8ebefc..f30bd56f 100644 --- a/src/downloads/store.test.ts +++ b/src/downloads/store.test.ts @@ -751,6 +751,8 @@ describe("refreshing a downloaded book's chapters", () => { ); adoptStartedChapters(ref, resized); adoptStartedChapters({ ...ref, path: 'B/Book' }, reordered()); + // The same order is the background refresh's to take, not a start's (it can be older). + adoptStartedChapters(ref, fresh()); expect(useDownloads.getState().entries[key]).toBe(e); }); }); diff --git a/src/downloads/store.ts b/src/downloads/store.ts index 2c1d96f0..300a4663 100644 --- a/src/downloads/store.ts +++ b/src/downloads/store.ts @@ -374,7 +374,7 @@ export function reviveEntry(e: DownloadEntry, allPresent: boolean): DownloadEntr * it was downloaded (a rescan, a community chapter list fitted onto its audio, the admin * switching it back), and offline it plays and lists what the manifest holds. Only the * chapters move: when the fresh answer's audio files are not the ones on the device (the - * same paths in the same order, `bookFileSpecs` as the download read them, and the same + * same paths in the order the manifest plays them now, `bookFileSpecs`, and the same * sizes where both answers know them: a file replaced in place keeps its path), the local * audio no longer matches and that is a new download, so nothing changes. The same files * in another order are taken only by a start that plays them that way @@ -404,7 +404,9 @@ function chaptersRefresh( const read = bookFileSpecs(manifest.book, manifest.chapters ?? undefined); const readSize = new Map(read.map((s) => [s.path, s.size])); // The device's files, each one named exactly once, in any order. - const onDevice = new Set(manifest.files.map((f) => f.relPath)); + const onDisk = manifest.files.map((f) => f.relPath); + const onDiskSet = new Set(onDisk); + const onDevice = new Set(onDiskSet); for (const s of want) { if (!onDevice.delete(s.path)) return null; const was = readSize.get(s.path) ?? 0; @@ -412,9 +414,8 @@ function chaptersRefresh( } // The order the manifest plays them in now: its chapters', else (saved chapters that name // no file list of their own) the order the download wrote them. - const onDisk = manifest.files.map((f) => f.relPath); const playing = - read.length === onDisk.length && read.every((s) => onDisk.includes(s.path)) + read.length === onDisk.length && read.every((s) => onDiskSet.has(s.path)) ? read.map((s) => s.path) : onDisk; const reordered = want.some((s, i) => s.path !== playing[i]); @@ -437,9 +438,11 @@ function chaptersRefresh( */ export function adoptStartedChapters(ref: BookRef, chapters: ChaptersResponse): void { const entry = downloadedEntryOf(ref); - const manifest = entry && chaptersRefresh(entry.manifest, chapters)?.manifest; - if (!manifest) return; - patchEntry(contentKeyOf(ref), { manifest }); + const refresh = entry && chaptersRefresh(entry.manifest, chapters); + // The same order is the background refresh's to take (`startChapterRefresh`): a start's + // answer can be older (cached) than one the manifest took while it loaded. + if (!refresh?.reordered) return; + patchEntry(contentKeyOf(ref), { manifest: refresh.manifest }); void persist(); } @@ -471,12 +474,11 @@ export function startChapterRefresh(): () => void { const book = chaptersKeyParts(event.query.queryKey); const fresh = event.action.data as ChaptersResponse | undefined; if (!book || !fresh) return; - const key = downloadKey(book.cid, book.libraryId, book.path); - const entry = useDownloads.getState().entries[key]; - if (entry?.status !== 'downloaded') return; - const manifest = refreshedChapters(entry.manifest, fresh); + const ref = { connectionId: book.cid, libraryId: book.libraryId, path: book.path }; + const entry = downloadedEntryOf(ref); + const manifest = entry && refreshedChapters(entry.manifest, fresh); if (!manifest) return; - patchEntry(key, { manifest }); + patchEntry(contentKeyOf(ref), { manifest }); // A burst of answers (a reconnect's refetches, a search fan-out) saves the registry // once, not once per book. persistSoon(); diff --git a/src/playback/book-queue.ts b/src/playback/book-queue.ts index 2ce7a958..1d20ad6c 100644 --- a/src/playback/book-queue.ts +++ b/src/playback/book-queue.ts @@ -34,8 +34,9 @@ export type FileSpec = { path: string; duration: number; size: number }; * The distinct audio files that make up a book, in play order - the single * source of truth for both playback (`buildBookQueue`) and offline downloads, so * a download writes the files in play order (a later start can play the same files - * in another; the chapters say which: `adoptStartedChapters` in the downloads store). Prefers an explicit file list; falls back to the files referenced by the - * chapters; finally the book path itself. This must never resolve to a folder path. + * in another; the chapters say which: `adoptStartedChapters` in the downloads store). + * Prefers an explicit file list; falls back to the files referenced by the chapters; + * finally the book path itself. This must never resolve to a folder path. */ export function bookFileSpecs(book: Book, chapterData?: ChaptersResponse): FileSpec[] { const files = chapterData?.files?.length ? chapterData.files : (book.files ?? []); diff --git a/src/playback/store.test.ts b/src/playback/store.test.ts index 5dd22390..914bb9e8 100644 --- a/src/playback/store.test.ts +++ b/src/playback/store.test.ts @@ -1430,6 +1430,28 @@ describe('a downloaded book re-ordered on its server', () => { expect(order()).toEqual(paths); }); + it("plays the download's order when a start has no chapters for it", async () => { + // The server's item lists the files in its new order; its chapters read failed. + const fresh = { ...book, files: chaptersIn(serverOrder).files }; + await usePlayer.getState().playBook('c1', 2, fresh, undefined, 0); + const tracks = usePlayer.getState().nowPlaying?.queue.tracks.map((t) => t.url); + expect(tracks).toEqual(['file:///dl/0.mp3', 'file:///dl/1.mp3']); + expect(order()).toEqual(paths); + }); + + it("plays the download's file list when it saved no chapters and a start has none", async () => { + const e = useDownloads.getState().entries[key]; + const saved = { ...book, files: chaptersIn(paths).files }; + useDownloads.setState({ + entries: { [key]: { ...e, manifest: { ...e.manifest, book: saved, chapters: null } } }, + }); + const fresh = { ...book, files: chaptersIn(serverOrder).files }; + await usePlayer.getState().playBook('c1', 2, fresh, undefined, 0); + const tracks = usePlayer.getState().nowPlaying?.queue.tracks.map((t) => t.url); + expect(tracks).toEqual(['file:///dl/0.mp3', 'file:///dl/1.mp3']); + expect(order()).toEqual(paths); + }); + it('takes nothing from a start overtaken by another before its load landed', async () => { let loaded: () => void = () => {}; (mockSvc.load as jest.Mock).mockImplementationOnce( diff --git a/src/playback/store.ts b/src/playback/store.ts index db98e493..254e8773 100644 --- a/src/playback/store.ts +++ b/src/playback/store.ts @@ -819,11 +819,16 @@ export const usePlayer = create()((set, get) => ({ !local && mayNeedWebTranscode(book, chapterData) && (await resolveWebTranscode(connectionId, api, book, chapterData)); + // A downloaded book given no chapters (its chapters read failed) plays its download's + // files and chapters, the order its manifest holds, so the engine never holds an order + // the manifest does not (`adoptStartedChapters`). + const own = dl && !chapterData ? dl.manifest : undefined; + const queueChapters = chapterData ?? own?.chapters ?? undefined; const queue = buildBookQueue( api, libraryId, - book, - chapterData, + own ? { ...book, files: own.book.files, chapters: own.book.chapters } : book, + queueChapters, local, useSettings.getState().virtualChapterInterval, transcode, @@ -918,7 +923,7 @@ export const usePlayer = create()((set, get) => ({ if (selectBookKey(get()) !== key) return; // The engine holds the queue built from these chapters now: a download takes them, // with a new file order when its server re-ordered the files (`adoptStartedChapters`). - if (dl && chapterData) adoptStartedChapters(ref, chapterData); + if (dl && queueChapters) adoptStartedChapters(ref, queueChapters); await svc.setRate(speed); if (!current()) return; await startEngine(svc);