From b9e5fe3a29ca8926162f961fed63ddf5f930af04 Mon Sep 17 00:00:00 2001 From: neon <18559658+neonsolstice@users.noreply.github.com> Date: Sat, 24 Oct 2026 21:06:09 +0000 Subject: [PATCH] fix(koreader): keep the configured file name on downloads Plugins from 1.7.0 request a delivery variant, and every such download had " - bo--" added to its name. Only copies that would share a path are renamed now: a narrated EPUB's text-only copy and later same-format files. A new chained default also stops the default pattern naming authorless standalone books "Standalone/ - ". See docs/READ_ALOUD_DELIVERY.md. Closes #1893 --- .../file-naming/lib/pattern-highlight.ts | 1 + docs/READ_ALOUD_DELIVERY.md | 2 + packages/types/src/pattern-resolver.ts | 30 +++-- .../koreader/koreader-catalog.service.test.ts | 119 ++++++++++++++++++ .../koreader/koreader-catalog.service.ts | 47 ++++++- .../modules/upload/pattern-resolver.test.ts | 58 ++++++++- server/test/read-aloud-delivery.e2e-spec.ts | 9 +- 7 files changed, 244 insertions(+), 22 deletions(-) diff --git a/client/src/features/settings/file-naming/lib/pattern-highlight.ts b/client/src/features/settings/file-naming/lib/pattern-highlight.ts index 9677d87fa..929f43f9b 100644 --- a/client/src/features/settings/file-naming/lib/pattern-highlight.ts +++ b/client/src/features/settings/file-naming/lib/pattern-highlight.ts @@ -7,6 +7,7 @@ * {token:modifier} a placeholder with a transform * <...> an optional group, dropped when a placeholder inside it is empty * <a|b> the same, with `b` used when `a` cannot be filled + * <a|b|c> `b` only when it fills, otherwise `c` * / a path separator */ export type PatternPieceKind = 'literal' | 'token' | 'modifier' | 'optional' | 'fallback' | 'separator' diff --git a/docs/READ_ALOUD_DELIVERY.md b/docs/READ_ALOUD_DELIVERY.md index 068f0f6ba..18d0d6b98 100644 --- a/docs/READ_ALOUD_DELIVERY.md +++ b/docs/READ_ALOUD_DELIVERY.md @@ -69,6 +69,8 @@ The `read-aloud-delivery` E2E suite covers bytes, ranges, authorization, indepen Set the credential default in Settings > KOReader > Sync Settings > KOReader EPUB delivery. A device can override it, or use the credential default. New credentials created in the supported web interface start with text-only delivery. Existing credentials inherit text-only until configured; the plugin imports an existing local choice once for that device if the server default has not been configured. Import never overwrites a saved device choice or an explicit credential default. +Downloads keep the device's file naming pattern for either variant. Only copies that would otherwise share a path are renamed: the text-only copy of a narrated EPUB adds ` - Text Only`, and each same-format file after a book's first adds its file id in brackets. + Plugin 1.7.0 reads the current server preference before a fresh acquisition. Download > Settings > Read-aloud EPUB delivery edits the same server device setting. Single-download options include Use BookOrbit default and a choice of either EPUB variant. Changes apply to future acquisitions. Issued selections and interrupted bulk checkpoints retain their captured variant, including when a web default changes before resume. The `koreader-delivery-settings` web feature and `readAloudDeliverySettings` plugin capability gate the new fields and routes. Against older servers, the plugin retains its local default and explicit override compatibility. A failed or invalid settings response does not silently select a different rendition. Credentials remain unique per BookOrbit account; devices sharing them may have independent overrides. diff --git a/packages/types/src/pattern-resolver.ts b/packages/types/src/pattern-resolver.ts index 237ac4ddf..a59d2b204 100644 --- a/packages/types/src/pattern-resolver.ts +++ b/packages/types/src/pattern-resolver.ts @@ -3,7 +3,7 @@ export const DEFAULT_UPLOAD_PATTERN_BOOK_PER_FILE = export const DEFAULT_UPLOAD_PATTERN_BOOK_PER_FOLDER = "<{authors:first}|Unknown Author>/<{series}/><{seriesIndex}. ><{title}|{originalFilename}>< ({year})>/<{seriesIndex}. ><{title}|{originalFilename}>< ({year})>"; export const DEFAULT_DOWNLOAD_PATTERN = "{originalFilename}"; -export const DEFAULT_KOREADER_DEVICE_PATTERN = "<Series/{series}/|Standalone/{authors:first} - ><{seriesIndex:fixed2} - >{title}"; +export const DEFAULT_KOREADER_DEVICE_PATTERN = "<Series/{series}/|Standalone/{authors:first} - |Standalone/><{seriesIndex:fixed2} - >{title}"; export const EXAMPLE_PATTERN_METADATA: Record<string, string> = { title: "Neuromancer", @@ -143,18 +143,17 @@ export interface PlaceholderResolution { export function resolvePlaceholders(pattern: string, values: Record<string, string>): PlaceholderResolution { let usedFallback = false; let droppedOptional = false; - // Handle optional blocks with else clause: <primary|fallback> + // Optional blocks: <primary|fallback|...|default>. The first branch whose placeholders all + // fill wins; the last branch of a block with a fallback is used as-is, as a lone fallback is. const resolved = pattern.replace(/<([^<>]+)>/g, (_, blockContent: string) => { - const pipeIndex = blockContent.indexOf("|"); - const primary = pipeIndex >= 0 ? blockContent.substring(0, pipeIndex) : blockContent; - const fallback = pipeIndex >= 0 ? blockContent.substring(pipeIndex + 1) : null; + const branches = blockContent.split("|"); + const conditional = branches.length > 1 ? branches.slice(0, -1) : branches; + const filled = conditional.findIndex((branch) => checkAllPlaceholdersPresent(branch, values)); - if (checkAllPlaceholdersPresent(primary, values)) { - return resolveModifierPlaceholders(primary, values); - } - if (fallback != null) { + if (filled === 0) return resolveModifierPlaceholders(branches[0], values); + if (branches.length > 1) { usedFallback = true; - return resolveModifierPlaceholders(fallback, values); + return resolveModifierPlaceholders(filled > 0 ? branches[filled] : branches[branches.length - 1], values); } droppedOptional = true; return ""; @@ -328,6 +327,17 @@ export function resolveUploadPath(pattern: string, values: Record<string, string return limitPathSegmentBytes(ensurePathLastSegmentExtension(resolved, dotExt), dotExt); } +/** + * Appends text to the file stem of a resolved path, before its extension, keeping the last + * segment within the byte limit. A segment that has to be shortened ends in a hash of the full + * name, so two suffixed names stay distinct even when the suffix itself is cut. + */ +export function appendToPathStem(path: string, suffix: string, ext: string): string { + const dotExt = normalizeDotExt(ext); + const extension = dotExt && path.toLowerCase().endsWith(dotExt.toLowerCase()) ? path.slice(-dotExt.length) : ""; + return limitPathSegmentBytes(`${path.slice(0, path.length - extension.length)}${suffix}${extension}`, dotExt); +} + /** * Resolves a file naming pattern to a filename (no path separators). * - If the resolved pattern contains folder segments, only the last segment is used. diff --git a/server/src/modules/koreader/koreader-catalog.service.test.ts b/server/src/modules/koreader/koreader-catalog.service.test.ts index db99dceee..558d0eda6 100644 --- a/server/src/modules/koreader/koreader-catalog.service.test.ts +++ b/server/src/modules/koreader/koreader-catalog.service.test.ts @@ -1224,6 +1224,15 @@ describe('KoreaderCatalogService', () => { expect(fallback.files[0]?.devicePath).toBe('Default/Dune.epub'); }); + it('files a standalone book without an author under the default pattern by title alone', async () => { + const { service, bookService } = makeService(); + bookService.getDetail.mockResolvedValue(makeDetail({ seriesId: null, seriesName: null, seriesIndex: null, authors: [] })); + + const detail = await service.getBookDetail(makeUser(), 10); + + expect(detail.files[0]?.devicePath).toBe('Standalone/Dune.epub'); + }); + it('streams thumbnails with access checks and etags', async () => { const { service, bookService } = makeService(); const reply = makeReply(); @@ -2091,6 +2100,116 @@ describe('KoreaderCatalogService', () => { }); }); + describe('device paths for variant requests', () => { + const organization = { fileNamingPattern: '{authors}/{title}', seriesFileNamingPattern: '', standaloneFileNamingPattern: '' }; + + function detailFile(id: number, narrated: boolean, role = 'content') { + return { + id, + format: 'epub', + role, + sizeBytes: 1234, + absolutePath: `/books/dune-${id}.epub`, + createdAt: new Date('2026-01-01T00:00:00.000Z'), + filename: `dune-${id}.epub`, + durationSeconds: null, + mediaOverlay: narrated ? { available: true, durationSeconds: 100 } : null, + }; + } + + function manifestFile(id: number, narrated: boolean) { + return { + id, + format: 'epub', + mediaOverlayAvailable: narrated, + sizeBytes: 1234, + fileHash: `${id}-hash`, + filename: `dune-${id}.epub`, + contentVersion: new Date('2026-02-01T00:00:00.000Z'), + }; + } + + async function devicePaths( + variant: 'original' | 'audioless_epub' | undefined, + files: { id: number; narrated: boolean; role?: string }[], + overrides: Record<string, unknown> = {}, + deviceOrganization: typeof organization | null = organization, + ) { + const { service, opdsBookService, bookService } = makeService(deviceOrganization); + const contentFiles = files.filter((file) => file.role !== 'supplement'); + opdsBookService.getBookManifestPage.mockResolvedValueOnce({ + rows: [makeManifestRow({ ...overrides, files: contentFiles.map((file) => manifestFile(file.id, file.narrated)) })], + hasNext: false, + }); + bookService.getDetail.mockResolvedValueOnce( + makeDetail({ ...overrides, files: files.map((file) => detailFile(file.id, file.narrated, file.role)) }), + ); + + const [manifest, detail] = await Promise.all([ + service.getBulkManifest(makeUser({ id: 7 }), makeQuery({ deviceId: 'device-1', variant })), + service.getBookDetail(makeUser({ id: 7 }), 10, 'device-1', variant), + ]); + const byId = (list: { id: number; devicePath: string }[]) => Object.fromEntries(list.map((file) => [file.id, file.devicePath])); + const detailPaths = byId(detail.files); + expect(byId(manifest.items[0]!.files)).toEqual(detailPaths); + return detailPaths; + } + + it.each(['original', 'audioless_epub'] as const)('keeps the configured name for a plain EPUB requested as %s', async (variant) => { + await expect(devicePaths(variant, [{ id: 100, narrated: false }])).resolves.toEqual({ 100: 'Frank Herbert/Dune.epub' }); + }); + + it('names a single-file book exactly as a request without a variant does', async () => { + const legacy = await devicePaths(undefined, [{ id: 100, narrated: false }], {}, null); + + expect(legacy).toEqual({ 100: 'Series/Dune/01.00 - Dune.epub' }); + await expect(devicePaths('original', [{ id: 100, narrated: false }], {}, null)).resolves.toEqual(legacy); + await expect(devicePaths('audioless_epub', [{ id: 100, narrated: false }], {}, null)).resolves.toEqual(legacy); + }); + + it('keeps the configured name for a narrated original and marks only its text-only copy', async () => { + await expect(devicePaths('original', [{ id: 100, narrated: true }])).resolves.toEqual({ 100: 'Frank Herbert/Dune.epub' }); + await expect(devicePaths('audioless_epub', [{ id: 100, narrated: true }])).resolves.toEqual({ + 100: 'Frank Herbert/Dune - Text Only.epub', + }); + }); + + it('keeps the configured name for the first same-format file and marks later ones by file id', async () => { + const files = [ + { id: 101, narrated: true }, + { id: 100, narrated: false }, + ]; + + await expect(devicePaths('original', files)).resolves.toEqual({ + 100: 'Frank Herbert/Dune.epub', + 101: 'Frank Herbert/Dune [101].epub', + }); + await expect(devicePaths('audioless_epub', files)).resolves.toEqual({ + 100: 'Frank Herbert/Dune.epub', + 101: 'Frank Herbert/Dune - Text Only [101].epub', + }); + }); + + it('does not count a supplement of the same format as a sibling', async () => { + await expect( + devicePaths('original', [ + { id: 99, narrated: false, role: 'supplement' }, + { id: 100, narrated: false, role: 'primary' }, + ]), + ).resolves.toEqual({ 100: 'Frank Herbert/Dune.epub' }); + }); + + it('keeps a marked long name within the segment limit and distinct from the original', async () => { + const overrides = { title: 'Long Title '.repeat(30).trim() }; + const original = (await devicePaths('original', [{ id: 100, narrated: true }], overrides))[100]!; + const textOnly = (await devicePaths('audioless_epub', [{ id: 100, narrated: true }], overrides))[100]!; + + expect(textOnly).not.toBe(original); + expect(textOnly.endsWith('.epub')).toBe(true); + expect(Buffer.byteLength(textOnly.split('/').pop()!, 'utf8')).toBeLessThanOrEqual(255); + }); + }); + describe('streamFile', () => { const readAlongFile = { id: 100, diff --git a/server/src/modules/koreader/koreader-catalog.service.ts b/server/src/modules/koreader/koreader-catalog.service.ts index 61e9abbbe..dda36b370 100644 --- a/server/src/modules/koreader/koreader-catalog.service.ts +++ b/server/src/modules/koreader/koreader-catalog.service.ts @@ -14,7 +14,7 @@ import { } from '@nestjs/common'; import type { FastifyReply } from 'fastify'; -import { DEFAULT_KOREADER_DEVICE_PATTERN, resolveUploadPath } from '@bookorbit/types'; +import { appendToPathStem, DEFAULT_KOREADER_DEVICE_PATTERN, resolveUploadPath } from '@bookorbit/types'; import { buildPatternTokens } from '../../common/utils/pattern-tokens.utils'; import type { AchievementItem, @@ -645,7 +645,15 @@ export class KoreaderCatalogService { selection: variant && userId ? this.deliverySelection?.descriptor(userId, 'koreader', file.id, variant, file.contentVersion.toISOString()) : undefined, downloadUrl: this.downloadUrl(file.id, variant, userId, file.contentVersion.toISOString()), - devicePath: this.variantDevicePath(devicePath, downloadVariant, hasSameFormatSibling, variant ? file.id : undefined), + devicePath: variant + ? this.deliveryDevicePath( + devicePath, + extension, + downloadVariant, + file.mediaOverlayAvailable, + this.laterSiblingId(row.files, file.id, extension), + ) + : this.variantDevicePath(devicePath, downloadVariant, hasSameFormatSibling), }; }); @@ -1208,7 +1216,7 @@ export class KoreaderCatalogService { const title = detail.title ?? (basename(detail.folderPath) || `Book ${detail.id}`); const files = detail.files .filter((file) => file.role === 'primary' || file.role === 'content') - .map<KoreaderCatalogFile>((file) => { + .map<KoreaderCatalogFile>((file, _index, deliverable) => { const extension = this.normalizeFormat(file.format); const downloadVariant = extension === 'epub' && variant !== 'original' && (file.mediaOverlay?.available === true || variant === 'audioless_epub') @@ -1258,7 +1266,15 @@ export class KoreaderCatalogService { selection: variant && userId ? this.deliverySelection?.descriptor(userId, 'koreader', file.id, variant, file.contentVersion ?? '') : undefined, downloadUrl: this.downloadUrl(file.id, variant, userId, file.contentVersion ?? ''), - devicePath: this.variantDevicePath(devicePath, downloadVariant, hasSameFormatSibling, variant ? file.id : undefined), + devicePath: variant + ? this.deliveryDevicePath( + devicePath, + extension, + downloadVariant, + file.mediaOverlay?.available === true, + this.laterSiblingId(deliverable, file.id, extension), + ) + : this.variantDevicePath(devicePath, downloadVariant, hasSameFormatSibling), }; }); @@ -1323,12 +1339,31 @@ export class KoreaderCatalogService { return `${CATALOG_BASE}/files/${fileId}/download?variant=${variant}&selection=${encodeURIComponent(selection)}`; } - private variantDevicePath(path: string, variant: KoreaderCatalogFile['downloadVariant'], hasSameFormatSibling: boolean, fileId?: number): string { - if (fileId) return path.replace(/\.([^./]+)$/, ` - bo-${fileId}-${variant}.$1`); + // Naming for plugins that never request a variant. Their released builds keep these names. + private variantDevicePath(path: string, variant: KoreaderCatalogFile['downloadVariant'], hasSameFormatSibling: boolean): string { if (variant !== 'audioless_epub' || !hasSameFormatSibling) return path; return /\.epub$/i.test(path) ? path.replace(/\.epub$/i, ' - Read Along.epub') : `${path} - Read Along.epub`; } + // A variant-aware plugin keeps the configured name and renames only copies that would + // otherwise share a path: the text-only form of a narrated EPUB, and every same-format file + // after a book's first. Those carry their file id, so adding a file never renames another. + private deliveryDevicePath( + path: string, + extension: string, + variant: KoreaderCatalogFile['downloadVariant'], + narrated: boolean, + laterSiblingId: number | null, + ): string { + let named = variant === 'audioless_epub' && narrated ? appendToPathStem(path, ' - Text Only', extension) : path; + if (laterSiblingId !== null) named = appendToPathStem(named, ` [${laterSiblingId}]`, extension); + return named; + } + + private laterSiblingId(files: readonly { id: number; format: string | null }[], fileId: number, extension: string): number | null { + return files.some((candidate) => candidate.id < fileId && this.normalizeFormat(candidate.format) === extension) ? fileId : null; + } + private async buildRelatedSections( user: RequestUser, bookId: number, diff --git a/server/src/modules/upload/pattern-resolver.test.ts b/server/src/modules/upload/pattern-resolver.test.ts index d68ee1d47..36dafd6a9 100644 --- a/server/src/modules/upload/pattern-resolver.test.ts +++ b/server/src/modules/upload/pattern-resolver.test.ts @@ -1,6 +1,8 @@ import { + appendToPathStem, applyModifier, DEFAULT_DOWNLOAD_PATTERN, + DEFAULT_KOREADER_DEVICE_PATTERN, DEFAULT_UPLOAD_PATTERN_BOOK_PER_FILE, DEFAULT_UPLOAD_PATTERN_BOOK_PER_FOLDER, EXAMPLE_PATTERN_METADATA, @@ -299,6 +301,24 @@ describe('replacePlaceholders', () => { }); }); + describe('chained fallbacks <primary|fallback|default>', () => { + const pattern = '<{series}|{publisher}|Unsorted>'; + + it('uses the first branch whose tokens all fill', () => { + expect(replacePlaceholders(pattern, FULL)).toBe('Sprawl'); + expect(replacePlaceholders(pattern, { ...FULL, series: '' })).toBe('Ace Books'); + }); + + it('uses the last branch as-is when no earlier branch fills', () => { + expect(replacePlaceholders(pattern, { ...FULL, series: '', publisher: '' })).toBe('Unsorted'); + expect(replacePlaceholders('<{series}|{publisher}|{language}>', { ...FULL, series: '', publisher: '', language: '' })).toBe(''); + }); + + it('reports a chained group that fell back', () => { + expect(resolvePlaceholders(pattern, { ...FULL, series: '' })).toEqual({ text: 'Ace Books', usedFallback: true, droppedOptional: false }); + }); + }); + describe('author fallback pattern', () => { it('uses the author when present', () => { expect(replacePlaceholders('<{authors:first}|Unknown Author>', FULL)).toBe('William Gibson'); @@ -376,9 +396,20 @@ describe('resolvePlaceholders', () => { describe('KOReader device pattern', () => { it('routes series and standalone books into separate folders', () => { - const pattern = '<Series/{series}/|Standalone/{authors:first} - ><{seriesIndex:fixed2} - >{title}'; - expect(resolveUploadPath(pattern, FULL, 'epub')).toBe('Series/Sprawl/01.00 - Neuromancer.epub'); - expect(resolveUploadPath(pattern, { ...FULL, series: '', seriesIndex: '' }, 'epub')).toBe('Standalone/William Gibson - Neuromancer.epub'); + expect(resolveUploadPath(DEFAULT_KOREADER_DEVICE_PATTERN, FULL, 'epub')).toBe('Series/Sprawl/01.00 - Neuromancer.epub'); + expect(resolveUploadPath(DEFAULT_KOREADER_DEVICE_PATTERN, { ...FULL, series: '', seriesIndex: '' }, 'epub')).toBe( + 'Standalone/William Gibson - Neuromancer.epub', + ); + }); + + it('names a standalone book without an author by its title alone', () => { + expect(resolveUploadPath(DEFAULT_KOREADER_DEVICE_PATTERN, { ...FULL, series: '', seriesIndex: '', authors: '' }, 'epub')).toBe( + 'Standalone/Neuromancer.epub', + ); + }); + + it('keeps a series book without an author in its series folder', () => { + expect(resolveUploadPath(DEFAULT_KOREADER_DEVICE_PATTERN, { ...FULL, authors: '' }, 'epub')).toBe('Series/Sprawl/01.00 - Neuromancer.epub'); }); it('keeps each library in its own subtree when the pattern starts with the library token', () => { @@ -665,6 +696,27 @@ describe('path segment byte limits', () => { }); }); +describe('appendToPathStem', () => { + it('inserts the suffix before the extension, matching it case-insensitively', () => { + expect(appendToPathStem('Frank Herbert/Dune.epub', ' - Text Only', 'epub')).toBe('Frank Herbert/Dune - Text Only.epub'); + expect(appendToPathStem('Dune.EPUB', ' [101]', '.epub')).toBe('Dune [101].EPUB'); + }); + + it('appends the suffix when the path does not end in the extension', () => { + expect(appendToPathStem('Books/Dune', ' [101]', 'epub')).toBe('Books/Dune [101]'); + }); + + it('keeps a long suffixed segment within the limit and distinct from its unsuffixed form', () => { + const path = resolveUploadPath('{title}', { ...FULL, title: 'Long Title '.repeat(22).trim() }, 'epub')!; + const suffixed = appendToPathStem(path, ' - Text Only', 'epub'); + + expect(Buffer.byteLength(path, 'utf8')).toBeLessThanOrEqual(MAX_PATH_SEGMENT_BYTES); + expect(Buffer.byteLength(suffixed, 'utf8')).toBeLessThanOrEqual(MAX_PATH_SEGMENT_BYTES); + expect(suffixed.endsWith('.epub')).toBe(true); + expect(suffixed).not.toBe(path); + }); +}); + // ── validatePattern ─────────────────────────────────────────────────────────── describe('validatePattern', () => { diff --git a/server/test/read-aloud-delivery.e2e-spec.ts b/server/test/read-aloud-delivery.e2e-spec.ts index fc9e01a26..921082abd 100644 --- a/server/test/read-aloud-delivery.e2e-spec.ts +++ b/server/test/read-aloud-delivery.e2e-spec.ts @@ -37,10 +37,10 @@ describe('Read-aloud delivery contracts (e2e)', { timeout: 180_000 }, () => { headers: authHeader(owner.accessToken), ...(payload !== undefined ? { payload: payload as Record<string, unknown> } : {}), }); - async function detail(variant: string) { + async function detail(variant?: string) { const response = await ctx.app.inject({ method: 'GET', - url: `/api/v1/koreader/plugin/catalog/books/${book.bookId}?variant=${variant}`, + url: `/api/v1/koreader/plugin/catalog/books/${book.bookId}${variant ? `?variant=${variant}` : ''}`, headers: koHeaders(), }); expect(response.statusCode).toBe(200); @@ -258,7 +258,10 @@ describe('Read-aloud delivery contracts (e2e)', { timeout: 180_000 }, () => { const text = await detail('audioless_epub'); expect(original.sizeBytes).toBe(sourceBytes.length); expect(text.sizeBytes).toBeNull(); - expect(original.devicePath).not.toBe(text.devicePath); + const configured = (await detail()).devicePath; + expect(configured).toMatch(/\.epub$/); + expect(original.devicePath).toBe(configured); + expect(text.devicePath).toBe(configured.replace(/\.epub$/, ' - Text Only.epub')); const response = await acquire(original.downloadUrl); expect(response.statusCode).toBe(200); expect(response.rawPayload).toEqual(sourceBytes);