From 65f4fa7fcac1b23c3c1b3a1e74e7bf8108bbf9a8 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Wed, 17 Jun 2026 22:14:49 +0000 Subject: [PATCH] fix(library): only process books marked for rename in bulk rename The bulk rename job was incorrectly iterating over every book in the library instead of restricting the operation to books that actually require a rename. This caused unnecessary processing and logged excessive noise. We now compute the preview first and filter the list of candidate book IDs down to those with the 'will_rename' status before executing the rename loop. Closes #361 --- .../utils/achievementToast.test.ts | 4 +- client/src/features/author/api/author.test.ts | 12 +- .../book/lib/book-card-mapper.test.ts | 6 +- .../composables/useNotifications.test.ts | 24 +-- .../composables/useReaderProgress.test.ts | 2 +- .../composables/useReaderSettings.test.ts | 6 +- .../composables/useLiveScanBooks.test.ts | 14 +- .../statistics/api/statistics.api.test.ts | 10 +- .../features/tools/api/entity-manager.test.ts | 2 +- .../composables/useEntityManager.test.ts | 27 ++- client/tsconfig.app.json | 2 +- client/tsconfig.vitest.json | 7 +- .../file-write/bulk-rename.repository.test.ts | 184 ++++++++++++++++++ .../file-write/bulk-rename.repository.ts | 10 +- .../library/bulk-rename.service.test.ts | 123 ++++++++++-- .../modules/library/bulk-rename.service.ts | 7 +- 16 files changed, 351 insertions(+), 89 deletions(-) create mode 100644 server/src/modules/file-write/bulk-rename.repository.test.ts diff --git a/client/src/features/achievements/utils/achievementToast.test.ts b/client/src/features/achievements/utils/achievementToast.test.ts index 2780e8ed2..eb8286a52 100644 --- a/client/src/features/achievements/utils/achievementToast.test.ts +++ b/client/src/features/achievements/utils/achievementToast.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' -vi.mock('vue-sonner', () => ({ toast: { success: vi.fn() } })) -vi.mock('canvas-confetti', () => ({ default: vi.fn() })) +vi.mock('vue-sonner', () => ({ toast: { success: vi.fn<(...args: unknown[]) => unknown>() } })) +vi.mock('canvas-confetti', () => ({ default: vi.fn<(...args: unknown[]) => unknown>() })) import { toast } from 'vue-sonner' import confetti from 'canvas-confetti' diff --git a/client/src/features/author/api/author.test.ts b/client/src/features/author/api/author.test.ts index ba97bd9aa..a05288ca3 100644 --- a/client/src/features/author/api/author.test.ts +++ b/client/src/features/author/api/author.test.ts @@ -15,7 +15,7 @@ import { MAX_AUTHOR_IMAGE_BYTES, } from './author' -vi.mock('@/lib/api', () => ({ api: vi.fn() })) +vi.mock('@/lib/api', () => ({ api: vi.fn<(...args: unknown[]) => unknown>() })) const mockedApi = vi.mocked(api) @@ -30,7 +30,7 @@ function mockErrorResponse(status = 500) { function mockStreamResponse(lines: string[]) { let index = 0 const reader = { - read: vi.fn().mockImplementation(() => { + read: vi.fn<(...args: unknown[]) => unknown>().mockImplementation(() => { if (index < lines.length) { const value = new TextEncoder().encode(lines[index++] + '\n') return Promise.resolve({ done: false, value }) @@ -38,7 +38,7 @@ function mockStreamResponse(lines: string[]) { return Promise.resolve({ done: true, value: undefined }) }), } - return { ok: true, status: 200, body: { getReader: () => reader }, json: vi.fn() } as unknown as Response + return { ok: true, status: 200, body: { getReader: () => reader }, json: vi.fn<(...args: unknown[]) => unknown>() } as unknown as Response } beforeEach(() => { @@ -273,12 +273,12 @@ describe('fetchAuthorMetadataCandidates', () => { it('includes optional params in the URL', async () => { mockedApi.mockResolvedValue(mockOkResponse([])) - await fetchAuthorMetadataCandidates({ q: 'tolkien', region: 'en', limit: 5, providers: ['openlibrary', 'musicbrainz'] }) + await fetchAuthorMetadataCandidates({ q: 'tolkien', region: 'en', limit: 5, providers: ['audnexus'] }) const url = mockedApi.mock.calls[0][0] as string expect(url).toContain('region=en') expect(url).toContain('limit=5') - expect(url).toContain('providers=openlibrary%2Cmusicbrainz') + expect(url).toContain('providers=audnexus') }) it('throws on non-ok response', async () => { @@ -314,7 +314,7 @@ describe('bulkRefreshAuthorsMetadata', () => { ] mockedApi.mockResolvedValue(mockStreamResponse(lines)) - const onProgress = vi.fn() + const onProgress = vi.fn<(...args: unknown[]) => unknown>() await bulkRefreshAuthorsMetadata([10, 11], onProgress) expect(onProgress).toHaveBeenCalledTimes(2) diff --git a/client/src/features/book/lib/book-card-mapper.test.ts b/client/src/features/book/lib/book-card-mapper.test.ts index 9b5535a6a..19033d71f 100644 --- a/client/src/features/book/lib/book-card-mapper.test.ts +++ b/client/src/features/book/lib/book-card-mapper.test.ts @@ -166,8 +166,8 @@ describe('mergeBookCardWithDetail', () => { const detail = makeDetail({ audioMetadata: { narrators: [ - { id: 1, name: 'Narrator One' }, - { id: 2, name: 'Narrator Two' }, + { id: 1, name: 'Narrator One', sortName: null, displayOrder: 0 }, + { id: 2, name: 'Narrator Two', sortName: null, displayOrder: 1 }, ], durationSeconds: 3600, abridged: false, @@ -187,7 +187,7 @@ describe('mergeBookCardWithDetail', () => { it('returns empty narrators array when audioMetadata is undefined', () => { const detail = makeDetail() - ;(detail as BookDetail & { audioMetadata: undefined }).audioMetadata = undefined as unknown as null + ;(detail as { audioMetadata: unknown }).audioMetadata = undefined const result = mergeBookCardWithDetail(makeBook(), detail) expect(result.narrators).toEqual([]) }) diff --git a/client/src/features/notifications/composables/useNotifications.test.ts b/client/src/features/notifications/composables/useNotifications.test.ts index 5e84bf92f..ea4d5105f 100644 --- a/client/src/features/notifications/composables/useNotifications.test.ts +++ b/client/src/features/notifications/composables/useNotifications.test.ts @@ -2,19 +2,19 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import type { NotificationItem } from '@bookorbit/types' import { NotificationType } from '@bookorbit/types' -const mockApi = vi.fn() -const mockGetAccessToken = vi.fn().mockReturnValue('token') +const mockApi = vi.fn<(...args: unknown[]) => unknown>() +const mockGetAccessToken = vi.fn<(...args: unknown[]) => unknown>().mockReturnValue('token') vi.mock('@/lib/api', () => ({ api: (...args: unknown[]) => mockApi(...args), getAccessToken: () => mockGetAccessToken(), })) -const mockIo = vi.fn() +const mockIo = vi.fn<(...args: unknown[]) => unknown>() vi.mock('socket.io-client', () => ({ io: (...args: unknown[]) => mockIo(...args), })) -const mockShowAchievementToast = vi.fn() +const mockShowAchievementToast = vi.fn<(...args: unknown[]) => unknown>() vi.mock('@/features/achievements/utils/achievementToast', () => ({ showAchievementToast: (...args: unknown[]) => mockShowAchievementToast(...args), })) @@ -43,10 +43,10 @@ function makeNotification(overrides: Partial = {}): Notificati function makeMockSocket() { return { - on: vi.fn(), - off: vi.fn(), - disconnect: vi.fn(), - emit: vi.fn(), + on: vi.fn<(...args: unknown[]) => unknown>(), + off: vi.fn<(...args: unknown[]) => unknown>(), + disconnect: vi.fn<(...args: unknown[]) => unknown>(), + emit: vi.fn<(...args: unknown[]) => unknown>(), connected: false, } } @@ -91,7 +91,7 @@ describe('useNotifications', () => { const { subscribe } = useNotifications() subscribe() - const notificationNewHandler = mockSocket.on.mock.calls.find(([event]: [string]) => event === 'notification:new')?.[1] as + const notificationNewHandler = mockSocket.on.mock.calls.find(([event]: unknown[]) => event === 'notification:new')?.[1] as | ((item: NotificationItem) => void) | undefined @@ -116,7 +116,7 @@ describe('useNotifications', () => { const { subscribe } = useNotifications() subscribe() - const notificationNewHandler = mockSocket.on.mock.calls.find(([event]: [string]) => event === 'notification:new')?.[1] as ( + const notificationNewHandler = mockSocket.on.mock.calls.find(([event]: unknown[]) => event === 'notification:new')?.[1] as ( item: NotificationItem, ) => void @@ -137,7 +137,7 @@ describe('useNotifications', () => { const { subscribe } = useNotifications() subscribe() - const notificationNewHandler = mockSocket.on.mock.calls.find(([event]: [string]) => event === 'notification:new')?.[1] as ( + const notificationNewHandler = mockSocket.on.mock.calls.find(([event]: unknown[]) => event === 'notification:new')?.[1] as ( item: NotificationItem, ) => void @@ -161,7 +161,7 @@ describe('useNotifications', () => { const useNotifications = await loadModule() const { subscribe } = useNotifications() subscribe() - return mockSocket.on.mock.calls.find(([event]: [string]) => event === 'notification:new')?.[1] as (item: NotificationItem) => void + return mockSocket.on.mock.calls.find(([event]: unknown[]) => event === 'notification:new')?.[1] as (item: NotificationItem) => void } it('uses message as name when present', async () => { diff --git a/client/src/features/reader/shared/composables/useReaderProgress.test.ts b/client/src/features/reader/shared/composables/useReaderProgress.test.ts index 8821df301..50d1193f6 100644 --- a/client/src/features/reader/shared/composables/useReaderProgress.test.ts +++ b/client/src/features/reader/shared/composables/useReaderProgress.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import { ref } from 'vue' -vi.mock('@/lib/api', () => ({ api: vi.fn() })) +vi.mock('@/lib/api', () => ({ api: vi.fn<(...args: unknown[]) => unknown>() })) import { api } from '@/lib/api' import { formatTimeRemaining, useReaderProgress } from './useReaderProgress' diff --git a/client/src/features/reader/shared/composables/useReaderSettings.test.ts b/client/src/features/reader/shared/composables/useReaderSettings.test.ts index c2daf82ab..9affb7445 100644 --- a/client/src/features/reader/shared/composables/useReaderSettings.test.ts +++ b/client/src/features/reader/shared/composables/useReaderSettings.test.ts @@ -2,13 +2,13 @@ import { describe, it, expect, vi, beforeEach } from 'vitest' import { ref } from 'vue' import { CBX_READER_DEFAULTS, READER_GROUP_DEFAULTS } from '@bookorbit/types' -const apiMock = vi.hoisted(() => vi.fn()) +const apiMock = vi.hoisted(() => vi.fn<(...args: unknown[]) => Promise>()) vi.mock('@/lib/api', () => ({ api: apiMock })) -const toastMock = vi.hoisted(() => ({ success: vi.fn() })) +const toastMock = vi.hoisted(() => ({ success: vi.fn<(...args: unknown[]) => unknown>() })) vi.mock('vue-sonner', () => ({ toast: toastMock })) -const useAuthMock = vi.hoisted(() => vi.fn(() => ({ user: ref(null) }))) +const useAuthMock = vi.hoisted(() => vi.fn<(...args: unknown[]) => unknown>(() => ({ user: ref(null) }))) vi.mock('@/features/auth/composables/useAuth', () => ({ useAuth: useAuthMock })) import { useReaderSettings, useReaderDefaultSettings } from './useReaderSettings' diff --git a/client/src/features/scanner/composables/useLiveScanBooks.test.ts b/client/src/features/scanner/composables/useLiveScanBooks.test.ts index 5be0e3f8e..f90bf2aa3 100644 --- a/client/src/features/scanner/composables/useLiveScanBooks.test.ts +++ b/client/src/features/scanner/composables/useLiveScanBooks.test.ts @@ -3,12 +3,12 @@ import { ref } from 'vue' import type { BookCard, ScanBooksAddedEvent } from '@bookorbit/types' const mockSocket = { - on: vi.fn(), - off: vi.fn(), + on: vi.fn<(...args: unknown[]) => unknown>(), + off: vi.fn<(...args: unknown[]) => unknown>(), } vi.mock('./useScanProgress', () => ({ - getSocket: vi.fn(() => mockSocket), + getSocket: vi.fn<(...args: unknown[]) => unknown>(() => mockSocket), })) import { useLiveScanBooks } from './useLiveScanBooks' @@ -36,7 +36,7 @@ function makeBook(overrides: Partial = {}): BookCard { } function captureHandler(): (event: ScanBooksAddedEvent) => void { - const call = mockSocket.on.mock.calls.find(([event]: [string]) => event === 'scan:books:added') + const call = mockSocket.on.mock.calls.find(([event]: unknown[]) => event === 'scan:books:added') const handler = call?.[1] as ((event: ScanBooksAddedEvent) => void) | undefined if (!handler) throw new Error('scan:books:added handler not registered') return handler @@ -46,7 +46,7 @@ describe('useLiveScanBooks', () => { beforeEach(() => { vi.useFakeTimers() vi.clearAllMocks() - mockGetSocket.mockReturnValue(mockSocket as ReturnType) + mockGetSocket.mockReturnValue(mockSocket as unknown as ReturnType) }) afterEach(() => { @@ -178,7 +178,7 @@ describe('useLiveScanBooks', () => { expect(existingBooks.value).toHaveLength(1) vi.clearAllMocks() - mockGetSocket.mockReturnValue(mockSocket as ReturnType) + mockGetSocket.mockReturnValue(mockSocket as unknown as ReturnType) const { start: start2, newBookIds: newBookIds2 } = useLiveScanBooks(libraryId, existingBooks, total) start2() @@ -213,7 +213,7 @@ describe('useLiveScanBooks', () => { start() start() - const calls = mockSocket.on.mock.calls.filter(([event]: [string]) => event === 'scan:books:added') + const calls = mockSocket.on.mock.calls.filter(([event]: unknown[]) => event === 'scan:books:added') expect(calls).toHaveLength(1) }) diff --git a/client/src/features/statistics/api/statistics.api.test.ts b/client/src/features/statistics/api/statistics.api.test.ts index 3196c919f..55e8708f9 100644 --- a/client/src/features/statistics/api/statistics.api.test.ts +++ b/client/src/features/statistics/api/statistics.api.test.ts @@ -37,7 +37,7 @@ import { } from './statistics.api' import type { StatisticsFilterConfig } from '@bookorbit/types' -vi.mock('@/lib/api', () => ({ api: vi.fn() })) +vi.mock('@/lib/api', () => ({ api: vi.fn<(...args: unknown[]) => unknown>() })) const mockedApi = vi.mocked(api) @@ -51,8 +51,8 @@ function mockErrorResponse(status = 500) { const filters: StatisticsFilterConfig = { libraryIds: [1, 2], - booksOverTimeGranularity: 'month', - booksOverTimeRange: '1y', + booksOverTimeGranularity: 'monthly', + booksOverTimeRange: 'last-year', } beforeEach(() => { @@ -111,8 +111,8 @@ describe('fetchBooksAddedOverTime', () => { const url = mockedApi.mock.calls[0][0] as string expect(url).toContain('/api/v1/statistics/books-added-over-time') - expect(url).toContain('granularity=month') - expect(url).toContain('range=1y') + expect(url).toContain('granularity=monthly') + expect(url).toContain('range=last-year') expect(result).toEqual(data) }) diff --git a/client/src/features/tools/api/entity-manager.test.ts b/client/src/features/tools/api/entity-manager.test.ts index 0d311de55..33a4b6474 100644 --- a/client/src/features/tools/api/entity-manager.test.ts +++ b/client/src/features/tools/api/entity-manager.test.ts @@ -16,7 +16,7 @@ import { getEntityInfo, } from './entity-manager' -vi.mock('@/lib/api', () => ({ api: vi.fn() })) +vi.mock('@/lib/api', () => ({ api: vi.fn<(...args: unknown[]) => unknown>() })) const mockedApi = vi.mocked(api) diff --git a/client/src/features/tools/composables/useEntityManager.test.ts b/client/src/features/tools/composables/useEntityManager.test.ts index 91360dd0b..076c644ac 100644 --- a/client/src/features/tools/composables/useEntityManager.test.ts +++ b/client/src/features/tools/composables/useEntityManager.test.ts @@ -2,19 +2,19 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import type { BrowseEntityItem, DuplicateCluster, DuplicateScanStatus, DismissedPairInfo } from '@bookorbit/types' vi.mock('../api/entity-manager', () => ({ - getDuplicateScanStatus: vi.fn(), - refreshDuplicates: vi.fn(), - browseEntities: vi.fn(), - mergeEntities: vi.fn(), - renameEntity: vi.fn(), - deleteEntity: vi.fn(), - bulkDeleteEntities: vi.fn(), - splitEntity: vi.fn(), - scanDuplicates: vi.fn(), - dismissPair: vi.fn(), - undismissPair: vi.fn(), - getDismissedPairs: vi.fn(), - getEntityInfo: vi.fn(), + getDuplicateScanStatus: vi.fn<(...args: unknown[]) => unknown>(), + refreshDuplicates: vi.fn<(...args: unknown[]) => unknown>(), + browseEntities: vi.fn<(...args: unknown[]) => unknown>(), + mergeEntities: vi.fn<(...args: unknown[]) => unknown>(), + renameEntity: vi.fn<(...args: unknown[]) => unknown>(), + deleteEntity: vi.fn<(...args: unknown[]) => unknown>(), + bulkDeleteEntities: vi.fn<(...args: unknown[]) => unknown>(), + splitEntity: vi.fn<(...args: unknown[]) => unknown>(), + scanDuplicates: vi.fn<(...args: unknown[]) => unknown>(), + dismissPair: vi.fn<(...args: unknown[]) => unknown>(), + undismissPair: vi.fn<(...args: unknown[]) => unknown>(), + getDismissedPairs: vi.fn<(...args: unknown[]) => unknown>(), + getEntityInfo: vi.fn<(...args: unknown[]) => unknown>(), })) import * as entityManagerApi from '../api/entity-manager' @@ -113,7 +113,6 @@ describe('useEntityManager', () => { }) it('triggers scan when status transitions from computing to done', async () => { - const { scanDuplicates: mockScan } = entityManagerApi mockGetDuplicateScanStatus.mockResolvedValueOnce(makeStatus('computing')) vi.mocked(entityManagerApi.scanDuplicates).mockResolvedValue({ clusters: [], diff --git a/client/tsconfig.app.json b/client/tsconfig.app.json index c0f2d8670..fefc10868 100644 --- a/client/tsconfig.app.json +++ b/client/tsconfig.app.json @@ -1,7 +1,7 @@ { "extends": "@vue/tsconfig/tsconfig.dom.json", "include": ["env.d.ts", "src/**/*", "src/**/*.vue"], - "exclude": ["src/**/__tests__/*"], + "exclude": ["src/**/__tests__/*", "src/**/*.test.ts", "src/**/*.spec.ts"], "compilerOptions": { // Extra safety for array and object lookups, but may have false positives. "noUncheckedIndexedAccess": true, diff --git a/client/tsconfig.vitest.json b/client/tsconfig.vitest.json index 9470627dc..f7e8faa4e 100644 --- a/client/tsconfig.vitest.json +++ b/client/tsconfig.vitest.json @@ -3,13 +3,14 @@ // Override to include only test files and clear exclusions. // Application code imported in tests is automatically included via module resolution. - "include": ["src/**/__tests__/*", "env.d.ts"], + "include": ["src/**/__tests__/*", "src/**/*.test.ts", "src/**/*.spec.ts", "env.d.ts"], "exclude": [], "compilerOptions": { // Vitest runs in a different environment than the application code. - // Adjust lib and types accordingly. - "lib": [], + // Adjust lib and types accordingly. ES2022 matches the app's target so + // newer built-ins (e.g. Array.prototype.at) are available in tests. + "lib": ["ES2022"], "types": ["node", "jsdom"], // noUncheckedIndexedAccess produces false positives in tests where the diff --git a/server/src/modules/file-write/bulk-rename.repository.test.ts b/server/src/modules/file-write/bulk-rename.repository.test.ts new file mode 100644 index 000000000..a60fc828e --- /dev/null +++ b/server/src/modules/file-write/bulk-rename.repository.test.ts @@ -0,0 +1,184 @@ +import { drizzle } from 'drizzle-orm/node-postgres'; + +import * as schema from '../../db/schema'; +import { BulkRenameRepository } from './bulk-rename.repository'; + +describe('BulkRenameRepository', () => { + function queryChain(rows: unknown) { + return { + from: vi.fn().mockReturnThis(), + innerJoin: vi.fn().mockReturnThis(), + where: vi.fn().mockReturnThis(), + orderBy: vi.fn().mockResolvedValue(rows), + }; + } + + function subqueryChain() { + return { + from: vi.fn().mockReturnThis(), + where: vi.fn().mockReturnThis(), + }; + } + + describe('findAllBooksForLibrary', () => { + it('maps book rows and groups authors by book', async () => { + const bookRows = [ + { + bookId: 1, + absolutePath: '/lib/old/a.epub', + relPath: 'old/a.epub', + format: 'epub', + libraryFolderPath: '/lib', + organizationMode: 'book_per_file', + fileNamingPattern: '{title}', + bookFolderPath: '/lib/old/a.epub', + title: 'A', + subtitle: null, + publisher: null, + language: 'en', + isbn13: null, + publishedYear: 2001, + seriesName: null, + seriesIndex: null, + }, + { + bookId: 2, + absolutePath: '/lib/old/b.epub', + relPath: 'old/b.epub', + format: 'epub', + libraryFolderPath: '/lib', + organizationMode: 'book_per_file', + fileNamingPattern: '{title}', + bookFolderPath: '/lib/old/b.epub', + title: 'B', + subtitle: null, + publisher: null, + language: null, + isbn13: null, + publishedYear: null, + seriesName: null, + seriesIndex: null, + }, + ]; + const authorRows = [ + { bookId: 1, name: 'Author One' }, + { bookId: 1, name: 'Author Two' }, + { bookId: 2, name: 'Author Three' }, + ]; + + const db = { + select: vi.fn().mockReturnValueOnce(queryChain(bookRows)).mockReturnValueOnce(queryChain(authorRows)).mockReturnValue(subqueryChain()), + }; + + const repo = new BulkRenameRepository(db as never); + const result = await repo.findAllBooksForLibrary(7); + + expect(result).toHaveLength(2); + expect(result[0]).toMatchObject({ + bookId: 1, + title: 'A', + absolutePath: '/lib/old/a.epub', + authors: ['Author One', 'Author Two'], + metadata: { language: 'en', publishedYear: 2001 }, + }); + expect(result[1]).toMatchObject({ bookId: 2, authors: ['Author Three'] }); + }); + + it('returns books with empty author lists when none are linked', async () => { + const bookRows = [ + { + bookId: 1, + absolutePath: '/lib/old/a.epub', + relPath: 'old/a.epub', + format: 'epub', + libraryFolderPath: '/lib', + organizationMode: 'book_per_file', + fileNamingPattern: '{title}', + bookFolderPath: '/lib/old/a.epub', + title: 'A', + subtitle: null, + publisher: null, + language: null, + isbn13: null, + publishedYear: null, + seriesName: null, + seriesIndex: null, + }, + ]; + + const db = { + select: vi.fn().mockReturnValueOnce(queryChain(bookRows)).mockReturnValueOnce(queryChain([])).mockReturnValue(subqueryChain()), + }; + + const repo = new BulkRenameRepository(db as never); + const result = await repo.findAllBooksForLibrary(7); + + expect(result).toHaveLength(1); + expect(result[0].authors).toEqual([]); + }); + + it('returns an empty array and skips the author query when the library has no books', async () => { + const db = { select: vi.fn().mockReturnValueOnce(queryChain([])) }; + + const repo = new BulkRenameRepository(db as never); + const result = await repo.findAllBooksForLibrary(7); + + expect(result).toEqual([]); + expect(db.select).toHaveBeenCalledTimes(1); + }); + + it('fetches authors via a library subquery, never a per-book parameter list (issue #361)', async () => { + const calls: Array<{ text: string; params: unknown[] }> = []; + const fakeClient = { + query: vi.fn().mockImplementation((cfg: { text: string }, params: unknown[]) => { + calls.push({ text: cfg.text, params }); + // The first (books) query must return at least one row so the author query runs. + return Promise.resolve({ rows: calls.length === 1 ? [new Array(16).fill(null)] : [] }); + }), + }; + + const db = drizzle({ client: fakeClient as never, schema }); + const repo = new BulkRenameRepository(db as never); + + await repo.findAllBooksForLibrary(42); + + expect(calls).toHaveLength(2); + + const authorQuery = calls[1]; + // The ONLY bind parameter is the libraryId - not one entry per book - so the statement can + // never exceed PostgreSQL's 65535-parameter wire-protocol limit, regardless of library size. + expect(authorQuery.params).toEqual([42]); + + const sqlText = authorQuery.text.toLowerCase(); + expect(sqlText).toContain('in (select'); + expect(sqlText).toContain('"library_id"'); + }); + }); + + describe('findLibrarySettings', () => { + function settingsChain(rows: unknown) { + return { + from: vi.fn().mockReturnThis(), + where: vi.fn().mockReturnThis(), + limit: vi.fn().mockResolvedValue(rows), + }; + } + + it('returns the settings row when the library exists', async () => { + const row = { fileRenameEnabled: true, fileNamingPattern: '{title}', organizationMode: 'book_per_file', watch: false }; + const db = { select: vi.fn().mockReturnValue(settingsChain([row])) }; + + const repo = new BulkRenameRepository(db as never); + + await expect(repo.findLibrarySettings(1)).resolves.toEqual(row); + }); + + it('returns null when the library is missing', async () => { + const db = { select: vi.fn().mockReturnValue(settingsChain([])) }; + + const repo = new BulkRenameRepository(db as never); + + await expect(repo.findLibrarySettings(999)).resolves.toBeNull(); + }); + }); +}); diff --git a/server/src/modules/file-write/bulk-rename.repository.ts b/server/src/modules/file-write/bulk-rename.repository.ts index eb6f5e118..1f3c54f2e 100644 --- a/server/src/modules/file-write/bulk-rename.repository.ts +++ b/server/src/modules/file-write/bulk-rename.repository.ts @@ -63,8 +63,7 @@ export class BulkRenameRepository { .where(eq(books.libraryId, libraryId)) .orderBy(asc(books.id)); - const bookIds = rows.map((r) => r.bookId); - if (bookIds.length === 0) return []; + if (rows.length === 0) return []; const authorRows = await this.db .select({ @@ -73,7 +72,7 @@ export class BulkRenameRepository { }) .from(bookAuthors) .innerJoin(authors, eq(authors.id, bookAuthors.authorId)) - .where(inArray(bookAuthors.bookId, bookIds)) + .where(inArray(bookAuthors.bookId, this.db.select({ id: books.id }).from(books).where(eq(books.libraryId, libraryId)))) .orderBy(asc(bookAuthors.bookId), asc(bookAuthors.displayOrder)); const authorsByBook = new Map(); @@ -129,9 +128,4 @@ export class BulkRenameRepository { return row ?? null; } - - async findLibraryBookIds(libraryId: number): Promise { - const rows = await this.db.select({ id: books.id }).from(books).where(eq(books.libraryId, libraryId)).orderBy(asc(books.id)); - return rows.map((r) => r.id); - } } diff --git a/server/src/modules/library/bulk-rename.service.test.ts b/server/src/modules/library/bulk-rename.service.test.ts index c5d3a4c76..1d8f8c54d 100644 --- a/server/src/modules/library/bulk-rename.service.test.ts +++ b/server/src/modules/library/bulk-rename.service.test.ts @@ -30,13 +30,19 @@ function makeBookData(overrides: Partial & { bookId?: number }; } +// Under the default '{title}' pattern, a book whose file sits in /library/folder/* resolves to +// /library/.* and therefore always lands in the will_rename bucket. Distinct titles keep +// each new path unique so they are never reclassified as collisions. +function willRenameBook(id: number): BulkRenameBookData { + return makeBookData({ bookId: id, title: `Book ${id}`, absolutePath: `/library/folder/book-${id}.epub` }); +} + describe('BulkRenameService', () => { let service: BulkRenameService; const bulkRenameRepo = { findAllBooksForLibrary: vi.fn(), findLibrarySettings: vi.fn(), - findLibraryBookIds: vi.fn(), }; const fileRenameRepo = { @@ -411,9 +417,9 @@ describe('BulkRenameService', () => { watch: false, }; - it('renames all books sequentially and reports summary', async () => { + it('renames only the will_rename candidates and reports summary', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1, 2, 3]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1), willRenameBook(2), willRenameBook(3)]); const results: FileRenameResult[] = [ { status: 'success', durationMs: 10 }, @@ -439,9 +445,63 @@ describe('BulkRenameService', () => { expect(fileRenameService.performRename).toHaveBeenCalledTimes(3); }); + it('only renames will_rename books, never the unchanged or collision ones', async () => { + bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); + + const willRename = willRenameBook(1); + // Already at its target path under the '{title}' pattern -> unchanged. + const unchanged = makeBookData({ bookId: 2, title: 'Settled', absolutePath: '/library/Settled.epub' }); + // Two books resolving to /library/Same.epub -> both collision. + const collideA = makeBookData({ bookId: 3, title: 'Same', absolutePath: '/library/folder/a.epub' }); + const collideB = makeBookData({ bookId: 4, title: 'Same', absolutePath: '/library/folder/b.epub' }); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRename, unchanged, collideA, collideB]); + fileRenameService.performRename.mockResolvedValue({ status: 'success', durationMs: 10 }); + + const events: any[] = []; + const summary = await service.execute(1, 42, { + onProgress: (e) => events.push(e), + isCancelled: () => false, + }); + + expect(fileRenameService.performRename).toHaveBeenCalledTimes(1); + expect(fileRenameService.performRename).toHaveBeenCalledWith(1, 42, false, true); + expect(events).toEqual([{ bookId: 1, status: 'success', reason: undefined }]); + expect(summary.processed).toBe(1); + expect(summary.succeeded).toBe(1); + }); + + it('suppresses per-book notifications during bulk rename', async () => { + bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); + fileRenameService.performRename.mockResolvedValue({ status: 'success', durationMs: 10 }); + + await service.execute(1, 42, { + onProgress: () => {}, + isCancelled: () => false, + }); + + // performRename(bookId, userId, force=false, suppressNotification=true) + expect(fileRenameService.performRename).toHaveBeenCalledWith(1, 42, false, true); + }); + + it('does nothing per-book when there are no will_rename candidates but still notifies completion', async () => { + bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); + const unchanged = makeBookData({ bookId: 1, title: 'Settled', absolutePath: '/library/Settled.epub' }); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([unchanged]); + + const summary = await service.execute(1, 42, { + onProgress: () => {}, + isCancelled: () => false, + }); + + expect(fileRenameService.performRename).not.toHaveBeenCalled(); + expect(summary).toMatchObject({ processed: 0, succeeded: 0, failed: 0, skipped: 0, cancelled: false }); + expect(notificationService.notify).toHaveBeenCalledWith(expect.objectContaining({ type: 'bulk_rename_completed' })); + }); + it('stops when cancelled and reports partial progress', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1, 2, 3]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1), willRenameBook(2), willRenameBook(3)]); let callCount = 0; fileRenameService.performRename.mockImplementation(() => { @@ -486,7 +546,7 @@ describe('BulkRenameService', () => { it('prevents concurrent execution on same library', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); let resolveRename: ((value: FileRenameResult) => void) | undefined; fileRenameService.performRename.mockImplementation( @@ -517,7 +577,7 @@ describe('BulkRenameService', () => { it('stops and restarts file watcher when library has watch enabled', async () => { const watchSettings = { ...defaultSettings, watch: true }; bulkRenameRepo.findLibrarySettings.mockResolvedValue(watchSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); fileRenameService.performRename.mockResolvedValue({ status: 'success', durationMs: 10 }); await service.execute(1, 42, { @@ -531,7 +591,7 @@ describe('BulkRenameService', () => { it('does not touch file watcher when library has watch disabled', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); fileRenameService.performRename.mockResolvedValue({ status: 'success', durationMs: 10 }); await service.execute(1, 42, { @@ -546,7 +606,7 @@ describe('BulkRenameService', () => { it('restarts file watcher even after failure', async () => { const watchSettings = { ...defaultSettings, watch: true }; bulkRenameRepo.findLibrarySettings.mockResolvedValue(watchSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); fileRenameService.performRename.mockRejectedValue(new Error('disk error')); await service.execute(1, 42, { @@ -560,7 +620,7 @@ describe('BulkRenameService', () => { it('counts failed renames from performRename', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1, 2]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1), willRenameBook(2)]); fileRenameService.performRename.mockResolvedValueOnce({ status: 'failed', reason: 'collision', durationMs: 5 }); fileRenameService.performRename.mockResolvedValueOnce({ status: 'success', durationMs: 10 }); @@ -579,7 +639,7 @@ describe('BulkRenameService', () => { it('handles thrown errors from performRename gracefully', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); fileRenameService.performRename.mockRejectedValue(new Error('disk error')); const events: any[] = []; @@ -593,9 +653,33 @@ describe('BulkRenameService', () => { expect(events[0].reason).toBe('disk error'); }); + it('notifies failure and rethrows when the run errors unexpectedly', async () => { + bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); + fileRenameService.performRename.mockResolvedValue({ status: 'success', durationMs: 10 }); + + await expect( + service.execute(1, 42, { + onProgress: () => {}, + isCancelled: () => { + throw new Error('stream broke'); + }, + }), + ).rejects.toThrow('stream broke'); + + expect(notificationService.notify).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'bulk_rename_failed', + title: 'Bulk rename failed', + }), + ); + // The running lock must be released even on an unexpected failure. + expect(service.isRunning(1)).toBe(false); + }); + it('sends success notification when all renames succeed', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); fileRenameService.performRename.mockResolvedValue({ status: 'success', durationMs: 10 }); await service.execute(1, 42, { @@ -613,7 +697,7 @@ describe('BulkRenameService', () => { it('sends failure notification when some renames fail', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); fileRenameService.performRename.mockResolvedValue({ status: 'failed', reason: 'collision', durationMs: 5 }); await service.execute(1, 42, { @@ -631,7 +715,7 @@ describe('BulkRenameService', () => { it('clears running lock after execution completes', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); fileRenameService.performRename.mockResolvedValue({ status: 'success', durationMs: 10 }); expect(service.isRunning(1)).toBe(false); @@ -647,23 +731,22 @@ describe('BulkRenameService', () => { it('invalidates preview cache on execute', async () => { bulkRenameRepo.findLibrarySettings.mockResolvedValue(defaultSettings); - - const book = makeBookData({ bookId: 1, title: 'CacheTest' }); - book.metadata.title = 'CacheTest'; - bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([book]); + bulkRenameRepo.findAllBooksForLibrary.mockResolvedValue([willRenameBook(1)]); + fileRenameService.performRename.mockResolvedValue({ status: 'success', durationMs: 10 }); await service.getPreview(1, 1, 50); expect(bulkRenameRepo.findAllBooksForLibrary).toHaveBeenCalledTimes(1); - bulkRenameRepo.findLibraryBookIds.mockResolvedValue([1]); - fileRenameService.performRename.mockResolvedValue({ status: 'success', durationMs: 10 }); + // execute() recomputes the preview internally to derive the will_rename set (call #2), + // and clears the cache so the following getPreview recomputes again (call #3). await service.execute(1, 42, { onProgress: () => {}, isCancelled: () => false, }); + expect(bulkRenameRepo.findAllBooksForLibrary).toHaveBeenCalledTimes(2); await service.getPreview(1, 1, 50); - expect(bulkRenameRepo.findAllBooksForLibrary).toHaveBeenCalledTimes(2); + expect(bulkRenameRepo.findAllBooksForLibrary).toHaveBeenCalledTimes(3); }); }); diff --git a/server/src/modules/library/bulk-rename.service.ts b/server/src/modules/library/bulk-rename.service.ts index 021a038df..68bc9f207 100644 --- a/server/src/modules/library/bulk-rename.service.ts +++ b/server/src/modules/library/bulk-rename.service.ts @@ -92,8 +92,9 @@ export class BulkRenameService { this.runningLibraries.add(libraryId); this.previewCache.delete(libraryId); - const bookIds = await this.bulkRenameRepo.findLibraryBookIds(libraryId); - this.logger.log(`[${event}] [start] libraryId=${libraryId} userId=${userId} bookCount=${bookIds.length} - bulk rename started`); + const preview = await this.computeFullPreview(libraryId); + const bookIds = preview.items.filter((item) => item.status === 'will_rename').map((item) => item.bookId); + this.logger.log(`[${event}] [start] libraryId=${libraryId} userId=${userId} candidateCount=${bookIds.length} - bulk rename started`); let watcherWasStopped = false; if (settings.watch) { @@ -110,7 +111,7 @@ export class BulkRenameService { if (options.isCancelled()) break; try { - const result = await this.fileRenameService.performRename(bookId, userId); + const result = await this.fileRenameService.performRename(bookId, userId, false, true); if (result.status === 'success') succeeded++; else if (result.status === 'failed') failed++;