From 6cbf8f5c3516a2f3a78bee29422b34d5e1db8cd9 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Fri, 18 Sep 2026 15:58:18 +0000 Subject: [PATCH] fix(library): preserve folder creation order Keep unrelated folder selections in the order users add them, and make API reads deterministic so web and iOS defaults use the same first folder. Fixes #1412 --- .../__tests__/folder-paths.spec.ts | 4 ++ .../library/composables/folder-paths.ts | 4 +- .../library/library.repository.test.ts | 39 ++++++++++++++++++- .../src/modules/library/library.repository.ts | 15 ++++--- .../modules/library/library.service.test.ts | 26 +++++++++---- server/src/modules/library/library.service.ts | 10 +++-- 6 files changed, 78 insertions(+), 20 deletions(-) diff --git a/client/src/features/library/composables/__tests__/folder-paths.spec.ts b/client/src/features/library/composables/__tests__/folder-paths.spec.ts index b7e2e915f..d3206dd57 100644 --- a/client/src/features/library/composables/__tests__/folder-paths.spec.ts +++ b/client/src/features/library/composables/__tests__/folder-paths.spec.ts @@ -11,6 +11,10 @@ describe('folder path selection', () => { expect(consolidateFolderPaths(['/books/fiction', '/audio', '/books', '/books/fantasy'])).toEqual(['/audio', '/books']) }) + it('preserves the order of unrelated folders', () => { + expect(consolidateFolderPaths(['/library/zz', '/library/alpha-long', '/library/m'])).toEqual(['/library/zz', '/library/alpha-long', '/library/m']) + }) + it('finds the selected path that already covers a nested folder', () => { expect(coveringFolderPath('/books/fiction/scifi', ['/books', '/books/fiction'])).toBe('/books/fiction') }) diff --git a/client/src/features/library/composables/folder-paths.ts b/client/src/features/library/composables/folder-paths.ts index 31b6b1b08..56eed7e51 100644 --- a/client/src/features/library/composables/folder-paths.ts +++ b/client/src/features/library/composables/folder-paths.ts @@ -15,8 +15,8 @@ export function coveringFolderPath(path: string, candidates: string[]): string | } export function consolidateFolderPaths(paths: string[]): string[] { - const unique = [...new Set(paths.map(normalizeFolderPath))].sort((a, b) => a.length - b.length || a.localeCompare(b)) - return unique.filter((path, index) => !unique.slice(0, index).some((candidate) => isPathInside(path, candidate))) + const unique = [...new Set(paths.map(normalizeFolderPath))] + return unique.filter((path) => !unique.some((candidate) => candidate !== path && isPathInside(path, candidate))) } function isPathInside(path: string, parent: string): boolean { diff --git a/server/src/modules/library/library.repository.test.ts b/server/src/modules/library/library.repository.test.ts index 14551a679..4e6266047 100644 --- a/server/src/modules/library/library.repository.test.ts +++ b/server/src/modules/library/library.repository.test.ts @@ -7,7 +7,7 @@ vi.mock('drizzle-orm', () => ({ sql: vi.fn((strings: TemplateStringsArray, ...values: unknown[]) => ({ op: 'sql', text: strings.join(''), values })), })); -import { books, libraries } from '../../db/schema'; +import { books, libraries, libraryFolders } from '../../db/schema'; import { LIBRARY_BOOK_STATUS_PRESENT } from './library.constants'; import { LibraryRepository } from './library.repository'; @@ -113,6 +113,43 @@ describe('LibraryRepository', () => { expect(orderBy).toHaveBeenCalledWith(libraries.id); }); + it('orders folders by creation time with an id tie-breaker', async () => { + const byLibraryOrderBy = vi.fn().mockResolvedValue([]); + const byLibraryWhere = vi.fn().mockReturnValue({ orderBy: byLibraryOrderBy }); + const byLibraryFrom = vi.fn().mockReturnValue({ where: byLibraryWhere }); + db.select.mockReturnValueOnce({ from: byLibraryFrom }); + + await repo.findFoldersByLibrary(7); + + expect(byLibraryOrderBy).toHaveBeenCalledWith(libraryFolders.createdAt, libraryFolders.id); + + const allOrderBy = vi.fn().mockResolvedValue([]); + const allFrom = vi.fn().mockReturnValue({ orderBy: allOrderBy }); + db.select.mockReturnValueOnce({ from: allFrom }); + + await repo.findAllFolders(); + + expect(allOrderBy).toHaveBeenCalledWith(libraryFolders.libraryId, libraryFolders.createdAt, libraryFolders.id); + + const scopedOrderBy = vi.fn().mockResolvedValue([]); + const scopedWhere = vi.fn().mockReturnValue({ orderBy: scopedOrderBy }); + const scopedFrom = vi.fn().mockReturnValue({ where: scopedWhere }); + db.select.mockReturnValueOnce({ from: scopedFrom }); + + await repo.findFoldersByLibraryIds([7, 8]); + + expect(scopedOrderBy).toHaveBeenCalledWith(libraryFolders.libraryId, libraryFolders.createdAt, libraryFolders.id); + + const pathsOrderBy = vi.fn().mockResolvedValue([]); + const pathsInnerJoin = vi.fn().mockReturnValue({ orderBy: pathsOrderBy }); + const pathsFrom = vi.fn().mockReturnValue({ innerJoin: pathsInnerJoin }); + db.select.mockReturnValueOnce({ from: pathsFrom }); + + await repo.findAllFolderPaths(); + + expect(pathsOrderBy).toHaveBeenCalledWith(libraries.displayOrder, libraries.name, libraryFolders.createdAt, libraryFolders.id); + }); + it('getStats aggregates counts, sizes, and format map', async () => { const countChain = { from: vi.fn().mockReturnValue({ diff --git a/server/src/modules/library/library.repository.ts b/server/src/modules/library/library.repository.ts index 075b8c580..e98c092cd 100644 --- a/server/src/modules/library/library.repository.ts +++ b/server/src/modules/library/library.repository.ts @@ -89,23 +89,28 @@ export class LibraryRepository { } findFoldersByLibrary(libraryId: number) { - return this.db.select().from(libraryFolders).where(eq(libraryFolders.libraryId, libraryId)); + return this.db.select().from(libraryFolders).where(eq(libraryFolders.libraryId, libraryId)).orderBy(libraryFolders.createdAt, libraryFolders.id); } findAllFolders() { - return this.db.select().from(libraryFolders); + return this.db.select().from(libraryFolders).orderBy(libraryFolders.libraryId, libraryFolders.createdAt, libraryFolders.id); } findFoldersByLibraryIds(libraryIds: number[]) { if (libraryIds.length === 0) return Promise.resolve([]); - return this.db.select().from(libraryFolders).where(inArray(libraryFolders.libraryId, libraryIds)); + return this.db + .select() + .from(libraryFolders) + .where(inArray(libraryFolders.libraryId, libraryIds)) + .orderBy(libraryFolders.libraryId, libraryFolders.createdAt, libraryFolders.id); } findAllFolderPaths() { return this.db .select({ libraryId: libraryFolders.libraryId, path: libraryFolders.path, libraryName: libraries.name }) .from(libraryFolders) - .innerJoin(libraries, eq(libraries.id, libraryFolders.libraryId)); + .innerJoin(libraries, eq(libraries.id, libraryFolders.libraryId)) + .orderBy(libraries.displayOrder, libraries.name, libraryFolders.createdAt, libraryFolders.id); } insert(data: typeof libraries.$inferInsert) { @@ -120,7 +125,7 @@ export class LibraryRepository { .returning(); } - insertFolder(data: typeof libraryFolders.$inferInsert) { + insertFolders(data: (typeof libraryFolders.$inferInsert)[]) { return this.db.insert(libraryFolders).values(data).returning(); } diff --git a/server/src/modules/library/library.service.test.ts b/server/src/modules/library/library.service.test.ts index 41a9b0cb5..2e4bf6f7b 100644 --- a/server/src/modules/library/library.service.test.ts +++ b/server/src/modules/library/library.service.test.ts @@ -42,7 +42,7 @@ describe('LibraryService', () => { findFoldersByLibrary: vi.fn(), findByName: vi.fn(), insert: vi.fn(), - insertFolder: vi.fn(), + insertFolders: vi.fn(), update: vi.fn(), deleteFolder: vi.fn(), findBookIdsByLibrary: vi.fn(), @@ -169,7 +169,10 @@ describe('LibraryService', () => { it('create applies defaults, inserts folders, and starts an async scan', async () => { libraryRepo.findByName.mockResolvedValue([]); libraryRepo.insert.mockResolvedValue([{ id: 5, name: 'Sci-Fi', icon: 'BookOpen' }]); - libraryRepo.insertFolder.mockResolvedValueOnce([{ id: 11, path: '/a' }]).mockResolvedValueOnce([{ id: 12, path: '/b' }]); + libraryRepo.insertFolders.mockResolvedValue([ + { id: 11, path: '/a' }, + { id: 12, path: '/b' }, + ]); const result = await service.create({ name: 'Sci-Fi', icon: 'BookOpen', folders: ['/a', '/b'] } as any); @@ -187,6 +190,10 @@ describe('LibraryService', () => { ); expect(scannerService.startScanAsync).toHaveBeenCalledWith(5); expect(fileWatcherService.startWatcher).not.toHaveBeenCalled(); + expect(libraryRepo.insertFolders).toHaveBeenCalledWith([ + { libraryId: 5, path: '/a' }, + { libraryId: 5, path: '/b' }, + ]); expect(result.folders).toEqual([ { id: 11, path: '/a' }, { id: 12, path: '/b' }, @@ -196,7 +203,7 @@ describe('LibraryService', () => { it('create passes file write defaults to insert', async () => { libraryRepo.findByName.mockResolvedValue([]); libraryRepo.insert.mockResolvedValue([{ id: 5, name: 'Sci-Fi', icon: 'BookOpen' }]); - libraryRepo.insertFolder.mockResolvedValueOnce([{ id: 11, path: '/a' }]); + libraryRepo.insertFolders.mockResolvedValue([{ id: 11, path: '/a' }]); await service.create({ name: 'Sci-Fi', icon: 'BookOpen', folders: ['/a'] } as any); @@ -220,7 +227,10 @@ describe('LibraryService', () => { it('create starts watcher immediately when watch is enabled', async () => { libraryRepo.findByName.mockResolvedValue([]); libraryRepo.insert.mockResolvedValue([{ id: 6, name: 'Watched', icon: 'BookOpen', watch: true }]); - libraryRepo.insertFolder.mockResolvedValueOnce([{ id: 21, path: '/watch-a' }]).mockResolvedValueOnce([{ id: 22, path: '/watch-b' }]); + libraryRepo.insertFolders.mockResolvedValue([ + { id: 21, path: '/watch-a' }, + { id: 22, path: '/watch-b' }, + ]); await service.create({ name: 'Watched', icon: 'BookOpen', folders: ['/watch-a', '/watch-b'], watch: true } as any); @@ -231,7 +241,7 @@ describe('LibraryService', () => { it('create registers the configured scan schedule', async () => { libraryRepo.findByName.mockResolvedValue([]); libraryRepo.insert.mockResolvedValue([{ id: 6, name: 'Scheduled', icon: 'BookOpen', watch: false }]); - libraryRepo.insertFolder.mockResolvedValueOnce([{ id: 21, path: '/scheduled' }]); + libraryRepo.insertFolders.mockResolvedValue([{ id: 21, path: '/scheduled' }]); await service.create({ name: 'Scheduled', @@ -256,7 +266,7 @@ describe('LibraryService', () => { await expect(service.create({ name: 'Sci-Fi', icon: 'BookOpen', folders: ['/outside'] } as any)).rejects.toBeInstanceOf(ForbiddenException); expect(libraryRepo.insert).not.toHaveBeenCalled(); - expect(libraryRepo.insertFolder).not.toHaveBeenCalled(); + expect(libraryRepo.insertFolders).not.toHaveBeenCalled(); }); it('create rejects missing icons', async () => { @@ -282,7 +292,7 @@ describe('LibraryService', () => { await service.update(3, { folders: ['/keep', '/add'] } as any); expect(libraryRepo.deleteFolder).toHaveBeenCalledWith(2); - expect(libraryRepo.insertFolder).toHaveBeenCalledWith({ libraryId: 3, path: '/add' }); + expect(libraryRepo.insertFolders).toHaveBeenCalledWith([{ libraryId: 3, path: '/add' }]); expect(fileWatcherService.startWatcher).not.toHaveBeenCalled(); expect(fileWatcherService.stopWatcher).not.toHaveBeenCalled(); }); @@ -294,7 +304,7 @@ describe('LibraryService', () => { await expect(service.update(3, { folders: ['/outside'] } as any)).rejects.toBeInstanceOf(ForbiddenException); expect(libraryRepo.update).not.toHaveBeenCalled(); - expect(libraryRepo.insertFolder).not.toHaveBeenCalled(); + expect(libraryRepo.insertFolders).not.toHaveBeenCalled(); expect(libraryRepo.deleteFolder).not.toHaveBeenCalled(); }); diff --git a/server/src/modules/library/library.service.ts b/server/src/modules/library/library.service.ts index 77689fddb..3dcb4d87e 100644 --- a/server/src/modules/library/library.service.ts +++ b/server/src/modules/library/library.service.ts @@ -161,19 +161,19 @@ export class LibraryService { fileRenameEnabled: dto.fileRenameEnabled ?? false, }); - const folders = await Promise.all(folderPaths.map((path) => this.libraryRepo.insertFolder({ libraryId: library.id, path }))); + const folders = await this.libraryRepo.insertFolders(folderPaths.map((path) => ({ libraryId: library.id, path }))); if (library.watch) { await this.fileWatcherService.startWatcher( library.id, - folders.map(([f]) => f.path), + folders.map((folder) => folder.path), ); } this.scanScheduler.syncSchedule(library.id, dto.autoScanCronExpression ?? null); this.scannerService.startScanAsync(library.id); - return { ...normalizeLibraryOrganizationMode(library), folders: folders.map(([f]) => f) }; + return { ...normalizeLibraryOrganizationMode(library), folders }; } async update(id: number, dto: UpdateLibraryDto) { @@ -213,7 +213,9 @@ export class LibraryService { await Promise.all(toRemove.map((f) => this.libraryRepo.deleteFolder(f.id))); const toAdd = folderPaths.filter((p) => !existingByPath.has(p)); - await Promise.all(toAdd.map((path) => this.libraryRepo.insertFolder({ libraryId: id, path }))); + if (toAdd.length > 0) { + await this.libraryRepo.insertFolders(toAdd.map((path) => ({ libraryId: id, path }))); + } } const folders = await this.libraryRepo.findFoldersByLibrary(id);