From 92028648465c7dd3b9dee2728c2e4e31930e7b65 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Mon, 6 Jul 2026 01:30:32 +0900 Subject: [PATCH] fix: real fix for library-save storage-permission crash + narrowed view-transition filter (#4943) * fix(library): request storage permission when saving to a custom folder On Android a custom library folder on shared storage needs All Files Access. The import/settings/migrate paths request it, but the library-save path (updateBook/updateBooks -> saveLibraryBooks) did not, so on a device without the permission every save failed with EACCES; because callers (sync, imports) don't await/catch it, it surfaced as an unhandled-rejection crash (Sentry READEST-A). saveLibraryBooks now catches a storage-permission error, requests the permission through the existing flow, and retries once. It prompts at most once per session so background saves don't repeatedly open system settings; a still-denied save is logged rather than crashing (the user was already shown the All Files Access screen). Fixes READEST-A Co-Authored-By: Claude Opus 4.8 (1M context) * fix(sentry): drop only the benign hidden-tab View Transition error The View Transition API skips a transition when the tab is hidden; that unhandled rejection is expected browser behavior and pure noise, so it is dropped in before_send. A transition timeout abort (READEST-9) is NOT dropped: a slow DOM update can signal a real performance problem, so it stays visible. Fixes READEST-7 Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- apps/readest-app/src-tauri/src/lib.rs | 9 +++ .../src-tauri/src/sentry_config.rs | 29 ++++++- .../__tests__/services/app-service.test.ts | 75 +++++++++++++++++++ .../src/__tests__/utils/permission.test.ts | 23 ++++++ apps/readest-app/src/services/appService.ts | 27 ++++++- apps/readest-app/src/utils/permission.ts | 10 +++ 6 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 apps/readest-app/src/__tests__/utils/permission.test.ts diff --git a/apps/readest-app/src-tauri/src/lib.rs b/apps/readest-app/src-tauri/src/lib.rs index bfbf3c27..6ab1481c 100644 --- a/apps/readest-app/src-tauri/src/lib.rs +++ b/apps/readest-app/src-tauri/src/lib.rs @@ -321,6 +321,15 @@ pub fn run() { // the OS as "Linux"; relabel it "Android" (and recover the Android // version from the kernel string) so events group correctly. before_send: Some(std::sync::Arc::new(|mut event| { + // Drop known-benign browser noise (e.g. View Transition + // skipped/aborted) before it is reported. + if event.exception.values.iter().any(|ex| { + ex.value + .as_deref() + .is_some_and(sentry_config::is_ignored_browser_error) + }) { + return None; + } if let Some(sentry::protocol::Context::Os(os)) = event.contexts.get_mut("os") { if let Some(name) = sentry_config::corrected_os_name( std::env::consts::OS, diff --git a/apps/readest-app/src-tauri/src/sentry_config.rs b/apps/readest-app/src-tauri/src/sentry_config.rs index cd794eb8..285c6ad6 100644 --- a/apps/readest-app/src-tauri/src/sentry_config.rs +++ b/apps/readest-app/src-tauri/src/sentry_config.rs @@ -76,6 +76,15 @@ pub fn android_version_from_uname(release: &str) -> Option { } } +/// A known-benign browser error that is expected behavior, not an app bug, and +/// only adds noise to crash reporting: the View Transition API skips a +/// transition when the tab is hidden. Matched on the exception value so it is +/// dropped in `before_send`. NOTE: a transition *timeout* abort is deliberately +/// NOT ignored — a slow DOM update can signal a real performance problem. +pub fn is_ignored_browser_error(value: &str) -> bool { + value.contains("View transition was skipped because document visibility state is hidden") +} + /// C-ABI accessor for the compile-time Sentry DSN, used by the iOS native /// bootstrap (sentry-cocoa) so it starts with the same DSN as the Rust client /// without a second env read or fragile Info.plist / preprocessor plumbing. @@ -97,7 +106,7 @@ pub extern "C" fn readest_sentry_dsn() -> *const std::os::raw::c_char { mod tests { use super::{ android_version_from_uname, corrected_os_name, dsn_from_env, environment_for_version, - release_name, + is_ignored_browser_error, release_name, }; #[test] @@ -165,4 +174,22 @@ mod tests { assert_eq!(android_version_from_uname(""), None); assert_eq!(android_version_from_uname("6.1.162-androidX"), None); } + + #[test] + fn ignores_benign_hidden_view_transition_error() { + assert!(is_ignored_browser_error( + "InvalidStateError: View transition was skipped because document visibility state is hidden." + )); + } + + #[test] + fn keeps_view_transition_timeout_and_real_errors() { + // A transition timeout can signal a real perf problem — do NOT suppress it. + assert!(!is_ignored_browser_error( + "TimeoutError: Transition was aborted because of timeout in DOM update" + )); + assert!(!is_ignored_browser_error("TypeError: Load failed")); + assert!(!is_ignored_browser_error("concurrent use forbidden")); + assert!(!is_ignored_browser_error("")); + } } diff --git a/apps/readest-app/src/__tests__/services/app-service.test.ts b/apps/readest-app/src/__tests__/services/app-service.test.ts index 3ee139f9..79f0c4a4 100644 --- a/apps/readest-app/src/__tests__/services/app-service.test.ts +++ b/apps/readest-app/src/__tests__/services/app-service.test.ts @@ -67,9 +67,20 @@ vi.mock('@/utils/misc', async (importOriginal) => { }; }); +// Keep the real isStoragePermissionError; only stub the interactive request. +vi.mock('@/utils/permission', async (importOriginal) => { + const original = await importOriginal>(); + return { + ...original, + requestStoragePermission: vi.fn(), + }; +}); + import { BaseAppService } from '@/services/appService'; import * as Settings from '@/services/settingsService'; import * as BookSvc from '@/services/bookService'; +import * as LibrarySvc from '@/services/libraryService'; +import { requestStoragePermission } from '@/utils/permission'; // Concrete test implementation of BaseAppService class TestAppService extends BaseAppService { @@ -193,6 +204,70 @@ describe('BaseAppService', () => { }); }); + describe('saveLibraryBooks storage permission (READEST-A)', () => { + // clearAllMocks resets call history but not implementations, so restore the + // shared saveLibraryBooks mock's default after these override it. + afterEach(() => { + vi.mocked(LibrarySvc.saveLibraryBooks).mockReset().mockResolvedValue(undefined); + vi.mocked(requestStoragePermission).mockReset(); + }); + + const permError = () => + new Error( + 'Failed to save library.json: failed to open file at path: ' + + '/storage/emulated/0/Readest/Books/library.json.bak with error: ' + + 'Permission denied (os error 13)', + ); + + test('on Android, requests storage permission and retries once when granted', async () => { + service.isAndroidApp = true; + vi.mocked(LibrarySvc.saveLibraryBooks) + .mockRejectedValueOnce(permError()) + .mockResolvedValueOnce(undefined); + vi.mocked(requestStoragePermission).mockResolvedValue(true); + + await expect(service.saveLibraryBooks([])).resolves.toBeUndefined(); + expect(requestStoragePermission).toHaveBeenCalledTimes(1); + expect(LibrarySvc.saveLibraryBooks).toHaveBeenCalledTimes(2); + }); + + test('on Android, does not crash when permission is denied', async () => { + service.isAndroidApp = true; + vi.mocked(LibrarySvc.saveLibraryBooks).mockRejectedValue(permError()); + vi.mocked(requestStoragePermission).mockResolvedValue(false); + + await expect(service.saveLibraryBooks([])).resolves.toBeUndefined(); + // No retry when the permission was declined. + expect(LibrarySvc.saveLibraryBooks).toHaveBeenCalledTimes(1); + }); + + test('only prompts once per session across repeated failing saves', async () => { + service.isAndroidApp = true; + vi.mocked(LibrarySvc.saveLibraryBooks).mockRejectedValue(permError()); + vi.mocked(requestStoragePermission).mockResolvedValue(false); + + await service.saveLibraryBooks([]); + await service.saveLibraryBooks([]); + expect(requestStoragePermission).toHaveBeenCalledTimes(1); + }); + + test('re-throws non-permission errors', async () => { + service.isAndroidApp = true; + vi.mocked(LibrarySvc.saveLibraryBooks).mockRejectedValue(new Error('disk full')); + + await expect(service.saveLibraryBooks([])).rejects.toThrow('disk full'); + expect(requestStoragePermission).not.toHaveBeenCalled(); + }); + + test('does not intercept on non-Android platforms', async () => { + service.isAndroidApp = false; + vi.mocked(LibrarySvc.saveLibraryBooks).mockRejectedValue(permError()); + + await expect(service.saveLibraryBooks([])).rejects.toThrow('Permission denied'); + expect(requestStoragePermission).not.toHaveBeenCalled(); + }); + }); + describe('file operations', () => { test('openFile delegates to fs', async () => { await service.openFile('test.epub', 'Books'); diff --git a/apps/readest-app/src/__tests__/utils/permission.test.ts b/apps/readest-app/src/__tests__/utils/permission.test.ts new file mode 100644 index 00000000..f098e917 --- /dev/null +++ b/apps/readest-app/src/__tests__/utils/permission.test.ts @@ -0,0 +1,23 @@ +import { describe, test, expect } from 'vitest'; +import { isStoragePermissionError } from '@/utils/permission'; + +describe('isStoragePermissionError', () => { + test('detects Android EACCES / permission-denied save failures', () => { + expect( + isStoragePermissionError( + new Error( + 'Failed to save library.json: failed to open file at path: ' + + '/storage/emulated/0/Readest/Books/library.json.bak with error: ' + + 'Permission denied (os error 13)', + ), + ), + ).toBe(true); + expect(isStoragePermissionError('EACCES: permission denied')).toBe(true); + }); + + test('ignores unrelated errors', () => { + expect(isStoragePermissionError(new Error('disk full'))).toBe(false); + expect(isStoragePermissionError(new Error('JSON parse error'))).toBe(false); + expect(isStoragePermissionError(undefined)).toBe(false); + }); +}); diff --git a/apps/readest-app/src/services/appService.ts b/apps/readest-app/src/services/appService.ts index be2a4993..5482e617 100644 --- a/apps/readest-app/src/services/appService.ts +++ b/apps/readest-app/src/services/appService.ts @@ -19,6 +19,7 @@ import type { BookNav } from '@/services/nav'; import { getLibraryFilename, getLibraryBackupFilename } from '@/utils/book'; import { getOSPlatform } from '@/utils/misc'; +import { isStoragePermissionError, requestStoragePermission } from '@/utils/permission'; import { ProgressHandler } from '@/utils/transfer'; import { CustomTextureInfo } from '@/styles/textures'; import { CustomFont, CustomFontInfo } from '@/styles/fonts'; @@ -441,7 +442,31 @@ export abstract class BaseAppService implements AppService { return LibrarySvc.loadLibraryBooks(this.fs, this.generateCoverImageUrl.bind(this)); } + // Prompt for storage permission at most once per session (see saveLibraryBooks). + private storagePermissionRequested = false; + async saveLibraryBooks(books: Book[], options?: SaveLibraryBooksOptions): Promise { - return LibrarySvc.saveLibraryBooks(this.fs, books, options); + try { + return await LibrarySvc.saveLibraryBooks(this.fs, books, options); + } catch (error) { + // A custom library folder on Android shared storage needs All Files + // Access. Without it the write fails with EACCES and, because callers + // (sync, imports) don't await/catch this, it surfaced as an unhandled + // rejection crash (Sentry READEST-A). Re-request the permission through + // the same flow used at import time and retry once. Only prompt once per + // session so background saves don't repeatedly yank the user to system + // settings; after that a still-denied save is logged, not crashed — + // the user was already shown the All Files Access screen. + if (!this.isAndroidApp || !isStoragePermissionError(error)) { + throw error; + } + if (!this.storagePermissionRequested) { + this.storagePermissionRequested = true; + if (await requestStoragePermission()) { + return await LibrarySvc.saveLibraryBooks(this.fs, books, options); + } + } + console.warn('[library] not saved: storage permission not granted', error); + } } } diff --git a/apps/readest-app/src/utils/permission.ts b/apps/readest-app/src/utils/permission.ts index 89fc2fa9..c9ea4050 100644 --- a/apps/readest-app/src/utils/permission.ts +++ b/apps/readest-app/src/utils/permission.ts @@ -4,6 +4,16 @@ interface Permissions { manageStorage: PermissionState; } +/** + * Whether a thrown error is an Android storage-permission denial (EACCES). A + * custom library folder on shared storage needs All Files Access; without it + * file writes fail with "Permission denied (os error 13)". + */ +export const isStoragePermissionError = (error: unknown): boolean => { + const message = error instanceof Error ? error.message : String(error); + return /os error 13|permission denied|eacces/i.test(message); +}; + export const requestStoragePermission = async (): Promise => { let permission = await invoke('plugin:native-bridge|checkPermissions'); if (permission.manageStorage !== 'granted') {