From 8f969b12359520d19c8d3ffb31afe1b431385bb7 Mon Sep 17 00:00:00 2001 From: Jonathan Finley Date: Sat, 14 Mar 2026 00:09:48 -0400 Subject: [PATCH] fix: complete large-library browse follow-up paths --- server/controllers/LibraryController.js | 1 + server/utils/queries/libraryBrowseStrategy.js | 40 ++++- .../utils/queries/libraryItemsBookFilters.js | 81 +++++---- .../queries/libraryItemsPodcastFilters.js | 1 + ...braryController.largeLibraryBrowse.test.js | 156 +++++++++++++++++- .../utils/libraryBrowseStrategy.test.js | 33 +++- 6 files changed, 259 insertions(+), 53 deletions(-) diff --git a/server/controllers/LibraryController.js b/server/controllers/LibraryController.js index 35d9cabad..5d972bb18 100644 --- a/server/controllers/LibraryController.js +++ b/server/controllers/LibraryController.js @@ -646,6 +646,7 @@ class LibraryController { if (filterByGroup === 'series' && filterByValue !== 'no-series' && payload.collapseseries) { const seriesId = libraryFilters.decode(payload.filterBy.split('.')[1]) payload.sortBy = libraryItemsBookFilters.getCollapsedSeriesBrowseSort(payload.sortBy) + payload.browseProfile = browseProfile const collapsedSeriesWindow = { limit: Number(payload.limit) || 0, offset: Number(payload.offset) || 0 diff --git a/server/utils/queries/libraryBrowseStrategy.js b/server/utils/queries/libraryBrowseStrategy.js index 91e26eb23..7d084d443 100644 --- a/server/utils/queries/libraryBrowseStrategy.js +++ b/server/utils/queries/libraryBrowseStrategy.js @@ -2,11 +2,38 @@ function getTitleCursorKey() { return global.ServerSettings && global.ServerSettings.sortingIgnorePrefix ? 'titleIgnorePrefix' : 'title' } -function getKeysetCursorKeys(sortBy, collapseseries) { +function getProgressCursorKeys(sortBy, filterValue) { + const supportedFilters = ['finished', 'in-progress', 'audio-in-progress', 'ebook-in-progress', 'ebook-finished'] + const supportedFinishedAtFilters = ['finished', 'ebook-finished'] + + if (sortBy === 'progress' && supportedFilters.includes(filterValue)) { + return ['mediaProgresses.updatedAt', 'id'] + } + + if (sortBy === 'progress.createdAt' && supportedFilters.includes(filterValue)) { + return ['mediaProgresses.createdAt', 'id'] + } + + if (sortBy === 'progress.finishedAt' && supportedFinishedAtFilters.includes(filterValue)) { + return ['mediaProgresses.finishedAt', 'id'] + } + + return [] +} + +function getKeysetCursorKeys({ mediaType, sortBy, filterGroup, filterValue, collapseseries }) { + if (mediaType === 'podcast') { + return [] + } + if (sortBy === 'media.metadata.title' && collapseseries) { return [] } + if (filterGroup === 'progress') { + return getProgressCursorKeys(sortBy, filterValue) + } + if (sortBy === 'media.metadata.title') { return [getTitleCursorKey(), 'id'] } @@ -30,12 +57,12 @@ function getKeysetCursorKeys(sortBy, collapseseries) { return [] } -function getPaginationMode(pageMode, sortBy, collapseseries) { +function getPaginationMode({ mediaType, pageMode, sortBy, filterGroup, filterValue, collapseseries }) { if (pageMode !== 'endless') { return 'offset' } - return getKeysetCursorKeys(sortBy, collapseseries).length ? 'keyset' : 'offset' + return getKeysetCursorKeys({ mediaType, sortBy, filterGroup, filterValue, collapseseries }).length ? 'keyset' : 'offset' } function getFamily({ mediaType, sortBy, filterGroup, collapseseries }) { @@ -50,9 +77,10 @@ function getFamily({ mediaType, sortBy, filterGroup, collapseseries }) { return 'plain-browse' } -function getLibraryBrowseStrategy({ mediaType, sortBy, filterGroup, pageMode, collapseseries } = {}) { +function getLibraryBrowseStrategy({ mediaType, sortBy, filterGroup, filterValue, pageMode, collapseseries } = {}) { const normalizedSort = sortBy || 'media.metadata.title' - const paginationMode = getPaginationMode(pageMode, normalizedSort, collapseseries) + const cursorKeys = getKeysetCursorKeys({ mediaType, sortBy: normalizedSort, filterGroup, filterValue, collapseseries }) + const paginationMode = getPaginationMode({ mediaType, pageMode, sortBy: normalizedSort, filterGroup, filterValue, collapseseries }) if (normalizedSort === 'random') { return { @@ -71,7 +99,7 @@ function getLibraryBrowseStrategy({ mediaType, sortBy, filterGroup, pageMode, co countMode: paginationMode === 'keyset' ? 'deferred-exact' : 'exact-on-initial-page', deepScrollAllowed: paginationMode === 'keyset', tieBreaker: 'id', - cursorKeys: getKeysetCursorKeys(normalizedSort, collapseseries) + cursorKeys } } diff --git a/server/utils/queries/libraryItemsBookFilters.js b/server/utils/queries/libraryItemsBookFilters.js index 792320bb8..9a85b16f9 100644 --- a/server/utils/queries/libraryItemsBookFilters.js +++ b/server/utils/queries/libraryItemsBookFilters.js @@ -774,11 +774,11 @@ module.exports = { const nullDir = sortDesc ? 'DESC NULLS FIRST' : 'ASC NULLS LAST' return [[Sequelize.literal(`CAST(\`series.bookSeries.sequence\` AS FLOAT) ${nullDir}`)]] } else if (sortBy === 'progress') { - return [[Sequelize.literal(`mediaProgresses.updatedAt ${dir} NULLS LAST`)]] + return [[Sequelize.literal(`mediaProgresses.updatedAt ${dir} NULLS LAST`)], libraryItemIdOrder] } else if (sortBy === 'progress.createdAt') { - return [[Sequelize.literal(`mediaProgresses.createdAt ${dir} NULLS LAST`)]] + return [[Sequelize.literal(`mediaProgresses.createdAt ${dir} NULLS LAST`)], libraryItemIdOrder] } else if (sortBy === 'progress.finishedAt') { - return [[Sequelize.literal(`mediaProgresses.finishedAt ${dir} NULLS LAST`)]] + return [[Sequelize.literal(`mediaProgresses.finishedAt ${dir} NULLS LAST`)], libraryItemIdOrder] } else if (sortBy === 'random') { return [Database.sequelize.random()] } else if (sortBy === 'updatedAt') { @@ -1114,6 +1114,7 @@ module.exports = { mediaType: 'book', sortBy, filterGroup, + filterValue, pageMode: browseRequestOptions?.pageMode, collapseseries }) @@ -1311,20 +1312,60 @@ module.exports = { limit: Number(window.limit) || null, offset: Number(window.offset) || 0 } + const rowsTiming = createBrowsePhaseTiming(payload?.browseProfile, 'rows') + const countTiming = createBrowsePhaseTiming(payload?.browseProfile, 'count') - const metaResult = await Database.sequelize.query(buildCollapsedSeriesMetaQuery(baseQuery, payload.hideSingleBookSeries), { + const metaResult = await runWithBrowsePhase(countTiming, async () => Database.sequelize.query(buildCollapsedSeriesMetaQuery(baseQuery, payload.hideSingleBookSeries), { replacements, type: Database.sequelize.QueryTypes.SELECT - }) + })) const metaRows = Array.isArray(metaResult[0]) ? metaResult[0] : metaResult const meta = Array.isArray(metaRows) ? metaRows[0] : metaRows const fallbackToPlain = Number(meta?.plainRowCount || 0) === 0 && Number(meta?.collapsedRowCount || 0) === 1 - const selectedRowsResult = await Database.sequelize.query(buildCollapsedSeriesRowsQuery(baseQuery, sortContext, payload.hideSingleBookSeries, fallbackToPlain), { - replacements, - type: Database.sequelize.QueryTypes.SELECT + const { visibleRows, hydratedBooks } = await runWithBrowsePhase(rowsTiming, async () => { + const selectedRowsResult = await Database.sequelize.query(buildCollapsedSeriesRowsQuery(baseQuery, sortContext, payload.hideSingleBookSeries, fallbackToPlain), { + replacements, + type: Database.sequelize.QueryTypes.SELECT + }) + const selectedRows = Array.isArray(selectedRowsResult[0]) ? selectedRowsResult[0] : selectedRowsResult + + const selectedPlainBookIds = selectedRows.filter((row) => row.rowType === 'plain').map((row) => row.anchorBookId) + const selectedSeriesIds = selectedRows.filter((row) => row.rowType === 'collapsed').map((row) => row.subseriesId) + if (!selectedPlainBookIds.length && !selectedSeriesIds.length) { + return { + visibleRows: selectedRows, + hydratedBooks: [] + } + } + + const hydrateWhere = [] + if (selectedPlainBookIds.length) { + hydrateWhere.push({ id: selectedPlainBookIds }) + } + if (selectedSeriesIds.length) { + hydrateWhere.push(existsLiteral(`SELECT 1 FROM "bookSeries" collapseSeries WHERE collapseSeries."bookId" = book.id AND collapseSeries."seriesId" IN (:selectedCollapsedSeriesIds)`)) + } + + const loadedBooks = await Database.bookModel.findAll({ + ...findOptions, + where: [ + userPermissionBookWhere.bookWhere, + { + [Sequelize.Op.or]: hydrateWhere + } + ], + replacements: { + ...userPermissionBookWhere.replacements, + selectedCollapsedSeriesIds: selectedSeriesIds + } + }) + + return { + visibleRows: selectedRows, + hydratedBooks: loadedBooks + } }) - const visibleRows = Array.isArray(selectedRowsResult[0]) ? selectedRowsResult[0] : selectedRowsResult const selectedPlainBookIds = visibleRows.filter((row) => row.rowType === 'plain').map((row) => row.anchorBookId) const selectedSeriesIds = visibleRows.filter((row) => row.rowType === 'collapsed').map((row) => row.subseriesId) @@ -1335,28 +1376,6 @@ module.exports = { } } - const hydrateWhere = [] - if (selectedPlainBookIds.length) { - hydrateWhere.push({ id: selectedPlainBookIds }) - } - if (selectedSeriesIds.length) { - hydrateWhere.push(existsLiteral(`SELECT 1 FROM "bookSeries" collapseSeries WHERE collapseSeries."bookId" = book.id AND collapseSeries."seriesId" IN (:selectedCollapsedSeriesIds)`)) - } - - const hydratedBooks = await Database.bookModel.findAll({ - ...findOptions, - where: [ - userPermissionBookWhere.bookWhere, - { - [Sequelize.Op.or]: hydrateWhere - } - ], - replacements: { - ...userPermissionBookWhere.replacements, - selectedCollapsedSeriesIds: selectedSeriesIds - } - }) - const allLibraryItems = hydratedBooks .map((bookExpanded) => mapCollapsedSeriesBookToLibraryItem(bookExpanded)) .filter((libraryItem) => user?.checkCanAccessLibraryItem ? user.checkCanAccessLibraryItem(libraryItem) : true) diff --git a/server/utils/queries/libraryItemsPodcastFilters.js b/server/utils/queries/libraryItemsPodcastFilters.js index 01897f096..83b40bb44 100644 --- a/server/utils/queries/libraryItemsPodcastFilters.js +++ b/server/utils/queries/libraryItemsPodcastFilters.js @@ -212,6 +212,7 @@ module.exports = { mediaType: 'podcast', sortBy, filterGroup, + filterValue, pageMode: browseRequestOptions?.pageMode, collapseseries: browseRequestOptions?.collapseseries }) diff --git a/test/server/controllers/LibraryController.largeLibraryBrowse.test.js b/test/server/controllers/LibraryController.largeLibraryBrowse.test.js index 744144dc3..81ed422aa 100644 --- a/test/server/controllers/LibraryController.largeLibraryBrowse.test.js +++ b/test/server/controllers/LibraryController.largeLibraryBrowse.test.js @@ -315,6 +315,34 @@ describe('LibraryController large-library browse contract', () => { }) }) + it('forwards the live browse profile into collapsed-series browse requests', async () => { + const getCollapsedSeriesWindowStub = sinon.stub(libraryItemsBookFilters, 'getCollapsedSeriesWindow').resolves({ + libraryItems: [], + count: 0 + }) + sinon.stub(libraryHelpers, 'toCollapsedSeriesPayload').resolves([]) + + const req = { + query: { + filter: `series.${Buffer.from('series_1').toString('base64')}`, + collapseseries: '1', + limit: '20', + sort: 'sequence' + }, + library: { id: 'lib_1', mediaType: 'book', isVirtual: false, settings: {} }, + user: { id: 'user_1' } + } + const res = { json: sinon.spy() } + + await LibraryController.getLibraryItems(req, res) + + expect(getCollapsedSeriesWindowStub.firstCall.args[4].browseProfile).to.include({ + route: 'GET /api/libraries/:id/items', + libraryId: 'lib_1' + }) + expect(getCollapsedSeriesWindowStub.firstCall.args[4].browseProfile.mark).to.be.a('function') + }) + it('does not run exact count work or joined findAndCountAll again on follow-up keyset chunks', async () => { const countStub = sinon.stub(Database.bookModel, 'count').resolves(123) const findAllStub = sinon.stub(Database.bookModel, 'findAll').resolves([]) @@ -341,17 +369,18 @@ describe('LibraryController large-library browse contract', () => { expect(findAndCountAllStub.called).to.equal(false) }) - it('falls back to offset mode for endless progress browse because null-aware keyset is unsupported', () => { + it('uses keyset metadata for supported endless progress browse', () => { const strategy = getLibraryBrowseStrategy({ mediaType: 'book', sortBy: 'progress', filterGroup: 'progress', + filterValue: 'in-progress', pageMode: 'endless' }) - expect(strategy.paginationMode).to.equal('offset') - expect(strategy.countMode).to.equal('exact-on-initial-page') - expect(strategy.deepScrollAllowed).to.equal(false) + expect(strategy.paginationMode).to.equal('keyset') + expect(strategy.countMode).to.equal('deferred-exact') + expect(strategy.deepScrollAllowed).to.equal(true) }) it('falls back to offset mode for collapsed-series endless title browse because the SQL sort key differs', () => { @@ -394,6 +423,44 @@ describe('LibraryController large-library browse contract', () => { expect(findAllStub.called).to.equal(false) }) + it('records browse phases for collapsed-series browse queries when a live browse profile is present', async () => { + const queryStub = sinon.stub(Database.sequelize, 'query') + queryStub.onCall(0).callsFake(async () => [{ rawBookCount: 1, plainRowCount: 1, collapsedRowCount: 0 }]) + queryStub.onCall(1).callsFake(async () => [{ rowType: 'plain', anchorBookId: 'book_1', anchorLibraryItemId: 'li_1', subseriesId: null }]) + sinon.stub(Database.bookModel, 'findAll').resolves([ + { + libraryItem: { + id: 'li_1', + mediaType: 'book', + toOldJSONMinified() { + return { id: 'li_1', media: { metadata: {}, duration: 1 } } + } + }, + series: [{ id: 'series_1', name: 'Main Series', bookSeries: { sequence: '1' } }], + bookAuthors: [] + } + ]) + + const browseProfile = createBrowseRequestProfile({ + route: 'GET /api/libraries/:id/items', + libraryId: 'lib_1' + }) + + await libraryItemsBookFilters.getCollapsedSeriesWindow( + 'lib_1', + 'series_1', + { id: 'user_1', canAccessExplicitContent: true, accessAllTags: true, checkCanAccessLibraryItem: () => true }, + [], + { sortBy: 'sequence', sortDesc: false, hideSingleBookSeries: false, browseProfile }, + { limit: 20, offset: 0 } + ) + + const summary = finishBrowseRequestProfile(browseProfile, { slowMs: 0 }) + + expect(summary.phases).to.have.property('count') + expect(summary.phases).to.have.property('rows') + }) + it('uses bounded batch query options for collapsed-series follow-up windows', async () => { const findByPkStub = sinon.stub(Database.seriesModel, 'findByPk').resolves({ books: [] }) const queryStub = sinon.stub(Database.sequelize, 'query') @@ -485,6 +552,31 @@ describe('LibraryController large-library browse contract', () => { expect(queryStub.secondCall.args[0]).to.include('filterSequenceSort') }) + it('does not advertise keyset metadata for podcast endless browse without cursor follow-up support', async () => { + sinon.stub(Database.podcastModel, 'findAndCountAll').resolves({ rows: [], count: 4 }) + sinon.stub(libraryItemsPodcastFilters, 'getMediaGroupQuery').returns({ mediaWhere: {}, replacements: {} }) + sinon.stub(libraryItemsPodcastFilters, 'getUserPermissionPodcastWhereQuery').returns({ podcastWhere: [], replacements: {} }) + sinon.stub(libraryItemsPodcastFilters, 'getOrder').returns([]) + + const result = await libraryItemsPodcastFilters.getFilteredLibraryItems( + 'lib_2', + { id: 'user_2', canAccessExplicitContent: true, accessAllTags: true }, + null, + null, + 'media.metadata.title', + false, + [], + 40, + 0, + { cursor: 'cursor-9', pageMode: 'endless' } + ) + + expect(result.paginationMode).to.equal('offset') + expect(result.deepScrollAllowed).to.equal(false) + expect(result.countMode).to.equal('exact-on-initial-page') + expect(result.nextCursor).to.equal(null) + }) + it('falls back to sequence-based collapsed browse ordering for unsupported sorts', async () => { const queryStub = sinon.stub(Database.sequelize, 'query') queryStub.onCall(0).resolves([{ rawBookCount: 0, plainRowCount: 0, collapsedRowCount: 0 }]) @@ -737,7 +829,53 @@ describe('LibraryController large-library browse contract', () => { expect(findAllStub.firstCall.args[0].order[1][0].val || findAllStub.firstCall.args[0].order[1][0]).to.include('id') }) - it('uses offset fallback for progress browse requests with null-valued sort fields', async () => { + it('uses keyset pagination for supported progress browse requests', async () => { + const countStub = sinon.stub(Database.bookModel, 'count').resolves(123) + const findAllStub = sinon.stub(Database.bookModel, 'findAll').resolves([ + { + id: 'book-1', + mediaProgresses: [{ updatedAt: '2024-01-01T00:00:00.000Z' }], + libraryItem: { id: 'item-1', dataValues: {} } + }, + { + id: 'book-2', + mediaProgresses: [{ updatedAt: '2024-01-02T00:00:00.000Z' }], + libraryItem: { id: 'item-2', dataValues: {} } + }, + { + id: 'book-3', + mediaProgresses: [{ updatedAt: '2024-01-03T00:00:00.000Z' }], + libraryItem: { id: 'item-3', dataValues: {} } + } + ]) + const findAndCountAllStub = sinon.stub(Database.bookModel, 'findAndCountAll').resolves({ rows: [], count: 7 }) + + const result = await libraryItemsBookFilters.getFilteredLibraryItems( + 'lib_1', + { id: 'user_1', canAccessExplicitContent: true, accessAllTags: true }, + 'progress', + 'in-progress', + 'progress', + true, + false, + [], + 2, + 0, + false, + { pageMode: 'endless' } + ) + + expect(result.paginationMode).to.equal('keyset') + expect(result.countMode).to.equal('deferred-exact') + expect(result.nextCursor).to.be.a('string') + expect(findAllStub.calledOnce).to.equal(true) + expect(countStub.calledOnce).to.equal(true) + expect(findAndCountAllStub.called).to.equal(false) + expect(findAllStub.firstCall.args[0].order[1][0].val || findAllStub.firstCall.args[0].order[1][0]).to.include('libraryItem') + expect(findAllStub.firstCall.args[0].order[1][0].val || findAllStub.firstCall.args[0].order[1][0]).to.include('id') + }) + + it('skips exact counts on follow-up supported progress browse chunks', async () => { const countStub = sinon.stub(Database.bookModel, 'count').resolves(123) const findAllStub = sinon.stub(Database.bookModel, 'findAll').resolves([]) const findAndCountAllStub = sinon.stub(Database.bookModel, 'findAndCountAll').resolves({ rows: [], count: 7 }) @@ -757,11 +895,11 @@ describe('LibraryController large-library browse contract', () => { { cursor: 'cursor-2', pageMode: 'endless' } ) - expect(result.paginationMode).to.equal('offset') - expect(result.countMode).to.equal('exact-on-initial-page') - expect(findAndCountAllStub.calledOnce).to.equal(true) - expect(findAllStub.called).to.equal(false) + expect(result.paginationMode).to.equal('keyset') + expect(result.count).to.equal(null) + expect(findAllStub.calledOnce).to.equal(true) expect(countStub.called).to.equal(false) + expect(findAndCountAllStub.called).to.equal(false) }) it('builds EXISTS predicates for browse filters and permission-tag checks instead of correlated count subqueries', () => { diff --git a/test/server/utils/libraryBrowseStrategy.test.js b/test/server/utils/libraryBrowseStrategy.test.js index bf47355ed..53cf0e6de 100644 --- a/test/server/utils/libraryBrowseStrategy.test.js +++ b/test/server/utils/libraryBrowseStrategy.test.js @@ -64,17 +64,18 @@ describe('libraryBrowseStrategy', () => { expect(strategy.cursorKeys).to.deep.equal(['createdAt', 'id']) }) - it('falls back to offset pagination for progress browse because null-aware keyset is unsupported', () => { + it('uses keyset pagination for supported in-progress browse sorts', () => { const strategy = getLibraryBrowseStrategy({ mediaType: 'book', sortBy: 'progress', filterGroup: 'progress', + filterValue: 'in-progress', pageMode: 'endless' }) expect(strategy.family).to.equal('progress-browse') - expect(strategy.paginationMode).to.equal('offset') - expect(strategy.cursorKeys).to.deep.equal([]) + expect(strategy.paginationMode).to.equal('keyset') + expect(strategy.cursorKeys).to.deep.equal(['mediaProgresses.updatedAt', 'id']) }) it('uses keyset pagination for updated browse', () => { @@ -120,24 +121,40 @@ describe('libraryBrowseStrategy', () => { expect(strategy.cursorKeys).to.deep.equal(['authorNamesLastFirst', 'title', 'id']) }) - it('falls back to offset pagination for progress created-at browse because null-aware keyset is unsupported', () => { + it('uses keyset pagination for supported progress created-at browse', () => { const strategy = getLibraryBrowseStrategy({ mediaType: 'book', sortBy: 'progress.createdAt', filterGroup: 'progress', + filterValue: 'in-progress', pageMode: 'endless' }) expect(strategy.family).to.equal('progress-browse') - expect(strategy.paginationMode).to.equal('offset') - expect(strategy.cursorKeys).to.deep.equal([]) + expect(strategy.paginationMode).to.equal('keyset') + expect(strategy.cursorKeys).to.deep.equal(['mediaProgresses.createdAt', 'id']) }) - it('falls back to offset pagination for progress finished-at browse because null-aware keyset is unsupported', () => { + it('uses keyset pagination for supported progress finished-at browse', () => { const strategy = getLibraryBrowseStrategy({ mediaType: 'book', sortBy: 'progress.finishedAt', filterGroup: 'progress', + filterValue: 'finished', + pageMode: 'endless' + }) + + expect(strategy.family).to.equal('progress-browse') + expect(strategy.paginationMode).to.equal('keyset') + expect(strategy.cursorKeys).to.deep.equal(['mediaProgresses.finishedAt', 'id']) + }) + + it('falls back to offset pagination for unsupported progress/null browse combinations', () => { + const strategy = getLibraryBrowseStrategy({ + mediaType: 'book', + sortBy: 'progress.finishedAt', + filterGroup: 'progress', + filterValue: 'in-progress', pageMode: 'endless' }) @@ -205,6 +222,8 @@ describe('libraryBrowseStrategy', () => { }) expect(strategy.family).to.equal('podcast-browse') + expect(strategy.paginationMode).to.equal('offset') + expect(strategy.deepScrollAllowed).to.equal(false) }) it('falls back to offset for unsupported endless-scroll sorts', () => {