diff --git a/client/src/features/book/composables/__tests__/useBookNavigation.spec.ts b/client/src/features/book/composables/__tests__/useBookNavigation.spec.ts index 04770f69a..0553dd811 100644 --- a/client/src/features/book/composables/__tests__/useBookNavigation.spec.ts +++ b/client/src/features/book/composables/__tests__/useBookNavigation.spec.ts @@ -1,3 +1,4 @@ +import { nextTick, watch } from 'vue' import { afterEach, describe, expect, it } from 'vitest' import type { BookCard } from '@bookorbit/types' import { useBookNavigation } from '../useBookNavigation' @@ -139,4 +140,45 @@ describe('useBookNavigation', () => { expect(await nav.getNextId(2)).toBe(null) }) }) + + describe('re-sync stability (boundary loop guard)', () => { + it('does not invalidate bookIds when the slot context is structurally unchanged', async () => { + const nav = useBookNavigation() + nav.setBookSlotContext([makeBook(1), makeBook(2), makeBook(3)], 100) + + let fires = 0 + const stop = watch(nav.bookIds, () => { + fires++ + }) + + // A re-sync with the same ids/indexes (e.g. while a lazy block is still + // in flight) must not retrigger watchers that recompute next/prev id. + nav.setBookSlotContext([makeBook(1), makeBook(2), makeBook(3)], 100) + await nextTick() + expect(fires).toBe(0) + + // A genuine change still propagates. + nav.setBookSlotContext([makeBook(1), makeBook(2), makeBook(3), makeBook(4)], 100) + await nextTick() + expect(fires).toBe(1) + + stop() + }) + + it('does not invalidate bookIds when the fully-loaded context is unchanged', async () => { + const nav = useBookNavigation() + nav.setBookContext([1, 2, 3], 3) + + let fires = 0 + const stop = watch(nav.bookIds, () => { + fires++ + }) + + nav.setBookContext([1, 2, 3], 3) + await nextTick() + expect(fires).toBe(0) + + stop() + }) + }) }) diff --git a/client/src/features/book/composables/__tests__/useBookViewContext.spec.ts b/client/src/features/book/composables/__tests__/useBookViewContext.spec.ts index 8d8e9c7f4..f8cb8238c 100644 --- a/client/src/features/book/composables/__tests__/useBookViewContext.spec.ts +++ b/client/src/features/book/composables/__tests__/useBookViewContext.spec.ts @@ -72,6 +72,46 @@ describe('useBookViewContext', () => { expect(nextId).toBe(3) }) + it('advances across a block boundary when load-more resolves asynchronously', async () => { + const nav = useBookNavigation() + // 100 contiguous loaded slots (indexes 0..99); book 100 is the last loaded + // of the first block, with 900 more still to load. + const slots = ref(Array.from({ length: 100 }, (_, i) => makeBook(i + 1))) + const total = ref(1000) + + let releaseLoad: (() => void) | undefined + const loadMore = vi.fn<() => Promise>( + () => + new Promise((resolve) => { + releaseLoad = () => { + slots.value = [...slots.value, ...Array.from({ length: 100 }, (_, i) => makeBook(i + 101))] + resolve() + } + }), + ) + + const Harness = defineComponent({ + setup() { + useBookViewContext(slots, total, loadMore) + return {} + }, + template: '
', + }) + + const wrapper = mount(Harness) + // Mirror real navigation: the grid/list view unmounts when the editor opens. + wrapper.unmount() + + const nextPromise = nav.getNextId(100) + await Promise.resolve() + // The next block is still loading, so the id is not resolved synchronously. + expect(releaseLoad).toBeDefined() + releaseLoad?.() + + expect(await nextPromise).toBe(101) + expect(loadMore).toHaveBeenCalledTimes(1) + }) + it('reports the absolute slot index after a jump rail loads a far block', async () => { const nav = useBookNavigation() // First block (rows 0-1) plus a far block (rows 5000-5001) loaded; the diff --git a/client/src/features/book/composables/__tests__/useBookViewWindow.spec.ts b/client/src/features/book/composables/__tests__/useBookViewWindow.spec.ts new file mode 100644 index 000000000..d33887482 --- /dev/null +++ b/client/src/features/book/composables/__tests__/useBookViewWindow.spec.ts @@ -0,0 +1,222 @@ +import { nextTick, ref } from 'vue' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { BookCard, BooksPage, GroupRule, JumpBucket } from '@bookorbit/types' + +const fetchMock = vi.fn<(url: string, init?: RequestInit) => Promise>() + +vi.mock('@/lib/api', () => ({ + api: (url: string, init?: RequestInit) => fetchMock(url, init), +})) + +import { BOOK_WINDOW_BLOCK_SIZE } from '../useBookWindow' +import { useBookViewWindow } from '../useBookViewWindow' + +function makeBook(id: number): BookCard { + return { + id, + status: 'active', + title: `Book ${id}`, + authors: [], + seriesName: null, + seriesIndex: null, + files: [], + publishedYear: null, + language: null, + genres: [], + tags: [], + rating: null, + readingProgress: null, + readStatus: null, + addedAt: '2024-01-01T00:00:00Z', + updatedAt: null, + metadataScore: null, + hasCover: false, + hasMetadataLocks: false, + lockedFields: [], + subtitle: null, + publisher: null, + pageCount: null, + isbn13: null, + narrators: [], + } as BookCard +} + +function pageFor(block: number, total: number): BooksPage { + const start = block * BOOK_WINDOW_BLOCK_SIZE + const count = Math.max(0, Math.min(BOOK_WINDOW_BLOCK_SIZE, total - start)) + return { + items: Array.from({ length: count }, (_, i) => makeBook(start + i + 1)), + total, + page: block, + size: BOOK_WINDOW_BLOCK_SIZE, + } +} + +function bucket(key: string, index: number): JumpBucket { + return { key, label: key, index, count: 1 } as JumpBucket +} + +function mockApi(total: number, buckets: JumpBucket[] = []) { + fetchMock.mockImplementation((url, init) => { + if (url.includes('jump-buckets')) { + return Promise.resolve({ ok: true, json: () => Promise.resolve({ buckets }) }) + } + const body = JSON.parse(String(init?.body)) as { pagination: { page: number } } + return Promise.resolve({ ok: true, json: () => Promise.resolve(pageFor(body.pagination.page, total)) }) + }) +} + +async function flush() { + await Promise.resolve() + await Promise.resolve() + await nextTick() +} + +function setup(options: { scope?: number | null; viewMode?: string; collapse?: boolean; q?: string } = {}) { + const scopeId = ref(options.scope === undefined ? 1 : options.scope) + const viewMode = ref(options.viewMode ?? 'table') + const collapseEnabled = ref(options.collapse ?? false) + const q = ref(options.q ?? '') + const win = useBookViewWindow({ + scopeId, + listEndpoint: (id) => `/api/v1/libraries/${id}/books`, + bucketsEndpoint: (id) => `/api/v1/libraries/${id}/books/jump-buckets`, + viewMode, + collapseEnabled, + q, + }) + return { win, scopeId, viewMode, collapseEnabled, q } +} + +function listRequests(): number[] { + return fetchMock.mock.calls + .filter(([url]) => !url.includes('jump-buckets')) + .map(([, init]) => (JSON.parse(String(init?.body)) as { pagination: { page: number } }).pagination.page) +} + +describe('useBookViewWindow', () => { + beforeEach(() => { + fetchMock.mockReset() + }) + + it('loads the first block on creation and exposes window state', async () => { + mockApi(250) + const { win } = setup() + await flush() + + expect(win.total.value).toBe(250) + expect(win.contiguousPrefix.value).toHaveLength(100) + expect(win.hasMorePrefix.value).toBe(true) + }) + + it('does not fetch when the scope id is invalid', async () => { + mockApi(250) + const { win } = setup({ scope: null }) + await flush() + + expect(fetchMock).not.toHaveBeenCalled() + expect(win.total.value).toBe(0) + }) + + it('loadMorePrefix resolves only after the next contiguous chunk has loaded', async () => { + mockApi(500) + const { win } = setup() + await flush() + expect(win.contiguousPrefix.value).toHaveLength(100) + + let resolveBlock: ((value: unknown) => void) | undefined + fetchMock.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveBlock = resolve + }), + ) + + let settled = false + const pending = win.loadMorePrefix()!.then(() => { + settled = true + }) + await flush() + expect(settled).toBe(false) + + resolveBlock?.({ ok: true, json: () => Promise.resolve(pageFor(1, 500)) }) + await pending + expect(settled).toBe(true) + expect(win.contiguousPrefix.value).toHaveLength(200) + expect(win.hasMorePrefix.value).toBe(true) + }) + + it('handleRange loads far blocks and handleFirstVisibleIndex tracks the viewport', async () => { + mockApi(500) + const { win } = setup() + await flush() + fetchMock.mockClear() + + win.handleRange(300, 320) + await flush() + expect(listRequests()).toEqual([3]) + + win.handleFirstVisibleIndex(42) + expect(win.firstVisibleIndex.value).toBe(42) + }) + + it('reflects filter, collapse, and search in the query', async () => { + mockApi(120) + const { win } = setup({ collapse: true, q: ' dune ' }) + await flush() + + const filter: GroupRule = { type: 'group', join: 'AND', rules: [] } + win.filter.value = filter + + expect(win.query.value).toMatchObject({ + collapseSeries: true, + q: 'dune', + filter, + }) + }) + + it('reverses the letter template for a descending sort', async () => { + mockApi(120) + const { win } = setup() + await flush() + + expect(win.letterTemplate.value[0]).toBe('#') + + win.sort.value = [{ field: 'title', dir: 'desc' }] + await nextTick() + expect(win.letterTemplate.value[0]).not.toBe('#') + expect(win.bucketKind.value).not.toBeNull() + }) + + it('keeps the jump rail hidden in table mode', async () => { + mockApi(250, [bucket('A', 0), bucket('B', 120)]) + const { win } = setup({ viewMode: 'table' }) + await flush() + + expect(win.railVisible.value).toBe(false) + expect(win.railGutterReserved.value).toBe(false) + }) + + it('shows the jump rail in grid mode once buckets load and jumps scroll to the bucket index', async () => { + mockApi(250, [bucket('A', 0), bucket('B', 120)]) + const { win } = setup({ viewMode: 'grid' }) + await flush() + + expect(win.railVisible.value).toBe(true) + expect(win.activeBucketKey.value).toBe('A') + expect(typeof win.railGutterReserved.value).toBe('boolean') + + const scrollTo = vi.fn() + win.registerScroller(scrollTo) + fetchMock.mockClear() + + win.handleJump(bucket('B', 120)) + await flush() + + expect(scrollTo).toHaveBeenCalledWith(120) + expect(win.firstVisibleIndex.value).toBe(120) + expect(listRequests()).toContain(1) + + win.releaseRailGutter() + }) +}) diff --git a/client/src/features/book/composables/__tests__/useBookWindow.spec.ts b/client/src/features/book/composables/__tests__/useBookWindow.spec.ts index 165fb45f5..6c3131fad 100644 --- a/client/src/features/book/composables/__tests__/useBookWindow.spec.ts +++ b/client/src/features/book/composables/__tests__/useBookWindow.spec.ts @@ -244,4 +244,203 @@ describe('useBookWindow', () => { expect(window.total.value).toBe(299) expect(isBookPlaceholder(window.slots.value[100]!)).toBe(true) }) + + it('ensureRange resolves only after the requested block has finished loading', async () => { + mockBlocks(500) + const { window } = makeWindow() + await flush() + + let resolveBlock: ((value: unknown) => void) | undefined + fetchMock.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveBlock = resolve + }), + ) + + let settled = false + const pending = window.ensureRange(150, 199).then(() => { + settled = true + }) + await flush() + expect(settled).toBe(false) + expect(isBookPlaceholder(window.slots.value[150]!)).toBe(true) + + resolveBlock?.({ ok: true, json: () => Promise.resolve(pageFor(1, 500)) }) + await pending + expect(settled).toBe(true) + expect(isBookPlaceholder(window.slots.value[150]!)).toBe(false) + }) + + it('ensureRange resolves immediately without fetching when the range is already loaded', async () => { + mockBlocks(50) + const { window } = makeWindow() + await flush() + fetchMock.mockClear() + + await window.ensureRange(0, 49) + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('ensureRange resolves even when the block fails to load', async () => { + mockBlocks(300) + const { window } = makeWindow() + await flush() + fetchMock.mockClear() + fetchMock.mockResolvedValueOnce({ ok: false, status: 500 }) + + let settled = false + await window.ensureRange(100, 150).then(() => { + settled = true + }) + + expect(settled).toBe(true) + expect(requestedBlocks()).toEqual([1]) + expect(window.error.value).toBe('HTTP 500') + }) + + it('ensureRange awaits an already in-flight block rather than starting a second fetch', async () => { + mockBlocks(500) + const { window } = makeWindow() + await flush() + fetchMock.mockClear() + + let resolveBlock: ((value: unknown) => void) | undefined + fetchMock.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveBlock = resolve + }), + ) + + let firstSettled = false + let secondSettled = false + const first = window.ensureRange(150, 199).then(() => { + firstSettled = true + }) + const second = window.ensureRange(150, 199).then(() => { + secondSettled = true + }) + await flush() + expect(requestedBlocks()).toEqual([1]) + expect(firstSettled).toBe(false) + expect(secondSettled).toBe(false) + + resolveBlock?.({ ok: true, json: () => Promise.resolve(pageFor(1, 500)) }) + await Promise.all([first, second]) + expect(firstSettled).toBe(true) + expect(secondSettled).toBe(true) + }) + + it('retry refetches previously failed blocks and clears the error', async () => { + fetchMock.mockResolvedValueOnce({ ok: false, status: 500 }) + const { window } = makeWindow() + await flush() + expect(window.error.value).toBe('HTTP 500') + + fetchMock.mockClear() + mockBlocks(120) + window.retry() + await flush() + + expect(requestedBlocks()).toEqual([0]) + expect(window.error.value).toBeNull() + expect(window.total.value).toBe(120) + }) + + it('retry reloads from scratch when the listing is empty', async () => { + mockBlocks(0) + const { window } = makeWindow() + await flush() + expect(window.total.value).toBe(0) + + fetchMock.mockClear() + mockBlocks(40) + window.retry() + await flush() + + expect(requestedBlocks()).toEqual([0]) + expect(window.total.value).toBe(40) + }) + + it('retry re-ensures the first block without refetching what is already loaded', async () => { + mockBlocks(250) + const { window } = makeWindow() + await flush() + fetchMock.mockClear() + + window.retry() + await flush() + + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('booksProxy reconciles assignments by id (remove, update, prepend)', async () => { + mockBlocks(50) + const { window } = makeWindow() + await flush() + expect(window.booksProxy.value).toHaveLength(50) + + const current = window.booksProxy.value + window.booksProxy.value = [ + makeBook(900), + ...current.filter((card) => card.id !== 1).map((card) => (card.id === 2 ? makeBook(2, { title: 'Renamed' }) : card)), + ] + + const ids = window.loadedCards.value.map((card) => card.id) + expect(ids).not.toContain(1) + expect(ids).toContain(900) + expect((window.loadedCards.value.find((card) => card.id === 2) as BookCard).title).toBe('Renamed') + expect(window.total.value).toBe(50) + }) + + it('removeBooks is a no-op when none of the ids are loaded', async () => { + mockBlocks(20) + const { window } = makeWindow() + await flush() + + window.removeBooks([9999]) + expect(window.total.value).toBe(20) + expect(window.slots.value).toHaveLength(20) + }) + + it('prependBooks ignores cards whose ids are already loaded', async () => { + mockBlocks(20) + const { window } = makeWindow() + await flush() + + window.prependBooks([makeBook(1), makeBook(2)]) + expect(window.total.value).toBe(20) + }) + + it('booksProxy assignment with no structural change leaves the window untouched', async () => { + mockBlocks(20) + const { window } = makeWindow() + await flush() + + window.booksProxy.value = window.booksProxy.value + expect(window.total.value).toBe(20) + expect(window.loadedCards.value).toHaveLength(20) + }) + + it('updateBook is a no-op when the id is not loaded', async () => { + mockBlocks(20) + const { window } = makeWindow() + await flush() + + const before = window.slots.value + window.updateBook(makeBook(9999, { title: 'Ghost' })) + expect(window.slots.value).toBe(before) + }) + + it('booksProxy replaces a card in place when only its contents change', async () => { + mockBlocks(20) + const { window } = makeWindow() + await flush() + + window.booksProxy.value = window.booksProxy.value.map((card) => (card.id === 5 ? makeBook(5, { title: 'Updated' }) : card)) + + expect((window.loadedCards.value.find((card) => card.id === 5) as BookCard).title).toBe('Updated') + expect(window.total.value).toBe(20) + }) }) diff --git a/client/src/features/book/composables/useBookNavigation.ts b/client/src/features/book/composables/useBookNavigation.ts index af0b91a38..211df7090 100644 --- a/client/src/features/book/composables/useBookNavigation.ts +++ b/client/src/features/book/composables/useBookNavigation.ts @@ -17,9 +17,22 @@ const positionById = computed(() => { return map }) +// Skip the reassignment when the listing is structurally identical. A re-sync +// with the same ids/indexes (e.g. while a lazy block is still in flight) must +// not invalidate `entries`, or watchers that recompute the next/prev id would +// fire in a loop and starve the in-flight fetch. +function sameEntries(a: NavEntry[], b: NavEntry[]): boolean { + if (a.length !== b.length) return false + for (let i = 0; i < a.length; i++) { + if (a[i]!.id !== b[i]!.id || a[i]!.index !== b[i]!.index) return false + } + return true +} + export function useBookNavigation() { const setBookContext = (ids: number[], totalCount: number) => { - entries.value = ids.map((id, index) => ({ id, index })) + const next = ids.map((id, index) => ({ id, index })) + if (!sameEntries(entries.value, next)) entries.value = next total.value = totalCount } @@ -29,7 +42,7 @@ export function useBookNavigation() { const slot = slots[i]! if (!isBookPlaceholder(slot)) next.push({ id: slot.id, index: i }) } - entries.value = next + if (!sameEntries(entries.value, next)) entries.value = next total.value = totalCount } diff --git a/client/src/features/book/composables/useBookViewWindow.ts b/client/src/features/book/composables/useBookViewWindow.ts index 27a0a0218..a5b5a9974 100644 --- a/client/src/features/book/composables/useBookViewWindow.ts +++ b/client/src/features/book/composables/useBookViewWindow.ts @@ -90,7 +90,7 @@ export function useBookViewWindow(options: { function loadMorePrefix() { const start = window.contiguousPrefix.value.length - window.ensureRange(start, start + LIST_CHUNK - 1) + return window.ensureRange(start, start + LIST_CHUNK - 1) } return { diff --git a/client/src/features/book/composables/useBookWindow.ts b/client/src/features/book/composables/useBookWindow.ts index 4116af953..1a0683c6d 100644 --- a/client/src/features/book/composables/useBookWindow.ts +++ b/client/src/features/book/composables/useBookWindow.ts @@ -30,7 +30,7 @@ export function useBookWindow(options: { endpoint: Ref; query: Re const initialized = shallowRef(false) const error = shallowRef(null) - const inFlight = new Set() + const inFlight = new Map>() const failedAt = new Map() let generation = 0 let controller: AbortController | null = null @@ -58,13 +58,24 @@ export function useBookWindow(options: { endpoint: Ref; query: Re return true } - async function fetchBlock(block: number) { + // Returns the block's load promise so callers (e.g. list-mode load-more) can + // await the fetch instead of firing it and racing the result. A block already + // in flight returns its existing promise rather than starting a second fetch. + function fetchBlock(block: number): Promise { + const existing = inFlight.get(block) + if (existing) return existing + if (!options.endpoint.value) return Promise.resolve() + const promise = loadBlock(block) + inFlight.set(block, promise) + return promise + } + + async function loadBlock(block: number) { const endpoint = options.endpoint.value if (!endpoint) return const gen = generation const signal = controller?.signal - inFlight.add(block) loading.value = true try { @@ -106,9 +117,9 @@ export function useBookWindow(options: { endpoint: Ref; query: Re } } - function ensureRange(startIndex: number, endIndex: number) { - if (!options.endpoint.value) return - if (initialized.value && total.value === 0 && failedAt.size === 0) return + function ensureRange(startIndex: number, endIndex: number): Promise { + if (!options.endpoint.value) return Promise.resolve() + if (initialized.value && total.value === 0 && failedAt.size === 0) return Promise.resolve() const start = Math.max(0, startIndex) let end = Math.max(start, endIndex) if (total.value > 0) end = Math.min(end, total.value - 1) @@ -116,13 +127,19 @@ export function useBookWindow(options: { endpoint: Ref; query: Re const firstBlock = Math.floor(start / BOOK_WINDOW_BLOCK_SIZE) const lastBlock = Math.floor(end / BOOK_WINDOW_BLOCK_SIZE) + const pending: Promise[] = [] for (let block = firstBlock; block <= lastBlock; block++) { - if (inFlight.has(block)) continue + const existing = inFlight.get(block) + if (existing) { + pending.push(existing) + continue + } const failed = failedAt.get(block) if (failed !== undefined && Date.now() - failed < FAILED_BLOCK_RETRY_MS) continue if (initialized.value && blockFullyLoaded(block)) continue - void fetchBlock(block) + pending.push(fetchBlock(block)) } + return pending.length > 0 ? Promise.all(pending).then(() => undefined) : Promise.resolve() } function invalidateInFlight() {