forked from akai/readest
perf(library): in-place re-import is a no-op on the same file path (#4597)
Re-importing a folder via the in-place option used to reopen, parse,
and partial-MD5 every file before its byHash entry could short-circuit
the import. Worse, the byHash short-circuit treated every hit as "user
dropped a fresh file matching a known book", so it refreshed
createdAt/updatedAt/downloadedAt and cleared filePath/deletedAt. For a
user re-scanning the same external folder, that quietly rewrote sort
order and wiped soft-delete state. And once the import returned, the
ingest pipeline still ran group / tag / upload work — including a
path-derived empty groupId that silently clobbered manual
GroupingModal assignments on every re-scan.
This change adds an explicit byFilePath fast path at the top of
`ingestFile` so a re-scan returns the existing library entry verbatim,
before any I/O AND before any downstream side effect:
byFilePath hit -> the same on-disk source is being re-scanned in
place; the right answer is no-op. Don't open the
file, don't touch any timestamps, don't re-cover,
don't run the group / tag / upload steps.
byHash hit -> a different source path resolves to a known book
(e.g. the user dropped a copy from elsewhere, or
a soft-deleted book is being revived); the
existing "refresh timestamps + clear deletedAt"
behavior in importBook is correct here and is
left intact.
Implementation:
- BookLookupIndex carries byHash / byMetaKey / byFilePath, with
byFilePath built only from non-deleted books that have an absolute
filePath. normalizeFilePathForIndex is the shared key function so
shouldImportInPlace and the index agree on case-insensitive
filesystems (macOS / iOS / Windows). osPlatform threads through
buildBookLookupIndex and BaseAppService.importBook so the renderer
and the importer compute the same key for the same file.
- ingestService.ingestFile gains a byFilePath fast path before its
importBook call: when `inPlace` was decided positive, the source
is a real on-disk path (not a PSE stream / URL / content URI),
and lookupIndex.byFilePath has a hit, return the existing Book
directly. Returning here — rather than inside importBook — is
deliberate: it skips the downstream group / tag / upload steps so
a re-scan can never silently overwrite a manual GroupingModal
assignment via a path-derived empty groupId.
- ingest-service.test.ts covers both halves: an in-place re-import
short-circuits importBook entirely (no call, existing object
returned, createdAt / updatedAt / groupId / groupName all
untouched); a copy-mode import (no external library folders) with
the same byFilePath entry still goes through importBook so dedup
falls back to byHash. import-metahash.test.ts retains the
BookLookupIndex builder test that deleted and url-backed books
are excluded from byFilePath.
Net effect: re-importing a folder of N already-imported books does
zero file opens, zero parses, zero MD5 passes, and leaves every book's
groupId / createdAt / deletedAt / cover untouched.
This commit is contained in:
@@ -648,4 +648,23 @@ describe('importBook with BookLookupIndex', () => {
|
||||
// Should reuse the existing book object via lookup index
|
||||
expect(result).toBe(existingBook);
|
||||
});
|
||||
|
||||
it('buildBookLookupIndex skips deleted and url-backed books in byFilePath', async () => {
|
||||
const inPlaceBook = makeBook({
|
||||
hash: 'a',
|
||||
filePath: '/lib/a.epub',
|
||||
});
|
||||
const deletedBook = makeBook({
|
||||
hash: 'b',
|
||||
filePath: '/lib/b.epub',
|
||||
deletedAt: Date.now(),
|
||||
});
|
||||
const urlBook = makeBook({ hash: 'c', filePath: 'https://example.com/c.epub' });
|
||||
|
||||
const lookupIndex = buildBookLookupIndex([inPlaceBook, deletedBook, urlBook], 'linux');
|
||||
|
||||
expect(lookupIndex.byFilePath.get('/lib/a.epub')).toBe(inPlaceBook);
|
||||
expect(lookupIndex.byFilePath.has('/lib/b.epub')).toBe(false);
|
||||
expect(lookupIndex.byFilePath.has('https://example.com/c.epub')).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -543,6 +543,87 @@ describe('ingestFile', () => {
|
||||
expect(importBook.mock.calls[0]?.[2]).toMatchObject({ inPlace: false });
|
||||
});
|
||||
|
||||
// ------ in-place + byFilePath fast path ------
|
||||
// ingestFile probes the byFilePath index before delegating to importBook
|
||||
// so a re-scan of an already-imported external folder skips file I/O,
|
||||
// parsing, hashing, AND every downstream side effect (group / tag / upload
|
||||
// decisions). The fast path returning early is what guarantees a manual
|
||||
// GroupingModal assignment survives a folder re-import — without it, a
|
||||
// path-derived empty groupId would clobber the existing group.
|
||||
|
||||
test('byFilePath hit short-circuits importBook entirely on in-place re-import', async () => {
|
||||
const { appService, settings, isLoggedIn, importBook } = makeDeps({
|
||||
externalLibraryFolders: ['/Users/me/Library'],
|
||||
osPlatform: 'macos',
|
||||
});
|
||||
const sourcePath = '/Users/me/Library/sample.epub';
|
||||
const existing: Book = {
|
||||
hash: 'previously-hashed',
|
||||
format: 'EPUB',
|
||||
title: 'Existing',
|
||||
author: 'Author',
|
||||
filePath: sourcePath,
|
||||
createdAt: 1000,
|
||||
updatedAt: 2000,
|
||||
groupId: 'manual',
|
||||
groupName: 'Manual/Group',
|
||||
};
|
||||
// macOS is case-insensitive, so the index key is lowercased to match
|
||||
// what `normalizeFilePathForIndex` produces in production.
|
||||
const lookupIndex = {
|
||||
byHash: new Map(),
|
||||
byMetaKey: new Map(),
|
||||
byFilePath: new Map([[sourcePath.toLowerCase(), existing]]),
|
||||
} as unknown as Parameters<typeof ingestFile>[0]['lookupIndex'];
|
||||
const book = await ingestFile(
|
||||
{
|
||||
file: sourcePath,
|
||||
books: [existing],
|
||||
lookupIndex,
|
||||
groupId: '',
|
||||
groupName: undefined,
|
||||
},
|
||||
{ appService, settings, isLoggedIn },
|
||||
);
|
||||
expect(book).toBe(existing);
|
||||
// importBook never ran, so no file I/O, no parser, no partialMD5.
|
||||
expect(importBook).not.toHaveBeenCalled();
|
||||
// Existing fields are untouched: createdAt / updatedAt / group all stay.
|
||||
expect(existing.createdAt).toBe(1000);
|
||||
expect(existing.updatedAt).toBe(2000);
|
||||
expect(existing.groupId).toBe('manual');
|
||||
expect(existing.groupName).toBe('Manual/Group');
|
||||
});
|
||||
|
||||
test('without inPlace the byFilePath index is ignored and importBook runs', async () => {
|
||||
// Fast path is gated on `inPlace`. A classic copy-mode import (no
|
||||
// external library folder configured) that happens to point at a path
|
||||
// already in the library must still go through importBook so dedup
|
||||
// falls back to byHash.
|
||||
const { appService, settings, isLoggedIn, importBook } = makeDeps();
|
||||
const sourcePath = '/Users/me/Downloads/sample.epub';
|
||||
const existing: Book = {
|
||||
hash: 'previously-hashed',
|
||||
format: 'EPUB',
|
||||
title: 'Existing',
|
||||
author: 'Author',
|
||||
filePath: sourcePath,
|
||||
createdAt: 1000,
|
||||
updatedAt: 2000,
|
||||
};
|
||||
const lookupIndex = {
|
||||
byHash: new Map(),
|
||||
byMetaKey: new Map(),
|
||||
byFilePath: new Map([[sourcePath, existing]]),
|
||||
} as unknown as Parameters<typeof ingestFile>[0]['lookupIndex'];
|
||||
await ingestFile(
|
||||
{ file: sourcePath, books: [existing], lookupIndex },
|
||||
{ appService, settings, isLoggedIn },
|
||||
);
|
||||
expect(importBook).toHaveBeenCalledTimes(1);
|
||||
expect(importBook.mock.calls[0]?.[2]).toMatchObject({ inPlace: false });
|
||||
});
|
||||
|
||||
// ------ in-place + cloud upload ------
|
||||
// In-place imports are still uploaded so the user gets backup / cross-device
|
||||
// sync. Only transient imports opt out of upload entirely. The on-the-wire
|
||||
|
||||
@@ -678,7 +678,12 @@ const LibraryPageContent = ({ searchParams }: { searchParams: ReadonlyURLSearchP
|
||||
// O(1) instead of O(n) over the existing library. importBook also keeps
|
||||
// the index updated as new books are appended, so subsequent files in
|
||||
// the same batch see the additions.
|
||||
const lookupIndex = buildBookLookupIndex(library);
|
||||
//
|
||||
// `osPlatform` is required for the `byFilePath` arm: on case-insensitive
|
||||
// filesystems (macOS / iOS / Windows) two paths that differ only in
|
||||
// casing must hash to the same key, so the in-place fast path in
|
||||
// importBook can recognize a re-import of the same file.
|
||||
const lookupIndex = buildBookLookupIndex(library, appService?.osPlatform);
|
||||
const failedImports: Array<{ filename: string; errorMessage: string }> = [];
|
||||
const successfulImports: string[] = [];
|
||||
|
||||
|
||||
@@ -261,6 +261,10 @@ export abstract class BaseAppService implements AppService {
|
||||
return BookSvc.importBook(this.fs, file, books, {
|
||||
saveBookConfig: this.saveBookConfig.bind(this),
|
||||
generateCoverImageUrl: this.generateCoverImageUrl.bind(this),
|
||||
// Pass the host platform through so the in-place fast path and the
|
||||
// lookup index can normalize source paths consistently on
|
||||
// case-insensitive filesystems (macOS / iOS / Windows).
|
||||
osPlatform: this.osPlatform,
|
||||
...options,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { SystemSettings } from '@/types/settings';
|
||||
import { FileSystem, AppPlatform, BaseDir } from '@/types/system';
|
||||
import { FileSystem, AppPlatform, BaseDir, OsPlatform } from '@/types/system';
|
||||
import {
|
||||
Book,
|
||||
BookConfig,
|
||||
@@ -44,9 +44,10 @@ import {
|
||||
type BookFileContentSource,
|
||||
} from './bookContent';
|
||||
|
||||
export function buildBookLookupIndex(books: Book[]): BookLookupIndex {
|
||||
export function buildBookLookupIndex(books: Book[], osPlatform?: OsPlatform): BookLookupIndex {
|
||||
const byHash = new Map<string, Book>();
|
||||
const byMetaKey = new Map<string, Book[]>();
|
||||
const byFilePath = new Map<string, Book>();
|
||||
for (const book of books) {
|
||||
byHash.set(book.hash, book);
|
||||
if (book.metaHash && !book.deletedAt) {
|
||||
@@ -55,8 +56,38 @@ export function buildBookLookupIndex(books: Book[]): BookLookupIndex {
|
||||
if (list) list.push(book);
|
||||
else byMetaKey.set(key, [book]);
|
||||
}
|
||||
// In-place books carry the absolute source path on `filePath` (set by
|
||||
// importBook below). Indexing them here lets a re-import of the exact
|
||||
// same file short-circuit before touching disk or computing partialMD5.
|
||||
// Skip URL-backed entries (remote books) and tombstoned ones.
|
||||
if (book.filePath && !isValidURL(book.filePath) && !book.deletedAt) {
|
||||
const key = normalizeFilePathForIndex(book.filePath, osPlatform);
|
||||
if (key) byFilePath.set(key, book);
|
||||
}
|
||||
}
|
||||
return { byHash, byMetaKey };
|
||||
return { byHash, byMetaKey, byFilePath };
|
||||
}
|
||||
|
||||
/**
|
||||
* Normalize an absolute file path into a stable map key for `byFilePath`.
|
||||
*
|
||||
* Mirrors the same rules `ingestService.shouldImportInPlace` uses to compare
|
||||
* paths against the user's in-place roots so both sides agree on whether a
|
||||
* given source file matches a previously-indexed book:
|
||||
* - Backslashes are normalized to `/`.
|
||||
* - Trailing slashes are stripped.
|
||||
* - On case-insensitive filesystems (macOS / iOS / Windows) the key is
|
||||
* lowercased. Linux / Android keep the original casing.
|
||||
*
|
||||
* Returns an empty string for non-string / falsy input so callers can do a
|
||||
* `if (key) map.set(key, …)` guard without an extra null check.
|
||||
*/
|
||||
export function normalizeFilePathForIndex(path: string, osPlatform?: OsPlatform): string {
|
||||
if (!path) return '';
|
||||
const caseInsensitive =
|
||||
osPlatform === 'macos' || osPlatform === 'ios' || osPlatform === 'windows';
|
||||
const n = path.replace(/\\/g, '/').replace(/\/+$/, '');
|
||||
return caseInsensitive ? n.toLowerCase() : n;
|
||||
}
|
||||
|
||||
export interface CoverContext {
|
||||
@@ -223,6 +254,14 @@ export async function mergeBooks(
|
||||
export interface ImportBookInternalOptions extends ImportBookOptions {
|
||||
saveBookConfig: (book: Book, config: BookConfig) => Promise<void>;
|
||||
generateCoverImageUrl: (book: Book) => Promise<string>;
|
||||
/**
|
||||
* Used by the in-place fast path to normalize the source path the same way
|
||||
* `buildBookLookupIndex` does. Optional: when omitted the fast path falls
|
||||
* back to a case-sensitive comparison, which is still safe (it will simply
|
||||
* miss matches that differ only in casing on macOS / iOS / Windows and the
|
||||
* import will proceed down the slow path as before).
|
||||
*/
|
||||
osPlatform?: OsPlatform;
|
||||
}
|
||||
|
||||
export async function importBook(
|
||||
@@ -246,8 +285,10 @@ export async function importBook(
|
||||
transient = false,
|
||||
inPlace = false,
|
||||
lookupIndex,
|
||||
osPlatform,
|
||||
} = options;
|
||||
const isPseStream = typeof file === 'string' && isPseStreamFileName(file);
|
||||
|
||||
try {
|
||||
let loadedBook: BookDoc;
|
||||
let format: BookFormat;
|
||||
@@ -530,6 +571,18 @@ export async function importBook(
|
||||
if (existingBook) existingBook.filePath = file;
|
||||
}
|
||||
}
|
||||
// Now that `filePath` is set, keep the path index in sync so later files
|
||||
// in the same batch (and the next call into importBook) can hit the
|
||||
// in-place fast path. Only persistent in-place imports go into the
|
||||
// index — transient previews are short-lived and never persisted to
|
||||
// the library so indexing them would just leak references.
|
||||
if (lookupIndex && inPlace && !transient && typeof file === 'string') {
|
||||
const indexedBook = existingBook || book;
|
||||
if (indexedBook.filePath) {
|
||||
const key = normalizeFilePathForIndex(indexedBook.filePath, osPlatform);
|
||||
if (key) lookupIndex.byFilePath.set(key, indexedBook);
|
||||
}
|
||||
}
|
||||
book.coverImageUrl = await generateCoverImageUrlFn(book);
|
||||
const f = file as ClosableFile;
|
||||
if (f && f.close) {
|
||||
|
||||
@@ -2,6 +2,9 @@ import type { Book, BookLookupIndex } from '@/types/book';
|
||||
import type { AppService, OsPlatform } from '@/types/system';
|
||||
import type { SystemSettings } from '@/types/settings';
|
||||
import { transferManager } from '@/services/transferManager';
|
||||
import { normalizeFilePathForIndex } from '@/services/bookService';
|
||||
import { isContentURI, isValidURL } from '@/utils/misc';
|
||||
import { isPseStreamFileName } from '@/services/opds/pseStream';
|
||||
|
||||
export interface IngestFileDeps {
|
||||
appService: AppService;
|
||||
@@ -102,12 +105,13 @@ function shouldImportInPlace(
|
||||
// case-sensitive and stay strict. We do not attempt unicode normalization
|
||||
// (NFC/NFD) — APFS handles that at the FS layer and `toLocaleLowerCase`
|
||||
// with the wrong locale would introduce its own bugs (e.g. Turkish `İ`).
|
||||
const caseInsensitive =
|
||||
osPlatform === 'macos' || osPlatform === 'ios' || osPlatform === 'windows';
|
||||
const norm = (p: string) => {
|
||||
const n = p.replace(/\\/g, '/').replace(/\/+$/, '');
|
||||
return caseInsensitive ? n.toLowerCase() : n;
|
||||
};
|
||||
//
|
||||
// Defer the actual canonicalization to `normalizeFilePathForIndex` so the
|
||||
// path index (`BookLookupIndex.byFilePath`) and this in-place decision
|
||||
// agree on what counts as the same path — otherwise a re-import could
|
||||
// hit the in-place branch here but miss the fast-path dedup in
|
||||
// importBook (or vice versa).
|
||||
const norm = (p: string) => normalizeFilePathForIndex(p, osPlatform);
|
||||
const target = norm(file);
|
||||
|
||||
// If the file already lives inside Readest's own managed books directory
|
||||
@@ -160,6 +164,43 @@ export async function ingestFile(
|
||||
appBooksPrefix ?? null,
|
||||
);
|
||||
|
||||
// In-place re-import fast path. When the source file lives under one of
|
||||
// the user's registered external library folders and the byFilePath index
|
||||
// already knows about it, skip importBook entirely and return the existing
|
||||
// library entry verbatim. No fs.openFile, no native parser, no partialMD5,
|
||||
// no timestamp / cover / config writes — and crucially no downstream group
|
||||
// / tag / upload logic, so a re-scan can't silently rewrite library sort
|
||||
// order or clobber a manual GroupingModal assignment via a path-derived
|
||||
// group string.
|
||||
//
|
||||
// This is intentionally separate from importBook's byHash / byMetaKey
|
||||
// dedup: a byHash hit means a *different* source path resolves to a known
|
||||
// book (drop a copy from elsewhere, or revive a soft-deleted entry), which
|
||||
// correctly clears `deletedAt` and refreshes timestamps. A byFilePath hit
|
||||
// is the same on-disk file at the same path — there is nothing to refresh,
|
||||
// and refreshing would silently rewrite library sort order on every
|
||||
// re-scan. Soft-deleted books are excluded from `byFilePath` at index
|
||||
// build time so they fall through to byHash and get resurrected.
|
||||
//
|
||||
// Reject URLs, content URIs and PSE streams defensively. The byFilePath
|
||||
// index only carries real on-disk paths, but `inPlace` could in principle
|
||||
// be set on a non-path source by a buggy caller.
|
||||
if (
|
||||
inPlace &&
|
||||
!opts.transient &&
|
||||
opts.lookupIndex &&
|
||||
typeof opts.file === 'string' &&
|
||||
!isPseStreamFileName(opts.file) &&
|
||||
!isValidURL(opts.file) &&
|
||||
!isContentURI(opts.file)
|
||||
) {
|
||||
const key = normalizeFilePathForIndex(opts.file, appService.osPlatform);
|
||||
const existing = key ? opts.lookupIndex.byFilePath.get(key) : undefined;
|
||||
if (existing) {
|
||||
return existing;
|
||||
}
|
||||
}
|
||||
|
||||
const book = await appService.importBook(opts.file, opts.books, {
|
||||
lookupIndex: opts.lookupIndex,
|
||||
transient: opts.transient,
|
||||
|
||||
@@ -45,6 +45,13 @@ export const FIXED_LAYOUT_FORMATS: Set<BookFormat> = new Set(['PDF', 'CBZ']);
|
||||
export interface BookLookupIndex {
|
||||
byHash: Map<string, Book>;
|
||||
byMetaKey: Map<string, Book[]>; // key = `${metaHash}:${format}`
|
||||
// Maps normalized absolute source path -> Book for in-place imports.
|
||||
// Lets the importer recognize "I already have this exact file" without
|
||||
// having to open, parse, and hash it again. Only books with a non-empty
|
||||
// `filePath` and `!deletedAt` are indexed. The key is produced by
|
||||
// `normalizeFilePathForIndex` so callers must use the same helper to
|
||||
// probe; importBook handles that internally.
|
||||
byFilePath: Map<string, Book>;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user