diff --git a/apps/readest-app/src/__tests__/services/cloud-service.test.ts b/apps/readest-app/src/__tests__/services/cloud-service.test.ts index 10909f7b..6556cc6c 100644 --- a/apps/readest-app/src/__tests__/services/cloud-service.test.ts +++ b/apps/readest-app/src/__tests__/services/cloud-service.test.ts @@ -216,7 +216,7 @@ describe('cloudService', () => { expect(book.uploadedAt).toBe(1000); }); - test('removes the in-place source file and the sidecar dir for in-place books', async () => { + test('wipes the sidecar dir but NEVER the in-place source file', async () => { const book = createMockBook({ filePath: '/Users/me/Library/sample.epub' }); vi.mocked(mockFs.exists).mockImplementation(async (path, base) => { if (base === 'None' && path === book.filePath) return true; @@ -226,9 +226,11 @@ describe('cloudService', () => { await deleteBook(mockFs, book, 'purge'); - // External source file (outside Books//) is removed... - expect(mockFs.removeFile).toHaveBeenCalledWith('/Users/me/Library/sample.epub', 'None'); - // ...and the metadata sidecar directory is wiped too. + // The user's original "read in place" file lives outside Books// + // and must be left untouched — deleting a book from Readest never + // removes the user's source file. + expect(mockFs.removeFile).not.toHaveBeenCalledWith('/Users/me/Library/sample.epub', 'None'); + // ...but the app-generated metadata sidecar directory is still wiped. expect(mockFs.removeDir).toHaveBeenCalledWith(book.hash, 'Books', true); }); @@ -287,12 +289,14 @@ describe('cloudService', () => { }); }); - // In-place imports keep their content at a user-controlled location - // (book.filePath, base 'None') rather than under Books//. For - // 'local'/'both' deletes that source file IS the local copy and gets - // removed (symmetric with deleting Books//.epub for a - // normal book). The cloud upload path is shared, so cross-device sync - // can still pull the book back. + // In-place imports ("Read books in place") keep their content at a + // user-controlled location (book.filePath, base 'None') OUTSIDE Readest's + // Books/<hash>/ dir — Readest never copied it. Deleting such a book from + // Readest must NEVER remove that source file; only the app-generated + // sidecars (cover.png, config.json, ...) under Books/<hash>/ are ours to + // delete. The cloud upload path is shared, so cross-device sync can still + // pull the book back. (Regression: in-place delete used to wipe the + // user's originals.) describe('in-place (book.filePath set)', () => { const mockInPlaceExists = (book: Book, coverExists = true) => { vi.mocked(mockFs.exists).mockImplementation(async (path, base) => { @@ -302,26 +306,24 @@ describe('cloudService', () => { }); }; - test('local action removes the user-controlled source file', async () => { + test('local action does NOT remove the user-controlled source file', async () => { const book = createMockBook({ filePath: '/Users/me/Library/sample.epub' }); mockInPlaceExists(book); await deleteBook(mockFs, book, 'local'); - // The source file is read from base 'None' (absolute path), not Books/. - expect(mockFs.removeFile).toHaveBeenCalledWith('/Users/me/Library/sample.epub', 'None'); + // The external source (base 'None', absolute path) is the user's own + // file and must survive a Readest-side delete. + expect(mockFs.removeFile).not.toHaveBeenCalledWith('/Users/me/Library/sample.epub', 'None'); }); - test('local action does not remove Books/<hash>/<title>.epub', async () => { + test('local action removes no files at all (nothing managed to delete)', async () => { const book = createMockBook({ filePath: '/Users/me/Library/sample.epub' }); mockInPlaceExists(book); await deleteBook(mockFs, book, 'local'); - // The hash-copy path lives only on a normal book; for an in-place book, - // the resolver can probe it, but deletion must target the external source. - expect(mockFs.removeFile).not.toHaveBeenCalledWith( - `${book.hash}/${book.title}.epub`, - 'Books', - ); + // There is no managed Books/<hash>/<title>.epub copy for an in-place + // book, and the external source is off-limits, so nothing is removed. + expect(mockFs.removeFile).not.toHaveBeenCalled(); }); test('local action still clears downloadedAt', async () => { @@ -335,7 +337,6 @@ describe('cloudService', () => { }); test('local action does not throw when the source file is missing', async () => { - // exists() returns false → no removeFile call, but no error either. vi.mocked(mockFs.exists).mockResolvedValue(false); const book = createMockBook({ filePath: '/Users/me/Library/sample.epub', @@ -347,21 +348,7 @@ describe('cloudService', () => { expect(book.downloadedAt).toBeNull(); }); - test('local action swallows errors from removeFile (best-effort source delete)', async () => { - vi.mocked(mockFs.removeFile).mockRejectedValueOnce(new Error('EPERM')); - const book = createMockBook({ - filePath: '/Users/me/Library/sample.epub', - downloadedAt: 12345, - }); - mockInPlaceExists(book); - - // Must not throw, and must still flip the metadata bit so the UI - // reflects the user's delete intent. - await deleteBook(mockFs, book, 'local'); - expect(book.downloadedAt).toBeNull(); - }); - - test('both action removes both the source file and the cover sidecar', async () => { + test('both action removes the cover sidecar but NEVER the source file', async () => { const book = createMockBook({ filePath: '/Users/me/Library/sample.epub', uploadedAt: null, @@ -369,11 +356,11 @@ describe('cloudService', () => { mockInPlaceExists(book); await deleteBook(mockFs, book, 'both'); - // Source file under user-controlled path: - expect(mockFs.removeFile).toHaveBeenCalledWith('/Users/me/Library/sample.epub', 'None'); - // Cover sidecar under Books/<hash>/: + // Cover sidecar under Books/<hash>/ is app-generated → removable: expect(mockFs.removeFile).toHaveBeenCalledWith(`${book.hash}/cover.png`, 'Books'); - // We must never poke at Books/<hash>/<title>.epub for an in-place book. + // The user's source file is NEVER removed: + expect(mockFs.removeFile).not.toHaveBeenCalledWith('/Users/me/Library/sample.epub', 'None'); + // And there is no managed copy to poke at for an in-place book. expect(mockFs.removeFile).not.toHaveBeenCalledWith( `${book.hash}/${book.title}.epub`, 'Books', diff --git a/apps/readest-app/src/app/library/components/ImportFromFolderDialog.tsx b/apps/readest-app/src/app/library/components/ImportFromFolderDialog.tsx index 55e03668..4a8c79fb 100644 --- a/apps/readest-app/src/app/library/components/ImportFromFolderDialog.tsx +++ b/apps/readest-app/src/app/library/components/ImportFromFolderDialog.tsx @@ -62,11 +62,11 @@ export interface ImportFromFolderResult { * (`settings.externalLibraryFolders`) and import its books in place * — Readest will read each file straight from its original location * instead of copying it into Books/<hash>/. Sidecars (cover, config, - * notes) still live in Readest's data dir, so deleting the local - * copy of a book on Readest's side will physically remove the - * source file under the registered folder. Defaults to `false`, - * which keeps the legacy "copy into Readest" behaviour and leaves - * the registered folder list untouched. + * notes) still live in Readest's data dir. Deleting such a book from + * Readest only removes those Readest-side sidecars; the original + * source file under the registered folder is left untouched. + * Defaults to `false`, which keeps the legacy "copy into Readest" + * behaviour and leaves the registered folder list untouched. */ readInPlace: boolean; } diff --git a/apps/readest-app/src/services/cloudService.ts b/apps/readest-app/src/services/cloudService.ts index 332c2639..6791a23b 100644 --- a/apps/readest-app/src/services/cloudService.ts +++ b/apps/readest-app/src/services/cloudService.ts @@ -26,17 +26,13 @@ export async function deleteBook( ): Promise<void> { if (deleteAction === 'local' || deleteAction === 'both' || deleteAction === 'purge') { const source = await resolveBookContentSource(fs, book); - if (source.kind === 'external') { - try { - if (await fs.exists(source.path, source.base)) { - await fs.removeFile(source.path, source.base); - } - } catch (error) { - // Best effort: a missing/permission-denied source shouldn't block - // the metadata-side bookkeeping that follows. - console.log('Failed to remove in-place source file:', error); - } - } else if (source.kind === 'managed' && deleteAction !== 'purge') { + // Only remove files Readest itself created. A 'managed' source lives under + // our Books/<hash>/ dir (a copy we made on import), so it is ours to delete. + // An 'external' source is the user's own file at a user-controlled location + // (book.filePath, base 'None') — e.g. a "Read books in place" import or a + // transiently-opened file. Deleting a book from Readest must NEVER remove + // that source file; doing so silently destroyed users' originals. + if (source.kind === 'managed' && deleteAction !== 'purge') { // Purge wipes the whole directory below, so skip the per-file removal. if (await fs.exists(source.path, source.base)) { await fs.removeFile(source.path, source.base); @@ -46,8 +42,8 @@ export async function deleteBook( // Purge erases the entire app-generated Books/<hash>/ directory — the // managed book file, cover.png, and (the reason for issue #4615) // config.json (reading progress, notes, bookmarks) + nav.json that the - // other delete actions leave behind. For in-place books the external - // source file was already removed above; this clears the sidecar dir. + // other delete actions leave behind. In-place books keep their external + // source file untouched; this only clears Readest's own sidecar dir. if (deleteAction === 'purge') { const dir = getDir(book); if (await fs.exists(dir, 'Books')) {